From 8ea6b9f025f2fc04126a6a0aaec0d673ff69c57e Mon Sep 17 00:00:00 2001 From: maliming Date: Wed, 22 Apr 2026 11:37:03 +0800 Subject: [PATCH] Make 2FA flows tenant-aware and enforce pre-checks --- .../Identity/AspNetCore/AbpSignInManager.cs | 58 ++++++++++++- .../Volo/Abp/Identity/IdentityUserManager.cs | 17 ---- .../GetTwoFactorAuthenticationUser_Tests.cs | 83 +++++++++++++++++++ .../AspNetCore/SignInTestController.cs | 14 ++++ .../Abp/Identity/IdentityUserManager_Tests.cs | 49 ----------- 5 files changed, 154 insertions(+), 67 deletions(-) diff --git a/modules/identity/src/Volo.Abp.Identity.AspNetCore/Volo/Abp/Identity/AspNetCore/AbpSignInManager.cs b/modules/identity/src/Volo.Abp.Identity.AspNetCore/Volo/Abp/Identity/AspNetCore/AbpSignInManager.cs index 4b3147c14e..c8d3b5c275 100644 --- a/modules/identity/src/Volo.Abp.Identity.AspNetCore/Volo/Abp/Identity/AspNetCore/AbpSignInManager.cs +++ b/modules/identity/src/Volo.Abp.Identity.AspNetCore/Volo/Abp/Identity/AspNetCore/AbpSignInManager.cs @@ -1,4 +1,5 @@ -using System.Threading.Tasks; +using System.Security.Claims; +using System.Threading.Tasks; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; @@ -133,6 +134,61 @@ public class AbpSignInManager : SignInManager return await IdentityUserManager.FindSharedUserByLoginAsync(loginProvider, providerKey); } + public override async Task GetTwoFactorAuthenticationUserAsync() + { + var result = await Context.AuthenticateAsync(IdentityConstants.TwoFactorUserIdScheme); + if (result?.Principal == null) + { + return null; + } + + var userId = result.Principal.FindFirstValue(ClaimTypes.Name); + if (string.IsNullOrWhiteSpace(userId)) + { + return null; + } + + return await IdentityUserManager.FindSharedUserByIdAsync(userId); + } + + public override async Task TwoFactorSignInAsync(string provider, string code, bool isPersistent, bool rememberClient) + { + var user = await GetTwoFactorAuthenticationUserAsync(); + if (user == null) + { + return SignInResult.Failed; + } + + using (CurrentTenant.Change(user.TenantId)) + { + return await base.TwoFactorSignInAsync(provider, code, isPersistent, rememberClient); + } + } + + public override async Task TwoFactorRecoveryCodeSignInAsync(string recoveryCode) + { + var user = await GetTwoFactorAuthenticationUserAsync(); + if (user == null) + { + return SignInResult.Failed; + } + + using (CurrentTenant.Change(user.TenantId)) + { + // Base TwoFactorRecoveryCodeSignInAsync does not invoke PreSignInCheck, which means + // AbpSignInManager's IsActive / ShouldChangePassword checks would be bypassed. Run the + // same pre-sign-in checks here so recovery-code sign-in has the same gating as the + // regular two-factor sign-in path. + var preSignInCheckResult = await PreSignInCheck(user); + if (preSignInCheckResult != null) + { + return preSignInCheckResult; + } + + return await base.TwoFactorRecoveryCodeSignInAsync(recoveryCode); + } + } + /// /// This is to call the protection method PreSignInCheck /// diff --git a/modules/identity/src/Volo.Abp.Identity.Domain/Volo/Abp/Identity/IdentityUserManager.cs b/modules/identity/src/Volo.Abp.Identity.Domain/Volo/Abp/Identity/IdentityUserManager.cs index c0470eef80..46f7a635f0 100644 --- a/modules/identity/src/Volo.Abp.Identity.Domain/Volo/Abp/Identity/IdentityUserManager.cs +++ b/modules/identity/src/Volo.Abp.Identity.Domain/Volo/Abp/Identity/IdentityUserManager.cs @@ -745,21 +745,4 @@ public class IdentityUserManager : UserManager, IDomainService } } } - - public override async Task FindByIdAsync(string userId) - { - // In shared user sharing strategy, a user id is globally unique. Fall back to - // a cross-tenant lookup so callers that hit FindByIdAsync from a non-matching - // tenant context (notably the 2FA mid-flow invoked by ASP.NET Core Identity - // base SignInManager paths) can still resolve the user by id. - // Downstream operations on the returned entity should still be scoped to - // user.TenantId explicitly via CurrentTenant.Change. - var user = await base.FindByIdAsync(userId); - if (user != null || MultiTenancyOptions.Value.UserSharingStrategy == TenantUserSharingStrategy.Isolated) - { - return user; - } - - return await FindSharedUserByIdAsync(userId); - } } diff --git a/modules/identity/test/Volo.Abp.Identity.AspNetCore.Tests/Volo/Abp/Identity/AspNetCore/GetTwoFactorAuthenticationUser_Tests.cs b/modules/identity/test/Volo.Abp.Identity.AspNetCore.Tests/Volo/Abp/Identity/AspNetCore/GetTwoFactorAuthenticationUser_Tests.cs index 267464c646..bbedc2c259 100644 --- a/modules/identity/test/Volo.Abp.Identity.AspNetCore.Tests/Volo/Abp/Identity/AspNetCore/GetTwoFactorAuthenticationUser_Tests.cs +++ b/modules/identity/test/Volo.Abp.Identity.AspNetCore.Tests/Volo/Abp/Identity/AspNetCore/GetTwoFactorAuthenticationUser_Tests.cs @@ -59,4 +59,87 @@ public class GetTwoFactorAuthenticationUser_Tests : SharedAbpIdentityAspNetCoreT content.ShouldBe("null"); } + + [Fact] + public async Task TwoFactorSignInAsync_Should_Resolve_Tenant_User_In_Shared_Mode() + { + // If the tenant switch inside AbpSignInManager.TwoFactorSignInAsync regresses, + // the base implementation would not find the user and return SignInResult.Failed. + // An inactive tenant user lets PreSignInCheck return NotAllowed, proving the user + // was actually located and inspected under its own tenant context. + var tenantUserId = await CreateInactiveTenantUserAsync("shared-2fa-signin", "shared-2fa-signin@abp.io"); + + await WriteTwoFactorCookieAsync(tenantUserId); + + var response = await Client.GetAsync("/api/signin-test/two-factor-signin?provider=Email&code=invalid"); + response.EnsureSuccessStatusCode(); + var result = await response.Content.ReadAsStringAsync(); + + result.ShouldBe("NotAllowed"); + } + + [Fact] + public async Task TwoFactorRecoveryCodeSignInAsync_Should_Resolve_Tenant_User_In_Shared_Mode() + { + // AbpSignInManager.TwoFactorRecoveryCodeSignInAsync runs PreSignInCheck after locating + // the user under its own tenant context. An inactive tenant user therefore produces + // NotAllowed only when the override actually resolves the user; a regression would + // return Failed instead. + var tenantUserId = await CreateInactiveTenantUserAsync("shared-2fa-recovery", "shared-2fa-recovery@abp.io"); + + await WriteTwoFactorCookieAsync(tenantUserId); + + var response = await Client.GetAsync("/api/signin-test/two-factor-recovery-signin?recoveryCode=invalid"); + response.EnsureSuccessStatusCode(); + var result = await response.Content.ReadAsStringAsync(); + + result.ShouldBe("NotAllowed"); + } + + private async Task CreateInactiveTenantUserAsync(string userName, string email) + { + return await CreateTenantUserAsync(userName, email, isActive: false); + } + + private async Task CreateTenantUserAsync(string userName, string email, bool isActive) + { + var userRepository = GetRequiredService(); + var currentTenant = GetRequiredService(); + var unitOfWorkManager = GetRequiredService(); + + var tenantId = Guid.NewGuid(); + Guid userId; + + using (var uow = unitOfWorkManager.Begin()) + { + using (currentTenant.Change(tenantId)) + { + var user = new IdentityUser(Guid.NewGuid(), userName, email, tenantId); + if (!isActive) + { + user.SetIsActive(false); + } + await userRepository.InsertAsync(user); + userId = user.Id; + } + await uow.CompleteAsync(); + } + + return userId; + } + + private async Task WriteTwoFactorCookieAsync(Guid userId) + { + var writeResponse = await Client.GetAsync($"/api/signin-test/write-two-factor-cookie?userId={userId}"); + writeResponse.EnsureSuccessStatusCode(); + + if (writeResponse.Headers.TryGetValues("Set-Cookie", out var setCookies)) + { + Client.DefaultRequestHeaders.Remove("Cookie"); + foreach (var cookie in setCookies) + { + Client.DefaultRequestHeaders.Add("Cookie", cookie.Split(';').First()); + } + } + } } diff --git a/modules/identity/test/Volo.Abp.Identity.AspNetCore.Tests/Volo/Abp/Identity/AspNetCore/SignInTestController.cs b/modules/identity/test/Volo.Abp.Identity.AspNetCore.Tests/Volo/Abp/Identity/AspNetCore/SignInTestController.cs index fe77766cb3..8922915e6d 100644 --- a/modules/identity/test/Volo.Abp.Identity.AspNetCore.Tests/Volo/Abp/Identity/AspNetCore/SignInTestController.cs +++ b/modules/identity/test/Volo.Abp.Identity.AspNetCore.Tests/Volo/Abp/Identity/AspNetCore/SignInTestController.cs @@ -45,4 +45,18 @@ public class SignInTestController : AbpController var user = await _signInManager.GetTwoFactorAuthenticationUserAsync(); return Content(user?.Id.ToString() ?? "null"); } + + [Route("two-factor-signin")] + public async Task TwoFactorSignIn(string provider, string code) + { + var result = await _signInManager.TwoFactorSignInAsync(provider, code, false, false); + return Content(result.ToString()); + } + + [Route("two-factor-recovery-signin")] + public async Task TwoFactorRecoverySignIn(string recoveryCode) + { + var result = await _signInManager.TwoFactorRecoveryCodeSignInAsync(recoveryCode); + return Content(result.ToString()); + } } diff --git a/modules/identity/test/Volo.Abp.Identity.Domain.Tests/Volo/Abp/Identity/IdentityUserManager_Tests.cs b/modules/identity/test/Volo.Abp.Identity.Domain.Tests/Volo/Abp/Identity/IdentityUserManager_Tests.cs index 803bc258a3..1982080cf1 100644 --- a/modules/identity/test/Volo.Abp.Identity.Domain.Tests/Volo/Abp/Identity/IdentityUserManager_Tests.cs +++ b/modules/identity/test/Volo.Abp.Identity.Domain.Tests/Volo/Abp/Identity/IdentityUserManager_Tests.cs @@ -678,55 +678,6 @@ public class SharedTenantUserSharingStrategy_IdentityUserManager_Tests : AbpIden } } - [Fact] - public async Task FindByIdAsync_Should_Fall_Back_To_Cross_Tenant_Lookup_In_Shared_Mode() - { - // In shared user sharing strategy, UserManager.FindByIdAsync must resolve a - // tenant user even when CurrentTenant is host. This protects any caller that - // hits FindByIdAsync without an explicit shared-aware wrapper (e.g. ASP.NET - // Core Identity SignInManager internals for TwoFactorSignInAsync and - // TwoFactorRecoveryCodeSignInAsync). - var tenantId = Guid.NewGuid(); - IdentityUser tenantUser; - - using (var uow = _unitOfWorkManager.Begin()) - { - tenantUser = await CreateUserAsync(tenantId, "shared-findbyid-fallback", "shared-findbyid-fallback@abp.io"); - await uow.CompleteAsync(); - } - - using (_currentTenant.Change(null)) - { - var user = await _identityUserManager.FindByIdAsync(tenantUser.Id.ToString()); - - user.ShouldNotBeNull(); - user.Id.ShouldBe(tenantUser.Id); - user.TenantId.ShouldBe(tenantId); - } - } - - [Fact] - public async Task FindByIdAsync_Should_Return_Current_Tenant_Result_Without_Fallback_When_Found() - { - var tenantId = Guid.NewGuid(); - IdentityUser tenantUser; - - using (var uow = _unitOfWorkManager.Begin()) - { - tenantUser = await CreateUserAsync(tenantId, "shared-findbyid-hit", "shared-findbyid-hit@abp.io"); - await uow.CompleteAsync(); - } - - using (_currentTenant.Change(tenantId)) - { - var user = await _identityUserManager.FindByIdAsync(tenantUser.Id.ToString()); - - user.ShouldNotBeNull(); - user.Id.ShouldBe(tenantUser.Id); - user.TenantId.ShouldBe(tenantId); - } - } - [Fact] public async Task Login_Then_TwoFactor_MidFlow_Should_Resolve_Same_Tenant_User_In_Shared_Mode() {