Don't throw exception on status code 200, 201 and 204

[dva-vladik]

I don't know how to work around this exception except for changing the source file.

Teamleader may also return 201 and 204. It doesn't mean it's a bad response.
https://developer.teamleader.eu/#/introduction/general-principles/status-codes

[mark-gerarts]

Thanks for the PR! Are you actually getting errors in your application? It seems strange to me that the OAuth endpoint returns something else than a 200 in the case of a successful authentication.

That being said, anything 2xx is a success, so we could even take it a step further and consider everything < 300 as successful?

[dva-vladik]

Thank you for your fast response. I apologize that I didn't return the courtesy.

Yes, the authentication does always return 200. But, the thing is, is that I'm using league/oauth2-client to also make API requests. League even provides an example where they make a request to some fictitious resource. https://oauth2-client.thephpleague.com/usage/.

So I thought that it's meant to be used in that way. I believed that it'd be easier to just use the working out of the box request/response system league provides, instead of curl.

---

Why does checkResponse give problems if it's only used to authenticate?

If you want some resource; first, you make a request with getAuthenticatedRequest.
Then you have to use getParsedResponse to unpack the league response.

If we look inside the AbstractServer, you can see checkResponse is being called every time you parse some response. https://github.com/thephpleague/oauth2-client/blob/master/src/Provider/AbstractProvider.php#L618
(And Teamleader class is extending AbstractServer)

So, I conclude that checkResponse is not only used to authenticate.

Some servers return other than 200 on API requests. So there are only 2 solutions to this problem.
(For example: when you create webooks, Teamleader returns 204. https://developer.teamleader.eu/#/reference/other/webhooks/webhooks.register)

1. Do not override checkResponse
2. If you do override checkResponse, make sure to handle all possible valid responses

---

It's also possible that I understand it all wrong. Let me know if you think that I do.

I hope this provides an adequate elaboration of the problem.

[mark-gerarts]

I believe your understanding is correct. We haven't encountered this issue ourselves because we do not use getParsedResponse directly. We get the authenticated request from the provider, and then send it using a separate HTTP client (as you can see here).

Your way of working with the provider is completely valid as well, so yes, we should absolutely check for successful status codes other than 200.

I think only checking for 200, 201, and 204 might not be enough though. The API docs use the words "Common used status codes", which implies they might use other ones as well. It'll probably be safer to treat all 2xx codes as successful - basically the way Symfony does it. What do you think?

If you adjust your PR to do a similar success check I'll be more than happy to merge it!

[dva-vladik]

I was about to push something like this:
if ($response->getStatusCode() < 200 || $response->getStatusCode() >= 300) {...

But before I committed I wanted to see what other providers did (which both of us should have done way earlier).
I think we missed the concept of this abstract method entirely.

Examples of other prominent providers:
https://github.com/thephpleague/oauth2-facebook/blob/master/src/Provider/Facebook.php#L155
https://github.com/thephpleague/oauth2-google/blob/master/src/Provider/Google.php#L102
https://github.com/thephpleague/oauth2-instagram/blob/master/src/Provider/Instagram.php#L152

I believe it's meant to check if the response is erroneous based protocols in the body, not just the status code from the headers.

So I guess more research is needed since it's not as simple as: "is it status 200 or not".

I can take a look at it if you still want me to.

[mark-gerarts]

Good point. I think we went with the 200 check because the Teamleader API docs do not explicitly mention what is returned in the event of an error in the authentication flow.

The providers you linked all check for the error property, which lies in line with the RFC docs. The RFC also mentions this error response has a 400 status code, so checking for 2xx is not wrong per se. But, when using the provider as you (and many others) do, you probably don't want an IdentityProviderException on non-auth related error responses. So my guess is we will probably have to check for the error property as well.

Let us know if you find something else, but I believe it's safe to take this approach. And thank you for thorough research!