-
Notifications
You must be signed in to change notification settings - Fork 279
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
base: develop
Are you sure you want to change the base?
Changes from 3 commits
5609e3a
6713930
abb6229
b231246
9109c6c
5cc44a2
d664205
7fe2e96
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 client errors (400..<500), the callback will provide the status code in the .clientError(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 | ||
|
@@ -366,8 +366,12 @@ open class OAuth2: OAuth2Base { | |
do { | ||
let data = try response.responseData() | ||
let json = try self.parseRefreshTokenResponseData(data) | ||
if response.response.statusCode >= 400 { | ||
self.clientConfig.refreshToken = nil | ||
switch response.response.statusCode { | ||
case 500: | ||
throw OAuth2Error.serverError | ||
case 400..<500: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check still seems too strong. How about 408? They may happen just because of bad connectivity. I would rather never delete the refresh token here and let the callback deal with it. For this we would need a more detailed error with at least the response status code. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is a good point. Yes, it would probably be good to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with never deleting the token and leaving that to the specific implementation. What I suggest is adding the check for the server error since it is already implemented for 500s and adding a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to be on the safe side, @p2 could you confirm that this is ok not deleting the token here? I am just worried if nil refresh tokens are not used to check some state somewhere else in the library. |
||
throw OAuth2Error.clientError(response.response.statusCode) | ||
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)]") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not return serverError for any 5xx status code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment in the error class said it only handled 500.
Since it is used in other places as well, it would require migration for all usages. That's why I left it like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine to not delete the token here, the
didFail
method will be called with an error and the library user can then take action accordingly.Indeed,
serverError
is used for the specificserver_error
OAuth2 error code. How about a newserverErrorWithStatus(Int)
error that can be thrown here? If so we could adapt toclientErrorWithStatus(Int)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, don't forget to add your name to CONTRIBUTORS, see CONTRIBUTING.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into it a bit more and figured out that in case the response was not parsed for the JSON, it would return
.invalidGrant
error. I changed the logic a bit to only try and parse the response if the status code was 2xx. Otherwise, the other errors would not have been thrown. It makes sense to only try to parse the response if the request returned successfully.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my test, for a 400, the json parsing fails and returns
invalidGrant
. which means the new errors will never be thrown. In case the parsing fails and it caught by the catch block then the new errors make no sense - unless the backend returns a parsable json but a status code different than 200, which is very unlikelyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a correctly behaving server, any 400 response status is accompanied by valid JSON with an
error
key and optional anerror_description
key, see here: https://tools.ietf.org/html/rfc6749#section-5.2There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in this case, the parsing of the json would already throw the right error, like
.invalidGrant
. For the other errors, assuming they happen, like 500 for example, where there will be no json response, the response code will not be handled through the new code, but through the parsing, which will fail returning some of the predefined errors.Considering the 4xx errors are handled by the json parsing according to the standard, I would remove them and only leave the 5xx,
.serverErrorWithStatus
error case and unify the 4xx and 2xx cases in the switch, since the same code can handle both of themThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I misunderstood, I thought you said that any
400
will always throwinvalidGrant
.So in a correctly working server, a 400 will never even touch the
response.response.statusCode
switch. I forgot about that. It seems we should keep the 400 case in case the server returns gibberish and throw a newclientErrorWithStatus(Int)
error. And do the same for 500 withserverErrorWithStatus(Int)
. Do you think that makes sense?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spend quite some time thinking what would make most sense. I merged the cases where te json contains valuable information - 4xx and 2xx and left the serverError handling separate. I removed the
clientErrorWithStatus(Int)
because I do not believe it makes sense since it will never be returned, but always return the result of the json parsing.Let me know what you think.