From 566a23d3b6af739eff56e221212db36b2f76f775 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Fri, 18 Aug 2017 13:19:34 +0200 Subject: [PATCH] Unified user representation for web and API calls Both approaches now use a pillar.auth.UserClass instance. g.current_user is now always set to that instance, even for web entry points. This UserClass instance can still be keyed like the old dict, but this is for temporary compatibility and shouldn't be relied on in new or touched code. --- pillar/api/projects/hooks.py | 5 +- pillar/api/utils/authentication.py | 16 ++-- pillar/api/utils/authorization.py | 27 +++--- pillar/auth/__init__.py | 91 ++++++++++++++++++--- pillar/tests/__init__.py | 32 ++++++++ tests/test_api/test_auth.py | 38 ++++----- tests/test_api/test_blender_id_subclient.py | 2 +- tests/test_api/test_nodes.py | 5 +- 8 files changed, 156 insertions(+), 60 deletions(-) diff --git a/pillar/api/projects/hooks.py b/pillar/api/projects/hooks.py index 67af39ce..b1410020 100644 --- a/pillar/api/projects/hooks.py +++ b/pillar/api/projects/hooks.py @@ -110,6 +110,8 @@ def after_inserting_projects(projects): def after_inserting_project(project, db_user): + from pillar.auth import UserClass + project_id = project['_id'] user_id = db_user['_id'] @@ -135,7 +137,8 @@ def after_inserting_project(project, db_user): log.debug('Made user %s member of group %s', user_id, admin_group_id) # Assign the group to the project with admin rights - is_admin = authorization.is_admin(db_user) + owner_user = UserClass.construct('', db_user) + is_admin = authorization.is_admin(owner_user) world_permissions = ['GET'] if is_admin else [] permissions = { 'world': world_permissions, diff --git a/pillar/api/utils/authentication.py b/pillar/api/utils/authentication.py index 619ac52b..1fdd59fe 100644 --- a/pillar/api/utils/authentication.py +++ b/pillar/api/utils/authentication.py @@ -15,13 +15,17 @@ from flask import g from flask import request from flask import current_app +from pillar.auth import UserClass + log = logging.getLogger(__name__) -CLI_USER = { - 'user_id': 'CLI', +CLI_USER = UserClass.construct('CLI', { + '_id': 'CLI', 'groups': [], 'roles': {'admin'}, -} + 'email': 'local@nowhere', + 'username': 'CLI', +}) def force_cli_user(): @@ -74,6 +78,8 @@ def validate_this_token(token, oauth_subclient=None): :rtype: dict """ + from pillar.auth import UserClass + g.current_user = None _delete_expired_tokens() @@ -98,9 +104,7 @@ def validate_this_token(token, oauth_subclient=None): log.debug('Validation failed, user not logged in') return None - g.current_user = {'user_id': db_user['_id'], - 'groups': db_user['groups'], - 'roles': set(db_user.get('roles', []))} + g.current_user = UserClass.construct(token, db_user) return db_user diff --git a/pillar/api/utils/authorization.py b/pillar/api/utils/authorization.py index 0902e03a..ccc96acd 100644 --- a/pillar/api/utils/authorization.py +++ b/pillar/api/utils/authorization.py @@ -7,6 +7,8 @@ from flask import abort from flask import current_app from werkzeug.exceptions import Forbidden +from pillar.auth import UserClass + CHECK_PERMISSIONS_IMPLEMENTED_FOR = {'projects', 'nodes', 'flamenco_jobs'} log = logging.getLogger(__name__) @@ -334,17 +336,20 @@ def ab_testing(require_roles=set(), return decorator -def user_has_role(role, user=None): +def user_has_role(role, user: UserClass=None): """Returns True iff the user is logged in and has the given role.""" if user is None: user = g.get('current_user') + if user is not None and not isinstance(user, UserClass): + raise TypeError(f'g.current_user should be instance of UserClass, not {type(user)}') + elif not isinstance(user, UserClass): + raise TypeError(f'user should be instance of UserClass, not {type(user)}') if user is None: return False - roles = user.get('roles') or () - return role in roles + return user.has_role(role) def user_matches_roles(require_roles=set(), @@ -359,22 +364,14 @@ def user_matches_roles(require_roles=set(), returning True. """ - if not isinstance(require_roles, set): - raise TypeError('require_roles param should be a set, but is a %r' % type(require_roles)) - - if require_all and not require_roles: - raise ValueError('require_login(require_all=True) cannot be used with empty require_roles.') - - current_user = g.get('current_user') - + current_user: UserClass = g.get('current_user') if current_user is None: return False - intersection = require_roles.intersection(current_user['roles']) - if require_all: - return len(intersection) == len(require_roles) + if not isinstance(current_user, UserClass): + raise TypeError(f'g.current_user should be instance of UserClass, not {type(current_user)}') - return not bool(require_roles) or bool(intersection) + return current_user.matches_roles(require_roles, require_all) def is_admin(user): diff --git a/pillar/auth/__init__.py b/pillar/auth/__init__.py index 732ed85f..00139124 100644 --- a/pillar/auth/__init__.py +++ b/pillar/auth/__init__.py @@ -1,5 +1,6 @@ """Authentication code common to the web and api modules.""" +import collections import logging import typing @@ -8,6 +9,8 @@ import flask_login import flask_oauthlib.client from werkzeug.local import LocalProxy +import bson + from ..api import utils from ..api.utils import authentication @@ -20,11 +23,53 @@ class UserClass(flask_login.UserMixin): self.id = token self.username: str = None self.full_name: str = None + self.user_id: bson.ObjectId = None self.objectid: str = None self.gravatar: str = None self.email: str = None self.roles: typing.List[str] = [] - self.groups: typing.List[str] = [] + self.groups: typing.List[str] = [] # NOTE: these are stringified object IDs. + self.group_ids: typing.List[bson.ObjectId] = [] + + @classmethod + def construct(cls, token: str, db_user: dict) -> 'UserClass': + """Constructs a new UserClass instance from a Mongo user document.""" + + user = UserClass(token) + + user.user_id = db_user['_id'] + user.roles = db_user.get('roles') or [] + user.group_ids = db_user.get('groups') or [] + user.email = db_user.get('email') or '' + user.username = db_user['username'] + user.full_name = db_user.get('full_name') or '' + + # Derived properties + user.objectid = str(db_user['_id']) + user.gravatar = utils.gravatar(user.email) + user.groups = [str(g) for g in user.group_ids] + + return user + + def __getitem__(self, item): + """Compatibility layer with old dict-based g.current_user object.""" + + if item == 'user_id': + return self.user_id + if item == 'groups': + return self.group_ids + if item == 'roles': + return set(self.roles) + + raise KeyError(f'No such key {item!r}') + + def get(self, key, default=None): + """Compatibility layer with old dict-based g.current_user object.""" + + try: + return self[key] + except KeyError: + return default def has_role(self, *roles): """Returns True iff the user has one or more of the given roles.""" @@ -34,6 +79,32 @@ class UserClass(flask_login.UserMixin): return bool(set(self.roles).intersection(set(roles))) + def matches_roles(self, + require_roles=set(), + require_all=False) -> bool: + """Returns True iff the user's roles matches the query. + + :param require_roles: set of roles. + :param require_all: + When False (the default): if the user's roles have a + non-empty intersection with the given roles, returns True. + When True: require the user to have all given roles before + returning True. + """ + + if not isinstance(require_roles, set): + raise TypeError(f'require_roles param should be a set, but is {type(require_roles)!r}') + + if require_all and not require_roles: + raise ValueError('require_login(require_all=True) cannot be used with ' + 'empty require_roles.') + + intersection = require_roles.intersection(self.roles) + if require_all: + return len(intersection) == len(require_roles) + + return not bool(require_roles) or bool(intersection) + class AnonymousUser(flask_login.AnonymousUserMixin, UserClass): def __init__(self): @@ -43,27 +114,20 @@ class AnonymousUser(flask_login.AnonymousUserMixin, UserClass): return False -def _load_user(token): + +def _load_user(token) -> typing.Union[UserClass, AnonymousUser]: """Loads a user by their token. :returns: returns a UserClass instance if logged in, or an AnonymousUser() if not. - :rtype: UserClass """ db_user = authentication.validate_this_token(token) if not db_user: return AnonymousUser() - login_user = UserClass(token) - login_user.email = db_user['email'] - login_user.objectid = str(db_user['_id']) - login_user.username = db_user['username'] - login_user.gravatar = utils.gravatar(db_user['email']) - login_user.roles = db_user.get('roles', []) - login_user.groups = [str(g) for g in db_user['groups'] or ()] - login_user.full_name = db_user.get('full_name', '') + user = UserClass.construct(token, db_user) - return login_user + return user def config_login_manager(app): @@ -83,11 +147,14 @@ def config_login_manager(app): def login_user(oauth_token: str, *, load_from_db=False): """Log in the user identified by the given token.""" + from flask import g + if load_from_db: user = _load_user(oauth_token) else: user = UserClass(oauth_token) flask_login.login_user(user) + g.current_user = user def get_blender_id_oauth_token(): diff --git a/pillar/tests/__init__.py b/pillar/tests/__init__.py index 70514cfd..48e4eba4 100644 --- a/pillar/tests/__init__.py +++ b/pillar/tests/__init__.py @@ -278,6 +278,38 @@ class AbstractPillarTest(TestMinimal): return user_id + def create_user_object(self, user_id=ObjectId(), roles=frozenset(), group_ids=None): + """Creates a pillar.auth.UserClass object. + + :rtype: pillar.auth.UserClass + """ + + from pillar.auth import UserClass + + db_user = copy.deepcopy(ctd.EXAMPLE_USER) + db_user['_id'] = user_id + db_user['roles'] = list(roles) if roles is not None else None + + if group_ids is not None: + db_user['groups'] = list(group_ids) + + return UserClass.construct('', db_user) + + def login_api_as(self, user_id=ObjectId(), roles=frozenset(), group_ids=None): + """Creates a pillar.auth.UserClass object and sets it as g.current_user + + Requires an active request context! + + :rtype: pillar.auth.UserClass + """ + + from flask import g + + user = self.create_user_object(user_id, roles, group_ids) + g.current_user = user + + return user + def create_valid_auth_token(self, user_id, token='token'): now = datetime.datetime.now(tz_util.utc) future = now + datetime.timedelta(days=1) diff --git a/tests/test_api/test_auth.py b/tests/test_api/test_auth.py index 51a0d52d..d4ec5d26 100644 --- a/tests/test_api/test_auth.py +++ b/tests/test_api/test_auth.py @@ -536,14 +536,12 @@ class RequireRolesTest(AbstractPillarTest): called[0] = True with self.app.test_request_context(): - g.current_user = {'user_id': ObjectId(24 * 'a'), - 'roles': ['succubus']} + self.login_api_as(ObjectId(24 * 'a'), roles=['succubus']) call_me() self.assertTrue(called[0]) def test_some_roles_required(self): - from flask import g from pillar.api.utils.authorization import require_login called = [False] @@ -553,19 +551,16 @@ class RequireRolesTest(AbstractPillarTest): called[0] = True with self.app.test_request_context(): - g.current_user = {'user_id': ObjectId(24 * 'a'), - 'roles': ['succubus']} + self.login_api_as(ObjectId(24 * 'a'), ['succubus']) self.assertRaises(Forbidden, call_me) self.assertFalse(called[0]) with self.app.test_request_context(): - g.current_user = {'user_id': ObjectId(24 * 'a'), - 'roles': ['admin']} + self.login_api_as(ObjectId(24 * 'a'), ['admin']) call_me() self.assertTrue(called[0]) def test_all_roles_required(self): - from flask import g from pillar.api.utils.authorization import require_login called = [False] @@ -576,39 +571,38 @@ class RequireRolesTest(AbstractPillarTest): called[0] = True with self.app.test_request_context(): - g.current_user = {'user_id': ObjectId(24 * 'a'), - 'roles': ['admin']} + self.login_api_as(ObjectId(24 * 'a'), ['admin']) self.assertRaises(Forbidden, call_me) self.assertFalse(called[0]) with self.app.test_request_context(): - g.current_user = {'user_id': ObjectId(24 * 'a'), - 'roles': ['service']} + self.login_api_as(ObjectId(24 * 'a'), ['service']) self.assertRaises(Forbidden, call_me) self.assertFalse(called[0]) with self.app.test_request_context(): - g.current_user = {'user_id': ObjectId(24 * 'a'), - 'roles': ['badger']} + self.login_api_as(ObjectId(24 * 'a'), ['badger']) self.assertRaises(Forbidden, call_me) self.assertFalse(called[0]) with self.app.test_request_context(): - g.current_user = {'user_id': ObjectId(24 * 'a'), - 'roles': ['service', 'badger']} + self.login_api_as(ObjectId(24 * 'a'), ['service', 'badger']) call_me() self.assertTrue(called[0]) def test_user_has_role(self): from pillar.api.utils.authorization import user_has_role + def make_user(roles): + return self.create_user_object(ObjectId(), roles=roles) + with self.app.test_request_context(): - self.assertTrue(user_has_role('subscriber', {'roles': ['aap', 'noot', 'subscriber']})) - self.assertTrue(user_has_role('subscriber', {'roles': ['aap', 'subscriber']})) - self.assertFalse(user_has_role('admin', {'roles': ['aap', 'noot', 'subscriber']})) - self.assertFalse(user_has_role('admin', {'roles': []})) - self.assertFalse(user_has_role('admin', {'roles': None})) - self.assertFalse(user_has_role('admin', {})) + self.assertTrue(user_has_role('subscriber', make_user(['aap', 'noot', 'subscriber']))) + self.assertTrue(user_has_role('subscriber', make_user(['aap', 'subscriber']))) + self.assertFalse(user_has_role('admin', make_user(['aap', 'noot', 'subscriber']))) + self.assertFalse(user_has_role('admin', make_user([]))) + self.assertFalse(user_has_role('admin', make_user(None))) + self.assertFalse(user_has_role('admin', None)) class UserCreationTest(AbstractPillarTest): diff --git a/tests/test_api/test_blender_id_subclient.py b/tests/test_api/test_blender_id_subclient.py index afae0c19..eb465c5c 100644 --- a/tests/test_api/test_blender_id_subclient.py +++ b/tests/test_api/test_blender_id_subclient.py @@ -71,7 +71,7 @@ class BlenderIdSubclientTest(AbstractPillarTest): with self.app.test_request_context(headers={'Authorization': auth_header}): self.assertTrue(auth.validate_token()) self.assertIsNotNone(g.current_user) - self.assertEqual(db_user['_id'], g.current_user['user_id']) + self.assertEqual(db_user['_id'], g.current_user.user_id) def _common_user_test(self, expected_status_code, scst=TEST_SUBCLIENT_TOKEN, expected_full_name=TEST_FULL_NAME, diff --git a/tests/test_api/test_nodes.py b/tests/test_api/test_nodes.py index f71573d7..8c997e78 100644 --- a/tests/test_api/test_nodes.py +++ b/tests/test_api/test_nodes.py @@ -38,10 +38,9 @@ class NodeContentTypeTest(AbstractPillarTest): 'name': 'My first test node'} with self.app.test_request_context(): - g.current_user = {'user_id': user_id, + self.login_api_as(user_id, roles={'subscriber', 'admin'}, # This group is hardcoded in the EXAMPLE_PROJECT. - 'groups': [ObjectId('5596e975ea893b269af85c0e')], - 'roles': {'subscriber', 'admin'}} + group_ids=[ObjectId('5596e975ea893b269af85c0e')]) nodes = self.app.data.driver.db['nodes'] # Create the node.