Remove user session instead of setting LOGOUT_SESSION_KEY on logout?

[egroeper]

First: Thanks for this nice application!

May I ask: What are the reasons for checking the value of LOGOUT_SESSION_KEY in the session of the current user in the middleware?
The comment states, that this is needed for logout to work. But in my tests simply deleting the user session in the ShibbolethLogoutView seems to work as well and is cleaner in my opinion:

--- views.py.orig	2017-10-04 21:10:54.571524640 +0000
+++ views.py	2017-10-04 21:10:45.759517570 +0000
@@ -8,6 +8,7 @@
 from django.shortcuts import redirect
 from django.utils.decorators import method_decorator
 from django.views.generic import TemplateView
+from django.contrib.sessions.models import Session
 
 from urllib import quote
 
@@ -69,7 +70,8 @@
         auth.logout(self.request)
         #Set session key that middleware will use to force 
         #Shibboleth reauthentication.
-        self.request.session[LOGOUT_SESSION_KEY] = True
+        #self.request.session[LOGOUT_SESSION_KEY] = True
+        Session.objects.filter(session_key=self.request.session.session_key).delete()
         #Get target url in order of preference.
         target = LOGOUT_REDIRECT_URL or\
                  quote(request.build_absolute_uri())

Am I missing something?
Would you accept a pull request for the change (of course a complete PR, this is only a proof of concept)

[egroeper]

My bad: In my tests everything works, even if I neither set LOGOUT_SESSION_KEY nor delete the user session (which should be done by logout?).

Is LOGOUT_SESSION_KEY a protection against still existing Shibboleth sp session in older Shibboleth installations?

For me redirecting to /Shibboleth.sso/Logout?return=%s (sp SLO endpoint) works perfectly fine and redirects to the IdP SLO endpoint.
That way IdP and SP Shibboleth sessions get deleted / invalidated and I think, that this protection isn't needed in that case.

[bcail]

does the automated LogoutTest still pass with your changes? It may be that certain configurations work fine without the LOGOUT_SESSION_KEY, but others still need them. I'm not sure.

[egroeper]

No it doesn't.
I'm new to this. But looking at the test code, I think, that the test is incorrect (for my usecase).

If REMOTE_USER is set by the webserver, doesn't that mean, that the shibboleth sp of our service has a valid shibboleth session for the user? In that case relogin would be correct, in my opinion.
In my understanding this is simulated by the LogoutTest, which does the same request for (first) login as for checking the logout.

This brings me back to my question:

Is LOGOUT_SESSION_KEY a protection against still existing Shibboleth sp session in older Shibboleth installations?

If the login shall not work, although all headers are sent correctly, the answer to this question is probably "yes".

In current shibboleth setups with SLO support, it is possible to end the shibboleth sp session and the shibboleth idp session by visiting the sp SLO endpoint, that then redirects to the idp slo endpoint.

[lawlesst]

Is LOGOUT_SESSION_KEY a protection against still existing Shibboleth sp session in older Shibboleth installations?

Yes, if I recall correctly. When this was implemented, the support for logout was a bit murky. Seems like things have changed. I agree that deleting the session is cleaner.

[egroeper]

Yes, if I recall correctly. When this was implemented, the support for logout was a bit murky. Seems like things have changed. I agree that deleting the session is cleaner.

No. This was an error of mine. Deleting the session shouldn't help. You either need the "hack" of setting LOGOUT_SESSION_KEY or you don't need anything at all. auth.logout(self.request) should take care of the django session.

Redirecting to the sp SLO endpoint (via LOGOUT_URL) takes care of the Shibboleth sessions (sp and idp).

So the only remaining question is: Should all LOGOUT_SESSION_KEY-stuff be removed or are there sites around, that still need this workaround?

[bcail]

Seems like maybe we can remove the LOGOUT_SESSION_KEY code. The Logout view already logs the user out at the django level, so as long as the redirect url forces Shib re-authentication, we may be OK. It might be a matter of making sure the rights URLs in the app are forces to require a valid Shib session.

@egroeper if you want to submit a PR for this, I'll take a look at it.

[egroeper]

@bcail Done.