[PM-24192] Move account recovery logic to command #6184
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6184 +/- ##
==========================================
+ Coverage 51.86% 51.98% +0.11%
==========================================
Files 1902 1907 +5
Lines 84058 84330 +272
Branches 7501 7530 +29
==========================================
+ Hits 43598 43840 +242
- Misses 38766 38784 +18
- Partials 1694 1706 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
|
Reverting to draft while I implement some changes requested by Product. Specifically, we want to alert the user before they submit the dialog, so I intend to add a prevalidate endpoint to check permissions at the time the dialog opens. This avoids additional load on the Members endpoint when the page first loads. |
- Add test for when target user is not a provider (should succeed) - Add test for when calling user is a provider (should succeed) - Ensures complete coverage of provider permission scenarios in AdminRecoverAccountCommand
eliykat
force-pushed
the
ac/pm-24192/server-prevent-organizations-from-recovering-a-provider-account
branch
from
d311b39 to
ab76446
Compare
eliykat
changed the base branch from
main
to
ac/pm-24192/organization-context
…m-recovering-a-provider-account
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
JimmyVo16
left a comment
JimmyVo16
left a comment
There was a problem hiding this comment.
Some questions, but no blockers.
|
|
||
| public async Task InitializeAsync() | ||
| { | ||
| _ownerEmail = $"org-user-reset-password-integration-test-{Guid.NewGuid()}@bitwarden.com"; |
There was a problem hiding this comment.
Don’t we normally use example.com for test mock-ups?
There was a problem hiding this comment.
Yes we do and yes we should!
| var targetUserProviderUsers = | ||
| await providerUserRepository.GetManyByUserAsync(targetOrganizationUser.UserId.Value); | ||
|
|
||
| if (targetUserProviderUsers.Any(providerUser => !currentContext.ProviderUser(providerUser.ProviderId))) |
There was a problem hiding this comment.
Just to confirm my understanding: if this user is a provider in any reseller, then we cannot reset their password?
There was a problem hiding this comment.
The business rule is: to recover a provider user's account, you must also be a member for the same provider(s).
In effect, this code iterates through the target user's ProviderUser associations and calls currentContext to make sure the current user is also a member for the same providers.
If the current user is not a member for the same providers, that indicates a privilege escalation so we block it.
There was a problem hiding this comment.
I've added a comment and some xmldoc on ICurrentContext to clarify this a little.
| throw new NotFoundException(); | ||
| } | ||
|
|
||
| var authorizationResult = await _authorizationService.AuthorizeAsync(User, targetOrganizationUser, new RecoverAccountAuthorizationRequirement()); |
There was a problem hiding this comment.
Just thinking out loud: this looks like it could be in a middleware that we can attach to the controller method. That way, it's cleaner and we can add it to multiple places. In some languages, you can even hydrate the request object in the middleware so the data can be passed along to other middlewares.
There was a problem hiding this comment.
This is definitely doable, but at this stage we have no cause to reuse this elsewhere, and the logic is already encapsulated in the handler. I think it's clearer to make the explicit call. As you've found with your logging case, moving logic into the middleware isn't always very clear to the reader.
That said, Billing Team do have a neat attribute that fetches and injects the organization for the current request: https://github.com/bitwarden/server/blob/main/src/Api/Billing/Controllers/VNext/OrganizationBillingVNextController.cs#L36-L44. We could do something similar to get the orgUser, make sure they exist, and make sure they match the current organization - given that that is a very common but critical bit of boilerplate. I've already fussed this PR a lot though so I'd prefer to leave it for future experimentation.
- Renamed method from PutResetPasswordvNext to PutResetPasswordNew following C# naming conventions │ - Changed visibility from public to private since authorization is handled by the calling method PutResetPassword │ - Removed [FromBody] attribute as it's not needed for private methods
- Return TypedResults.NotFound() instead of throwing NotFoundException - Return TypedResults.BadRequest(ModelState) instead of throwing BadRequestException - Applies to both PutResetPassword and PutResetPasswordNew methods
- Added #nullable enable before the method - Added #nullable restore after the method - Keeps file-level #nullable disable intact for legacy code
- Tests for legacy path when feature flag is disabled - Tests for new path when feature flag is enabled - Tests for authorization failures returning BadRequest - Tests for organization user not found scenarios - Tests for successful and failed account recovery
- Keep attribute for future when this becomes a public endpoint
- Changed #nullable restore to #nullable disable - Added note about partial migration for future developers
…event-organizations-from-recovering-a-provider-account
|
I will take a final look at Claude's feedback tomorrow, but all major issues have been addressed. |
This also allows more detailed authorization failure messages because we are guaranteed an order of execution within the same handler.
|
@JimmyVo16 This is ready for review. The main change I've made is to combine the 2 authorization handlers - the logic is very similar, but now it runs in a single handler in sequence, rather than both handlers being executed separately. I ultimately decided this was clearer than 2 handlers, and allows for more descriptive error messages for each failure case. Claude's latest review failed because we hit our usage limits. You can see the last review in the comment history, but everything there seems unnecessary or wrong - let me know if you disagree. |
eliykat
deleted the
ac/pm-24192/server-prevent-organizations-from-recovering-a-provider-account
branch
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-24192
📔 Objective
Move the recover account logic out of
UserServiceand into its own command. See ticket for more information.This is generally a lift & shift, except for the new
CheckProviderPermissionsAsyncprivate method, and tests (which are all new).📸 Screenshots
⏰ Reminders before review
🦮 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