[pm-15621] Refactor DeleteManagedOrganizationUserAccountCommand

[JimmyVo16]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-15621

📔 Objective

1. Refactored DeleteUserAsync to call DeleteManyUsersAsync.
2. Removed the logic for deleting an organization. Since all organizations require at least one owner, EnsureUserIsNotSoleOrganizationOwnerAsync prevents the code from reaching this point.
3. Separated validation from canceling membership.

📸 Screenshots

I think test coverage is sufficient here, and it's low risk since it's behind a feature, but I did do a quick smoke test by starting up the service.

⏰ 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]

Codecov Report

Attention: Patch coverage is 94.44444% with 5 lines in your changes missing coverage. Please review.

Project coverage is 44.50%. Comparing base (bde11da) to head (ed7a3ef).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
...ers/DeleteManagedOrganizationUserAccountCommand.cs	94.44%	2 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5345      +/-   ##
==========================================
+ Coverage   44.30%   44.50%   +0.19%     
==========================================
  Files        1497     1500       +3     
  Lines       69212    69655     +443     
  Branches     6241     6260      +19     
==========================================
+ Hits        30665    30997     +332     
- Misses      37224    37337     +113     
+ Partials     1323     1321       -2     

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

[github-actions]

Checkmarx One – Scan Summary & Details – b8e8e402-b3b5-438d-8596-dd19a268e060

New Issues (11)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	Privacy_Violation	/src/Core/Auth/Services/Implementations/AuthRequestService.cs: 221	details Method UpdateAuthRequestAsync at line 221 of /src/Core/Auth/Services/Implementations/AuthRequestService.cs sends user information outside the appli... Attack Vector
	Privacy_Violation	/src/Core/Auth/Services/Implementations/AuthRequestService.cs: 230	details Method UpdateAuthRequestAsync at line 230 of /src/Core/Auth/Services/Implementations/AuthRequestService.cs sends user information outside the appli... Attack Vector
	Privacy_Violation	/src/Core/Auth/Services/Implementations/AuthRequestService.cs: 173	details Method CreateAuthRequestAsync at line 173 of /src/Core/Auth/Services/Implementations/AuthRequestService.cs sends user information outside the appli... Attack Vector
	Privacy_Violation	/src/Core/NotificationHub/NotificationHubPushNotificationService.cs: 192	details Method PushAuthRequestAsync at line 192 of /src/Core/NotificationHub/NotificationHubPushNotificationService.cs sends user information outside the a... Attack Vector
	Heap_Inspection	/src/Core/Constants.cs: 169	details Method at line 169 of /src/Core/Constants.cs defines EnablePasswordManagerSyncAndroid, which is designated to contain user passwords. However, whi... Attack Vector
	Heap_Inspection	/src/Core/Constants.cs: 170	details Method at line 170 of /src/Core/Constants.cs defines EnablePasswordManagerSynciOS, which is designated to contain user passwords. However, while p... Attack Vector
	Log_Forging	/src/Api/Vault/Controllers/CiphersController.cs: 173	details Method PostAdmin at line 173 of /src/Api/Vault/Controllers/CiphersController.cs gets user input from element model. This element’s value flows thro... Attack Vector
	Log_Forging	/src/Api/Platform/Push/Controllers/PushController.cs: 75	details Method PostSend at line 75 of /src/Api/Platform/Push/Controllers/PushController.cs gets user input from element model. This element’s value flows t... Attack Vector
	Log_Forging	/src/Api/Vault/Controllers/CiphersController.cs: 158	details Method PostCreate at line 158 of /src/Api/Vault/Controllers/CiphersController.cs gets user input from element model. This element’s value flows thr... Attack Vector
	Use_Of_Hardcoded_Password	/src/Core/Constants.cs: 170	details The application uses the hard-coded password EnablePasswordManagerSynciOS for authentication purposes, either using it to verify users' identities,... Attack Vector
	Use_Of_Hardcoded_Password	/src/Core/Constants.cs: 169	details The application uses the hard-coded password EnablePasswordManagerSyncAndroid for authentication purposes, either using it to verify users' identit... Attack Vector

Fixed Issues (8)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	CSRF	/src/Billing/Controllers/BitPayController.cs: 56
	Privacy_Violation	/src/Core/Auth/Services/Implementations/AuthRequestService.cs: 230
	Privacy_Violation	/src/Core/Auth/Services/Implementations/AuthRequestService.cs: 221
	Privacy_Violation	/src/Core/Auth/Services/Implementations/AuthRequestService.cs: 173
	Privacy_Violation	/src/Core/NotificationHub/NotificationHubPushNotificationService.cs: 246
	Log_Forging	/src/Api/Vault/Controllers/CiphersController.cs: 173
	Log_Forging	/src/Api/Platform/Push/Controllers/PushController.cs: 75
	Log_Forging	/src/Api/Vault/Controllers/CiphersController.cs: 158

[eliykat]

Nice work @JimmyVo16 , I think this is an improvement. I also have some thoughts about the try/catch blocks here which I shared in our dev channel - I wanted to run it past the team before asking for it here, but let me know what you think.

[BTreston]

There are some failing tests in DeleteManagedOrganizationUserAccountCommandTests

[eliykat]

Just a few minor items.

I think the userDeletionResults tuple makes sense (that is, we need to match these objects up and then pass them around together), but is very verbose and awkward. Definitely a case for better/stronger typed ValidationResult classes, which we should experiment with in the future (whoever gets there first, really).

[eliykat]

This overlaps with PreventOrganizationSoleOwnerDeletion (maybe what you meant to refer to in your comment above?)

• PreventOrganizationSoleOwnerDeletion prevents you from deleting the sole owner of this organization
• EnsureUserIsNotSoleOrganizationOwnerAsync prevents you from deleting the sole owner of any organization

I suggest we can just rely on the latter given that it obviously includes the former. Both also require database calls, and we don't want to hit the db unnecessarily.

I also question whether PreventOrganizationSoleOwnerDeletion would ever be tripped, given that you can't delete yourself and only owners can delete other owners. If you're the sole owner, who would be able to delete you?

[JimmyVo16]

After much thinking, yeah i think about right. EnsureUserIsNotSoleOrganizationOwnerAsync should handle all cases for use, so we can remove PreventOrganizationSoleOwnerDeletion.

(me thinking out loud) These logics are pretty complex. I wonder if there is a way to make them discoverable than deep in a validation method.

[eliykat]

(me thinking out loud) These logics are pretty complex. I wonder if there is a way to make them discoverable than deep in a validation method.

I would say validation is the primary source of complexity in our commands (and in old service code that should be commands). It's definitely a strong argument for making this code as simple and as easy to follow as possible.

In terms of discoverability/location, I believe Jared had previously mooted separate Validator classes which encapsulate the validation logic for a given command. Not sure if that makes it more discoverable, but it would provide a stronger separation than private methods.

[eliykat]

Also - tests are failing. At a glance, maybe just because the order of validation has changed?

[eliykat]

Great work @JimmyVo16 !

EDIT: putting a needs-qa label on it because it needs QA before merge. However it would be ideal if QA can hit it before we release the larger feature, can you please mention this to them?

I addressed Brandon's comment, and I think he's out today, so I don't want this to block QA.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[JimmyVo16]

Update: QA found an edge case that I missed.

Changes:

1. For single user deletion, throw an exception if there is an error like the existing behavior.
2. Add test coverage for this.
3. Create InternalDeleteManyUsersAsync that would handle all the deletion logic then have DeleteManyUsersAsync and DeleteUserAsync handle the interfaces their clients expects.

Testing for the edge case

Testing.for.self.delete.mov

Regression testing

Happy.path.for.multiple.user.deletions.mov

happy.path.for.single.user.deletion.mov