Refactor RPInitiatedLogoutView

[tonial]

Description of the Change

Following this comment : #1270 (comment) I tried to refactor the view.

• removing post_logout_redirect_uri from validate_logout_request outputs
• adding a must_prompt method in the view (which would additionally allow anybody to easily override it)

@dopry I didn't have a revelation on how to completely clean those multiple variables.
The fact that we need to call validate_logout_request() from both form_valid() and 'get() does not help.

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

[codecov]

Codecov Report

Merging #1274 (1e7bcf4) into master (0965100) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1274      +/-   ##
==========================================
- Coverage   97.39%   97.34%   -0.05%     
==========================================
  Files          32       32              
  Lines        2035     2073      +38     
==========================================
+ Hits         1982     2018      +36     
- Misses         53       55       +2     

Files Changed	Coverage Δ
oauth2_provider/views/oidc.py	99.15% <100.00%> (-0.85%)	⬇️

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

[dopry]

I'm sorry I haven't been present the last month or so. I've been dealing with some personal issues. I'm catching up on my OSS backlog the next two weeks. I should get to this review next week. Thank you for your patience.

[francoisfreitag]

Nice improvement, helps with overriding the prompt in subclasses. 👍

[n2ygk]

One immediate concern about redefining the return value of a public method.

[n2ygk]

This adds a lot of lines of code that approve logout functionality, but without added documentation, it's not clear to me if DOT users who would benefit from this new functionality without reading the source code. Is the current documentation of RP-Initiated Logout sufficient or is there new configuration info that it would help to document?

[tonial]

This PR intends to make the code easier to read and to override. Here are the two reasons why :

• The first is linked to this comment you made in the initial logout PR which I share.
• The second is because in my own project using OIDC I had to override this function and it's not easy or clean (see here if you're interested).

Regarding the documentation : I feel like the current documentation is already enough.
Looking at the comments in the project PRs or issues, users of DOT already know how to override the views to customize OIDC, even if it's not in the documentation.

Maybe we could add a part explaining how to easily override OIDC views, by defining a urlpattern that replaces oauth2_provider/urls.py (like I did here).
Do you think it would help ?

[n2ygk]

Thanks @tonial. I believe that was @dopry's comment. It appears they have become too busy lately and I am trying to bring this PR to closure without a deep understanding of this part of OIDC.

Yes, a short example with the urls, etc. that would help people understand how to configure it would be helpful I think.

[dopry]

@n2ygk thanks for pinging me. My time is freeing up with my kiddo back in school and summer break coming to an end. I'll dive into this today.

[tonial]

Oh, I'm sorry for mixing up your usernames... 🤦

I've added some documentation in the Advanced topics.

[dopry]

This is a move in the right direction. I think we could break out a few additional methods to make the logout validation a little simpler and allow developers to surgically override id_token/client validation and redirect_uri validation. I could see someone overriding user/client validation to send events to security logging infrastructure. I could see post_logout_redirect_uri validation being overriding to hard code restraints at a global level independent of Application.

@n2ygk there isn't any new configuration here, just some implementation refactoring to make it more flexible for developers.

I don't think we need to worry too much about documenting how to extend the view class or override urlpatterns. I hope most developers who are getting tasked with OIDC Provider implementations are savvy with regard to Django routing and extending classes to override methods in python. At the very least I feel that is outside of the scope of this issue and should be a separate documentation PR

[tonial]

@dopry I've split the validation as you suggested.

I even added a get_request_application() so that validate_logout_request() only calls several methods.

[dopry]

@tonial This is looking very svelte now! From just a code review perspective we're looking very good. I'll run it through it's paces early next week.

[dopry]

@tonial looking at codecov, it looks like we're missing coverage for a few lines as well... https://app.codecov.io/gh/jazzband/django-oauth-toolkit/pull/1274/blob/oauth2_provider/views/oidc.py

I don't think I'd block merging this over those 2 branches, but they'd be super nice to have.

[n2ygk]

Hey thanks for improving the docs as well as the RP Initiated logout view