Skip to content

Backchannel Logout #1573

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lullis
Copy link
Contributor

@lullis lullis commented May 6, 2025

  • Add migration to add backchannel logout support
  • Add model for Logout Token
  • Add admin for Logout Token
  • Add parameters related to backchannel logout on OIDC Discovery View
  • Change application creation and update form
  • Change template that renders information about the application
  • Add "handlers" module for signals handlers definition.

Fixes #1545

Description of the Change

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@lullis lullis requested a review from tonial May 6, 2025 10:59
@lullis
Copy link
Contributor Author

lullis commented May 6, 2025

Before going further with this PR and working on tests/documentation, could please someone comment about the general structure of the implementation?

My main concerns:

  • This is implemented as a handler of the user_logged_out signal, which means that it will be executed during the logout cycle. I'm not sure if it's a good idea to have a potentially blocking operation as part of the request cycle. If it is not a good idea to have this being executed on the request cycle, then what would be the recommended approach? Should this functionality try to auto-detect background workers/celery/django-rq, or perhaps just provide this function in some form of "workers" module and leave it up to application developers to hook it up themselves?
  • I'm reasonably sure that I shouldn't be using request.session.session_key for the session identifier (it's not guaranteed that IDPs will be using it on their servers), but on the other hand I'm not entirely sure what should be used as the "sid" field for RPs that require it: should it be derived from the IDLoginToken somehow?
  • Logic being in the "models" module instead of "validators": unless I'm mistaken, oauth logic is delegated to validators and oauthlib to handle all forms of RP requests, but given that backchannel logout is something where the OP is initiating the request, it seems different from everything else so far.

@dopry
Copy link
Contributor

dopry commented May 19, 2025

@lullis

  1. for now the back channel can live in the logout request, we can move it later. Maybe start with a fire and forget approach to minimize blocking. Rather than logging out as a part of the OP logout, maybe we have a list of clients to explicitly logout without necessarily terminating the OP session?
  2. You should be using the sid from the tokens I believe. Do we issue the sid claim currently? If not this may not be applicable at this juncture.
  3. not sure off hand. will look more closely, but try to following existing patterns where possible.

@lullis
Copy link
Contributor Author

lullis commented May 31, 2025

@dopry

  1. Maybe I am missing something, but how would you set up any fire-and-forget function handler in the response-request cycle?

  2. Yeah, I don't think there is anything about the sid claim. So would it be okay if we had a first version where the server supports backchannel logout, but not the session identifier?

@lullis lullis force-pushed the 1545_backchannel_logout branch 3 times, most recently from 36a433e to 4aeac81 Compare June 3, 2025 01:58
Copy link

codecov bot commented Jun 3, 2025

Codecov Report

Attention: Patch coverage is 69.72477% with 33 lines in your changes missing coverage. Please review.

Project coverage is 96.06%. Comparing base (8d3e7a9) to head (4aeac81).

Files with missing lines Patch % Lines
oauth2_provider/models.py 54.68% 29 Missing ⚠️
oauth2_provider/views/oidc.py 33.33% 2 Missing ⚠️
oauth2_provider/checks.py 88.88% 1 Missing ⚠️
oauth2_provider/views/application.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1573      +/-   ##
==========================================
- Coverage   97.38%   96.06%   -1.32%     
==========================================
  Files          34       35       +1     
  Lines        2214     2315     +101     
==========================================
+ Hits         2156     2224      +68     
- Misses         58       91      +33     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lullis lullis force-pushed the 1545_backchannel_logout branch from 4aeac81 to 8750834 Compare June 3, 2025 03:10
@lullis lullis marked this pull request as ready for review June 3, 2025 10:08
@dopry
Copy link
Contributor

dopry commented Jun 3, 2025

  1. I forget the world hasn't moved to async/await across the board. It's fine to do the work in the signal handler.
  2. The spec says A Logout Token MUST contain either a sub or a sid Claim, and MAY contain both. If a sid Claim is not present, the intent is that all sessions at the RP for the End-User identified by the iss and sub Claims be logged out. so if there is no SID, we need a SUB.

lullis added 2 commits June 6, 2025 14:54
 - Add migration to add backchannel logout support
 - Add model for Logout Token
 - Add admin for Logout Token
 - Add parameters related to backchannel logout on OIDC Discovery View
 - Change application creation and update form
 - Change template that renders information about the application
 -
@lullis lullis force-pushed the 1545_backchannel_logout branch from b89d426 to d3c2684 Compare June 6, 2025 12:54
@dopry
Copy link
Contributor

dopry commented Jun 10, 2025

@lullis it looks like the next step is expanding test coverage right now the patch is just shy of 70%, our target is 100% coverage on patches going forward. At the very least we aim to match the current total coverage.

@lullis
Copy link
Contributor Author

lullis commented Jun 11, 2025

@dopry this coverage report seems to be stale and based on a previous commit. How can I re-run the coverage report?

@dopry
Copy link
Contributor

dopry commented Jun 13, 2025

It seems something has changed with the jazzband Codecov account and our codecov uploads are getting rate limited. I don't have the necessary permissions to add a the codecov repository upload token to the repo secrets and update our CI to use it. We're working on transferring out of jazzband right now to get away from those types of restrictions. In the meantime I'll keep trying to rerun the build to get coverage uploaded.

@lullis
Copy link
Contributor Author

lullis commented Jun 13, 2025

I will take a look to see if I can run the report of the diff locally. Running coverage for the whole report showed that all files touched were at least at 99% coverage.

@dopry
Copy link
Contributor

dopry commented Jul 11, 2025

@lullis I hear you. Right now I'm in a bit of a holding pattern waiting on @jezdez to transfer the project out of jazzband so we can fix our codecov setup and get back to merging code. He was supposed to do it the last two weeks, but flaked each time.

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.

OIDC Back-Channel Logout
2 participants