dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.28k stars 9.96k forks source link

amr claim lost after ClaimPrincipal renewal #44666

Open jherbert opened 1 year ago

jherbert commented 1 year ago

General inquiry type issue...

amr claim type was added in #10636 and #5783 using the SignInWithClaimsAsync overload on the SignInManager. When the validation interval is hit UserClaimsPrincipalFactory.GenerateClaimsAsync gets called to refresh the ClaimPrincipal claims but the amr claim is lost. Is this by design? We were attempting to put a feature behind 2fa only and using this claim as authorization. If it is suggestions on some possible workarounds? We could add this claim again in our UserClaimsPrincipalFactory. implementation if the user has 2fa enabled with an assumption the user did actually authenticate with it?

Recreation steps

  1. new razor app with indvidual identity
  2. Shorten validation interval so you don't have to wait the default 30 minutes.
    builder.Services.Configure<SecurityStampValidatorOptions>(options =>
    {
    options.ValidationInterval = TimeSpan.FromSeconds(30);
    });
  3. Signin you'll see the amr claim on the user.
  4. Wait 30 seconds, refresh page and amr claim is gone
blowdart commented 1 year ago

So this is somewhat odd. The amr claim is for the specific sign-in that took place. Once you're into GenerateClaimsAsync you're onto user claims, not claims for the login. I don't think you can know for sure at that point whether an amr claim or not is appropriate.

We acknowledge this sucks, and we'll think for 8.0 about if/how we could persist it.

ghost commented 1 year ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

reponemec commented 1 year ago

Until a fix is released, can SecurityStampValidatorOptions.OnRefreshingPrincipal handler be used as a workaround? Handler will return lost claims to the game.

jherbert commented 10 months ago

@reponemec Good suggestion here, OnRefreshingPrincipal has both a CurrentPrincipal and NewPrincipal property here.