From faecd60927e143c97d3ba03c24f31f1097467ed3 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Tue, 22 Sep 2026 21:21:26 -0500 Subject: [PATCH 01/28] [PM-40673 Part 3] update 2FA remember token revoke call sites --- .../ResetUserTwoFactorCommand.cs | 4 + .../Auth/UserFeatures/TwoFactorAuth/readme.md | 22 ++++ .../Services/Implementations/UserService.cs | 13 ++ ...ationUsersControllerRecoverAccountTests.cs | 112 +++++++++++++++++ .../TwoFactorControllerEmailTests.cs | 49 ++++++++ .../AdminRecoverAccountCommandTests.cs | 5 + .../ResetUserTwoFactorCommandTests.cs | 41 +++++- test/Core.Test/Services/UserServiceTests.cs | 119 ++++++++++++++++++ .../Endpoints/RememberedDeviceLoginTests.cs | 62 +++++++++ 9 files changed, 426 insertions(+), 1 deletion(-) 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..05132673b21e 100644 --- a/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md +++ b/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md @@ -2,6 +2,28 @@ 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. +## Remembered devices + +A user completing 2FA can ask for the device to be remembered. The server records the device in `TwoFactorRememberToken` — one row per `(UserId, DeviceId)` — and issues a `TwoFactorRememberTokenable` carrying a copy of that row's `Stamp`. On a later login the client presents the token as `TwoFactorProviderType.Remember` and skips the challenge. + +### Revocation + +The token is only honored while the stamp it carries still matches the row's. `IRevokeTwoFactorRememberTokensCommand.RevokeAllForUserAsync` writes a new stamp to every row for a user, which stops all of their remember tokens being honored at once. The rows are kept, so `CreationDate` still records when each device was first remembered and `RevisionDate` records when it was cut off. + +Revocation is deliberately narrow. It affects remember-me only — access tokens, refresh tokens, and live sessions are untouched, so it does not sign the user out anywhere. Their devices simply have to complete 2FA again on the next login. + +Three flows revoke, all of them teardowns that leave the account with no 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 their **last** 2FA provider | `UserService.DisableTwoFactorProviderAsync` | + +Removing one of several providers does **not** revoke: the user still has working 2FA, so their remembered devices stay trusted. Untrusting a device does not revoke either — that is a separate trusted-device concept. + +Two further properties hold without any explicit call. A password change or account key rotation rotates `User.SecurityStamp`, which remember tokens also carry and which is compared on every use. And a remember token is never honored while the account has no 2FA configured at all, whatever the row says — so a teardown that somehow failed to revoke still cannot let a stale token stand in as a second factor until a new provider is enrolled. + ## 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. diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs index ea1b9c7782c9..dd0485dfd5ce 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; @@ -750,6 +754,10 @@ public async Task DisableTwoFactorProviderAsync(User user, TwoFactorProviderType if (!await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(user)) { + // Runs post-save because whether this was the last provider is only known once the + // remaining providers are known. Re-running the method cannot retry this: the provider + // is already gone, so the early return at the top takes over. + await _revokeTwoFactorRememberTokensCommand.RevokeAllForUserAsync(user.Id); await CheckPoliciesOnTwoFactorRemovalAsync(user); } } @@ -763,6 +771,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/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs index a89d62546142..223bd4c4a371 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,114 @@ private async Task LoginAsAccountRecoveryManagerWithMorePermis return targetCustomOrgUser; } + + /// ResetMasterPassword, which is a different code path. + /// + /// + /// Asserted on the stored stamp, which is the value a presented token is compared against, so a + /// changed stamp is what "the previously issued tokens no longer work" means. The end-to-end + /// replay is covered in the Identity integration tests. + /// + [Fact] + public async Task RecoverAccount_ResetTwoFactorOnly_RevokesRememberedDevices() + { + // 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 SeedRememberedDeviceAsync(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 + /// must leave remembered devices alone — over-firing would sign people out of remember-me for + /// no reason. + /// + [Fact] + public async Task RecoverAccount_ResetMasterPasswordOnly_LeavesRememberedDevicesAlone() + { + // 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 SeedRememberedDeviceAsync(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 = "encrypted-recovery-key", + }); + + // 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); + } + + private async Task<(Device Device, TwoFactorRememberToken Row)> SeedRememberedDeviceAsync( + 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/TwoFactor/TwoFactorControllerEmailTests.cs b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs index ccaf63927370..9181261c76e8 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs @@ -845,6 +845,55 @@ public async Task EmailLoginCode_StoredInPreviousFormat_Rejected() AssertTwoFactorRejected(result); } + /// + /// Removing the user's last two-factor provider stops their remembered devices from being + /// trusted. Email is the only provider in this fixture, so the real DELETE below is the + /// last-provider path. + /// + /// + /// Asserted on the stored stamp rather than by replaying a token: the stamp is the value a + /// presented token is compared against, so a changed stamp is exactly what "the previously + /// issued tokens no longer work" means. Replaying end-to-end is covered in the Identity tests. + /// + [Fact] + public async Task DeleteEmail_RemovesLastProvider_RevokesRememberedDevices() + { + await EnrollUserInEmail(); + var user = (await _userRepository.GetByEmailAsync(_userEmail))!; + + var deviceRepository = _factory.GetService(); + var rememberTokenRepository = _factory.GetService(); + + var device = await deviceRepository.CreateAsync(new Device + { + UserId = user.Id, + Name = "chrome-test", + Type = DeviceType.ChromeBrowser, + Identifier = Guid.NewGuid().ToString(), + Active = true, + LastActivityDate = DateTime.UtcNow, + }); + await rememberTokenRepository.UpsertAsync(new TwoFactorRememberToken + { + UserId = user.Id, + DeviceId = device.Id, + Stamp = "stamp-before-teardown", + ExpirationDate = DateTime.UtcNow.AddDays(30), + }); + + var getResponse = await _client.PostAsJsonAsync("/two-factor/get-email", + new { MasterPasswordHash = MasterPasswordHash }); + var (_, uvToken) = await ReadEnabledAndUserVerificationTokenAsync(getResponse, "email"); + + var disableResponse = await SendJsonAsync(_client, HttpMethod.Delete, "/two-factor/email", + new TwoFactorEmailDeleteRequestModel { UserVerificationToken = uvToken }); + Assert.Equal(HttpStatusCode.NoContent, disableResponse.StatusCode); + + var row = await rememberTokenRepository.GetByUserIdDeviceIdAsync(user.Id, device.Id); + Assert.NotNull(row); + Assert.NotEqual("stamp-before-teardown", row.Stamp); + } + // --------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------- diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/AccountRecovery/AdminRecoverAccountCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/AccountRecovery/AdminRecoverAccountCommandTests.cs index f44a4b746cc1..2aff7bf8b29d 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/AccountRecovery/AdminRecoverAccountCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/AccountRecovery/AdminRecoverAccountCommandTests.cs @@ -76,6 +76,11 @@ await sutProvider.GetDependency().DidNotReceive().LogOrganization Arg.Any(), Arg.Is(EventType.OrganizationUser_AdminResetTwoFactor)); + // The two flags are independent, and a password-only recovery must not tear down two-factor + // state — including the devices the user has remembered. + 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/TwoFactorAuth/ResetUserTwoFactorCommandTests.cs b/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ResetUserTwoFactorCommandTests.cs index 42d136383b58..9fc5f63b3216 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_RevokesRememberedDevices(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/Services/UserServiceTests.cs b/test/Core.Test/Services/UserServiceTests.cs index 8ed0d7a5ab39..87f551dd8530 100644 --- a/test/Core.Test/Services/UserServiceTests.cs +++ b/test/Core.Test/Services/UserServiceTests.cs @@ -10,6 +10,7 @@ using Bit.Core.AdminConsole.OrganizationFeatures.Policies.PolicyRequirements; using Bit.Core.AdminConsole.Repositories; using Bit.Core.Auth.Enums; +using Bit.Core.Auth.UserFeatures.TwoFactorAuth; using Bit.Core.Auth.Models; using Bit.Core.Auth.Services; using Bit.Core.Auth.UserFeatures.TwoFactorAuth.Interfaces; @@ -265,6 +266,59 @@ public async Task IsClaimedByAnyOrganizationAsync_WithOrganizationUseOrganizatio Assert.False(result); } + /// + /// Removing the last provider leaves the account with no second factor, so the devices that were + /// trusted under the old one should stop being trusted. + /// + [Theory, BitAutoData] + public async Task DisableTwoFactorProviderAsync_LastProviderRemoved_RevokesRememberedDevices( + 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); + } + + /// + /// Going from several providers to fewer leaves working two-factor in place, so remembered + /// devices stay trusted. Over-firing here would log people out of remember-me for no reason. + /// + [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 +522,71 @@ await sutProvider.GetDependency() .LogUserEventAsync(user.Id, EventType.User_Recovered2fa); } + [Theory, BitAutoData] + public async Task RecoverTwoFactorAsync_CorrectCode_RevokesRememberedDevices( + 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); + } + + /// + /// An unverified request must not be able to rotate anything, so revocation sits after the + /// recovery-code comparison. + /// + [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/RememberedDeviceLoginTests.cs b/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs index f175298b3fbe..659ad79f75e2 100644 --- a/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs +++ b/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs @@ -32,6 +32,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(); @@ -204,6 +207,65 @@ public async Task LegacyRememberToken_IsUpgradedToCurrentFormat() AssertHelper.AssertJsonProperty(replayBody.RootElement, "access_token", JsonValueKind.String); } + /// + /// Using a recovery code tears two-factor down, and the devices trusted under the old + /// configuration stop being trusted with it. + /// + /// + /// The replay happens after two-factor is re-enabled, deliberately. While the account + /// has no second factor the token is refused for that reason alone, so a test that stopped at + /// the teardown would pass even with the revocation removed entirely — it would prove the + /// product behavior and nothing about the mechanism. + /// + [Fact] + public async Task RecoveryCodeTeardown_RevokesRememberedDevices() + { + var (factory, user) = await CreateFactoryWithTwoFactorUserAsync(); + var userRepository = factory.Services.GetRequiredService(); + + const string recoveryCode = "recoverycode123"; + user.TwoFactorRecoveryCode = recoveryCode; + await userRepository.ReplaceAsync(user); + + var (_, rememberToken) = await factory.TokensFromPasswordWithTwoFactorAsync( + _testEmail, _testPassword, twoFactorProviderType: _emailProvider, twoFactorToken: _emailToken); + Assert.False(string.IsNullOrEmpty(rememberToken)); + + // Tear down two-factor with the recovery code. Provider sent as a number, per the validator + // ordering note on the factory helper. + var teardown = await factory.ContextFromPasswordWithTwoFactorAsync( + _testEmail, + _testPassword, + twoFactorProviderType: _recoveryCodeProvider, + twoFactorToken: recoveryCode, + twoFactorRemember: "0"); + using (var teardownBody = await AssertHelper.AssertResponseTypeIs(teardown)) + { + AssertHelper.AssertJsonProperty(teardownBody.RootElement, "access_token", JsonValueKind.String); + } + + // Put a second factor back, so the replay below is decided by the remembered-device 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); + } + /// /// The token is bound to the device it was issued to, so presenting it from another device /// falls back to a two-factor challenge. From 787eeb1cb2cf9bfdc843a9f4337a83365aabd9f2 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Sat, 17 Oct 2026 07:51:34 -0500 Subject: [PATCH 02/28] [PM-40673 Part 3] update README and test comment for removed 2FA-enabled check --- src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md | 2 +- .../Endpoints/RememberedDeviceLoginTests.cs | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md b/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md index 05132673b21e..1bdfea6415c2 100644 --- a/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md +++ b/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md @@ -22,7 +22,7 @@ Three flows revoke, all of them teardowns that leave the account with no second Removing one of several providers does **not** revoke: the user still has working 2FA, so their remembered devices stay trusted. Untrusting a device does not revoke either — that is a separate trusted-device concept. -Two further properties hold without any explicit call. A password change or account key rotation rotates `User.SecurityStamp`, which remember tokens also carry and which is compared on every use. And a remember token is never honored while the account has no 2FA configured at all, whatever the row says — so a teardown that somehow failed to revoke still cannot let a stale token stand in as a second factor until a new provider is enrolled. +Two further properties hold without any explicit call. A password change or account key rotation rotates `User.SecurityStamp`, which remember tokens also carry and which is compared on every use. And while the account has no 2FA at all, whether its own or enforced by its organization, no second factor is requested at login, so a remember token is not consulted. ## User Verification diff --git a/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs b/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs index 659ad79f75e2..35c22dddc638 100644 --- a/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs +++ b/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs @@ -213,9 +213,8 @@ public async Task LegacyRememberToken_IsUpgradedToCurrentFormat() /// /// /// The replay happens after two-factor is re-enabled, deliberately. While the account - /// has no second factor the token is refused for that reason alone, so a test that stopped at - /// the teardown would pass even with the revocation removed entirely — it would prove the - /// product behavior and nothing about the mechanism. + /// 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. /// [Fact] public async Task RecoveryCodeTeardown_RevokesRememberedDevices() From 013fa6107eed848dafc4edd7228a872bf9e230a9 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Sat, 17 Oct 2026 09:10:24 -0500 Subject: [PATCH 03/28] [PM-40673 Part 3] run policy enforcement before remember-token revoke when the last provider is removed. --- .../Services/Implementations/UserService.cs | 10 ++-- test/Core.Test/Services/UserServiceTests.cs | 47 +++++++++++++++++++ 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs index dd0485dfd5ce..e2eadfd029e3 100644 --- a/src/Core/Services/Implementations/UserService.cs +++ b/src/Core/Services/Implementations/UserService.cs @@ -755,10 +755,14 @@ public async Task DisableTwoFactorProviderAsync(User user, TwoFactorProviderType if (!await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(user)) { // Runs post-save because whether this was the last provider is only known once the - // remaining providers are known. Re-running the method cannot retry this: the provider - // is already gone, so the early return at the top takes over. - await _revokeTwoFactorRememberTokensCommand.RevokeAllForUserAsync(user.Id); + // remaining providers are known. Re-running the method cannot retry either step below: + // the provider is already gone, so the early return at the top takes over. + + // When the last 2FA provider is removed, CheckPoliciesOnTwoFactorRemovalAsync revokes the + // user from organizations that require 2FA. This check goes before remember-token revocation + // so that a failing remember-token revoke cannot skip it. await CheckPoliciesOnTwoFactorRemovalAsync(user); + await _revokeTwoFactorRememberTokensCommand.RevokeAllForUserAsync(user.Id); } } diff --git a/test/Core.Test/Services/UserServiceTests.cs b/test/Core.Test/Services/UserServiceTests.cs index 87f551dd8530..3a63d86d9880 100644 --- a/test/Core.Test/Services/UserServiceTests.cs +++ b/test/Core.Test/Services/UserServiceTests.cs @@ -292,6 +292,53 @@ await sutProvider.GetDependency() .RevokeAllForUserAsync(user.Id); } + /// + /// Removing the last provider also revokes the user from organizations that require two-step login. + /// That enforcement runs before the remember-token revoke, so a failing remember-token revoke cannot + /// skip it. + /// + [Theory, BitAutoData] + public async Task DisableTwoFactorProviderAsync_LastProviderRemoved_RememberTokenRevocationFails_StillEnforcesPolicies( + SutProvider sutProvider, User user, Organization organization, Guid organizationUserId) + { + // Arrange + user.SetTwoFactorProviders(new Dictionary + { + [TwoFactorProviderType.Email] = new() { Enabled = true } + }); + organization.Enabled = true; + organization.UseSso = true; + sutProvider.GetDependency() + .GetAsync(user.Id) + .Returns(new RequireTwoFactorPolicyRequirement( + [ + new PolicyDetails + { + OrganizationId = organization.Id, + OrganizationUserId = organizationUserId, + OrganizationUserStatus = OrganizationUserStatusType.Confirmed, + PolicyType = PolicyType.TwoFactorAuthentication + } + ])); + sutProvider.GetDependency() + .GetManyByIdsAsync(Arg.Is>(ids => ids.Contains(organization.Id))) + .Returns(new[] { organization }); + 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() + .Received(1) + .RevokeNonCompliantOrganizationUsersAsync( + Arg.Is(r => r.OrganizationId == organization.Id && + r.OrganizationUsers.First().Id == organizationUserId)); + } + /// /// Going from several providers to fewer leaves working two-factor in place, so remembered /// devices stay trusted. Over-firing here would log people out of remember-me for no reason. From 85b9ac37995db24219a624d62d2a8c2d8a421537 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Sat, 17 Oct 2026 16:14:13 -0500 Subject: [PATCH 04/28] [PM-40673 Part 3] clean up test comments (remove plan-style prefixes, reword stamp-rotation summary) --- .../OrganizationUsersControllerRecoverAccountTests.cs | 2 +- .../TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs index 223bd4c4a371..8fc96f3c0c78 100644 --- a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs +++ b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs @@ -533,7 +533,7 @@ private async Task LoginAsAccountRecoveryManagerWithMorePermis return targetCustomOrgUser; } - /// /// An administrator resetting only two-factor stops that member's remembered devices from /// being trusted. This flag combination had no coverage: every other recovery test here sets /// ResetMasterPassword, which is a different code path. diff --git a/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs b/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs index 17ac6c5cdd34..2b388d77a994 100644 --- a/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs +++ b/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs @@ -100,7 +100,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 remember token they hold. + /// [Theory, BitAutoData] public async Task ValidateAsync_UserSecurityStampRotated_ReturnsFalse( User user, Guid deviceId) From 86a21db6e30d9e4e1a5ef3cf4f1a785c11deebf2 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Sat, 17 Oct 2026 17:04:22 -0500 Subject: [PATCH 05/28] [PM-40673 Part 3] run dotnet format on test imports --- test/Core.Test/Services/UserServiceTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Core.Test/Services/UserServiceTests.cs b/test/Core.Test/Services/UserServiceTests.cs index 3a63d86d9880..d5e097af76a6 100644 --- a/test/Core.Test/Services/UserServiceTests.cs +++ b/test/Core.Test/Services/UserServiceTests.cs @@ -10,9 +10,9 @@ using Bit.Core.AdminConsole.OrganizationFeatures.Policies.PolicyRequirements; using Bit.Core.AdminConsole.Repositories; using Bit.Core.Auth.Enums; -using Bit.Core.Auth.UserFeatures.TwoFactorAuth; 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; From 217338d90d06ff44c5b1f7c4403e4cf170733b00 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Sun, 18 Oct 2026 22:22:11 -0500 Subject: [PATCH 06/28] [PM-40673 Part 3] skip remember token issuance for recovery-code logins --- .../RequestValidators/BaseRequestValidator.cs | 5 +++-- .../Endpoints/RememberedDeviceLoginTests.cs | 13 ++++++++++--- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/Identity/IdentityServer/RequestValidators/BaseRequestValidator.cs b/src/Identity/IdentityServer/RequestValidators/BaseRequestValidator.cs index 8ce108913b5a..c6da7d8d4cbf 100644 --- a/src/Identity/IdentityServer/RequestValidators/BaseRequestValidator.cs +++ b/src/Identity/IdentityServer/RequestValidators/BaseRequestValidator.cs @@ -372,8 +372,9 @@ await BuildErrorResultAsync("Two-step token is invalid. Try again.", true, conte 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) + // If the 2FA auth was rememberMe do not send another token; a remember login must not renew itself. + // A recovery-code login removes 2FA, so don't mint a new token. + && twoFactorProviderType is not (TwoFactorProviderType.Remember or TwoFactorProviderType.RecoveryCode)) { validatorContext.RememberMeRequested = true; } diff --git a/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs b/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs index 35c22dddc638..10ee67ced94e 100644 --- a/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs +++ b/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs @@ -215,9 +215,15 @@ public async Task LegacyRememberToken_IsUpgradedToCurrentFormat() /// 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 to be remembered. Either way it must not + /// come back with a new remember token, which would undo the revocation for that device. + /// /// - [Fact] - public async Task RecoveryCodeTeardown_RevokesRememberedDevices() + [Theory] + [InlineData("0")] + [InlineData("1")] + public async Task RecoveryCodeTeardown_RevokesRememberedDevices(string twoFactorRemember) { var (factory, user) = await CreateFactoryWithTwoFactorUserAsync(); var userRepository = factory.Services.GetRequiredService(); @@ -237,10 +243,11 @@ public async Task RecoveryCodeTeardown_RevokesRememberedDevices() _testPassword, twoFactorProviderType: _recoveryCodeProvider, twoFactorToken: recoveryCode, - twoFactorRemember: "0"); + 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 remembered-device state From 4a5312136df416e21eb81786396a4c669db333ea Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Sun, 18 Oct 2026 22:31:10 -0500 Subject: [PATCH 07/28] [PM-40673 Part 3] update comments on token revocation --- .../TwoFactorAuth/IRevokeTwoFactorRememberTokensCommand.cs | 2 +- src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Core/Auth/UserFeatures/TwoFactorAuth/IRevokeTwoFactorRememberTokensCommand.cs b/src/Core/Auth/UserFeatures/TwoFactorAuth/IRevokeTwoFactorRememberTokensCommand.cs index 14976b2b18a6..8ef31f6318ed 100644 --- a/src/Core/Auth/UserFeatures/TwoFactorAuth/IRevokeTwoFactorRememberTokensCommand.cs +++ b/src/Core/Auth/UserFeatures/TwoFactorAuth/IRevokeTwoFactorRememberTokensCommand.cs @@ -3,7 +3,7 @@ public interface IRevokeTwoFactorRememberTokensCommand { /// - /// Stops every remember token this user holds from being honored, on all of their devices. + /// Stops every 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, diff --git a/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md b/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md index 1bdfea6415c2..5a62a0b959c8 100644 --- a/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md +++ b/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md @@ -8,7 +8,7 @@ A user completing 2FA can ask for the device to be remembered. The server record ### Revocation -The token is only honored while the stamp it carries still matches the row's. `IRevokeTwoFactorRememberTokensCommand.RevokeAllForUserAsync` writes a new stamp to every row for a user, which stops all of their remember tokens being honored at once. The rows are kept, so `CreationDate` still records when each device was first remembered and `RevisionDate` records when it was cut off. +The token is only honored while the stamp it carries still matches the row's. `IRevokeTwoFactorRememberTokensCommand.RevokeAllForUserAsync` writes a new stamp to every row for a user, which stops every remember token recorded in those rows from being honored at once. The rows are kept, so `CreationDate` still records when each device was first remembered and `RevisionDate` records when it was cut off. Revocation is deliberately narrow. It affects remember-me only — access tokens, refresh tokens, and live sessions are untouched, so it does not sign the user out anywhere. Their devices simply have to complete 2FA again on the next login. From c77b85f12e28a25251ed560c9e9d119bed83c775 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Sun, 18 Oct 2026 22:40:49 -0500 Subject: [PATCH 08/28] [PM-40673 Part 3] use a valid encrypted key in the password-only recover-account test --- .../OrganizationUsersControllerRecoverAccountTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs index 8fc96f3c0c78..ca4c0973e460 100644 --- a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs +++ b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs @@ -606,7 +606,7 @@ public async Task RecoverAccount_ResetMasterPasswordOnly_LeavesRememberedDevices ResetMasterPassword = true, ResetTwoFactor = false, NewMasterPasswordHash = "new-master-password-hash", - Key = "encrypted-recovery-key", + Key = "2.AAECAwQFBgcICQoLDA0ODw==|QmFzZTY0UGFydA==|AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=", }); // Assert From 40204ac7cc9f50822de97bce1032e9763a69b750 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Mon, 19 Oct 2026 07:11:59 -0500 Subject: [PATCH 09/28] [PM-40673 Part 3] correct test comments on password-only recovery and recovery-code teardown --- .../OrganizationUsersControllerRecoverAccountTests.cs | 4 ++-- .../Endpoints/RememberedDeviceLoginTests.cs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs index ca4c0973e460..59ba55feeb2a 100644 --- a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs +++ b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs @@ -579,8 +579,8 @@ public async Task RecoverAccount_ResetTwoFactorOnly_RevokesRememberedDevices() /// /// The two flags are independent. A password-only recovery is not a two-factor teardown, so it - /// must leave remembered devices alone — over-firing would sign people out of remember-me for - /// no reason. + /// does not rotate the remember-token rows. (The password change itself still invalidates + /// existing remember tokens through the security stamp.) /// [Fact] public async Task RecoverAccount_ResetMasterPasswordOnly_LeavesRememberedDevicesAlone() diff --git a/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs b/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs index 10ee67ced94e..644276f79080 100644 --- a/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs +++ b/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs @@ -236,8 +236,8 @@ public async Task RecoveryCodeTeardown_RevokesRememberedDevices(string twoFactor _testEmail, _testPassword, twoFactorProviderType: _emailProvider, twoFactorToken: _emailToken); Assert.False(string.IsNullOrEmpty(rememberToken)); - // Tear down two-factor with the recovery code. Provider sent as a number, per the validator - // ordering note on the factory helper. + // 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, From 2776a6c28765ed640be5959953bd32f9c4342f0f Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Mon, 19 Oct 2026 07:55:40 -0500 Subject: [PATCH 10/28] [PM-40673 Part 3] to avoid confusion, use 'remembered' for remember-token wording, keep 'trusted' for TDE --- src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md | 2 +- .../OrganizationUsersControllerRecoverAccountTests.cs | 2 +- .../Controllers/TwoFactor/TwoFactorControllerEmailTests.cs | 4 ++-- test/Core.Test/Services/UserServiceTests.cs | 4 ++-- .../Endpoints/RememberedDeviceLoginTests.cs | 6 +++--- .../Repositories/TwoFactorRememberTokenRepositoryTests.cs | 2 +- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md b/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md index 5a62a0b959c8..50189cc7755d 100644 --- a/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md +++ b/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md @@ -20,7 +20,7 @@ Three flows revoke, all of them teardowns that leave the account with no second | A user disables 2FA with a recovery code | `UserService.RecoverTwoFactorAsync` | | A user removes their **last** 2FA provider | `UserService.DisableTwoFactorProviderAsync` | -Removing one of several providers does **not** revoke: the user still has working 2FA, so their remembered devices stay trusted. Untrusting a device does not revoke either — that is a separate trusted-device concept. +Removing one of several providers does **not** revoke: the user still has working 2FA, so their devices stay remembered. Untrusting a device (Trusted Device Encryption) does not revoke either — that is a separate mechanism. Two further properties hold without any explicit call. A password change or account key rotation rotates `User.SecurityStamp`, which remember tokens also carry and which is compared on every use. And while the account has no 2FA at all, whether its own or enforced by its organization, no second factor is requested at login, so a remember token is not consulted. diff --git a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs index 59ba55feeb2a..c06dd7a1245a 100644 --- a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs +++ b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs @@ -535,7 +535,7 @@ private async Task LoginAsAccountRecoveryManagerWithMorePermis /// /// An administrator resetting only two-factor stops that member's remembered devices from - /// being trusted. This flag combination had no coverage: every other recovery test here sets + /// being remembered. This flag combination had no coverage: every other recovery test here sets /// ResetMasterPassword, which is a different code path. /// /// diff --git a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs index 9181261c76e8..d9b05937b0a2 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs @@ -846,8 +846,8 @@ public async Task EmailLoginCode_StoredInPreviousFormat_Rejected() } /// - /// Removing the user's last two-factor provider stops their remembered devices from being - /// trusted. Email is the only provider in this fixture, so the real DELETE below is the + /// Removing the user's last two-factor provider stops their remembered devices from + /// being remembered. Email is the only provider in this fixture, so the real DELETE below is the /// last-provider path. /// /// diff --git a/test/Core.Test/Services/UserServiceTests.cs b/test/Core.Test/Services/UserServiceTests.cs index d5e097af76a6..fc82d061afe0 100644 --- a/test/Core.Test/Services/UserServiceTests.cs +++ b/test/Core.Test/Services/UserServiceTests.cs @@ -268,7 +268,7 @@ public async Task IsClaimedByAnyOrganizationAsync_WithOrganizationUseOrganizatio /// /// Removing the last provider leaves the account with no second factor, so the devices that were - /// trusted under the old one should stop being trusted. + /// remembered under the old one should stop being remembered. /// [Theory, BitAutoData] public async Task DisableTwoFactorProviderAsync_LastProviderRemoved_RevokesRememberedDevices( @@ -341,7 +341,7 @@ await sutProvider.GetDependency() /// /// Going from several providers to fewer leaves working two-factor in place, so remembered - /// devices stay trusted. Over-firing here would log people out of remember-me for no reason. + /// devices stay remembered. Over-firing here would log people out of remember-me for no reason. /// [Theory, BitAutoData] public async Task DisableTwoFactorProviderAsync_OtherProvidersRemain_DoesNotRevoke( diff --git a/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs b/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs index 644276f79080..2c80c25b28e2 100644 --- a/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs +++ b/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs @@ -208,8 +208,8 @@ public async Task LegacyRememberToken_IsUpgradedToCurrentFormat() } /// - /// Using a recovery code tears two-factor down, and the devices trusted under the old - /// configuration stop being trusted with it. + /// Using a recovery code tears two-factor down, and the user's remembered devices + /// stop being remembered with it. /// /// /// The replay happens after two-factor is re-enabled, deliberately. While the account @@ -299,7 +299,7 @@ public async Task RememberToken_PresentedFromDifferentDevice_IsRefused() 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. + // the password was fine, the device just is not remembered. 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/Infrastructure.IntegrationTest/Auth/Repositories/TwoFactorRememberTokenRepositoryTests.cs b/test/Infrastructure.IntegrationTest/Auth/Repositories/TwoFactorRememberTokenRepositoryTests.cs index 0ccf0ce482c8..6194547645c2 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 the device was first remembered. Assert.Equal(inserted.Id, updated.Id); Assert.Equal(inserted.CreationDate, updated.CreationDate); Assert.True(updated.RevisionDate >= inserted.RevisionDate); From 4ec06a369af29f525bdd7657fb991e95488b929f Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Mon, 19 Oct 2026 08:46:08 -0500 Subject: [PATCH 11/28] [PM-40673 Part 3] remove unnecessary comments, and revoke remember tokens before saving when the last 2FA provider is removed --- .../Services/Implementations/UserService.cs | 21 ++++---- ...ationUsersControllerRecoverAccountTests.cs | 10 ---- .../TwoFactorControllerEmailTests.cs | 10 ---- .../AdminRecoverAccountCommandTests.cs | 2 - test/Core.Test/Services/UserServiceTests.cs | 48 ++++--------------- 5 files changed, 22 insertions(+), 69 deletions(-) diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs index e2eadfd029e3..7201dbe69f18 100644 --- a/src/Core/Services/Implementations/UserService.cs +++ b/src/Core/Services/Implementations/UserService.cs @@ -749,20 +749,23 @@ public async Task DisableTwoFactorProviderAsync(User user, TwoFactorProviderType providers.Remove(type); user.SetTwoFactorProviders(providers); + + // 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 removedLastProvider = !await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(user); + if (removedLastProvider) + { + await _revokeTwoFactorRememberTokensCommand.RevokeAllForUserAsync(user.Id); + } + await SaveUserAsync(user); await _eventService.LogUserEventAsync(user.Id, EventType.User_Disabled2fa); - if (!await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(user)) + if (removedLastProvider) { - // Runs post-save because whether this was the last provider is only known once the - // remaining providers are known. Re-running the method cannot retry either step below: - // the provider is already gone, so the early return at the top takes over. - - // When the last 2FA provider is removed, CheckPoliciesOnTwoFactorRemovalAsync revokes the - // user from organizations that require 2FA. This check goes before remember-token revocation - // so that a failing remember-token revoke cannot skip it. + // Revokes the user from organizations that require 2FA. await CheckPoliciesOnTwoFactorRemovalAsync(user); - await _revokeTwoFactorRememberTokensCommand.RevokeAllForUserAsync(user.Id); } } diff --git a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs index c06dd7a1245a..5aa12bf32d82 100644 --- a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs +++ b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs @@ -533,16 +533,6 @@ private async Task LoginAsAccountRecoveryManagerWithMorePermis return targetCustomOrgUser; } - /// - /// An administrator resetting only two-factor stops that member's remembered devices from - /// being remembered. This flag combination had no coverage: every other recovery test here sets - /// ResetMasterPassword, which is a different code path. - /// - /// - /// Asserted on the stored stamp, which is the value a presented token is compared against, so a - /// changed stamp is what "the previously issued tokens no longer work" means. The end-to-end - /// replay is covered in the Identity integration tests. - /// [Fact] public async Task RecoverAccount_ResetTwoFactorOnly_RevokesRememberedDevices() { diff --git a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs index d9b05937b0a2..5fd4028a779d 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs @@ -845,16 +845,6 @@ public async Task EmailLoginCode_StoredInPreviousFormat_Rejected() AssertTwoFactorRejected(result); } - /// - /// Removing the user's last two-factor provider stops their remembered devices from - /// being remembered. Email is the only provider in this fixture, so the real DELETE below is the - /// last-provider path. - /// - /// - /// Asserted on the stored stamp rather than by replaying a token: the stamp is the value a - /// presented token is compared against, so a changed stamp is exactly what "the previously - /// issued tokens no longer work" means. Replaying end-to-end is covered in the Identity tests. - /// [Fact] public async Task DeleteEmail_RemovesLastProvider_RevokesRememberedDevices() { diff --git a/test/Core.Test/AdminConsole/OrganizationFeatures/AccountRecovery/AdminRecoverAccountCommandTests.cs b/test/Core.Test/AdminConsole/OrganizationFeatures/AccountRecovery/AdminRecoverAccountCommandTests.cs index 2aff7bf8b29d..3c3cc9700ef3 100644 --- a/test/Core.Test/AdminConsole/OrganizationFeatures/AccountRecovery/AdminRecoverAccountCommandTests.cs +++ b/test/Core.Test/AdminConsole/OrganizationFeatures/AccountRecovery/AdminRecoverAccountCommandTests.cs @@ -76,8 +76,6 @@ await sutProvider.GetDependency().DidNotReceive().LogOrganization Arg.Any(), Arg.Is(EventType.OrganizationUser_AdminResetTwoFactor)); - // The two flags are independent, and a password-only recovery must not tear down two-factor - // state — including the devices the user has remembered. await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() .ResetAsync(default!); diff --git a/test/Core.Test/Services/UserServiceTests.cs b/test/Core.Test/Services/UserServiceTests.cs index fc82d061afe0..19cc8d871eda 100644 --- a/test/Core.Test/Services/UserServiceTests.cs +++ b/test/Core.Test/Services/UserServiceTests.cs @@ -266,10 +266,6 @@ public async Task IsClaimedByAnyOrganizationAsync_WithOrganizationUseOrganizatio Assert.False(result); } - /// - /// Removing the last provider leaves the account with no second factor, so the devices that were - /// remembered under the old one should stop being remembered. - /// [Theory, BitAutoData] public async Task DisableTwoFactorProviderAsync_LastProviderRemoved_RevokesRememberedDevices( SutProvider sutProvider, User user) @@ -293,36 +289,19 @@ await sutProvider.GetDependency() } /// - /// Removing the last provider also revokes the user from organizations that require two-step login. - /// That enforcement runs before the remember-token revoke, so a failing remember-token revoke cannot - /// skip it. + /// Removing the last provider revokes the remember tokens before anything is saved. If token revocation + /// fails, the removal is not persisted, so the user can simply retry instead of being left with no + /// two-factor and remember tokens that would still validate upon re-enabling 2FA. /// [Theory, BitAutoData] - public async Task DisableTwoFactorProviderAsync_LastProviderRemoved_RememberTokenRevocationFails_StillEnforcesPolicies( - SutProvider sutProvider, User user, Organization organization, Guid organizationUserId) + public async Task DisableTwoFactorProviderAsync_LastProviderRemoved_RememberTokenRevocationFails_DoesNotSaveRemoval( + SutProvider sutProvider, User user) { // Arrange user.SetTwoFactorProviders(new Dictionary { [TwoFactorProviderType.Email] = new() { Enabled = true } }); - organization.Enabled = true; - organization.UseSso = true; - sutProvider.GetDependency() - .GetAsync(user.Id) - .Returns(new RequireTwoFactorPolicyRequirement( - [ - new PolicyDetails - { - OrganizationId = organization.Id, - OrganizationUserId = organizationUserId, - OrganizationUserStatus = OrganizationUserStatusType.Confirmed, - PolicyType = PolicyType.TwoFactorAuthentication - } - ])); - sutProvider.GetDependency() - .GetManyByIdsAsync(Arg.Is>(ids => ids.Contains(organization.Id))) - .Returns(new[] { organization }); sutProvider.GetDependency() .RevokeAllForUserAsync(user.Id) .Returns(Task.FromException(new InvalidOperationException("database unavailable"))); @@ -332,17 +311,14 @@ await Assert.ThrowsAsync( () => sutProvider.Sut.DisableTwoFactorProviderAsync(user, TwoFactorProviderType.Email)); // Assert + await sutProvider.GetDependency() + .DidNotReceiveWithAnyArgs() + .ReplaceAsync(default!); await sutProvider.GetDependency() - .Received(1) - .RevokeNonCompliantOrganizationUsersAsync( - Arg.Is(r => r.OrganizationId == organization.Id && - r.OrganizationUsers.First().Id == organizationUserId)); + .DidNotReceiveWithAnyArgs() + .RevokeNonCompliantOrganizationUsersAsync(default!); } - /// - /// Going from several providers to fewer leaves working two-factor in place, so remembered - /// devices stay remembered. Over-firing here would log people out of remember-me for no reason. - /// [Theory, BitAutoData] public async Task DisableTwoFactorProviderAsync_OtherProvidersRemain_DoesNotRevoke( SutProvider sutProvider, User user) @@ -589,10 +565,6 @@ await sutProvider.GetDependency() .RevokeAllForUserAsync(user.Id); } - /// - /// An unverified request must not be able to rotate anything, so revocation sits after the - /// recovery-code comparison. - /// [Theory, BitAutoData] public async Task RecoverTwoFactorAsync_IncorrectCode_DoesNotRevoke( User user, SutProvider sutProvider) From d11a83b8813ee4611596f423598928d93a6dbf7c Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Wed, 28 Oct 2026 09:00:53 -0500 Subject: [PATCH 12/28] [PM-40673 Part 3] use real emailed 2FA code in recovery-code remember test --- .../Endpoints/RememberedDeviceLoginTests.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs b/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs index 2c80c25b28e2..f8900d9ff34c 100644 --- a/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs +++ b/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs @@ -232,8 +232,10 @@ public async Task RecoveryCodeTeardown_RevokesRememberedDevices(string twoFactor 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: _emailToken); + _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 From 32671405cb18a13c1de0b4fc0d73f6be76e89653 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Tue, 3 Nov 2026 16:43:19 -0600 Subject: [PATCH 13/28] [PM-40673 Part 3] refactor remember-token provider exclusions to use a named bool variable; update comments --- .../RequestValidators/BaseRequestValidator.cs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/Identity/IdentityServer/RequestValidators/BaseRequestValidator.cs b/src/Identity/IdentityServer/RequestValidators/BaseRequestValidator.cs index c6da7d8d4cbf..fb3c5ce8ca7b 100644 --- a/src/Identity/IdentityServer/RequestValidators/BaseRequestValidator.cs +++ b/src/Identity/IdentityServer/RequestValidators/BaseRequestValidator.cs @@ -367,14 +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; a remember login must not renew itself. - // A recovery-code login removes 2FA, so don't mint a new token. - && twoFactorProviderType is not (TwoFactorProviderType.Remember or TwoFactorProviderType.RecoveryCode)) + + // 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; } From fb18563d02cece9c1da439b0e546338bed9ad4e7 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Tue, 3 Nov 2026 21:07:32 -0600 Subject: [PATCH 14/28] [PM-40673 Part 3] rename "remembered device" wording to "2FA remember token" across code, tests, and docs --- ...DeleteExpiredTwoFactorRememberTokensJob.cs | 2 +- .../Auth/Entities/TwoFactorRememberToken.cs | 8 ++-- .../Tokenables/TwoFactorRememberTokenable.cs | 2 +- .../ITwoFactorRememberTokenRepository.cs | 2 +- .../IIssueTwoFactorRememberTokenCommand.cs | 4 +- .../IRevokeTwoFactorRememberTokensCommand.cs | 7 +-- .../IValidateTwoFactorRememberTokenQuery.cs | 4 +- .../Auth/UserFeatures/TwoFactorAuth/readme.md | 46 +++++++++++-------- .../Services/Implementations/UserService.cs | 8 ++-- .../CustomValidatorRequestContext.cs | 4 +- .../RequestValidators/BaseRequestValidator.cs | 4 +- .../ITwoFactorAuthenticationValidator.cs | 4 +- .../TwoFactorAuthenticationValidator.cs | 2 +- .../TwoFactorRememberTokenRepository.cs | 8 ++-- .../TwoFactorRememberTokenRepository.cs | 6 +-- ...ationUsersControllerRecoverAccountTests.cs | 14 +++--- .../TwoFactorControllerEmailTests.cs | 2 +- ...IssueTwoFactorRememberTokenCommandTests.cs | 2 +- .../ResetUserTwoFactorCommandTests.cs | 2 +- ...vokeTwoFactorRememberTokensCommandTests.cs | 4 +- ...alidateTwoFactorRememberTokenQueryTests.cs | 2 +- test/Core.Test/Services/UserServiceTests.cs | 13 +++--- .../Endpoints/IdentityServerTwoFactorTests.cs | 2 +- ...cs => TwoFactorRememberTokenLoginTests.cs} | 28 +++++------ .../TwoFactorRememberTokenRepositoryTests.cs | 6 +-- .../Factories/IdentityApplicationFactory.cs | 6 +-- 26 files changed, 98 insertions(+), 94 deletions(-) rename test/Identity.IntegrationTest/Endpoints/{RememberedDeviceLoginTests.cs => TwoFactorRememberTokenLoginTests.cs} (92%) 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..cfb767342450 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); 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 8ef31f6318ed..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 recorded for this user 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/readme.md b/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md index 50189cc7755d..df95a65cfd5e 100644 --- a/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md +++ b/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md @@ -2,27 +2,33 @@ 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. -## Remembered devices +## 2FA remember tokens -A user completing 2FA can ask for the device to be remembered. The server records the device in `TwoFactorRememberToken` — one row per `(UserId, DeviceId)` — and issues a `TwoFactorRememberTokenable` carrying a copy of that row's `Stamp`. On a later login the client presents the token as `TwoFactorProviderType.Remember` and skips the challenge. +### 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 -The token is only honored while the stamp it carries still matches the row's. `IRevokeTwoFactorRememberTokensCommand.RevokeAllForUserAsync` writes a new stamp to every row for a user, which stops every remember token recorded in those rows from being honored at once. The rows are kept, so `CreationDate` still records when each device was first remembered and `RevisionDate` records when it was cut off. +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. + +Three 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` | -Revocation is deliberately narrow. It affects remember-me only — access tokens, refresh tokens, and live sessions are untouched, so it does not sign the user out anywhere. Their devices simply have to complete 2FA again on the next login. +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. -Three flows revoke, all of them teardowns that leave the account with no second factor: +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. -| Flow | Where | -| --- | --- | -| An administrator resets a member's 2FA | `ResetUserTwoFactorCommand.ResetAsync` | -| A user disables 2FA with a recovery code | `UserService.RecoverTwoFactorAsync` | -| A user removes their **last** 2FA provider | `UserService.DisableTwoFactorProviderAsync` | +Untrusting a device (Trusted Device Encryption) does not revoke 2FA remember tokens — that is a separate mechanism. -Removing one of several providers does **not** revoke: the user still has working 2FA, so their devices stay remembered. Untrusting a device (Trusted Device Encryption) does not revoke either — that is a separate mechanism. +### Cleanup -Two further properties hold without any explicit call. A password change or account key rotation rotates `User.SecurityStamp`, which remember tokens also carry and which is compared on every use. And while the account has no 2FA at all, whether its own or enforced by its organization, no second factor is requested at login, so a remember token is not consulted. +Revoked `TwoFactorRememberToken` rows stay until their `ExpirationDate` passes. `DeleteExpiredTwoFactorRememberTokensJob` then deletes them. ## User Verification @@ -81,14 +87,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 7201dbe69f18..664817d59edb 100644 --- a/src/Core/Services/Implementations/UserService.cs +++ b/src/Core/Services/Implementations/UserService.cs @@ -750,11 +750,13 @@ 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 removedLastProvider = !await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(user); - if (removedLastProvider) + var twoFactorAuthNoLongerEnabled = !await _twoFactorIsEnabledQuery.TwoFactorIsEnabledAsync(user); + + if (twoFactorAuthNoLongerEnabled) { await _revokeTwoFactorRememberTokensCommand.RevokeAllForUserAsync(user.Id); } @@ -762,7 +764,7 @@ public async Task DisableTwoFactorProviderAsync(User user, TwoFactorProviderType await SaveUserAsync(user); await _eventService.LogUserEventAsync(user.Id, EventType.User_Disabled2fa); - if (removedLastProvider) + if (twoFactorAuthNoLongerEnabled) { // Revokes the user from organizations that require 2FA. await CheckPoliciesOnTwoFactorRemovalAsync(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 fb3c5ce8ca7b..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); } 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 5aa12bf32d82..c128f9eee2bf 100644 --- a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs +++ b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs @@ -534,7 +534,7 @@ private async Task LoginAsAccountRecoveryManagerWithMorePermis } [Fact] - public async Task RecoverAccount_ResetTwoFactorOnly_RevokesRememberedDevices() + public async Task RecoverAccount_ResetTwoFactorOnly_RevokesTwoFactorRememberTokens() { // Arrange var (ownerEmail, _) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory, @@ -547,7 +547,7 @@ public async Task RecoverAccount_ResetTwoFactorOnly_RevokesRememberedDevices() var userRepository = _factory.GetService(); var targetUser = (await userRepository.GetByEmailAsync(targetEmail))!; - var (device, _) = await SeedRememberedDeviceAsync(targetUser, "stamp-before-reset"); + var (device, _) = await SeedTwoFactorRememberTokenAsync(targetUser, "stamp-before-reset"); // Act var response = await _client.PutAsJsonAsync( @@ -569,11 +569,11 @@ public async Task RecoverAccount_ResetTwoFactorOnly_RevokesRememberedDevices() /// /// The two flags are independent. A password-only recovery is not a two-factor teardown, so it - /// does not rotate the remember-token rows. (The password change itself still invalidates - /// existing remember tokens through the security stamp.) + /// 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_LeavesRememberedDevicesAlone() + public async Task RecoverAccount_ResetMasterPasswordOnly_LeavesTwoFactorRememberTokensAlone() { // Arrange var (ownerEmail, _) = await OrganizationTestHelpers.CreateNewUserWithAccountAsync(_factory, @@ -586,7 +586,7 @@ public async Task RecoverAccount_ResetMasterPasswordOnly_LeavesRememberedDevices var userRepository = _factory.GetService(); var targetUser = (await userRepository.GetByEmailAsync(targetEmail))!; - var (device, _) = await SeedRememberedDeviceAsync(targetUser, "stamp-untouched"); + var (device, _) = await SeedTwoFactorRememberTokenAsync(targetUser, "stamp-untouched"); // Act var response = await _client.PutAsJsonAsync( @@ -608,7 +608,7 @@ public async Task RecoverAccount_ResetMasterPasswordOnly_LeavesRememberedDevices Assert.Equal("stamp-untouched", row.Stamp); } - private async Task<(Device Device, TwoFactorRememberToken Row)> SeedRememberedDeviceAsync( + private async Task<(Device Device, TwoFactorRememberToken Row)> SeedTwoFactorRememberTokenAsync( User user, string stamp) { var device = await _factory.GetService().CreateAsync(new Device diff --git a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs index 5fd4028a779d..495595165335 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs @@ -846,7 +846,7 @@ public async Task EmailLoginCode_StoredInPreviousFormat_Rejected() } [Fact] - public async Task DeleteEmail_RemovesLastProvider_RevokesRememberedDevices() + public async Task DeleteEmail_TwoFactorAuthNoLongerEnabled_RevokesTwoFactorRememberTokens() { await EnrollUserInEmail(); var user = (await _userRepository.GetByEmailAsync(_userEmail))!; 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 9fc5f63b3216..0b364d4f7626 100644 --- a/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ResetUserTwoFactorCommandTests.cs +++ b/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ResetUserTwoFactorCommandTests.cs @@ -80,7 +80,7 @@ await sutProvider.GetDependency() } [Theory, BitAutoData] - public async Task ResetAsync_RevokesRememberedDevices(User user) + public async Task ResetAsync_RevokesTwoFactorRememberTokens(User user) { // Arrange var sutProvider = GetSutProvider(); 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 2b388d77a994..17b5627fbdff 100644 --- a/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs +++ b/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs @@ -101,7 +101,7 @@ public async Task ValidateAsync_RowStampRotated_ReturnsFalse( } /// - /// The account-wide check: rotating the user's security stamp invalidates every remember token they hold. + /// 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( diff --git a/test/Core.Test/Services/UserServiceTests.cs b/test/Core.Test/Services/UserServiceTests.cs index 19cc8d871eda..8b7277d713dd 100644 --- a/test/Core.Test/Services/UserServiceTests.cs +++ b/test/Core.Test/Services/UserServiceTests.cs @@ -267,7 +267,7 @@ public async Task IsClaimedByAnyOrganizationAsync_WithOrganizationUseOrganizatio } [Theory, BitAutoData] - public async Task DisableTwoFactorProviderAsync_LastProviderRemoved_RevokesRememberedDevices( + public async Task DisableTwoFactorProviderAsync_TwoFactorAuthNoLongerEnabled_RevokesTwoFactorRememberTokens( SutProvider sutProvider, User user) { // Arrange @@ -289,12 +289,13 @@ await sutProvider.GetDependency() } /// - /// Removing the last provider revokes the remember tokens before anything is saved. If token revocation - /// fails, the removal is not persisted, so the user can simply retry instead of being left with no - /// two-factor and remember tokens that would still validate upon re-enabling 2FA. + /// 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_LastProviderRemoved_RememberTokenRevocationFails_DoesNotSaveRemoval( + public async Task DisableTwoFactorProviderAsync_TwoFactorAuthNoLongerEnabled_RememberTokenRevocationFails_DoesNotSaveRemoval( SutProvider sutProvider, User user) { // Arrange @@ -546,7 +547,7 @@ await sutProvider.GetDependency() } [Theory, BitAutoData] - public async Task RecoverTwoFactorAsync_CorrectCode_RevokesRememberedDevices( + public async Task RecoverTwoFactorAsync_CorrectCode_RevokesTwoFactorRememberTokens( User user, SutProvider sutProvider) { // Arrange 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 92% rename from test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs rename to test/Identity.IntegrationTest/Endpoints/TwoFactorRememberTokenLoginTests.cs index f8900d9ff34c..abda3f2b136a 100644 --- a/test/Identity.IntegrationTest/Endpoints/RememberedDeviceLoginTests.cs +++ b/test/Identity.IntegrationTest/Endpoints/TwoFactorRememberTokenLoginTests.cs @@ -18,12 +18,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 . @@ -76,7 +76,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() @@ -122,8 +122,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] @@ -208,22 +208,22 @@ public async Task LegacyRememberToken_IsUpgradedToCurrentFormat() } /// - /// Using a recovery code tears two-factor down, and the user's remembered devices - /// stop being remembered with it. + /// 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 to be remembered. Either way it must not - /// come back with a new remember token, which would undo the revocation for that device. + /// 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_RevokesRememberedDevices(string twoFactorRemember) + public async Task RecoveryCodeTeardown_RevokesTwoFactorRememberTokens(string twoFactorRemember) { var (factory, user) = await CreateFactoryWithTwoFactorUserAsync(); var userRepository = factory.Services.GetRequiredService(); @@ -252,7 +252,7 @@ public async Task RecoveryCodeTeardown_RevokesRememberedDevices(string twoFactor Assert.False(teardownBody.RootElement.TryGetProperty("TwoFactorToken", out _)); } - // Put a second factor back, so the replay below is decided by the remembered-device state + // 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); @@ -300,8 +300,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 remembered. + // 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/Infrastructure.IntegrationTest/Auth/Repositories/TwoFactorRememberTokenRepositoryTests.cs b/test/Infrastructure.IntegrationTest/Auth/Repositories/TwoFactorRememberTokenRepositoryTests.cs index 6194547645c2..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 still records when the device was first remembered. + // 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 — From 87057b89a80179fcf3870bde9cc6acdb2870448a Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Tue, 3 Nov 2026 23:12:32 -0600 Subject: [PATCH 15/28] [PM-40673 Part 3] add unit test for 2FA remember token exclusion on RecoveryCode and Remember logins --- .../BaseRequestValidatorTests.cs | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) 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( From e363a0c1f2c6fb013688f2c7a2638378b1f59590 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Tue, 3 Nov 2026 23:40:29 -0600 Subject: [PATCH 16/28] [PM-40673 Part 3] add integration tests for remember-token revoke when other or premium-only providers remain --- .../TwoFactorControllerEmailTests.cs | 116 +++++++++++++----- 1 file changed, 88 insertions(+), 28 deletions(-) diff --git a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs index 495595165335..ab3f70be6b79 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs @@ -849,39 +849,48 @@ public async Task EmailLoginCode_StoredInPreviousFormat_Rejected() public async Task DeleteEmail_TwoFactorAuthNoLongerEnabled_RevokesTwoFactorRememberTokens() { await EnrollUserInEmail(); - var user = (await _userRepository.GetByEmailAsync(_userEmail))!; + var (device, _) = await SeedTwoFactorRememberTokenAsync("stamp-before-teardown"); - var deviceRepository = _factory.GetService(); - var rememberTokenRepository = _factory.GetService(); + await DeleteEmailProviderAsync(); - var device = await deviceRepository.CreateAsync(new Device - { - UserId = user.Id, - Name = "chrome-test", - Type = DeviceType.ChromeBrowser, - Identifier = Guid.NewGuid().ToString(), - Active = true, - LastActivityDate = DateTime.UtcNow, - }); - await rememberTokenRepository.UpsertAsync(new TwoFactorRememberToken - { - UserId = user.Id, - DeviceId = device.Id, - Stamp = "stamp-before-teardown", - ExpirationDate = DateTime.UtcNow.AddDays(30), - }); + await AssertTwoFactorRememberTokenStampAsync(device, expectRotated: true, "stamp-before-teardown"); + } - var getResponse = await _client.PostAsJsonAsync("/two-factor/get-email", - new { MasterPasswordHash = MasterPasswordHash }); - var (_, uvToken) = await ReadEnabledAndUserVerificationTokenAsync(getResponse, "email"); + /// + /// 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"); - var disableResponse = await SendJsonAsync(_client, HttpMethod.Delete, "/two-factor/email", - new TwoFactorEmailDeleteRequestModel { UserVerificationToken = uvToken }); - Assert.Equal(HttpStatusCode.NoContent, disableResponse.StatusCode); + await DeleteEmailProviderAsync(); - var row = await rememberTokenRepository.GetByUserIdDeviceIdAsync(user.Id, device.Id); - Assert.NotNull(row); - Assert.NotEqual("stamp-before-teardown", row.Stamp); + 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"); } // --------------------------------------------------------------------- @@ -894,6 +903,57 @@ 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); + } + + 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); From c3aaad98902a114f976a708c6cf25bd85d8a1657 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Tue, 3 Nov 2026 23:53:04 -0600 Subject: [PATCH 17/28] [PM-40673 Part 3] add integration test for 2FA remember-token revoke combined with 2FA-policy org revocation --- .../TwoFactorControllerEmailTests.cs | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs index ab3f70be6b79..99e60b0fb930 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs @@ -6,6 +6,9 @@ using Bit.Api.IntegrationTest.Factories; using Bit.Api.IntegrationTest.Helpers; using Bit.Core.AdminConsole.Entities; +using Bit.Core.Billing.Enums; +using Bit.Core.AdminConsole.Repositories; +using Bit.Core.AdminConsole.Enums; using Bit.Core.Auth.Entities; using Bit.Core.Auth.Enums; using Bit.Core.Auth.Identity.TokenProviders; @@ -893,6 +896,26 @@ await SetUserTwoFactorProvidersJsonAsync(_userRepository, _userEmail, 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 // --------------------------------------------------------------------- @@ -930,6 +953,36 @@ private static string CombineProvidersJson(params string[] providersJson) => 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(); From 162bc16b615e796a55cb0ec4d1b42780c4cd3f5b Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Wed, 4 Nov 2026 00:00:11 -0600 Subject: [PATCH 18/28] [PM-40673 Part 3] add integration test that a wrong recovery code does not revoke 2FA remember tokens --- .../TwoFactorRememberTokenLoginTests.cs | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/test/Identity.IntegrationTest/Endpoints/TwoFactorRememberTokenLoginTests.cs b/test/Identity.IntegrationTest/Endpoints/TwoFactorRememberTokenLoginTests.cs index abda3f2b136a..1ae5517c77e3 100644 --- a/test/Identity.IntegrationTest/Endpoints/TwoFactorRememberTokenLoginTests.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; @@ -274,6 +275,61 @@ public async Task RecoveryCodeTeardown_RevokesTwoFactorRememberTokens(string two 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. From f719c3906926efdd71fe135c64ada2a6b7a8f55f Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Wed, 4 Nov 2026 00:06:43 -0600 Subject: [PATCH 19/28] [PM-40673 Part 3] add integration test for admin recovery resetting both password and 2FA --- ...ationUsersControllerRecoverAccountTests.cs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs index c128f9eee2bf..21700cf9e1b5 100644 --- a/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs +++ b/test/Api.IntegrationTest/AdminConsole/Controllers/OrganizationUsersControllerRecoverAccountTests.cs @@ -608,6 +608,42 @@ public async Task RecoverAccount_ResetMasterPasswordOnly_LeavesTwoFactorRemember 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) { From 2e7a9eb63a6920aac9dc24f8538915bce4253557 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Wed, 4 Nov 2026 00:19:53 -0600 Subject: [PATCH 20/28] [PM-40673 Part 3] add integration tests for 2FA remember-token revoke via Authenticator, YubiKey, Duo, and WebAuthn endpoints --- .../TwoFactorControllerAuthenticatorTests.cs | 28 +++++++++++ .../TwoFactor/TwoFactorControllerDuoTests.cs | 22 ++++++++ .../TwoFactorControllerWebAuthnTests.cs | 22 ++++++++ .../TwoFactorControllerYubiKeyTests.cs | 22 ++++++++ .../TwoFactorIntegrationTestHelpers.cs | 50 +++++++++++++++++++ 5 files changed, 144 insertions(+) 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..61d126676495 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerDuoTests.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerDuoTests.cs @@ -74,6 +74,28 @@ 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. The user has no premium, so the premium-only provider does not count either way. + /// + [Fact] + public async Task DeleteDuo_TwoFactorAuthNoLongerEnabled_RevokesTwoFactorRememberTokens() + { + await EnrollUserInDuo(); + 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/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..6d4cc3af9d53 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerYubiKeyTests.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerYubiKeyTests.cs @@ -74,6 +74,28 @@ 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. The user has no premium, so the premium-only provider does not count either way. + /// + [Fact] + public async Task DeleteYubiKey_TwoFactorAuthNoLongerEnabled_RevokesTwoFactorRememberTokens() + { + await EnrollUserInYubiKey(); + 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..d0b79b1a1cd5 100644 --- a/test/Api.IntegrationTest/Auth/Helpers/TwoFactorIntegrationTestHelpers.cs +++ b/test/Api.IntegrationTest/Auth/Helpers/TwoFactorIntegrationTestHelpers.cs @@ -1,5 +1,10 @@ using System.Text; using System.Text.Json; +using Xunit; +using Bit.Core.Enums; +using Bit.Core.Auth.Repositories; +using Bit.Core.Auth.Entities; +using Bit.Api.IntegrationTest.Factories; using Bit.Core.Auth.Enums; using Bit.Core.Auth.Models.Business.Tokenables; using Bit.Core.Entities; @@ -113,6 +118,51 @@ 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); + } + // --------------------------------------------------------------------- // Emailed two-factor codes // --------------------------------------------------------------------- From 7f46e34dcba7e2a5ddf110a0a9764ef6b68e75df Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Wed, 4 Nov 2026 00:37:17 -0600 Subject: [PATCH 21/28] [PM-40673 Part 3] fix using ordering in two-factor integration test files --- .../TwoFactor/TwoFactorControllerEmailTests.cs | 4 ++-- .../Auth/Helpers/TwoFactorIntegrationTestHelpers.cs | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs index 99e60b0fb930..703f19443720 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerEmailTests.cs @@ -6,9 +6,8 @@ using Bit.Api.IntegrationTest.Factories; using Bit.Api.IntegrationTest.Helpers; using Bit.Core.AdminConsole.Entities; -using Bit.Core.Billing.Enums; -using Bit.Core.AdminConsole.Repositories; 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; @@ -16,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; diff --git a/test/Api.IntegrationTest/Auth/Helpers/TwoFactorIntegrationTestHelpers.cs b/test/Api.IntegrationTest/Auth/Helpers/TwoFactorIntegrationTestHelpers.cs index d0b79b1a1cd5..10387e42682f 100644 --- a/test/Api.IntegrationTest/Auth/Helpers/TwoFactorIntegrationTestHelpers.cs +++ b/test/Api.IntegrationTest/Auth/Helpers/TwoFactorIntegrationTestHelpers.cs @@ -1,19 +1,19 @@ using System.Text; using System.Text.Json; -using Xunit; -using Bit.Core.Enums; -using Bit.Core.Auth.Repositories; -using Bit.Core.Auth.Entities; 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; From 3d809bcf537464d358f8399e94ae488b90b1ce87 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Wed, 4 Nov 2026 14:49:19 -0600 Subject: [PATCH 22/28] [PM-40673 Part 3] update clock comments in ValidateTwoFactorRememberTokenQueryTests --- .../ValidateTwoFactorRememberTokenQueryTests.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs b/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs index 17b5627fbdff..3683379b9ea9 100644 --- a/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs +++ b/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs @@ -41,7 +41,8 @@ private static TwoFactorRememberTokenable Tokenable(User user, Guid deviceId, st DeviceIdentifier = _deviceIdentifier, Stamp = stamp, SecurityStamp = user.SecurityStamp, - // Real clock: ExpiringTokenable reads DateTime.UtcNow directly. + // Real clock, relative to now: a tokenable built here uses the system clock (its TimeProvider is + // internal to Core), so a fixed date would eventually read as expired and make the tests flaky. ExpirationDate = DateTime.UtcNow.AddDays(30), }; @@ -219,8 +220,8 @@ public async Task ValidateAsync_TokenItselfExpired_ReturnsFalse( { 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. + // Real clock on purpose: a tokenable built here uses the system clock, which the injected + // FakeTimeProvider cannot reach. Keep the offset relative to now. tokenable.ExpirationDate = DateTime.UtcNow.AddMinutes(-1); Assert.False(await sutProvider.Sut.ValidateAsync(user, _deviceIdentifier, _token)); From 558a3320704bdd8e0ae864efdcefbe3f761912c4 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Thu, 5 Nov 2026 08:16:19 -0600 Subject: [PATCH 23/28] [PM-40673 Part 3] re-word doc comment on RotateStampsByUserIdAsync --- src/Core/Auth/Repositories/ITwoFactorRememberTokenRepository.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Core/Auth/Repositories/ITwoFactorRememberTokenRepository.cs b/src/Core/Auth/Repositories/ITwoFactorRememberTokenRepository.cs index cfb767342450..cb854cecc486 100644 --- a/src/Core/Auth/Repositories/ITwoFactorRememberTokenRepository.cs +++ b/src/Core/Auth/Repositories/ITwoFactorRememberTokenRepository.cs @@ -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. /// /// From 6ce9ded4840194b4a757abb39fc3a79acf2ab573 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Thu, 5 Nov 2026 08:24:36 -0600 Subject: [PATCH 24/28] [PM-40673 Part 3] grant premium in Duo and YubiKey revoke tests so 2FA is enabled before the delete --- .../Auth/Controllers/TwoFactor/TwoFactorControllerDuoTests.cs | 4 +++- .../Controllers/TwoFactor/TwoFactorControllerYubiKeyTests.cs | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerDuoTests.cs b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerDuoTests.cs index 61d126676495..59078245b66b 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerDuoTests.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerDuoTests.cs @@ -76,12 +76,14 @@ public async Task GetDuo_ValidSecret_ReturnsTokenUsableForDelete() /// /// Removing the only provider leaves 2FA no longer enabled, so the user's 2FA remember tokens are - /// revoked. The user has no premium, so the premium-only provider does not count either way. + /// 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", diff --git a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerYubiKeyTests.cs b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerYubiKeyTests.cs index 6d4cc3af9d53..1aea89c921f9 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerYubiKeyTests.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/TwoFactor/TwoFactorControllerYubiKeyTests.cs @@ -76,12 +76,14 @@ public async Task GetYubiKey_ValidSecret_ReturnsTokenUsableForDelete() /// /// Removing the only provider leaves 2FA no longer enabled, so the user's 2FA remember tokens are - /// revoked. The user has no premium, so the premium-only provider does not count either way. + /// 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", From 61890b5e5adae2663e97c3be53f7dbb6226908dc Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Thu, 5 Nov 2026 09:32:15 -0600 Subject: [PATCH 25/28] [PM-40673 Part 3] revoke 2FA remember tokens when emergency access takeover removes the grantor's 2FA --- .../EmergencyAccess/EmergencyAccessService.cs | 17 ++- .../Auth/UserFeatures/TwoFactorAuth/readme.md | 15 +-- .../EmergencyAccessControllerTest.cs | 46 ++++++++ .../EmergencyAccessServiceTests.cs | 110 ++++++++++++++++++ 4 files changed, 180 insertions(+), 8 deletions(-) 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/readme.md b/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md index df95a65cfd5e..4c5da3848187 100644 --- a/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md +++ b/src/Core/Auth/UserFeatures/TwoFactorAuth/readme.md @@ -12,13 +12,14 @@ When a user completes 2FA, they can optionally select the "Don't ask again on th 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. -Three 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` | +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. diff --git a/test/Api.IntegrationTest/Auth/Controllers/EmergencyAccessControllerTest.cs b/test/Api.IntegrationTest/Auth/Controllers/EmergencyAccessControllerTest.cs index 616a714c6386..a7905562fe90 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,51 @@ 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"); + } + /// /// Verifies the boundary validator's agreement checks fire on /// POST /emergency-access/{id}/password: a mismatched KDF or salt diff --git a/test/Core.Test/Auth/UserFeatures/EmergencyAccess/EmergencyAccessServiceTests.cs b/test/Core.Test/Auth/UserFeatures/EmergencyAccess/EmergencyAccessServiceTests.cs index bd66d2a4821c..4a5c0c618c03 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, @@ -2004,4 +2041,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)); + } } From b13474ffbb320af991f429f4dbb09a369cda2a33 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Thu, 5 Nov 2026 16:37:41 -0600 Subject: [PATCH 26/28] [PM-40673 Part 3] assert that a rejected emergency access takeover does not revoke 2FA remember tokens --- .../Controllers/EmergencyAccessControllerTest.cs | 10 ++++++++++ .../Helpers/TwoFactorIntegrationTestHelpers.cs | 14 ++++++++++++++ .../EmergencyAccess/EmergencyAccessServiceTests.cs | 3 +++ 3 files changed, 27 insertions(+) diff --git a/test/Api.IntegrationTest/Auth/Controllers/EmergencyAccessControllerTest.cs b/test/Api.IntegrationTest/Auth/Controllers/EmergencyAccessControllerTest.cs index a7905562fe90..7515e506a020 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/EmergencyAccessControllerTest.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/EmergencyAccessControllerTest.cs @@ -274,6 +274,7 @@ public async Task PostPassword_V2_RevokesGrantorTwoFactorRememberTokens() 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); @@ -291,6 +292,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"); } /// @@ -315,6 +320,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); @@ -332,6 +338,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/Helpers/TwoFactorIntegrationTestHelpers.cs b/test/Api.IntegrationTest/Auth/Helpers/TwoFactorIntegrationTestHelpers.cs index 10387e42682f..3e9d1059f808 100644 --- a/test/Api.IntegrationTest/Auth/Helpers/TwoFactorIntegrationTestHelpers.cs +++ b/test/Api.IntegrationTest/Auth/Helpers/TwoFactorIntegrationTestHelpers.cs @@ -163,6 +163,20 @@ public static async Task AssertTwoFactorRememberTokenRotatedAsync( 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/Auth/UserFeatures/EmergencyAccess/EmergencyAccessServiceTests.cs b/test/Core.Test/Auth/UserFeatures/EmergencyAccess/EmergencyAccessServiceTests.cs index 4a5c0c618c03..7c0ec6212420 100644 --- a/test/Core.Test/Auth/UserFeatures/EmergencyAccess/EmergencyAccessServiceTests.cs +++ b/test/Core.Test/Auth/UserFeatures/EmergencyAccess/EmergencyAccessServiceTests.cs @@ -1896,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] From 5b3c6e1b03a87fa6c0ce85617c2d66973e941ac0 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Thu, 5 Nov 2026 16:46:57 -0600 Subject: [PATCH 27/28] [PM-40673 Part 3] add integration test for 2FA remember token revoke on the legacy emergency access takeover path --- .../EmergencyAccessControllerTest.cs | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/test/Api.IntegrationTest/Auth/Controllers/EmergencyAccessControllerTest.cs b/test/Api.IntegrationTest/Auth/Controllers/EmergencyAccessControllerTest.cs index 7515e506a020..97f47671b04f 100644 --- a/test/Api.IntegrationTest/Auth/Controllers/EmergencyAccessControllerTest.cs +++ b/test/Api.IntegrationTest/Auth/Controllers/EmergencyAccessControllerTest.cs @@ -259,6 +259,49 @@ public async Task PostPassword_V2_RevokesGrantorTwoFactorRememberTokens() 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 From 37b44437d27bec5114a8dfe6f6b82529de3bd8a6 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Fri, 6 Nov 2026 17:19:23 -0600 Subject: [PATCH 28/28] [PM-40673 Part 3] use the fake clock for token expiry in ValidateTwoFactorRememberTokenQueryTests --- ...alidateTwoFactorRememberTokenQueryTests.cs | 85 ++++++++++++++++--- 1 file changed, 73 insertions(+), 12 deletions(-) diff --git a/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs b/test/Core.Test/Auth/UserFeatures/TwoFactorAuth/ValidateTwoFactorRememberTokenQueryTests.cs index 3683379b9ea9..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,9 +56,12 @@ private static TwoFactorRememberTokenable Tokenable(User user, Guid deviceId, st DeviceIdentifier = _deviceIdentifier, Stamp = stamp, SecurityStamp = user.SecurityStamp, - // Real clock, relative to now: a tokenable built here uses the system clock (its TimeProvider is - // internal to Core), so a fixed date would eventually read as expired and make the tests flaky. - 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) => @@ -53,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, }; /// @@ -64,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>() @@ -179,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); @@ -214,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: a tokenable built here uses the system clock, which the injected - // FakeTimeProvider cannot reach. Keep the offset relative to now. - 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)); }