-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
V9: Fix login timeout #12029
Merged
Merged
V9: Fix login timeout #12029
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nds request Also adds the TicketExpiresClaim before validating the the security stamp, otherwise the claim won't be merged and "dissappear", leading to the user being instantly logged out Also only EnsureValidSessionId if not RemainingSeconds request, otherwise the session will always be valid, since the remaining seconds request renews it.
Besides what the comment used to state these claims are only issued when logging in, leading you to be logged out once the claims are merged, furthermore when we check the session ID we verify that you session has not expired.
If we don't we lose 30 minutes of our ExpireTimeSpan every time the principal refreshes
And use MergeAllClaims on refreshing principal instead.
bergmania
reviewed
Feb 22, 2022
src/Umbraco.Web.BackOffice/Security/ConfigureBackOfficeCookieOptions.cs
Outdated
Show resolved
Hide resolved
bergmania
approved these changes
Feb 22, 2022
@bergmania @nikolajlauridsen Thank you for addressing this |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes multiple problems I found with timeout, ensuring that it now works as expected. This fixes #11965
There were two major problems a couple of minor ones
GetRemainingTimeout
The frontend requests GetRemainingTimeout every 30 seconds to see if it should redirect the user to the login page. The Problem with this is a real chicken or egg scenario without previous implementation. In order to get the remaining time out seconds from the
TicketExpiresClaimType
, we must authenticate the user, but with the old implementation the very act of authenticating the user counts as a "backoffice request", which automatically refreshed the cookie and our database login session, meaning that in effect you would never get logged out!To fix this I've had to do two things, first of all, I've our auth cookie expiration away from
SlidingExpiration
and we now instead manage this "manually", there is two reasons for this, the first is that we don't want this token to update when we requestGetRemainingTimeout
, and withSlidingExpiration
this will always happen regardless, once the half the lifespan of the token has passed. Another subtle side effect of usingSlidingExpiration
is that ourTimeOut
setting won't be honest, for instance, you might set your timeout to be 12 hours, poke around the back office for 5 hours, and step away, now you'd expect to be logged out after 12 hours, however, you won't, you'd be logged out after 7 hours, since you never hit the halfway point of your token, meaning it was never refreshed.Secondly, I've added a check
IsRemainingSecondsRequest
where we check if the current request we're trying to authenticate is from the GetRemainingTimeoutSeconds action, if it is, we skip doingEnsureValidSessionId
, and we don't renew the token.The purpose of
EnsureValidSessionId
is to validate that our database login session is not older than our timespan allows, however with the check also updates theLastValidatedUtc
, meaning that if we call this from the remaining seconds request it'll never expire, and additionally since theGetRemainingTimeout
, very much serves the same purpose, but using the token expiration instead, it doesn't make sense to also callEnsureValidSessionId
.Refreshing principal
Now you might think, but wait, I do get logged out automatically, and that is correct. But that's caused by a different bug.
By default, the auth principal will be refreshed every 30 minutes, and this is when you'd be logged out. There are two reasons for this.
First off when the principal refreshes we have some custom code that copies our custom claims we've added over to the new identity, but when doing so we explicitly ignored the
CookiePath
andSessionIdClaimType
, this meant that when your principal refreshed you lost you SessionID, essentially ending your session,EnsureValidSessionId
would then catch this and log you out, I've changed this so when the principal is refreshed we copy all claims.Secondly, there was a bug related to the custom
TicketExpiresClaim
, previously we added the claim after the call tosecurityStampValidator.ValidateAsync(ctx);
, this meant that the claim was not added before we merged the claim, and that led to theGetRemainingTimeout
to log you out because it though the session had expired, I've changed this so we now add the claim before the call, and everything works as intended.Lastly I found a weird bug in relation to refreshing the principal, essentially when the principal is refreshed the token gets a new
IssuedUtc
, corresponding to when the principal was refreshed, however theExpiresUtc
remains the same. When the token is then renewed the framework then calculates theExpireTimeSpan
by doingExpiresUtc - IssuedUtc
, resulting in the timespan being reduced by 30 minutes for every principal refresh! To fix this I've updated it so we now explicitly set theIssuedUtc
andExpiresUtc
when renewing the token.Testing
Note: There's a gotcha when it comes to the timespan string in the appsettings.json, if you want to specify 24 hours or above you must do so in the format:
d.HH:MM:SS
, for instance if you want 24 hours the string is"1.00:00:00"
When testing I checked than you get logged out at the correct interval when
I also tested that the auth token is created correctly when you switch user, and that external login still works as intended, and that
GetRemainingTimeoutSeconds
counts and updates correctly.If you don't want to wait for the 30 minutes it takes for the security stamp to be validated you can change the validation interval in
ConfigureSecurityStampOptions
during testing.