thephpleague / oauth2-server

A spec compliant, secure by default PHP OAuth 2.0 Server
https://oauth2.thephpleague.com
MIT License
6.49k stars 1.12k forks source link

OpenID Connect implementation #1355

Closed shineability closed 11 months ago

shineability commented 1 year ago

I'm trying to add OpenID Connect functionality to Passport for a project I'm working on. I investigated some packages and was able to successfully have an ID token returned in the response.

What eventually drove me here was the (unexpected) need to add a nonce parameter to the authorization request and have it returned as a claim in the ID token as specified in the spec. Apparently I was not the only one having this issue: #1003, #1110, #962, #1316, ...

Both the packages and this merge request (#1316) seem to focus on the OpenID claims (profile, email, address, ...) and extending the BearerResponse with IdTokenResponse to return an ID token.

But none of them seem to provide a solution to add the OpenID Connect specific request parameters to the flow. Some of them (prompt, display, ui_locales , ...) are only relevant when authenticating the user, but other parameters such as nonce, max-age, acr_values, ... could have an effect on which claims need to be added to the ID token and need to be passed on all the way to where the ID token is generated. Also, redirect_uri should be required for OpenID.

I don't consider myself an expert and I'm still learning (and very often getting confused) about OAuth and OpenID Connect, but would you agree that this part is missing?

Without going into too much detail, I feel these changes seem to be necessary:

shineability commented 11 months ago

@Sephster You are probably busy and I realise it's not just a simple question to answer, but is there any value in my comment/remark above?

Sephster commented 11 months ago

Hi @shineability - at this stage there are no plans to get OpenID connect into this package. It would be quite a lot of work and there are packages that build upon this one to provide this functionality so at this time, I don't think we'd merge any such PRs.