Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ClientRegistration.clientSettings.requireProofKey to Enable PKCE #16386

Closed
wants to merge 1 commit into from

Conversation

kse-music
Copy link
Contributor

Closes gh-16382

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 9, 2025
@kse-music kse-music force-pushed the gh-16382 branch 2 times, most recently from 712bae9 to 921cbd7 Compare January 10, 2025 03:46
@rwinch
Copy link
Member

rwinch commented Jan 13, 2025

Thanks for the PR @kse-music I'm putting this on pause as the team discusses the underlying ticket.

PS: Sorry for not flagging the ticket for team discussion until after you had already created the PR.

@rwinch
Copy link
Member

rwinch commented Jan 16, 2025

@kse-music Thanks again for the PR!

After speaking with @jgrandja offline, he suggested the property ClientRegistration.clientSettings.requireProofKey=(true|false) to align with Autorization Server's RegisteredClient.[clientSettings](https://github.com/spring-projects/spring-authorization-server/blob/1.4.1/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/client/RegisteredClient.java#L73).requireProofKey. In this case, we can introduce the ClientSettings in the same package as ClientRegistration and it does NOT need to be backed by a Map or extend AbstractSettings. If needed, we can later introduce AbstractSettings and back it by a Map at that point.

Also, I would expect that this PR enable PKCE for that RegisteredClient if the new property is set to true.

@kse-music kse-music changed the title Add ClientRegistration codeChallengeMethod to Enable PKCE Add ClientRegistration.clientSettings.requireProofKey to Enable PKCE Jan 17, 2025
@rwinch rwinch self-assigned this Jan 17, 2025
@rwinch rwinch added type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 17, 2025
@rwinch rwinch added this to the 6.5.0-M1 milestone Jan 17, 2025
rwinch added a commit that referenced this pull request Jan 17, 2025
@rwinch rwinch closed this in 4fc99aa Jan 17, 2025
@rwinch
Copy link
Member

rwinch commented Jan 17, 2025

Thanks for the Pull Request! This is now merged into main via the merge commit 4fc99aa 😄

I included the original commit as 8d3e084 that was rebased along with additional polish:

  • 85d7cc1 Document requireProofKey
  • 004f386 Move ClientSettings to ClientRegistration
  • 4c53356 Ensure missing ClientRegistration.clientSettings JSON node works
  • f9498d3 PKCE cannot be true and AuthorizationGrantType != AUTHORIZATION_CODE
  • ab629cc Add AuthorizationGrantType.toString()
  • b0a4dcb ClientSettings equals, hashCode, toString
  • 2665a92 Ensure that ClientSettings cannot be null
  • 0ed7b18 DefaultServerOAuth2AuthorizationRequestResolver requireProofKey support
  • 8d3e084 Add ClientRegistration.clientSettings.requireProofKey to Enable PKCE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Add ClientRegistration.clientSettings.requireProofKey to Enable PKCE
3 participants