KeycloakRole equals only with itself

[Valeran86]

I use Keycloak Spring Adapter (KSA) to secure existing application. Today I realized that some functions didn't work anymore because of security checking like this:

GrantedAuthority adminRole = new MySpecialGrantedAuthority( "superadmin" );
for ( GrantedAuthority role : userRoles ) {
        if ( role.equals( adminRole ) ) {
          return true;
        }
      }

In this example, when I use KSA authorization fails.
I believe, that more preferable in KeycloakRole use this implementation of equals method.

[pedroigor]

@Valeran86, could you elaborate more on why it stop working ? Is it caused by upgrading to a specific spring version?

Or are you just trying to compare KeycloakRole instances with any GrantedAuthority instance?

I'm afraid that by changing that logic you may break existing use cases. Can't you just inherit your custom MySpecialGrantedAuthority from KeycloakRole ?

[Valeran86]

@pedroigor, thanks for you reply. Yes, I'm trying to compare KeycloakRole instance with MySpecialGrantedAuthority.

Can't you just inherit your custom MySpecialGrantedAuthority from KeycloakRole?
No, I'm afraid not, because in that case I need to change many parts of code in tests and production.

I'm afraid that by changing that logic you may break existing use cases.
Why do you think so? All tests in CI passed.

[pedroigor]

Thanks, @Valeran86.

Although all tests are passing, our tests coverage for spring adapter is not the ideal. We really lack integration tests and we always have to think twice when doing some specific changes.

Your changes look good for me but I'm not sure if people is relying on the current implementation of equals. I'll send an e-mail to both keycloak-user and keycloak-dev mailing lists about this. Depending on the feedback we have we are good to go.

[pedroigor]

@Valeran86, just sent an e-mail to both user and dev mailing lists. If nobody complains I think we are good to go.

The reason that makes me comfortable about this change is that the granted authority is really about a string representing some grant. In this case a role. So it should be fine to rely on both the supertype and getAuthority checks.

[pedroigor]

@Valeran86, could you please open a new PR referencing a JIRA in both title and commit message ?

[Valeran86]

@pedroigor, sorry for late answer. Am I still need open new PR?

[pedroigor]

@Valeran86, we did not get any objection for this change and I think it makes sense. If you still want this change, please open a new PR. But this time, please create a JIRA and reference it in both commit message and PR title.

[Valeran86]

@pedroigor, hello. I added JIRA task and opened new PR - #6173.