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.
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
feat: Refresh token expiration window #2827
feat: Refresh token expiration window #2827
Changes from 12 commits
751dbb5
219a58e
efa3b8e
7a52b36
45b9aea
7d572aa
9573041
a203f4b
1ebc99d
3bb1c20
8de23dc
249264f
821fba4
2e14277
07375a9
09d6801
89527fd
69ca392
8fa6c8c
bd8a15e
13cdea8
1f642d3
f244b61
73556c7
a487706
e992907
d1db135
45a8cff
2c7b95f
b1d37ca
42de645
bd2d446
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.
Could you make this explicit? A simple
:=
here would disable encryption, a significant security feature. Thanks!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.
Do we actually need
used
when we haveactive
already?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 think so,
findSessionBySignature
will return errors when a session is marked inactive.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.
Could you add this to the error context instead? Without a request context, it will be difficult to trace the log in a noisy environment :)
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 not sure what you mean here, is there code elsewhere that does this?
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.
Could you add this to the error context instead? Without a request context, it will be difficult to trace the log in a noisy environment :)
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.
So to understand this a bit better - when refreshing a token we do not deactivate it (
active: false
) immediately. Instead, if grace is enabled, we extend the expiry time by X from NOW, and we also setused: true
. The next time we refresh the token, and we are still in the refresh grace period, nothing is updated.When we refresh the token again, and NOW + x has passed, fosite will see the key as expired, implying that it is no longer active.
I think this has some serious implications. I am not 100% any more how token reuse detection works, but I think it doesn't trigger on expired tokens, only on inactive ones. Because an expired token has not been reused, it just expired! And since it's expired, you can't use it. A used (active=false) token though has been used, thus, reuse means invalidation of all other tokens.
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 think you're saying
I mentioned in an earlier comment that setting
active=false
won't work for grace periods, since calls tofindSessionBySignature
will not succeed. To properly detect that case (active==true, in_grace_period==true, expired
) something could be modified to deal with it, I'm worried that changingfindSessionBySignature
to throw an expired error may have undesirable repercussions. From what I can tell token validation, which includes expiration checking, is done at a different layer and is part of a Strategy. The persistence layer does not have access to token strategies.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 was what I was trying to say :)
The best way to prove/disprove this hypothesis is to introduce a test which covers this. If there's no regression on revokation, this should be fine! It would theoretically also be fine to revoke the token chain on expiration also, not just on reuse.
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.
Same as above