From 85eab0c6cb38e303524fc1a80054e3894572d1d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Tue, 11 Sep 2018 16:11:44 +0200 Subject: [PATCH] No longer hash auth tokens + store the token scopes This partially reverts commit c57aefd48b10ca3cabc9df162bc32efa62a6a21e. The code to check against hashed tokens remains, because existing tokens should still work. The unhashed tokens are necessary for fetching badges from Blender ID. --- pillar/api/eve_settings.py | 4 ++-- pillar/api/local_auth.py | 11 ++------- pillar/api/utils/authentication.py | 6 ++--- pillar/config.py | 1 + tests/test_api/test_auth.py | 25 +++++++++++---------- tests/test_api/test_blender_id_subclient.py | 12 +++------- tests/test_api/test_local_auth.py | 4 +--- tests/test_api/test_organizations.py | 7 ++---- 8 files changed, 27 insertions(+), 43 deletions(-) diff --git a/pillar/api/eve_settings.py b/pillar/api/eve_settings.py index 0fcb9200..a39caf3b 100644 --- a/pillar/api/eve_settings.py +++ b/pillar/api/eve_settings.py @@ -368,11 +368,11 @@ tokens_schema = { }, 'token': { 'type': 'string', - 'required': False, + 'required': True, }, 'token_hashed': { 'type': 'string', - 'required': True, + 'required': False, }, 'expire_time': { 'type': 'datetime', diff --git a/pillar/api/local_auth.py b/pillar/api/local_auth.py index 8b3fe1bc..a0d8f1c4 100644 --- a/pillar/api/local_auth.py +++ b/pillar/api/local_auth.py @@ -94,17 +94,10 @@ def generate_and_store_token(user_id, days=15, prefix=b'') -> dict: # Use 'xy' as altargs to prevent + and / characters from appearing. # We never have to b64decode the string anyway. - token_bytes = prefix + base64.b64encode(random_bits, altchars=b'xy').strip(b'=') - token = token_bytes.decode('ascii') + token = prefix + base64.b64encode(random_bits, altchars=b'xy').strip(b'=') token_expiry = utcnow() + datetime.timedelta(days=days) - token_data = store_token(user_id, token, token_expiry) - - # Include the token in the returned document so that it can be stored client-side, - # in configuration, etc. - token_data['token'] = token - - return token_data + return store_token(user_id, token.decode('ascii'), token_expiry) def hash_password(password: str, salt: typing.Union[str, bytes]) -> str: diff --git a/pillar/api/utils/authentication.py b/pillar/api/utils/authentication.py index 8ce39240..9d28cf95 100644 --- a/pillar/api/utils/authentication.py +++ b/pillar/api/utils/authentication.py @@ -200,7 +200,7 @@ def remove_token(token: str): tokens_coll = current_app.db('tokens') token_hashed = hash_auth_token(token) - # TODO: remove matching on unhashed tokens once all tokens have been hashed. + # TODO: remove matching on hashed tokens once all hashed tokens have expired. lookup = {'$or': [{'token': token}, {'token_hashed': token_hashed}]} del_res = tokens_coll.delete_many(lookup) log.debug('Removed token %r, matched %d documents', token, del_res.deleted_count) @@ -212,7 +212,7 @@ def find_token(token, is_subclient_token=False, **extra_filters): tokens_coll = current_app.db('tokens') token_hashed = hash_auth_token(token) - # TODO: remove matching on unhashed tokens once all tokens have been hashed. + # TODO: remove matching on hashed tokens once all hashed tokens have expired. lookup = {'$or': [{'token': token}, {'token_hashed': token_hashed}], 'is_subclient_token': True if is_subclient_token else {'$in': [False, None]}, 'expire_time': {"$gt": utcnow()}} @@ -246,7 +246,7 @@ def store_token(user_id, token: str, token_expiry, oauth_subclient_id=False, token_data = { 'user': user_id, - 'token_hashed': hash_auth_token(token), + 'token': token, 'expire_time': token_expiry, } if oauth_subclient_id: diff --git a/pillar/config.py b/pillar/config.py index 7af41916..93e3adad 100644 --- a/pillar/config.py +++ b/pillar/config.py @@ -29,6 +29,7 @@ DEBUG = False SECRET_KEY = '' # Authentication token hashing key. If empty falls back to UTF8-encoded SECRET_KEY with a warning. +# Not used to hash new tokens, but it is used to check pre-existing hashed tokens. AUTH_TOKEN_HMAC_KEY = b'' # Authentication settings diff --git a/tests/test_api/test_auth.py b/tests/test_api/test_auth.py index 13b97dfc..eff1286b 100644 --- a/tests/test_api/test_auth.py +++ b/tests/test_api/test_auth.py @@ -114,15 +114,16 @@ class AuthenticationTests(AbstractPillarTest): past = now - datetime.timedelta(days=1) subclient = self.app.config['BLENDER_ID_SUBCLIENT_ID'] + with self.app.test_request_context(): auth.store_token(user_id, 'nonexpired-main', future, None) auth.store_token(user_id, 'nonexpired-sub', future, subclient) token3 = auth.store_token(user_id, 'expired-sub', past, subclient) - # We should not find the given tokens as unhashed tokens. + # We should find the given tokens as unhashed tokens. tokens_coll = self.app.db('tokens') - self.assertIsNone(tokens_coll.find_one({'token': 'nonespired-main'})) - self.assertIsNone(tokens_coll.find_one({'token': 'nonespired-sub'})) - self.assertIsNone(tokens_coll.find_one({'token': 'expired-sub'})) + self.assertIsNotNone(tokens_coll.find_one({'token': 'nonexpired-main'})) + self.assertIsNotNone(tokens_coll.find_one({'token': 'nonexpired-sub'})) + self.assertIsNotNone(tokens_coll.find_one({'token': 'expired-sub'})) with self.app.test_request_context( headers={'Authorization': self.make_header('nonexpired-main')}): @@ -206,19 +207,19 @@ class AuthenticationTests(AbstractPillarTest): with self.app.test_request_context(): from pillar.api.utils import authentication as auth - tokdat_le = auth.store_token(user_id, 'long-expired', - now - datetime.timedelta(days=365), None) - tokdat_se = auth.store_token(user_id, 'short-expired', - now - datetime.timedelta(seconds=5), None) - tokdat_ne = auth.store_token(user_id, 'not-expired', - now + datetime.timedelta(days=1), None) + auth.store_token(user_id, 'long-expired', + now - datetime.timedelta(days=365), None) + auth.store_token(user_id, 'short-expired', + now - datetime.timedelta(seconds=5), None) + auth.store_token(user_id, 'not-expired', + now + datetime.timedelta(days=1), None) # Validation should clean up old tokens. auth.validate_this_token('je', 'moeder') token_coll = self.app.data.driver.db['tokens'] - self.assertEqual({tokdat_se['token_hashed'], tokdat_ne['token_hashed']}, - {item['token_hashed'] for item in token_coll.find()}) + self.assertEqual({'short-expired', 'not-expired'}, + {item['token'] for item in token_coll.find()}) def test_token_hashing_cli(self): from dateutil.parser import parse diff --git a/tests/test_api/test_blender_id_subclient.py b/tests/test_api/test_blender_id_subclient.py index 6047fcb5..eb465c5c 100644 --- a/tests/test_api/test_blender_id_subclient.py +++ b/tests/test_api/test_blender_id_subclient.py @@ -42,8 +42,6 @@ class BlenderIdSubclientTest(AbstractPillarTest): @responses.activate def test_store_multiple_tokens(self): - from pillar.api.utils.authentication import hash_auth_token - scst1 = '%s-1' % TEST_SUBCLIENT_TOKEN scst2 = '%s-2' % TEST_SUBCLIENT_TOKEN db_user1 = self._common_user_test(201, scst=scst1) @@ -53,10 +51,8 @@ class BlenderIdSubclientTest(AbstractPillarTest): # Now there should be two tokens. with self.app.test_request_context(): tokens = self.app.data.driver.db['tokens'] - self.assertIsNotNone(tokens.find_one( - {'user': db_user1['_id'], 'token_hashed': hash_auth_token(scst1)})) - self.assertIsNotNone(tokens.find_one( - {'user': db_user1['_id'], 'token_hashed': hash_auth_token(scst2)})) + self.assertIsNotNone(tokens.find_one({'user': db_user1['_id'], 'token': scst1})) + self.assertIsNotNone(tokens.find_one({'user': db_user1['_id'], 'token': scst2})) # There should still be only one auth element for blender-id in the user doc. self.assertEqual(1, len(db_user1['auth'])) @@ -80,8 +76,6 @@ class BlenderIdSubclientTest(AbstractPillarTest): def _common_user_test(self, expected_status_code, scst=TEST_SUBCLIENT_TOKEN, expected_full_name=TEST_FULL_NAME, mock_happy_blender_id=True): - from pillar.api.utils.authentication import hash_auth_token - if mock_happy_blender_id: self.mock_blenderid_validate_happy() @@ -110,7 +104,7 @@ class BlenderIdSubclientTest(AbstractPillarTest): # Check that the token was succesfully stored. tokens = self.app.data.driver.db['tokens'] db_token = tokens.find_one({'user': db_user['_id'], - 'token_hashed': hash_auth_token(scst)}) + 'token': scst}) self.assertIsNotNone(db_token) return db_user diff --git a/tests/test_api/test_local_auth.py b/tests/test_api/test_local_auth.py index f327fe0b..5c3e8285 100644 --- a/tests/test_api/test_local_auth.py +++ b/tests/test_api/test_local_auth.py @@ -38,8 +38,6 @@ class LocalAuthTest(AbstractPillarTest): self.assertEqual(200, resp.status_code, resp.data) def test_login_expired_token(self): - from pillar.api.utils.authentication import hash_auth_token - user_id = self.create_test_user() resp = self.client.post('/api/auth/make-token', @@ -54,7 +52,7 @@ class LocalAuthTest(AbstractPillarTest): tokens = self.app.data.driver.db['tokens'] exp = datetime.datetime.now(tz=tz_util.utc) - datetime.timedelta(1) - result = tokens.update_one({'token_hashed': hash_auth_token(token)}, + result = tokens.update_one({'token': token}, {'$set': {'expire_time': exp}}) self.assertEqual(1, result.modified_count) diff --git a/tests/test_api/test_organizations.py b/tests/test_api/test_organizations.py index 77c5b873..36189d74 100644 --- a/tests/test_api/test_organizations.py +++ b/tests/test_api/test_organizations.py @@ -989,8 +989,6 @@ class IPRangeLoginRolesTest(AbstractIPRangeSingleOrgTest): ]}) def _test_api(self, headers: dict, env: dict): - from pillar.api.utils.authentication import hash_auth_token - self.mock_blenderid_validate_happy() # This should check the IP of the user agains the organization IP ranges and update the # user in the database. @@ -1003,7 +1001,7 @@ class IPRangeLoginRolesTest(AbstractIPRangeSingleOrgTest): tokens_coll = self.app.db('tokens') token = tokens_coll.find_one({ 'user': bson.ObjectId(me['_id']), - 'token_hashed': hash_auth_token('usertoken'), + 'token': 'usertoken', }) self.assertEqual(self.org_roles, set(token['org_roles'])) @@ -1033,7 +1031,6 @@ class IPRangeLoginRolesTest(AbstractIPRangeSingleOrgTest): self._test_api_remote_addr('192.168.3.254') def _test_web_forwarded_for(self, ip_addr: str, ip_roles: typing.Set[str]): - from pillar.api.utils.authentication import hash_auth_token from pillar import auth self.mock_blenderid_validate_happy() @@ -1053,7 +1050,7 @@ class IPRangeLoginRolesTest(AbstractIPRangeSingleOrgTest): tokens_coll = self.app.db('tokens') token = tokens_coll.find_one({ 'user': bson.ObjectId(my_id), - 'token_hashed': hash_auth_token('usertoken'), + 'token': 'usertoken', 'expire_time': {'$gt': datetime.datetime.now(tz_util.utc)}, }) self.assertEqual(ip_roles, set(token.get('org_roles', [])))