Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Upgrade to .Net7] SecurityStampValidator<TUser> forces signout of TwoFactorRememberMeScheme which leads to InvalidOperationException in case scheme is not registered #47368

Open
1 task done
plachor opened this issue Mar 22, 2023 · 3 comments
Labels
area-identity Includes: Identity and providers bug This issue describes a behavior which is not expected - a bug. cost: XS Will take up to half a day to complete Pillar: Technical Debt triaged
Milestone

Comments

@plachor
Copy link

plachor commented Mar 22, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

Hi, in my project I'm relaying on AddIdentityCore followed by checrrypicked registrations like AddSignInManager and etc. It allows me to setup minimal Identity for my project. I do not want roles nor I do not want default authentication schemes.

Till now I have not yet invested in MFA or 2FA as most of users of this product are authenticated through external authentication scheme (which supports MFA).

So I have custom authentication schemes. (OpenIdConnect, Cookie and External for SSO). However I do not have TwoFactorRememberMeScheme as I do not use it. I've tried to bump my project to .NET7 from .NET6. However it turns out that in .NET7 default SecurityStampValidator<TUser> is forcing sign-out of TwoFactorRememberMeScheme scheme.

This ends with InvalidOperationException as there is no handler registered for that scheme. Check: https://github.com/dotnet/aspnetcore/blob/main/src/Identity/Core/src/SecurityStampValidator.cs#L137

Expected Behavior

Since you allow minimal setup as AddIdentityCore is public and not obsolete than perhaps you should not enforce some hard-coded scheme sign-out. in default implementation of SecurityStampValidator

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

7.0.201

Anything else?

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-identity Includes: Identity and providers label Mar 22, 2023
@mkArtakMSFT mkArtakMSFT added the bug This issue describes a behavior which is not expected - a bug. label Mar 28, 2023
@mkArtakMSFT mkArtakMSFT modified the milestones: .NET 8 Planning, Backlog Mar 28, 2023
@ghost
Copy link

ghost commented Mar 28, 2023

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@dotnet dotnet deleted a comment Mar 28, 2023
@jorrit
Copy link

jorrit commented Mar 29, 2023

This addition seems to work, I hope it is an OK workaround.

            // Workaround for https://github.com/dotnet/aspnetcore/issues/47368
            .AddCookie(IdentityConstants.TwoFactorRememberMeScheme);

I think the SecurityStampValidator should check if the scheme is registered before invoking the signout method. It seems that IAuthenticationSchemeProvider provides this.

@mkArtakMSFT mkArtakMSFT added bug This issue describes a behavior which is not expected - a bug. and removed bug This issue describes a behavior which is not expected - a bug. labels Nov 21, 2023
@mkArtakMSFT mkArtakMSFT modified the milestones: Backlog, AuthPlanning Nov 21, 2023
@mkArtakMSFT mkArtakMSFT added Pillar: Technical Debt cost: XS Will take up to half a day to complete triaged labels Nov 21, 2023
@halter73
Copy link
Member

I think the SecurityStampValidator should check if the scheme is registered before invoking the signout method. It seems that IAuthenticationSchemeProvider provides this.

This sounds right to me too. We've already started doing this in SignInManager starting in .NET 8 so it can support MapIdentityApi<TUser>().

public virtual async Task<bool> IsTwoFactorClientRememberedAsync(TUser user)
{
if (await _schemes.GetSchemeAsync(IdentityConstants.TwoFactorRememberMeScheme) == null)

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@mkArtakMSFT mkArtakMSFT modified the milestones: AuthPlanning, Backlog Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-identity Includes: Identity and providers bug This issue describes a behavior which is not expected - a bug. cost: XS Will take up to half a day to complete Pillar: Technical Debt triaged
Projects
None yet
Development

No branches or pull requests

6 participants
@halter73 @jorrit @wtgodbe @plachor @mkArtakMSFT and others