Fixed ClientAuthenticationMethod inconsistent equals and hashCode

[jlaci]

In org.springframework.security.oauth2.core.ClientAuthenticationMethod the equals method was comparing the value using equalsIgnoreCase, however the hashCode was returning the original string's hashCode. This meant that if the value was only differing in the case, equals would return true, but the hashCode wouldn't match, causing all sorts of problems, such as in a Collection calling contains returning false. E.g.:

ClientAuthenticationMethod foo1 = new ClientAuthenticationMethod("foo")
ClientAuthenticationMethod foo2 = new ClientAuthenticationMethod("FOO")

foo1.equals(foo2);    // This returns true

List<ClientAuthenticationMethod> list = new ArrayList<>();
list.add(foo1);

list.contains(foo2);  // This returns false

I fixed the issue by calling an explicit toLowerCase() in both cases.

[jgrandja]

Thanks for the PR @jlaci. Please see review comment.

Also, please add a test.

[jgrandja]

Please remove toLowerCase() and use equals() for exact matching.

[jlaci]

In that case, I would also have to change the hashCode from return this.getValue().toLowerCase(Locale.ROOT).hashCode(); to return this.getValue().hashCode();.

But be aware, that this would also change the current behaviour of the equals() method, as it currently ignores the case for equality check. That would break any code using the current equals() on differently capitalised values. It is possible that some code depend on that functionality, while not being affected by the current bug, by not using hashCode, even implicitly (i.e. by calling contains in a collection).

I think that the original intention here, was to ignore the case for comparison, and the hashCode was simply buggy. Nevertheless if you say that case should matter, then I'll update both methods.

[jgrandja]

Thanks for pointing that out @jlaci, as you're right this could potentially break existing applications. Nevertheless, case does matter as the standard client authn methods should be compared using exact matching.

I'm thinking we might push this to main branch, which is targeted for 6.0 and would allow for this potentially breaking change. This "bug" has been around for a few years now and it hasn't caused any issues up until now. Would you be ok if we push this to 6.0 and not 5.7.x?

[jlaci]

Okay, I've updated the PR as requested. 6.0 is not a problem for me, I already have a workaround in place where I've encountered this bug, I just wanted to spare others from bashing their heads :) Please feel free to comment if you need anything else changed.

[jgrandja]

Thanks @jlaci ! Can you please add a test in ClientAuthenticationMethodTests that ensures exact comparison.

[jgrandja]

Thanks for the updates @jlaci and apologies for the delay. This is now in main.