Skip to content

Switch to the user's tenant in AbpSignInManager.#24942

Merged
EngincanV merged 1 commit intorel-10.2from
AbpSignInManager-Change-Tenant
Feb 25, 2026
Merged

Switch to the user's tenant in AbpSignInManager.#24942
EngincanV merged 1 commit intorel-10.2from
AbpSignInManager-Change-Tenant

Conversation

@maliming
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings February 25, 2026 03:38
@maliming maliming added this to the 10.2-final milestone Feb 25, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds tenant context switching to sign-in operations in AbpSignInManager to ensure that tenant-specific settings and data filtering are correctly applied during authentication. The changes address a multi-tenancy bug where sign-in operations were not executing in the user's tenant context, which could cause incorrect behavior when users from different tenants authenticate.

Changes:

  • Injected ICurrentTenant dependency to enable tenant context switching
  • Wrapped sign-in operations (UpdateUserAsync, SignInOrTwoFactorAsync, PasswordSignInAsync) with CurrentTenant.Change(user.TenantId) to ensure they execute in the correct tenant context
  • Refactored _identityUserManager from private field to protected IdentityUserManager property for better extensibility
Comments suppressed due to low confidence (2)

modules/identity/src/Volo.Abp.Identity.AspNetCore/Volo/Abp/Identity/AspNetCore/AbpSignInManager.cs:119

  • Missing tenant context switch for ExternalLoginSignInAsync. The PreSignInCheck and SignInOrTwoFactorAsync calls access tenant-specific settings (e.g., password change period settings via SettingProvider in ShouldPeriodicallyChangePasswordAsync). These operations should execute within the user's tenant context to ensure correct settings and data filtering are applied. Wrap lines 113-118 with using (CurrentTenant.Change(user.TenantId)) { ... } similar to the pattern used in PasswordSignInAsync at lines 99-102.
    public override async Task<SignInResult> ExternalLoginSignInAsync(string loginProvider, string providerKey, bool isPersistent, bool bypassTwoFactor)
    {
        var user = await FindByLoginAsync(loginProvider, providerKey);
        if (user == null)
        {
            return SignInResult.Failed;
        }

        var error = await PreSignInCheck(user);
        if (error != null)
        {
            return error;
        }
        return await SignInOrTwoFactorAsync(user, isPersistent, loginProvider, bypassTwoFactor);
    }

modules/identity/src/Volo.Abp.Identity.AspNetCore/Volo/Abp/Identity/AspNetCore/AbpSignInManager.cs:103

  • Missing test coverage for multi-tenant sign-in scenarios. The changes introduce tenant context switching for sign-in operations, but there are no tests verifying that sign-in works correctly when users belong to different tenants. Consider adding tests that verify: 1) Password sign-in with tenant users and tenant-specific settings, 2) External login sign-in with tenant users, 3) ExternalLoginSignInAsync with tenant users. This is especially important given the critical bug fix for ExternalLoginSignInAsync missing tenant context.
    public override async Task<SignInResult> PasswordSignInAsync(
        string userName,
        string password,
        bool isPersistent,
        bool lockoutOnFailure)
    {
        IdentityUser user;
        foreach (var externalLoginProviderInfo in AbpOptions.ExternalLoginProviders.Values)
        {
            var externalLoginProvider = (IExternalLoginProvider)Context.RequestServices
                .GetRequiredService(externalLoginProviderInfo.Type);

            if (await externalLoginProvider.TryAuthenticateAsync(userName, password))
            {
                user = await FindByNameAsync(userName);
                if (user == null)
                {
                    if (externalLoginProvider is IExternalLoginProviderWithPassword externalLoginProviderWithPassword)
                    {
                        user = await externalLoginProviderWithPassword.CreateUserAsync(userName, externalLoginProviderInfo.Name, password);
                    }
                    else
                    {
                        user = await externalLoginProvider.CreateUserAsync(userName, externalLoginProviderInfo.Name);
                    }
                }
                else
                {
                    using (CurrentTenant.Change(user.TenantId))
                    {
                        if (externalLoginProvider is IExternalLoginProviderWithPassword externalLoginProviderWithPassword)
                        {
                            await externalLoginProviderWithPassword.UpdateUserAsync(user, externalLoginProviderInfo.Name, password);
                        }
                        else
                        {
                            await externalLoginProvider.UpdateUserAsync(user, externalLoginProviderInfo.Name);
                        }
                    }
                }

                using (CurrentTenant.Change(user.TenantId))
                {
                    return await SignInOrTwoFactorAsync(user, isPersistent);
                }
            }
        }

        user = await FindByNameAsync(userName);
        if (user == null)
        {
            return SignInResult.Failed;
        }

        using (CurrentTenant.Change(user.TenantId))
        {
            return await PasswordSignInAsync(user, password, isPersistent, lockoutOnFailure);
        }
    }

@maliming maliming requested a review from EngincanV February 25, 2026 05:30
@EngincanV EngincanV merged commit cc274c8 into rel-10.2 Feb 25, 2026
7 of 9 checks passed
@EngincanV EngincanV deleted the AbpSignInManager-Change-Tenant branch February 25, 2026 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants