ronvanderheijden / openid-connect

Adds the OpenID Connect identity layer to the PHP League's OAuth2 Server. With Laravel Passport support.
MIT License
41 stars 11 forks source link

Adding support for a nonce #17

Closed moufmouf closed 3 months ago

moufmouf commented 8 months ago

According to the OpenID connect spec:

nonce String value used to associate a Client session with an ID Token, and to mitigate replay attacks. The value is passed through unmodified from the Authentication Request to the ID Token. If present in the ID Token, Clients MUST verify that the nonce Claim Value is equal to the value of the nonce parameter sent in the Authentication Request. If present in the Authentication Request, Authorization Servers MUST include a nonce Claim in the ID Token with the Claim Value being the nonce value sent in the Authentication Request. Authorization Servers SHOULD perform no other processing on nonce values used. The nonce value is a case-sensitive string.

Right now, if a client passes a "nounce", we don't give it back and the client fails. This is happening to me right now with the client from Matrix Synapse.

Here, I'm creating a new service (CurrentRequestService). With this new service, I can get the current PSR-7 request.

I extend the AuthCodeGrant and inject this service into the extended class. With this, I can:

Then, in the IdTokenResponse, I read the "code" (if it is present), extract the "nounce" and inject it in the ID token as a new claim.

The whole process is inspired by this comment: https://github.com/steverhoades/oauth2-openid-connect-server/issues/47#issuecomment-1228370632

With those changes, nounce is correctly handled and I've successfully tested a connection with the OpenID client from Matrix Synapse.

This PR is built on top of #16

moufmouf commented 3 months ago

Hey @ronvanderheijden ,

Following your last review (and the fact I broke CI), I realized that the CI checks were not visible when making a PR from a fork. I slightly changed the configuration of the CI to be sure that when a PR is open, Github actions are run (and are visible in the PR).

So long story short, the CI is now ok! Let's merge this!