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

feat: make client_id and client_secret optional for the token endpoint #116

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

lart2150
Copy link
Contributor

bshaffer/oauth2-server-php supports sending the client id and secret as http basic auth. With client id and secret set as required clients that send them via basic auth instead of in the request body will get an error when they try to exchange the token. This PR allows the client to send the client id and secret either in the request body or basic auth.

bshaffer/oauth2-server-php supports sending the client id and secret as http basic auth.  With client id and secret set as required clients that send them via basic auth instead of in the request body will get an error when they try to exchange the token.
@ashfame ashfame self-requested a review October 18, 2024 18:33
Copy link
Member

@ashfame ashfame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for highlighting the issue and making the PR!

@ashfame
Copy link
Member

ashfame commented Oct 18, 2024

For reference: OIDC specifies the following auth methods for client:

  • client_secret_basic
  • client_secret_post
  • client_secret_jwt
  • private_key_jwt

We had only used client_secret_post so far, which requires both client_id and client_secret but default auth method for client (if not chosen at registration time) is indeed client_secret_basic. So, it should be supported and for that, this PR is indeed correct to not do validation and let the bshaffer lib handle it.

For auth methods client_secret_jwt and private_key_jwt, two more fields become required: client_assertion and client_assertion_type. For the sake of defining all expected params, I will add them in a follow up PR.

@ashfame ashfame merged commit 5da4e1c into Automattic:main Oct 18, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants