Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Sep 7, 2021

Avoid client secret double encoding when updating a registered client

Closes gh-389

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 7, 2021
@ghost ghost force-pushed the gh-389 branch 4 times, most recently from e99b115 to 0b49366 Compare September 8, 2021 10:03
@ghost
Copy link
Author

ghost commented Sep 17, 2021

hi @jgrandja. When 10273 will be released in the spring-security project the implementation will no longer be valid as the prefix and suffix can be changed => checking for { } to determine if it's already encoded is not enough, we must use the same prefix/suffix that is used in DelegatingPasswordEncoder

@jgrandja
Copy link
Collaborator

Hi @ovidiupopa91. I realized shortly after I proposed the fix that it wasn't a robust solution and that we needed an alternative solution. And yes, 10273 just validated that.

I'm catching up with things this week after returning from PTO so please give me until next week to propose a more robust solution. I'll also review your other PR later next week. Thanks.

@jgrandja jgrandja self-assigned this Sep 22, 2021
@jgrandja jgrandja added status: duplicate A duplicate of another issue type: enhancement A general enhancement type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Sep 22, 2021
@jgrandja jgrandja added this to the 0.2.1 milestone Sep 22, 2021
@jgrandja
Copy link
Collaborator

Hi @ovidiupopa91. This fix is a bit tricky. However, there is one easy fix that can be applied in order to avoid the double encoding.

We currently don't allow updates for id, client_id and client_id_issued_at. We could also disable updates for client_secret and client_secret_expires_at. This would at least resolve the double encoding, which is the main issue we need to address.

I realize this limits the update capabilities, but applications can still customize based on their requirements. We might have to revisit this at a later point but I would say let's go with disabling updates for client_secret and client_secret_expires_at.

@ghost
Copy link
Author

ghost commented Sep 23, 2021

Hi @jgrandja . Ok, I will rollback to the initial state and I will remove client_secret and client_secret_expires_at from the update statement.

This might have to be revisited at a later point, but to check if a value is encoded or not is quite tricky. The decision was to remove client_secret and client_secret_expires_at from the update statement

Closes spring-projectsgh-389
@jgrandja
Copy link
Collaborator

Thanks for the updates @ovidiupopa91 ! This is now in main.
FYI, I added minor polish to the test.
Thanks!

@jgrandja jgrandja closed this Sep 23, 2021
@ghost ghost deleted the gh-389 branch September 23, 2021 18:07
@Shabin
Copy link

Shabin commented Feb 7, 2023

Hi, is there any plan for allowing the update of client_secret_expires_at? I have a requirement to extend the client validity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client secret double encoding issue when updating an existing registered client
4 participants