Do not remove refresh_token on server errors #281

catalinaturlea · 2018-09-12T09:49:30Z

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.

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.

p2 · 2018-09-12T16:30:15Z

Sources/Flows/OAuth2.swift

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


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

balland · 2018-09-13T15:22:42Z

Sources/Flows/OAuth2.swift

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


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?

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?

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

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 · 2018-09-14T07:56:48Z

This should fix #238

balland · 2018-09-14T12:43:42Z

Sources/Flows/OAuth2.swift

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


Should we not return serverError for any 5xx status code?

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.

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

Also, don't forget to add your name to CONTRIBUTORS, see CONTRIBUTING.

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.

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

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

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

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?

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.

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

p2 · 2018-10-04T05:26:33Z

Sources/Flows/OAuth2.swift

+					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:


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 · 2020-03-14T19:35:59Z

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

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 · 2024-02-08T09:36:22Z

Agree @agg23 , this should be revisited.

Catalina Turlea added 2 commits September 12, 2018 11:48

Do not remove refresh_token on server errors

5609e3a

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.

Updated the comment to reflect the changes

6713930

p2 reviewed Sep 12, 2018

View reviewed changes

balland reviewed Sep 13, 2018

View reviewed changes

Added specific client error and removed setting the token to nil

abb6229

catalinaturlea force-pushed the develop branch from 69a2ca5 to abb6229 Compare September 14, 2018 07:27

balland reviewed Sep 14, 2018

View reviewed changes

Added description for the clientError

b231246

catalinaturlea force-pushed the develop branch 4 times, most recently from 2ec31e0 to 8c8bc87 Compare September 17, 2018 19:43

Catalina Turlea added 3 commits September 18, 2018 09:55

Added ServerErrorWithCode and updated comments

9109c6c

Updated contributors list

5cc44a2

Parse to json only in the case of a successful response

d664205

catalinaturlea force-pushed the develop branch from 8c8bc87 to d664205 Compare September 18, 2018 07:56

Removed client error and added json parsing for 4xx errors

7fe2e96

p2 reviewed Oct 4, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not remove refresh_token on server errors #281

Do not remove refresh_token on server errors #281

catalinaturlea commented Sep 12, 2018

p2 Sep 12, 2018

balland Sep 13, 2018

p2 Sep 14, 2018

catalinaturlea Sep 14, 2018

balland Sep 14, 2018

balland commented Sep 14, 2018

balland Sep 14, 2018

catalinaturlea Sep 14, 2018

p2 Sep 14, 2018

p2 Sep 14, 2018 •

edited

catalinaturlea Sep 17, 2018

catalinaturlea Sep 18, 2018

p2 Sep 18, 2018

catalinaturlea Sep 19, 2018

p2 Sep 19, 2018

catalinaturlea Oct 2, 2018 •

edited

p2 Oct 4, 2018

agg23 commented Mar 14, 2020 •

edited

ossus-lib commented Feb 8, 2024

Do not remove refresh_token on server errors #281

Are you sure you want to change the base?

Do not remove refresh_token on server errors #281

Conversation

catalinaturlea commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balland commented Sep 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p2 Sep 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catalinaturlea Oct 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agg23 commented Mar 14, 2020 • edited

ossus-lib commented Feb 8, 2024

p2 Sep 14, 2018 •

edited

catalinaturlea Oct 2, 2018 •

edited

agg23 commented Mar 14, 2020 •

edited