Backchannel Logout

[lullis]

• Add migration to add backchannel logout support
• 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]

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]

@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]

@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?

[codecov]

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.

[dopry]

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.

[dopry]

@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]

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

[dopry]

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]

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]

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

[hansegucker]

Hi @dopry, is there any update?

[dopry]

No @jezdez still hasn't transferred the repo.

[dopry]

@lullis org is transferred. Can you rebase this PR on the latest master?

[codecov]

Codecov Report

❌ Patch coverage is 96.96970% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
oauth2_provider/models.py	93.54%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[dopry]

@lullis it looks like we still have some pretty big gaps in test code coverage for the backchannel logout functionality. Will you have time to work on that? Is there another of your PRs that you would want to prioritize?

[lullis]

@dopry , I've increased the test coverage as much as I could, but codecov is hung up on one line on application/models.py not being covered. Any tips on the best approach to test that exceptions are being caught in the middle of a function, or is it okay as it is?

[dopry]

@lullis I would mock id_token.send_backchannel_logout_request() to throw an exception along the lines of

from unittest.mock import patch, Mock
with patch('your_module.your_function') as mock_func: 
    mock_func.side_effect = ValueError("Something went wrong!")

before exercising send_backchannel_logout_requests()

Here are some links that might shed some light on how to test exceptions.

https://docs.python.org/3/library/unittest.mock.html
https://stackoverflow.com/questions/28305406/mocking-a-function-to-raise-an-exception-to-test-an-except-block