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

@@ -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
+    :param email: optional email address associated with the account.
-    :type email: str
     :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
+    log.info('Creating service account %s with roles %s', user_id, roles)
-    users_coll = current_app.db()['users']
+    user = {'_id': user_id,
-    user = users_coll.find_one({'email': email})
+            'username': f'SRV-{user_id}',
-    if user:
+            'groups': [],
-        # Check whether updating is allowed at all.
+            'roles': roles,
-        if update_existing is None:
+            'settings': {'email_communications': 0},
-            raise ValueError('User %s already exists' % email)
+            '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.
+    if status != 201:
-        roles = list(set(roles).union({'service'}).union(user['roles']))
+        raise ServiceAccountCreationError('Error creating user {}: {}'.format(user_id, result))
-        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))
     user.update(result)
 
     # Create an authentication token that won't expire for a long time.

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):

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,
+        service_definition
-        update_existing=update_existing
     )
 
     print('Service account information:')

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.

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),
+        resp = self.client.get(
-                               headers={'Authorization': self.make_header('token')})
+            '/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']},
+                                'methods': ['DELETE', 'GET', 'POST', 'PUT']},
-                                {'group': ObjectId('5596e975ea893b269af85c0f'),
+                               {'group': ObjectId('5596e975ea893b269af85c0f'),
-                                 'methods': ['GET']},
+                                'methods': ['GET']},
-                                {'group': ObjectId('564733b56dcaf85da2faee8a'),
+                               {'group': ObjectId('564733b56dcaf85da2faee8a'),
-                                 'methods': ['GET']}],
+                                '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']},
+                             'methods': ['DELETE', 'GET', 'POST', 'PUT']},
-                             {'group': ObjectId('5596e975ea893b269af85c0f'),
+                            {'group': ObjectId('5596e975ea893b269af85c0f'),
-                              'methods': ['DELETE', 'GET']},
+                             'methods': ['DELETE', 'GET']},
-                             {'group': ObjectId('564733b56dcaf85da2faee8a'),
+                            {'group': ObjectId('564733b56dcaf85da2faee8a'),
-                              'methods': ['GET']}],
+                             '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']},
+                             'methods': ['DELETE', 'GET', 'POST', 'PUT']},
-                             {'group': ObjectId('5596e975ea893b269af85c0f'),
+                            {'group': ObjectId('5596e975ea893b269af85c0f'),
-                              'methods': ['DELETE', 'GET']},
+                             'methods': ['DELETE', 'GET']},
-                             {'group': ObjectId('564733b56dcaf85da2faee8a'),
+                            {'group': ObjectId('564733b56dcaf85da2faee8a'),
-                              'methods': ['GET']}],
+                             '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)

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'])