[Regression] Role/claim changes in identity aren't persisted into the auth cookie

[kanadaj]

When ASP.NET Core Identity tries to update the current user's claims, it updates the Principal of the current request, however the update to the claims is not persisted to the cookie.

I've also made an issue for this in aspnet/Identity before I've managed to figure out the details: aspnet/Identity#1845

I'm not sure which repo this issue really belongs to considering this seems like an interop issue.

Repro repo with steps: https://github.com/kanadaj/aspnetrefreshrolesrepro/tree/master

[kanadaj]

@Tratcher here you go. I still think the problem is that Identity modifies context.Principal but Security persists context.Ticket.Principal inside
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationHandler.cs#L88
A quick verification if this is the case would be welcome since I'm running a system in production and it's causing issues for the users, but the pipeline is large enough for me to be unsure.

[kanadaj]

Security/test/Microsoft.AspNetCore.Authentication.Test/CookieTests.cs

Line 958 in 1df139e

var id = ctx.Principal.Identities.First();

This test doesn't cover the case where ctx.ReplacePrincipal() is used to replace the principal with a new one, since you're using the reference directly.

[kanadaj]

As a quick and dirty workaround, this seems to work

options.OnRefreshingPrincipal = context =>
{
    // Any original code in OnRefreshingPrincipal
    // ...

    // Copy the new principal claims into the old object while keeping the reference to context.ticket.Principal
    var oldIdentity = context.CurrentPrincipal.Identities.First();
    var newIdentity = context.NewPrincipal.Identities.First();

    Claim claim;
    while ((claim = oldIdentity.Claims.FirstOrDefault()) != null)
    {
        oldIdentity.RemoveClaim(claim);
    }

    foreach (var newClaim in newIdentity.Claims)
    {
        oldIdentity.AddClaim(newClaim);
    }

    return Task.FromResult(0);
};

[Tratcher]

@HaoK can you take a look? If this regressed then it's a patch candidate.

[HaoK]

Sure, I wouldn't be surprised if this was a regression from the cloning change

[HaoK]

Well so this is interesting, looks like the security stamp might have been relying on the old behavior before to persist updates to the cookie:

See https://github.com/aspnet/Identity/blob/dev/src/Identity/SecurityStampValidator.cs#L80

Expectation was that ReplacePrincipal + ShouldRenew would cause the cookie to be updated for the user. I'll add a functional test in identity confirming that this is indeed broken now

[HaoK]

This is the regression change with the cloning, might need to just revert? 1df139e

Since it does change the semantics of ReplacePrincipal (which Identity's SecurityStampValidator was relying on)

[kanadaj]

@HaoK Just wondering here, why not keep a reference to the ticket instead and set up context.Principal for the CookieValidatePrincipalContext to point to the ticket's Principal via get and set? Or would that break other things?

[HaoK]

So certainly we can work around the new behavior in identity, but if this broke identity, it should at least be looked at like a regression.

[kanadaj]

Or alternatively, alter ReplacePrincipal to also set the ticket's principal.

[kanadaj]

That's true

[Tratcher]

@kanadaj may be right, we should be able to fix ReplacePrincipal to allow explicit cookie updates and still clone to prevent later app contamination.

[Tratcher]

@muratg this needs triaging. It's a regression in 2.1 and may warrant a patch.

[muratg]

cc @Eilon

[Eilon]

Patch is approved.

[Eilon]

@HaoK - are we good to close this bug now? Is the fix in the patch and in 2.2 and in master?

[HaoK]

Yep, thx for reminding me, fixed via #1811