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

Conversation

catalinaturlea
Copy link

This prevents the user to be logged out with .noRefreshToken when the next refresh token request is triggered in case the error was a server error and not a client error.

Catalina Turlea added 2 commits September 12, 2018 11:48
This prevents the user to be logged out with `.noRefreshToken` when the next refresh token request is triggered in case the error was a server error and not a client error.
@@ -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.
If the request returns an client error, the refresh token is thrown away.
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for also updating the comment – can you change to "a client error"? :)

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

@balland
Copy link

balland commented Sep 14, 2018

This should fix #238

if response.response.statusCode >= 400 {
self.clientConfig.refreshToken = nil
switch response.response.statusCode {
case 500:
Copy link

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?

Copy link
Author

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.

/// A 500 was thrown.
 case serverError

Since it is used in other places as well, it would require migration for all usages. That's why I left it like this.

Copy link
Owner

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 specific server_error OAuth2 error code. How about a new serverErrorWithStatus(Int) error that can be thrown here? If so we could adapt to clientErrorWithStatus(Int).

Copy link
Owner

@p2 p2 Sep 14, 2018

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.

Copy link
Author

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.

Copy link
Author

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 unlikely

Copy link
Owner

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 an error_description key, see here: https://tools.ietf.org/html/rfc6749#section-5.2

Copy link
Author

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 them

Copy link
Owner

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 throw invalidGrant.

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 new clientErrorWithStatus(Int) error. And do the same for 500 with serverErrorWithStatus(Int). Do you think that makes sense?

Copy link
Author

@catalinaturlea catalinaturlea Oct 2, 2018

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.

balland added a commit to balland/OAuth2 that referenced this pull request Sep 14, 2018
Otherwise this may cause any error or loss of connectivity leading to
refresh token deletion. There is a pending PR on the main repo
p2#281

Related to sensational/popscan-android#977
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?

@agg23
Copy link

agg23 commented Mar 14, 2020

Why was this PR never merged? Are there still unopened questions?

balland added a commit to balland/OAuth2 that referenced this pull request Sep 22, 2020
Otherwise this may cause any error or loss of connectivity leading to
refresh token deletion. There is a pending PR on the main repo
p2#281

Related to sensational/popscan-android#977
ajandrade pushed a commit to sensational/OAuth2 that referenced this pull request Dec 28, 2022
Otherwise this may cause any error or loss of connectivity leading to
refresh token deletion. There is a pending PR on the main repo
p2#281

Related to sensational/popscan-android#977
@ossus-lib
Copy link
Collaborator

Agree @agg23 , this should be revisited.

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

Successfully merging this pull request may close these issues.

5 participants