OTA-Insight / djangosaml2idp

SAML 2.0 Identity Provider in Django
Apache License 2.0
104 stars 96 forks source link

Logout bug #121

Open Amertz08 opened 3 years ago

Amertz08 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

longshine commented 3 years ago

+1 with the same issue

Amertz08 commented 3 years ago

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

MarcoSteinacher commented 3 years ago

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.

charron-tom commented 2 years ago

+1 same issue