Skip to content

OIDC logout support (PP-3726)#3120

Open
tdilauro wants to merge 5 commits intomainfrom
feature/saml-oidc-logout
Open

OIDC logout support (PP-3726)#3120
tdilauro wants to merge 5 commits intomainfrom
feature/saml-oidc-logout

Conversation

@tdilauro
Copy link
Contributor

@tdilauro tdilauro commented Mar 9, 2026

Description

Improves the initial OIDC RP-Initiated Logout flow with several enhancements:

  • Bearer-token-based logout: The logout endpoint now reads token data directly from the CM Authorization Bearer header rather than requiring the client to pass an id_token_hint query parameter. This avoids a DB lookup that would fail after token refresh (since the credential value changes on refresh but the patron's bearer token still holds the old value).
  • Stored id_token for logout hint: The raw ID token JWT is now persisted in the credential JSON at authentication (and on refresh), so it can be used as id_token_hint in RP-Initiated Logout requests.
  • Token revocation support (RFC 7009): Added a new optional revocation_endpoint configuration field. On logout, the OAuth refresh token is revoked best-effort to prevent silent re-authentication at the IdP after local CM session invalidation.
  • Graceful fallback: If the provider does not support RP-Initiated Logout, or if no stored id_token is available (credentials predating this feature), logout completes locally and redirects to success without an IdP round-trip.
  • Logout link in authentication document: If the provider supports any logout mechanism, a "rel": "logout" link is now included in the authentication document returned to clients.
  • Authentication manager caching: get_authentication_manager() now caches the OIDCAuthenticationManager instance for the provider's lifetime, preventing repeated OIDC discovery document fetches on every authenticated request.

Motivation and Context

  • Fixes and extends the OIDC logout flow introduce with the initial OIDC implementation. The original implementation required clients to pass an id_token_hint, which was fragile after token refresh (the stored credential changes but the patron's bearer token does not). This rework makes logout more reliable and self-contained, using only the bearer token already required for authenticated requests.

  • Also addresses a performance issue where the OIDC discovery document was being fetched on every request to get_authentication_manager().

[Jira PP-3726]

How Has This Been Tested?

  • Test pass locally.
  • CI tests passed.

Checklist

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@tdilauro tdilauro requested a review from a team March 9, 2026 15:22
@codecov
Copy link

codecov bot commented Mar 9, 2026

Codecov Report

❌ Patch coverage is 91.91919% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.24%. Comparing base (94fba06) to head (13df2ba).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...manager/integration/patron_auth/oidc/controller.py 81.08% 6 Missing and 1 partial ⚠️
...alace/manager/integration/patron_auth/oidc/auth.py 97.14% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #3120    +/-   ##
========================================
  Coverage   93.23%   93.24%            
========================================
  Files         492      492            
  Lines       45429    45533   +104     
  Branches     6250     6263    +13     
========================================
+ Hits        42357    42456    +99     
- Misses       1984     1990     +6     
+ Partials     1088     1087     -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dbernstein
Copy link
Contributor

Other minor issues:

WIP commit message
One commit message still says “WIP - experimenting, but problems with endpoint logout”. Consider squashing or rewording before merge.

OIDC_INVALID_ID_TOKEN_HINT removal
Removing OIDC_INVALID_ID_TOKEN_HINT is consistent with no longer requiring id_token_hint. Confirm there are no remaining references (e.g. in tests or docs).

Test coverage for post_logout_redirect_uri validation
If validation of post_logout_redirect_uri is added later, tests should cover both valid and invalid URIs.

Documentation for revocation_endpoint
The new revocation_endpoint configuration is documented in the model. A short note in admin or deployment docs would help operators.

Copy link
Contributor

@dbernstein dbernstein left a comment

Choose a reason for hiding this comment

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

Hi @tdilauro : this looks great overall. I'm giving it the thumbs up. There are some minor comments to consider.

There is one issue that I believe should be addressed before merging: ie validating the redirect url. But I leave it to you to make the call.

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