JdbcRegisteredClientRepository hashes client secret on save

[ghost]

Closes gh-378

[ghost]

still in draft as I will rebase it on top of main after the changes done in gh-356 are merged.

[jgrandja]

Thanks for the PR @ovidiupopa91 ! This is now in main.

[jgrandja]

@ovidiupopa91 I think there may be a double-encoding bug with this update. I decided to merge anyway but can you confirm if there is a bug or not.

In the scenario where an existing client is updated via save(), the client-secret might be double encoded depending on how the backing PasswordEncoder is implemented. This would result in a failed client authentication on a subsequent grant flow. Can you please confirm if this is an issue?

FYI, I'm on PTO all of next week and returning Aug 9 so I'll follow up with you then.

Thanks again for all your help !

[ghost]

Hi @jgrandja . I was thinking about this as well. I will let you know when I have a definitive answer.
Enjoy your time off 😀

[ghost]

Hi @jgrandja . I can confirm that bug is reproducible. I wrote a test

RegisteredClient originalRegisteredClient = TestRegisteredClients.registeredClient().build();
this.registeredClientRepository.save(originalRegisteredClient);
RegisteredClient registeredClient = this.registeredClientRepository.findById(originalRegisteredClient.getId());
assertThat(delegatingPasswordEncoder.matches(originalRegisteredClient.getClientSecret(), registeredClient.getClientSecret())).isTrue();
this.registeredClientRepository.save(registeredClient);
registeredClient = this.registeredClientRepository.findById(originalRegisteredClient.getId());
assertThat(delegatingPasswordEncoder.matches(originalRegisteredClient.getClientSecret(), registeredClient.getClientSecret())).isTrue();

The first assert is passing but the second one is not -> because of the double encoding issue.

The good news is that in the framework, the save method it's only called from OidcClientRegistrationAuthenticationProvider . It is also used in tests and the sample project.

In the OidcClientRegistrationAuthenticationProvider the code looks like this

RegisteredClient registeredClient = create(clientRegistrationAuthentication.getClientRegistration());
this.registeredClientRepository.save(registeredClient);

and the create method is all the time generating a new id -> we will never end up in this particular scenario until the update flow is implemented.

[jgrandja]

Thanks for confirming @ovidiupopa91.

Would you mind creating a new issue for this with the details?

We'll need to have this fixed before we implement gh-355.

[ghost]

welcome back @jgrandja . Issue created: gh-389

[jgrandja]

Thanks @ovidiupopa91 ! I assigned it to you but this can wait until 0.2.1.