[AC-1721] Include limit collection creation/deletion in license file #3388

vincentsalucci · 2023-10-30T20:45:27Z

Type of change

Objective

Make the LimitCollectionCreationDeletion property available for license sync if the FeatureFlag is enabled

Code changes

src/Core/Entities/Organization.cs Added LimitCollectionCreationDeletion property from license
src/Core/Models/Business/OrganizationLicense.cs: Added LimitCollectionCreationDeletion property from org. Bumped license file version. Updated ValidLicenseVersion range. Updated GetDataBytes to check for new version and property. Updated VerifyData to account for new version.
src/Core/Services/Implementations/OrganizationService.cs: Sets property if the feature flag allows or defaults to true otherwise

Before you submit

Please check for formatting errors (dotnet format --verify-no-changes) (required)
If making database changes - make sure you also update Entity Framework queries and/or migrations
Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team

* Update sql files to add Manage permission * Add migration script * Rename collection manage migration file to remove duplicate migration date * Migrations * Add manage to models * Add manage to repository * Add constraint to Manage columns * Migration lint fixes * Add manage to OrganizationUserUserDetails_ReadWithCollectionsById * Add missing manage fields * Add 'Manage' to UserCollectionDetails * Use CREATE OR ALTER where possible

* feat: update org table with new column, write migration, refs AC-1374 * feat: update views with new column, refs AC-1374 * feat: Alter sprocs (org create/update) to include new column, refs AC-1374 * feat: update entity/data/request/response models to handle new column, refs AC-1374 * feat: update necessary Provider related views during migration, refs AC-1374 * fix: update org create to default new column to false, refs AC-1374 * feat: added new API/request model for collection management and removed property from update request model, refs AC-1374 * fix: renamed migration script to be after secrets manage beta column changes, refs AC-1374 * fix: dotnet format, refs AC-1374 * feat: add ef migrations to reflect mssql changes, refs AC-1374 * fix: dotnet format, refs AC-1374 * feat: update API signature to accept Guid and explain Cd verbiage, refs AC-1374

…3194) * [AC-1174] Introduce BulkAuthorizationHandler.cs * [AC-1174] Introduce CollectionUserAuthorizationHandler * [AC-1174] Add CreateForNewCollection CollectionUser requirement * [AC-1174] Add some more details to CollectionCustomization * [AC-1174] Formatting * [AC-1174] Add CollectionGroupOperation.cs * [AC-1174] Introduce CollectionGroupAuthorizationHandler.cs * [AC-1174] Cleanup CollectionFixture customization Implement and use re-usable extension method to support seeded Guids * [AC-1174] Introduce WithValueFromList AutoFixtureExtensions Modify CollectionCustomization to use multiple organization Ids for auto generated test data * [AC-1174] Simplify CollectionUserAuthorizationHandler.cs Modify the authorization handler to only perform authorization logic. Validation logic will need to be handled by any calling commands/controllers instead. * [AC-1174] Introduce shared CollectionAccessAuthorizationHandlerBase A shared base authorization handler was created for both CollectionUser and CollectionGroup resources, as they share the same underlying management authorization logic. * [AC-1174] Update CollectionUserAuthorizationHandler and CollectionGroupAuthorizationHandler to use the new CollectionAccessAuthorizationHandlerBase class * [AC-1174] Formatting * [AC-1174] Cleanup typo and redundant ToList() call * [AC-1174] Add check for provider users * [AC-1174] Reduce nested loops * [AC-1174] Introduce ICollectionAccess.cs * [AC-1174] Remove individual CollectionGroup and CollectionUser auth handlers and use base class instead * [AC-1174] Tweak unit test to fail minimally * [AC-1174] Reorganize authorization handlers in Core project * [AC-1174] Introduce new AddCoreAuthorizationHandlers() extension method * [AC-1174] Move CollectionAccessAuthorizationHandler into Api project * [AC-1174] Move CollectionFixture to Vault folder * [AC-1174] Rename operation to CreateUpdateDelete * [AC-1174] Require single organization for collection access authorization handler - Add requirement that all target collections must belong to the same organization - Simplify logic related to multiple organizations - Update tests and helpers - Use ToHashSet to improve lookup time * [AC-1174] Fix null reference exception * [AC-1174] Throw bad request exception when collections belong to different organizations * [AC-1174] Switch to CollectionAuthorizationHandler instead of CollectionAccessAuthorizationHandler to reduce complexity

…llections

…#3252)

* [AC-1117] Add manage permission (#3126) * Update sql files to add Manage permission * Add migration script * Rename collection manage migration file to remove duplicate migration date * Migrations * Add manage to models * Add manage to repository * Add constraint to Manage columns * Migration lint fixes * Add manage to OrganizationUserUserDetails_ReadWithCollectionsById * Add missing manage fields * Add 'Manage' to UserCollectionDetails * Use CREATE OR ALTER where possible * [AC-1374] Limit collection creation/deletion to Owner/Admin (#3145) * feat: update org table with new column, write migration, refs AC-1374 * feat: update views with new column, refs AC-1374 * feat: Alter sprocs (org create/update) to include new column, refs AC-1374 * feat: update entity/data/request/response models to handle new column, refs AC-1374 * feat: update necessary Provider related views during migration, refs AC-1374 * fix: update org create to default new column to false, refs AC-1374 * feat: added new API/request model for collection management and removed property from update request model, refs AC-1374 * fix: renamed migration script to be after secrets manage beta column changes, refs AC-1374 * fix: dotnet format, refs AC-1374 * feat: add ef migrations to reflect mssql changes, refs AC-1374 * fix: dotnet format, refs AC-1374 * feat: update API signature to accept Guid and explain Cd verbiage, refs AC-1374 * feat: created collection auth handler/operations, added LimitCollectionCdOwnerAdmin to CurrentContentOrganization, refs AC-1125 * feat: create vault service collection extensions and register with base services, refs AC-1125 * feat: deprecated CurrentContext.CreateNewCollections, refs AC-1125 * feat: deprecate DeleteAnyCollection for single resource usages, refs AC-1125 * feat: move service registration to api, update references, refs AC-1125 * feat: add bulk delete authorization handler, refs AC-1125 * feat: always assign user and give manage access on create, refs AC-1125 * fix: updated CurrentContextOrganization type, refs AC-1125 * feat: combined existing collection authorization handlers/operations, refs AC-1125 * fix: OrganizationServiceTests -> CurrentContentOrganization typo, refs AC-1125 * fix: format, refs AC-1125 * fix: update collection controller tests, refs AC-1125 * fix: dotnet format, refs AC-1125 * feat: removed extra BulkAuthorizationHandler, refs AC-1125 * fix: dotnet format, refs AC-1125 * fix: change string to guid for org id, update bulk delete request model, refs AC-1125 * fix: remove delete many collection check, refs AC-1125 * fix: clean up collection auth handler, refs AC-1125 * fix: format fix for CollectionOperations, refs AC-1125 * fix: removed unnecessary owner check, add org null check to custom permission validation, refs AC-1125 * fix: remove unused methods in CurrentContext, refs AC-1125 * fix: removed obsolete test, fixed failling delete many test, refs AC-1125 * fix: CollectionAuthorizationHandlerTests fixes, refs AC-1125 * fix: OrganizationServiceTests fix broken test by mocking GetOrganization, refs AC-1125 * fix: CollectionAuthorizationHandler - remove unused repository, refs AC-1125 * feat: moved UserId null check to common method, refs AC-1125 * fix: updated auth handler tests to remove dependency on requirement for common code checks, refs AC-1125 * feat: updated conditionals/comments for create/delete methods within colleciton auth handler, refs AC-1125 * feat: added create/delete collection auth handler success methods, refs AC-1125 * fix: new up permissions to prevent excessive null checks, refs AC-1125 * fix: remove old reference to CreateNewCollections, refs AC-1125 * fix: typo within ViewAssignedCollections method, refs AC-1125 --------- Co-authored-by: Robyn MacCallum <robyntmaccallum@gmail.com>

…llections

…, refs AC-1649 (#3282)

* [AC-1174] Update SelectionReadOnlyRequestModel to use Guid for Id property * [AC-1174] Introduce initial bulk-access collection endpoint * [AC-1174] Introduce BulkAddCollectionAccessCommand and validation logic/tests * [AC-1174] Add CreateOrUpdateAccessMany method to CollectionRepository * [AC-1174] Add event logs for bulk add collection access command * [AC-1174] Add User_BumpAccountRevisionDateByCollectionIds and database migration script * [AC-1174] Implement EF repository method * [AC-1174] Improve null checks * [AC-1174] Remove unnecessary BulkCollectionAccessRequestModel helpers * [AC-1174] Add unit tests for new controller endpoint * [AC-1174] Fix formatting * [AC-1174] Remove comment * [AC-1174] Remove redundant organizationId parameter * [AC-1174] Ensure user and group Ids are distinct * [AC-1174] Cleanup tests based on PR feedback * [AC-1174] Formatting * [AC-1174] Update CollectionGroup alias in the sproc * [AC-1174] Add some additional comments to SQL sproc * [AC-1174] Add comment explaining additional SaveChangesAsync call --------- Co-authored-by: Thomas Rittson <trittson@bitwarden.com>

* Rename LimitCollectionCdOwnerAdmin -> LimitCollectionCreationDeletion * Rename and bump migration script

…cks (#3301) * fix: remove EditAnyCollection from Create/Delete permission check, refs AC-1666 * fix: updated comment, refs AC-1666

…aveAsync(...) (#3312) * fix: remove AssignUserId from CollectionService.SaveAsync, refs AC-1669 * fix: add manage access conditional before creating collection, refs AC-1669 * fix: move access logic for create/update, fix all tests, refs AC-1669 * fix: add CollectionAccessSelection fixture, update tests, update bad reqeuest message, refs AC-1669 * fix: format, refs AC-1669 * fix: update null params with specific arg.is null checks, refs Ac-1669 * fix: update attribute class name, refs AC-1669

…llections

* Add feature flags for FlexibleCollections and BulkCollectionAccess * Flag new routes and behaviour --------- Co-authored-by: Rui Tomé <108268980+r-tome@users.noreply.github.com>

…llections

eliykat · 2023-11-22T04:32:44Z

Can Billing Team please review organizationLicense changes carefully as the codeowners to be.

eliykat · 2023-11-23T20:49:33Z

I've added unit tests for this class in #3474, I'd like us to merge those first so that this PR can benefit from them.

eliykat

EDIT: see review below (sorry for Github spam)

eliykat

A couple of things that just occurred to me.

eliykat · 2023-11-23T22:48:32Z

src/Core/Entities/Organization.cs

@@ -258,5 +258,6 @@ public void UpdateFromLicense(OrganizationLicense license)
        UseSecretsManager = license.UseSecretsManager;
        SmSeats = license.SmSeats;
        SmServiceAccounts = license.SmServiceAccounts;
+        LimitCollectionCreationDeletion = license.LimitCollectionCreationDeletion;


This needs similar feature flag logic to OrganizationService. The OrgService method creates an organization, this method updates an existing organization. In both cases we want to ignore the license property and use the default value unless FC has been enabled for self-hosted.

I suggest just passing in a bool for the flag state.

eliykat · 2023-11-23T22:50:08Z

src/Core/Models/Business/OrganizationLicense.cs

+                    // LimitCollectionCreationDeletion was added in Version 14
+                    (Version >= 14 || !p.Name.Equals(nameof(LimitCollectionCreationDeletion))) &&


One problem with the current approach is that if the feature flag is off, the license file won't match the org object (because the create/update methods will choose to ignore the value in the license file). So there's a risk that validation will fail. ~~Given that it's not a billing-critical property, maybe we just exclude it from validation for now.~~ We should exclude this from validation until FC is enabled everywhere.

Alternatively, maybe we don't flag any of this logic and we just let the value be updated from the license file. After all, the actual logic will still be flagged on self-hosted. My concern there was that it'll be a latent change that will suddenly take effect after updating the self-hosted instance, and that could be unexpected.

~~There's a bit to this so let's discuss in the next FC sync.~~

@vincentsalucci I think the conclusion from our sync was that my feedback here is still accurate. Amended the comment above.

Once #3474 is merged, you'll need to update the tests with this version of the license file. Happy to help with that if required.

…se, refs AC-1721

eliykat

EDIT: see comment below. Otherwise looks good.

eliykat · 2023-12-05T05:37:53Z

src/Core/Models/Business/OrganizationLicense.cs

@@ -135,6 +136,7 @@ public OrganizationLicense()
    public bool UseSecretsManager { get; set; }
    public int? SmSeats { get; set; }
    public int? SmServiceAccounts { get; set; }
+    public bool LimitCollectionCreationDeletion { get; set; }


This might need a default value of true. i.e.

Suggested change

public bool LimitCollectionCreationDeletion { get; set; }

public bool LimitCollectionCreationDeletion { get; set; } = true;

Think of the situation where a self-hosted server is updated and has the flag on (so we'll use this value), but it's using an existing (old) license file that doesn't have this value in it. In that case, this class will be instantiated and this will default to false, which will then overwrite the default value of true in the db.

This is part of the trouble of having true as a default value instead of false. Won't be doing that again for sure.

Thought of yet another thing

…fs AC-1721

differsthecat and others added 30 commits August 11, 2023 14:50

Merge branch 'master' into feature/flexible-collections

78bdd51

Merge branch 'master' into feature/flexible-collections

2f45726

fix: merge conflict resolution

d5da5bb

Merge branch 'master' into feature/flexible-collections

e87c20c

Fix improper merge conflict resolution

e8053e2

Merge branch 'master' into feature/flexible-collections

3dfd38c

Merge remote-tracking branch 'origin/master' into feature/flexible-co…

3c9c8ac

…llections

Merge branch 'master' into feature/flexible-collections

4ac1b10

fix: add permission check for collection management api, refs AC-1647 (…

064a28c

…#3252)

Merge branch 'master' into feature/flexible-collections

acd3997

Merge remote-tracking branch 'origin/master' into feature/flexible-co…

9f5fec6

…llections

Merge branch 'master' into feature/flexible-collections

ffa09d1

refactor: remove organizationId from CollectionBulkDeleteRequestModel…

f2acf1c

…, refs AC-1649 (#3282)

Merge branch 'master' into feature/flexible-collections

2c7d02d

[AC-1646] Rename LimitCollectionCdOwnerAdmin column (#3300)

a3f554a

* Rename LimitCollectionCdOwnerAdmin -> LimitCollectionCreationDeletion * Rename and bump migration script

Merge branch 'master' into feature/flexible-collections

30b91cd

Merge branch 'master' into feature/flexible-collections

dd10614

[AC-1666] Removed EditAnyCollection from Create/Delete permission che…

fbb7aa1

…cks (#3301) * fix: remove EditAnyCollection from Create/Delete permission check, refs AC-1666 * fix: updated comment, refs AC-1666

Merge branch 'master' into feature/flexible-collections

0abd7c3

Merge remote-tracking branch 'origin/master' into feature/flexible-co…

fed3252

…llections

Merge branch 'master' into feature/flexible-collections

6bc38ac

[AC-1713] [Flexible collections] Add feature flags to server (#3334)

3b049a6

* Add feature flags for FlexibleCollections and BulkCollectionAccess * Flag new routes and behaviour --------- Co-authored-by: Rui Tomé <108268980+r-tome@users.noreply.github.com>

Merge remote-tracking branch 'origin/master' into feature/flexible-co…

ae18e76

…llections

Add joint codeownership for auth handlers (#3346)

52e723c

vincentsalucci removed the hold Hold this PR or item until later; DO NOT MERGE label Nov 8, 2023

vincentsalucci requested review from r-tome and eliykat November 8, 2023 21:02

Merge branch 'master' into ac/ac-1721/limit-collection-license-ffc

c5fc542

eliykat requested review from cturnbull-bitwarden and a team November 22, 2023 04:32

eliykat mentioned this pull request Nov 23, 2023

[AC-1741] Include owners/admins can manage all collections setting in license file #3458

Merged

eliykat reviewed Nov 23, 2023

View reviewed changes

vincentsalucci added 6 commits November 29, 2023 11:10

Merge branch 'master' into ac/ac-1721/limit-collection-license-ffc

ee83803

feat: add feature flag to organization entity for updating from licen…

3a0e236

…se, refs AC-1721

Merge branch 'master' into ac/ac-1721/limit-collection-license-ffc

7c3db2e

feat: updated license fixture with new version (14), refs AC-1721

7a309da

Merge branch 'master' into ac/ac-1721/limit-collection-license-ffc

65dc89b

feat: disable validity checks for version 14, refs AC-1721

845ad29

vincentsalucci requested a review from eliykat December 1, 2023 21:16

fix: dotnet format, refs AC-1721

7599a71

eliykat previously approved these changes Dec 5, 2023

View reviewed changes

eliykat reviewed Dec 5, 2023

View reviewed changes

vincentsalucci added 2 commits December 6, 2023 13:05

Merge branch 'master' into ac/ac-1721/limit-collection-license-ffc

4d69a17

feat: default org license LimitCollectionCreationDeletion to true, re…

fced6cb

…fs AC-1721

vincentsalucci requested a review from eliykat December 6, 2023 19:29

eliykat approved these changes Dec 7, 2023

View reviewed changes

cturnbull-bitwarden approved these changes Dec 8, 2023

View reviewed changes

gbubemismith approved these changes Dec 8, 2023

View reviewed changes

vincentsalucci merged commit e6ce9ff into master Dec 8, 2023
41 of 42 checks passed

vincentsalucci deleted the ac/ac-1721/limit-collection-license-ffc branch December 8, 2023 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AC-1721] Include limit collection creation/deletion in license file #3388

[AC-1721] Include limit collection creation/deletion in license file #3388

vincentsalucci commented Oct 30, 2023

eliykat commented Nov 22, 2023

eliykat commented Nov 23, 2023 •

edited

Loading

eliykat left a comment •

edited

Loading

eliykat left a comment

eliykat Nov 23, 2023 •

edited

Loading

eliykat Nov 23, 2023 •

edited

Loading

eliykat Nov 28, 2023 •

edited

Loading

eliykat left a comment •

edited

Loading

eliykat Dec 5, 2023 •

edited

Loading

		// LimitCollectionCreationDeletion was added in Version 14
		(Version >= 14 \|\| !p.Name.Equals(nameof(LimitCollectionCreationDeletion))) &&

	public bool LimitCollectionCreationDeletion { get; set; }
	public bool LimitCollectionCreationDeletion { get; set; } = true;

[AC-1721] Include limit collection creation/deletion in license file #3388

[AC-1721] Include limit collection creation/deletion in license file #3388

Conversation

vincentsalucci commented Oct 30, 2023

Type of change

Objective

Code changes

Before you submit

eliykat commented Nov 22, 2023

eliykat commented Nov 23, 2023 • edited Loading

eliykat left a comment • edited Loading

Choose a reason for hiding this comment

eliykat left a comment

Choose a reason for hiding this comment

eliykat Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

eliykat Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

eliykat Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

eliykat left a comment • edited Loading

Choose a reason for hiding this comment

eliykat Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

eliykat commented Nov 23, 2023 •

edited

Loading

eliykat left a comment •

edited

Loading

eliykat Nov 23, 2023 •

edited

Loading

eliykat Nov 23, 2023 •

edited

Loading

eliykat Nov 28, 2023 •

edited

Loading

eliykat left a comment •

edited

Loading

eliykat Dec 5, 2023 •

edited

Loading