Fail with oauth errors instead of masking them

[thefloweringash]

Fixes #192 by making the code parameter optional, and allows the super method to raise errors.

I defer to the authors to assess the security implications.

[kevingriffin]

Any chance of getting this merged?

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[thefloweringash]

The logic in master still requires all responses, including error responses, to include a code, and throws and fails if the response doesn't. I believe this PR is still relevant and not stale.

[simi]

@thefloweringash hello, thanks for check and your activity. The reason of marking this as a stale is to bring back some activity.

This introduces backwards incompatible change AFAIK. I'll test and see what we can do about that other than bumping major version. Some kind of deprecation cycle will be nice. Any ideas?

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[chrisandreae]

To me, this seems like fixing a bug rather than changing an intended spec. As such, I don't feel that a major version change should be necessary.

I think there's an expectation coming from Omniauth that strategies will pass up an error returned from the provider's callback. That omniauth-facebook didn't do this (and instead crashed out with :no_authorization_code) is deviating from that spec. Fixing that isn't changing the expected behaviour between omniauth and its consumers, since the expected spec was already to do the updated behaviour.

This is an externally observable behavior change, but that is true of any bug fix. I don’t think it’s reasonable to say that every bug fix is a backward-incompatible change because some inputs are no longer mishandled. (At some level, every change is an XKCD spacebar heater!)

[kevingriffin]

Was any testing done on this to work out if it'd be possible to get it in?

[molfar]

Why is this PR is not merged?

[simi]

It is not clear to me how to release (per #309 (comment)).

Would be major bump enough?

[kevingriffin]

As @chrisandreae commented, although it is introducing a change, the change is to remove a crash. Perhaps someone is relying on the crash, if it doesn’t seem like it’s necessarily a major bump to me.