-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[PM-24192] Move account recovery logic to command #6184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
eliykat
merged 53 commits into
main
from
ac/pm-24192/server-prevent-organizations-from-recovering-a-provider-account
Oct 31, 2025
Merged
Changes from all commits
Commits
Show all changes
53 commits
Select commit
Hold shift + click to select a range
dcfe1fd
Initial commit: OrganizationContext class
eliykat 7137dee
Move caching to class level, add tests
eliykat a8103f6
Tweaks
eliykat 6b233fb
Fix test
eliykat fab969a
Move existing logic into command
eliykat 8e224f7
Add tests
eliykat b0adf4a
Add provider check
eliykat 1d41fbf
Fix method name to be consistent with command
eliykat 39e09e5
Fix test naming
eliykat 89053ff
register in DI
eliykat a094aa6
Remove unnecessary db call
eliykat 1eb1942
Update error string
eliykat 115ddc5
Add missing test coverage for CheckProviderPermissionsAsync logic
eliykat 1f1e358
Make methods static
eliykat 12895fa
WIP refactor: use authorization handler
eliykat 856aa22
Refactor handler to use new OrganizationContext service
eliykat c37b3b3
Review and refactor tests
eliykat ab76446
Consolidate test cases
eliykat ee337e4
Fix merge conflict resolution errors
eliykat 2ade117
Revert old code, feature flag changes
eliykat fbd479e
Revert unused declaration
eliykat a91546e
Tweak comment
eliykat 2ffef5c
Merge remote-tracking branch 'origin/main' into ac/pm-24192/server-pr…
eliykat f70b04c
Fix bug: could be a provider for any org
eliykat d449e24
Merge remote-tracking branch 'origin/main' into ac/pm-24192/server-pr…
eliykat 563d9e8
Split handlers
eliykat 31fdd9d
Merge remote-tracking branch 'origin/main' into ac/pm-24192/server-pr…
eliykat c14552c
undo unrelated change
eliykat deb8dac
tweaks
eliykat 37b751d
Split handlers
eliykat 38daec2
Build out tests
eliykat 684138a
remove unrelated files
eliykat 59dd29d
dotnet format
eliykat a2a6f38
api integration tests
eliykat 69282c7
dotnet format
eliykat 4936e6c
Merge branch 'main' into ac/pm-24192/server-prevent-organizations-fro…
eliykat 3dcc79a
Rename PutResetPasswordvNext to PutResetPasswordNew and make private …
eliykat d6d6d99
Replace exceptions with TypedResults in reset password methods
eliykat 585f952
Enable nullable reference types for PutResetPasswordNew method
eliykat 4ef1097
Add unit tests for PutResetPassword method
eliykat a2b3d7b
Restore [FromBody] attribute to PutResetPasswordNew method
eliykat e535e12
Fix nullable directive and add partial migration note
eliykat aea2dab
use example.com domain
eliykat 040ba54
Add ICurrentContext xmldoc
eliykat c29374b
Add more comments to make Claude happy
eliykat 1b1b7c7
Fix imports
eliykat 85fabd7
Update tests to match expected behavior
eliykat 600186e
Merge remote-tracking branch 'origin/main' into ac/pm-24192/server-pr…
eliykat aff648e
Fix test failures
eliykat 467e4e5
Combine authorization handlers to clarify code flow
eliykat 5f463fc
Add comment
eliykat facecce
dotnet format
eliykat 1722a57
Refer to provider member rather than provider
eliykat File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
110 changes: 110 additions & 0 deletions
110
src/Api/AdminConsole/Authorization/RecoverAccountAuthorizationHandler.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| using System.Security.Claims; | ||
| using Bit.Core.AdminConsole.Repositories; | ||
| using Bit.Core.Context; | ||
| using Bit.Core.Entities; | ||
| using Bit.Core.Enums; | ||
| using Microsoft.AspNetCore.Authorization; | ||
|
|
||
| namespace Bit.Api.AdminConsole.Authorization; | ||
|
|
||
| /// <summary> | ||
| /// An authorization requirement for recovering an organization member's account. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Note: this is different to simply being able to manage account recovery. The user must be recovering | ||
| /// a member who has equal or lesser permissions than them. | ||
| /// </remarks> | ||
| public class RecoverAccountAuthorizationRequirement : IAuthorizationRequirement; | ||
|
|
||
| /// <summary> | ||
| /// Authorizes members and providers to recover a target OrganizationUser's account. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This prevents privilege escalation by ensuring that a user cannot recover the account of | ||
| /// another user with a higher role or with provider membership. | ||
| /// </remarks> | ||
| public class RecoverAccountAuthorizationHandler( | ||
| IOrganizationContext organizationContext, | ||
| ICurrentContext currentContext, | ||
| IProviderUserRepository providerUserRepository) | ||
| : AuthorizationHandler<RecoverAccountAuthorizationRequirement, OrganizationUser> | ||
| { | ||
| public const string FailureReason = "You are not permitted to recover this user's account."; | ||
| public const string ProviderFailureReason = "You are not permitted to recover a Provider member's account."; | ||
|
|
||
| protected override async Task HandleRequirementAsync(AuthorizationHandlerContext context, | ||
| RecoverAccountAuthorizationRequirement requirement, | ||
| OrganizationUser targetOrganizationUser) | ||
| { | ||
| // Step 1: check that the User has permissions with respect to the organization. | ||
| // This may come from their role in the organization or their provider relationship. | ||
| var canRecoverOrganizationMember = | ||
| AuthorizeMember(context.User, targetOrganizationUser) || | ||
| await AuthorizeProviderAsync(context.User, targetOrganizationUser); | ||
|
|
||
| if (!canRecoverOrganizationMember) | ||
| { | ||
| context.Fail(new AuthorizationFailureReason(this, FailureReason)); | ||
| return; | ||
| } | ||
|
|
||
| // Step 2: check that the User has permissions with respect to any provider the target user is a member of. | ||
| // This prevents an organization admin performing privilege escalation into an unrelated provider. | ||
| var canRecoverProviderMember = await CanRecoverProviderAsync(targetOrganizationUser); | ||
| if (!canRecoverProviderMember) | ||
| { | ||
| context.Fail(new AuthorizationFailureReason(this, ProviderFailureReason)); | ||
| return; | ||
| } | ||
|
|
||
| context.Succeed(requirement); | ||
| } | ||
|
|
||
| private async Task<bool> AuthorizeProviderAsync(ClaimsPrincipal currentUser, OrganizationUser targetOrganizationUser) | ||
| { | ||
| return await organizationContext.IsProviderUserForOrganization(currentUser, targetOrganizationUser.OrganizationId); | ||
| } | ||
|
|
||
| private bool AuthorizeMember(ClaimsPrincipal currentUser, OrganizationUser targetOrganizationUser) | ||
| { | ||
| var currentContextOrganization = organizationContext.GetOrganizationClaims(currentUser, targetOrganizationUser.OrganizationId); | ||
| if (currentContextOrganization == null) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Current user must have equal or greater permissions than the user account being recovered | ||
| var authorized = targetOrganizationUser.Type switch | ||
| { | ||
| OrganizationUserType.Owner => currentContextOrganization.Type is OrganizationUserType.Owner, | ||
| OrganizationUserType.Admin => currentContextOrganization.Type is OrganizationUserType.Owner or OrganizationUserType.Admin, | ||
| _ => currentContextOrganization is | ||
| { Type: OrganizationUserType.Owner or OrganizationUserType.Admin } | ||
| or { Type: OrganizationUserType.Custom, Permissions.ManageResetPassword: true } | ||
| }; | ||
|
|
||
| return authorized; | ||
| } | ||
|
|
||
| private async Task<bool> CanRecoverProviderAsync(OrganizationUser targetOrganizationUser) | ||
| { | ||
| if (!targetOrganizationUser.UserId.HasValue) | ||
| { | ||
| // If an OrganizationUser is not linked to a User then it can't be linked to a Provider either. | ||
| // This is invalid but does not pose a privilege escalation risk. Return early and let the command | ||
| // handle the invalid input. | ||
| return true; | ||
| } | ||
|
|
||
| var targetUserProviderUsers = | ||
| await providerUserRepository.GetManyByUserAsync(targetOrganizationUser.UserId.Value); | ||
|
|
||
| // If the target user belongs to any provider that the current user is not a member of, | ||
| // deny the action to prevent privilege escalation from organization to provider. | ||
| // Note: we do not expect that a user is a member of more than 1 provider, but there is also no guarantee | ||
| // against it; this returns a sequence, so we handle the possibility. | ||
| var authorized = targetUserProviderUsers.All(providerUser => currentContext.ProviderUser(providerUser.ProviderId)); | ||
| return authorized; | ||
| } | ||
| } | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
79 changes: 79 additions & 0 deletions
79
src/Core/AdminConsole/OrganizationFeatures/AccountRecovery/AdminRecoverAccountCommand.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| using Bit.Core.AdminConsole.Enums; | ||
| using Bit.Core.AdminConsole.Repositories; | ||
| using Bit.Core.Entities; | ||
| using Bit.Core.Enums; | ||
| using Bit.Core.Exceptions; | ||
| using Bit.Core.Platform.Push; | ||
| using Bit.Core.Repositories; | ||
| using Bit.Core.Services; | ||
| using Microsoft.AspNetCore.Identity; | ||
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.AccountRecovery; | ||
|
|
||
| public class AdminRecoverAccountCommand(IOrganizationRepository organizationRepository, | ||
| IPolicyRepository policyRepository, | ||
| IUserRepository userRepository, | ||
| IMailService mailService, | ||
| IEventService eventService, | ||
| IPushNotificationService pushNotificationService, | ||
| IUserService userService, | ||
| TimeProvider timeProvider) : IAdminRecoverAccountCommand | ||
| { | ||
| public async Task<IdentityResult> RecoverAccountAsync(Guid orgId, | ||
| OrganizationUser organizationUser, string newMasterPassword, string key) | ||
| { | ||
| // Org must be able to use reset password | ||
| var org = await organizationRepository.GetByIdAsync(orgId); | ||
| if (org == null || !org.UseResetPassword) | ||
| { | ||
| throw new BadRequestException("Organization does not allow password reset."); | ||
| } | ||
|
|
||
| // Enterprise policy must be enabled | ||
| var resetPasswordPolicy = | ||
| await policyRepository.GetByOrganizationIdTypeAsync(orgId, PolicyType.ResetPassword); | ||
| if (resetPasswordPolicy == null || !resetPasswordPolicy.Enabled) | ||
| { | ||
| throw new BadRequestException("Organization does not have the password reset policy enabled."); | ||
| } | ||
|
|
||
| // Org User must be confirmed and have a ResetPasswordKey | ||
| if (organizationUser == null || | ||
| organizationUser.Status != OrganizationUserStatusType.Confirmed || | ||
| organizationUser.OrganizationId != orgId || | ||
| string.IsNullOrEmpty(organizationUser.ResetPasswordKey) || | ||
| !organizationUser.UserId.HasValue) | ||
| { | ||
| throw new BadRequestException("Organization User not valid"); | ||
| } | ||
|
|
||
| var user = await userService.GetUserByIdAsync(organizationUser.UserId.Value); | ||
| if (user == null) | ||
| { | ||
| throw new NotFoundException(); | ||
| } | ||
|
|
||
| if (user.UsesKeyConnector) | ||
| { | ||
| throw new BadRequestException("Cannot reset password of a user with Key Connector."); | ||
| } | ||
|
|
||
| var result = await userService.UpdatePasswordHash(user, newMasterPassword); | ||
| if (!result.Succeeded) | ||
| { | ||
| return result; | ||
| } | ||
|
|
||
| user.RevisionDate = user.AccountRevisionDate = timeProvider.GetUtcNow().UtcDateTime; | ||
| user.LastPasswordChangeDate = user.RevisionDate; | ||
| user.ForcePasswordReset = true; | ||
| user.Key = key; | ||
|
|
||
| await userRepository.ReplaceAsync(user); | ||
| await mailService.SendAdminResetPasswordEmailAsync(user.Email, user.Name, org.DisplayName()); | ||
| await eventService.LogOrganizationUserEventAsync(organizationUser, EventType.OrganizationUser_AdminResetPassword); | ||
| await pushNotificationService.PushLogOutAsync(user.Id); | ||
|
|
||
| return IdentityResult.Success; | ||
| } | ||
| } |
24 changes: 24 additions & 0 deletions
24
src/Core/AdminConsole/OrganizationFeatures/AccountRecovery/IAdminRecoverAccountCommand.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| using Bit.Core.Entities; | ||
| using Bit.Core.Exceptions; | ||
| using Microsoft.AspNetCore.Identity; | ||
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.AccountRecovery; | ||
|
|
||
| /// <summary> | ||
| /// A command used to recover an organization user's account by an organization admin. | ||
| /// </summary> | ||
| public interface IAdminRecoverAccountCommand | ||
| { | ||
| /// <summary> | ||
| /// Recovers an organization user's account by resetting their master password. | ||
| /// </summary> | ||
| /// <param name="orgId">The organization the user belongs to.</param> | ||
| /// <param name="organizationUser">The organization user being recovered.</param> | ||
| /// <param name="newMasterPassword">The user's new master password hash.</param> | ||
| /// <param name="key">The user's new master-password-sealed user key.</param> | ||
| /// <returns>An IdentityResult indicating success or failure.</returns> | ||
| /// <exception cref="BadRequestException">When organization settings, policy, or user state is invalid.</exception> | ||
| /// <exception cref="NotFoundException">When the user does not exist.</exception> | ||
| Task<IdentityResult> RecoverAccountAsync(Guid orgId, OrganizationUser organizationUser, | ||
| string newMasterPassword, string key); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.