Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat(rule mail collector): add criterion for quarantine released #18943

Open
wants to merge 5 commits into
base: 10.0/bugfixes
Choose a base branch
from

Conversation

Rom1-B
Copy link
Contributor

@Rom1-B Rom1-B commented Feb 7, 2025

Checklist before requesting a review

Please delete options that are not relevant.

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.
  • This change requires a documentation update.

Description

  • It fixes !36253
  • Here is a brief description of what this PR does

When an email is quarantined by Microsoft and then released by an admin, GLPI considers it as an automated email and blocks it using the default rule:

image

This PR adds a criterion to the default rule to identify that an email has been released from quarantine by modifying the rule as follows:

image

However, since this criterion is specific to Microsoft, the default rule is not modified. Users must add it manually if needed.

@trasher
Copy link
Contributor

trasher commented Feb 7, 2025

This probably requires a test case

@Rom1-B Rom1-B changed the base branch from main to 10.0/bugfixes February 7, 2025 10:39
Copy link
Contributor

@trasher trasher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reverted the change in src/RuleMailCollector.php (then ran db:install), tests are still green. I'd expect they fail, should'nt they?

@Rom1-B
Copy link
Contributor Author

Rom1-B commented Feb 10, 2025

I've reverted the change in src/RuleMailCollector.php (then ran db:install), tests are still green. I'd expect they fail, should'nt they?

In src/RuleMailCollector.php, it's just the list of criteria that can be added from the interface, but in fact nothing blocks rules using an unlisted criterion. My test checks the behaviour of the rule, but not whether or not the criterion is available in the interface. Should we add a block of this kind?

Copy link
Member

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it could be possible to pass all headers in a _headers to the input in the RuleMailCollectorCollection::prepareInputDataForProcess() method, add a _headers criterion, and adapt the RuleCriteria::match() method to handle array of values in the different pattern types.

@@ -110,6 +110,12 @@ public function getCriterias()
$criterias['x-uce-status']['table'] = '';
$criterias['x-uce-status']['type'] = 'text';

$criterias['_headers'] = [
'name' => __('Full email headers'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this would be more precise:

Suggested change
'name' => __('Full email headers'),
'name' => __('Entire email header string'),

@cconard96 What would be the most relevant text here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just Headers would be fine IMO.

If headers contains XXX then ...

@cedric-anne cedric-anne requested a review from trasher February 10, 2025 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants