Allow custom OAuth2ErrorHttpMessageConverter with OAuth2ErrorResponse…

[khamlaoui]

Expected Behavior

When we get errors from IDP (in our case its Okta), we should get a correctly formed OAuth2Error with the error code and description.

Current Behavior

The root cause error is lost, because the error conversion is not compliant with OKTA error response.
Okta error example:
{ "errorCode": "invalid_client", "errorSummary": "Invalid value for 'client_id' parameter.", "errorLink": "invalid_client", "errorId": "oaeUy44O3E3Q6mR4ZIcy50gKg", "errorCauses": [] }
The OAuth2ErrorConverter try to map mandatory parameter with name error however with Okta this attribute is named with errorCode

At the end, we lost the Okta error, and we get IllegalArgumentException raised by the constructor of OAuth2Error

A quick fix, could be allowing setter for OAuth2ErrorHttpMessageConverter in OAuth2ErrorResponseErrorHandler or add a constructor with OAuth2ErrorHttpMessageConverter .

Closes gh-10425

[sjohnr]

Thanks for the PR @khamlaoui. This is a great start! I have added some feedback inline, see below.

[sjohnr]

Thanks for the updates, @khamlaoui! Just a few more updates noted below. When finished, please squash your changes into a single commit.

[khamlaoui]

@sjohnr changes are pushed and commits squashed

[khamlaoui]

@sjohnr sorry i was missing to rethrow the checked. IOException.

[khamlaoui]

@sjohnr i fixed the issue with test checkstyle

[rwinch]

While probably not the most popular opinion, I think we should wait until 5.7. The change is minor, but we really need to do our best to stick to semantic versioning unless there is an extremely good reason to do otherwise. The workaround for this is that users can copy a single class and add the setter and then delete the copy when 5.7 becomes available in 6 months.

[khamlaoui]

@rwinch what is the risk associated with integrating this small change in 5.6?

[khamlaoui]

@sjohnr do i have to change the since to 5.7 ?

[sjohnr]

@sjohnr do i have to change the since to 5.7 ?

If you could, that would be great. Actually, looks like you have already, thanks!

I think Rob's point is that we should stick to our stated policy on semantic versioning, not so much that there's a specific risk. Thanks for the contribution @khamlaoui! Appreciate the fast turnaround even so.

[khamlaoui]

@sjohnr thanks for feedbacks !
I will try to contribute if possible to add a feature for private_key_jwt( load public and private keys from keystore)