Conversation
|
New Issues (3)Checkmarx found the following issues in this Pull Request
Fixed Issues (4)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.88% 56.88%
=======================================
Files 2028 2028
Lines 88830 88830
Branches 7918 7918
=======================================
+ Hits 50532 50533 +1
+ Misses 36468 36467 -1
Partials 1830 1830 ☔ 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).
There was a problem hiding this comment.
Yeah, unfortunately "event" is an overloaded term. The interfaces are called "Policy Update Event," so it would make sense to use "event" for them, but I can make sure to always add the adjective "Policy Update" to avoid confusion. There is also an audit log event, and I make sure to always include an adjective for that as well.
As for handlers, we can add the adjective "policy" (e.g., "policy handler") to reduce confusion.
the policy getting saved is the event
I'm not sure if this makes sense. It's an HTTP request that comes in, so normally people would call it a "request."
Originally, I was using “hook” interchangeably with “event,” but maybe I can just keep it as a verb to avoid confusion.
How does this sound? I'm open to other suggestions as well.
When an organization policy is created or updated, the save workflow runs a series of ordered steps. A policy handler can hook into any step by implementing the corresponding Policy Update Event interface.
Note: If you do not need to hook into any step, you do not need to create a policy handler. The policy will simply upsert to the database with an audit log event.
src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyUpdateEvents/README.md
Outdated
Show resolved
Hide resolved
| ## 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.
There was a problem hiding this comment.
Good catch. Also, this is completed. This is the only issue I'm aware of. I'm happy to add more if you know of anything else. I used a bullet list so we can easily add new ones.
| ### `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?
There was a problem hiding this comment.
We're still in a transition state, so right now we need to support both, in my opinion. But it looks like the epic is almost done, so maybe I can wait until it's complete before merging this doc (without the legacy pattern)? This will make it cleaner, and we don't have to revisit it later.
src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyUpdateEvents/README.md
Outdated
Show resolved
Hide resolved
| ## 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.
There was a problem hiding this comment.
This feels relevant to the other discussion. It seems like we should narrow the scope to "adding new policy types," but I feel like this section is important since we can't predict the future. Do you think a separate doc for the whole system would be better, or should we better organize this existing one?
|
|
||
| --- | ||
|
|
||
| # IPolicyValidator (Legacy) |
There was a problem hiding this comment.
This has been deprecated for a while. When can we get rid of it?
There was a problem hiding this comment.
I know we're in the process of getting rid of it, and I think it will be done soon. But since it's still in the codebase, I added it to the doc to give more context because removing it is pretty trivial. It's also not something I feel strongly about, so I'm happy with not including it as well.
| 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.
There was a problem hiding this comment.
The outcome of this discussion will affect this.
…teEvents/README.md Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
…teEvents/README.md Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
…teEvents/README.md Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
…teEvents/README.md Co-authored-by: Thomas Rittson <31796059+eliykat@users.noreply.github.com>
|
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-29129
📔 Objective
📸 Screenshots
No code changes, just a markdown file.