HewlettPackard / jupyterhub-samlauthenticator

jupyterhub-samlauthenticator
MIT License
37 stars 26 forks source link

Put bad attribute back #50

Closed jpobley closed 4 years ago

jpobley commented 4 years ago

The current fix for backwards compatibility breaks the logout flow.

traitlets will ignore any attribute in jupyterhub_config.py that's not defined on the class. When the typo was fixed, it meant that the bad attribute name couldn't be set on the class anymore.

Currently, when the user logs out, the authenticator will check it and raise an AttributeError, sending a 500: Internal Server Error to the UI.

[I 2020-04-21 21:05:01.922 JupyterHub samlauthenticator:732] User logged out: test.user
[E 2020-04-21 21:05:01.927 JupyterHub web:1792] Uncaught exception GET /hub/logout (::ffff:172.19.0.1)
    HTTPServerRequest(protocol='http', host='localhost:8000', method='GET', uri='/hub/logout', version='HTTP/1.1', remote_ip='::ffff:172.19.0.1')
    Traceback (most recent call last):
      File "/usr/local/lib/python3.6/dist-packages/tornado/web.py", line 1703, in _execute
        result = await result
      File "/usr/local/lib/python3.6/dist-packages/samlauthenticator/samlauthenticator.py", line 754, in get
        forwad_on_logout = True if authenticator_self.slo_forwad_on_logout else False
    AttributeError: 'SAMLAuthenticator' object has no attribute 'slo_forwad_on_logout'

Screen Shot 2020-04-21 at 10 54 11 PM

Developer Certificate of Origin Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors. 660 York Street, Suite 102, San Francisco, CA 94110 USA

Everyone is permitted to copy and distribute verbatim copies of this license document, but changing it is not allowed.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or

(b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or

(c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it.

(d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved.

Signed-off-by: JP Obley jpobley@gmail.com

distortedsignal commented 4 years ago

Good catch. Is there a way that we can check this in a test?

jpobley commented 4 years ago

Good catch. Is there a way that we can check this in a test?

Hmmm. A test for the configuration or the handler? Testing configuration feels like we'd be testing traitlets. Your instinct is right, though: this bug definitely would have been caught by a test case for the handler.

Writing a test for any of the handlers is made complicated by the handlers themselves being defined in the SAMLAuthenticator.get_handlers() method. We could move the handler definitions out of that function, either to a new module or keep them in the same one, and then have their __init__ methods take the authenticator instance as an argument.

What do you think? It would increase the size of this PR, but I can take a stab at it between changing diapers.

distortedsignal commented 4 years ago

Nah - I think this is fine. Thanks for the work. I'll try to get this up later today.