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.

Sign up for GitHub

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

Do not remove refresh_token on server errors #281

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5609e3a
Do not remove refresh_token on server errors
Sep 12, 2018
6713930
Updated the comment to reflect the changes
Sep 12, 2018
abb6229
Added specific client error and removed setting the token to nil
Sep 14, 2018
b231246
Added description for the clientError
Sep 14, 2018
9109c6c
Added ServerErrorWithCode and updated comments
Sep 18, 2018
5cc44a2
Updated contributors list
Sep 17, 2018
d664205
Parse to json only in the case of a successful response
Sep 17, 2018
7fe2e96
Removed client error and added json parsing for 4xx errors
Oct 2, 2018
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
7 changes: 6 additions & 1 deletion Sources/Flows/OAuth2.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,9 +366,14 @@ open class OAuth2: OAuth2Base {
do {
let data = try response.responseData()
let json = try self.parseRefreshTokenResponseData(data)
if response.response.statusCode >= 400 {
switch response.response.statusCode {
case 400..<500:
Copy link

Choose a reason for hiding this comment

The 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?

Copy link
Owner

Choose a reason for hiding this comment

The 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 OAuth2Error.clientError(Int), which takes the status code, and change OAuth2Error.serverError to OAuth2Error.serverError(Int) and raise either of these two if status >= 400 (and never delete the token). @catalinaturlea , what do you think?

Copy link
Author

Choose a reason for hiding this comment

The 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 clientError(int) for all the cases which are not covered by the other errors. I ll update the PR and you can let me know what you think

Copy link

Choose a reason for hiding this comment

The 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.

self.clientConfig.refreshToken = nil
throw OAuth2Error.generic("Failed with status \(response.response.statusCode)")
case 500...599:
throw OAuth2Error.generic("Failed with status \(response.response.statusCode)")
default:
break
}
self.logger?.debug("OAuth2", msg: "Did use refresh token for access token [\(nil != self.clientConfig.accessToken)]")
callback(json, nil)
Expand Down