This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
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.
Document the usage of refresh tokens. #11427
Document the usage of refresh tokens. #11427
Changes from 3 commits
b650438
0af540e
6c2256f
95fe78e
90f18b6
e6104a8
4e1359f
9303f8c
8583224
3e2f56a
ceb032a
b4cefa8
2b03c75
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest "duration" rather than "length" to avoid confusion with a string length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or long-lived as below.
Presumably both these terms refer to finite, but large expiry times, rather than no expiry at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about if I don't want to offer these at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. Perhaps this ought to have some special behaviour for
0
to reject logins from incapable clients.That said, you're no worse off offering a 5 minute nonrefreshable access token if you usually offer 5 minute refreshable access token — though the user experience is a concern here: rejecting the login altogether at least protects the user from thinking they're fine and getting logged out abruptly 5 minutes later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two uses of inconvenience here. Can we explain how they're inconvenienced, i.e. that they must re-authenticate with the login credentials?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Short answer: it's misleading but I mean that the clients keep refreshing their access tokens when needed so they always have a valid one.
I'm making a bit of an assumption that an active client wants to be syncing all the time.
To sync, it needs a valid access token.
I'm then saying that an 'active' client that has a valid refresh token but an invalid access token will refresh immediately, because otherwise it can't sync (and therefore it contradicts the fact that it's supposedly an 'active' client and all active clients sync all the time).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I parse this as
A <= R
. Judging by point 1 below, I think you don't want to allowA = R
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's nothing too wrong with
A = R
; clients would just have to be diligent about refreshing...The only confusing part is that the section below explaining the range of inactivities that will kick a client would then have a lower bound of zero.
In reality this depends on client behaviour and it's unlikely that clients would wait until the absolute last second to refresh their tokens, but I was trying to be helpful to admins who are thinking something like 'I want sessions inactive for 15 minutes to be logged out' and trying to paint a picture that it depends on when the clients happen to have refreshed their token — are you really OK with that meaning that an inactivity of (e.g. 5 minutes) could also trigger that behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow this. What is the trigger for a session to be logged out? Your refresh token expires without being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key thing I didn't understand here is that the start of the inactivity period can be before your access and refresh tokens were generated. I think the example below would be improved by this.
(Is it always the case that the active access and active refresh tokens are generated at the same instant?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by this and on first hand it sounds wrong but I think it's more that I've misunderstood.
Yup, maybe I should actually explain that 'refreshing an access token' means that you exchange your (old) refresh token for a new access token AND a new refresh token at the same time. It's kind of implicit knowledge to me now but I suppose I didn't actually tell you that before :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like
Suppose I am granted an access token and refresh token at 12:00. These last for 5 and 20 minutes respectively.
Feels like an information overload. I wonder if it'd be simpler to just say something like
Hmm. I'm not really convinced by my effort there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm attempting (maybe not very well) to paint a picture of how homeserver admins can configure their servers to have which user-visible behaviours.
What clients should do should hopefully be obvious from the spec.
Basically, if a homeserver admin (or one of our customers...) has a problem statement like 'I want sessions to get logged out automatically if inactive for 15 minutes', this document should help them decide what values they need and hopefully explain why you get a bit of a range of times (with the widest range assuming that clients are lazy and only refresh at the last possible moment)...