diff --git a/src/Admin/Auth/Jobs/DeleteExpiredTwoFactorRememberTokensJob.cs b/src/Admin/Auth/Jobs/DeleteExpiredTwoFactorRememberTokensJob.cs index 5427368491ed..b45d38978eb8 100644 --- a/src/Admin/Auth/Jobs/DeleteExpiredTwoFactorRememberTokensJob.cs +++ b/src/Admin/Auth/Jobs/DeleteExpiredTwoFactorRememberTokensJob.cs @@ -6,7 +6,7 @@ namespace Bit.Admin.Auth.Jobs; /// -/// Clears out remember-token rows whose expiry has passed. Hygiene rather than correctness — an +/// Clears out `TwoFactorRememberToken` rows whose expiry has passed. Hygiene rather than correctness — an /// expired row is refused at validation time regardless, the row count is bounded by the number of /// devices, and the expiry index makes the sweep cheap. Weekly is sufficient. /// diff --git a/src/Core/Auth/Entities/TwoFactorRememberToken.cs b/src/Core/Auth/Entities/TwoFactorRememberToken.cs index d0274fe6f811..b91e4d17902c 100644 --- a/src/Core/Auth/Entities/TwoFactorRememberToken.cs +++ b/src/Core/Auth/Entities/TwoFactorRememberToken.cs @@ -5,7 +5,7 @@ namespace Bit.Core.Auth.Entities; /// -/// Server-side state backing a single remembered device's two-factor "remember me" token. +/// Server-side state backing the 2FA remember token issued to a single device. /// One row per (, ). /// /// @@ -19,12 +19,12 @@ public class TwoFactorRememberToken : ITableObject /// /// The owning user. Derivable through , but stored directly so that - /// revoking every remembered device for one user is an index seek. + /// revoking every 2FA remember token for one user is an index seek. /// public Guid UserId { get; set; } /// - /// The remembered device. Note this is Device.Id, not the client-supplied device identifier. + /// The device the 2FA remember token was issued to. Note this is Device.Id, not the client-supplied device identifier. /// public Guid DeviceId { get; set; } @@ -35,7 +35,7 @@ public class TwoFactorRememberToken : ITableObject public string Stamp { get; set; } = null!; /// - /// When this device was first remembered. Preserved when the row is re-issued. + /// When a 2FA remember token was first issued for this device. Preserved when the row is re-issued. /// public DateTime CreationDate { get; set; } = DateTime.UtcNow; diff --git a/src/Core/Auth/Models/Business/Tokenables/TwoFactorRememberTokenable.cs b/src/Core/Auth/Models/Business/Tokenables/TwoFactorRememberTokenable.cs index 43fb86ccf063..833665b2f0b3 100644 --- a/src/Core/Auth/Models/Business/Tokenables/TwoFactorRememberTokenable.cs +++ b/src/Core/Auth/Models/Business/Tokenables/TwoFactorRememberTokenable.cs @@ -51,7 +51,7 @@ public class TwoFactorRememberTokenable : ExpiringTokenable public string Stamp { get; internal set; } = null!; // TODO: PM-44171 - Consider removing SecurityStamp from this token. The row's Stamp already - // covers the same ground more precisely, and carrying the user stamp ties remember-me lifetime + // covers the same ground more precisely, and carrying the user stamp ties 2FA remember token lifetime // to every user stamp rotation. Any removal has to stay backwards compatible: tokens already // issued carry this field, so treat it as optional on read before dropping it. /// diff --git a/src/Core/Auth/Repositories/ITwoFactorRememberTokenRepository.cs b/src/Core/Auth/Repositories/ITwoFactorRememberTokenRepository.cs index b29eb5f67d75..cb854cecc486 100644 --- a/src/Core/Auth/Repositories/ITwoFactorRememberTokenRepository.cs +++ b/src/Core/Auth/Repositories/ITwoFactorRememberTokenRepository.cs @@ -9,7 +9,7 @@ namespace Bit.Core.Auth.Repositories; public interface ITwoFactorRememberTokenRepository { /// - /// Reads the row for one remembered device, or null if the device has never been remembered. + /// Reads the row for the 2FA remember token issued to one device, or null if none has ever been issued for it. /// Task GetByUserIdDeviceIdAsync(Guid userId, Guid deviceId); @@ -21,7 +21,7 @@ public interface ITwoFactorRememberTokenRepository Task UpsertAsync(TwoFactorRememberToken token); /// - /// Rotates the Stamp on every row for this user, invalidating all of their remember + /// Rotates the Stamp on every row for this user, invalidating all of their 2FA remember /// tokens at once. Rows are kept. /// /// diff --git a/src/Core/Auth/UserFeatures/EmergencyAccess/EmergencyAccessService.cs b/src/Core/Auth/UserFeatures/EmergencyAccess/EmergencyAccessService.cs index 62853d398c9e..541aedfd928f 100644 --- a/src/Core/Auth/UserFeatures/EmergencyAccess/EmergencyAccessService.cs +++ b/src/Core/Auth/UserFeatures/EmergencyAccess/EmergencyAccessService.cs @@ -9,6 +9,7 @@ using Bit.Core.Auth.Enums; using Bit.Core.Auth.Models.Business.Tokenables; using Bit.Core.Auth.Models.Data; +using Bit.Core.Auth.UserFeatures.TwoFactorAuth; using Bit.Core.Auth.UserFeatures.UserMasterPassword.Data; using Bit.Core.Auth.UserFeatures.UserMasterPassword.Interfaces; using Bit.Core.Entities; @@ -41,6 +42,7 @@ public class EmergencyAccessService : IEmergencyAccessService private readonly IDataProtectorTokenFactory _dataProtectorTokenizer; private readonly IRemoveOrganizationUserCommand _removeOrganizationUserCommand; private readonly IPolicyRequirementQuery _policyRequirementQuery; + private readonly IRevokeTwoFactorRememberTokensCommand _revokeTwoFactorRememberTokensCommand; public EmergencyAccessService( IEmergencyAccessRepository emergencyAccessRepository, @@ -55,7 +57,8 @@ public EmergencyAccessService( GlobalSettings globalSettings, IDataProtectorTokenFactory dataProtectorTokenizer, IRemoveOrganizationUserCommand removeOrganizationUserCommand, - IPolicyRequirementQuery policyRequirementQuery) + IPolicyRequirementQuery policyRequirementQuery, + IRevokeTwoFactorRememberTokensCommand revokeTwoFactorRememberTokensCommand) { _emergencyAccessRepository = emergencyAccessRepository; _organizationUserRepository = organizationUserRepository; @@ -70,6 +73,7 @@ public EmergencyAccessService( _dataProtectorTokenizer = dataProtectorTokenizer; _removeOrganizationUserCommand = removeOrganizationUserCommand; _policyRequirementQuery = policyRequirementQuery; + _revokeTwoFactorRememberTokensCommand = revokeTwoFactorRememberTokensCommand; } public async Task InviteAsync(User grantorUser, string emergencyContactEmail, EmergencyAccessType accessType, int waitTime) @@ -402,6 +406,11 @@ public async Task FinishRecoveryTakeoverAsync( // Disable New Device Verification since it will otherwise block logins grantor.VerifyDevices = false; + // Takeover removes the grantor's 2FA, so their 2FA remember tokens must stop being honored too. + // The changes above are only in memory until the save below. Revoking first means a failed revoke + // persists nothing, so the stored grantor (including their 2FA) is unchanged and the takeover can be retried. + await _revokeTwoFactorRememberTokensCommand.RevokeAllForUserAsync(grantor.Id); + await _userRepository.ReplaceAsync(grantor); // Remove grantor from all organizations unless Owner @@ -437,6 +446,12 @@ public async Task PasswordAsync(Guid emergencyAccessId, User granteeUser, string grantor.SetTwoFactorProviders([]); // Disable New Device Verification since it will otherwise block logins grantor.VerifyDevices = false; + + // Takeover removes the grantor's 2FA, so their 2FA remember tokens must stop being honored too. + // The changes above are only in memory until the save below. Revoking first means a failed revoke + // persists nothing, so the stored grantor (including their 2FA) is unchanged and the takeover can be retried. + await _revokeTwoFactorRememberTokensCommand.RevokeAllForUserAsync(grantor.Id); + await _userRepository.ReplaceAsync(grantor); // Remove grantor from all organizations unless Owner diff --git a/src/Core/Auth/UserFeatures/TwoFactorAuth/IIssueTwoFactorRememberTokenCommand.cs b/src/Core/Auth/UserFeatures/TwoFactorAuth/IIssueTwoFactorRememberTokenCommand.cs index afe7961d66a7..b33f9612be37 100644 --- a/src/Core/Auth/UserFeatures/TwoFactorAuth/IIssueTwoFactorRememberTokenCommand.cs +++ b/src/Core/Auth/UserFeatures/TwoFactorAuth/IIssueTwoFactorRememberTokenCommand.cs @@ -5,12 +5,12 @@ namespace Bit.Core.Auth.UserFeatures.TwoFactorAuth; public interface IIssueTwoFactorRememberTokenCommand { /// - /// Records that this device is remembered and mints the token the client presents to skip the + /// Records a 2FA remember token for this device and mints the token the client presents to skip the /// two-factor challenge on its next login. /// /// /// Writing the row assigns it a fresh stamp, so any token previously issued for this device - /// stops being honored — one live remember token per device, newest wins. + /// stops being honored — one live 2FA remember token per device, newest wins. /// /// The protected token string, to be returned to the client verbatim. Task IssueAsync(User user, Device device); diff --git a/src/Core/Auth/UserFeatures/TwoFactorAuth/IRevokeTwoFactorRememberTokensCommand.cs b/src/Core/Auth/UserFeatures/TwoFactorAuth/IRevokeTwoFactorRememberTokensCommand.cs index 14976b2b18a6..ac1fcecdf811 100644 --- a/src/Core/Auth/UserFeatures/TwoFactorAuth/IRevokeTwoFactorRememberTokensCommand.cs +++ b/src/Core/Auth/UserFeatures/TwoFactorAuth/IRevokeTwoFactorRememberTokensCommand.cs @@ -3,12 +3,7 @@ public interface IRevokeTwoFactorRememberTokensCommand { /// - /// Stops every remember token this user holds from being honored, on all of their devices. + /// Stops every 2FA remember token recorded for this user from being honored, on all of their devices. /// - /// - /// Scoped to remember-me only: access tokens, refresh tokens, and live sessions are unaffected, - /// so this does not log the user out anywhere. Devices stay remembered as rows — they simply - /// require a two-factor challenge again on next login. - /// Task RevokeAllForUserAsync(Guid userId); } diff --git a/src/Core/Auth/UserFeatures/TwoFactorAuth/IValidateTwoFactorRememberTokenQuery.cs b/src/Core/Auth/UserFeatures/TwoFactorAuth/IValidateTwoFactorRememberTokenQuery.cs index 8ce9bbc64301..f8fa57e9cf6f 100644 --- a/src/Core/Auth/UserFeatures/TwoFactorAuth/IValidateTwoFactorRememberTokenQuery.cs +++ b/src/Core/Auth/UserFeatures/TwoFactorAuth/IValidateTwoFactorRememberTokenQuery.cs @@ -5,7 +5,7 @@ namespace Bit.Core.Auth.UserFeatures.TwoFactorAuth; public interface IValidateTwoFactorRememberTokenQuery { /// - /// Decides whether a presented remember token entitles this request to skip the two-factor + /// Decides whether a presented 2FA remember token entitles this request to skip the two-factor /// challenge. /// /// @@ -13,7 +13,7 @@ public interface IValidateTwoFactorRememberTokenQuery /// own command rather than folded in here. /// /// This validates the token only. Whether the user is required to complete two-factor at all is - /// the caller's decision, made before a remember token is considered. + /// the caller's decision, made before a 2FA remember token is considered. /// /// /// The authenticating user. diff --git a/src/Core/Auth/UserFeatures/TwoFactorAuth/Implementations/ResetUserTwoFactorCommand.cs b/src/Core/Auth/UserFeatures/TwoFactorAuth/Implementations/ResetUserTwoFactorCommand.cs index 9ef734e102c4..7f2e47844fd2 100644 --- a/src/Core/Auth/UserFeatures/TwoFactorAuth/Implementations/ResetUserTwoFactorCommand.cs +++ b/src/Core/Auth/UserFeatures/TwoFactorAuth/Implementations/ResetUserTwoFactorCommand.cs @@ -5,10 +5,14 @@ namespace Bit.Core.Auth.UserFeatures.TwoFactorAuth.Implementations; public class ResetUserTwoFactorCommand( IUserRepository userRepository, + IRevokeTwoFactorRememberTokensCommand revokeTwoFactorRememberTokensCommand, TimeProvider timeProvider) : IResetUserTwoFactorCommand { public async Task ResetAsync(User user) { + // Sits before ReplaceAsync so a failure here leaves 2FA intact and a retry re-runs both steps. + await revokeTwoFactorRememberTokensCommand.RevokeAllForUserAsync(user.Id); + user.TwoFactorProviders = null; user.TwoFactorRecoveryCode = null; user.RevisionDate = user.AccountRevisionDate = timeProvider.GetUtcNow().UtcDateTime; diff --git a/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md b/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md index 3ad1eeb8e2d9..4c5da3848187 100644 --- a/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md +++ b/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md @@ -2,6 +2,35 @@ This area of the codebase covers enrollment and management of the multi-factor authentication providers Bitwarden supports — Authenticator (TOTP), YubiKey OTP, Duo (personal and organization), WebAuthn, and Email — plus supporting flows such as recovery codes, login-time challenges, and administrative resets. The sections below document specific aspects of the 2FA domain. +## 2FA remember tokens + +### Issuance + +When a user completes 2FA, they can optionally select the "Don't ask again on this device for 30 days" checkbox on the `/2fa` screen. When this box is checked, the server records a row in the `TwoFactorRememberToken` table — one row per `(UserId, DeviceId)` — and issues a 2FA remember token (see `TwoFactorRememberTokenable`) carrying a copy of that row's `Stamp`. On a later login the client presents the token as `TwoFactorProviderType.Remember` and skips the 2FA challenge. + +### Revocation + +A 2FA remember token is only honored while the `Stamp` it carries still matches its corresponding `TwoFactorRememberToken` row. `IRevokeTwoFactorRememberTokensCommand.RevokeAllForUserAsync` writes a new `Stamp` to every row for a user, which stops every 2FA remember token issued against these rows from being honored at once. The rows are kept, so `CreationDate` still records when each device was first issued a 2FA remember token and `RevisionDate` records when it was cut off. + +Four flows revoke, all of them teardowns that leave the account with no effective second factor: + +| Flow | Where | +| -------------------------------------------------------- | ------------------------------------------------------------------------------------- | +| An administrator resets a member's 2FA | `ResetUserTwoFactorCommand.ResetAsync` | +| A user disables 2FA with a recovery code | `UserService.RecoverTwoFactorAsync` | +| A user removes a provider and 2FA is no longer enabled | `UserService.DisableTwoFactorProviderAsync` | +| A grantee takes over an account through emergency access | `EmergencyAccessService.FinishRecoveryTakeoverAsync` and the obsolete `PasswordAsync` | + +Removing a provider does **not** revoke if the user still has working 2FA, so their 2FA remember tokens stay valid. A provider that requires premium does not count when the user has no premium access, so removing the only non-premium provider revokes even though a premium-only provider remains. + +A password change or account key rotation also stops every 2FA remember token from being honored, without a call to `RevokeAllForUserAsync`. Each `TwoFactorRememberTokenable` carries a copy of `User.SecurityStamp` from the time it was issued. The server compares that copy with the current `User.SecurityStamp` on every use. Both events rotate `User.SecurityStamp`, so no existing 2FA remember token matches. + +Untrusting a device (Trusted Device Encryption) does not revoke 2FA remember tokens — that is a separate mechanism. + +### Cleanup + +Revoked `TwoFactorRememberToken` rows stay until their `ExpirationDate` passes. `DeleteExpiredTwoFactorRememberTokensJob` then deletes them. + ## User Verification When a user manages their own 2FA enrollment (configuring a new provider, updating an existing one, removing one), the server requires proof that the human at the keyboard is the account owner. This section covers how that proof is established and replayed across the read → write step of a per-provider management flow. @@ -59,14 +88,14 @@ Replays of a still-valid token against the same `(UserId, ProviderType)` are int Every per-provider 2FA management endpoint follows the same shape: -| Provider | GET (mints) | Chain (replays) | PUT (replays) | DELETE (replays) | -| -------------- | --------------------------------------------- | ------------------------------------------ | --------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------- | -| Authenticator | `POST /two-factor/get-authenticator` | — | `PUT /two-factor/authenticator` | `DELETE /two-factor/authenticator` | -| YubiKey | `POST /two-factor/get-yubikey` | — | `PUT /two-factor/yubikey` | `DELETE /two-factor/yubikey` | -| Duo (personal) | `POST /two-factor/get-duo` | — | `PUT /two-factor/duo` | `DELETE /two-factor/duo` | -| Duo (org) | `POST /organizations/{id}/two-factor/get-duo` | — | `PUT /organizations/{id}/two-factor/duo` | `DELETE /organizations/{id}/two-factor/duo` | -| WebAuthn | `POST /two-factor/get-webauthn` | `POST /two-factor/get-webauthn-challenge` | `PUT /two-factor/webauthn` | `DELETE /two-factor/webauthn` (per-credential), `DELETE /two-factor/webauthn/all` (bulk) | -| Email | `POST /two-factor/get-email` | `POST /two-factor/send-email` | `PUT /two-factor/email` | `DELETE /two-factor/email` | +| Provider | GET (mints) | Chain (replays) | PUT (replays) | DELETE (replays) | +| -------------- | --------------------------------------------- | ----------------------------------------- | ---------------------------------------- | ---------------------------------------------------------------------------------------- | +| Authenticator | `POST /two-factor/get-authenticator` | — | `PUT /two-factor/authenticator` | `DELETE /two-factor/authenticator` | +| YubiKey | `POST /two-factor/get-yubikey` | — | `PUT /two-factor/yubikey` | `DELETE /two-factor/yubikey` | +| Duo (personal) | `POST /two-factor/get-duo` | — | `PUT /two-factor/duo` | `DELETE /two-factor/duo` | +| Duo (org) | `POST /organizations/{id}/two-factor/get-duo` | — | `PUT /organizations/{id}/two-factor/duo` | `DELETE /organizations/{id}/two-factor/duo` | +| WebAuthn | `POST /two-factor/get-webauthn` | `POST /two-factor/get-webauthn-challenge` | `PUT /two-factor/webauthn` | `DELETE /two-factor/webauthn` (per-credential), `DELETE /two-factor/webauthn/all` (bulk) | +| Email | `POST /two-factor/get-email` | `POST /two-factor/send-email` | `PUT /two-factor/email` | `DELETE /two-factor/email` | The Chain column captures interior steps that replay an existing UV token and return data the next call needs — for WebAuthn, `get-webauthn-challenge` replays the `get-webauthn` token and returns FIDO2 registration options for the PUT; for Email, `send-email` replays the `get-email` token and ships an OTP to the user's inbox that the PUT then carries back alongside the same UV token. No new UV token is minted at the chain step; the original one stays valid through the PUT. diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs index ea1b9c7782c9..664817d59edb 100644 --- a/src/Core/Services/Implementations/UserService.cs +++ b/src/Core/Services/Implementations/UserService.cs @@ -14,6 +14,7 @@ using Bit.Core.AdminConsole.Repositories; using Bit.Core.Auth.Enums; using Bit.Core.Auth.Services; +using Bit.Core.Auth.UserFeatures.TwoFactorAuth; using Bit.Core.Auth.UserFeatures.TwoFactorAuth.Interfaces; using Bit.Core.Billing; using Bit.Core.Billing.Licenses; @@ -68,6 +69,7 @@ public class UserService : UserManager, IUserService private readonly IStripeSyncService _stripeSyncService; private readonly IRevokeNonCompliantOrganizationUserCommand _revokeNonCompliantOrganizationUserCommand; private readonly ITwoFactorIsEnabledQuery _twoFactorIsEnabledQuery; + private readonly IRevokeTwoFactorRememberTokensCommand _revokeTwoFactorRememberTokensCommand; private readonly IDistributedCache _distributedCache; private readonly IPolicyRequirementQuery _policyRequirementQuery; private readonly IHasPremiumAccessQuery _hasPremiumAccessQuery; @@ -102,6 +104,7 @@ public UserService( IStripeSyncService stripeSyncService, IRevokeNonCompliantOrganizationUserCommand revokeNonCompliantOrganizationUserCommand, ITwoFactorIsEnabledQuery twoFactorIsEnabledQuery, + IRevokeTwoFactorRememberTokensCommand revokeTwoFactorRememberTokensCommand, IDistributedCache distributedCache, IPolicyRequirementQuery policyRequirementQuery, IHasPremiumAccessQuery hasPremiumAccessQuery, @@ -140,6 +143,7 @@ public UserService( _stripeSyncService = stripeSyncService; _revokeNonCompliantOrganizationUserCommand = revokeNonCompliantOrganizationUserCommand; _twoFactorIsEnabledQuery = twoFactorIsEnabledQuery; + _revokeTwoFactorRememberTokensCommand = revokeTwoFactorRememberTokensCommand; _distributedCache = distributedCache; _policyRequirementQuery = policyRequirementQuery; _hasPremiumAccessQuery = hasPremiumAccessQuery; @@ -745,11 +749,24 @@ public async Task DisableTwoFactorProviderAsync(User user, TwoFactorProviderType providers.Remove(type); user.SetTwoFactorProviders(providers); + + // Check if two factor authentication in general is now disabled for the user. + // Evaluated before the save, against the in-memory user that no longer has the provider. + // Revoking before the save means a failed revoke leaves the provider in place, so the user can + // simply retry. If evaluated after the save, the early return at the top would block any retry. + var twoFactorAuthNoLongerEnabled = !await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(user); + + if (twoFactorAuthNoLongerEnabled) + { + await _revokeTwoFactorRememberTokensCommand.RevokeAllForUserAsync(user.Id); + } + await SaveUserAsync(user); await _eventService.LogUserEventAsync(user.Id, EventType.User_Disabled2fa); - if (!await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(user)) + if (twoFactorAuthNoLongerEnabled) { + // Revokes the user from organizations that require 2FA. await CheckPoliciesOnTwoFactorRemovalAsync(user); } } @@ -763,6 +780,11 @@ public async Task RecoverTwoFactorAsync(User user, string recoveryCode) return false; } + // Sits after the recovery-code comparison so an unverified request cannot rotate stamps. + // Sits before SaveUserAsync, which consumes the recovery code: a failure here leaves the + // code usable, so a retry re-runs both steps. + await _revokeTwoFactorRememberTokensCommand.RevokeAllForUserAsync(user.Id); + user.TwoFactorProviders = null; user.TwoFactorRecoveryCode = CoreHelpers.SecureRandomString(32, upper: false, special: false); await SaveUserAsync(user); diff --git a/src/Identity/IdentityServer/CustomValidatorRequestContext.cs b/src/Identity/IdentityServer/CustomValidatorRequestContext.cs index feb83f624e48..c0a5be8a337d 100644 --- a/src/Identity/IdentityServer/CustomValidatorRequestContext.cs +++ b/src/Identity/IdentityServer/CustomValidatorRequestContext.cs @@ -58,13 +58,13 @@ public class CustomValidatorRequestContext /// public bool RememberMeRequested { get; set; } = false; /// - /// Whether the Remember Me token presented on this request was in the previous format and should + /// Whether the 2FA remember token presented on this request was in the previous format and should /// be replaced with one in the current format on this response. /// /// /// Distinct from on purpose. It is set only when a /// previous-format token was accepted, never merely because the request used the Remember - /// provider — which is what keeps a remember-login from extending its own lifetime. + /// provider — which is what keeps a login that used a 2FA remember token from extending its own lifetime. /// public bool RememberUpgradeRequired { get; set; } = false; } diff --git a/src/Identity/IdentityServer/RequestValidators/BaseRequestValidator.cs b/src/Identity/IdentityServer/RequestValidators/BaseRequestValidator.cs index 8ce108913b5a..2327e069e0eb 100644 --- a/src/Identity/IdentityServer/RequestValidators/BaseRequestValidator.cs +++ b/src/Identity/IdentityServer/RequestValidators/BaseRequestValidator.cs @@ -116,10 +116,10 @@ protected async Task ValidateAsync(T context, ValidatedTokenRequest request, return; } - // A remember token is issued either because the user asked for one, or because they + // A 2FA remember token is issued either because the user asked for one, or because they // presented one in the previous format and it is being replaced. The second condition is // set only on a successful previous-format validation — never merely because the request - // used the Remember provider, which would let an active user's remember-me renew forever. + // used the Remember provider, which would let an active user's 2FA remember token renew forever. await BuildSuccessResultAsync(validatorContext.User, context, validatorContext.Device, validatorContext.RememberMeRequested || validatorContext.RememberUpgradeRequired); } @@ -367,13 +367,15 @@ await BuildErrorResultAsync("Two-step token is invalid. Try again.", true, conte validatorContext.TwoFactorRecoveryRequested = true; } - // 3d. When the 2FA authentication is successful, we can check if the user wants a - // rememberMe token. + // 3d. When the 2FA authentication is successful, we can check if the user wants a TwoFactorRememberToken var twoFactorRemember = request.Raw["TwoFactorRemember"] == "1"; - // Check if the user wants a rememberMe token. - if (twoFactorRemember - // if the 2FA auth was rememberMe do not send another token. - && twoFactorProviderType != TwoFactorProviderType.Remember) + + // A `Remember` login must not renew itself, so don't mint a new token. + // A `RecoveryCode` login removes 2FA, so don't mint a new token. + var providerAllowsNewRememberToken = + twoFactorProviderType is not (TwoFactorProviderType.Remember or TwoFactorProviderType.RecoveryCode); + + if (twoFactorRemember && providerAllowsNewRememberToken) { validatorContext.RememberMeRequested = true; } diff --git a/src/Identity/IdentityServer/RequestValidators/ITwoFactorAuthenticationValidator.cs b/src/Identity/IdentityServer/RequestValidators/ITwoFactorAuthenticationValidator.cs index 008686bd96e2..276a487e5ce5 100644 --- a/src/Identity/IdentityServer/RequestValidators/ITwoFactorAuthenticationValidator.cs +++ b/src/Identity/IdentityServer/RequestValidators/ITwoFactorAuthenticationValidator.cs @@ -35,7 +35,7 @@ public interface ITwoFactorAuthenticationValidator /// secret passed from the user and consumed by the two-factor provider's verify method /// /// The client-generated device identifier from the raw request. An email code only verifies for the - /// device it was issued to, and a remember token is bound to its device. + /// device it was issued to, and a 2FA remember token is bound to its device. /// Task VerifyTwoFactorAsync( User user, @@ -47,7 +47,7 @@ Task VerifyTwoFactorAsync( /// Whether the presented second factor was accepted. /// -/// Set when a remember token in the pre-TwoFactorRememberTokenable format was accepted, so +/// Set when a 2FA remember token in the pre-TwoFactorRememberTokenable format was accepted, so /// the caller knows to issue a replacement in the current format on this response. /// public record TwoFactorVerificationResult(bool Succeeded, bool LegacyRememberUpgradeRequired = false); diff --git a/src/Identity/IdentityServer/RequestValidators/TwoFactorAuthenticationValidator.cs b/src/Identity/IdentityServer/RequestValidators/TwoFactorAuthenticationValidator.cs index 039446118e61..77dbd70f92e3 100644 --- a/src/Identity/IdentityServer/RequestValidators/TwoFactorAuthenticationValidator.cs +++ b/src/Identity/IdentityServer/RequestValidators/TwoFactorAuthenticationValidator.cs @@ -160,7 +160,7 @@ public async Task VerifyTwoFactorAsync( // null and the lookup below would reject it. It is validated on its own terms here instead. if (type is TwoFactorProviderType.Remember) { - // TODO: PM-44171 - Remove the legacy token path. PM-40673 introduced a new remember token format and stopped + // TODO: PM-44171 - Remove the legacy token path. PM-40673 introduced a new 2FA remember token format and stopped // issuing legacy format tokens. Legacy tokens have a 30 day expiration, so we must wait until more than 30 days // have passed after the release of PM-40673 before we remove this legacy path. var isLegacyFormat = token is null || !token.StartsWith(TwoFactorRememberTokenable.ClearTextPrefix, StringComparison.Ordinal); diff --git a/src/Infrastructure.Dapper/Auth/Repositories/TwoFactorRememberTokenRepository.cs b/src/Infrastructure.Dapper/Auth/Repositories/TwoFactorRememberTokenRepository.cs index 41d3ed6ef29f..4e9f2ee2369e 100644 --- a/src/Infrastructure.Dapper/Auth/Repositories/TwoFactorRememberTokenRepository.cs +++ b/src/Infrastructure.Dapper/Auth/Repositories/TwoFactorRememberTokenRepository.cs @@ -55,8 +55,8 @@ public async Task UpsertAsync(TwoFactorRememberToken tok catch (SqlException e) when (e.Number is 2601 or 2627) { // Unique-index backstop ([IX_TwoFactorRememberToken_UserId_DeviceId]): a concurrent - // remember-login on the same device inserted the row between our read and our insert. - // Update the winner's row instead of failing the login. + // login that requested a 2FA remember token for the same device inserted the row between + // our read and our insert. Update the winner's row instead of failing the login. existing = await GetByUserIdDeviceIdAsync(token.UserId, token.DeviceId); if (existing == null || !await TryUpdateAsync(token, existing)) { @@ -87,8 +87,8 @@ private async Task TryUpdateAsync(TwoFactorRememberToken token, TwoFactorR return false; } - // CreationDate stays as first written, so the row still records when the device was first - // remembered; the update procedure does not write it. + // CreationDate stays as first written, so the row still records when a 2FA remember token + // was first issued for the device; the update procedure does not write it. token.Id = existing.Id; token.CreationDate = existing.CreationDate; return true; diff --git a/src/Infrastructure.EntityFramework/Auth/Repositories/TwoFactorRememberTokenRepository.cs b/src/Infrastructure.EntityFramework/Auth/Repositories/TwoFactorRememberTokenRepository.cs index 8e7f1f403b5b..a8f2d9478915 100644 --- a/src/Infrastructure.EntityFramework/Auth/Repositories/TwoFactorRememberTokenRepository.cs +++ b/src/Infrastructure.EntityFramework/Auth/Repositories/TwoFactorRememberTokenRepository.cs @@ -43,8 +43,8 @@ public TwoFactorRememberTokenRepository(IServiceScopeFactory serviceScopeFactory } catch (DbUpdateException e) when (IsDuplicateKeyException(e)) { - // Unique-index backstop: a concurrent remember-login on the same device inserted the row - // between our read and our insert. Update the winner's row instead of failing the login. + // Unique-index backstop: a concurrent login that requested a 2FA remember token for the same + // device inserted the row between our read and our insert. Update the winner's row instead of failing the login. existing = await GetByUserIdDeviceIdAsync(token.UserId, token.DeviceId); if (existing == null || !await TryUpdateAsync(token, existing)) { @@ -73,7 +73,7 @@ private async Task TryUpdateAsync( return false; } - // CreationDate is left alone so the row still records when the device was first remembered, + // CreationDate is left alone so the row still records when a 2FA remember token was first issued for the device, // matching the MSSQL procedure. token.Id = existing.Id; token.CreationDate = existing.CreationDate; diff --git a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs index a89d62546142..21700cf9e1b5 100644 --- a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs +++ b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs @@ -9,6 +9,8 @@ using Bit.Core.AdminConsole.Enums; using Bit.Core.AdminConsole.Enums.Provider; using Bit.Core.AdminConsole.Repositories; +using Bit.Core.Auth.Entities; +using Bit.Core.Auth.Repositories; using Bit.Core.Billing.Enums; using Bit.Core.Entities; using Bit.Core.Enums; @@ -530,4 +532,140 @@ private async Task LoginAsAccountRecoveryManagerWithMorePermis return targetCustomOrgUser; } + + [Fact] + public async Task RecoverAccount_ResetTwoFactorOnly_RevokesTwoFactorRememberTokens() + { + // Arrange + var (ownerEmail, _) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory, + _organization.Id, OrganizationUserType.Owner); + await _loginHelper.LoginAsync(ownerEmail); + + var (targetEmail, targetOrgUser) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync( + _factory, _organization.Id, OrganizationUserType.User); + await SetResetPasswordKeyAsync(targetOrgUser); + + var userRepository = _factory.GetService(); + var targetUser = (await userRepository.GetByEmailAsync(targetEmail))!; + var (device, _) = await SeedTwoFactorRememberTokenAsync(targetUser, "stamp-before-reset"); + + // Act + var response = await _client.PutAsJsonAsync( + $"organizations/{_organization.Id}/users/{targetOrgUser.Id}/recover-account", + new OrganizationUserResetPasswordRequestModel + { + ResetMasterPassword = false, + ResetTwoFactor = true, + }); + + // Assert + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + + var rememberTokenRepository = _factory.GetService(); + var row = await rememberTokenRepository.GetByUserIdDeviceIdAsync(targetUser.Id, device.Id); + Assert.NotNull(row); + Assert.NotEqual("stamp-before-reset", row.Stamp); + } + + /// + /// The two flags are independent. A password-only recovery is not a two-factor teardown, so it + /// does not rotate the 2FA remember token rows. (The password change itself still invalidates + /// existing 2FA remember tokens through the security stamp.) + /// + [Fact] + public async Task RecoverAccount_ResetMasterPasswordOnly_LeavesTwoFactorRememberTokensAlone() + { + // Arrange + var (ownerEmail, _) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory, + _organization.Id, OrganizationUserType.Owner); + await _loginHelper.LoginAsync(ownerEmail); + + var (targetEmail, targetOrgUser) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync( + _factory, _organization.Id, OrganizationUserType.User); + await SetResetPasswordKeyAsync(targetOrgUser); + + var userRepository = _factory.GetService(); + var targetUser = (await userRepository.GetByEmailAsync(targetEmail))!; + var (device, _) = await SeedTwoFactorRememberTokenAsync(targetUser, "stamp-untouched"); + + // Act + var response = await _client.PutAsJsonAsync( + $"organizations/{_organization.Id}/users/{targetOrgUser.Id}/recover-account", + new OrganizationUserResetPasswordRequestModel + { + ResetMasterPassword = true, + ResetTwoFactor = false, + NewMasterPasswordHash = "new-master-password-hash", + Key = "2.AAECAwQFBgcICQoLDA0ODw==|QmFzZTY0UGFydA==|AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=", + }); + + // Assert + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + + var rememberTokenRepository = _factory.GetService(); + var row = await rememberTokenRepository.GetByUserIdDeviceIdAsync(targetUser.Id, device.Id); + Assert.NotNull(row); + Assert.Equal("stamp-untouched", row.Stamp); + } + + [Fact] + public async Task RecoverAccount_ResetMasterPasswordAndTwoFactor_RevokesTwoFactorRememberTokens() + { + // Arrange + var (ownerEmail, _) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory, + _organization.Id, OrganizationUserType.Owner); + await _loginHelper.LoginAsync(ownerEmail); + + var (targetEmail, targetOrgUser) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync( + _factory, _organization.Id, OrganizationUserType.User); + await SetResetPasswordKeyAsync(targetOrgUser); + + var userRepository = _factory.GetService(); + var targetUser = (await userRepository.GetByEmailAsync(targetEmail))!; + var (device, _) = await SeedTwoFactorRememberTokenAsync(targetUser, "stamp-before-reset"); + + // Act + var response = await _client.PutAsJsonAsync( + $"organizations/{_organization.Id}/users/{targetOrgUser.Id}/recover-account", + new OrganizationUserResetPasswordRequestModel + { + ResetMasterPassword = true, + ResetTwoFactor = true, + NewMasterPasswordHash = "new-master-password-hash", + Key = "2.AAECAwQFBgcICQoLDA0ODw==|QmFzZTY0UGFydA==|AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=", + }); + + // Assert + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + + var rememberTokenRepository = _factory.GetService(); + var row = await rememberTokenRepository.GetByUserIdDeviceIdAsync(targetUser.Id, device.Id); + Assert.NotNull(row); + Assert.NotEqual("stamp-before-reset", row.Stamp); + } + + private async Task<(Device Device, TwoFactorRememberToken Row)> SeedTwoFactorRememberTokenAsync( + User user, string stamp) + { + var device = await _factory.GetService().CreateAsync(new Device + { + UserId = user.Id, + Name = "chrome-test", + Type = DeviceType.ChromeBrowser, + Identifier = Guid.NewGuid().ToString(), + Active = true, + LastActivityDate = DateTime.UtcNow, + }); + + var row = await _factory.GetService() + .UpsertAsync(new TwoFactorRememberToken + { + UserId = user.Id, + DeviceId = device.Id, + Stamp = stamp, + ExpirationDate = DateTime.UtcNow.AddDays(30), + }); + + return (device, row); + } } diff --git a/test/Api.IntegrationTest/Auth/Controllers/EmergencyAccessControllerTest.cs b/test/Api.IntegrationTest/Auth/Controllers/EmergencyAccessControllerTest.cs index 616a714c6386..97f47671b04f 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/EmergencyAccessControllerTest.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/EmergencyAccessControllerTest.cs @@ -15,6 +15,7 @@ using Bit.Core.Tokens; using Microsoft.AspNetCore.Identity; using Xunit; +using static Bit.Api.IntegrationTest.Auth.Helpers.TwoFactorIntegrationTestHelpers; namespace Bit.Api.IntegrationTest.Auth.Controllers; @@ -213,6 +214,94 @@ public async Task PostPassword_V2_LegacyKdfBelowMinimum_Success( Assert.Empty(updatedGrantor.GetTwoFactorProviders() ?? []); } + /// + /// A takeover removes the grantor's 2FA, so the 2FA remember tokens they had issued stop being honored + /// too: the row's stamp is rotated. Uses the V2 payload, which finishes through + /// FinishRecoveryTakeoverAsync. + /// + [Fact] + public async Task PostPassword_V2_RevokesGrantorTwoFactorRememberTokens() + { + // Arrange: the grantor has 2FA and a device holding a 2FA remember token. The request must echo the + // grantor's stored KDF, so set it explicitly rather than relying on the factory default. + var grantor = await _userRepository.GetByEmailAsync(_grantorEmail); + Assert.NotNull(grantor); + grantor.Kdf = _defaultKdfRequest.KdfType; + grantor.KdfIterations = _defaultKdfRequest.Iterations; + grantor.SetTwoFactorProviders(new Dictionary + { + [TwoFactorProviderType.Email] = new() { Enabled = true }, + }); + await _userRepository.ReplaceAsync(grantor); + var device = await SeedTwoFactorRememberTokenAsync(_factory, _grantorEmail, "stamp-before-takeover"); + + await _loginHelper.LoginAsync(_granteeEmail); + + var requestModel = new EmergencyAccessPasswordRequestModel + { + AuthenticationData = BuildAuthData(_defaultKdfRequest, _grantorEmail), + UnlockData = BuildUnlockData(_defaultKdfRequest, _grantorEmail), + }; + + // Act + using var message = new HttpRequestMessage( + HttpMethod.Post, $"/emergency-access/{_emergencyAccessId}/password"); + message.Content = JsonContent.Create(requestModel); + var response = await _client.SendAsync(message); + + // Assert + if (!response.IsSuccessStatusCode) + { + var errorContent = await response.Content.ReadAsStringAsync(); + Assert.Fail($"Expected success but got {response.StatusCode}. Error: {errorContent}"); + } + + await AssertTwoFactorRememberTokenRotatedAsync(_factory, _grantorEmail, device, "stamp-before-takeover"); + } + + /// + /// The legacy payload (NewMasterPasswordHash and Key, no V2 fields) finishes through the + /// obsolete PasswordAsync, which also removes the grantor's 2FA. Their 2FA remember tokens must + /// stop being honored there too. Remove this test together with the legacy path in PM-33141. + /// + [Fact] + public async Task PostPassword_Legacy_RevokesGrantorTwoFactorRememberTokens() + { + // Arrange: the grantor has 2FA and a device holding a 2FA remember token. + var grantor = await _userRepository.GetByEmailAsync(_grantorEmail); + Assert.NotNull(grantor); + grantor.SetTwoFactorProviders(new Dictionary + { + [TwoFactorProviderType.Email] = new() { Enabled = true }, + }); + await _userRepository.ReplaceAsync(grantor); + var device = await SeedTwoFactorRememberTokenAsync(_factory, _grantorEmail, "stamp-before-takeover"); + + await _loginHelper.LoginAsync(_granteeEmail); + + // Only the legacy fields are set, so the controller dispatches to PasswordAsync. + var requestModel = new EmergencyAccessPasswordRequestModel + { + NewMasterPasswordHash = _newMasterPasswordHash, + Key = _masterKeyWrappedUserKey, + }; + + // Act + using var message = new HttpRequestMessage( + HttpMethod.Post, $"/emergency-access/{_emergencyAccessId}/password"); + message.Content = JsonContent.Create(requestModel); + var response = await _client.SendAsync(message); + + // Assert + if (!response.IsSuccessStatusCode) + { + var errorContent = await response.Content.ReadAsStringAsync(); + Assert.Fail($"Expected success but got {response.StatusCode}. Error: {errorContent}"); + } + + await AssertTwoFactorRememberTokenRotatedAsync(_factory, _grantorEmail, device, "stamp-before-takeover"); + } + /// /// Verifies the boundary validator's agreement checks fire on /// POST /emergency-access/{id}/password: a mismatched KDF or salt @@ -228,6 +317,7 @@ public async Task PostPassword_V2_LegacyKdfBelowMinimum_Success( public async Task PostPassword_V2_MismatchedKdfOrSalt_BadRequest( string mismatchKind, string perturbedSide, string expectedError) { + var device = await SeedTwoFactorRememberTokenAsync(_factory, _grantorEmail, "stamp-before-rejected-takeover"); await _loginHelper.LoginAsync(_granteeEmail); var (auth, unlock) = BuildMismatchedAuthAndUnlock(mismatchKind, perturbedSide); @@ -245,6 +335,10 @@ public async Task PostPassword_V2_MismatchedKdfOrSalt_BadRequest( Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); var content = await response.Content.ReadAsStringAsync(); Assert.Contains(expectedError, content); + + // A rejected takeover must not cut off the grantor's 2FA remember tokens. + await AssertTwoFactorRememberTokenUnchangedAsync( + _factory, _grantorEmail, device, "stamp-before-rejected-takeover"); } /// @@ -269,6 +363,7 @@ public async Task PostPassword_V2_NotRecoveryApproved_BadRequest( Assert.NotNull(emergencyAccess); emergencyAccess.Status = invalidStatus; await _emergencyAccessRepository.ReplaceAsync(emergencyAccess); + var device = await SeedTwoFactorRememberTokenAsync(_factory, _grantorEmail, "stamp-before-rejected-takeover"); await _loginHelper.LoginAsync(_granteeEmail); @@ -286,6 +381,10 @@ public async Task PostPassword_V2_NotRecoveryApproved_BadRequest( Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); var content = await response.Content.ReadAsStringAsync(); Assert.Contains("Emergency Access not valid.", content); + + // A rejected takeover must not cut off the grantor's 2FA remember tokens. + await AssertTwoFactorRememberTokenUnchangedAsync( + _factory, _grantorEmail, device, "stamp-before-rejected-takeover"); } /// diff --git a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerAuthenticatorTests.cs b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerAuthenticatorTests.cs index eb69da8a186c..ba31b7ca5b83 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerAuthenticatorTests.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerAuthenticatorTests.cs @@ -128,6 +128,34 @@ public async Task GetAuthenticator_ValidSecret_ReturnsTokenUsableForDelete() Assert.Null(refreshed!.GetTwoFactorProvider(TwoFactorProviderType.Authenticator)); } + /// + /// Removing the only provider leaves 2FA no longer enabled, so the user's 2FA remember tokens are + /// revoked. Same DisableTwoFactorProviderAsync path as Email. + /// + [Fact] + public async Task DeleteAuthenticator_TwoFactorAuthNoLongerEnabled_RevokesTwoFactorRememberTokens() + { + await EnrollUserInAuthenticator(); + var device = await SeedTwoFactorRememberTokenAsync(_factory, _userEmail, "stamp-before-teardown"); + + var getResponse = await _client.PostAsJsonAsync("/two-factor/get-authenticator", + new { MasterPasswordHash = MasterPasswordHash }); + Assert.Equal(HttpStatusCode.OK, getResponse.StatusCode); + var root = await ReadJsonRootAsync(getResponse); + var key = root.GetProperty("authenticator").GetProperty("key").GetString()!; + var uvToken = root.GetProperty("userVerificationToken").GetString()!; + + var disableResponse = await SendJsonAsync(_client, HttpMethod.Delete, "/two-factor/authenticator", + new TwoFactorAuthenticatorDeleteRequestModel + { + Key = key, + UserVerificationToken = uvToken, + }); + Assert.Equal(HttpStatusCode.NoContent, disableResponse.StatusCode); + + await AssertTwoFactorRememberTokenRotatedAsync(_factory, _userEmail, device, "stamp-before-teardown"); + } + [Fact] public async Task PutAuthenticator_ValidTokenAndCode_UpdatesProvider() { diff --git a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerDuoTests.cs b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerDuoTests.cs index 21c29ffbbead..59078245b66b 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerDuoTests.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerDuoTests.cs @@ -74,6 +74,30 @@ public async Task GetDuo_ValidSecret_ReturnsTokenUsableForDelete() Assert.Null(refreshed!.GetTwoFactorProvider(TwoFactorProviderType.Duo)); } + /// + /// Removing the only provider leaves 2FA no longer enabled, so the user's 2FA remember tokens are + /// revoked. Duo needs premium, so the user is granted it first: 2FA is enabled before the delete + /// and no longer enabled after it. + /// + [Fact] + public async Task DeleteDuo_TwoFactorAuthNoLongerEnabled_RevokesTwoFactorRememberTokens() + { + await EnrollUserInDuo(); + await GrantPremium(); + var device = await SeedTwoFactorRememberTokenAsync(_factory, _userEmail, "stamp-before-teardown"); + + var getResponse = await _client.PostAsJsonAsync("/two-factor/get-duo", + new { MasterPasswordHash = MasterPasswordHash }); + Assert.Equal(HttpStatusCode.OK, getResponse.StatusCode); + var (_, uvToken) = await ReadEnabledAndUserVerificationTokenAsync(getResponse, "duo"); + + var disableResponse = await SendJsonAsync(_client, HttpMethod.Delete, "/two-factor/duo", + new TwoFactorDuoDeleteRequestModel { UserVerificationToken = uvToken }); + Assert.Equal(HttpStatusCode.NoContent, disableResponse.StatusCode); + + await AssertTwoFactorRememberTokenRotatedAsync(_factory, _userEmail, device, "stamp-before-teardown"); + } + [Fact] public async Task PutDuo_ValidTokenAndPremium_UpdatesProvider() { diff --git a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs index ccaf63927370..703f19443720 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs @@ -6,6 +6,8 @@ using Bit.Api.IntegrationTest.Factories; using Bit.Api.IntegrationTest.Helpers; using Bit.Core.AdminConsole.Entities; +using Bit.Core.AdminConsole.Enums; +using Bit.Core.AdminConsole.Repositories; using Bit.Core.Auth.Entities; using Bit.Core.Auth.Enums; using Bit.Core.Auth.Identity.TokenProviders; @@ -13,6 +15,7 @@ using Bit.Core.Auth.Models.Data; using Bit.Core.Auth.Repositories; using Bit.Core.Auth.UserFeatures.TwoFactorAuth; +using Bit.Core.Billing.Enums; using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Platform.Push; @@ -845,6 +848,74 @@ public async Task EmailLoginCode_StoredInPreviousFormat_Rejected() AssertTwoFactorRejected(result); } + [Fact] + public async Task DeleteEmail_TwoFactorAuthNoLongerEnabled_RevokesTwoFactorRememberTokens() + { + await EnrollUserInEmail(); + var (device, _) = await SeedTwoFactorRememberTokenAsync("stamp-before-teardown"); + + await DeleteEmailProviderAsync(); + + await AssertTwoFactorRememberTokenStampAsync(device, expectRotated: true, "stamp-before-teardown"); + } + + /// + /// Removing one provider while another still works leaves 2FA enabled, so the 2FA remember token is + /// not revoked. Uses the real TwoFactorIsEnabledQuery; the unit tests mock it. + /// + [Fact] + public async Task DeleteEmail_OtherProviderRemains_DoesNotRevokeTwoFactorRememberTokens() + { + await SetUserTwoFactorProvidersJsonAsync(_userRepository, _userEmail, + CombineProvidersJson( + BuildEmailProvidersJson(_userEmail), + BuildAuthenticatorProvidersJson("JBSWY3DPEHPK3PXP"))); + var (device, _) = await SeedTwoFactorRememberTokenAsync("stamp-untouched"); + + await DeleteEmailProviderAsync(); + + await AssertTwoFactorRememberTokenStampAsync(device, expectRotated: false, "stamp-untouched"); + } + + /// + /// A provider that requires premium does not count for a user without premium. Removing Email leaves + /// only YubiKey, so 2FA is no longer effectively enabled and the 2FA remember token is revoked, even + /// though a provider remains. Uses the real TwoFactorIsEnabledQuery; the unit tests mock it. + /// + [Fact] + public async Task DeleteEmail_OnlyPremiumProviderRemainsAndUserHasNoPremium_RevokesTwoFactorRememberTokens() + { + await SetUserTwoFactorProvidersJsonAsync(_userRepository, _userEmail, + CombineProvidersJson( + BuildEmailProvidersJson(_userEmail), + BuildYubiKeyProvidersJson())); + var (device, _) = await SeedTwoFactorRememberTokenAsync("stamp-before-teardown"); + + await DeleteEmailProviderAsync(); + + await AssertTwoFactorRememberTokenStampAsync(device, expectRotated: true, "stamp-before-teardown"); + } + + /// + /// Both effects of removing a provider so that 2FA is no longer enabled happen together: the 2FA + /// remember tokens are revoked (before the save), and the user is revoked from organizations that + /// require 2FA (after it). + /// + [Fact] + public async Task DeleteEmail_TwoFactorAuthNoLongerEnabledWithTwoFactorPolicy_RevokesTokensAndOrganizationUser() + { + await EnrollUserInEmail(); + var (device, _) = await SeedTwoFactorRememberTokenAsync("stamp-before-teardown"); + var organizationUser = await JoinOrganizationRequiringTwoFactorAsync(); + + await DeleteEmailProviderAsync(); + + await AssertTwoFactorRememberTokenStampAsync(device, expectRotated: true, "stamp-before-teardown"); + var revoked = await _factory.GetService().GetByIdAsync(organizationUser.Id); + Assert.NotNull(revoked); + Assert.Equal(OrganizationUserStatusType.Revoked, revoked.Status); + } + // --------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------- @@ -855,6 +926,87 @@ private Task EnrollUserInEmail() => SetUserTwoFactorProvidersJsonAsync( _userRepository, _userEmail, BuildEmailProvidersJson(_userEmail)); + /// Merges single-provider JSON objects into one object holding all of the providers. + private static string CombineProvidersJson(params string[] providersJson) => + "{" + string.Join(",", providersJson.Select(json => json[1..^1])) + "}"; + + private async Task<(Device Device, TwoFactorRememberToken Row)> SeedTwoFactorRememberTokenAsync(string stamp) + { + var user = await GetUserAsync(); + var device = await _factory.GetService().CreateAsync(new Device + { + UserId = user.Id, + Name = "chrome-test", + Type = DeviceType.ChromeBrowser, + Identifier = Guid.NewGuid().ToString(), + Active = true, + LastActivityDate = DateTime.UtcNow, + }); + var row = await _factory.GetService().UpsertAsync( + new TwoFactorRememberToken + { + UserId = user.Id, + DeviceId = device.Id, + Stamp = stamp, + ExpirationDate = DateTime.UtcNow.AddDays(30), + }); + return (device, row); + } + + /// + /// Adds the test user as a confirmed member of an organization that enforces the two-factor policy. + /// The organization needs a separate confirmed owner: the command that revokes non-compliant members + /// refuses to leave an organization without one, and reports that as an error result rather than + /// throwing, so the member would silently stay confirmed. + /// + private async Task JoinOrganizationRequiringTwoFactorAsync() + { + var ownerEmail = $"two-factor-owner-{Guid.NewGuid()}@bitwarden.com"; + await _factory.LoginWithNewAccount(ownerEmail); + var (organization, _) = await OrganizationTestHelpers.SignUpAsync( + _factory, + plan: PlanType.EnterpriseAnnually, + ownerEmail: ownerEmail, + passwordManagerSeats: 10, + paymentMethod: PaymentMethodType.Card); + + organization.UsePolicies = true; + await _factory.GetService().ReplaceAsync(organization); + await _factory.GetService().CreateAsync(new Policy + { + OrganizationId = organization.Id, + Type = PolicyType.TwoFactorAuthentication, + Enabled = true, + }); + + return await OrganizationTestHelpers.CreateUserAsync( + _factory, organization.Id, _userEmail, OrganizationUserType.User); + } + + private async Task DeleteEmailProviderAsync() + { + var uvToken = await GetEmailUserVerificationTokenAsync(); + var response = await SendJsonAsync(_client, HttpMethod.Delete, "/two-factor/email", + new TwoFactorEmailDeleteRequestModel { UserVerificationToken = uvToken }); + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + } + + private async Task AssertTwoFactorRememberTokenStampAsync(Device device, bool expectRotated, string originalStamp) + { + var user = await GetUserAsync(); + var row = await _factory.GetService() + .GetByUserIdDeviceIdAsync(user.Id, device.Id); + Assert.NotNull(row); + if (expectRotated) + { + Assert.NotEqual(originalStamp, row.Stamp); + } + else + { + Assert.Equal(originalStamp, row.Stamp); + } + } + private async Task GetUserAsync() { var user = await _userRepository.GetByEmailAsync(_userEmail); diff --git a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerWebAuthnTests.cs b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerWebAuthnTests.cs index a8391a85b6b0..00088c394255 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerWebAuthnTests.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerWebAuthnTests.cs @@ -204,6 +204,28 @@ public async Task DeleteWebAuthnAll_ValidToken_RemovesProvider() Assert.Null(refreshed!.GetTwoFactorProvider(TwoFactorProviderType.WebAuthn)); } + /// + /// Removing the only provider leaves 2FA no longer enabled, so the user's 2FA remember tokens are + /// revoked. Covers the bulk-disable endpoint. + /// + [Fact] + public async Task DeleteWebAuthnAll_TwoFactorAuthNoLongerEnabled_RevokesTwoFactorRememberTokens() + { + await EnrollUserInWebAuthn(); + var device = await SeedTwoFactorRememberTokenAsync(_factory, _userEmail, "stamp-before-teardown"); + + var getResponse = await _client.PostAsJsonAsync("/two-factor/get-webauthn", + new { MasterPasswordHash = MasterPasswordHash }); + Assert.Equal(HttpStatusCode.OK, getResponse.StatusCode); + var (_, uvToken) = await ReadEnabledAndUserVerificationTokenAsync(getResponse, "webAuthn"); + + var disableResponse = await SendJsonAsync(_client, HttpMethod.Delete, "/two-factor/webauthn/all", + new TwoFactorWebAuthnDeleteAllRequestModel { UserVerificationToken = uvToken }); + Assert.Equal(HttpStatusCode.NoContent, disableResponse.StatusCode); + + await AssertTwoFactorRememberTokenRotatedAsync(_factory, _userEmail, device, "stamp-before-teardown"); + } + [Fact] public async Task DeleteWebAuthnAll_SingleCredentialEnrollment_RemovesProvider() { diff --git a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerYubiKeyTests.cs b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerYubiKeyTests.cs index 1e5668990663..1aea89c921f9 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerYubiKeyTests.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerYubiKeyTests.cs @@ -74,6 +74,30 @@ public async Task GetYubiKey_ValidSecret_ReturnsTokenUsableForDelete() Assert.Null(refreshed!.GetTwoFactorProvider(TwoFactorProviderType.YubiKey)); } + /// + /// Removing the only provider leaves 2FA no longer enabled, so the user's 2FA remember tokens are + /// revoked. YubiKey needs premium, so the user is granted it first: 2FA is enabled before the delete + /// and no longer enabled after it. + /// + [Fact] + public async Task DeleteYubiKey_TwoFactorAuthNoLongerEnabled_RevokesTwoFactorRememberTokens() + { + await EnrollUserInYubiKey(); + await GrantPremium(); + var device = await SeedTwoFactorRememberTokenAsync(_factory, _userEmail, "stamp-before-teardown"); + + var getResponse = await _client.PostAsJsonAsync("/two-factor/get-yubikey", + new { MasterPasswordHash = MasterPasswordHash }); + Assert.Equal(HttpStatusCode.OK, getResponse.StatusCode); + var (_, uvToken) = await ReadEnabledAndUserVerificationTokenAsync(getResponse, "yubiKey"); + + var disableResponse = await SendJsonAsync(_client, HttpMethod.Delete, "/two-factor/yubikey", + new TwoFactorYubiKeyDeleteRequestModel { UserVerificationToken = uvToken }); + Assert.Equal(HttpStatusCode.NoContent, disableResponse.StatusCode); + + await AssertTwoFactorRememberTokenRotatedAsync(_factory, _userEmail, device, "stamp-before-teardown"); + } + [Fact] public async Task PutYubiKey_ValidTokenAndPremium_UpdatesProvider() { diff --git a/test/Api.IntegrationTest/Auth/Helpers/TwoFactorIntegrationTestHelpers.cs b/test/Api.IntegrationTest/Auth/Helpers/TwoFactorIntegrationTestHelpers.cs index ffe65304c4b3..3e9d1059f808 100644 --- a/test/Api.IntegrationTest/Auth/Helpers/TwoFactorIntegrationTestHelpers.cs +++ b/test/Api.IntegrationTest/Auth/Helpers/TwoFactorIntegrationTestHelpers.cs @@ -1,14 +1,19 @@ using System.Text; using System.Text.Json; +using Bit.Api.IntegrationTest.Factories; +using Bit.Core.Auth.Entities; using Bit.Core.Auth.Enums; using Bit.Core.Auth.Models.Business.Tokenables; +using Bit.Core.Auth.Repositories; using Bit.Core.Entities; +using Bit.Core.Enums; using Bit.Core.Repositories; using Bit.Core.Services; using Bit.Core.Tokens; using Bit.Test.Common.Fakes; using Core.Auth.Enums; using NSubstitute; +using Xunit; namespace Bit.Api.IntegrationTest.Auth.Helpers; @@ -113,6 +118,65 @@ public static async Task GrantPremiumAsync(IUserRepository userRepository, strin await userRepository.UpsertAsync(user); } + // --------------------------------------------------------------------- + // 2FA remember tokens + // --------------------------------------------------------------------- + + /// + /// Gives the user a device with a row carrying the given stamp, as a + /// completed "don't ask again on this device" login would. Returns the device. + /// + public static async Task SeedTwoFactorRememberTokenAsync( + ApiApplicationFactory factory, string userEmail, string stamp) + { + var user = (await factory.GetService().GetByEmailAsync(userEmail))!; + var device = await factory.GetService().CreateAsync(new Device + { + UserId = user.Id, + Name = "chrome-test", + Type = DeviceType.ChromeBrowser, + Identifier = Guid.NewGuid().ToString(), + Active = true, + LastActivityDate = DateTime.UtcNow, + }); + await factory.GetService().UpsertAsync(new TwoFactorRememberToken + { + UserId = user.Id, + DeviceId = device.Id, + Stamp = stamp, + ExpirationDate = DateTime.UtcNow.AddDays(30), + }); + return device; + } + + /// + /// Asserts that the device's row no longer carries the stamp it was + /// seeded with, which is what stops its 2FA remember tokens from being honored. + /// + public static async Task AssertTwoFactorRememberTokenRotatedAsync( + ApiApplicationFactory factory, string userEmail, Device device, string originalStamp) + { + var user = (await factory.GetService().GetByEmailAsync(userEmail))!; + var row = await factory.GetService() + .GetByUserIdDeviceIdAsync(user.Id, device.Id); + Assert.NotNull(row); + Assert.NotEqual(originalStamp, row.Stamp); + } + + /// + /// Asserts that the device's row still carries the stamp it was seeded + /// with, so its 2FA remember token is still honored. + /// + public static async Task AssertTwoFactorRememberTokenUnchangedAsync( + ApiApplicationFactory factory, string userEmail, Device device, string originalStamp) + { + var user = (await factory.GetService().GetByEmailAsync(userEmail))!; + var row = await factory.GetService() + .GetByUserIdDeviceIdAsync(user.Id, device.Id); + Assert.NotNull(row); + Assert.Equal(originalStamp, row.Stamp); + } + // --------------------------------------------------------------------- // Emailed two-factor codes // --------------------------------------------------------------------- diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/AccountRecovery/AdminRecoverAccountCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/AccountRecovery/AdminRecoverAccountCommandTests.cs index f44a4b746cc1..3c3cc9700ef3 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/AccountRecovery/AdminRecoverAccountCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/AccountRecovery/AdminRecoverAccountCommandTests.cs @@ -76,6 +76,9 @@ await sutProvider.GetDependency().DidNotReceive().LogOrganization Arg.Any(), Arg.Is(EventType.OrganizationUser_AdminResetTwoFactor)); + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .ResetAsync(default!); + await sutProvider.GetDependency().Received(1) .PushAsync(Arg.Is>(n => n.Type == PushType.LogOut && n.TargetId == user.Id)); } diff --git a/test/Core.Test/Auth/UserFeatures/EmergencyAccess/EmergencyAccessServiceTests.cs b/test/Core.Test/Auth/UserFeatures/EmergencyAccess/EmergencyAccessServiceTests.cs index bd66d2a4821c..7c0ec6212420 100644 --- a/test/Core.Test/Auth/UserFeatures/EmergencyAccess/EmergencyAccessServiceTests.cs +++ b/test/Core.Test/Auth/UserFeatures/EmergencyAccess/EmergencyAccessServiceTests.cs @@ -8,6 +8,7 @@ using Bit.Core.Auth.Models.Business.Tokenables; using Bit.Core.Auth.Models.Data; using Bit.Core.Auth.UserFeatures.EmergencyAccess; +using Bit.Core.Auth.UserFeatures.TwoFactorAuth; using Bit.Core.Auth.UserFeatures.UserMasterPassword.Data; using Bit.Core.Auth.UserFeatures.UserMasterPassword.Interfaces; using Bit.Core.Entities; @@ -21,6 +22,7 @@ using Bit.Test.Common.AutoFixture.Attributes; using Microsoft.AspNetCore.Identity; using NSubstitute; +using NSubstitute.ExceptionExtensions; using OneOf; using Xunit; @@ -1686,6 +1688,41 @@ public async Task PasswordAsync_Disables_NewDeviceVerification_And_TwoFactorProv await sutProvider.GetDependency().Received().ReplaceAsync(grantor); } + [Theory, BitAutoData] + public async Task PasswordAsync_RevokesGrantorTwoFactorRememberTokens( + SutProvider sutProvider, User requestingUser, User grantor) + { + EnableEmailTwoFactor(grantor); + ArrangeTakeover(sutProvider, requestingUser, grantor); + + await sutProvider.Sut.PasswordAsync(Guid.NewGuid(), requestingUser, "new-master-password-hash", "new-user-key"); + + await sutProvider.GetDependency() + .Received(1) + .RevokeAllForUserAsync(grantor.Id); + await sutProvider.GetDependency().Received(1).ReplaceAsync(grantor); + } + + /// + /// The revoke runs before the grantor is saved. If it fails, nothing is persisted, so the grantor's 2FA is + /// left intact and the takeover can be retried. + /// + [Theory, BitAutoData] + public async Task PasswordAsync_RevokingTwoFactorRememberTokensFails_DoesNotSaveGrantor( + SutProvider sutProvider, User requestingUser, User grantor) + { + EnableEmailTwoFactor(grantor); + ArrangeTakeover(sutProvider, requestingUser, grantor); + sutProvider.GetDependency() + .RevokeAllForUserAsync(grantor.Id) + .ThrowsAsync(new InvalidOperationException("revoke failed")); + + await Assert.ThrowsAsync( + () => sutProvider.Sut.PasswordAsync(Guid.NewGuid(), requestingUser, "new-master-password-hash", "new-user-key")); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().ReplaceAsync(default!); + } + [Theory, BitAutoData] public async Task ViewAsync_EmergencyAccessTypeNotView_ThrowsBadRequest( SutProvider sutProvider, @@ -1859,6 +1896,9 @@ public async Task FinishRecoveryTakeoverAsync_MasterPasswordServiceReturnsErrors await sutProvider.GetDependency().DidNotReceive().ReplaceAsync(Arg.Any()); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() .RemoveUserAsync(default, default(Guid)); + // A takeover that did not go through must not cut off the grantor's 2FA remember tokens. + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .RevokeAllForUserAsync(default); } [Theory, BitAutoData] @@ -2004,4 +2044,77 @@ public async Task FinishRecoveryTakeoverAsync_Disables_NewDeviceVerification_And Assert.False(grantor.VerifyDevices); await sutProvider.GetDependency().Received().ReplaceAsync(grantor); } + + [Theory, BitAutoData] + public async Task FinishRecoveryTakeoverAsync_RevokesGrantorTwoFactorRememberTokens( + SutProvider sutProvider, User granteeUser, User grantor) + { + EnableEmailTwoFactor(grantor); + ArrangeFinishRecoveryTakeover(sutProvider, granteeUser, grantor); + + await sutProvider.Sut.FinishRecoveryTakeoverAsync(Guid.NewGuid(), granteeUser, BuildUnlockData(), BuildAuthData()); + + await sutProvider.GetDependency() + .Received(1) + .RevokeAllForUserAsync(grantor.Id); + await sutProvider.GetDependency().Received(1).ReplaceAsync(grantor); + } + + /// + /// The revoke runs before the grantor is saved. If it fails, nothing is persisted, so the grantor's 2FA is + /// left intact and the takeover can be retried. + /// + [Theory, BitAutoData] + public async Task FinishRecoveryTakeoverAsync_RevokingTwoFactorRememberTokensFails_DoesNotSaveGrantor( + SutProvider sutProvider, User granteeUser, User grantor) + { + EnableEmailTwoFactor(grantor); + ArrangeFinishRecoveryTakeover(sutProvider, granteeUser, grantor); + sutProvider.GetDependency() + .RevokeAllForUserAsync(grantor.Id) + .ThrowsAsync(new InvalidOperationException("revoke failed")); + + await Assert.ThrowsAsync( + () => sutProvider.Sut.FinishRecoveryTakeoverAsync( + Guid.NewGuid(), granteeUser, BuildUnlockData(), BuildAuthData())); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().ReplaceAsync(default!); + } + + private static void EnableEmailTwoFactor(User user) => + user.SetTwoFactorProviders(new Dictionary + { + [TwoFactorProviderType.Email] = new TwoFactorProvider + { + MetaData = new Dictionary { ["Email"] = user.Email }, + Enabled = true + } + }); + + private static void ArrangeTakeover( + SutProvider sutProvider, User granteeUser, User grantor) + { + var emergencyAccess = new Core.Auth.Entities.EmergencyAccess + { + GrantorId = grantor.Id, + GranteeId = granteeUser.Id, + Status = EmergencyAccessStatusType.RecoveryApproved, + Type = EmergencyAccessType.Takeover, + }; + sutProvider.GetDependency() + .GetByIdAsync(Arg.Any()) + .Returns(emergencyAccess); + sutProvider.GetDependency() + .GetByIdAsync(grantor.Id) + .Returns(grantor); + } + + private static void ArrangeFinishRecoveryTakeover( + SutProvider sutProvider, User granteeUser, User grantor) + { + ArrangeTakeover(sutProvider, granteeUser, grantor); + sutProvider.GetDependency() + .PrepareSetInitialOrUpdateExistingMasterPasswordAsync(grantor, Arg.Any()) + .Returns(OneOf.FromT0(grantor)); + } } diff --git a/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/IssueTwoFactorRememberTokenCommandTests.cs b/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/IssueTwoFactorRememberTokenCommandTests.cs index 682789feadb4..09cf37d9bfd7 100644 --- a/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/IssueTwoFactorRememberTokenCommandTests.cs +++ b/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/IssueTwoFactorRememberTokenCommandTests.cs @@ -69,7 +69,7 @@ public async Task IssueAsync_UpsertsRowWithStampMatchingToken( } /// - /// Re-remembering a device writes a different stamp, which is what orphans the token that + /// Re-issuing a 2FA remember token for a device writes a different stamp, which is what orphans the token that /// device was holding. /// [Theory, BitAutoData] diff --git a/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ResetUserTwoFactorCommandTests.cs b/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ResetUserTwoFactorCommandTests.cs index 42d136383b58..0b364d4f7626 100644 --- a/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ResetUserTwoFactorCommandTests.cs +++ b/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ResetUserTwoFactorCommandTests.cs @@ -1,4 +1,5 @@ -using Bit.Core.Auth.UserFeatures.TwoFactorAuth.Implementations; +using Bit.Core.Auth.UserFeatures.TwoFactorAuth; +using Bit.Core.Auth.UserFeatures.TwoFactorAuth.Implementations; using Bit.Core.Entities; using Bit.Core.Repositories; using Bit.Test.Common.AutoFixture; @@ -77,4 +78,42 @@ await sutProvider.GetDependency() .Received(1) .ReplaceAsync(user); } + + [Theory, BitAutoData] + public async Task ResetAsync_RevokesTwoFactorRememberTokens(User user) + { + // Arrange + var sutProvider = GetSutProvider(); + + // Act + await sutProvider.Sut.ResetAsync(user); + + // Assert + await sutProvider.GetDependency() + .Received(1) + .RevokeAllForUserAsync(user.Id); + } + + /// + /// Revocation runs before the write that clears the providers, so a failure leaves two-factor + /// intact and the whole operation can simply be retried. A refactor that moves the call after + /// the write would break that, and nothing else would notice. + /// + [Theory, BitAutoData] + public async Task ResetAsync_RevocationFails_DoesNotClearTwoFactor(User user) + { + // Arrange + var sutProvider = GetSutProvider(); + sutProvider.GetDependency() + .RevokeAllForUserAsync(user.Id) + .Returns(Task.FromException(new InvalidOperationException("database unavailable"))); + + // Act + await Assert.ThrowsAsync(() => sutProvider.Sut.ResetAsync(user)); + + // Assert + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .ReplaceAsync(default!); + } } diff --git a/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/RevokeTwoFactorRememberTokensCommandTests.cs b/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/RevokeTwoFactorRememberTokensCommandTests.cs index 7df8efa7765b..0b1436a1fbef 100644 --- a/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/RevokeTwoFactorRememberTokensCommandTests.cs +++ b/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/RevokeTwoFactorRememberTokensCommandTests.cs @@ -35,8 +35,8 @@ await sutProvider.GetDependency() } /// - /// Revocation is scoped to remember-me. It must not delete rows, which would destroy the record - /// of when each device was first remembered. + /// Revocation is scoped to 2FA remember tokens. It must not delete rows, which would destroy the record + /// of when a 2FA remember token was first issued for each device. /// [Theory, BitAutoData] public async Task RevokeAllForUserAsync_DoesNotDeleteRows(Guid userId) diff --git a/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs b/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs index 17ac6c5cdd34..2c2d477b9d51 100644 --- a/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs +++ b/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs @@ -16,14 +16,29 @@ namespace Bit.Core.Test.Auth.UserFeatures.TwoFactorAuth; /// One test per rejection path. Each is asserted in isolation — a test that broke two checks at once /// would still pass if one of them were dropped from the implementation. /// +/// +/// Every time value here comes from one frozen at _now. The real clock +/// is never read, so the results do not depend on the day the tests run. +/// [SutProviderCustomize] public class ValidateTwoFactorRememberTokenQueryTests { private const string _deviceIdentifier = "device-identifier"; private const string _token = "protected-token"; + /// The instant the fake clock is frozen at. private static readonly DateTime _now = new(2026, 10, 2, 12, 0, 0, DateTimeKind.Utc); + /// + /// When a valid token and its row lapse. In production the row takes its expiry from the token, + /// so the two match. + /// + private static readonly DateTime _expiration = _now.AddDays(30); + + /// + /// Builds the query with a fake clock frozen at _now. The fake clock moves only when a test + /// calls SetUtcNow or Advance on it. + /// private static SutProvider GetSutProvider() { var sutProvider = new SutProvider() @@ -33,7 +48,7 @@ private static SutProvider GetSutProvider() return sutProvider; } - private static TwoFactorRememberTokenable Tokenable(User user, Guid deviceId, string stamp) => + private static TwoFactorRememberTokenable Tokenable(User user, Guid deviceId, string stamp, TimeProvider clock) => new() { UserId = user.Id, @@ -41,8 +56,12 @@ private static TwoFactorRememberTokenable Tokenable(User user, Guid deviceId, st DeviceIdentifier = _deviceIdentifier, Stamp = stamp, SecurityStamp = user.SecurityStamp, - // Real clock: ExpiringTokenable reads DateTime.UtcNow directly. - ExpirationDate = DateTime.UtcNow.AddDays(30), + ExpirationDate = _expiration, + // A token judges its own expiry with its own clock, which defaults to the system clock. In + // production a presented token is deserialized and keeps that default. Here the data protector is + // mocked, so the fake clock is assigned directly to judge the token against the same frozen + // instant as the row. + TimeProvider = clock, }; private static TwoFactorRememberToken Row(User user, Guid deviceId, string stamp) => @@ -52,7 +71,7 @@ private static TwoFactorRememberToken Row(User user, Guid deviceId, string stamp UserId = user.Id, DeviceId = deviceId, Stamp = stamp, - ExpirationDate = _now.AddDays(1), + ExpirationDate = _expiration, }; /// @@ -63,7 +82,7 @@ private static (TwoFactorRememberTokenable tokenable, TwoFactorRememberToken row SutProvider sutProvider, User user, Guid deviceId) { const string stamp = "row-stamp"; - var tokenable = Tokenable(user, deviceId, stamp); + var tokenable = Tokenable(user, deviceId, stamp, sutProvider.GetDependency()); var row = Row(user, deviceId, stamp); sutProvider.GetDependency>() @@ -100,7 +119,9 @@ public async Task ValidateAsync_RowStampRotated_ReturnsFalse( Assert.False(await sutProvider.Sut.ValidateAsync(user, _deviceIdentifier, _token)); } - /// The account-wide check inherited from the previous design. + /// + /// The account-wide check: rotating the user's security stamp invalidates every 2FA remember token they hold. + /// [Theory, BitAutoData] public async Task ValidateAsync_UserSecurityStampRotated_ReturnsFalse( User user, Guid deviceId) @@ -176,11 +197,11 @@ public async Task ValidateAsync_RowExpired_ReturnsFalse( } /// - /// Expiry is exclusive of the expiry instant, matching ExpiringTokenable.IsExpired: a row is - /// still honored at exactly its ExpirationDate and refused one tick later. + /// A row is honored through its ExpirationDate itself and refused from one tick later, the same + /// boundary ExpiringTokenable.IsExpired applies to the token. /// [Theory, BitAutoData] - public async Task ValidateAsync_RowExpiresExactlyNow_ReturnsTrue(User user, Guid deviceId) + public async Task ValidateAsync_RowExpirationEqualsExactlyNow_ReturnsTrue(User user, Guid deviceId) { var sutProvider = GetSutProvider(); var (_, row) = ArrangeValid(sutProvider, user, deviceId); @@ -211,16 +232,59 @@ public async Task ValidateAsync_TokenCannotBeUnprotected_ReturnsFalse( Assert.False(await sutProvider.Sut.ValidateAsync(user, _deviceIdentifier, _token)); } + /// The token's own expiry, which is checked before the row is read. [Theory, BitAutoData] public async Task ValidateAsync_TokenItselfExpired_ReturnsFalse( User user, Guid deviceId) { var sutProvider = GetSutProvider(); var (tokenable, _) = ArrangeValid(sutProvider, user, deviceId); - // Real clock on purpose: ExpiringTokenable.IsExpired reads DateTime.UtcNow directly, so the - // injected TimeProvider cannot reach it. - tokenable.ExpirationDate = DateTime.UtcNow.AddMinutes(-1); + tokenable.ExpirationDate = _now.AddMinutes(-1); + + Assert.False(await sutProvider.Sut.ValidateAsync(user, _deviceIdentifier, _token)); + } + + /// + /// The token is honored through its ExpirationDate itself and refused from one tick later, which is + /// what ExpiringTokenable.IsExpired enforces and the same boundary the row check applies to the row. + /// + [Theory, BitAutoData] + public async Task ValidateAsync_TokenExpirationEqualsExactlyNow_ReturnsTrue(User user, Guid deviceId) + { + var sutProvider = GetSutProvider(); + var (tokenable, _) = ArrangeValid(sutProvider, user, deviceId); + tokenable.ExpirationDate = _now; + + Assert.True(await sutProvider.Sut.ValidateAsync(user, _deviceIdentifier, _token)); + } + + [Theory, BitAutoData] + public async Task ValidateAsync_TokenExpiredOneTickAgo_ReturnsFalse(User user, Guid deviceId) + { + var sutProvider = GetSutProvider(); + var (tokenable, _) = ArrangeValid(sutProvider, user, deviceId); + tokenable.ExpirationDate = _now.AddTicks(-1); + + Assert.False(await sutProvider.Sut.ValidateAsync(user, _deviceIdentifier, _token)); + } + + /// + /// Moving the one shared clock forward is enough to expire both the token and its row: honored + /// right up to the expiry instant, refused one tick after it. + /// + [Theory, BitAutoData] + public async Task ValidateAsync_ClockAdvancesPastExpiration_StopsHonoringToken(User user, Guid deviceId) + { + var sutProvider = GetSutProvider(); + ArrangeValid(sutProvider, user, deviceId); + var clock = sutProvider.GetDependency(); + + Assert.True(await sutProvider.Sut.ValidateAsync(user, _deviceIdentifier, _token)); + + clock.Advance(_expiration - _now); + Assert.True(await sutProvider.Sut.ValidateAsync(user, _deviceIdentifier, _token)); + clock.Advance(TimeSpan.FromTicks(1)); Assert.False(await sutProvider.Sut.ValidateAsync(user, _deviceIdentifier, _token)); } diff --git a/test/Core.Test/Services/UserServiceTests.cs b/test/Core.Test/Services/UserServiceTests.cs index 8ed0d7a5ab39..8b7277d713dd 100644 --- a/test/Core.Test/Services/UserServiceTests.cs +++ b/test/Core.Test/Services/UserServiceTests.cs @@ -12,6 +12,7 @@ using Bit.Core.Auth.Enums; using Bit.Core.Auth.Models; using Bit.Core.Auth.Services; +using Bit.Core.Auth.UserFeatures.TwoFactorAuth; using Bit.Core.Auth.UserFeatures.TwoFactorAuth.Interfaces; using Bit.Core.Billing.Models; using Bit.Core.Billing.Models.Business; @@ -265,6 +266,83 @@ public async Task IsClaimedByAnyOrganizationAsync_WithOrganizationUseOrganizatio Assert.False(result); } + [Theory, BitAutoData] + public async Task DisableTwoFactorProviderAsync_TwoFactorAuthNoLongerEnabled_RevokesTwoFactorRememberTokens( + SutProvider sutProvider, User user) + { + // Arrange + user.SetTwoFactorProviders(new Dictionary + { + [TwoFactorProviderType.Email] = new() { Enabled = true } + }); + sutProvider.GetDependency() + .GetAsync(user.Id) + .Returns(new RequireTwoFactorPolicyRequirement([])); + + // Act + await sutProvider.Sut.DisableTwoFactorProviderAsync(user, TwoFactorProviderType.Email); + + // Assert + await sutProvider.GetDependency() + .Received(1) + .RevokeAllForUserAsync(user.Id); + } + + /// + /// Removing a provider such that 2FA is no longer enabled revokes the 2FA remember tokens before + /// anything is saved. If revocation fails, the removal is not persisted, so the user can simply + /// retry instead of being left with 2FA disabled and 2FA remember tokens that would still validate + /// upon re-enabling 2FA. + /// + [Theory, BitAutoData] + public async Task DisableTwoFactorProviderAsync_TwoFactorAuthNoLongerEnabled_RememberTokenRevocationFails_DoesNotSaveRemoval( + SutProvider sutProvider, User user) + { + // Arrange + user.SetTwoFactorProviders(new Dictionary + { + [TwoFactorProviderType.Email] = new() { Enabled = true } + }); + sutProvider.GetDependency() + .RevokeAllForUserAsync(user.Id) + .Returns(Task.FromException(new InvalidOperationException("database unavailable"))); + + // Act + await Assert.ThrowsAsync( + () => sutProvider.Sut.DisableTwoFactorProviderAsync(user, TwoFactorProviderType.Email)); + + // Assert + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .ReplaceAsync(default!); + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .RevokeNonCompliantOrganizationUsersAsync(default!); + } + + [Theory, BitAutoData] + public async Task DisableTwoFactorProviderAsync_OtherProvidersRemain_DoesNotRevoke( + SutProvider sutProvider, User user) + { + // Arrange + user.SetTwoFactorProviders(new Dictionary + { + [TwoFactorProviderType.Email] = new() { Enabled = true }, + [TwoFactorProviderType.Authenticator] = new() { Enabled = true } + }); + sutProvider.GetDependency() + .TwoFactorIsEnabledAsync(user) + .Returns(true); + + // Act + await sutProvider.Sut.DisableTwoFactorProviderAsync(user, TwoFactorProviderType.Email); + + // Assert + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .RevokeAllForUserAsync(default); + } + [Theory, BitAutoData] public async Task DisableTwoFactorProviderAsync_WhenOrganizationHas2FAPolicyEnabled_DisablingAllProviders_RevokesUserAndSendsEmail( SutProvider sutProvider, User user, @@ -468,6 +546,67 @@ await sutProvider.GetDependency() .LogUserEventAsync(user.Id, EventType.User_Recovered2fa); } + [Theory, BitAutoData] + public async Task RecoverTwoFactorAsync_CorrectCode_RevokesTwoFactorRememberTokens( + User user, SutProvider sutProvider) + { + // Arrange + var recoveryCode = "1234"; + user.TwoFactorRecoveryCode = recoveryCode; + sutProvider.GetDependency() + .GetAsync(user.Id) + .Returns(new RequireTwoFactorPolicyRequirement([])); + + // Act + await sutProvider.Sut.RecoverTwoFactorAsync(user, recoveryCode); + + // Assert + await sutProvider.GetDependency() + .Received(1) + .RevokeAllForUserAsync(user.Id); + } + + [Theory, BitAutoData] + public async Task RecoverTwoFactorAsync_IncorrectCode_DoesNotRevoke( + User user, SutProvider sutProvider) + { + // Arrange + user.TwoFactorRecoveryCode = "4567"; + + // Act + await sutProvider.Sut.RecoverTwoFactorAsync(user, "1234"); + + // Assert + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .RevokeAllForUserAsync(default); + } + + /// + /// Revocation runs before the save that consumes the recovery code. If it ran after, a failure + /// would commit the teardown and lose the revocation permanently: the user's next login carries + /// no recovery code, so this method is never reached again. + /// + [Theory, BitAutoData] + public async Task RecoverTwoFactorAsync_RevocationFails_DoesNotConsumeRecoveryCode( + User user, SutProvider sutProvider) + { + // Arrange + var recoveryCode = "1234"; + user.TwoFactorRecoveryCode = recoveryCode; + sutProvider.GetDependency() + .RevokeAllForUserAsync(user.Id) + .Returns(Task.FromException(new InvalidOperationException("database unavailable"))); + + // Act + await Assert.ThrowsAsync( + () => sutProvider.Sut.RecoverTwoFactorAsync(user, recoveryCode)); + + // Assert + Assert.Equal(recoveryCode, user.TwoFactorRecoveryCode); + Assert.NotNull(user.TwoFactorProviders); + } + [Theory, BitAutoData] public async Task RecoverTwoFactorAsync_IncorrectCode_ReturnsFalse( User user, SutProvider sutProvider) diff --git a/test/Identity.IntegrationTest/Endpoints/IdentityServerTwoFactorTests.cs b/test/Identity.IntegrationTest/Endpoints/IdentityServerTwoFactorTests.cs index fdc2d5edeff2..f91e096cf743 100644 --- a/test/Identity.IntegrationTest/Endpoints/IdentityServerTwoFactorTests.cs +++ b/test/Identity.IntegrationTest/Endpoints/IdentityServerTwoFactorTests.cs @@ -651,7 +651,7 @@ public async Task TokenEndpoint_GrantTypePassword_ValidRecoveryCode_SuccessAndTw } /// - /// The remember-me token returned by an earlier two-factor login completes a later login on its own. + /// The 2FA remember token returned by an earlier two-factor login completes a later login on its own. /// [Fact] public async Task TokenEndpoint_GrantTypePassword_ValidRememberToken_Success() diff --git a/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs b/test/Identity.IntegrationTest/Endpoints/TwoFactorRememberTokenLoginTests.cs similarity index 58% rename from test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs rename to test/Identity.IntegrationTest/Endpoints/TwoFactorRememberTokenLoginTests.cs index f175298b3fbe..1ae5517c77e3 100644 --- a/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs +++ b/test/Identity.IntegrationTest/Endpoints/TwoFactorRememberTokenLoginTests.cs @@ -2,6 +2,7 @@ using Bit.Core.Auth.Enums; using Bit.Core.Auth.Models.Api.Request.Accounts; using Bit.Core.Auth.Models.Business.Tokenables; +using Bit.Core.Auth.Repositories; using Bit.Core.Auth.Services; using Bit.Core.Entities; using Bit.Core.Enums; @@ -18,12 +19,12 @@ namespace Bit.Identity.IntegrationTest.Endpoints; /// End-to-end coverage for the "remember this device" flow: a token issued by one login is honored /// by the next, and is refused when it should not be. /// -public class RememberedDeviceLoginTests +public class TwoFactorRememberTokenLoginTests { - private const string _testEmail = "test+rememberdevice@email.com"; + private const string _testEmail = "test+twofactorremembertoken@email.com"; private const string _testPassword = "master_password_hash"; private const string _userEmailTwoFactor = - """{"1": { "Enabled": true, "MetaData": { "Email": "test+rememberdevice@email.com"}}}"""; + """{"1": { "Enabled": true, "MetaData": { "Email": "test+twofactorremembertoken@email.com"}}}"""; /// /// The numeric value of . @@ -32,6 +33,9 @@ public class RememberedDeviceLoginTests private const string _emailProvider = "1"; + /// The numeric value of . + private const string _recoveryCodeProvider = "8"; + private static async Task<(IdentityApplicationFactory Factory, User User)> CreateFactoryWithTwoFactorUserAsync() { var factory = new IdentityApplicationFactory(); @@ -73,7 +77,7 @@ private static async Task EmailLoginCodeAsync( } /// - /// A two-factor login that asks to be remembered gets a token back. + /// A two-factor login that asks for a 2FA remember token gets one back. /// [Fact] public async Task RememberRequested_ResponseCarriesRememberToken() @@ -119,8 +123,8 @@ public async Task RememberToken_ReplayedOnNextLogin_SkipsTwoFactorChallenge() } /// - /// A login that presents a remember token must not be issued another one, or an active user's - /// remember-me would renew forever. Replayed with and without asking to be remembered again, since + /// A login that presents a 2FA remember token must not be issued another one, or an active user's + /// 2FA remember token would renew forever. Replayed with and without asking for another 2FA remember token, since /// a client can send TwoFactorRemember=1 regardless. /// [Theory] @@ -204,6 +208,128 @@ public async Task LegacyRememberToken_IsUpgradedToCurrentFormat() AssertHelper.AssertJsonProperty(replayBody.RootElement, "access_token", JsonValueKind.String); } + /// + /// Using a recovery code tears two-factor down, and the user's 2FA remember tokens + /// stop being honored with it. + /// + /// + /// The replay happens after two-factor is re-enabled, deliberately. While the account + /// has no second factor, no challenge is issued at all and the replay would log in on the + /// password alone, so it would say nothing about whether the revocation worked. + /// + /// The recovery login is sent with and without asking for a 2FA remember token. Either way it must not + /// come back with a new 2FA remember token, which would undo the revocation for that device. + /// + /// + [Theory] + [InlineData("0")] + [InlineData("1")] + public async Task RecoveryCodeTeardown_RevokesTwoFactorRememberTokens(string twoFactorRemember) + { + var (factory, user) = await CreateFactoryWithTwoFactorUserAsync(); + var userRepository = factory.Services.GetRequiredService(); + + const string recoveryCode = "recoverycode123"; + user.TwoFactorRecoveryCode = recoveryCode; + await userRepository.ReplaceAsync(user); + + var emailCode = await EmailLoginCodeAsync( + factory, _testEmail, IdentityApplicationFactory.DefaultDeviceIdentifier); + var (_, rememberToken) = await factory.TokensFromPasswordWithTwoFactorAsync( + _testEmail, _testPassword, twoFactorProviderType: _emailProvider, twoFactorToken: emailCode); + Assert.False(string.IsNullOrEmpty(rememberToken)); + + // Tear down two-factor with the recovery code. The provider is sent as its numeric value because + // BaseRequestValidator.DetermineValidationOrder only recognises a recovery-code request via int.TryParse. + var teardown = await factory.ContextFromPasswordWithTwoFactorAsync( + _testEmail, + _testPassword, + twoFactorProviderType: _recoveryCodeProvider, + twoFactorToken: recoveryCode, + twoFactorRemember: twoFactorRemember); + using (var teardownBody = await AssertHelper.AssertResponseTypeIs(teardown)) + { + AssertHelper.AssertJsonProperty(teardownBody.RootElement, "access_token", JsonValueKind.String); + Assert.False(teardownBody.RootElement.TryGetProperty("TwoFactorToken", out _)); + } + + // Put a second factor back, so the replay below is decided by the 2FA remember token state + // rather than by the account simply having no second factor. + var recoveredUser = await userRepository.GetByEmailAsync(_testEmail); + Assert.NotNull(recoveredUser); + recoveredUser.TwoFactorProviders = _userEmailTwoFactor; + await userRepository.ReplaceAsync(recoveredUser); + + var replay = await factory.ContextFromPasswordWithTwoFactorAsync( + _testEmail, + _testPassword, + twoFactorProviderType: _rememberProvider, + twoFactorToken: rememberToken!, + twoFactorRemember: "0"); + + using var replayBody = await AssertHelper.AssertResponseTypeIs(replay); + var root = replayBody.RootElement; + + Assert.False(root.TryGetProperty("access_token", out _)); + var error = AssertHelper.AssertJsonProperty(root, "error_description", JsonValueKind.String).GetString(); + Assert.Equal("Two factor required.", error); + } + + /// + /// A wrong recovery code must not tear anything down. Revocation only happens once the code is verified, + /// so a failed attempt leaves the user's 2FA remember tokens honored. Otherwise anyone who knows the + /// password could cut off every remembered device by guessing. + /// + [Fact] + public async Task WrongRecoveryCode_DoesNotRevokeTwoFactorRememberTokens() + { + var (factory, user) = await CreateFactoryWithTwoFactorUserAsync(); + var userRepository = factory.Services.GetRequiredService(); + + user.TwoFactorRecoveryCode = "recoverycode123"; + await userRepository.ReplaceAsync(user); + + var emailCode = await EmailLoginCodeAsync( + factory, _testEmail, IdentityApplicationFactory.DefaultDeviceIdentifier); + var (_, rememberToken) = await factory.TokensFromPasswordWithTwoFactorAsync( + _testEmail, _testPassword, twoFactorProviderType: _emailProvider, twoFactorToken: emailCode); + Assert.False(string.IsNullOrEmpty(rememberToken)); + + var device = await factory.GetService() + .GetByIdentifierAsync(IdentityApplicationFactory.DefaultDeviceIdentifier, user.Id); + Assert.NotNull(device); + var rememberTokenRepository = factory.GetService(); + var rowBefore = await rememberTokenRepository.GetByUserIdDeviceIdAsync(user.Id, device.Id); + Assert.NotNull(rowBefore); + + // Attempt recovery with the wrong code. It must be refused. + var wrongCode = await factory.ContextFromPasswordWithTwoFactorAsync( + _testEmail, + _testPassword, + twoFactorProviderType: _recoveryCodeProvider, + twoFactorToken: "not-the-recovery-code", + twoFactorRemember: "0"); + using (var wrongCodeBody = await AssertHelper.AssertResponseTypeIs(wrongCode)) + { + Assert.False(wrongCodeBody.RootElement.TryGetProperty("access_token", out _)); + } + + // The row's stamp is untouched... + var rowAfter = await rememberTokenRepository.GetByUserIdDeviceIdAsync(user.Id, device.Id); + Assert.NotNull(rowAfter); + Assert.Equal(rowBefore.Stamp, rowAfter.Stamp); + + // ...so the old 2FA remember token still skips the challenge. + var replay = await factory.ContextFromPasswordWithTwoFactorAsync( + _testEmail, + _testPassword, + twoFactorProviderType: _rememberProvider, + twoFactorToken: rememberToken!, + twoFactorRemember: "0"); + using var replayBody = await AssertHelper.AssertResponseTypeIs(replay); + AssertHelper.AssertJsonProperty(replayBody.RootElement, "access_token", JsonValueKind.String); + } + /// /// The token is bound to the device it was issued to, so presenting it from another device /// falls back to a two-factor challenge. @@ -230,8 +356,8 @@ public async Task RememberToken_PresentedFromDifferentDevice_IsRefused() using var body = await AssertHelper.AssertResponseTypeIs(context); var root = body.RootElement; - // A refused remember token falls back to a challenge rather than a generic auth failure: - // the password was fine, the device just is not trusted. + // A refused 2FA remember token falls back to a challenge rather than a generic auth failure: + // the password was fine, the 2FA remember token is just no longer honored. Assert.False(root.TryGetProperty("access_token", out _)); var error = AssertHelper.AssertJsonProperty(root, "error_description", JsonValueKind.String).GetString(); Assert.Equal("Two factor required.", error); diff --git a/test/Identity.Test/IdentityServer/BaseRequestValidatorTests.cs b/test/Identity.Test/IdentityServer/BaseRequestValidatorTests.cs index f829d82c7f19..aef6bd29b31e 100644 --- a/test/Identity.Test/IdentityServer/BaseRequestValidatorTests.cs +++ b/test/Identity.Test/IdentityServer/BaseRequestValidatorTests.cs @@ -1172,6 +1172,79 @@ await _userRepository.Received(1).ReplaceAsync(Arg.Is(u => "TwoFactorRecoveryRequested flag should be set for audit/logging"); } + /// + /// A successful 2FA login that asks for a 2FA remember token (TwoFactorRemember = "1") is only issued one + /// when the provider allows it. A Remember login must not renew itself, and a RecoveryCode + /// login removes 2FA, so neither mints a new token. Email is the control: it proves that the + /// request really would have been issued a token, so the other two cases can fail. + /// + [Theory] + [BitAutoData(TwoFactorProviderType.RecoveryCode, false)] + [BitAutoData(TwoFactorProviderType.Remember, false)] + [BitAutoData(TwoFactorProviderType.Email, true)] + public async Task ValidateAsync_TwoFactorRememberRequested_IssuesTwoFactorRememberTokenOnlyForAllowedProviders( + TwoFactorProviderType providerType, + bool expectTokenIssued, + [AuthFixtures.ValidatedTokenRequest] ValidatedTokenRequest tokenRequest, + [AuthFixtures.CustomValidatorRequestContext] + CustomValidatorRequestContext requestContext, + GrantValidationResult grantResult) + { + // Arrange + var context = CreateContext(tokenRequest, requestContext, grantResult); + var user = requestContext.User; + const string twoFactorToken = "valid-two-factor-token"; + + _sut.isValid = true; + + _ssoRequestValidator.ValidateAsync(requestContext.User, tokenRequest, requestContext) + .Returns(Task.FromResult(true)); + + _twoFactorAuthenticationValidator + .RequiresTwoFactorAsync(user, tokenRequest) + .Returns(Task.FromResult(new Tuple(true, null))); + + // The user asks for a 2FA remember token and completes 2FA with the provider under test + tokenRequest.Raw["TwoFactorRemember"] = "1"; + tokenRequest.Raw["TwoFactorProvider"] = ((int)providerType).ToString(); + tokenRequest.Raw["TwoFactorToken"] = twoFactorToken; + + _twoFactorAuthenticationValidator + .VerifyTwoFactorAsync(user, null, providerType, twoFactorToken, Arg.Any()) + .Returns(Task.FromResult(new TwoFactorVerificationResult(true))); + + _deviceValidator.ValidateRequestDeviceAsync(tokenRequest, requestContext) + .Returns(Task.FromResult(true)); + + _userService.IsLegacyUser(Arg.Any()) + .Returns(false); + + _userAccountKeysQuery.Run(Arg.Any()).Returns(new UserAccountKeysData + { + PublicKeyEncryptionKeyPairData = new PublicKeyEncryptionKeyPairData( + "test-private-key", + "test-public-key" + ) + }); + + // Act + await _sut.ValidateAsync(context); + + // Assert + Assert.False(context.GrantResult.IsError, "Authentication should succeed for the provider under test"); + + if (expectTokenIssued) + { + await _issueTwoFactorRememberTokenCommand.Received(1) + .IssueAsync(Arg.Any(), Arg.Any()); + } + else + { + await _issueTwoFactorRememberTokenCommand.DidNotReceive() + .IssueAsync(Arg.Any(), Arg.Any()); + } + } + [Theory] [BitAutoData] public async Task ValidateAsync_ClientVersionValidator_IsInvoked( diff --git a/test/Infrastructure.IntegrationTest/Auth/Repositories/TwoFactorRememberTokenRepositoryTests.cs b/test/Infrastructure.IntegrationTest/Auth/Repositories/TwoFactorRememberTokenRepositoryTests.cs index 0ccf0ce482c8..a24fee0a09cd 100644 --- a/test/Infrastructure.IntegrationTest/Auth/Repositories/TwoFactorRememberTokenRepositoryTests.cs +++ b/test/Infrastructure.IntegrationTest/Auth/Repositories/TwoFactorRememberTokenRepositoryTests.cs @@ -100,7 +100,7 @@ public async Task UpsertAsync_SameUserAndDevice_UpdatesInPlaceAndPreservesCreati Assert.NotNull(updated); Assert.Equal("stamp-two", updated.Stamp); - // Same row rather than a second one, and the row remembers when the device was first trusted. + // Same row rather than a second one, and the row still records when a 2FA remember token was first issued for the device. Assert.Equal(inserted.Id, updated.Id); Assert.Equal(inserted.CreationDate, updated.CreationDate); Assert.True(updated.RevisionDate >= inserted.RevisionDate); @@ -111,7 +111,7 @@ public async Task UpsertAsync_SameUserAndDevice_UpdatesInPlaceAndPreservesCreati // ------------------------------------------------------------------------------------------- /// - /// Two logins requesting "remember me" for the same device at the same instant must not produce a + /// Two logins requesting a 2FA remember token for the same device at the same instant must not produce a /// unique-key violation, which would surface to the user as a failed login. /// /// @@ -296,7 +296,7 @@ public async Task RotateStampsByUserIdAsync_RotatesEveryRowForThatUser( Assert.NotNull(afterOne); Assert.NotNull(afterTwo); - // Every device the user remembered is cut off, and the rows are kept rather than deleted. + // Every 2FA remember token the user holds is cut off, and the rows are kept rather than deleted. Assert.NotEqual(beforeOne.Stamp, afterOne.Stamp); Assert.NotEqual(beforeTwo.Stamp, afterTwo.Stamp); Assert.Equal(beforeOne.Id, afterOne.Id); diff --git a/test/IntegrationTestCommon/Factories/IdentityApplicationFactory.cs b/test/IntegrationTestCommon/Factories/IdentityApplicationFactory.cs index d173ce8f626c..21222366165f 100644 --- a/test/IntegrationTestCommon/Factories/IdentityApplicationFactory.cs +++ b/test/IntegrationTestCommon/Factories/IdentityApplicationFactory.cs @@ -154,8 +154,8 @@ public async Task ContextFromPasswordAsync( } /// - /// "1" asks the server to remember the device. Pass "0" to mirror a client re-authenticating - /// with a stored remember token, which is what clients actually send in that case. + /// "1" asks the server to issue a 2FA remember token. Pass "0" to mirror a client re-authenticating + /// with a stored 2FA remember token, which is what clients actually send in that case. /// public async Task ContextFromPasswordWithTwoFactorAsync( string username, @@ -188,7 +188,7 @@ public async Task ContextFromPasswordWithTwoFactorAsync( /// /// Completes a two-factor login and returns the access token alongside the TwoFactorToken - /// response field, which is the remember token when the server issued one. + /// response field, which is the 2FA remember token when the server issued one. /// /// /// RememberToken is null when the response carried no TwoFactorToken field at all —