-
Notifications
You must be signed in to change notification settings - Fork 201
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
EXO 2.7 Rego Bug Fix | Only Enabled Correctly Configured Rules Pass #130
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I tested this by disabling the needed mail flow rule. On main 2.7, incorrectly passes but on this branch, it correctly displays a failure. After re-enabling it, it correctly showed a pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested out the changes using our G5 test tenant. There I noticed that the updates did cover cases where one or more rules were present but disabled. Those worked correctly. However, the implementation notes and admin center showed each rule also has a Mode
setting which can be set to one of the following: Enforce
,Audit
, or AuditAndNotify
. The latter will flag such message, but not actually enforce the baseline policy item. As such, I recommend checking each rules mode and only considering those set to Enforce
as properly meeting the intent of the rule since the baseline includes setting the mode to Enforce
in the implementation guidance.
I've made code suggestions to both the provider and unit tests that should address this. I actually tested the provider change, but didn't validate the suggested unit test changes.
Let me know what you think about the proposed changes or reach out if you wish to discuss further.
I have pushed changes to the Rego and the Unit Testing so now it requires rules to be both Enforced and Enabled. @schrolla How does it look now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updates address the additional use cases identified before. Minor change requested to update error text for clarity. Update required in test and associated unit tests. I tried to note suggestions for each, but there may be additional unit tests to update. Fix and validate unit tests pass to ensure all have been updated as needed.
Error messages have been updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-ran tests and shows updated error message when set to fail. All unit tests also pass. Looks good for merge to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge ready
π£ Description
π Motivation and context
π§ͺ Testing
β Pre-approval checklist
in code comments.
to reflect the changes in this PR.
β Pre-merge checklist
β Post-merge checklist