Don't check for auth token during OPTIONS preflight #258

SimonBiggs · 2017-09-05T12:58:42Z

Details provided within #257

rebase

rolweber · 2017-09-05T13:28:51Z

kernel_gateway/mixins.py

@@ -65,7 +65,7 @@ def prepare(self):
        package.
        """
        server_token = self.settings.get('kg_auth_token')
-        if server_token:
+        if (server_token != '') & (self.request.method != 'OPTIONS'):


That should be and instead of &. Python uses keywords for logical operators.
https://docs.python.org/2/library/stdtypes.html#boolean-operations-and-or-not
https://docs.python.org/2/library/stdtypes.html#bitwise-operations-on-integer-types

Not sure if != '' is the correct check here. Could server_token be None instead of the empty string?

if server_token and self.request.method != 'OPTIONS':

Also looks like a test is failing because self.request does not exist.

Traceback (most recent call last): File "/home/travis/build/jupyter/kernel_gateway/kernel_gateway/tests/test_mixins.py", line 49, in test_no_token_required self.mixin.prepare() File "/home/travis/build/jupyter/kernel_gateway/kernel_gateway/mixins.py", line 68, in prepare if (server_token != '') & (self.request.method != 'OPTIONS'): AttributeError: 'TestableTokenAuthHandler' object has no attribute 'request'

parente · 2017-09-05T19:43:02Z

@SimonBiggs thanks for submitting this. I'm a little miffed why self.request is not available in prepare after glancing at the tornado source for the RequestHandler base class. Maybe it's a test case problem. Try fixing the if-statement per the comments above and let's see where we stand.

If you get stuck debugging the syntax and test problems, feel free to ping for help.

SimonBiggs · 2017-09-05T20:32:53Z

Thanks for the feedback. In https://github.com/jupyter/kernel_gateway/blob/master/kernel_gateway/gatewayapp.py the following is given:

@default('auth_token')
def _auth_token_default(self):
    return os.getenv(self.auth_token_env, '')

I think that makes it so that if auth_token isn't defined it will always be ''. I can explicitly test against '' or if I swap to using logical and instead of & I will also just be able to leave it as if server_token.

I'll update the pull request soon to account for non existing self.requests.method

SimonBiggs · 2017-09-05T21:23:40Z

Is there a way to mock CORS requests within nose? I don't really know where to begin. Anyway, I'm just writing a test to cover this case with OPTIONS, however it would be nice to also test CORS + authentication token directly.

parente · 2017-09-05T23:32:29Z

kernel_gateway/tests/test_jupyter_websocket.py

@@ -150,6 +150,20 @@ def _set_api():
        self.assertRaises(ImportError, _set_api)

    @gen_test
+    def test_options_and_auth_token(self):
+        "OPTIONS requests should not need to submit a token."


Make this triple-quoted so that it's a canonical docstring.

parente · 2017-09-05T23:33:48Z

I haven't ever tried to mock a CORS request with Python. I think the test you've added to see if OPTIONS succeeds when a token is set provides enough test coverage and value.

SimonBiggs · 2017-09-06T01:01:24Z

@parente Thanks for that.

rolweber · 2017-09-06T05:56:22Z

kernel_gateway/mixins.py

@@ -64,8 +64,14 @@ def prepare(self):
        with the `@web.authenticated` decorated methods in the notebook
        package.
        """
+        request_is_options = False
+        if 'request' in dir(self):


This check is not needed, see my latest comment in the conversation.

(original review comment here:)
I'm not too firm in Python myself, but I think the calls to dir() are creating two temporary lists in this section, just to check whether there are attributes. Try the following instead (I haven't checked the exact syntax):

request_is_options = hasattr(self, 'request') and getattr(self.request, 'method', '') == 'OPTIONS'

https://stackoverflow.com/a/610893/5629418
https://stackoverflow.com/a/611708/5629418
This could also be inlined below as

if server_token and not (...):

But looking further below, client_token is also read from self.request without checking whether that exists, and a request without method wouldn't make any sense. I agree with @parente that it is probably a problem with the test setup if self.request doesn't exist.

rolweber · 2017-09-06T06:04:28Z

kernel_gateway/tests/test_jupyter_websocket.py

@@ -150,6 +150,20 @@ def _set_api():
        self.assertRaises(ImportError, _set_api)

    @gen_test
+    def test_options_and_auth_token(self):


Could you please rename this to test_options_without_auth_token?

rolweber · 2017-09-06T06:21:54Z

I found the error message after all...

   AttributeError: 'TestableTokenAuthHandler' object has no attribute 'request'

That's not from a Tornado test, but from some mock I set up to test the auth handler. There is exactly one test which doesn't set the request attribute:
https://github.com/SimonBiggs/kernel_gateway/blob/master/kernel_gateway/tests/test_mixins.py#L46

@SimonBiggs, could you please just copy the attrs = and self.mixin.request = lines from one of the other tests there, and get rid of the check for existence of self.request in the actual code?

rolweber · 2017-09-06T06:32:12Z

@SimonBiggs: On second thought, could you add another test case for OPTIONS to the test_mixins.py file? The idea was to cover the complete logic of the auth mixin there.

SimonBiggs · 2017-09-06T10:00:21Z

@rolweber @parente I think I managed to implement all of the feedback. Hope I didn't miss anything. This is a much nicer result.

rolweber

Yes, this looks very nice. Thanks for flying several rounds :-)

rolweber · 2017-09-06T10:50:04Z

@parente: I'll leave the final look and merge to you, in case I missed something.

parente · 2017-09-06T11:58:18Z

LGTM to me. Thanks @SimonBiggs for the code and tests. 🍰 for your first contrib here.

SimonBiggs · 2017-09-06T17:14:54Z

Thanks :). It's a pleasure :). Might this be able to be pushed out in a patch version release to pip?

Simon Biggs added 2 commits September 5, 2017 22:36

Merge pull request #1 from jupyter/master

08909b9

rebase

Don't check for auth token during OPTIONS preflight

40ef4b4

SimonBiggs mentioned this pull request Sep 5, 2017

Issue with using auth_token and CORS requests #257

Closed

rolweber reviewed Sep 5, 2017

View reviewed changes

SimonBiggs added 3 commits September 6, 2017 07:06

check requests and method exist before testing for OPTIONS http method

7cf9eea

requests to request

044bb50

remove debug prints

7eade65

include test case for options with auth token

b137f07

parente reviewed Sep 5, 2017

View reviewed changes

Fix docstring styling.

03c53b4

rolweber reviewed Sep 6, 2017

View reviewed changes

SimonBiggs added 2 commits September 6, 2017 19:54

fixing tests

da19174

conflict

39cd6e0

rolweber approved these changes Sep 6, 2017

View reviewed changes

parente merged commit e92d5eb into jupyter-server:master Sep 6, 2017

parente mentioned this pull request Sep 9, 2017

Backports for 2.0.1 release #260

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't check for auth token during OPTIONS preflight #258

Don't check for auth token during OPTIONS preflight #258

SimonBiggs commented Sep 5, 2017

rolweber Sep 5, 2017

rolweber Sep 5, 2017 •

edited

Loading

parente Sep 5, 2017

parente Sep 5, 2017

parente commented Sep 5, 2017

SimonBiggs commented Sep 5, 2017 •

edited

Loading

SimonBiggs commented Sep 5, 2017

parente Sep 5, 2017

parente commented Sep 5, 2017

SimonBiggs commented Sep 6, 2017

rolweber Sep 6, 2017 •

edited

Loading

rolweber Sep 6, 2017

rolweber commented Sep 6, 2017 •

edited

Loading

rolweber commented Sep 6, 2017

SimonBiggs commented Sep 6, 2017

rolweber left a comment

rolweber commented Sep 6, 2017

parente commented Sep 6, 2017

SimonBiggs commented Sep 6, 2017

Don't check for auth token during OPTIONS preflight #258

Don't check for auth token during OPTIONS preflight #258

Conversation

SimonBiggs commented Sep 5, 2017

rolweber Sep 5, 2017

Choose a reason for hiding this comment

rolweber Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

parente Sep 5, 2017

Choose a reason for hiding this comment

parente Sep 5, 2017

Choose a reason for hiding this comment

parente commented Sep 5, 2017

SimonBiggs commented Sep 5, 2017 • edited Loading

SimonBiggs commented Sep 5, 2017

parente Sep 5, 2017

Choose a reason for hiding this comment

parente commented Sep 5, 2017

SimonBiggs commented Sep 6, 2017

rolweber Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

rolweber Sep 6, 2017

Choose a reason for hiding this comment

rolweber commented Sep 6, 2017 • edited Loading

rolweber commented Sep 6, 2017

SimonBiggs commented Sep 6, 2017

rolweber left a comment

Choose a reason for hiding this comment

rolweber commented Sep 6, 2017

parente commented Sep 6, 2017

SimonBiggs commented Sep 6, 2017

rolweber Sep 5, 2017 •

edited

Loading

SimonBiggs commented Sep 5, 2017 •

edited

Loading

rolweber Sep 6, 2017 •

edited

Loading

rolweber commented Sep 6, 2017 •

edited

Loading