[AC-2971] Draft: Policy strategy pattern #47
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile
| /// <summary> | ||
| /// The PolicyType that the strategy is responsible for handling. | ||
| /// </summary> | ||
| public PolicyType Type { get; } |
There was a problem hiding this comment.
style: Consider making this property read-only to ensure immutability
| var userOrgs = await _organizationUserRepository.GetManyByManyUsersAsync( | ||
| removableOrgUsers.Select(ou => ou.UserId!.Value)); |
There was a problem hiding this comment.
style: This query might be inefficient for large numbers of users. Consider optimizing or paginating
| // TODO: OrganizationService causes a circular dependency here, we either pass it into all handlers | ||
| // TODO: like we do with PolicyService at the moment, or we investigate and break that circle | ||
| // await _organizationService.DeleteUserAsync(policy.OrganizationId, orgUser.Id, | ||
| // savingUserId); |
There was a problem hiding this comment.
logic: Circular dependency issue needs to be resolved before this code can be fully implemented
| } | ||
| } | ||
|
|
||
| public async Task HandleDisable(Policy policy, Guid? savingUserId) |
There was a problem hiding this comment.
style: savingUserId parameter is unused in this method. Consider removing if not needed
| var policies = await _policyRepository.GetManyByOrganizationIdAsync(policy.OrganizationId); | ||
| if (policies.Any(p => p.Type == PolicyType.RequireSso && p.Enabled)) | ||
| { | ||
| throw new BadRequestException("Single Sign-On Authentication policy is enabled."); | ||
| } | ||
|
|
||
| if (policies.Any(p => p.Type == PolicyType.MaximumVaultTimeout && p.Enabled)) | ||
| { | ||
| throw new BadRequestException("Maximum Vault Timeout policy is enabled."); | ||
| } | ||
|
|
||
| if (policies.Any(p => p.Type == PolicyType.ResetPassword && p.Enabled)) | ||
| { | ||
| throw new BadRequestException("Account Recovery policy is enabled."); | ||
| } |
There was a problem hiding this comment.
style: Consider refactoring repeated policy checks into a separate method for better maintainability
| var currentPolicyIsEnabled = | ||
| policy.Id != default && | ||
| (await _policyRepository.GetByIdAsync(policy.Id))?.Enabled == true; |
There was a problem hiding this comment.
style: This logic could be moved to a separate method for better readability
|
|
||
| var now = DateTime.UtcNow; | ||
| if (policy.Id == default(Guid)) | ||
| var policyStrategy = _policyStrategies.Single(strategy => strategy.Type == policy.Type); |
There was a problem hiding this comment.
logic: Handle case where no matching strategy is found
🎟️ Tracking
TBA
📔 Objective
This is a proof of concept for refactoring the server-side PolicyService.
PolicyService.SaveAsynctakes up most of the class, and it has the following problems:This PR suggests implementing a strategy pattern to handle enabling and disabling policies. Each
PolicyTypeshould have a correspondingIPolicyStrategyimplementation, which can be used to handle actions when the policy is changed from enabled -> disabled or vice versa.PolicyServicethen injects all strategies and simply delegates the work to the matching strategy.This also allows for easier unit testing, which can be done for each strategy, rather than the entire PolicyService class.
This PR shows how this could work, using the Single Org policy as an example. Please provide feedback to help us plan future tech debt work.
Future improvements - but out of scope for now
These strategies could also be used for other policy-specific logic, such as:
Separating strategies from the service could also let us have different code ownership, e.g. teams own policies that affect their own domains (something we've talked about for a while now).
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changesGreptile Summary
This pull request implements a strategy pattern for policy management in the PolicyService, aiming to improve modularity and reduce complexity.
IPolicyStrategyinterface andSingleOrgPolicyStrategyimplementation for handling Single Organization policyPolicyService.SaveAsyncto delegate policy handling to appropriate strategiesPolicyServiceCollectionExtensionsfor registering policy-related servicesSharedWeb/Utilities/ServiceCollectionExtensions.csto includeAddPolicyServices()methodPolicyServiceby removing conditional logic and reducing dependencies