Turn on LimitCollectionCreationDeletionSplit for self host
#35
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
22 file(s) reviewed, 19 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -1,4 +1,6 @@ | |||
| @model OrganizationViewModel | |||
| @inject Bit.Core.Services.IFeatureService FeatureService | |||
There was a problem hiding this comment.
style: Consider moving the IFeatureService injection to the top of the file for consistency with other injections
| else | ||
| { | ||
| <dt class="col-sm-4 col-lg-3">Limit collection creation to administrators</dt> | ||
| <dd id="pm-collection-creation" class="col-sm-8 col-lg-9">@(Model.Organization.LimitCollectionCreation ? "On" : "Off")</dd> |
There was a problem hiding this comment.
style: Update id attribute to 'pm-collection-creation-limit' for clarity
| <dd id="pm-collection-creation" class="col-sm-8 col-lg-9">@(Model.Organization.LimitCollectionCreation ? "On" : "Off")</dd> | ||
|
|
||
| <dt class="col-sm-4 col-lg-3">Limit collection deletion to administrators</dt> | ||
| <dd id="pm-collection-deletion" class="col-sm-8 col-lg-9">@(Model.Organization.LimitCollectionDeletion ? "On" : "Off")</dd> |
There was a problem hiding this comment.
style: Add id attribute 'pm-collection-deletion-limit' for consistency
| !_featureService.IsEnabled(FeatureFlagKeys.LimitCollectionCreationDeletionSplit) | ||
| ) | ||
| { | ||
| throw new BadRequestException("Only allowed when not self hosted."); |
There was a problem hiding this comment.
style: Consider using a more specific error message that explains why this operation is not allowed for self-hosted installations when the feature is disabled
| // Deperectated: https://bitwarden.atlassian.net/browse/PM-10863 | ||
| public bool LimitCollectionCreationDeletion { get; set; } |
There was a problem hiding this comment.
syntax: Typo in 'Deprecated'
| public bool LimitCollectionCreation { get; set; } | ||
| public bool LimitCollectionDeletion { get; set; } |
There was a problem hiding this comment.
style: Consider adding XML documentation comments for these new properties to explain their purpose and usage
| /// properties that were added for versions 14 and 15. These properties | ||
| /// were later removed! When you increment to version 16 please delete | ||
| /// this comment. | ||
| /// </remarks> | ||
| public const int CurrentLicenseFileVersion = 14; |
There was a problem hiding this comment.
logic: CurrentLicenseFileVersion is set to 14, but the ValidLicenseVersion check allows up to version 15. Consider aligning these values.
| // `LimitCollectonCreationDeletionSplit` feature flag state isn't | ||
| // relevant for this test. The flag is never checked for in this | ||
| // test. This is asserted below. | ||
| ArrangeOrganizationAbility_WithLimitCollectionCreationDeletionSplitFeatureEnabled(sutProvider, organization, true, true); |
There was a problem hiding this comment.
style: Consider moving this comment to a more general location if it applies to multiple tests
| // `LimitCollectonCreationDeletionSplit` feature flag state isn't | ||
| // relevant for this test. The flag is never checked for in this | ||
| // test. This is asserted below. | ||
| ArrangeOrganizationAbility_WithLimitCollectionCreationDeletionSplitFeatureEnabled(sutProvider, organization, true, true); |
There was a problem hiding this comment.
style: This comment is duplicated from earlier in the file. Consider creating a constant or helper method for this explanation
| @@ -1336,14 +1810,37 @@ public async Task CachesCollectionsWithCanManagePermissions( | |||
| await sutProvider.GetDependency<ICollectionRepository>().Received(1).GetManyByUserIdAsync(Arg.Any<Guid>()); | |||
| } | |||
|
|
|||
| private static void ArrangeOrganizationAbility( | |||
| private static void ArrangeOrganizationAbility_WithLimitCollectionCreationDeletionSplitFeatureDisabled( | |||
There was a problem hiding this comment.
style: These two methods are very similar. Consider refactoring to reduce duplication
🎟️ Tracking
🧵 Jira Ticket: PM-10863
📚 Stacked PRs
server: SplitLimitCollectionCreationDeletioninto two database columns bitwarden/server#4709server: SplitOrganization.LimitCollectionCreationDeletioninto two separate business rules bitwarden/server#4730clients: SplitOrganization.LimitCollectionCreationDeletioninto two separate business rules bitwarden/clients#11223server: Turn onLimitCollectionCreationDeletionSplitfor self host bitwarden/server#4808⬆️ YOU ARE HERE
server: [PM-14821] [PM-14822] RemoveLimitCollectionCreationDeletionSplitfeature flag bitwarden/server#4809clients: [PM-14821] RemoveLimitCollectionCreationDeletionSplitfeature flag bitwarden/clients#11258server: DropLimitCollectionCreationDeletionfrom the database bitwarden/server#4810📔 Objective
🤳 The Bigger Picture
There is an option in the Admin Console that removes collection creation and
deletion controls from all organization members that are not in the Owner and
Admin roles. This PR stack splits this conjoined option into two separate
controls: one to block
createoperations and one to blockdeleteoperations. This is a part of an effort to make these settings better align
with what customers expect it to do.
👉 This Pull Request 👍
Over the last two PRs we implemented the functional changes requested in this
body of work in
serverandclients. This PR is much more simple. It justenables the feature for self hosted installations by manually setting the
feature flag to
true.This PR and the previous two in the stack encompass all of the functional
changes to the system in this PR stack. By now all requested functionality
should be deployed and available to customers. The next three pull requests
are maintenance tasks to support evolutionary database design and feature
toggling.
📸 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 enables the 'LimitCollectionCreationDeletionSplit' feature for self-hosted installations, splitting the combined collection creation and deletion control into separate options.
LimitCollectionCreationandLimitCollectionDeletionproperties to various models and entities, replacing the deprecatedLimitCollectionCreationDeletionOrganizationServiceandUpdateOrganizationLicenseCommandto handle the new split controlsBulkCollectionAuthorizationHandlerto implement separate logic for creation and deletion permissions