✨(oidc) add backend logout endpoint #28
Conversation
qbey
force-pushed
the
qbey/add-backchannel-logout
branch
2 times, most recently
from
079d690 to
9e2683b
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements OIDC back-channel logout functionality according to the OpenID Connect Back-Channel Logout 1.0 specification. The IdP can now send server-to-server logout requests to kill user sessions when they log out elsewhere.
- Adds a new
OIDCBackChannelLogoutViewthat handles JWT-based logout requests from identity providers - Stores the OIDC session ID (
sid) during authentication for future session targeting - Includes comprehensive JWT validation, JTI replay protection, and session management
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lasuite/oidc_login/views.py | Adds the main OIDCBackChannelLogoutView class with complete OIDC logout implementation |
| src/lasuite/oidc_login/urls.py | Adds URL routing for the new backchannel logout endpoint |
| src/lasuite/oidc_login/backends.py | Stores OIDC session ID (sid) in session during authentication |
| tests/oidc_login/test_views.py | Comprehensive test coverage for all backchannel logout scenarios |
| gitlint/gitlint_emoji.py | Removes extra blank line (formatting cleanup) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
qbey
force-pushed
the
qbey/add-backchannel-logout
branch
from
e0220ad to
8c9966d
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an OpenID Connect back-channel logout endpoint and URL; implements logout_token JWT validation, JTI replay protection, session termination; stores token "sid" in session during authenticate; includes extensive back-channel logout tests; minor formatting tweak in a gitlint file. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor IdP as OIDC Provider
participant App as App Server
participant BCL as Back-Channel Logout View
participant JWT as JWT Validator (JOSE)
participant Cache as JTI Cache
participant Sess as Session Store
participant DB as User DB
rect rgb(245,245,255)
note over IdP,App: Back-channel logout flow
IdP->>BCL: POST /backchannel-logout (logout_token)
BCL->>JWT: Validate signature + claims (typ, iss, aud, exp/iat, jti, events, no nonce)
alt invalid token/claims
JWT-->>BCL: Error
BCL-->>IdP: 400 JSON (Cache-Control: no-store)
else valid
BCL->>Cache: Check/add jti (replay protection)
alt replay detected
Cache-->>BCL: Exists
BCL-->>IdP: 400 JSON (replay)
else first-seen
Cache-->>BCL: Stored
opt Resolve sessions
alt sid present
BCL->>Sess: Terminate session(s) by sid
else sub present
BCL->>DB: Lookup user(s) by sub
BCL->>Sess: Terminate all sessions for user(s)
end
end
BCL-->>IdP: 204 No Content (Cache-Control: no-store)
end
end
end
sequenceDiagram
autonumber
actor User
participant App as App Server
participant Auth as OIDC Backend
participant Sess as Session
note over User,App: Authentication flow (sid stored)
User->>App: Login via OIDC
App->>Auth: authenticate(token_info)
Auth->>Sess: Save refresh token
Auth->>Sess: Save `oidc_sid` = token_info["sid"]
Auth-->>App: Return User object
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
gitlint/gitlint_emoji.py(0 hunks)src/lasuite/oidc_login/backends.py(1 hunks)src/lasuite/oidc_login/urls.py(2 hunks)src/lasuite/oidc_login/views.py(3 hunks)tests/oidc_login/test_views.py(3 hunks)
💤 Files with no reviewable changes (1)
- gitlint/gitlint_emoji.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
src/lasuite/oidc_login/views.py
Outdated
| if success: | ||
| logger.info("Back-channel logout successful for sub=%s, sid=%s", sub, sid) | ||
| # According to the spec, return 200 OK | ||
| response = HttpResponse(status=200) |
There was a problem hiding this comment.
| response = HttpResponse(status=200) | |
| response = HttpResponse(status=204) |
Without body, a 204 should be used (coucou coderabbit)
There was a problem hiding this comment.
If the logout succeeded, the RP MUST respond with HTTP 200 OK. However, note that some Web frameworks will substitute an HTTP 204 No Content response for an HTTP 200 OK when the HTTP body is empty. Therefore, OPs should be prepared to also process an HTTP 204 No Content response as a successful response.
Indeed 👍
This endpoint can be called by the IdP to kill all the user sessions. In order to allow to kill only one session, we need to store the session ID when login from IdP...
Storing this ID allows to kill only a specific session when calling the backchannel logout endpoint.
qbey
force-pushed
the
qbey/add-backchannel-logout
branch
from
ee22b23 to
4e27243
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/lasuite/oidc_login/views.py (1)
575-607: Add explicit pass statements to empty method bodies.The placeholder methods
revoke_refresh_tokensandpropagate_logout_to_downstream_rpsshould includepassstatements or raiseNotImplementedErrorto make their unimplemented status explicit.Apply this diff:
def revoke_refresh_tokens(self, user): """ Revoke user's refresh tokens (section 2.7 of the spec). According to the spec: - Refresh tokens WITHOUT offline_access MUST be revoked - Refresh tokens WITH offline_access MUST NOT be revoked normally NOTE: This implementation depends on the token management system. To be implemented if needed by the project Args: user: The user whose tokens should be revoked """ + pass def propagate_logout_to_downstream_rps(self, user, sub, sid): """ Propagate logout to downstream RPs if this RP is also an OP. According to section 2.7 of the spec, if the RP receiving the logout is itself an OP serving other RPs, it should propagate the logout by sending logout requests to its own RPs. NOTE: To be implemented if needed by the project. Args: user: The user to log out sub: Subject identifier sid: Session ID (optional) """ + pass
🧹 Nitpick comments (1)
src/lasuite/oidc_login/views.py (1)
486-508: Session iteration may impact performance at scale.The code iterates through all active sessions (lines 486, 513) to find matching user sessions. With thousands of concurrent sessions, this could become slow. However, Django's session framework doesn't provide efficient querying by decoded session data, so this approach is reasonable given the framework constraints.
For production deployments with high session counts, consider:
- Using a fast cache backend (Redis/Memcached) for session storage
- Implementing session partitioning or custom session storage that allows querying by user_id/oidc_sid
- Monitoring backchannel logout endpoint performance and setting appropriate timeouts
Also applies to: 513-550
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
gitlint/gitlint_emoji.py(0 hunks)src/lasuite/oidc_login/backends.py(1 hunks)src/lasuite/oidc_login/urls.py(2 hunks)src/lasuite/oidc_login/views.py(3 hunks)tests/oidc_login/test_views.py(3 hunks)
💤 Files with no reviewable changes (1)
- gitlint/gitlint_emoji.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lasuite/oidc_login/backends.py
🧰 Additional context used
🧬 Code graph analysis (3)
src/lasuite/oidc_login/urls.py (1)
src/lasuite/oidc_login/views.py (1)
OIDCBackChannelLogoutView(163-606)
src/lasuite/oidc_login/views.py (2)
src/lasuite/oidc_login/backends.py (1)
OIDCAuthenticationBackend(58-312)tests/test_project/user/models.py (1)
User(7-18)
tests/oidc_login/test_views.py (2)
src/lasuite/oidc_login/views.py (7)
OIDCBackChannelLogoutView(163-606)post(102-122)post(195-249)error_response(251-269)is_valid_logout_event(379-411)check_and_store_jti(413-447)logout_user_sessions(449-573)tests/factories.py (1)
UserFactory(11-19)
🔇 Additional comments (6)
src/lasuite/oidc_login/urls.py (1)
6-6: LGTM! Clean URL wiring for the backchannel logout endpoint.The import and URL pattern follow Django conventions and integrate well with the existing OIDC URL structure.
Also applies to: 16-20
tests/oidc_login/test_views.py (3)
365-489: Excellent test coverage for backchannel logout error paths.The tests thoroughly cover validation failures (missing token, invalid token, missing claims, JTI replay) and verify correct error responses with appropriate status codes and headers.
569-648: Strong end-to-end test with real JWT validation.The test generates a real RSA keypair, mocks the JWKS endpoint, and validates the full backchannel logout flow including JWT signature verification and session deletion. This provides good confidence in the implementation.
651-863: Comprehensive edge case coverage for session handling.The tests cover important edge cases including:
- sid-only logout with user resolution
- Orphaned sessions (user deleted)
- Anonymous sessions
- Invalid user IDs
- Multiple users/sessions
This thorough coverage helps prevent regressions.
src/lasuite/oidc_login/views.py (2)
470-573: Well-implemented fix for sid-only backchannel logout.The refactored logic properly handles the sid-only case (lines 510-551) by:
- Searching for the session by sid first
- Lazily resolving the user from the session when found
- Deleting orphaned/anonymous sessions appropriately
- Only calling user-related methods when a user was resolved (lines 569-571)
This addresses the critical security issue flagged in the previous review where sid-only tokens were not properly logging out sessions.
287-340: Standardize on joserfc for JWT handling. The code currently uses PyJWT only for header inspection and byte conversion, while token decode and validation rely onjoserfc.jwt. To avoid maintenance overhead and potential mismatches, replacepyjwtimports (get_unverified_header,DecodeError,InvalidTokenError,force_bytes) with the correspondingjoserfc.jwtAPI once you confirm it supports unverified header extraction and equivalent error types.
2 participants
Purpose
This endpoint can be called by the IdP to kill all the user sessions.
In order to allow to kill only one session, we need to store the session ID when login from IdP...
Proposal
Description...
Summary by CodeRabbit
New Features
Tests
Style