Improve error messages in OidcIdTokenValidator

[raphaelDL]

This commit ensures that error messages contain more specific
information regarding the reported error.

Fixes: gh-6323

[jgrandja]

Thanks for the PR @raphaelDL ! Please see my comments inline.

[jgrandja]

Having the errors as a member is not thread-safe. There may be 2 users authenticating at the same time that use the same client (client-id) and therefore would use the same JwtDecoder and this OidcIdTokenValidator for validation. See below comment for proposed change.

[raphaelDL]

Oh I see, I missed that. I moved this inside the method

[jgrandja]

As per previous comment, instead of errors as a member, let's create a method variable Map<String, Object> invalidClaims. For all validation checks to follow, just populate the claim name and value.

[raphaelDL]

thank you Joe, just to be sure, in the first step, when verifying that all required claims are present, this map would be populated with, for example: invalidClaims.put(JwtClaimNames.SUB, subject) with the value being always null.

[jgrandja]

Yes, that is correct. So when OAuth2Error.description is built it would display as follows: sub (null), ...other invalid claims

[jgrandja]

Let's change the method signature to invalidIdToken(Map<String, Object> invalidClaims). This method would return one OAuth2Error with a generic message that lists the invalidClaims in comma-delimited format.

[raphaelDL]

Then, here I use the keys and join them with a comma to create an OAuth2Error like "Required claims were not found: issuer, iat".
What about the rest of the validations that are performed once we are sure that the required claims are present? Should we keep the message stating why the validation failed?

I submitted new changes... let me know what you think, as always I'm happy to make changes if needed.

Thank you

[jgrandja]

Let's keep the message generic.

For example:

If one of the required claims is not present than fail-fast and return this message:

The ID Token contains invalid claims: sub (null), aud (null)

NOTE: Check all required claims before failing fast

If all required claims are present than proceed with the rest of the validation and if there is a failure than display the same generic message:

The ID Token contains invalid claims: aud (invalid-aud), azp (invalid-azp)

[raphaelDL]

Ok Joe, I submitted new changes

[jgrandja]

Let's remove the methods requiredClaimError and invalidIdTokenError and build the OAuth2Error in this method. Please ensure the message text is same as mentioned in this comment.

[jgrandja]

Also, can you update the tests to assert on the OAuth2Error.description. And add a new that has at least 2 invalid claims present. Thanks!

[raphaelDL]

Ok, I've added the changes :)

[jgrandja]

Thanks for the PR @raphaelDL ! This is now in master.

I added a polish commit that fixed some extra spacing and formatting.
I also removed the test validateIdTokenWhenNoClaimsThenHasErrors since this is already being tested in JwtTests.constructorWhenClaimsIsEmptyThenThrowIllegalArgumentException.

Thank you so much for all your efforts as of late in contributing to Spring Security. Keep up the great work and we look forward to your next contribution! :)