[AAP-72137] [oauth2_provider] Add RP-initiated logout support#984
[AAP-72137] [oauth2_provider] Add RP-initiated logout support#984BrennanPaciorek wants to merge 1 commit intoansible:develfrom
Conversation
📝 WalkthroughWalkthroughThis change adds RP-Initiated Logout functionality for OAuth2/OIDC providers by establishing default configuration settings, adding a new logout endpoint URL route, and introducing comprehensive test coverage for logout behavior across multiple scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
BrennanPaciorek
force-pushed
the
AAP-72137-RP-Logout
branch
from
0a7383e to
0508c86
Compare
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 0508c86. Configure here.
| # In production, this would be properly signed with RS256 or HS256 | ||
| jwt_string = base64.urlsafe_b64encode(json.dumps(claims).encode()).decode() | ||
|
|
||
| return (id_token, jwt_string, app) |
There was a problem hiding this comment.
Unused fixture with misleading docstring and invalid JWT
Low Severity
The id_token_for_user fixture is defined but never used by any of the 15 tests in this file. It also has two latent bugs: the docstring says it returns a 2-tuple (id_token_object, jwt_string) but it actually returns a 3-tuple (id_token, jwt_string, app), and the jwt_string it constructs is just raw base64-encoded JSON, not a valid JWT (header.payload.signature format), so it would fail if ever passed as an id_token_hint. This unused fixture also pulls in several otherwise-unused imports (base64, json, datetime, timedelta, timezone, OAuth2IDToken).
Reviewed by Cursor Bugbot for commit 0508c86. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test_app/tests/oauth2_provider/views/test_rp_initiated_logout.py (1)
28-61: Unused fixtureid_token_for_user.This fixture is defined but not referenced by any test in the module. The "JWT" it produces is also not a real JWT — it is just
base64(json)with no header or signature — so if a future test did use it, most logout validation paths would reject it. Either wire it into the tests that actually exerciseid_token_hintvalidation (producing a properly signed token via the provider's key) or remove it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_app/tests/oauth2_provider/views/test_rp_initiated_logout.py` around lines 28 - 61, The id_token_for_user fixture is unused and its jwt_string is not a real signed JWT; either remove the fixture or update tests to use it and produce a proper signed ID token. If you choose to keep it, replace the naive base64(json) construction in id_token_for_user with a JWT signed by the test OIDC provider (use the project's existing signing helper or the provider's private key) and ensure tests that exercise id_token_hint validation reference id_token_for_user (e.g., use its returned jwt_string when calling the RP-initiated logout view). If you choose to remove it, delete the id_token_for_user fixture and any related unused imports (OAuth2IDToken, base64, json) to avoid dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible_base/lib/dynamic_config/settings_logic.py`:
- Around line 288-293: The default currently sets
OIDC_RP_INITIATED_LOGOUT_ACCEPT_EXPIRED_TOKENS to True via
oauth2_provider.setdefault; change this default to False so expired
id_token_hint values are not accepted by default. Update the
oauth2_provider.setdefault call for
'OIDC_RP_INITIATED_LOGOUT_ACCEPT_EXPIRED_TOKENS' to False, and consider adding a
brief inline comment near OIDC_RP_INITIATED_LOGOUT_ALWAYS_PROMPT and
OIDC_RP_INITIATED_LOGOUT_ACCEPT_EXPIRED_TOKENS explaining integrators can opt-in
to accept expired tokens if desired.
In `@test_app/tests/oauth2_provider/views/test_rp_initiated_logout.py`:
- Around line 176-202: The test_logout_with_state_parameter currently can pass
vacuously when no redirect occurs; update the test so it arranges valid
preconditions that force the view to redirect and then assert unconditionally
that the state round-trips. Specifically, in test_logout_with_state_parameter
ensure the oauth2_application (app) has the post_logout_redirect_uri registered
(matching redirect_uri), supply a valid id_token_hint for that client and
establish an authenticated session for the client fixture so the view will
return a 302 from client.post to
get_relative_url('oauth2_provider:rp-initiated-logout'), then parse
response['Location'] and assert params['state'][0] == state without gating the
assertion behind status checks.
- Around line 64-73: Replace permissive assertions like "response.status_code !=
404" with precise, behavior-oriented checks: keep a single existence test
(test_logout_endpoint_exists) that simply resolves the URL via
get_relative_url('oauth2_provider:rp-initiated-logout') and asserts a successful
resolution, then update each behavior test
(test_logout_with_post_logout_redirect_uri,
test_logout_without_prompt_when_configured, test_logout_with_client_id_only,
test_logout_without_parameters, test_logout_url_matches_spec) to assert exact
expected statuses and responses using client.get/client.post under
override_settings(OAUTH2_PROVIDER=oidc_enabled_settings): e.g., assert 200 and
presence of the logout form/html for unauthenticated GETs (check
response.content), assert 302 and validate the Location header for successful
POST redirects, and assert 400 for invalid parameter combinations; use
get_relative_url and the named endpoint to locate the route and adjust
assertions accordingly.
- Around line 148-173: The test
test_logout_with_invalid_redirect_uri_when_strict currently allows too-broad
outcomes; tighten it to explicitly assert that the invalid redirect is not
honored by checking response is not a 302 to the malicious URL and/or that a
rejection is returned: after posting with post_logout_redirect_uri =
invalid_redirect (and client_id = app.client_id), assert either
response.status_code == 400 (and response.content does not contain the malicious
host) or assert response.status_code != 302 or, if 302, that
response['Location'] does not equal invalid_redirect; update the assertions in
test_logout_with_invalid_redirect_uri_when_strict accordingly to fail if the app
redirects to the malicious site.
---
Nitpick comments:
In `@test_app/tests/oauth2_provider/views/test_rp_initiated_logout.py`:
- Around line 28-61: The id_token_for_user fixture is unused and its jwt_string
is not a real signed JWT; either remove the fixture or update tests to use it
and produce a proper signed ID token. If you choose to keep it, replace the
naive base64(json) construction in id_token_for_user with a JWT signed by the
test OIDC provider (use the project's existing signing helper or the provider's
private key) and ensure tests that exercise id_token_hint validation reference
id_token_for_user (e.g., use its returned jwt_string when calling the
RP-initiated logout view). If you choose to remove it, delete the
id_token_for_user fixture and any related unused imports (OAuth2IDToken, base64,
json) to avoid dead code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ce865aa-4ef4-49bc-b56b-ce20d0ba562e
📒 Files selected for processing (3)
ansible_base/lib/dynamic_config/settings_logic.pyansible_base/oauth2_provider/urls.pytest_app/tests/oauth2_provider/views/test_rp_initiated_logout.py
| # RP-Initiated Logout (OIDC) settings | ||
| oauth2_provider.setdefault('OIDC_RP_INITIATED_LOGOUT_ENABLED', True) | ||
| oauth2_provider.setdefault('OIDC_RP_INITIATED_LOGOUT_DELETE_TOKENS', True) | ||
| oauth2_provider.setdefault('OIDC_RP_INITIATED_LOGOUT_STRICT_REDIRECT_URIS', True) | ||
| oauth2_provider.setdefault('OIDC_RP_INITIATED_LOGOUT_ALWAYS_PROMPT', False) | ||
| oauth2_provider.setdefault('OIDC_RP_INITIATED_LOGOUT_ACCEPT_EXPIRED_TOKENS', True) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm django-oauth-toolkit 2.3.0 behavior for these two settings
pip download django-oauth-toolkit==2.3.0 --no-deps -d /tmp/dot 2>/dev/null
cd /tmp/dot && ls *.whl | head -1 | xargs -I{} unzip -o {} -d dot_src >/dev/null
rg -nP 'OIDC_RP_INITIATED_LOGOUT_(ACCEPT_EXPIRED_TOKENS|ALWAYS_PROMPT)' /tmp/dot/dot_srcRepository: ansible/django-ansible-base
Length of output: 1072
🏁 Script executed:
# Check the exact settings in the ansible_base file
cat -n ansible_base/lib/dynamic_config/settings_logic.py | sed -n '285,295p'Repository: ansible/django-ansible-base
Length of output: 847
🏁 Script executed:
# Look for any PR description or related context files
fd -i 'readme|changelog|description' --type f | head -20Repository: ansible/django-ansible-base
Length of output: 195
🏁 Script executed:
# Search for git commits related to these OIDC settings
git log --all --oneline --grep='OIDC\|RP-Initiated' | head -10
git log --all -p --follow -S 'OIDC_RP_INITIATED_LOGOUT_ACCEPT_EXPIRED_TOKENS' -- ansible_base/lib/dynamic_config/settings_logic.py | head -100Repository: ansible/django-ansible-base
Length of output: 4948
🏁 Script executed:
# Search for any documentation or comments about why this was set
rg -n 'OIDC_RP_INITIATED_LOGOUT.*ACCEPT_EXPIRED' -B 5 -A 5 ansible_base/Repository: ansible/django-ansible-base
Length of output: 1402
🏁 Script executed:
# Look for any related issues or PR references in code comments
rg -n 'ACCEPT_EXPIRED_TOKENS|secure defaults' --type py | head -20Repository: ansible/django-ansible-base
Length of output: 649
🏁 Script executed:
# Get the full commit message for AAP-72137
git log --format=%B -n 1 0508c86ca9c3e885113f539ecfddb0bca0c4da98Repository: ansible/django-ansible-base
Length of output: 137
🏁 Script executed:
# Look at the test file to understand expected behavior and design rationale
head -100 test_app/tests/oauth2_provider/views/test_rp_initiated_logout.pyRepository: ansible/django-ansible-base
Length of output: 3488
🏁 Script executed:
# Search for any related issues or documentation about this feature
rg -n 'AAP-72137|RP-initiated logout' --type md | head -20Repository: ansible/django-ansible-base
Length of output: 53
Reconsider OIDC_RP_INITIATED_LOGOUT_ACCEPT_EXPIRED_TOKENS=True as a default.
Enabling OIDC_RP_INITIATED_LOGOUT_ACCEPT_EXPIRED_TOKENS by default means an expired id_token_hint will still be honored for RP-initiated logout, which relaxes integrity checks and tolerates replay of old tokens. The OIDC RP-Initiated Logout spec permits this, but most providers leave it off by default. The combination of OIDC_RP_INITIATED_LOGOUT_ALWAYS_PROMPT=False + ACCEPT_EXPIRED_TOKENS=True enables silent logout even with expired tokens, broadening the window for token reuse. Consider defaulting ACCEPT_EXPIRED_TOKENS to False and letting integrators opt in, which aligns with least-privilege principles.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ansible_base/lib/dynamic_config/settings_logic.py` around lines 288 - 293,
The default currently sets OIDC_RP_INITIATED_LOGOUT_ACCEPT_EXPIRED_TOKENS to
True via oauth2_provider.setdefault; change this default to False so expired
id_token_hint values are not accepted by default. Update the
oauth2_provider.setdefault call for
'OIDC_RP_INITIATED_LOGOUT_ACCEPT_EXPIRED_TOKENS' to False, and consider adding a
brief inline comment near OIDC_RP_INITIATED_LOGOUT_ALWAYS_PROMPT and
OIDC_RP_INITIATED_LOGOUT_ACCEPT_EXPIRED_TOKENS explaining integrators can opt-in
to accept expired tokens if desired.
| @pytest.mark.django_db | ||
| def test_logout_endpoint_exists(client, oidc_enabled_settings): | ||
| """ | ||
| Test that the /o/logout/ endpoint is accessible when OIDC is enabled. | ||
| """ | ||
| with override_settings(OAUTH2_PROVIDER=oidc_enabled_settings): | ||
| url = get_relative_url('oauth2_provider:rp-initiated-logout') | ||
| response = client.get(url) | ||
| # The endpoint should exist (not 404) | ||
| assert response.status_code != 404 |
There was a problem hiding this comment.
status_code != 404 is too permissive across most tests.
Many tests in this file only assert response.status_code != 404 (e.g., test_logout_endpoint_exists, test_logout_with_post_logout_redirect_uri, test_logout_without_prompt_when_configured, test_logout_with_client_id_only, test_logout_without_parameters, test_logout_url_matches_spec). That will pass on 500s and other regressions too, which undermines the value of the suite. Please tighten these to the specific status codes / response contents you actually expect (e.g., 200 with a form for unauthenticated GET, 302 with a parsed Location for valid POST, 400 for invalid params). Consolidating the "exists" checks into a single URL-resolution test and giving the behavior-oriented tests stronger assertions will catch real regressions instead of just route deletions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test_app/tests/oauth2_provider/views/test_rp_initiated_logout.py` around
lines 64 - 73, Replace permissive assertions like "response.status_code != 404"
with precise, behavior-oriented checks: keep a single existence test
(test_logout_endpoint_exists) that simply resolves the URL via
get_relative_url('oauth2_provider:rp-initiated-logout') and asserts a successful
resolution, then update each behavior test
(test_logout_with_post_logout_redirect_uri,
test_logout_without_prompt_when_configured, test_logout_with_client_id_only,
test_logout_without_parameters, test_logout_url_matches_spec) to assert exact
expected statuses and responses using client.get/client.post under
override_settings(OAUTH2_PROVIDER=oidc_enabled_settings): e.g., assert 200 and
presence of the logout form/html for unauthenticated GETs (check
response.content), assert 302 and validate the Location header for successful
POST redirects, and assert 400 for invalid parameter combinations; use
get_relative_url and the named endpoint to locate the route and adjust
assertions accordingly.
| @pytest.mark.django_db | ||
| def test_logout_with_invalid_redirect_uri_when_strict(client, oidc_enabled_settings, oauth2_application): | ||
| """ | ||
| Test that logout rejects invalid redirect URIs when STRICT_REDIRECT_URIS is enabled. | ||
| """ | ||
| app = oauth2_application[0] | ||
| invalid_redirect = 'https://malicious-site.com/callback' | ||
|
|
||
| strict_settings = { | ||
| **oidc_enabled_settings, | ||
| 'OIDC_RP_INITIATED_LOGOUT_STRICT_REDIRECT_URIS': True, | ||
| } | ||
|
|
||
| with override_settings(OAUTH2_PROVIDER=strict_settings): | ||
| url = get_relative_url('oauth2_provider:rp-initiated-logout') | ||
| response = client.post( | ||
| url, | ||
| { | ||
| 'post_logout_redirect_uri': invalid_redirect, | ||
| 'client_id': app.client_id, | ||
| }, | ||
| ) | ||
|
|
||
| # Should reject the invalid redirect URI | ||
| # The response depends on implementation - could be error or no redirect | ||
| assert response.status_code in [200, 400] |
There was a problem hiding this comment.
test_logout_with_invalid_redirect_uri_when_strict does not actually assert rejection.
Allowing status_code in [200, 400] plus no check on where (or whether) the redirect happens means this test passes even if the invalid redirect URI is silently honored (e.g., a 302 to malicious-site.com would fail, but a 200 rendering a logout form that then redirects anywhere would pass). To actually validate strict-redirect enforcement, assert either:
- the response is not a 302 to
invalid_redirect, or - the response status is specifically the rejection code DOT emits (typically 400 with an error body), and the body does not contain the malicious host.
Suggested tightening
- # Should reject the invalid redirect URI
- # The response depends on implementation - could be error or no redirect
- assert response.status_code in [200, 400]
+ # Must not redirect the user to the unregistered URI
+ assert response.status_code != 302 or invalid_redirect not in response.get('Location', '')
+ # And the malicious host must not appear in the rendered response
+ assert b'malicious-site.com' not in response.content🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test_app/tests/oauth2_provider/views/test_rp_initiated_logout.py` around
lines 148 - 173, The test test_logout_with_invalid_redirect_uri_when_strict
currently allows too-broad outcomes; tighten it to explicitly assert that the
invalid redirect is not honored by checking response is not a 302 to the
malicious URL and/or that a rejection is returned: after posting with
post_logout_redirect_uri = invalid_redirect (and client_id = app.client_id),
assert either response.status_code == 400 (and response.content does not contain
the malicious host) or assert response.status_code != 302 or, if 302, that
response['Location'] does not equal invalid_redirect; update the assertions in
test_logout_with_invalid_redirect_uri_when_strict accordingly to fail if the app
redirects to the malicious site.
| @pytest.mark.django_db | ||
| def test_logout_with_state_parameter(client, oidc_enabled_settings, oauth2_application): | ||
| """ | ||
| Test that the state parameter is preserved in the redirect. | ||
| """ | ||
| app = oauth2_application[0] | ||
| redirect_uri = 'https://example.com/callback' | ||
| state = 'test-state-value-123' | ||
|
|
||
| with override_settings(OAUTH2_PROVIDER=oidc_enabled_settings): | ||
| url = get_relative_url('oauth2_provider:rp-initiated-logout') | ||
| response = client.post( | ||
| url, | ||
| { | ||
| 'post_logout_redirect_uri': redirect_uri, | ||
| 'client_id': app.client_id, | ||
| 'state': state, | ||
| }, | ||
| ) | ||
|
|
||
| # If redirected, state should be in the redirect URL | ||
| if response.status_code == 302: | ||
| redirect_url = response['Location'] | ||
| parsed = urlparse(redirect_url) | ||
| params = parse_qs(parsed.query) | ||
| if 'state' in params: | ||
| assert params['state'][0] == state |
There was a problem hiding this comment.
test_logout_with_state_parameter can pass vacuously.
If the response isn't a 302, or state isn't in the parsed query, the assertions are skipped and the test passes without verifying anything. Arrange the preconditions (valid id_token_hint, registered post_logout_redirect_uri on the application, authenticated session) so the view actually redirects, then assert unconditionally that params['state'][0] == state. Otherwise this test gives false confidence that state round-trips correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test_app/tests/oauth2_provider/views/test_rp_initiated_logout.py` around
lines 176 - 202, The test_logout_with_state_parameter currently can pass
vacuously when no redirect occurs; update the test so it arranges valid
preconditions that force the view to redirect and then assert unconditionally
that the state round-trips. Specifically, in test_logout_with_state_parameter
ensure the oauth2_application (app) has the post_logout_redirect_uri registered
(matching redirect_uri), supply a valid id_token_hint for that client and
establish an authenticated session for the client fixture so the view will
return a 302 from client.post to
get_relative_url('oauth2_provider:rp-initiated-logout'), then parse
response['Location'] and assert params['state'][0] == state without gating the
assertion behind status checks.
|
2 participants
Description
This PR adds support for OIDC RP-initiated logout (end_session_endpoint) to enable proper single sign-out in SSO environments.
/o/logout/endpoint usingRPInitiatedLogoutViewfrom django-oauth-toolkit 2.3.0Type of Change
Self-Review Checklist
Testing Instructions
Prerequisites
Steps to Test
TOX_DOCKER_GATEWAY=0.0.0.0 tox -e py312-sqlite -- -k test_rp_initiated_logout -n0 --no-covTOX_DOCKER_GATEWAY=0.0.0.0 tox -e py312-sqlite -- -k test_oidc_url_patterns_match_dot -n0 --no-cov/o/.well-known/openid-configuration/and confirmend_session_endpointis presentExpected Results
"end_session_endpoint": "http://testserver/o/logout/"/o/logout/when OIDC is enabledAdditional Context
Implementation Details
Files Changed:
ansible_base/oauth2_provider/urls.py- Added logout URL patternansible_base/lib/dynamic_config/settings_logic.py- Added secure default configurationtest_app/tests/oauth2_provider/views/test_rp_initiated_logout.py- Comprehensive test suiteConfiguration Defaults:
OIDC_RP_INITIATED_LOGOUT_ENABLED: True - Feature enabledOIDC_RP_INITIATED_LOGOUT_DELETE_TOKENS: True - Revokes tokens on logoutOIDC_RP_INITIATED_LOGOUT_STRICT_REDIRECT_URIS: True - Validates redirect URIs against registered URLsOIDC_RP_INITIATED_LOGOUT_ALWAYS_PROMPT: False - Allows silent logoutOIDC_RP_INITIATED_LOGOUT_ACCEPT_EXPIRED_TOKENS: True - Accepts expired ID tokens for logoutSecurity Benefits:
Test Coverage
15 comprehensive tests covering:
References
Note
Medium Risk
Adds a new OIDC logout endpoint and enables token/session cleanup behavior by default, affecting authentication flows and redirect validation. Risk is moderate because misconfiguration could impact SSO logout behavior or introduce redirect/token handling regressions.
Overview
Adds OIDC RP-initiated logout support by wiring
RPInitiatedLogoutViewto a new/o/logout/route and ensuring it appears in OIDC discovery asend_session_endpoint.Introduces default
OAUTH2_PROVIDERsettings to enable RP-initiated logout, revoke tokens on logout, enforce strict post-logout redirect URI validation, and control prompting/expired-token handling. Adds a comprehensive test file covering endpoint availability, feature-flag gating, redirect/state handling, and default configuration.Reviewed by Cursor Bugbot for commit 0508c86. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit