Implement OIDC RP-Initiated Logout

[Qup42]

Fixes #1131

Description of the Change

Implement OIDC RP-Initiated Logout.

Regarding functionality the code is mostly finished. I am not yet satisfied with the structure of some parts of the code and would appreciate feedback or ideas to improve these points. Most functionality that django-oauth-toolkit provides is backed by oauthlib. This is not the case with the RP-Initiated Logout and I think that most of my uncertainty comes from that.

• The handling of exceptions. I am not quite happy with the new exception classes I introduced. But on the other hand the errors in other parts of django-oauth-toolkit are just passed on from oauthlib. The exceptions that are passend on don't require new exception classes.
• Location of validate_logout_request. For RP-Initiated Logout the actual core logic has to be implement in django-oauth-toolkit. Most other functionality can be forwarded to oauthlib via already existing classes (OAuthLibCore, etc.). But maybe there is a better place for this function.

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
• Review Exceptions
• Review validate_logout_request
• Test two untested cases in views/oidc.py
• Adapt code for Django 2.2

[codecov]

Codecov Report

Merging #1244 (8413052) into master (25f6de5) will increase coverage by 0.26%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1244      +/-   ##
==========================================
+ Coverage   96.97%   97.24%   +0.26%     
==========================================
  Files          31       31              
  Lines        1821     1994     +173     
==========================================
+ Hits         1766     1939     +173     
  Misses         55       55              

Impacted Files	Coverage Δ
oauth2_provider/settings.py	100.00% <ø> (ø)
oauth2_provider/urls.py	100.00% <ø> (ø)
oauth2_provider/exceptions.py	100.00% <100.00%> (ø)
oauth2_provider/forms.py	100.00% <100.00%> (ø)
..._provider/management/commands/createapplication.py	100.00% <100.00%> (ø)
oauth2_provider/models.py	98.55% <100.00%> (+0.01%)	⬆️
oauth2_provider/views/__init__.py	100.00% <100.00%> (ø)
oauth2_provider/views/mixins.py	99.29% <100.00%> (+0.04%)	⬆️
oauth2_provider/views/oidc.py	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[Qup42]

There is one thing causing the tests to fail:

1. Django 2.2 has a different access to the Response Headers than what I am used to. I will fix that. Fixed with a9667dc
2. Django has dropped support for Python <3.10 on the main branch with a commit from 18/01/2023. Fixing that is probably out of scope of this PR. Errors while running the tests with djmain are ignored. Fixed with Remove incompatible python+django version combinations from CI #1245.

[Qup42]

This PR should now be ready for a review. If I can do anything to make the review easier for you (e.g. deploy a demo app with the code, etc.), I'd be happy to help.
The build failure is caused by yet another external change (this time flake8-black). This should probably again be a separate PR.

[Andrew-Chen-Wang]

@Qup42 simply run black and your lint errors will probably be fine? Would be helpful to post what you get.

[Qup42]

@Qup42 simply run black and your lint errors will probably be fine? Would be helpful to post what you get.

Yes I have fixed them locally. I just wanted to state why the builds fail, as the lint errors are in locations that didn't change with this PR. So I was hesitant to add them to this PR and didn't push the changes yet.

[dopry]

@Qup42 Thank you so much for submitting this PR. I can't test it today, but I'll review first thing next week. I did do a quick scan over the code and the general gist looks good. A few thoughts came to mind

1. the url path is a bit long winded. is there a reason not to use /o/logout?
2. should enabling rp initiated logout be global or per app?
3. should a valid user session also be acceptable to allow logout if a client_id and hint are not provided?

[Qup42]

1. I will change it to logout/

2. That's an aspect I haven't thought about until now. It seems to me that the specs only considered the case of enabling it globally. I would prefer to stick to global enabling unless there is a common use case where this is required. If it is configurable per app some questions would arise as this situation is not standardized. E.g.

• The Discovery Endpoint is global and has no way to discern apps that may or may not have logout enabled. But optimally it should only show the logout URL to apps that also have it enabled. Alternatively should there be a way for an application to test whether remote logout is enabled for it?
• What is the behavior if an app tries to logout a user but does not have logout enabled? Automatic logout should probably not be possible. But should the user still be asked to logout and the return URL is also ignored or just show an error?

3. Not if we want to be compliant with the specs. Specs:

At the Logout Endpoint, the OP SHOULD ask the End-User whether to log out of the OP as well. Furthermore, the OP MUST ask the End-User this question if an id_token_hint was not provided or if the supplied ID Token does not belong to the current OP session with the RP and/or currently logged in End-User.

and

6. Security Considerations
   Logout requests without a valid id_token_hint value are a potential means of denial of service; therefore, OPs should obtain explicit confirmation from the End-User before acting upon them.

[dopry]

1. thanks.
2. Awesome job looking into the specs, and I concur that the discovery endpoint spec only seems to contemplate global logout support.
3. I wasn't asking about whether to prompt or not. I was wondering what happens when an id_token and client_hint are not provided. I see that the request passes through so it looks like prompting is required in that case.
4. What happens if a user is not authenticated and provides a hint and token? Can a user be maliciously logged out by an actor that has exfiltrated an expired token. does django's logout function verify the session is valid and matching before logging out a user? or should this implementation do that?

[dopry]

@Qup42 I've shared a gist of an end session endpoint I implemented for a project. I didn't implement the user confirmation part of the workflow, but I do have a sample of expired token validation you can grab and maybe a few other things... https://gist.github.com/dopry/36e8cb676b08697ad26612521993b57d

[n2ygk]

@Qup42 FYI I've just rebased this

[Qup42]

@Qup42 I've shared a gist of an end session endpoint I implemented for a project. I didn't implement the user confirmation part of the workflow, but I do have a sample of expired token validation you can grab and maybe a few other things... https://gist.github.com/dopry/36e8cb676b08697ad26612521993b57d

Looking at your gist I see that you delete the access tokens. Previously libraries built on top of django-oauth-toolkit would do the actual logout. Now that we actually also logout a user this has to be considered.

I'll integrate the token handling and token deletion from your gist in about three weeks once I'm done with my exams.

[Qup42]

3. I wasn't asking about whether to prompt or not. I was wondering what happens when an id_token and client_hint are not provided. I see the pass the request through so it looks like prompting is required in that case. What happens if a user is not authenticated and provides a hint and token? Can a user be maliciously logged out by an actor that has exfiltrated an expired token. does django's logout function verify there the session is valid before logging out a user or should this implementation do that?

• RP-Initiated Logout should be idempotent. When hint and token are provided logout can happen without interaction. So if a user is already logged out nothing should happen (it is probably a good idea to try to delete the access tokens nonetheless?).
• Yes. The specs allow this case and even encourage accepting time-expired tokens. But there is no obligation to accept expired tokens.
• Django's logout does nothing when no user is logged in. The user_logged_out signal seems to be invoked regardless of whether someone was actually logged out.

[Qup42]

@Qup42 I've shared a gist of an end session endpoint I implemented for a project. I didn't implement the user confirmation part of the workflow, but I do have a sample of expired token validation you can grab and maybe a few other things... https://gist.github.com/dopry/36e8cb676b08697ad26612521993b57d

Regarding expired tokens: have you tested that the code in your snipped does actually accept expired tokens? (Shouldn't JWTExpired be raised via JWT::init -> JWT::deserialize -> JWT::validate -> JWT::_check_provided_claims -> JWT::_check_default_claims -> JWT::_check_exp?)

Anyways ... if the code already accepts expired tokens - great. Otherwise check_claims=False in the constructor of JWT disables the check for the expiry date and the not before timestamp.

[dopry]

Thanks for the QC on the JWT check,,, you're right... I looked back at the blame and check_claims=False and key=key got lost in a mismerge in another feature branch... sigh... makes me wish I had written the tests when I wrote the code.

I think deleting tokens would probably be good. I used Oauth2AccessToken.objects.filter(user=request.user).delete(). I'd want to check that request wouldn't be too greedy and delete things like client credentials created by a user that should be persistent. I don't have an m2m or client credentials in my environment.

[dopry]

Looking good, Overloading _load_id_token with both validation and token loading caught my attention at first. Applying the single resposibility prinicpal here would lead us to decompose validating the provided token and loading a token into to two separate functions. It is paralell to the oauth2_validators._load_id_token, so there is a an argument for maintaining consistency, so I would approve this.

@Qup42 any plans on implementing token removal along side logout?

[Qup42]

Looking good, Overloading _load_id_token with both validation and token loading caught my attention at first. Applying the single resposibility prinicpal here would lead us to decompose validating the provided token and loading a token into to two separate functions. It is paralell to the oauth2_validators._load_id_token, so there is a an argument for maintaining consistency, so I would approve this.

@Qup42 any plans on implementing token removal along side logout?

Thanks for having a look. I also wasn't quite satisfied with that function. I have now split it into two: loading the IDToken and validating the claims. The difference to the validator is that the validation doesn't happen on the IDToken object. For validation jwt.JWT or the claims are required. Both can't be extracted from the IDToken.

No worries - I haven't forgotten the token removal ;) I will probably implement it in the next couple days.

[Qup42]

It can now be configured whether tokens should be deleted at all and for which types of clients (public/confidential) and authorization grant types.

Deleting tokens just because a User logs seems to be a bit against the idea of the tokens to me, especially for IDTokens. Currently I also delete the IDTokens of the user that is being logged out. I'm unsure about that aspect. Do you have any input on whether the IDTokens should also be deleted or kept? @dopry

[dopry]

@Qup42 thanks for your work on this. I'm really looking forward to getting it in. I think a lot of people can benefit from this work.

Deleting tokens just because a User logs seems to be a bit against the idea of the tokens to me, especially for IDTokens. Currently I also delete the IDTokens of the user that is being logged out. I'm unsure about that aspect. Do you have any input on whether the IDTokens should also be deleted or kept? @dopry

I'm not working from the concept of a token and how they're intended to be used, but from the Users objective of being logged out. If a user has requested to be logged out of a specific RP or at the IDP, I suspect their expectation is that their session is ended and that their 'live' PII is fully invalidated and wiped. When I log out of a server side session based app, my session is fully deleted along with any identification info contained there in. It may still live at rest in the DB, but the sessions and live copy of the PII is gone. From a more practical perspective, I've seen a number of RP implementations use an IDToken as an auth token. Not saying it's right or should be actively supported, but I've seen it and we should consider it. Not all RPs will validate an ID Token on every request or before expiration anyway. They would re-request it on expiration at which point they probably couldn't get it without a valid auth token or auth session. RPs could also just continue to used the 'invalidated' token. Nothing is stopping then, I feel like it's best to tell RPs that the ID token is no longer valid of they try to check one once a use has actively ended their session. This is just my opinion. I could be overlooking cases like offline RP access to user data etc that aren't a typical part of the workflows I use.

Since we have opposing views, I think we should try to lean back on spec to determine how to handle this scenario. Based on your reading of OAUTH related specs and tokens, why do you feel that ID Tokens shouldn't be deleted?

[dopry]

@Qup42 I wanted to check in. I've been busy the last few weeks. I think it's okay to delete the tokens. In what cases would you not want to delete them?

[Qup42]

@dopry Hmm I guess you're right. Doing what most users expect is probably the better approach here. The deletion of the tokens can also be turned of should that be required.

I also merged the master branch once more. What would be the next step going forward?

[dopry]

Everything looks good from a code review perspective. I just wanted to be sure you felt it was complete before I dove into testing. I'll try to get to it today.

[dopry]

@Qup42 I'm starting into testing today. I rebased your PR so the feature history will be linear.

[dopry]

Found a small thing. I'll get it if all goes well in further testing.

[Qup42]

@dopry I'll wait for you to finish reviewing and integrate the changes after that in bulk.

[dopry]

@Qup42 I've been testing, so far everything looks good. I'm still working on getting an RP setup to do a full integration test with a 3rd party oidc client library.

[n2ygk]

@dopry This might help with testing: https://github.com/n2ygk/dot-tutorial/blob/main/certification/notes.md

The heroku app is not currently set up but I can do that.

[dopry]

I was setting up an example IDP (django + django_oauth_toolkit) and RP (https://github.com/dopry/svelte-oidc) locally per #1266 to facilitate testing. My svelte-oidc project needed some love and I thought I'd kill two bird with one stone.

[tonial]

Hi.

First of all, I'm really glad to that you worked on this feature for DOT.
It was clearly not a small task, and I'm sure it will help a lot of people (including me !)

I've done some testing and I think there's a issue regarding logout without prompt sent from the RP server, so I added some comments in your pull request.

[dopry]

@Qup42 I've completed my review and it's working splendidly. The last changes are

• handling token parsing exceptions and noted by @tonial
• documentation typo I noted
• remove https validation from post_logout_redirect_uri and explain the trade off in the help text for AbtractApplication.post_logout_redirect_uri

[dopry]

l love the solution by using a strict setting. There is one typo in the docs, If you can get around to fixing it that would be awesome. I gotta run to a meeting, if you haven't gotten to it before I'm back I'll update it when I do a final testing pass.

[dopry]

LGTM.

[dopry]

@Qup42 Thank you so much for all your work on this. It is very appreciated.