Skip to content
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

Issue #367: Refresh token expiration fix #409

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

josephquigley
Copy link
Contributor

When OAuth2.doRefreshToken attempts to refresh an expired token that does not return an HTTP 400 error, then the re-authorization flow is never presented.

A more detailed explanation of the fix is explained by @matthewtintabee in the issue itself:

There appears to be a bug in one of the code paths in OAuth2.swift in 'doRefreshToken' in the case of an error. In the main 'do' section, if a generic error code 400 is returned, the refresh token is cleared and the next attempt to authorise therefore does not use it and things proceed as they should.

Normally, the invalid token error is identified earlier in 'parseRefreshTokenResponseData' which throws a more specific exception which is then handled by the exception block in 'doRefreshToken'. in this case, which is the normal path for this situation, the refresh token is not cleared, meaning that in the next attempt it tries to use the same token again, resulting in stalemate. Clearing the refresh token in the exception handler therefore fixes this problem.

@josephquigley josephquigley force-pushed the quigs/refresh_token_expiration_fix branch from 97f014c to 01e8a9b Compare October 4, 2023 17:52
@@ -391,6 +391,9 @@ open class OAuth2: OAuth2Base {
callback(json, nil)
}
catch let error {
// Fixes [Issue #367](https://github.com/p2/OAuth2/issues/367)
// Refresh token needs to be cleared out upon error, otherwise re-authorizing will not ocurr because the library thinks it has a valid refresh token and tries to fetch a new access token with an expired refresh token.
self.clientConfig.refreshToken = nil
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would clear the refresh token on any error, also e.g. when the server is temporarily unavailable (OAuth2Error.temporarilyUnavailable). I don't think this is desirable. The right way would be to check if the error is the invalid_grant error (which is thrown as OAuth2Error.invalidGrant) and delete the refresh token only in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants