Skip to content
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

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

Merged
merged 69 commits into from
Dec 21, 2023

Conversation

gbubemismith
Copy link
Member

Type of change

- [X] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Collection Management Settings should be added to the license file and should be updated in the self-hosted instance when the license file is updated.

This PR is dependent on #3379

Code changes

  • src/Api/AdminConsole/Controllers/OrganizationsController.cs: Updated endpoint to allow only cloud access.
  • src/Core/Entities/Organization.cs: Added AllowAdminAccessToAllCollectionItems from license.
  • src/Core/Models/Business/OrganizationLicense.cs: Added AllowAdminAccessToAllCollectionItems property to OrganizationLicense and incremented version numbering
  • src/Core/Services/Implementations/OrganizationService.cs Sets property if the feature flag is enabled or defaults to true

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

differsthecat and others added 30 commits August 11, 2023 14:50
* 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
* [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>
* [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
* Add feature flags for FlexibleCollections and BulkCollectionAccess

* Flag new routes and behaviour

---------

Co-authored-by: Rui Tomé <108268980+r-tome@users.noreply.github.com>
@eliykat
Copy link
Member

eliykat commented Nov 23, 2023

This needs to wait until #3388 is merged, which adds the first collection management setting (limit collection creation/deletion) to the license file. Currently, you're both trying to bump the license version at the same time. And both PRs should wait until unit tests in #3474 are merged.

Base automatically changed from vault/ac-1122/add-collection-management-setting to master November 27, 2023 19:44
@shane-melton shane-melton requested a review from a team as a code owner November 27, 2023 19:44
@bitwarden-bot
Copy link

Logo
Checkmarx One – Scan Summary & Detailsb95e4a1b-ec8a-40ef-8f2a-3052b6634588

No New Or Fixed Issues Found

@djsmith85 djsmith85 removed the request for review from a team December 8, 2023 16:45
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this was accidentally included

src/Core/Models/Business/OrganizationLicense.cs Outdated Show resolved Hide resolved
src/Core/Models/Business/OrganizationLicense.cs Outdated Show resolved Hide resolved
Copy link
Member

@eliykat eliykat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The license file part of it looks good, thanks for taking the time to figure out those tests.

Just noticed this, then we're good to go.

@@ -268,5 +268,6 @@ public void UpdateFromLicense(OrganizationLicense license, bool flexibleCollecti
SmSeats = license.SmSeats;
SmServiceAccounts = license.SmServiceAccounts;
LimitCollectionCreationDeletion = !flexibleCollectionsIsEnabled || license.LimitCollectionCreationDeletion;
AllowAdminAccessToAllCollectionItems = !flexibleCollectionsIsEnabled || license.AllowAdminAccessToAllCollectionItems;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming here isn't very clear, sorry. flexibleCollectionsIsEnabled refers to the MVP feature flag. But this is a v1 feature. So you'll need to pass in both:

public void UpdateFromLicense(OrganizationLicense license, bool flexibleCollectionsMvpIsEnabled, bool flexibleCollectionsV1IsEnabled)
{
   // snip
       LimitCollectionCreationDeletion = !flexibleCollectionsMvpIsEnabled || license.LimitCollectionCreationDeletion;
       AllowAdminAccessToAllCollectionItems = !flexibleCollectionsV1IsEnabled || license.AllowAdminAccessToAllCollectionItems;

It's awkward but we'll clean it up after release.

@@ -601,7 +601,8 @@ private async Task ValidateSignUpPoliciesAsync(Guid ownerId)
UseSecretsManager = license.UseSecretsManager,
SmSeats = license.SmSeats,
SmServiceAccounts = license.SmServiceAccounts,
LimitCollectionCreationDeletion = !flexibleCollectionsIsEnabled || license.LimitCollectionCreationDeletion
LimitCollectionCreationDeletion = !flexibleCollectionsIsEnabled || license.LimitCollectionCreationDeletion,
AllowAdminAccessToAllCollectionItems = !flexibleCollectionsIsEnabled || license.AllowAdminAccessToAllCollectionItems
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, AllowAdminAccessToAllCollectionItems should use the v1 feature flag, and please rename existing variable to clearly distinguish between them.

LimitCollectionCreationDeletion should continue to use the mvp feature flag.

@gbubemismith gbubemismith removed the hold Hold this PR or item until later; DO NOT MERGE label Dec 21, 2023
@gbubemismith gbubemismith merged commit 3bffd09 into main Dec 21, 2023
42 of 43 checks passed
@gbubemismith gbubemismith deleted the vault/ac-1741 branch December 21, 2023 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants