[AC-2170] Group modal - limit admin access - collections tab #3998

eliykat · 2024-04-17T03:41:11Z

Type of change

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

Objective

This is equivalent to #3934 but for groups. If FC v1 is enabled, and admins cannot manage all collections and items, then a user can only grant collection access to a group if:

they can manage the collection, or
have the Manage Groups custom permission

Code changes

GroupsController -
- creating a new group - make sure user is authorized to grant access to all the collections
- editing an existing group - the same arrangement logic as in [AC-2172] Member modal - limit admin access #3934 - we need to separate authorized and non-authorized collections, make sure that they're only editing what they're allowed to, and make sure that we include the existing readonly collections in the db call so we don't blast them away
CollectionAuthorizationHandler - the ManageGroups custom permission is using the new endpoint to be added in [AC-2084] Include Collection permissions for admin endpoints #3793 to get the user's collection permissions, this authorizes that
GroupsControllerTests - add tests, largely following the same as in [AC-2172] Member modal - limit admin access #3934.

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

github-actions · 2024-04-17T04:50:36Z

Checkmarx One – Scan Summary & Details – 54e6b3dd-c86f-47c1-881d-2d1a36943a70

New Issues

Issue	Source File / Package	Checkmarx Insight
CSRF	/src/Api/Public/Controllers/CollectionsController.cs: 87	Attack Vector
CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 132	Attack Vector
CSRF	/src/Api/Controllers/CollectionsController.cs: 212	Attack Vector
CSRF	/src/Api/Controllers/CollectionsController.cs: 270	Attack Vector
CSRF	/src/Api/Controllers/CollectionsController.cs: 270	Attack Vector
CSRF	/src/Api/Controllers/CollectionsController.cs: 212	Attack Vector
CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 132	Attack Vector
Privacy_Violation	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 703	Attack Vector
Privacy_Violation	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 650	Attack Vector
Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 615	Attack Vector
Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 678	Attack Vector
Missing_CSP_Header	/src/Core/MailTemplates/Handlebars/Provider/InitiateDeleteProvider.html.hbs: 10	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/AdminConsole/Controllers/OrganizationsController.cs: 891
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 173
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 744
	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/Vault/Controllers/CiphersController.cs: 563
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 563
	CSRF	/src/Api/Controllers/SettingsController.cs: 36
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 438
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 284
	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/Vault/Controllers/CiphersController.cs: 222
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 205
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 959
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 911
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 299
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 193
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 766
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1100
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 550
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 613
	CSRF	/src/Api/Controllers/CollectionsController.cs: 303
	CSRF	/src/Api/Controllers/CollectionsController.cs: 411
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 878
	CSRF	/src/Api/Controllers/CollectionsController.cs: 323
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 244
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 222
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 807
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 288
	CSRF	/src/Api/Controllers/CollectionsController.cs: 375
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 284
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 187
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 159
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 362
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 222
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 702
	CSRF	/src/Api/Auth/Controllers/TwoFactorController.cs: 403
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 193
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 408
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 128
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 900
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 571
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 174
	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/OrganizationUsersController.cs: 316
	CSRF	/src/Api/Controllers/CollectionsController.cs: 411
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 150
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 150
	CSRF	/src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 133
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 586
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 433
	CSRF	/src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 175
	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/Controllers/CollectionsController.cs: 375
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1023
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1023
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 188
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 805
	CSRF	/src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 188
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 791
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 144
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 550
	CSRF	/src/Api/Controllers/CollectionsController.cs: 303
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 607
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 607
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1046
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1046
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 313
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 244
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 114
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 230
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 331
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
	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/Vault/Controllers/CiphersController.cs: 942
	CSRF	/src/Api/Controllers/CollectionsController.cs: 187
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 127
	CSRF	/src/Api/Controllers/CollectionsController.cs: 156
	CSRF	/src/Identity/Controllers/AccountsController.cs: 72
	CSRF	/src/Identity/Controllers/AccountsController.cs: 50
	CSRF	/src/Api/Controllers/CollectionsController.cs: 196
	CSRF

More results are available on AST platform

codecov · 2024-04-23T05:03:42Z

Codecov Report

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

Project coverage is 38.34%. Comparing base (5012d56) to head (a5683ea).

Files	Patch %	Lines
...c/Api/AdminConsole/Controllers/GroupsController.cs	82.69%	6 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3998      +/-   ##
==========================================
+ Coverage   38.30%   38.34%   +0.03%     
==========================================
  Files        1195     1195              
  Lines       58223    58268      +45     
  Branches     5583     5589       +6     
==========================================
+ Hits        22302    22342      +40     
- Misses      34871    34875       +4     
- Partials     1050     1051       +1

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

…l---collections-tab-changes

This reverts commit 098d44e.

…to ac/ac-2170/group-modal---collections-tab-changes

…mission

…to ac/ac-2170/group-modal---collections-tab-changes

…nges

eliykat added 17 commits April 25, 2024 10:45

Check collection permissions when inviting a member

a716c3b

Update auth handlers to use v1 flag

20a0eb5

Initial implementation for PUT

5355232

Fix existing tests

f33aac6

Add tests for PUT

f04dafb

Improve tests

4221d94

dotnet format

54e59e8

Add test for invite

3d92618

Add test for handler

751db0b

Add missing Manage column to sproc

8c50db2

dotnet format

1d692e5

Create separate Put_vNext method and update tests

31a5be8

Add additional feature flag check for invite method

554a6c8

Fix test

275c39f

Update call to old method

75fd880

Undo unrelated change from feature branch

4326e7a

Update old SaveUserAsync reference

b8faed7

eliykat force-pushed the ac/ac-2172/member-modal---access-selector-changes branch from d4912be to b8faed7 Compare April 25, 2024 01:25

eliykat added 7 commits April 25, 2024 11:40

Simplify logic in auth handler

18d027e

Initial implementation for PUT

f681780

Check collection access when creating or updating group

a33a736

Fix existing tests

97a5c54

Add tests for POST collection access

6cc0178

Copy collection access tests and get passing

7fdd166

dotnet format

5d8517d

eliykat force-pushed the ac/ac-2170/group-modal---collections-tab-changes branch from ae04971 to 5d8517d Compare April 25, 2024 01:49

Remove unintended changes

e45ce79

Base automatically changed from ac/ac-2172/member-modal---access-selector-changes to main April 29, 2024 01:02

eliykat added 6 commits April 29, 2024 11:22

Merge remote-tracking branch 'origin/main' into ac/ac-2170/group-moda…

78351a8

…l---collections-tab-changes

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 mentioned this pull request Apr 30, 2024

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

Merged

eliykat added 3 commits April 30, 2024 11:33

Make comment wording more consistent

6fb0eed

Merge branch 'ac/ac-2538/server-fix-manageusers-custom-permission' in…

433cecc

…to ac/ac-2170/group-modal---collections-tab-changes

Use ModifyGroupAccess operation

2b85e78

eliykat changed the base branch from main to ac/ac-2538/server-fix-manageusers-custom-permission April 30, 2024 02:56

eliykat added 2 commits May 1, 2024 09:51

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

c2b949b

…mission

Merge branch 'ac/ac-2538/server-fix-manageusers-custom-permission' in…

c1f6e12

…to ac/ac-2170/group-modal---collections-tab-changes

Base automatically changed from ac/ac-2538/server-fix-manageusers-custom-permission to main May 1, 2024 00:06

eliykat and others added 3 commits May 1, 2024 10:07

Merge branch 'main' into ac/ac-2170/group-modal---collections-tab-cha…

548f173

…nges

Allow ManageGroups to get all collectionDetails

509da4e

Finish tests

a5683ea

eliykat marked this pull request as ready for review May 1, 2024 01:44

eliykat requested review from a team as code owners May 1, 2024 01:44

eliykat requested review from r-tome, Jingo88 and vincentsalucci and removed request for r-tome May 1, 2024 01:44

vincentsalucci approved these changes May 1, 2024

View reviewed changes

eliykat merged commit e302ee1 into main May 1, 2024
48 of 49 checks passed

eliykat deleted the ac/ac-2170/group-modal---collections-tab-changes branch May 1, 2024 23:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AC-2170] Group modal - limit admin access - collections tab #3998

[AC-2170] Group modal - limit admin access - collections tab #3998

eliykat commented Apr 17, 2024 •

edited

github-actions bot commented Apr 17, 2024 •

edited

codecov bot commented Apr 23, 2024 •

edited

[AC-2170] Group modal - limit admin access - collections tab #3998

[AC-2170] Group modal - limit admin access - collections tab #3998

Conversation

eliykat commented Apr 17, 2024 • edited

Type of change

Objective

Code changes

Before you submit

github-actions bot commented Apr 17, 2024 • edited

New Issues

Fixed Issues

codecov bot commented Apr 23, 2024 • edited

Codecov Report

eliykat commented Apr 17, 2024 •

edited

github-actions bot commented Apr 17, 2024 •

edited

codecov bot commented Apr 23, 2024 •

edited