Conversation
926e384 to
27f2c7b
Compare
|
@lullis Thank you for working on this. I've only given it a cursory review, but it looks like a great start. It'll probably be late next week before I can look at this closely. Ping me again if I don't manage to get you a review by next Friday. |
27f2c7b to
cd68405
Compare
ec3ccd9 to
3c3d1bf
Compare
3c3d1bf to
5b77eeb
Compare
|
@lullis I'm quite busy at the moment. I think that I should be able to have a look at this in the next ~3 months. Do please remind me if I forget it. |
|
@Qup42, just pinging to remind you about this PR. |
|
@lullis any guidance on testing procedures for this PR? |
|
@dopry I got a small app going to test these features at https://codeberg.org/raphael/oidc-client-testbed. I won't make any strong claims about its overall quality (I relied on Cursor to generate a lot of the boilerplate and the vue components), but I did check the actual functionality and it works well enough to check the session frame part. |
|
@lullis I was wondering more about test plans I could follow for testing manually. I can try to use your OIDC test bed, we also have the test/app/rp which we can use, although it may need to be updated. Not sure if I have session handling in that client. |
|
@dopry, I am reasonably confident that there is nothing about session handling in that client - at least not related to the session iframe. If started my vue app precisely because I couldn't find any demo application that exercised this functionality. Anyway, I was testing this with my app by running both the idp and the vue app. You can then login through the vue app, which will authenticate you in the idp. Once you login there will be a "session info" tab that will show you as logged in. You can open a console and you will see that the rp has an iframe which will be pinging the idp every few seconds. If you open another tab and log out directly on the IDP, the session will be finished and the rp will then show you as logged out. |
|
the RP uses my svelte-oidc package which is a proxy to https://github.com/DuendeArchive/identity-model-oidc-client-js more or less. I'm fairly certain https://github.com/DuendeArchive/identity-model-oidc-client-js/blob/dev/src/SessionMonitor.js handles session management in that library. Whether the tests/app/rp has session management enabled is another question. I've been meaning to update that to https://github.com/authts/oidc-client-ts, which is a TS port of the same library. |
dopry
left a comment
There was a problem hiding this comment.
Looking good... just two items stand out to me at the moment upon a quick review.
|
@dopry added missing tests, looks like codecov is happy now... |
|
@dopry Would be great if we could get forward with this feature, too ;-) |
|
A way you can help streamline the review process for me is lay out an easy end to end tests for me using the tests/rp and tests/idp or some other setup using docker compose so I can verify the functionality more easily without needing to implement a client to test each of these features myself. |
Can you please check if https://codeberg.org/raphael/oidc-client-testbed would work for you? It does have a way to test the session iframe. I can make a docker image if you want, as well. |
|
I only have a few hours a week for work on this project... Having to spin up and configure a 3rd party example will eat up all my review time for a week. 3rd Party examples do not create an easy way for other developers to test the features in the future, nor does it provide example implementations for consumers. I'd really like to keep all the functionality demonstration in tests/app/[idp,rp]. That said I'll try to get your example setup to test... just don't expect any additional review by me this week. Ultimately, I want all of this local so we can start working on setting up playwright tests that run through all the features so it's not something maintainers need to do manually with every change. |
|
Ping @lullis ! |
ae34028 to
3383a0b
Compare
|
@dopry , I've made changes to the rp client. Let me know if this works for you. |
|
@lullis I'll try to get to a review next week. Of the open ones, which would you prefer to see merged first? |
|
Backchannel logout, then session management, then RP-initiated registration. But any order is fine, if you think the PR is good to merge. |
d04db2f to
4bda75c
Compare
|
@lullis I'd love to get these in. Can you rebase them? |
There was a problem hiding this comment.
Pull request overview
Implements OpenID Connect Session Management 1.0 support in django-oauth-toolkit, adding server-side session-state generation, an OP check-session iframe endpoint, middleware to publish OP user-agent state, and related settings/checks/docs/tests (plus a small RP test app update).
Changes:
- Add
session_stateto authorization redirect responses when session management is enabled. - Add
SessionIFrameView+ template and advertise it via discovery metadata ascheck_session_iframe. - Introduce new session-management settings + Django system check, middleware, tests, and documentation updates.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_session_management.py | Adds middleware cookie behavior tests for OP user-agent state cookie. |
| tests/test_oidc_views.py | Adds discovery metadata and iframe/context tests + auth redirect assertion for session_state. |
| tests/test_django_checks.py | Adds system-check test requiring default session key when enabled. |
| tests/presets.py | Adds an OIDC settings preset enabling session management for tests. |
| tests/app/rp/vite.config.ts | Adjusts dev server host allowlist for the RP test app. |
| tests/app/rp/src/routes/session/+page.svelte | Adds a route to exercise session monitoring in the RP test app. |
| tests/app/rp/src/routes/device/+page.svelte | Switches device flow config to use public env vars. |
| tests/app/rp/src/routes/+page.svelte | Moves OIDC context usage out of the page (now provided by layout). |
| tests/app/rp/src/routes/+layout.svelte | Wraps app in OIDC context and enables session monitoring option. |
| tests/app/rp/src/lib/components/SessionMonitor.svelte | Adds a UI component to monitor check_session_iframe and session_state. |
| tests/app/rp/docker-compose.yml | Adds a compose setup for running the RP app (and cloudflared). |
| tests/app/rp/Dockerfile | Updates Node version and adds a dev build stage. |
| tests/app/rp/.env.example | Documents env vars for configuring the RP test app. |
| oauth2_provider/views/oidc.py | Adds discovery metadata field + SessionIFrameView. |
| oauth2_provider/views/base.py | Adds session_state computation and redirect URL mutation. |
| oauth2_provider/views/init.py | Exports SessionIFrameView. |
| oauth2_provider/utils.py | Adds session_management_state_key() helper. |
| oauth2_provider/urls.py | Adds the session-iframe/ route. |
| oauth2_provider/templates/oauth2_provider/check_session_iframe.html | Adds OP check-session iframe JS implementation. |
| oauth2_provider/templates/oauth2_provider/base.html | Adds a {% block js %} hook to support iframe JS. |
| oauth2_provider/settings.py | Adds session management settings defaults. |
| oauth2_provider/middleware.py | Adds middleware to set/delete OP user-agent state cookie. |
| oauth2_provider/checks.py | Adds system check validating default session key presence when enabled. |
| docs/settings.rst | Documents new settings (needs corrections). |
| docs/oidc.rst | Adds a Session Management docs section (needs corrections). |
| CHANGELOG.md | Notes OIDC Session Management support addition. |
| AUTHORS | Adds author entry. |
oauth2_provider/templates/oauth2_provider/check_session_iframe.html
Outdated
Show resolved
Hide resolved
2196446 to
7d88f72
Compare
|
@dopry , I checked all the suggestions from Copilot, added them and rebased the branch. This should be ready to merge now. |
There was a problem hiding this comment.
Pull request overview
Implements OpenID Connect Session Management 1.0 support, exposing the OP iframe endpoint and adding session_state to OIDC authorization responses so RPs can detect changes in the OP login session.
Changes:
- Add session management settings, Django system check validation, and documentation updates.
- Add
session_stateto OIDC authorization redirects (query/fragment) and advertisecheck_session_iframein discovery metadata. - Introduce an OP
session-iframeendpoint + middleware to set an OP user-agent-state cookie; add unit tests and an RP test app page to exercise the flow.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_session_management.py | Adds middleware + state-key unit tests for the new session management behavior. |
| tests/test_oidc_views.py | Adds tests for discovery metadata, session_state inclusion in redirects/fragments, and iframe context. |
| tests/test_django_checks.py | Adds a system-check test for missing default session key when session management is enabled. |
| tests/presets.py | Adds a preset enabling OIDC session management for tests. |
| tests/app/rp/vite.config.ts | Adjusts Vite dev server host allowlist for the RP test app. |
| tests/app/rp/src/routes/session/+page.svelte | Adds a route to render the session management monitor page. |
| tests/app/rp/src/routes/device/+page.svelte | Switches device flow config to public env vars instead of hardcoded constants. |
| tests/app/rp/src/routes/+page.svelte | Refactors the main page content to rely on layout-level OIDC context. |
| tests/app/rp/src/routes/+layout.svelte | Wraps the app in OidcContext (browser-only) and enables session monitoring in the RP app. |
| tests/app/rp/src/lib/components/SessionMonitor.svelte | Adds a UI component that loads check_session_iframe and monitors session changes via postMessage. |
| tests/app/rp/Dockerfile | Updates Node base image and introduces a dev stage for the RP app. |
| tests/app/rp/docker-compose.yml | Adds a compose setup for the RP dev environment (and optional cloudflared). |
| tests/app/rp/.env.example | Adds example public env vars for configuring the RP test app. |
| oauth2_provider/views/oidc.py | Adds check_session_iframe to discovery and introduces SessionIFrameView. |
| oauth2_provider/views/base.py | Adds session_state generation and injection into authorization redirect URIs. |
| oauth2_provider/views/init.py | Exports SessionIFrameView. |
| oauth2_provider/utils.py | Adds session_management_state_key() helper for computing OP user-agent-state. |
| oauth2_provider/urls.py | Registers the session-iframe/ endpoint. |
| oauth2_provider/templates/oauth2_provider/check_session_iframe.html | Adds the OP iframe template implementing the session check logic in JS. |
| oauth2_provider/templates/oauth2_provider/base.html | Adds a js template block in the document <head>. |
| oauth2_provider/settings.py | Adds new session management-related settings defaults. |
| oauth2_provider/middleware.py | Adds middleware to set/clear the OP UA state cookie for session management. |
| oauth2_provider/checks.py | Adds a Django system check ensuring the default session key is set when enabled. |
| docs/settings.rst | Documents new session-management settings (enabled + iframe endpoint). |
| docs/oidc.rst | Documents how to enable and configure session management. |
| CHANGELOG.md | Notes addition of OIDC Session Management support. |
| AUTHORS | Adds the contributor. |
oauth2_provider/templates/oauth2_provider/check_session_iframe.html
Outdated
Show resolved
Hide resolved
oauth2_provider/templates/oauth2_provider/check_session_iframe.html
Outdated
Show resolved
Hide resolved
tests/app/rp/.env.example
Outdated
| PUBLIC_OIDC_DISCOVERY_PATH=/o/.well-known/openid-configuration | ||
|
|
||
| # Session monitoring client ID (can be same as OIDC client ID) | ||
| PUBLIC_SESSION_CLIENT_ID=2EIxgjlyy5VgCp2fjhEpKLyRtSMMPK0hZ0gBpNdm No newline at end of file |
There was a problem hiding this comment.
PUBLIC_SESSION_CLIENT_ID is defined here but isn’t referenced anywhere in the RP test app code. Either remove it from the example env file or update the session management page/component to read this variable (instead of reusing PUBLIC_OAUTH_CLIENT_ID) to avoid configuration drift/confusion.
| PUBLIC_OIDC_DISCOVERY_PATH=/o/.well-known/openid-configuration | |
| # Session monitoring client ID (can be same as OIDC client ID) | |
| PUBLIC_SESSION_CLIENT_ID=2EIxgjlyy5VgCp2fjhEpKLyRtSMMPK0hZ0gBpNdm | |
| PUBLIC_OIDC_DISCOVERY_PATH=/o/.well-known/openid-configuration |
222692a to
65d2571
Compare
…ion-1_0.html) To enable it, user must add OIDC_SESSION_MANAGEMENT_ENABLED and provide OIDC_SESSION_MANAGEMENT_DEFAULT_SESSION_KEY on OAUTH2_PROVIDER settings, and add the proper middleware. This PR contains: - change in AuthorizationView to return 'session_state' parameter in authentication response - a SessionIFrameView as part of the OIDC views, which renders the content of the iframe used by RPs to keep track of session state changes. - middleware that sets the cookie - Documentation - Test for the changed authentication view To help with testing, there are also some improvements to the test RP. - Uses environment variables to allow connecting with other servers beyond localhost:8000 - Monitors authentication session state using the OIDC provider's check_session_iframe endpoint - Periodically checks session status every 5 seconds via postMessage communication with hidden iframe - Handles discovery document fetching and validates provider support for session management - Update docker setup to use Node 22 and provides an optional docker-compose with cloudflared integration. - Vite server configuration to allow configurable hosts for development
65d2571 to
f4c9a7c
Compare
Fixes #1130
Description of the Change
Implementation of OIDC Session Management. This PR:
OIDC_SESSION_MANAGEMENT_DEFAULT_KEYto be present.OIDC_SESSION_MANAGEMENT_DEFAULT_KEYif the user is not authenticated. This is enough for the OP to indicate whether the end user session has changed (logged in, logged out)Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS