djangosaml2idp icon indicating copy to clipboard operation
djangosaml2idp copied to clipboard

Logout bug

Open Amertz08 opened this issue 3 years ago • 6 comments

I'm still running djangosaml2idp==0.6.3 and running into an issue w/ the logout view. It looks like the resp is already a string and not an object causing an exception to be thrown.

  try:
      # hinfo returns request or response, it depends by request arg
      hinfo = self.IDP.apply_binding(binding, resp.__str__(), resp.destination, relay_state, response=True)
  except Exception as excp:
      logger.error("ServiceError: %s", excp)
      return self.handle_error(request, exception=excp, status=400)

Specifically resp.destination is causing the issue.

ServiceError: 'str' object has no attribute 'destination'

resp is set prior via an IDP method.

resp = self.IDP.create_logout_response(req_info.message, [binding])

I was looking through the newest code and it appears this might have been fixed already. Not entirely sure.

Amertz08 avatar Mar 04 '21 15:03 Amertz08

Digging into this further it appears that because we have "sign_response": True, set in settings.py the IDP.create_logout_response is returning the fully rendered XML instead of the response object. Thus causing the exception to happen. We cannot currently move to the models and thus we're stuck on 0.6.3. I basically plan on patching the view and then injecting it into our urls.py. I'm not entirely sure how to go about making the changes correctly.

Amertz08 avatar Mar 04 '21 16:03 Amertz08

This bug for sure still exists in the most recent version. create_logout_response calls _status_response which returns either the class or the string representation of the class. The view goes forward assuming it got the object and not the string.

Amertz08 avatar Mar 31 '21 17:03 Amertz08

+1 with the same issue

longshine avatar May 12 '21 13:05 longshine

@longshine I've been meaning to hop back on and get my PR fixed. What we did since we're stuck on 0.6.3 is just copy the view code over and implement the fix. Then interject into the urls.py to use our view before the packaged defined view.

Amertz08 avatar May 14 '21 17:05 Amertz08

I ran into the same problem with djangosaml2idp==0.7.2 and pysaml2==7.0.1. I'm not sure what the proper fix is for this problem, but the following worked for me:

--- site-packages/djangosaml2idp/views.py.orig	2021-07-05 17:40:31.471614643 +0200
+++ site-packages/djangosaml2idp/views.py	2021-08-24 16:30:34.455854131 +0200
@@ -375,6 +375,7 @@
             return error_cbv.handle_error(request, exception=excp, status_code=400)
 
         resp = idp_server.create_logout_response(req_info.message, [binding])
+        rinfo = idp_server.response_args(req_info.message, [binding])
 
         '''
         # TODO: SOAP
@@ -390,13 +391,13 @@
 
         try:
             # hinfo returns request or response, it depends by request arg
-            hinfo = idp_server.apply_binding(binding, resp.__str__(), resp.destination, relay_state, response=True)
+            hinfo = idp_server.apply_binding(rinfo["binding"], resp, rinfo["destination"], relay_state, response=True)
         except Exception as excp:
             logger.error("ServiceError: %s", excp)
             return error_cbv.handle_error(request, exception=excp, status=400)
 
-        logger.debug("--- {} Response [\n{}] ---".format(self.__service_name, repr_saml(resp.__str__().encode())))
-        logger.debug("--- binding: {} destination:{} relay_state:{} ---".format(binding, resp.destination, relay_state))
+        logger.debug("--- {} Response [\n{}] ---".format(self.__service_name, repr_saml(resp.encode())))
+        logger.debug("--- binding: {} destination:{} relay_state:{} ---".format(rinfo["binding"], rinfo["destination"], relay_state))
 
         # TODO: double check username session and saml login request
         # logout user from IDP

This fix was inspired by code that I found in pysaml2: site-packages/saml2/client.py, line 678.

marco-websource avatar Aug 24 '21 14:08 marco-websource

+1 same issue

charron-tom avatar Sep 30 '21 18:09 charron-tom