Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ansible_base/lib/dynamic_config/settings_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,13 @@ def get_mergeable_dab_settings(settings: dict) -> dict: # NOSONAR
# of DOT that we are using requires it by default.
oauth2_provider.setdefault('PKCE_REQUIRED', False)

# 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)
Comment on lines +288 to +293
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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_src

Repository: 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 -20

Repository: 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 -100

Repository: 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 -20

Repository: ansible/django-ansible-base

Length of output: 649


🏁 Script executed:

# Get the full commit message for AAP-72137
git log --format=%B -n 1 0508c86ca9c3e885113f539ecfddb0bca0c4da98

Repository: 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.py

Repository: 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 -20

Repository: 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.


oauth2_provider['OAUTH2_BACKEND_CLASS'] = 'ansible_base.oauth2_provider.authentication.OAuthLibCore'
oauth2_provider['APPLICATION_MODEL'] = DEFAULT_OAUTH2_APPLICATION_MODEL
oauth2_provider['ACCESS_TOKEN_MODEL'] = DEFAULT_OAUTH2_ACCESS_TOKEN
Expand Down
1 change: 1 addition & 0 deletions ansible_base/oauth2_provider/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
re_path(r"^\.well-known/openid-configuration/$", oauth_views.ConnectDiscoveryInfoView.as_view(), name="oidc-connect-discovery-info"),
re_path(r"^\.well-known/jwks\.json$", oauth_views.JwksInfoView.as_view(), name="jwks-info"),
re_path(r"^userinfo/$", oauth_views.UserInfoView.as_view(), name="user-info"),
re_path(r"^logout/$", oauth_views.RPInitiatedLogoutView.as_view(), name="rp-initiated-logout"),
]


Expand Down
345 changes: 345 additions & 0 deletions test_app/tests/oauth2_provider/views/test_rp_initiated_logout.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,345 @@
import base64
import json
from datetime import datetime, timedelta, timezone
from urllib.parse import parse_qs, urlparse

import pytest
from django.conf import settings
from django.test import override_settings

from ansible_base.lib.utils.response import get_relative_url
from ansible_base.oauth2_provider.models import OAuth2IDToken


@pytest.fixture
def oidc_enabled_settings():
"""Settings with OIDC enabled and RP-initiated logout configured."""
return {
**settings.OAUTH2_PROVIDER,
'OIDC_ENABLED': True,
'OIDC_RP_INITIATED_LOGOUT_ENABLED': True,
'OIDC_RP_INITIATED_LOGOUT_DELETE_TOKENS': True,
'OIDC_RP_INITIATED_LOGOUT_STRICT_REDIRECT_URIS': True,
'OIDC_RP_INITIATED_LOGOUT_ALWAYS_PROMPT': False,
'OIDC_RP_INITIATED_LOGOUT_ACCEPT_EXPIRED_TOKENS': True,
}


@pytest.fixture
def id_token_for_user(oauth2_application, user, oauth2_admin_access_token):
"""
Creates an ID token for testing RP-initiated logout.
Returns a tuple of (id_token_object, jwt_string).
"""
app = oauth2_application[0]
access_token = oauth2_admin_access_token[0]

# Create the ID token
id_token = OAuth2IDToken.objects.create(
application=app,
user=user if hasattr(user, 'username') else access_token.user,
expires=datetime.now(timezone.utc) + timedelta(hours=1),
scope='openid',
jti='test-jti-123',
)

# Create a minimal JWT for the ID token
# In a real scenario, this would be properly signed by the OIDC provider
claims = {
'iss': 'http://testserver/o',
'sub': str(id_token.user.pk),
'aud': app.client_id,
'exp': int((datetime.now(timezone.utc) + timedelta(hours=1)).timestamp()),
'iat': int(datetime.now(timezone.utc).timestamp()),
'jti': id_token.jti,
}

# For testing, we'll create a simple JWT
# 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)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 0508c86. Configure here.



@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
Comment on lines +64 to +73
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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_endpoint_requires_oidc_enabled(client):
"""
Test that the logout endpoint returns an error when OIDC is not enabled.
"""
oidc_disabled_settings = {
**settings.OAUTH2_PROVIDER,
'OIDC_ENABLED': False,
}
with override_settings(OAUTH2_PROVIDER=oidc_disabled_settings):
url = get_relative_url('oauth2_provider:rp-initiated-logout')
response = client.get(url)
# RPInitiatedLogoutView returns 404 when OIDC is disabled
assert response.status_code == 404


@pytest.mark.django_db
def test_logout_endpoint_requires_rp_logout_enabled(client):
"""
Test that the logout endpoint returns an error when RP-initiated logout is not enabled.
"""
rp_logout_disabled = {
**settings.OAUTH2_PROVIDER,
'OIDC_ENABLED': True,
'OIDC_RP_INITIATED_LOGOUT_ENABLED': False,
}
with override_settings(OAUTH2_PROVIDER=rp_logout_disabled):
url = get_relative_url('oauth2_provider:rp-initiated-logout')
response = client.get(url)
# RPInitiatedLogoutView returns 404 when RP-initiated logout is disabled
assert response.status_code == 404


@pytest.mark.django_db
def test_logout_get_request_displays_form(client, oidc_enabled_settings):
"""
Test that GET request to logout endpoint displays a logout confirmation form.
"""
with override_settings(OAUTH2_PROVIDER=oidc_enabled_settings):
url = get_relative_url('oauth2_provider:rp-initiated-logout')
response = client.get(url)
# Should display a form or confirmation page
assert response.status_code == 200
assert b'logout' in response.content.lower() or b'sign out' in response.content.lower()


@pytest.mark.django_db
def test_logout_with_post_logout_redirect_uri(client, oidc_enabled_settings, oauth2_application):
"""
Test logout with a valid post_logout_redirect_uri parameter.

Note: RPInitiatedLogoutView may return 400 if OIDC is not fully configured
(e.g., missing RSA private key for ID token verification).
"""
app = oauth2_application[0]
redirect_uri = 'https://example.com/callback'

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,
},
)

# The endpoint should be accessible (not 404)
# May return 400 if ID token validation fails or other validation errors
assert response.status_code != 404


@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]
Comment on lines +148 to +173
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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
Comment on lines +176 to +202
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.



@pytest.mark.django_db
def test_logout_endpoint_in_oidc_discovery(client, oidc_enabled_settings):
"""
Test that the logout endpoint is advertised in the OIDC discovery document.
"""
with override_settings(OAUTH2_PROVIDER=oidc_enabled_settings):
url = get_relative_url('oauth2_provider:oidc-connect-discovery-info')
response = client.get(url)
assert response.status_code == 200

discovery = response.json()
# Check if end_session_endpoint is present in discovery
assert 'end_session_endpoint' in discovery
assert 'logout' in discovery['end_session_endpoint']


@pytest.mark.django_db
def test_logout_without_prompt_when_configured(client, oidc_enabled_settings, oauth2_application):
"""
Test logout without showing prompt when ALWAYS_PROMPT is False.
"""
app = oauth2_application[0]
redirect_uri = 'https://example.com/callback'

no_prompt_settings = {
**oidc_enabled_settings,
'OIDC_RP_INITIATED_LOGOUT_ALWAYS_PROMPT': False,
}

with override_settings(OAUTH2_PROVIDER=no_prompt_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,
},
)

# The endpoint should be accessible (not 404)
assert response.status_code != 404


@pytest.mark.django_db
def test_logout_url_pattern_name(client, oidc_enabled_settings):
"""
Test that the logout URL pattern has the expected name 'rp-initiated-logout'.
"""
with override_settings(OAUTH2_PROVIDER=oidc_enabled_settings):
# This test verifies that get_relative_url works with the expected name
url = get_relative_url('oauth2_provider:rp-initiated-logout')
assert url is not None
assert '/o/logout/' in url


@pytest.mark.django_db
def test_logout_url_matches_spec(client, oidc_enabled_settings):
"""
Test that the logout URL matches the OIDC RP-Initiated Logout spec.
The endpoint should be accessible at /o/logout/
"""
with override_settings(OAUTH2_PROVIDER=oidc_enabled_settings):
# According to the spec and django-oauth-toolkit, it should be at /logout/
response = client.get('/o/logout/')
# Should not be 404
assert response.status_code != 404


@pytest.mark.django_db
def test_logout_accepts_both_get_and_post(client, oidc_enabled_settings):
"""
Test that the logout endpoint accepts both GET and POST requests.
"""
with override_settings(OAUTH2_PROVIDER=oidc_enabled_settings):
url = get_relative_url('oauth2_provider:rp-initiated-logout')

# Test GET request - should display form
get_response = client.get(url)
assert get_response.status_code == 200

# Test POST request - may return 400 for validation errors without proper params
post_response = client.post(url)
assert post_response.status_code != 404 # Endpoint exists and handles POST


@pytest.mark.django_db
def test_logout_with_client_id_only(client, oidc_enabled_settings, oauth2_application):
"""
Test logout with only client_id parameter (no ID token hint).
"""
app = oauth2_application[0]

with override_settings(OAUTH2_PROVIDER=oidc_enabled_settings):
url = get_relative_url('oauth2_provider:rp-initiated-logout')
response = client.post(
url,
{
'client_id': app.client_id,
},
)

# The endpoint should be accessible (not 404)
assert response.status_code != 404


@pytest.mark.django_db
def test_logout_without_parameters(client, oidc_enabled_settings):
"""
Test logout without any parameters.
"""
with override_settings(OAUTH2_PROVIDER=oidc_enabled_settings):
url = get_relative_url('oauth2_provider:rp-initiated-logout')
response = client.post(url)

# The endpoint should be accessible (not 404)
# May return 400 for validation errors without parameters
assert response.status_code != 404


@pytest.mark.django_db
def test_logout_configuration_defaults():
"""
Test that the default configuration includes the expected RP-initiated logout settings.
"""
# Verify that our default settings are present
oauth2_settings = settings.OAUTH2_PROVIDER

assert 'OIDC_RP_INITIATED_LOGOUT_ENABLED' in oauth2_settings
assert oauth2_settings['OIDC_RP_INITIATED_LOGOUT_ENABLED'] is True

assert 'OIDC_RP_INITIATED_LOGOUT_DELETE_TOKENS' in oauth2_settings
assert oauth2_settings['OIDC_RP_INITIATED_LOGOUT_DELETE_TOKENS'] is True

assert 'OIDC_RP_INITIATED_LOGOUT_STRICT_REDIRECT_URIS' in oauth2_settings
assert oauth2_settings['OIDC_RP_INITIATED_LOGOUT_STRICT_REDIRECT_URIS'] is True

assert 'OIDC_RP_INITIATED_LOGOUT_ALWAYS_PROMPT' in oauth2_settings
assert oauth2_settings['OIDC_RP_INITIATED_LOGOUT_ALWAYS_PROMPT'] is False

assert 'OIDC_RP_INITIATED_LOGOUT_ACCEPT_EXPIRED_TOKENS' in oauth2_settings
assert oauth2_settings['OIDC_RP_INITIATED_LOGOUT_ACCEPT_EXPIRED_TOKENS'] is True
Loading