[AC-292] Public Api - allow configuration of custom permissions #4022

eliykat · 2024-04-26T03:47:15Z

Type of change

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

Objective

Allow users to configure custom permissions via the Public API.

This also involved refactoring the OrganizationService invite user methods so that I wasn't adding to existing tech debt. I also considered extracting this to a command, however this turned out to be non-trivial, so I think it's best tackled separately.

Code changes

Following the commits in order:

b183325 - OrganizationService had too many very similar methods for inviting users - InviteUserAsync (x2 overloads), SaveUserSendInviteAsync, InviteUsersAsync (x2 overloads), and SaveUsersSendInvitesAsync. It was difficult to keep track of the control flow and understand where changes needed to happen. I remember we did this originally to avoid exposing nullable parameters to outside callers, however I think the result has overall been worse for it. I combined several of these methods so that we just have InviteUserAsync (single) -> InviteUsersAsync (multiple) -> private SaveUsersSendInvitesAsync.
- I think a better solution here is to have a LogSubject wrapper which can represent an OrgUser or an EventUser and that's only unwrapped once it reaches EventService - but out of scope here.
82fb3f8 - InviteUserAsync took an increasing number of parameters and I didn't want to add Permissions here. Refactor it to take an OrganizationUserInvite object, similar to InviteUsersAsync. Refactor calling locations to create this object (often from an existing request object) and pass it in.
4e65cb9 - finally :) add the custom permissions to the Public API request and response models. It is optional in all cases because we don't want to break existing scripts that aren't using it (either by throwing errors or by blasting away existing values).
32a3525 - if a custom user is being invited, but Permissions have not been specified, use a default Permissions object. This was prompted by AC-2318 which pointed out that our logic does not gracefully handle nulls here. That only occurred due to manipulation of the private API, however now that we're exposing it to the Public API we need to make sure we handle this case.
add tests, minor tweaks

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

…ission-v2

codecov · 2024-04-26T03:57:56Z

Codecov Report

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

Project coverage is 38.43%. Comparing base (7f9d7c0) to head (9e964f5).

Files	Patch %	Lines
...Api/AdminConsole/Public/Models/PermissionsModel.cs	0.00%	48 Missing ⚠️
.../Public/Models/Request/MemberCreateRequestModel.cs	0.00%	15 Missing ⚠️
.../Public/Models/Request/MemberUpdateRequestModel.cs	0.00%	10 Missing ⚠️
.../Api/AdminConsole/Public/Models/MemberBaseModel.cs	0.00%	9 Missing ⚠️
...le/Services/Implementations/OrganizationService.cs	77.41%	6 Missing and 1 partial ⚠️
...en_license/src/Scim/Models/ScimUserRequestModel.cs	91.42%	1 Missing and 2 partials ⚠️
...minConsole/Public/Controllers/MembersController.cs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4022      +/-   ##
==========================================
- Coverage   38.44%   38.43%   -0.01%     
==========================================
  Files        1209     1210       +1     
  Lines       58545    58633      +88     
  Branches     5585     5594       +9     
==========================================
+ Hits        22509    22537      +28     
- Misses      34991    35054      +63     
+ Partials     1045     1042       -3

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

github-actions · 2024-04-26T04:46:44Z

Checkmarx One – Scan Summary & Details – 8dc1cd54-3bfc-4360-bf0a-9a30a6fce516

New Issues

Issue	Source File / Package	Checkmarx Insight
CSRF	/src/Api/Public/Controllers/CollectionsController.cs: 87	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/Controllers/CollectionsController.cs: 212	Attack Vector
Missing_HSTS_Header	/bitwarden_license/src/Sso/Startup.cs: 57	Attack Vector
Privacy_Violation	/src/Api/Controllers/DevicesController.cs: 155	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/Vault/Controllers/CiphersController.cs: 222	Attack Vector
Log_Forging	/src/Api/Controllers/DevicesController.cs: 146	Attack Vector
Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 678	Attack Vector
Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 615	Attack Vector

Fixed Issues

Severity	Issue	Source File / Package
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 607
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 607
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 607
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 607
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 132
	CSRF	/src/Api/AdminConsole/Controllers/ProvidersController.cs: 141
	CSRF	/src/Api/SecretsManager/Controllers/AccessPoliciesController.cs: 229
	CSRF	/src/Admin/AdminConsole/Controllers/ProvidersController.cs: 309
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 163
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 163
	CSRF	/src/Api/Billing/Controllers/ProviderClientsController.cs: 30
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 190
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 333
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 333
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 669
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 645
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 891
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 173
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 711
	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/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: 159
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 286
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1120
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 222
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 570
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 193
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 312
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 301
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 174
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 722
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 187
	CSRF	/src/Api/Controllers/CollectionsController.cs: 303
	CSRF	/src/Api/Controllers/CollectionsController.cs: 411
	CSRF	/src/Api/Controllers/CollectionsController.cs: 323
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 530
	CSRF	/src/Admin/AdminConsole/Controllers/OrganizationsController.cs: 308
	CSRF	/src/Admin/AdminConsole/Controllers/ProvidersController.cs: 232
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 81
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 118
	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: 218
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 300
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 318
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 590
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 449
	CSRF	/src/Api/Controllers/CollectionsController.cs: 375
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 144
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 246
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 259
	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/AdminConsole/Controllers/OrganizationUsersController.cs: 286
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 926
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1043
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1043
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 545
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 432
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 898
	CSRF	/src/Api/Auth/Controllers/TwoFactorController.cs: 403
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 825
	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/AdminConsole/Controllers/ProviderUsersController.cs: 188
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 811
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 222
	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: 570
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 287
	CSRF	/src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 175
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1066
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1066
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 193
	CSRF	/src/Api/Controllers/SettingsController.cs: 36
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 867
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 361
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 627
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 627
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 221
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 583
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 583
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 748
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1100
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1017
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1017
	CSRF	/src/Api/Controllers/CollectionsController.cs: 411
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 931
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 276
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 572
	CSRF	/src/Api/Controllers/CollectionsController.cs: 303
	CSRF	/src/Api/Controllers/CollectionsController.cs: 375
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 407
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 774
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 246
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 786
	CSRF	/src/Api/AdminConsole/Controllers/ProviderOrganizationsController.cs: 53
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 59
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 127
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 519
	CSRF

More results are available on AST platform

…ission-v2

…--configure-custom-permission-v2

r-tome

Everything looks great to me! I noticed we don't have integration tests for the public MembersController. Is it feasible to add tests on this PR for the Post action?

…ission-v2

eliykat and others added 6 commits April 26, 2024 09:50

Recombine InviteUser overloads in OrganizationService

b183325

Refactor InviteUserAsync to take an object

82fb3f8

Add Custom Permissions to Public API

4e65cb9

dotnet format

fa2aae2

Set default permissions object

32a3525

Merge branch 'main' into ac/ac-292/public-api---configure-custom-perm…

b6a1952

…ission-v2

eliykat changed the title ~~Ac/ac 292/public api configure custom permission v2~~ [AC-292] Public Api - allow configuration of custom permissions Apr 26, 2024

eliykat and others added 10 commits April 29, 2024 09:47

Merge branch 'main' into ac/ac-292/public-api---configure-custom-perm…

855a91d

…ission-v2

Merge remote-tracking branch 'origin/main' into ac/ac-292/public-api-…

d42b5f0

…--configure-custom-permission-v2

Enable nullable

1d17a18

Fix test names to use proper sut method name

3c62d6b

Add tests

e444dde

Fix deserialization issue for PermissionsModel

baf44ae

Merge remote-tracking branch 'origin/main' into ac/ac-292/public-api-…

bad71ba

…--configure-custom-permission-v2

Update comment

9c9f8f7

Update comment

7115338

dotnet format

ce13267

eliykat marked this pull request as ready for review May 7, 2024 03:58

eliykat requested a review from a team as a code owner May 7, 2024 03:58

eliykat requested a review from vincentsalucci May 7, 2024 03:58

Add validation

bafdb8e

r-tome reviewed May 8, 2024

View reviewed changes

eliykat and others added 3 commits May 9, 2024 10:34

Merge branch 'main' into ac/ac-292/public-api---configure-custom-perm…

1c90693

…ission-v2

Merge branch 'main' into ac/ac-292/public-api---configure-custom-perm…

012e708

…ission-v2

dotnet format

125bb4a

eliykat requested a review from a team as a code owner May 10, 2024 05:59

Undo billing changes

9e964f5

eliykat removed the request for review from a team May 13, 2024 03:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AC-292] Public Api - allow configuration of custom permissions #4022

[AC-292] Public Api - allow configuration of custom permissions #4022

eliykat commented Apr 26, 2024 •

edited

codecov bot commented Apr 26, 2024 •

edited

github-actions bot commented Apr 26, 2024 •

edited

r-tome left a comment

[AC-292] Public Api - allow configuration of custom permissions #4022

Are you sure you want to change the base?

[AC-292] Public Api - allow configuration of custom permissions #4022

Conversation

eliykat commented Apr 26, 2024 • edited

Type of change

Objective

Code changes

Before you submit

codecov bot commented Apr 26, 2024 • edited

Codecov Report

github-actions bot commented Apr 26, 2024 • edited

New Issues

Fixed Issues

r-tome left a comment

Choose a reason for hiding this comment

eliykat commented Apr 26, 2024 •

edited

codecov bot commented Apr 26, 2024 •

edited

github-actions bot commented Apr 26, 2024 •

edited