[AC-2847] Simplify OrganizationUser and Group PUT methods and tests #4479

eliykat · 2024-07-09T00:57:53Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/AC-2847

📔 Objective

Review and tidy up the OrganizationUser and Group controller PUT methods and simplify their test coverage. This is largely done by moving their tests to a separate file and simplifying/DRYing up the mocks used for setup.

See the ticket above for more detail.

📸 Screenshots

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

codecov · 2024-07-09T01:07:13Z

Codecov Report

Attention: Patch coverage is 88.09524% with 15 lines in your changes missing coverage. Please review.

Project coverage is 41.57%. Comparing base (883a2da) to head (c706d2e).
Report is 1 commits behind head on main.

Files	Patch %	Lines
.../OrganizationFeatures/Groups/UpdateGroupCommand.cs	86.27%	4 Missing and 3 partials ⚠️
...c/Api/AdminConsole/Controllers/GroupsController.cs	73.33%	2 Missing and 2 partials ⚠️
...OrganizationUsers/UpdateOrganizationUserCommand.cs	91.83%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4479      +/-   ##
==========================================
+ Coverage   41.49%   41.57%   +0.07%     
==========================================
  Files        1268     1268              
  Lines       59986    60087     +101     
  Branches     5486     5508      +22     
==========================================
+ Hits        24893    24980      +87     
- Misses      33938    33948      +10     
- Partials     1155     1159       +4

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

github-actions · 2024-07-09T01:48:53Z

Checkmarx One – Scan Summary & Details – 18a868e1-9aa8-4416-bdf1-b34dbf2c925f

New Issues

Issue	Source File / Package	Checkmarx Insight
CSRF	/src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 110	Attack Vector
CSRF	/src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 133	Attack Vector
Privacy_Violation	/src/Api/Auth/Controllers/AccountsController.cs: 846	Attack Vector
Privacy_Violation	/src/Api/Auth/Controllers/AccountsController.cs: 548	Attack Vector
Privacy_Violation	/src/Api/Auth/Controllers/AccountsController.cs: 828	Attack Vector
Privacy_Violation	/src/Api/Vault/Controllers/CiphersController.cs: 961	Attack Vector
Privacy_Violation	/src/Api/Auth/Controllers/AccountsController.cs: 411	Attack Vector
Privacy_Violation	/src/Api/Controllers/DevicesController.cs: 129	Attack Vector
Privacy_Violation	/src/Api/Auth/Controllers/TwoFactorController.cs: 444	Attack Vector
Privacy_Violation	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 260	Attack Vector
Privacy_Violation	/src/Api/Controllers/DevicesController.cs: 155	Attack Vector
Privacy_Violation	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 429	Attack Vector
Privacy_Violation	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 376	Attack Vector
Log_Forging	/src/Api/Vault/Controllers/CiphersController.cs: 222	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 157	Attack Vector
Log_Forging	/src/Api/Vault/Controllers/CiphersController.cs: 953	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 122	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 104	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 131	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 148	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 838	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 94	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 188	Attack Vector
Log_Forging	/src/Api/Controllers/DevicesController.cs: 120	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 540	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 820	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 206	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 245	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 254	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 263	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 280	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 289	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 297	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 350	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 369	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 380	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/TwoFactorController.cs: 402	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 403	Attack Vector
Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 240	Attack Vector
Log_Forging	/src/Api/Controllers/DevicesController.cs: 146	Attack Vector
Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 404	Attack Vector
Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 341	Attack Vector

Fixed Issues

Severity	Issue	Source File / Package
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 365
	CSRF	/src/Api/Public/Controllers/CollectionsController.cs: 87
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 358
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 358
	CSRF	/src/Api/Controllers/CollectionsController.cs: 171
	CSRF	/src/Api/Controllers/CollectionsController.cs: 143
	CSRF	/bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 98
	CSRF	/bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 88
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 148
	CSRF	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 148
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 583
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 583
	Privacy_Violation	/src/Core/Auth/UserFeatures/UserMasterPassword/SetInitialMasterPasswordCommand.cs: 59
	Privacy_Violation	/src/Core/Auth/UserFeatures/UserMasterPassword/SetInitialMasterPasswordCommand.cs: 59
	Privacy_Violation	/src/Api/AdminConsole/Controllers/OrganizationAuthRequestsController.cs: 59
	Log_Forging	/src/Api/Vault/Controllers/CiphersController.cs: 583
	Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 245
	Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 365

… ac/ac-2847

r-tome

Looks great to me! Nicely detailed unit tests

eliykat · 2024-07-12T01:30:05Z

@r-tome I've made some further changes to future-proof this, please see discussion on the ticket.

r-tome

Great work!

…4479) * refactor controller logic * add additional validation checks to update commands * refactor and improve tests (cherry picked from commit 5df0e21)

eliykat added 4 commits July 9, 2024 09:17

Move PUT tests into separate file

4ff5442

Fix sut and add tests

c51d2bc

Simplify tests

2ceb86e

Dont require list when Ienumerable will do

9e9b4a3

eliykat changed the title ~~[AC-2847] Simplify OrganizationUser and Group PUT methods and improve tests~~ [AC-2847] Simplify OrganizationUser and Group PUT methods and tests Jul 9, 2024

eliykat added 4 commits July 9, 2024 12:08

Tests: remove null default

402a1be

Refine existing GroupController tests

f30a70d

Add test

7bf8ac6

Add test

8938175

eliykat marked this pull request as ready for review July 9, 2024 03:41

eliykat requested a review from a team as a code owner July 9, 2024 03:41

eliykat requested a review from r-tome July 9, 2024 03:41

eliykat and others added 3 commits July 9, 2024 13:41

Merge branch 'main' into ac/ac-2847

fd7df92

dotnet format

0de626b

Merge branch 'ac/ac-2847' of https://github.com/bitwarden/server into…

7542658

… ac/ac-2847

r-tome previously approved these changes Jul 9, 2024

View reviewed changes

eliykat added the needs-qa label Jul 9, 2024

eliykat and others added 3 commits July 11, 2024 11:30

Merge branch 'main' into ac/ac-2847

1a61ffa

Merge remote-tracking branch 'origin/main' into ac/ac-2847

f9a95dd

Verify orgIds

790d7c1

eliykat dismissed r-tome’s stale review via 790d7c1 July 11, 2024 23:29

eliykat added 3 commits July 12, 2024 10:31

Improve group validation

836e4bb

Refactor to return invalid ids

9438ce8

Add comment

b5194e4

eliykat requested a review from r-tome July 12, 2024 01:29

r-tome previously approved these changes Jul 12, 2024

View reviewed changes

bwdil previously approved these changes Jul 12, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into ac/ac-2847

1009e97

eliykat dismissed stale reviews from bwdil and r-tome via 1009e97 July 15, 2024 22:11

eliykat added 3 commits July 16, 2024 08:50

Use NotFoundExceptions

1f73877

Fix error messages

3bdf34c

Tweaks

251714b

eliykat requested a review from r-tome July 15, 2024 23:39

Fix test

c706d2e

eliykat removed the needs-qa label Jul 16, 2024

r-tome approved these changes Jul 16, 2024

View reviewed changes

eliykat merged commit 5df0e21 into main Jul 16, 2024
49 of 50 checks passed

eliykat deleted the ac/ac-2847 branch July 16, 2024 00:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AC-2847] Simplify OrganizationUser and Group PUT methods and tests #4479

[AC-2847] Simplify OrganizationUser and Group PUT methods and tests #4479

eliykat commented Jul 9, 2024 •

edited

Loading

codecov bot commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading

r-tome left a comment

eliykat commented Jul 12, 2024

r-tome left a comment

[AC-2847] Simplify OrganizationUser and Group PUT methods and tests #4479

[AC-2847] Simplify OrganizationUser and Group PUT methods and tests #4479

Conversation

eliykat commented Jul 9, 2024 • edited Loading

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

codecov bot commented Jul 9, 2024 • edited Loading

Codecov Report

github-actions bot commented Jul 9, 2024 • edited Loading

New Issues

Fixed Issues

r-tome left a comment

Choose a reason for hiding this comment

eliykat commented Jul 12, 2024

r-tome left a comment

Choose a reason for hiding this comment

eliykat commented Jul 9, 2024 •

edited

Loading

codecov bot commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading