handlers: do not fail login if refresh token gone #1670
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
|
It looks like the CI may have failed for an unrelated reason. Is there an easy way to retry it? |
|
Please rebase on master, so the CI issue can get resolved. |
|
It would be great to get this merged as its impacting us as well cc @willvrny |
There is a chance that offline storage could fall out of sync with the refresh token tables. One example is if dex crashes/is stopped in the middle of handling a login request. If the old refresh token associated with the offline session is deleted, and then the process stops, the offline session will still refer to the old token. Unfortunately, if this case occurs, there is no way to recover from it, since further logins will be halted due to dex being unable to clean up the old tokens till referenced in the offline session: the database is essentially corrupted. There doesn't seem to be a good reason to fail the auth request if the old refresh token is gone. This changes the logic in `handleAuthCode` to not fail the entire transaction if the old refresh token could not be deleted because it was not present. This has the effect of installing the new refresh token, and unpdating the offline storage, thereby fixing the issue, however it occured.
klarose
force-pushed
the
handle-refresh-no-present
branch
from
ca69047
to
ab5ea03
Compare
|
@bonifaido Rebased, but it looks like the lint timed out this time. :( |
|
Build is fixed now! :) |
bonifaido
approved these changes
Mar 19, 2020
|
@bonifaido any idea when a release with this will be cut? |
|
No planned released for now, but there are some fixes hanging around here, we will review a few and I will cut a patch release. Until that: https://quay.io/repository/dexidp/dex/manifest/sha256:2f5af70a79eae505d122ac40f684cff29fabda434f5fefeb49c0d888323a545d |
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.
There is a chance that offline storage could fall out of sync with the
refresh token tables. One example is if dex crashes/is stopped in the
middle of handling a login request. If the old refresh token associated
with the offline session is deleted, and then the process stops, the
offline session will still refer to the old token.
Unfortunately, if this case occurs, there is no way to recover from it,
since further logins will be halted due to dex being unable to clean up
the old tokens till referenced in the offline session: the database is
essentially corrupted.
There doesn't seem to be a good reason to fail the auth request if the
old refresh token is gone. This changes the logic in
handleAuthCodetonot fail the entire transaction if the old refresh token could not be
deleted because it was not present. This has the effect of installing
the new refresh token, and updating the offline storage, thereby fixing
the issue, however it occurred.
Fixes #1669