[AC-2172] Member modal - limit admin access

[eliykat]

Type of change

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

Objective

Back-end changes for bitwarden/clients#8343.

Stacked on #3793.

Adds additional authorization logic on the following endpoints:

• OrganizationUser.Invite
• OrganizationUser.Put

Also updates authorization handlers for v1 logic.

Code changes

• OrganizationUsersController

  • Invite - check that the saving user can modify access for the collections they're trying to give the invited user
  • Put - this is more complex. Remember that the access-selector does not emit a value for disabled rows, so the client will only send values for the collections the user can modify. This method now (1) ensures that the user can modify the collection access sent by the client, and (2) concats them with collection access that the user can't change before saving to the database. This ensures we don't overwrite collection access that we don't want to change.

• BulkCollectionAuthorizationHandler

  • cache the managed collections so that we can call this in a loop in the Put method, above. (Now that I've done this, I think we could remove the "bulk" implementation of this, which would greatly simplify things - but I'll leave that for another day.)
  • change the Update method to take into account v1 flag and collection management setting.

• OrganizationUser_ReadWithCollectionsById - maybe the last sproc that didn't have the Manage property 😁

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

[codecov]

Codecov Report

Attention: Patch coverage is 84.61538% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 38.13%. Comparing base (8142ba7) to head (2613c2e).

Files	Patch %	Lines
...Console/Controllers/OrganizationUsersController.cs	79.24%	5 Missing and 6 partials ⚠️
.../Collections/BulkCollectionAuthorizationHandler.cs	96.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3934      +/-   ##
==========================================
+ Coverage   38.02%   38.13%   +0.11%     
==========================================
  Files        1195     1195              
  Lines       58140    58194      +54     
  Branches     5568     5576       +8     
==========================================
+ Hits        22107    22192      +85     
+ Misses      34994    34957      -37     
- Partials     1039     1045       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[vincentsalucci]

Seems like we're starting to get some more business logic in the controller. Do you have any plans/concern here?

[eliykat]

I agree, but it is authorization logic still - you only have to sort the groups and collections in this way because you're accessing it via this endpoint as a user. It doesn't apply to using the public api, for example. This is reflected by the fact that it requires AuthorizationService for this logic, which isn't available in the core layer. So I think it stays here for now, although I do agree that the endpoint itself is on the longer side.

[vincentsalucci]

Besides my question, I think this is good!

[eliykat]

Adding the hold label to remind myself not to merge yet - it's stacked on another feature branch.

[eliykat]

I realised that these changes weren't feature flagged properly - in particular, PUT did not check for org.FlexibleCollections. PUT and POST both contained a v1 flag check inside the authorization handler, but I think it's better if we do it expressly in the controller as well. Therefore:

• POST now checks the v1 flag explicitly (as well as org.FlexibleCollections)
• PUT has been split into a separate vNext method which is only called if org.FlexibleCollections and v1 flag is enabled.

This generally just makes the new code more separate from the existing code and ensures that it's only run when the flag is turned on.

The base branch was changed.

[eliykat]

I've rebased this on main so that it's not blocked by #3793. It can't be QA'd without it, but it can be reviewed and merged, and new code is feature flagged so it won't do any harm in the meantime.

That said, I apologise for the rebase blasting away review history. I believe @vincentsalucci and @Jingo88 had reviewed up to and including 1d692e5, however commits after that need a proper review.

[rkac-bw]

lgtm