Websocket auth with token as query param #234
Conversation
|
I used |
|
Thanks for submitting this @rolweber. I'll have a pass through it soon. @minrk informed me yesterday that there is a long-term token capability in notebook 4.3+, not just the one time use token I noted in #211. I'm not sure if you found that or implemented your own (I haven't looked at the PR in depth yet), but it's something we should use if we can. |
|
@parente: Thanks. My PR doesn't generate tokens, it just verifies them. The difference is that it not only looks at the Authorization header, but also at query params. The token still has to be configured via KG_AUTH_TOKEN or similar. |
kernel_gateway/mixins.py
Outdated
| @@ -54,8 +55,14 @@ def prepare(self): | |||
| """ | |||
| server_token = self.settings.get('kg_auth_token') | |||
| if server_token: | |||
| client_token = self.request.headers.get('Authorization') | |||
| if client_token != 'token %s' % server_token: | |||
| client_token = self.get_argument('token', '') | |||
There was a problem hiding this comment.
Should probably default to None, not empty string.
kernel_gateway/mixins.py
Outdated
| if client_token == '': | ||
| client_token = self.request.headers.get('Authorization') | ||
| if client_token and client_token.startswith('token '): | ||
| client_token = client_token[len('token '):] |
There was a problem hiding this comment.
len('token ') should probably be computed once and stored a global in this module to avoid recompute it on every request.
| from kernel_gateway.mixins import JSONErrorsMixin | ||
| from kernel_gateway.mixins import TokenAuthorizationMixin, JSONErrorsMixin | ||
|
|
||
| class SuperTokenAuthHandler(object): |
There was a problem hiding this comment.
Is there a reason for defining this class just to have a prepare() instead of implementing prepare in TestableTokenAuthHandler which is the only subclass?
There was a problem hiding this comment.
I had to put it in a base class, because the tested method from the mixin calls super.prepare() in the good case. Maybe there's a more elegant way to do it, I still have to familiarize myself with Python.
@rolweber after reading through the jupyter/notebook master code, I still think the code in KG for generating and verifying tokens, plus the code in the PR here for checking query parameters for tokens, is redundant. https://github.com/jupyter/notebook/blob/68a514a29d0e79af66ce6dd75c87e991fb58126d/notebook/auth/login.py#L148, which feeds all of the |
|
@parente: You're probably right, but that's what issue #211 is about. I opened issue #234 with a smaller scope, because I'm trying to find my way into a code base I'm not familiar with, in a programming language I'm not familiar with. Issue #211 is simply out of my league at the moment. I'll update the PR with your suggested improvements some time this week. |
|
@rolweber Don't get me wrong: My notes above aren't meant to preclude merging this and then iterating on how to switch to what's in notebook server over time. I appreciate the PR you put together! |
|
@parente: Thanks. The code is updated now. |
|
ping? |
kernel_gateway/mixins.py
Outdated
| client_token = self.get_argument('token', '') | ||
| if client_token == '': | ||
| client_token = self.get_argument('token', None) | ||
| if client_token == None: |
|
@rolweber Missed the update from 7 days ago. Thanks for the ping. I had just one further piece of feedback, then we can merge. |
|
@parente: Thanks! I'll try to find time for backporting this change to 1.2.x tomorrow. |
|
@rolweber In it goes! 🍰 Would you like to be added as a maintainer to this repo so that you can help review and merge PRs as well? |
|
@parente: Great! It's been a while since I could last contribute to somebody else's OSS project :-) Yes, it would be cool if I could become a maintainer here. |
|
Whelp, it's your project now too. Welcome to the 🎉 . |
Fixes #233 for version 2.0: