identity: preserve session refresh schedule #4633
Merged
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
kenjenkins
force-pushed
the
kenjenkins/access-tracker
branch
2 times, most recently
from
d0aa93b
to
f583388
Compare
The databroker identity manager is responsible for refreshing session records, to account for overall session expiration as well as OAuth2 access token expiration. Refresh events are scheduled subject to a coolOffDuration (10 seconds, by default) relative to a lastRefresh timestamp. Currently, any update to a session record will reset the associated lastRefresh value and reschedule any pending refresh event for that session. If an update occurs close before a scheduled refresh event, this will push back the scheduled refresh event to 10 seconds from that time. This means that if a session is updated frequently enough (e.g. if there is a steady stream of requests that cause constant updates via the AccessTracker), the access token may expire before a refresh ever runs. To avoid this problem, do not update the lastRefresh time upon every session record update, but only if it hasn't yet been set. Instead, update the lastRefresh during the refresh attempt itself. Add unit tests to exercise these changes. There is a now() function as part of the manager configuration (to allow unit tests to set a fake time); update the Manager to use this function throughout.
kenjenkins
force-pushed
the
kenjenkins/access-tracker
branch
from
f583388
to
6a24b01
Compare
calebdoxsey
approved these changes
Oct 24, 2023
backport-actions-token bot
pushed a commit
that referenced
this pull request
Oct 24, 2023
The databroker identity manager is responsible for refreshing session records, to account for overall session expiration as well as OAuth2 access token expiration. Refresh events are scheduled subject to a coolOffDuration (10 seconds, by default) relative to a lastRefresh timestamp. Currently, any update to a session record will reset the associated lastRefresh value and reschedule any pending refresh event for that session. If an update occurs close before a scheduled refresh event, this will push back the scheduled refresh event to 10 seconds from that time. This means that if a session is updated frequently enough (e.g. if there is a steady stream of requests that cause constant updates via the AccessTracker), the access token may expire before a refresh ever runs. To avoid this problem, do not update the lastRefresh time upon every session record update, but only if it hasn't yet been set. Instead, update the lastRefresh during the refresh attempt itself. Add unit tests to exercise these changes. There is a now() function as part of the manager configuration (to allow unit tests to set a fake time); update the Manager to use this function throughout.
kenjenkins
added a commit
that referenced
this pull request
Oct 24, 2023
identity: preserve session refresh schedule (#4633) The databroker identity manager is responsible for refreshing session records, to account for overall session expiration as well as OAuth2 access token expiration. Refresh events are scheduled subject to a coolOffDuration (10 seconds, by default) relative to a lastRefresh timestamp. Currently, any update to a session record will reset the associated lastRefresh value and reschedule any pending refresh event for that session. If an update occurs close before a scheduled refresh event, this will push back the scheduled refresh event to 10 seconds from that time. This means that if a session is updated frequently enough (e.g. if there is a steady stream of requests that cause constant updates via the AccessTracker), the access token may expire before a refresh ever runs. To avoid this problem, do not update the lastRefresh time upon every session record update, but only if it hasn't yet been set. Instead, update the lastRefresh during the refresh attempt itself. Add unit tests to exercise these changes. There is a now() function as part of the manager configuration (to allow unit tests to set a fake time); update the Manager to use this function throughout. Co-authored-by: Kenneth Jenkins <51246568+kenjenkins@users.noreply.github.com>
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.
Summary
The databroker identity manager is responsible for refreshing session records, to account for overall session expiration as well as OAuth2 access token expiration.
Refresh events are scheduled subject to a
coolOffDuration(10 seconds, by default) relative to alastRefreshtimestamp. Currently, any update to a session record will reset the associatedlastRefreshvalue and reschedule any pending refresh event for that session. If an update occurs close before a scheduled refresh event, this will push back the scheduled refresh event to 10 seconds from that time.This means that if a session is updated frequently enough (e.g. if there is a steady stream of requests that cause constant updates via the AccessTracker), the access token may expire before a refresh ever runs.
To avoid this problem, do not update the
lastRefreshtime upon every session record update, but only if it hasn't yet been set. Instead, update thelastRefreshduring the refresh attempt itself.Add unit tests to exercise these changes. There is a
now()function as part of the manager configuration (to allow unit tests to set a fake time); update the Manager to use this function throughout.Related issues
https://github.com/pomerium/internal/issues/1558
User Explanation
Improve reliability of session refresh.
Checklist
improvement/bug/ etc)