[PM-18235] Add PersonalOwnershipPolicyRequirement #5439
Conversation
…nt if the PolicyRequirements flag is enabled Update unit tests
…th feature flag - Add support for checking personal ownership policy using PolicyRequirementQuery when feature flag is enabled - Update CipherService constructor to inject new dependencies - Add tests for personal vault restrictions with and without feature flag
|
New Issues (3)Checkmarx found the following issues in this Pull Request
Fixed Issues (16)Great job! The following issues were fixed in this Pull Request
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5439 +/- ##
==========================================
+ Coverage 44.52% 44.56% +0.03%
==========================================
Files 1514 1515 +1
Lines 70403 70429 +26
Branches 6352 6355 +3
==========================================
+ Hits 31347 31385 +38
+ Misses 37708 37695 -13
- Partials 1348 1349 +1 ☔ View full report in Codecov by Sentry. |
...nsole/OrganizationFeatures/Policies/PolicyRequirements/PersonalOwnershipPolicyRequirement.cs
Outdated
Show resolved
Hide resolved
| @@ -26,7 +29,8 @@ public class ImportCiphersCommand : IImportCiphersCommand | |||
| private readonly ICollectionRepository _collectionRepository; | |||
| private readonly IReferenceEventService _referenceEventService; | |||
| private readonly ICurrentContext _currentContext; | |||
|
|
|||
| private readonly IPolicyRequirementQuery _policyRequirementQuery; | |||
| private readonly IFeatureService _featureService; | |||
|
|
|||
| public ImportCiphersCommand( | |||
There was a problem hiding this comment.
🌱 This constructor now has 10 dependencies. That's a good indication that this code is doing too much and a refactoring is in order.
❓ What would you recommend changing if you could? Dream big!
There was a problem hiding this comment.
Excellent observation! I think an easy thing to do would be to separate the individual vault import and the org vault import each into its own command. From there we could extract the validation logic into a validation class.
.../OrganizationFeatures/Policies/PolicyRequirements/PersonalOwnershipPolicyRequirementTests.cs
Show resolved
Hide resolved
test/Core.Test/Tools/ImportFeatures/ImportCiphersAsyncCommandTests.cs
Outdated
Show resolved
Hide resolved
| @@ -139,8 +147,11 @@ public async Task SaveDetailsAsync(CipherDetails cipher, Guid savingUserId, Date | |||
| else | |||
| { | |||
| // Make sure the user can save new ciphers to their personal vault | |||
| var anyPersonalOwnershipPolicies = await _policyService.AnyPoliciesApplicableToUserAsync(savingUserId, PolicyType.PersonalOwnership); | |||
| if (anyPersonalOwnershipPolicies) | |||
| var isPersonalVaultRestricted = _featureService.IsEnabled(FeatureFlagKeys.PolicyRequirements) | |||
There was a problem hiding this comment.
This is not a blocker since it can be considered a personal preference, but I think ternary statements are difficult to read when they are used for more than assigning simple values; when multiple steps are involved, they become hard to follow.
I would suggest abstracting this out into a function and using the early return pattern with if statements. Maybe something like this.
Calling area
if (await IsPersonalVaultRestricted(savingUserId))
{
throw new BadRequestException("Due to an Enterprise Policy, you are restricted from saving items to your personal vault.");
}Function
public async Task<bool> IsPersonalVaultRestricted(Guid savingUserId)
{
if (_featureService.IsEnabled(FeatureFlagKeys.PolicyRequirements))
{
var policy = await _policyRequirementQuery.GetAsync<PersonalOwnershipPolicyRequirement>(savingUserId);
return policy.DisablePersonalOwnership;
}
return await _policyService.AnyPoliciesApplicableToUserAsync(savingUserId, PolicyType.PersonalOwnership);
}There was a problem hiding this comment.
Normally I would agree but this ternary is temporary - it will only exist while the PolicyRequirements feature flag exists. When we eventually remove that feature flag and only use PersonalOwnershipPolicyRequirement to check this then the private method doesn't make sense anymore because it will be a one liner. Having this a ternary makes it simpler to replace in the future.
| @@ -139,8 +147,11 @@ public async Task SaveDetailsAsync(CipherDetails cipher, Guid savingUserId, Date | |||
| else | |||
| { | |||
| // Make sure the user can save new ciphers to their personal vault | |||
There was a problem hiding this comment.
Non-blocker: I know it's not part of your scope, but can we replace this comment with a meaningful variable name? I think your variable IsPersonalVaultRestricted is a good option.
| { | ||
| [Theory, AutoData] | ||
| public void DisablePersonalOwnership_IsFalse_IfNoPersonalOwnershipPolicies( | ||
| [PolicyDetails(PolicyType.RequireSso)] PolicyDetails otherPolicy1, |
There was a problem hiding this comment.
Maybe we should follow the should-when naming pattern for consistency?
Consider: DisablePersonalOwnership_ShouldNotBeEnforced_WhenNoPersonalOwnershipPoliciesExist.
There was a problem hiding this comment.
I have renamed them to follow my usual pattern of <MethodName>_<WithSomething>_<ExpectedResult>
| } | ||
|
|
||
| [Theory, AutoData] | ||
| public void DisablePersonalOwnership_Not_EnforcedAgainstProviders( |
There was a problem hiding this comment.
Consider using this name or something similar: DisablePersonalOwnership_ShouldNotBeEnforced_WhenUserIsAProvider.
…rity - Improve test method names to better describe their purpose and behavior - Rename methods to follow a more descriptive naming convention - No functional changes to the test logic
|
@JimmyVo16 thanks for the review! I've addressed your comments, let me know if you want to discuss anything further |
|
|
Putting this back in draft as it'll require a refactor with the eventual changes from #5445 |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-18235
📔 Objective
Implement
PersonalOwnershipPolicyRequirementand use it on code checking if the "Remove individual vault" applies to the user.This standardizes policy enforcement by following our established
PolicyRequirementpattern, ensuring consistent handling of role/status exemptions.⏰ 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 changes