Try to check for `error` responses even when the status code isn't HTTP 400

[bonifaido]

According to https://tools.ietf.org/html/rfc6749#section-5.2 an error response is returned mostly when the HTTP status code is 400, but in some cases for example in GitHub an error response can be raised even when the status code is 200. In this case internal.RetrieveToken() parses the output but with missing parameters, and continues, but the token struct is half-baked.

[bonifaido]

I have managed to reproduce this issue with an invalid (non-existing) clientid and clientsecret.

Have you reported the issue to GitHub?

[bonifaido]

No I haven't, where can I do that?

https://github.com/contact

[bonifaido]

Reported it, thanks!

[bonifaido]

I managed to get back a fair answer to my request, which I actually totally agree with:

Thanks for that example, Nándor. I agree with you -- the behavior you observed is definitely confusing, but expected for now. Our OAuth flow has behaved like this for a long time (at least several years) and was based on older versions of the OAuth specification. As a result of that you might see some inconsistencies or behavior that would probably be different if we re-implemented it today. However, changing that behavior today has a good chance of breaking thousands of applications that are relying on the existing behavior. That's not something we'd like to do, so the inconsistencies are here to stay until we provide a new major revision of the implementation.

Thanks for mentioning this and I hope this explains things. If this behavior is causing problems for you -- please let us know (we'd be interested to hear what kinds of problems those are, other than the initially confusing status code).

The Token contains the Raw JSON properties of the response body, which would mean you could pull out the error:

tkn, err := src.Token()
if err != nil {
    if tkn != nil && tkn.AccessToken == "" {
        // Broken Github error response
        reqError, ok := tkn.Raw["error"].(string)
        if ok {
            handle reqError ...
        }
    }
}

Is that an acceptable solution?

[bonifaido]

That is absolutely fine, thank you!