-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[PM-29129] Add Policy Update Event README #7159
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
base: main
Are you sure you want to change the base?
Changes from all commits
1bef6e8
85c0f58
4db2b5c
58466d4
160db08
07a888b
e8d4dc2
e61e08d
65b8b6a
0df2a9a
8d0ed75
b72f7cc
e7e1e46
e7eca10
5d18fa7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,251 @@ | ||||||
| # IPolicyUpdateEvent | ||||||
|
|
||||||
| 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. | ||||||
|
|
||||||
| --- | ||||||
|
|
||||||
| ## Overview | ||||||
|
|
||||||
| 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. | ||||||
|
|
||||||
| ``` | ||||||
| SaveAsync() | ||||||
| ββ 1. Validate organization can use policies | ||||||
| ββ 2. Validate policy dependencies β IEnforceDependentPoliciesEvent | ||||||
| ββ 3. Run policy-specific validation β IPolicyValidationEvent | ||||||
| ββ 4. Execute pre-save side effects β IOnPolicyPreUpdateEvent | ||||||
| ββ 5. Upsert policy + log event | ||||||
| ββ 6. Execute post-save side effects β IOnPolicyPostUpdateEvent | ||||||
| ``` | ||||||
|
|
||||||
| The `PolicyEventHandlerHandlerFactory` resolves the correct handler for a given `PolicyType` and interface at each step. A handler is matched by its `IPolicyUpdateEvent.Type` property. At most one handler of each interface type is permitted per `PolicyType`. | ||||||
|
|
||||||
| --- | ||||||
|
|
||||||
| ## Limitations | ||||||
|
|
||||||
| 1. The save workflow is not atomic. If an unhandled exception occurs at any step, changes made by prior steps are not rolled back. For example, pre-save side effects that have already executed will not be undone if the upsert subsequently fails. | ||||||
|
|
||||||
|
Comment on lines
+29
to
+32
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
| --- | ||||||
|
|
||||||
| ## Interfaces | ||||||
|
|
||||||
| ### `IPolicyUpdateEvent` | ||||||
|
|
||||||
| The base interface that all policy event handlers must implement. | ||||||
|
|
||||||
| ```csharp | ||||||
| public interface IPolicyUpdateEvent | ||||||
| { | ||||||
| PolicyType Type { get; } | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| --- | ||||||
|
|
||||||
| ### `IEnforceDependentPoliciesEvent` | ||||||
|
|
||||||
| Declares prerequisite policies that must be enabled before this policy can be enabled. Also prevents a required policy from being disabled while a dependent policy is active. | ||||||
|
|
||||||
| ```csharp | ||||||
| public interface IEnforceDependentPoliciesEvent : IPolicyUpdateEvent | ||||||
| { | ||||||
| IEnumerable<PolicyType> RequiredPolicies { get; } | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| --- | ||||||
|
|
||||||
| ### `IPolicyValidationEvent` | ||||||
|
|
||||||
| Runs custom validation logic before the policy is saved. | ||||||
|
|
||||||
| ```csharp | ||||||
| public interface IPolicyValidationEvent : IPolicyUpdateEvent | ||||||
| { | ||||||
| Task<string> ValidateAsync(SavePolicyModel policyRequest, Policy? currentPolicy); | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| Return an empty string to pass validation. Return a non-empty error message to throw a `BadRequestException` and abort the save. | ||||||
|
|
||||||
| --- | ||||||
|
|
||||||
| ### `IOnPolicyPreUpdateEvent` | ||||||
|
|
||||||
| Executes side effects **before** the policy is upserted to the database. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this normal expected behavior, where if the previous step fails, it wouldn't move to the next one? Therefore, if it fails here, it won't save the policy. |
||||||
|
|
||||||
| ```csharp | ||||||
| public interface IOnPolicyPreUpdateEvent : IPolicyUpdateEvent | ||||||
| { | ||||||
| Task ExecutePreUpsertSideEffectAsync(SavePolicyModel policyRequest, Policy? currentPolicy); | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| Typical uses: revoking non-compliant users. | ||||||
|
|
||||||
| --- | ||||||
|
|
||||||
| ### `IOnPolicyPostUpdateEvent` | ||||||
|
|
||||||
| Executes side effects **after** the policy has been upserted to the database. | ||||||
|
|
||||||
| ```csharp | ||||||
| public interface IOnPolicyPostUpdateEvent : IPolicyUpdateEvent | ||||||
| { | ||||||
| Task ExecutePostUpsertSideEffectAsync( | ||||||
| SavePolicyModel policyRequest, | ||||||
| Policy postUpsertedPolicyState, | ||||||
| Policy? previousPolicyState); | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| Typical uses: creating collections, sending notifications that depend on the new policy state. | ||||||
|
|
||||||
| Note: This is more useful for enabling a policy than for disabling a policy, since when the policy is disabled, there is no easy way to find the users the policy should be enforced on. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this note is confusing/unnecessary. You wouldn't be enforcing a policy that has been disabled.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, which makes this event only relevant to enabling, not disabling. That's why I thought explicitly stating it here gives more context on how to use it, since the system allows you to add it for disabling as well. But we can remove it if it causes more confusion. |
||||||
|
|
||||||
| --- | ||||||
|
|
||||||
| ### `IPolicyEventHandlerFactory` | ||||||
|
|
||||||
| Resolves the correct handler for a given `PolicyType` and event interface type. | ||||||
|
|
||||||
| ```csharp | ||||||
| OneOf<T, None> GetHandler<T>(PolicyType policyType) where T : IPolicyUpdateEvent; | ||||||
| ``` | ||||||
|
|
||||||
| Returns the matching handler, or `None` if the policy type does not implement the requested interface. Throws `InvalidOperationException` if more than one handler is registered for the same `PolicyType` and interface. | ||||||
|
Comment on lines
+113
to
+121
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably not required here as it's internal.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My intention for this doc is to explain how this whole system works, so I feel like it's relevant. Do you want to narrow the scope to just how to add a new policy handler? If so, we can create another doc or reorganize the sections to explain how the whole system works. |
||||||
|
|
||||||
| --- | ||||||
|
|
||||||
| ## Adding a New Policy Handler | ||||||
|
|
||||||
| 1. Create a class in `PolicyValidators/` implementing 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()`. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we still using the legacy interfaces - is there any need to register them?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
|
|
||||||
| Note: No changes to `VNextSavePolicyCommand` or `PolicyEventHandlerHandlerFactory` are required. | ||||||
|
|
||||||
| ### Example | ||||||
|
|
||||||
| `AutomaticUserConfirmationPolicyEventHandler` is a good reference. It requires `SingleOrg`, validates organization compliance before enabling, and removes emergency access grants as a pre-save side effect. | ||||||
|
|
||||||
| **Step 1: Create the handler** (`PolicyValidators/AutomaticUserConfirmationPolicyEventHandler.cs`): | ||||||
|
|
||||||
| ```csharp | ||||||
| public class AutomaticUserConfirmationPolicyEventHandler( | ||||||
| IAutomaticUserConfirmationOrganizationPolicyComplianceValidator validator, | ||||||
| IOrganizationUserRepository organizationUserRepository, | ||||||
| IDeleteEmergencyAccessCommand deleteEmergencyAccessCommand) | ||||||
| : IPolicyValidationEvent, IEnforceDependentPoliciesEvent, IOnPolicyPreUpdateEvent | ||||||
| { | ||||||
| public PolicyType Type => PolicyType.AutomaticUserConfirmation; | ||||||
|
|
||||||
| // IEnforceDependentPoliciesEvent: SingleOrg must be enabled before this policy can be enabled | ||||||
| public IEnumerable<PolicyType> RequiredPolicies => [PolicyType.SingleOrg]; | ||||||
|
|
||||||
| // IPolicyValidationEvent: Validates org compliance | ||||||
| public async Task<string> ValidateAsync(SavePolicyModel savePolicyModel, Policy? currentPolicy) | ||||||
| { | ||||||
| var policyUpdate = savePolicyModel.PolicyUpdate | ||||||
| var isNotEnablingPolicy = policyUpdate is not { Enabled: true }; | ||||||
| var policyAlreadyEnabled = currentPolicy is { Enabled: true }; | ||||||
| if (isNotEnablingPolicy || policyAlreadyEnabled) | ||||||
| { | ||||||
| return string.Empty; | ||||||
| } | ||||||
|
|
||||||
| return (await validator.IsOrganizationCompliantAsync( | ||||||
| new AutomaticUserConfirmationOrganizationPolicyComplianceValidatorRequest(policyUpdate.OrganizationId))) | ||||||
| .Match( | ||||||
| error => error.Message, | ||||||
| _ => string.Empty); | ||||||
| } | ||||||
|
|
||||||
| // IOnPolicyPreUpdateEvent: Revokes non-compliant users, removes emergency access grants before enabling | ||||||
| public async Task ExecutePreUpsertSideEffectAsync(SavePolicyModel policyRequest, Policy? currentPolicy) | ||||||
| { | ||||||
| var isNotEnablingPolicy = policyRequest.PolicyUpdate is not { Enabled: true }; | ||||||
| var policyAlreadyEnabled = currentPolicy is { Enabled: true }; | ||||||
| if (isNotEnablingPolicy || policyAlreadyEnabled) | ||||||
| { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| var orgUsers = await organizationUserRepository.GetManyByOrganizationAsync(policyRequest.PolicyUpdate.OrganizationId, null); | ||||||
| var orgUserIds = orgUsers.Where(w => w.UserId != null).Select(s => s.UserId!.Value).ToList(); | ||||||
|
|
||||||
| await deleteEmergencyAccessCommand.DeleteAllByUserIdsAsync(orgUserIds); | ||||||
| } | ||||||
|
|
||||||
| // IOnPolicyPostUpdateEvent: No implementation is needed since this handler doesnβt require it. | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| **Step 2: Register the handler** in `PolicyServiceCollectionExtensions.AddPolicyUpdateEvents()`: | ||||||
|
|
||||||
| ```csharp | ||||||
| services.AddScoped<IPolicyUpdateEvent, AutomaticUserConfirmationPolicyEventHandler>(); | ||||||
| ``` | ||||||
|
|
||||||
| --- | ||||||
|
|
||||||
| ## 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. | ||||||
|
Comment on lines
+197
to
+218
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has been deprecated for a while. When can we get rid of it?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
|
|
||||||
| `IPolicyValidator` is the **old pattern** and is being phased out. It is consumed by `ISavePolicyCommand` / `PolicyService.SavePolicyAsync`. | ||||||
|
|
||||||
| ```csharp | ||||||
| public interface IPolicyValidator | ||||||
| { | ||||||
| PolicyType Type { get; } | ||||||
| IEnumerable<PolicyType> RequiredPolicies { get; } | ||||||
| Task<string> ValidateAsync(PolicyUpdate policyUpdate, Policy? currentPolicy); | ||||||
| Task OnSaveSideEffectsAsync(PolicyUpdate policyUpdate, Policy? currentPolicy); | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| --- | ||||||
|
|
||||||
| ## Reason for transition | ||||||
|
|
||||||
| 1. `IPolicyValidator` combines dependency enforcement (`RequiredPolicies`), validation (`ValidateAsync`), and pre-save side effects (`OnSaveSideEffectsAsync`) into a single flat interface. This makes it awkward to add a post-save side effect, since that must be executed after the policy is saved, which lives in a different abstraction. | ||||||
| 2. The request body has also expanded, and we need to support metadata that the server needs to perform operations, but that data is not intended to be saved with the policy. | ||||||
| 3. By breaking each event hook into a separate interface, it reduces boilerplate, and new hooks can be added without affecting existing services. | ||||||
|
|
||||||
| --- | ||||||
|
|
||||||
| ## During the transition | ||||||
|
|
||||||
| 1. New policies should implement **both** `IPolicyValidator` and the appropriate `IPolicyUpdateEvent` sub-interfaces so they work correctly regardless of which save path is called. Once `ISavePolicyCommand` is fully replaced by `IVNextSavePolicyCommand` and removed, the `IPolicyValidator` implementation can be dropped. | ||||||
| 2. Previous implementations of `IPolicyValidator` classes have a postfix of `Validator`, but once we move to `IPolicyUpdateEvent`, they should be renamed to `Handler`. This will reduce confusion since validation normally implies there are no write operations, but there are in this context. | ||||||
|
|
||||||
| --- | ||||||
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The outcome of this discussion will affect this.