From 69d7c5c5cec98619fa8a04b698b60175c5c7a807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Thu, 4 May 2017 13:02:35 +0200 Subject: [PATCH] Allow service accounts to be email-less This removes the ability of updating service accounts through the CLI (something we never used anyway), now that service accounts cannot be uniquely identified by their email address. --- pillar/api/service.py | 71 ++++++---------- pillar/api/users/hooks.py | 14 +++- pillar/cli.py | 5 +- pillar/tests/__init__.py | 19 +++++ tests/test_api/test_auth.py | 104 +++++++++++++++++++----- tests/test_api/test_service_accounts.py | 87 ++++++++++++++++++++ 6 files changed, 228 insertions(+), 72 deletions(-) create mode 100644 tests/test_api/test_service_accounts.py diff --git a/pillar/api/service.py b/pillar/api/service.py index 7bb4a07e..c857385c 100644 --- a/pillar/api/service.py +++ b/pillar/api/service.py @@ -1,7 +1,9 @@ """Service accounts.""" import logging +import typing +import bson import blinker import bson @@ -22,6 +24,10 @@ ROLES_WITH_GROUPS = {'admin', 'demo', 'subscriber'} role_to_group_id = {} +class ServiceAccountCreationError(Exception): + """Raised when a service account cannot be created.""" + + @blueprint.before_app_first_request def fetch_role_to_group_id_map(): """Fills the _role_to_group_id mapping upon application startup.""" @@ -173,62 +179,35 @@ def manage_user_group_membership(db_user, role, action): return user_groups -def create_service_account(email, roles, service, update_existing=None): +def create_service_account(email: str, roles: typing.Iterable, service: dict): """Creates a service account with the given roles + the role 'service'. - :param email: email address associated with the account - :type email: str + :param email: optional email address associated with the account. :param roles: iterable of role names :param service: dict of the 'service' key in the user. - :type service: dict - :param update_existing: callback function that receives an existing user to update - for this service, in case the email address is already in use by someone. - If not given or None, updating existing users is disallowed, and a ValueError - exception is thrown instead. :return: tuple (user doc, token doc) """ - from pillar.api.utils import remove_private_keys + # Create a user with the correct roles. + roles = sorted(set(roles).union({'service'})) + user_id = bson.ObjectId() - # Find existing - users_coll = current_app.db()['users'] - user = users_coll.find_one({'email': email}) - if user: - # Check whether updating is allowed at all. - if update_existing is None: - raise ValueError('User %s already exists' % email) + log.info('Creating service account %s with roles %s', user_id, roles) + user = {'_id': user_id, + 'username': f'SRV-{user_id}', + 'groups': [], + 'roles': roles, + 'settings': {'email_communications': 0}, + 'auth': [], + 'full_name': f'SRV-{user_id}', + 'service': service} + if email: + user['email'] = email + result, _, _, status = current_app.post_internal('users', user) - # Compute the new roles, and assign. - roles = list(set(roles).union({'service'}).union(user['roles'])) - user['roles'] = list(roles) - - # Let the caller perform any required updates. - log.info('Updating existing user %s to become service account for %s', - email, roles) - update_existing(user['service']) - - # Try to store the updated user. - result, _, _, status = current_app.put_internal('users', - remove_private_keys(user), - _id=user['_id']) - expected_status = 200 - else: - # Create a user with the correct roles. - roles = list(set(roles).union({'service'})) - user = {'username': email, - 'groups': [], - 'roles': roles, - 'settings': {'email_communications': 0}, - 'auth': [], - 'full_name': email, - 'email': email, - 'service': service} - result, _, _, status = current_app.post_internal('users', user) - expected_status = 201 - - if status != expected_status: - raise SystemExit('Error creating user {}: {}'.format(email, result)) + if status != 201: + raise ServiceAccountCreationError('Error creating user {}: {}'.format(user_id, result)) user.update(result) # Create an authentication token that won't expire for a long time. diff --git a/pillar/api/users/hooks.py b/pillar/api/users/hooks.py index 94843450..f6acede6 100644 --- a/pillar/api/users/hooks.py +++ b/pillar/api/users/hooks.py @@ -5,13 +5,14 @@ from eve.utils import parse_request from flask import current_app, g from pillar.api.users.routes import log from pillar.api.utils.authorization import user_has_role -from werkzeug.exceptions import Forbidden +from werkzeug import exceptions as wz_exceptions USER_EDITABLE_FIELDS = {'full_name', 'username', 'email', 'settings'} # These fields nobody is allowed to touch directly, not even admins. USER_ALWAYS_RESTORE_FIELDS = {'auth'} + def before_replacing_user(request, lookup): """Prevents changes to any field of the user doc, except USER_EDITABLE_FIELDS.""" @@ -52,6 +53,11 @@ def before_replacing_user(request, lookup): else: del put_data[db_key] + # Regular users should always have an email address + if 'service' not in put_data['roles']: + if not put_data.get('email'): + raise wz_exceptions.UnprocessableEntity('email field must be given') + def push_updated_user_to_algolia(user, original): """Push an update to the Algolia index when a user item is updated""" @@ -91,7 +97,7 @@ def check_user_access(request, lookup): return if not lookup and not current_user: - raise Forbidden() + raise wz_exceptions.Forbidden() # Add a filter to only return the current user. if '_id' not in lookup: @@ -106,10 +112,10 @@ def check_put_access(request, lookup): current_user = g.get('current_user') if not current_user: - raise Forbidden() + raise wz_exceptions.Forbidden() if str(lookup['_id']) != str(current_user['user_id']): - raise Forbidden() + raise wz_exceptions.Forbidden() def after_fetching_user(user): diff --git a/pillar/cli.py b/pillar/cli.py index c20f4825..cec12493 100644 --- a/pillar/cli.py +++ b/pillar/cli.py @@ -315,15 +315,14 @@ def badger(action, user_email, role): log.info('Status : %i', status) -def create_service_account(email, service_roles, service_definition, update_existing=None): +def create_service_account(email, service_roles, service_definition): from pillar.api import service from pillar.api.utils import dumps account, token = service.create_service_account( email, service_roles, - service_definition, - update_existing=update_existing + service_definition ) print('Service account information:') diff --git a/pillar/tests/__init__.py b/pillar/tests/__init__.py index 5d985410..abb80628 100644 --- a/pillar/tests/__init__.py +++ b/pillar/tests/__init__.py @@ -449,6 +449,25 @@ class AbstractPillarTest(TestMinimal): def patch(self, *args, **kwargs): return self.client_request('PATCH', *args, **kwargs) + def assertAllowsAccess(self, + token: typing.Union[str, dict], + expected_user_id: typing.Union[str, ObjectId] = None): + """Asserts that this authentication token allows access to /api/users/me.""" + + if isinstance(token, dict) and 'token' in token: + token = token['token'] + + if not isinstance(token, str): + raise TypeError(f'token should be a string, but is {token!r}') + if expected_user_id and not isinstance(expected_user_id, (str, ObjectId)): + raise TypeError('expected_user_id should be a string or ObjectId, ' + f'but is {expected_user_id!r}') + + resp = self.get('/api/users/me', expected_status=200, auth_token=token).json() + + if expected_user_id: + self.assertEqual(resp['_id'], str(expected_user_id)) + def mongo_to_sdk(data): """Transforms a MongoDB dict to a dict suitable to give to the PillarSDK. diff --git a/tests/test_api/test_auth.py b/tests/test_api/test_auth.py index 9a0dd726..2cb252b6 100644 --- a/tests/test_api/test_auth.py +++ b/tests/test_api/test_auth.py @@ -234,7 +234,8 @@ class UserListTests(AbstractPillarTest): def test_list_all_users_admin(self): # Admin access should result in all users - resp = self.client.get('/api/users', headers={'Authorization': self.make_header('admin-token')}) + resp = self.client.get('/api/users', + headers={'Authorization': self.make_header('admin-token')}) users = json.loads(resp.data) self.assertEqual(200, resp.status_code) @@ -281,8 +282,9 @@ class UserListTests(AbstractPillarTest): def test_own_user_subscriber_explicit_projection(self): # With a custom projection requesting the auth list projection = json.dumps({'auth': 1}) - resp = self.client.get('/api/users/%s?projection=%s' % ('123456789abc123456789abc', projection), - headers={'Authorization': self.make_header('token')}) + resp = self.client.get( + '/api/users/%s?projection=%s' % ('123456789abc123456789abc', projection), + headers={'Authorization': self.make_header('token')}) user_info = json.loads(resp.data) self.assertEqual(200, resp.status_code) @@ -502,7 +504,7 @@ class PermissionComputationTest(AbstractPillarTest): self.assertEqual( { 'groups': [{'group': ObjectId('5596e975ea893b269af85c0e'), - 'methods': ['DELETE', 'GET', 'POST', 'PUT']}], + 'methods': ['DELETE', 'GET', 'POST', 'PUT']}], 'world': ['GET'] }, self.sort(compute_aggr_permissions('projects', EXAMPLE_PROJECT, None))) @@ -511,11 +513,11 @@ class PermissionComputationTest(AbstractPillarTest): self.assertEqual( { 'groups': [{'group': ObjectId('5596e975ea893b269af85c0e'), - 'methods': ['DELETE', 'GET', 'POST', 'PUT']}, - {'group': ObjectId('5596e975ea893b269af85c0f'), - 'methods': ['GET']}, - {'group': ObjectId('564733b56dcaf85da2faee8a'), - 'methods': ['GET']}], + 'methods': ['DELETE', 'GET', 'POST', 'PUT']}, + {'group': ObjectId('5596e975ea893b269af85c0f'), + 'methods': ['GET']}, + {'group': ObjectId('564733b56dcaf85da2faee8a'), + 'methods': ['GET']}], 'world': ['GET'] }, self.sort(compute_aggr_permissions('projects', EXAMPLE_PROJECT, 'texture'))) @@ -530,11 +532,11 @@ class PermissionComputationTest(AbstractPillarTest): self.ensure_project_exists(project_overrides=EXAMPLE_PROJECT) self.assertEqual( {'groups': [{'group': ObjectId('5596e975ea893b269af85c0e'), - 'methods': ['DELETE', 'GET', 'POST', 'PUT']}, - {'group': ObjectId('5596e975ea893b269af85c0f'), - 'methods': ['DELETE', 'GET']}, - {'group': ObjectId('564733b56dcaf85da2faee8a'), - 'methods': ['GET']}], + 'methods': ['DELETE', 'GET', 'POST', 'PUT']}, + {'group': ObjectId('5596e975ea893b269af85c0f'), + 'methods': ['DELETE', 'GET']}, + {'group': ObjectId('564733b56dcaf85da2faee8a'), + 'methods': ['GET']}], 'world': ['GET']}, self.sort(compute_aggr_permissions('nodes', node, None))) @@ -544,11 +546,11 @@ class PermissionComputationTest(AbstractPillarTest): node['project'] = EXAMPLE_PROJECT self.assertEqual( {'groups': [{'group': ObjectId('5596e975ea893b269af85c0e'), - 'methods': ['DELETE', 'GET', 'POST', 'PUT']}, - {'group': ObjectId('5596e975ea893b269af85c0f'), - 'methods': ['DELETE', 'GET']}, - {'group': ObjectId('564733b56dcaf85da2faee8a'), - 'methods': ['GET']}], + 'methods': ['DELETE', 'GET', 'POST', 'PUT']}, + {'group': ObjectId('5596e975ea893b269af85c0f'), + 'methods': ['DELETE', 'GET']}, + {'group': ObjectId('564733b56dcaf85da2faee8a'), + 'methods': ['GET']}], 'world': ['GET']}, self.sort(compute_aggr_permissions('nodes', node, None))) @@ -638,3 +640,67 @@ class RequireRolesTest(AbstractPillarTest): self.assertFalse(user_has_role('admin', {'roles': []})) self.assertFalse(user_has_role('admin', {'roles': None})) self.assertFalse(user_has_role('admin', {})) + + +class UserCreationTest(AbstractPillarTest): + @responses.activate + def test_create_by_auth(self): + """Create user by authenticating against Blender ID.""" + + with self.app.test_request_context(): + users_coll = self.app.db().users + self.assertEqual(0, users_coll.count()) + + self.mock_blenderid_validate_happy() + token = 'this is my life now' + self.get('/api/users/me', auth_token=token) + + with self.app.test_request_context(): + users_coll = self.app.db().users + self.assertEqual(1, users_coll.count()) + + db_user = users_coll.find()[0] + self.assertEqual(db_user['email'], TEST_EMAIL_ADDRESS) + + def test_user_without_email_address(self): + """Regular users should always have an email address. + + Regular users are created by authentication with Blender ID, so we do not + have to test that (Blender ID ensures there is an email address). We do need + to test PUT access to erase the email address, though. + """ + + from pillar.api.utils import remove_private_keys + + user_id = self.create_user(24 * 'd', token='user-token') + + with self.app.test_request_context(): + users_coll = self.app.db().users + db_user = users_coll.find_one(user_id) + + puttable = remove_private_keys(db_user) + + empty_email = copy.deepcopy(puttable) + empty_email['email'] = '' + + without_email = copy.deepcopy(puttable) + del without_email['email'] + + etag = db_user['_etag'] + resp = self.put(f'/api/users/{user_id}', json=puttable, etag=etag, + auth_token='user-token', expected_status=200).json() + etag = resp['_etag'] + self.put(f'/api/users/{user_id}', json=empty_email, etag=etag, + auth_token='user-token', expected_status=422) + self.put(f'/api/users/{user_id}', json=without_email, etag=etag, + auth_token='user-token', expected_status=422) + + # An admin should be able to edit this user, but also not clear the email address. + self.create_user(24 * 'a', roles={'admin'}, token='admin-token') + resp = self.put(f'/api/users/{user_id}', json=puttable, etag=etag, + auth_token='admin-token', expected_status=200).json() + etag = resp['_etag'] + self.put(f'/api/users/{user_id}', json=empty_email, etag=etag, + auth_token='admin-token', expected_status=422) + self.put(f'/api/users/{user_id}', json=without_email, etag=etag, + auth_token='admin-token', expected_status=422) diff --git a/tests/test_api/test_service_accounts.py b/tests/test_api/test_service_accounts.py new file mode 100644 index 00000000..999bcd61 --- /dev/null +++ b/tests/test_api/test_service_accounts.py @@ -0,0 +1,87 @@ +import json + +from pillar.tests import AbstractPillarTest + + +class ServiceAccountCreationTest(AbstractPillarTest): + def test_create_service_account(self): + from pillar.api.utils.authentication import force_cli_user + from pillar.api import service + + with self.app.test_request_context(): + force_cli_user() + account, token = service.create_service_account( + 'jemoeder@jevader.nl', ['flamenco_manager'], {'flamenco_manager': {}}) + + self.assertEqual(f'SRV-{account["_id"]}', account['full_name']) + self.assertEqual(f'SRV-{account["_id"]}', account['username']) + self.assertEqual(['flamenco_manager', 'service'], account['roles']) + self.assertEqual([], account['auth']) + self.assertEqual({'flamenco_manager': {}}, account['service']) + + self.assertAllowsAccess(token, account['_id']) + + def test_without_email_address(self): + from pillar.api.utils.authentication import force_cli_user + from pillar.api.service import create_service_account as create_sa + + with self.app.test_request_context(): + force_cli_user() + account, token = create_sa('', ['flamenco_manager'], {'flamenco_manager': {}}) + + self.assertNotIn('email', account) + self.assertAllowsAccess(token, account['_id']) + + def test_two_without_email_address(self): + from pillar.api.utils.authentication import force_cli_user + from pillar.api.service import create_service_account as create_sa + + with self.app.test_request_context(): + force_cli_user() + + account1, token1 = create_sa('', ['flamenco_manager'], {'flamenco_manager': {}}) + account2, token2 = create_sa('', ['flamenco_manager'], {'flamenco_manager': {}}) + + self.assertAllowsAccess(token1, account1['_id']) + self.assertAllowsAccess(token2, account2['_id']) + + def test_put_without_email_address(self): + from pillar.api.utils import remove_private_keys + from pillar.api.utils.authentication import force_cli_user + from pillar.api.service import create_service_account as create_sa + + with self.app.test_request_context(): + force_cli_user() + account, token = create_sa('', ['flamenco_manager'], {'flamenco_manager': {}}) + + puttable = remove_private_keys(account) + user_id = account['_id'] + + # The user should be able to edit themselves, even without email address. + etag = account['_etag'] + puttable['full_name'] = 'þor' + resp = self.put(f'/api/users/{user_id}', + json=puttable, + auth_token=token['token'], + etag=etag).json() + etag = resp['_etag'] + + with self.app.test_request_context(): + users_coll = self.app.db().users + db_user = users_coll.find_one(user_id) + self.assertNotIn('email', db_user) + self.assertEqual('þor', db_user['full_name']) + + # An admin should be able to edit this email-less user. + self.create_user(24 * 'a', roles={'admin'}, token='admin-token') + puttable['username'] = 'bigdüde' + self.put(f'/api/users/{user_id}', + json=puttable, + auth_token='admin-token', + etag=etag) + + with self.app.test_request_context(): + users_coll = self.app.db().users + db_user = users_coll.find_one(user_id) + self.assertNotIn('email', db_user) + self.assertEqual('bigdüde', db_user['username'])