Conversation
|
New Issues (3)Checkmarx found the following issues in this Pull Request
Fixed Issues (3)Great job! The following issues were fixed in this Pull Request
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7159 +/- ##
=======================================
Coverage 56.87% 56.87%
=======================================
Files 2028 2028
Lines 88827 88827
Branches 7917 7917
=======================================
+ Hits 50523 50524 +1
+ Misses 36472 36471 -1
Partials 1832 1832 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
Thanks for coming back to this!
| When an organization policy is created or updated, the save workflow runs a series of ordered steps. Each step acts like a hook that a handler may listen to by implementing the particular policy event interface. | ||
|
|
||
| Note: If you don’t want to hook into these events, you don’t need to create a handler, and your policy will simply upsert to the database with log events. |
There was a problem hiding this comment.
I think the terminology could be clearer here: hook, handler, event, etc.
I suggest:
- the policy getting saved is the event
- the class that does something is a handler
Therefore: IEnforceDependentPoliciesHandler, IPolicyValidationHandler, etc.
The current wording makes it sound like the class is the event. But I think the class is the thing that handles the event (which is also how you describe it here).
|
|
||
| ``` | ||
| SaveAsync() | ||
| ├─ 1. Validate org can use policies |
There was a problem hiding this comment.
Avoid contractions:
| ├─ 1. Validate org can use policies | |
| ├─ 1. Validate organization can use policies |
| ## Limitations | ||
|
|
||
| 1. We don't have a way to keep this whole process idempotent, so if there is an exception at any point that is not being handled, the state will stay where the process failed. | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Section appears to be unfinished? Also, this is a good callout, but I think it's more about it not being atomic than idempotent. A failure does not roll back any changes already made.
| ### `IPolicyUpdateEvent` | ||
|
|
||
| The base interface that all policy event handlers must implement. | ||
|
|
||
| ```csharp | ||
| public interface IPolicyUpdateEvent | ||
| { | ||
| PolicyType Type { get; } | ||
| } | ||
| ``` | ||
|
|
||
| Every handler declares which `PolicyType` it handles via `Type`. All other event interfaces extend this one. |
There was a problem hiding this comment.
I suggest this can be omitted given that no other team will ever have to implement this directly.
| - **Enabling**: Each `PolicyType` in `RequiredPolicies` must already be enabled, otherwise a `BadRequestException` is thrown. | ||
| - **Disabling a required policy**: If any other policy has this policy listed as a requirement and is currently enabled, the disable action is blocked. |
There was a problem hiding this comment.
Suggestion: these lines are not required, you've already explained it above.
|
|
||
| 1. Create a class in `PolicyValidators/` implementing `IPolicyUpdateEvent` and any combination of the event interfaces above. | ||
| 2. Set `Type` to the appropriate `PolicyType`. | ||
| 3. Register the class as `IPolicyUpdateEvent` (and the legacy interfaces if needed) in `PolicyServiceCollectionExtensions.AddPolicyUpdateEvents()`. |
There was a problem hiding this comment.
Are we still using the legacy interfaces - is there any need to register them?
|
|
||
| ### Example | ||
|
|
||
| `AutomaticUserConfirmationPolicyEventHandler` is a good reference. It requires `SingleOrg`, validates org compliance before enabling, and removes emergency access grants as a pre-save side effect. |
There was a problem hiding this comment.
| `AutomaticUserConfirmationPolicyEventHandler` is a good reference. It requires `SingleOrg`, validates org compliance before enabling, and removes emergency access grants as a pre-save side effect. | |
| `AutomaticUserConfirmationPolicyEventHandler` is a good reference. It requires `SingleOrg`, validates organization compliance before enabling, and removes emergency access grants as a pre-save side effect. |
| ## Adding a New Event Interface | ||
|
|
||
| Use this when the existing interfaces don't cover your use case and you need a new hook in the save workflow. | ||
|
|
||
| ### Step 1: Define the interface in `PolicyUpdateEvents/Interfaces/`: | ||
|
|
||
| ```csharp | ||
| public interface IMyNewEvent : IPolicyUpdateEvent | ||
| { | ||
| Task ExecuteMyNewEventAsync(SavePolicyModel policyRequest, Policy? currentPolicy); | ||
| } | ||
| ``` | ||
|
|
||
| It must extend `IPolicyUpdateEvent`. | ||
|
|
||
| ### Step 2: Add a step to `SavePolicyCommand.SaveAsync()` or `VNextSavePolicyCommand.SaveAsync()` during transition | ||
|
|
||
| 1. Call your method at the appropriate position in the workflow | ||
| 2. You can use the existing `ExecutePolicyEventAsync<T>` helper or have your method use `policyEventHandlerFactory` directly to retrieve the handlers. | ||
| 3. **Note on cross-policy logic:** `IEnforceDependentPoliciesEvent` is a special case. It scans *all* registered handlers (not just the targeted policy's handler) to find dependents when disabling a policy. If your new interface requires similar cross-policy scanning, you will need to add that logic directly to `SavePolicyCommand` or `VNextSavePolicyCommand.SaveAsync()` during transition rather than using `ExecutePolicyEventAsync<T>`. | ||
|
|
||
| ### Step 3: Document the interface in the [Interfaces](#interfaces) section of this README and add it to the workflow diagram. |
There was a problem hiding this comment.
I don't expect this to happen often if ever - the current interfaces are fairly comprehensive. I think we could omit this just so the documentation stays focused on adding new policy types.
|
|
||
| --- | ||
|
|
||
| # IPolicyValidator (Legacy) |
There was a problem hiding this comment.
This has been deprecated for a while. When can we get rid of it?
| This is the policy update pattern that we want our system’s end state to follow. | ||
| This directory contains the interfaces and infrastructure for the policy save workflow used by `IVNextSavePolicyCommand`. | ||
| Currently, we’re using `IVNextSavePolicyCommand` to transition from the old `IPolicyValidator` pattern. |
There was a problem hiding this comment.
You have a separate section at the end to discuss the old pattern. I suggest this intro paragraph can just be focused on the new pattern.
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-29129
📔 Objective
📸 Screenshots
No code changes, just a markdown file.