p2 / OAuth2

OAuth2 framework for macOS and iOS, written in Swift.
Other
1.14k stars 276 forks source link

No Authentication when refresh_token is invalid #367

Open svencorell opened 3 years ago

svencorell commented 3 years ago

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 commented 3 years ago

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 commented 1 year ago

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

matthewtintabee commented 1 year ago

@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)
    }
}

`