-
Notifications
You must be signed in to change notification settings - Fork 135
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
Websocket auth with token as query param #234
Changes from 6 commits
04833c6
c06ff3b
2f4f2e0
e897143
3360881
7166c78
226268c
6598e17
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,10 +40,11 @@ class TokenAuthorizationMixin(object): | |
tornado.websocket.WebsocketHandlers. | ||
""" | ||
def prepare(self): | ||
"""Ensures the correct `Authorization: token <value>` is present in | ||
the request's header if an auth token is configured. | ||
"""Ensures the correct auth token is present, either as a parameter | ||
`token=<value>` or as a header `Authorization: token <value>`. | ||
Does nothing unless an auth token is configured in kg_auth_token. | ||
|
||
If kg_auth_token is set and the token is not in the header, responds | ||
If kg_auth_token is set and the token is not present, responds | ||
with 401 Unauthorized. | ||
|
||
Notes | ||
|
@@ -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', '') | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
else: | ||
client_token = None | ||
if client_token != server_token: | ||
return self.send_error(401) | ||
return super(TokenAuthorizationMixin, self).prepare() | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,10 +4,125 @@ | |
|
||
import json | ||
import unittest | ||
|
||
try: | ||
from unittest.mock import Mock, MagicMock | ||
except ImportError: | ||
# Python 2.7: use backport | ||
from mock import Mock, MagicMock | ||
|
||
from tornado import web | ||
from kernel_gateway.mixins import JSONErrorsMixin | ||
from kernel_gateway.mixins import TokenAuthorizationMixin, JSONErrorsMixin | ||
|
||
class SuperTokenAuthHandler(object): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason for defining this class just to have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
"""Super class for the handler using TokenAuthorizationMixin.""" | ||
is_prepared = False | ||
|
||
def prepare(self): | ||
# called by the mixin when authentication succeeds | ||
self.is_prepared = True | ||
|
||
class TestableTokenAuthHandler(TokenAuthorizationMixin, SuperTokenAuthHandler): | ||
"""Implementation that uses the TokenAuthorizationMixin for testing.""" | ||
def __init__(self, token=''): | ||
self.settings = { 'kg_auth_token': token } | ||
self.arguments = {} | ||
self.response = None | ||
self.status_code = None | ||
|
||
def send_error(self, status_code): | ||
self.status_code = status_code | ||
|
||
def get_argument(self, name, default=''): | ||
return self.arguments.get(name, default) | ||
|
||
|
||
class TestTokenAuthMixin(unittest.TestCase): | ||
"""Unit tests the Token authorization mixin.""" | ||
def setUp(self): | ||
"""Creates a handler that uses the mixin.""" | ||
self.mixin = TestableTokenAuthHandler('YouKnowMe') | ||
|
||
def test_no_token_required(self): | ||
"""Status should be None.""" | ||
self.mixin.settings['kg_auth_token'] = '' | ||
self.mixin.prepare() | ||
self.assertEqual(self.mixin.is_prepared, True) | ||
self.assertEqual(self.mixin.status_code, None) | ||
|
||
def test_missing_token(self): | ||
"""Status should be 'unauthorized'.""" | ||
attrs = { 'headers' : { | ||
} } | ||
self.mixin.request = Mock(**attrs) | ||
self.mixin.prepare() | ||
self.assertEqual(self.mixin.is_prepared, False) | ||
self.assertEqual(self.mixin.status_code, 401) | ||
|
||
def test_valid_header_token(self): | ||
"""Status should be None.""" | ||
attrs = { 'headers' : { | ||
'Authorization' : 'token YouKnowMe' | ||
} } | ||
self.mixin.request = Mock(**attrs) | ||
self.mixin.prepare() | ||
self.assertEqual(self.mixin.is_prepared, True) | ||
self.assertEqual(self.mixin.status_code, None) | ||
|
||
def test_wrong_header_token(self): | ||
"""Status should be 'unauthorized'.""" | ||
attrs = { 'headers' : { | ||
'Authorization' : 'token NeverHeardOf' | ||
} } | ||
self.mixin.request = Mock(**attrs) | ||
self.mixin.prepare() | ||
self.assertEqual(self.mixin.is_prepared, False) | ||
self.assertEqual(self.mixin.status_code, 401) | ||
|
||
def test_valid_url_token(self): | ||
"""Status should be None.""" | ||
self.mixin.arguments['token'] = 'YouKnowMe' | ||
attrs = { 'headers' : { | ||
} } | ||
self.mixin.request = Mock(**attrs) | ||
self.mixin.prepare() | ||
self.assertEqual(self.mixin.is_prepared, True) | ||
self.assertEqual(self.mixin.status_code, None) | ||
|
||
def test_wrong_url_token(self): | ||
"""Status should be 'unauthorized'.""" | ||
self.mixin.arguments['token'] = 'NeverHeardOf' | ||
attrs = { 'headers' : { | ||
} } | ||
self.mixin.request = Mock(**attrs) | ||
self.mixin.prepare() | ||
self.assertEqual(self.mixin.is_prepared, False) | ||
self.assertEqual(self.mixin.status_code, 401) | ||
|
||
def test_differing_tokens_valid_url(self): | ||
"""Status should be None, URL token takes precedence""" | ||
self.mixin.arguments['token'] = 'YouKnowMe' | ||
attrs = { 'headers' : { | ||
'Authorization' : 'token NeverHeardOf' | ||
} } | ||
self.mixin.request = Mock(**attrs) | ||
self.mixin.prepare() | ||
self.assertEqual(self.mixin.is_prepared, True) | ||
self.assertEqual(self.mixin.status_code, None) | ||
|
||
def test_differing_tokens_wrong_url(self): | ||
"""Status should be 'unauthorized', URL token takes precedence""" | ||
attrs = { 'headers' : { | ||
'Authorization' : 'token YouKnowMe' | ||
} } | ||
self.mixin.request = Mock(**attrs) | ||
self.mixin.arguments['token'] = 'NeverHeardOf' | ||
self.mixin.prepare() | ||
self.assertEqual(self.mixin.is_prepared, False) | ||
self.assertEqual(self.mixin.status_code, 401) | ||
|
||
|
||
class TestableHandler(JSONErrorsMixin): | ||
class TestableJSONErrorsHandler(JSONErrorsMixin): | ||
"""Implementation that uses the JSONErrorsMixin for testing.""" | ||
def __init__(self): | ||
self.headers = {} | ||
|
@@ -27,7 +142,7 @@ class TestJSONErrorsMixin(unittest.TestCase): | |
"""Unit tests the JSON errors mixin.""" | ||
def setUp(self): | ||
"""Creates a handler that uses the mixin.""" | ||
self.mixin = TestableHandler() | ||
self.mixin = TestableJSONErrorsHandler() | ||
|
||
def test_status(self): | ||
"""Status should be set on the response.""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably default to None, not empty string.