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

No Authentication when refresh_token is invalid #367

Open
svencorell opened this issue May 19, 2021 · 3 comments
Open

No Authentication when refresh_token is invalid #367

svencorell opened this issue May 19, 2021 · 3 comments

Comments

@svencorell
Copy link

svencorell commented May 19, 2021

As long as the refresh token is valid, everything works as expected. But as as soon as the refresh_token gets invalid, following error occurs when calling .authorize()

``[Trace]` OAuth2: RESPONSE
HTTP/1.1 400 bad request
Connection: close
Referrer-Policy: no-referrer
Content-Length: 67
Content-Type: application/json
X-XSS-Protection: 1; mode=block
Pragma: no-cache
Cache-Control: no-store
X-Frame-Options: SAMEORIGIN
Date: Wed, 19 May 2021 08:23:57 GMT
X-Content-Type-Options: nosniff
Strict-Transport-Security: max-age=31536000; includeSubDomains

{"error":"invalid_grant","error_description":"Token is not active"}

[Debug] OAuth2: Error refreshing access token: Token is not active
[Debug] OAuth2: Error refreshing token: Token is not active
[Debug] OAuth2: Token is not active ``

I would expect a redirect to the normal Auth-Page and the user has to sign in again.

@matthewtintabee
Copy link

I had this problem a few days ago and think I found the issue. 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.

It works in the testing that I have so far done. I haven't done enough testing or have enough familiarity with the code to be confident at making a formal fix proposal.

@danipralea
Copy link

I have the same issue.
@matthewtintabee besides not having a formal fix, can you provide an informal one (aka hack. or temporary fix)

@matthewtintabee
Copy link

@danipralea, here is my slightly modified version of doRefreshToken in OAuth2/Sources/Flows/OAuth2.swift. The logic is tweaked in the error case to modify when the existing refresh token is cleared and if necessary prevent re-rasising the more generic exception. I haven't run all the formal test cases but this works in the specific error case and in all normal cases for my app. I note that on master now the assignment to data and json try blocks are moved above the error checking but I don't believe that this makes any difference to this case.

`open func doRefreshToken(params: OAuth2StringDict? = nil, callback: @escaping ((OAuth2JSON?, OAuth2Error?) -> Void)) {
do {
let post = try tokenRequestForTokenRefresh(params: params).asURLRequest(for: self)
logger?.debug("OAuth2", msg: "Using refresh token to receive access token from (post.url?.description ?? "nil")")

		perform(request: post) { response in
			do {
				// Modified from the underlying project to prevent losing the refresh token when the server is not accessible
				// Our server generates a 500 if the original access token does not exist for some reason
				if response.response.statusCode >= 400 {
					if (![408, 429].contains(response.response.statusCode) && response.response.statusCode <= 500) || response.response.statusCode == 511 {
						self.clientConfig.refreshToken = nil
					}
					let error = OAuth2Error.generic("Failed with status \(response.response.statusCode)")
					callback(nil, error.asOAuth2Error)
					return
					//throw OAuth2Error.generic("Failed with status \(response.response.statusCode)")
				}
				let data = try response.responseData()
				let json = try self.parseRefreshTokenResponseData(data)
				self.logger?.debug("OAuth2", msg: "Did use refresh token for access token [\(nil != self.clientConfig.accessToken)]")
				if (self.useKeychain) {
					self.storeTokensToKeychain()
				}
				callback(json, nil)
			}
			catch let error {
				// Modified from the underlying project to remove the refresh token in this error case as well as the generic one
				self.clientConfig.refreshToken = nil
				self.logger?.debug("OAuth2", msg: "Error refreshing access token: \(error)")
				callback(nil, error.asOAuth2Error)
			}
		}
	}
	catch let error {
		callback(nil, error.asOAuth2Error)
	}
}

`

josephquigley added a commit to josephquigley/OAuth2 that referenced this issue Oct 4, 2023
josephquigley added a commit to josephquigley/OAuth2 that referenced this issue Oct 4, 2023
josephquigley added a commit to josephquigley/OAuth2 that referenced this issue Oct 4, 2023
josephquigley added a commit to josephquigley/OAuth2 that referenced this issue Oct 4, 2023
josephquigley added a commit to josephquigley/OAuth2 that referenced this issue Oct 4, 2023
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

No branches or pull requests

3 participants