Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logout bug #121

Open
Amertz08 opened this issue Mar 4, 2021 · 6 comments · May be fixed by #129
Open

Logout bug #121

Amertz08 opened this issue Mar 4, 2021 · 6 comments · May be fixed by #129

Comments

@Amertz08
Copy link
Contributor

Amertz08 commented Mar 4, 2021

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
Copy link
Contributor Author

Amertz08 commented Mar 4, 2021

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
Copy link
Contributor Author

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 Amertz08 linked a pull request Apr 8, 2021 that will close this issue
@longshine
Copy link

+1 with the same issue

@Amertz08
Copy link
Contributor Author

@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
Copy link

MarcoSteinacher commented Aug 24, 2021

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
Copy link

+1 same issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants