[AC-2538] Member modal - limit admin access - fix ManageUsers custom permission #4032

eliykat · 2024-04-30T01:30:10Z

Type of change

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

Objective

Fix bug for Flexible Collections v1: the BulkCollectionAuthorizationHandler did not authorize the "Manage Users" or "Manage Groups" custom permissions, which broke the flow for #3934 (and would've for #3998).

This requires that we split the ModifyAccess operation into ModifyUserAccess and ModifyGroupAccess, to match the granularity of the custom permissions, and use these new operations where required.

Code changes

src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionOperations.cs - delete ModifyAccess and replace it with ModifyUserAccess and ModifyGroupAccess
src/Api/AdminConsole/Controllers/OrganizationUsersController.cs - update to use ModifyUserAccess, which is always what you're doing via the OrganizationUsersController
src/Api/Controllers/CollectionsController.cs - update to use the new operations as appropriate (depending on what the endpoint is updating)
src/Api/Vault/AuthorizationHandlers/Collections/BulkCollectionAuthorizationHandler.cs - the biggest changes:
- make all private methods return a bool and call context.Succeed from the top-level method only. I really like how this simplified the handler
- add private methods for the 2x new operations/requirements. They are subsets of the more general Update operation, e.g. if you can Update then you can ModifyUserAccess. There's an argument for splitting these out completely (so that Update only refers to the collection and not its access) but I decided to pull up for now.
update tests.

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

This reverts commit 098d44e.

codecov · 2024-04-30T01:39:26Z

Codecov Report

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

Project coverage is 38.30%. Comparing base (3749fa6) to head (c2b949b).

Files	Patch %	Lines
src/Api/Controllers/CollectionsController.cs	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4032      +/-   ##
==========================================
- Coverage   38.30%   38.30%   -0.01%     
==========================================
  Files        1195     1195              
  Lines       58232    58223       -9     
  Branches     5584     5583       -1     
==========================================
- Hits        22307    22302       -5     
+ Misses      34873    34871       -2     
+ Partials     1052     1050       -2

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

github-actions · 2024-04-30T02:08:39Z

Checkmarx One – Scan Summary & Details – 6353fc61-d49f-438e-8aa8-b6a62f93b1ef

New Issues

Issue	Source File / Package	Checkmarx Insight
CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 891	Attack Vector
CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 173	Attack Vector
CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 744	Attack Vector
CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 900	Attack Vector
CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590	Attack Vector
CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 230	Attack Vector
CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590	Attack Vector
CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 560	Attack Vector
CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590	Attack Vector
CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 303	Attack Vector
Missing_HSTS_Header	/bitwarden_license/src/Sso/Startup.cs: 57	Attack Vector
Privacy_Violation	/src/Api/Vault/Controllers/CiphersController.cs: 919	Attack Vector
Privacy_Violation	/src/Core/Auth/UserFeatures/UserMasterPassword/SetInitialMasterPasswordCommand.cs: 59	Attack Vector
Privacy_Violation	/src/Core/Services/Implementations/UserService.cs: 647	Attack Vector
Privacy_Violation	/src/Core/Services/Implementations/UserService.cs: 813	Attack Vector
Privacy_Violation	/src/Core/Services/Implementations/UserService.cs: 592	Attack Vector
Privacy_Violation	/src/Core/Services/Implementations/UserService.cs: 842	Attack Vector
Log_Forging	/src/Api/Vault/Controllers/CiphersController.cs: 911	Attack Vector

Fixed Issues

Severity	Issue	Source File / Package
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 587
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 587
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 587
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 587
	CSRF	/src/Api/AdminConsole/Controllers/ProvidersController.cs: 141
	CSRF	/src/Admin/AdminConsole/Controllers/ProvidersController.cs: 309
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 145
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 145
	CSRF	/src/Api/Billing/Controllers/ProviderClientsController.cs: 30
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 190
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 331
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 331
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 678
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 702
	CSRF	/src/Api/Vault/Controllers/FoldersController.cs: 45
	CSRF	/src/Api/Controllers/SelfHosted/SelfHostedOrganizationLicensesController.cs: 51
	CSRF	/src/Api/Controllers/UsersController.cs: 22
	CSRF	/src/Api/Controllers/DevicesController.cs: 70
	CSRF	/src/Api/Controllers/DevicesController.cs: 57
	CSRF	/src/Api/AdminConsole/Public/Controllers/PoliciesController.cs: 69
	CSRF	/src/Api/AdminConsole/Public/Controllers/PoliciesController.cs: 49
	CSRF	/src/Api/AdminConsole/Public/Controllers/OrganizationController.cs: 42
	CSRF	/src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 92
	CSRF	/src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 49
	CSRF	/src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 142
	CSRF	/src/Api/AdminConsole/Controllers/ProviderOrganizationsController.cs: 52
	CSRF	/src/Api/AdminConsole/Controllers/PoliciesController.cs: 148
	CSRF	/src/Api/AdminConsole/Controllers/PoliciesController.cs: 78
	CSRF	/src/Api/AdminConsole/Controllers/PoliciesController.cs: 61
	CSRF	/bitwarden_license/src/Sso/Controllers/AccountController.cs: 163
	CSRF	/bitwarden_license/src/Sso/Controllers/AccountController.cs: 96
	CSRF	/bitwarden_license/src/Scim/Controllers/v2/UsersController.cs: 50
	CSRF	/src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 161
	CSRF	/src/Api/Auth/Controllers/EmergencyAccessController.cs: 159
	CSRF	/bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 98
	CSRF	/bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 88
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 299
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 550
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 159
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 222
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 607
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 607
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 702
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1023
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1023
	CSRF	/src/Api/Controllers/CollectionsController.cs: 303
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 193
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 244
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 174
	CSRF	/src/Api/Controllers/CollectionsController.cs: 410
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 284
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 187
	CSRF	/src/Api/Controllers/CollectionsController.cs: 374
	CSRF	/src/Api/Auth/Controllers/TwoFactorController.cs: 403
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 791
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 144
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 128
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 805
	CSRF	/src/Admin/AdminConsole/Controllers/OrganizationsController.cs: 308
	CSRF	/src/Admin/AdminConsole/Controllers/ProvidersController.cs: 232
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 77
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 114
	CSRF	/src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 175
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 550
	CSRF	/src/Api/Controllers/SettingsController.cs: 36
	CSRF	/src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 133
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 150
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 150
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 563
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 563
	CSRF	/src/Api/AdminConsole/Public/Controllers/OrganizationController.cs: 42
	CSRF	/src/Api/AdminConsole/Public/Controllers/OrganizationController.cs: 42
	CSRF	/src/Api/AdminConsole/Public/Controllers/OrganizationController.cs: 42
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 911
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 188
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 728
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1080
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 997
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 997
	CSRF	/src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 188
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1046
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1046
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 222
	CSRF	/src/Api/Controllers/CollectionsController.cs: 303
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 193
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 244
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 766
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 438
	CSRF	/src/Api/Controllers/CollectionsController.cs: 410
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 284
	CSRF	/src/Api/Controllers/CollectionsController.cs: 323
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1100
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 205
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 878
	CSRF	/src/Api/Controllers/CollectionsController.cs: 374
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 86
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 216
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 298
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 316
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 59
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 127
	CSRF	/src/Api/Controllers/CollectionsController.cs: 156
	CSRF	/src/Api/Controllers/CollectionsController.cs: 187
	CSRF	/src/Api/Controllers/CollectionsController.cs: 196
	CSRF	/src/Identity/Controllers/AccountsController.cs: 72
	CSRF	/src/Api/Public/Controllers/CollectionsController.cs: 64
	CSRF	/src/Api/Tools/Controllers/ImportCiphersController.cs: 50
	CSRF	/src/Api/Tools/Controllers/ImportCiphersController.cs: 66
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 111
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 125
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 942
	CSRF	/src/Identity/Controllers/AccountsController.cs: 50
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 313
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 362
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 433
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 571
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 222
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 586
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 288

More results are available on AST platform

vincentsalucci

I like centralizing the authorized boolean as well.

…mission

shane-melton

This looks great! I really like that we can simply return a boolean for the helpers and don't need to pass in the requirement everywhere!

eliykat added 5 commits April 30, 2024 10:45

Separate group and user collection operations

a4953cb

Split updating access out of Update operation

098d44e

Revert "Split updating access out of Update operation"

eb6fab2

This reverts commit 098d44e.

Fix Handler being too permissive

b78d7c6

Add tests for new operations

8d71aeb

eliykat requested review from shane-melton and vincentsalucci April 30, 2024 01:30

eliykat requested review from a team as code owners April 30, 2024 01:30

eliykat changed the title ~~Ac/ac 2538/server fix manageusers custom permission~~ [AC-2538] Member modal - limit admin access - fix ManageUsers custom permission Apr 30, 2024

Make comment wording more consistent

6fb0eed

eliykat mentioned this pull request Apr 30, 2024

Delete duplicate BulkCollectionOperation.ReadWithAccess #4029

Draft

vincentsalucci approved these changes Apr 30, 2024

View reviewed changes

Merge branch 'main' into ac/ac-2538/server-fix-manageusers-custom-per…

c2b949b

…mission

shane-melton approved these changes May 1, 2024

View reviewed changes

eliykat merged commit 5012d56 into main May 1, 2024
50 checks passed

eliykat deleted the ac/ac-2538/server-fix-manageusers-custom-permission branch May 1, 2024 00:06

eliykat mentioned this pull request May 1, 2024

[AC-2545] Member modal - limit admin access - prevent user from adding themselves to collection #4037

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AC-2538] Member modal - limit admin access - fix ManageUsers custom permission #4032

[AC-2538] Member modal - limit admin access - fix ManageUsers custom permission #4032

eliykat commented Apr 30, 2024 •

edited

codecov bot commented Apr 30, 2024 •

edited

github-actions bot commented Apr 30, 2024 •

edited

vincentsalucci left a comment

shane-melton left a comment

[AC-2538] Member modal - limit admin access - fix ManageUsers custom permission #4032

[AC-2538] Member modal - limit admin access - fix ManageUsers custom permission #4032

Conversation

eliykat commented Apr 30, 2024 • edited

Type of change

Objective

Code changes

Before you submit

codecov bot commented Apr 30, 2024 • edited

Codecov Report

github-actions bot commented Apr 30, 2024 • edited

New Issues

Fixed Issues

vincentsalucci left a comment

Choose a reason for hiding this comment

shane-melton left a comment

Choose a reason for hiding this comment

eliykat commented Apr 30, 2024 •

edited

codecov bot commented Apr 30, 2024 •

edited

github-actions bot commented Apr 30, 2024 •

edited