Fix regression in Cookies #1795
Conversation
| OnValidatePrincipal = ctx => | ||
| { | ||
| ctx.ShouldRenew = true; | ||
| ctx.ReplacePrincipal(new ClaimsPrincipal(new ClaimsIdentity(new GenericIdentity("Alice2", "Cookies2")))); |
There was a problem hiding this comment.
Is it not possible to update the AuthProperties at the same time?
There was a problem hiding this comment.
Hrm, we might have a similar bug with properties I guess due to the cloning change. Renew probably is stomping on any changes that were applied in the context
There was a problem hiding this comment.
There's no issue with properties because the context ends up taking the ticket properties
so updates to the ticket's properties show up automatically when they get copied:
since ticket.Properties == context.Properties
I'll update the test to set a property, but I don't think we need to change the fix
|
Do it. |
|
I’ve touched on this in aspnet/Mvc#8011, and just want to check: Do we really want to make a security stamp validation with a successful result refresh the identity, making it update all claims? Currently, we didn’t have this behavior before and an identity would always be the same until we sign in again. But with this, we are essentially updating the claims identity on every security stamp validation interval, which might be a nice but similarly also confusing side-effect. |
|
@poke this is how this security stamp feature was originally designed to work, identity would take care of ensuring a fresh set of user claims are regenerated every so often (validationInterval), which should be determined based on a performance/staleness tradeoff since as you pointed out, its not cheap to hit the database/generate user claims. |
|
More specifically, it worked that way in 2.0 but regressed in 2.1. |
|
Okay, as long as that’s the desired effect, I’m happy with it |
|
@ajcvickers I think the fix in this PR is good for patch approval now |
|
@natemcmaster this was approved for patch, should I merge this to release/2.1 and then port it back to master? |
|
Yes. Change this PR to target release/2.1 instead of master. Then, merge release/2.1 into release/2.2 (the bot hasn't automated this one yet). After that, the bot will open a PR to merge your release/2.2 change into master. |
* Use pattern matching to check exception type in JwtBearerHandler * Use expression body to configure events in JwtBearerHandler
|
Replaced by #1811 since the rebase didn't go well |
Fix for #1788
CookieCanBeUpdatedByValidatorDuringRefresh is the new regression test that fails for the identity scenario.
@Tratcher @ajcvickers @blowdart