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

Do not remove refresh_token on server errors #281

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CONTRIBUTORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ Contributors

Contributors to the codebase, in reverse chronological order:

- Catalina Turlea @catalinaturlea
- Maxime Le Moine, @MaximeLM
- Seb Skuse, @sebskuse
- David Hardiman, @dhardiman
Expand Down
4 changes: 4 additions & 0 deletions Sources/Base/OAuth2Error.swift
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ public enum OAuth2Error: Error, CustomStringConvertible, Equatable {
/// There is no delegate associated with the password grant flow instance.
case noPasswordGrantDelegate

/// Generic server error 5xx
case serverErrorWithStatus(Int)

// MARK: - Request errors

Expand Down Expand Up @@ -273,6 +275,8 @@ public enum OAuth2Error: Error, CustomStringConvertible, Equatable {
return message ?? "The requested scope is invalid, unknown, or malformed."
case .serverError:
return "The authorization server encountered an unexpected condition that prevented it from fulfilling the request."
case .serverErrorWithStatus(let statusCode):
return "The authorization server encountered an unexpected condition, returning \(statusCode)"
case .temporarilyUnavailable(let message):
return message ?? "The authorization server is currently unable to handle the request due to a temporary overloading or maintenance of the server."
case .invalidGrant(let message):
Expand Down
19 changes: 12 additions & 7 deletions Sources/Flows/OAuth2.swift
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ open class OAuth2: OAuth2Base {
/**
If there is a refresh token, use it to receive a fresh access token.

If the request returns an error, the refresh token is thrown away.
Does not remove the refresh_token in case of a failure. For server errors (5xx), the callback provides the status code in .serverErrorWithStatus(Int)

- parameter params: Optional key/value pairs to pass during token refresh
- parameter callback: The callback to call after the refresh token exchange has finished
Expand All @@ -364,14 +364,19 @@ open class OAuth2: OAuth2Base {

perform(request: post) { response in
do {
let data = try response.responseData()
let json = try self.parseRefreshTokenResponseData(data)
if response.response.statusCode >= 400 {
self.clientConfig.refreshToken = nil
let statusCode = response.response.statusCode
switch statusCode {
case 500...599:
throw OAuth2Error.serverErrorWithStatus(statusCode)
// 2xx and 4xx responses should contain valid json data which can be parsed
case 200..<300, 400..<500:
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not entirely sure whether we could see a 3xx in a response. I don't think we'd want to throw the generic error on 300 responses, should this just be 200..<500 or ever the default?

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)]")
callback(json, nil)
default:
throw OAuth2Error.generic("Failed with status \(response.response.statusCode)")
}
self.logger?.debug("OAuth2", msg: "Did use refresh token for access token [\(nil != self.clientConfig.accessToken)]")
callback(json, nil)
}
catch let error {
self.logger?.debug("OAuth2", msg: "Error refreshing access token: \(error)")
Expand Down