search

spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.55k stars 5.79k forks source link

Should OidcIdToken implement equals? #15156

Open MatthiasWinzeler opened 1 month ago

MatthiasWinzeler commented 1 month ago

Describe the bug I wondered if OidcIdToken should implement equals. While running some test, I realized that the claims on OidcIdToken are not compared when using assertEquals.

Interestingly, OAuth2AccessToken which follows the same pattern does implement it.

Expected behavior claims (and potentially other fields) are respected when comparing via equals. This might be considered a breaking change.

jzheaux commented 3 weeks ago

Hi, @MatthiasWinzeler. It may be valuable, though I'm not sure yet. Usually, we try and wait to add code until there is a clear reason. Is there something that you are trying to do in your production code that needs or is expecting OidcIdToken#equals to compare the claims?

If not, then I suggest we close this for now and reopen once the need arises.

This might be considered a breaking change.

That's correct. Unless it is tied to problematic behavior, we'd defer this change to Spring Security 7.

MatthiasWinzeler commented 3 weeks ago

@jzheaux For me, the benefit would mainly be in testing - so that assertEquals can be used. I was recently building an application based on Spring Authorization Server and tried to compare its OAuth2Authorization (which holds OidcIdToken next to many other fields). assertEquals for two OAuth2Authorizations was failing (since the ID token claims were not the same) but it was very hard to track down the difference. Every other field (for example the OAuthAccessToken was properly implementing equals, so I thought it was just forgotten on OidcIdToken.

That's my reason - I think there might be others. I am far away from state of the art Java best practices these days, but isn't it a code smell to not override equals when a subclass adds fields?

jzheaux commented 3 weeks ago

Thanks for the extra detail, @MatthiasWinzeler. Yes, I agree it is smelly in this case, so I've slated this for the 7.0 release.

baezzys commented 3 weeks ago

Hi @jzheaux. Can I try this issue?

jzheaux commented 3 weeks ago

Sure, @baezzys! The contribution is much appreciated. Remember that in this particular case (due to the breaks-passivity label), it won't be merged until Spring Security 7.