Random User.IsInRole result when UserRole changes

[ans-ashkan]

Is this a Bug or Feature request?:

Bug

Steps to reproduce (preferably a link to a GitHub repo with a repro project):

Clone and Run this project
do the following in order:

http://localhost:5000/users/getall -> 200: ["test"]
http://localhost:5000/users/signin?username=test&password=123 -> 200
http://localhost:5000/users/isinrole?role=admin -> {"isInRole":false,"identityName":"test"}
http://localhost:5000/users/adduserrole?username=test&role=admin -> 200
http://localhost:5000/users/isinrole?role=admin -> {"isInRole":**random true or false**,"identityName":"test"}
http://localhost:5000/users/signout -> 200
http://localhost:5000/users/signin?username=test&password=123 -> 200
http://localhost:5000/users/isinrole?role=admin -> {"isInRole":true,"identityName":"test"}

Description of the problem:

I'm using asp.net core 2.1 with some default identity settings and every time a role is changed the user should re-login to see the role changes.
If I invalidate Identity on each request with the following settings, then roles should be updated but UserIsInRole returns random results.

services.Configure<SecurityStampValidatorOptions>(options =>
{
     options.ValidationInterval = TimeSpan.Zero;
});

Version of Microsoft.AspNetCore.Mvc or Microsoft.AspNetCore.App or Microsoft.AspNetCore.All:

Microsoft.AspNetCore.App 2.1.1

SO question of this issue

[poke]

As I’ve found out, UserManager.AddToRoleAsync does not actually update the user’s security stamp, so the security stamp will not actually be invalidated. So this mechanism does not actually work for refreshing the login for the user on the basis of changed roles. The observed effect is still very interesting and I don’t really understand yet what is going on.

I’ve already been investigating this a bit, and looked at the browser requests more closely, and these are my findings so far:

The cookie will be kept for as long as the requests stay within the ValidationInterval. If we make multiple requests within that configured second, then only the first will have a Set-Cookie header, and the remaining ones will be kept. If we set the interval up to 5 seconds, we can make requests with the same identity for 5 seconds before we get a new Set-Cookie with an updated token.

By adding a Claims = User.Claims.Select(c => new { c.Type, c.Value }) line to the output of the IsInRole action, I was able to see the exact claims that are part of the identity. As expected, the SecurityStamp stays the same on all requests, regardless of whether I do additional Add/RemoveRole calls to change the roles.

However, and now this is very interesting, the role claim is only ever included when the ValidationInterval kicks in and updates the cookie. So on every request that has a Set-Cookie header, I can see the role while on all other requests, the role is missing. Yes, it is missing even afterwards. So multiple browser requests may look like this:

REQ  Cookie: <cookie-1>
RES  InRole: false

REQ  Cookie: <cookie-1>
RES  InRole: true
     Set-Cookie: <cookie-2>

REQ  Cookie: <cookie-2>
RES  InRole: false

REQ  Cookie: <cookie-2>
RES  InRole: false

REQ  Cookie: <cookie-2>
RES  InRole: false

REQ  Cookie: <cookie-2>
RES  InRole: true
     Set-Cookie: <cookie-3>

REQ  Cookie: <cookie-3>
RES  InRole: false

This seems very odd to me. I believe what is going on here is that when the validation interval expires, a new identity is being created which loads the roles from the database and displays the updated state. This is then available in the controller and results in the set role claim. The cookie authentication handler however then just reissues the previous identity which does not have the updated roles.

It would be correct for the cookie handler to reuse the existing identity (since as per the security stamp validation, the identity is still valid), but the temporarily created identity (probably for validation purposes) should not leak outside of the validation process.

So that really would be a bug. And that probably also already exists in the wild already while just being less visible since the default validation interval is 30 minutes, making this appear only for one request within 30 minutes and as such very unlikely to be observed.

[poke]

Okay. The problem is that the CookieAuthenticationHandler will just renew the existing ticket:

await Events.ValidatePrincipal(context);
// …
if (context.ShouldRenew)
{
    RequestRefresh(result.Ticket);
}

So in our case, where the security stamp is being validated is determined as valid (since it doesn’t change from a role change), we are getting in the SecurityStampVerified branch where the cookie renewal is being requested:

context.ReplacePrincipal(newPrincipal);
context.ShouldRenew = true;

The principal appears to be replaced there, however when we take a look at what ReplacePrincipal actually does, we can see why this fails:

public void ReplacePrincipal(ClaimsPrincipal principal) => Principal = principal;

So this updates the instance variable of CookieValidatePrincipalContext, which causes the cookie authentication handler to use this as the authenticated identity. But the cookie authentication handler renews the ticket, not the possibly replaced principal. And the ticket will still contain the original principal.

So this is obviously a bug. I see a few variants on how we could fix this (depending on what we determine is the right behavior):

• Option 1: Change the RequestRefresh call of the cookie authentication handler to also take the principal context’s principal, so that the renewal will use the possibly updated principal instead of the original one from the ticket. This will also generally allow the ValidatePrincipal event to modify the principal, not only for the security stamp validator.

• Option 3: If we look at this in a pure sense, changing a user’s roles does modify its security stamp. So the security stamp validation should not actually need to update the identity. So since the identity is still deemed as valid, it maybe should not be replaced by a fresh identity that may have updated properties.

  It would maybe be somewhat a weird behavior if the identity and its claims would change throughout the application (without performing another sign-in) just because some validation interval of a separate component has expired. I don’t know but maybe that is the wrong behavior here. So making the security stamp validator not replace the identity would be correct.

In any case, when we decide on a proper solution, I’ll be happy to provide the implementation, given that I’m already knee-deep in this topic.

[poke]

/cc @HaoK (since this was basically your work in aspnet/Identity#1351)

(and yes, Mvc is probably the wrong repo for this :/)

[kevinchalet]

FYI, the fact the "cookies refresh" stuff doesn't work properly in 2.1 is due to a known bug: aspnet/Security#1788

[khellang]

Looks like they're going with Option 1; aspnet/Security#1795 :smile

[poke]

*sigh* Guess I am too late to the party 😢

Guess we can close this then @ans-ashkan. I have also answered your SO question with a better approach.

[ans-ashkan]

Thanks everyone, It's nice to have such awesome community (which also is super fast).

[HaoK]

I'm glad you came to the same conclusion I did for the fix @poke :) But yeah this was a regression