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

Implement OIDC Session Management #322

Merged
merged 4 commits into from
Jun 29, 2024

Conversation

GREsau
Copy link
Contributor

@GREsau GREsau commented Jun 10, 2024

This PR adds an implementation of OpenID Connect Session Management 1.0 to the Contruum sample. This works by issuing a cookie containing a random session ID on login, and using that to calculate a suitable session_state value (based on the example in the spec) in the OIDC authorize response. It also adds a connect/checksession page and sets it in the discovery document's check_session_iframe property.

Because this uses cookies, it requires either:

  • The OP and RP to be on the same eTLD+1 so that they may share cookies (as happens to be true for my specific use-case)
  • The client to allow third-party cookies

This could theoretically be extended to use the Storage Access API to work fully in third-party contexts as long as the end-user approves access, although I have no plans to explore this avenue further because it's not required for my use-case.

With this change, I've verified that we can successfully run the OpenID Connect Core: Session Management Certification Profile Authorization server test profile in the OIDC certification suite, (which is why I put it in Contruum)

Misc. changes in this PR:

  • Enable hybrid flow - this isn't required, but I thought it was strange that the hard-coded clients have the rty: permissions for hybrid flows without them being enabled on the server
  • Add an OIDC logout endpoint, which is required for the Session Management certification tests. I put this endpoint at connect/checksession to avoid confusing it with the existing connect/signout endpoint which has nothing to do with OIDC and is purely concerned with signing the user out of their cookie auth

@kevinchalet
Copy link
Member

kevinchalet commented Jun 10, 2024

Hey,

Thanks for your PR!

The OP and RP to be on the same eTLD+1 so that they may share cookies (as happens to be true for my specific use-case)

This limitation is the reason why I've always refused to support that specification (and honestly, even when the third-party cookies ban was not a thing yet, it felt very clunky 😄). I really hope we'll see a better approach for that (and ideally, something that works across all types of apps, not just browser-based apps).

Implementing this clunky spec is not really something I want to encourage so I'm not sure it's a good idea to update the Contruum sample to support it. But since you seem to have a dedicated repo with the same tweaked sample, a good compromise would be to just update the README to add a link pointing to your repo 😃

The other changes are fine, please keep them.

@GREsau
Copy link
Contributor Author

GREsau commented Jun 10, 2024

I thought that might be the case, which is why I created the dedicated repo too 😄

I've reverted the session management commit, since the first commit contained all of the other changes

@kevinchalet
Copy link
Member

I thought that might be the case, which is why I created the dedicated repo too 😄

Woops, sorry for the late reply. Could you please update the README to add a link to your repo in the External samples section?

@GREsau
Copy link
Contributor Author

GREsau commented Jun 28, 2024

Done

@kevinchalet kevinchalet merged commit d46ccb9 into openiddict:dev Jun 29, 2024
3 checks passed
@kevinchalet
Copy link
Member

Merged, thanks for your contribution! ❤️

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