From 575a7ed1a7fd68e07c7ff1b9ccd4ebc6357812ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Fri, 18 Aug 2017 14:47:42 +0200 Subject: [PATCH] Introduced role-based capability system. It's still rather limited and hard-coded, but it works. --- pillar/api/projects/routes.py | 2 +- pillar/api/utils/authorization.py | 58 +++++++++++++++---- pillar/auth/__init__.py | 26 +++++++++ pillar/web/users/routes.py | 6 +- src/styles/_search.sass | 3 +- src/templates/_macros/_menu.jade | 2 +- src/templates/errors/403_embed.jade | 2 +- .../nodes/custom/comment/list_embed.jade | 2 +- src/templates/projects/edit_node_types.jade | 2 +- src/templates/projects/index_dashboard.jade | 6 +- src/templates/projects/sharing.jade | 6 +- src/templates/projects/view.jade | 2 +- src/templates/users/edit_embed.jade | 12 ++++ tests/test_api/test_auth.py | 35 ++++++++++- 14 files changed, 137 insertions(+), 27 deletions(-) diff --git a/pillar/api/projects/routes.py b/pillar/api/projects/routes.py index a4e7181a..1433fbfc 100644 --- a/pillar/api/projects/routes.py +++ b/pillar/api/projects/routes.py @@ -16,7 +16,7 @@ blueprint_api = Blueprint('projects_api', __name__) @blueprint_api.route('/create', methods=['POST']) -@authorization.require_login(require_roles={'admin', 'subscriber', 'demo'}) +@authorization.require_login(require_cap='subscriber') def create_project(overrides=None): """Creates a new project.""" diff --git a/pillar/api/utils/authorization.py b/pillar/api/utils/authorization.py index ccc96acd..8d5555da 100644 --- a/pillar/api/utils/authorization.py +++ b/pillar/api/utils/authorization.py @@ -265,12 +265,19 @@ def merge_permissions(*args): def require_login(require_roles=set(), + require_cap='', require_all=False): """Decorator that enforces users to authenticate. - Optionally only allows access to users with a certain role. + Optionally only allows access to users with a certain role and/or capability. + + Either check on roles or on a capability, but never on both. There is no + require_all check for capabilities; if you need to check for multiple + capabilities at once, it's a sign that you need to add another capability + and give it to everybody that needs it. :param require_roles: set of roles. + :param require_cap: a capability. :param require_all: When False (the default): if the user's roles have a non-empty intersection with the given roles, access is granted. @@ -279,7 +286,13 @@ def require_login(require_roles=set(), """ if not isinstance(require_roles, set): - raise TypeError('require_roles param should be a set, but is a %r' % type(require_roles)) + raise TypeError(f'require_roles param should be a set, but is {type(require_roles)!r}') + + if not isinstance(require_cap, str): + raise TypeError(f'require_caps param should be a str, but is {type(require_cap)!r}') + + if require_roles and require_cap: + raise ValueError('either use require_roles or require_cap, but not both') if require_all and not require_roles: raise ValueError('require_login(require_all=True) cannot be used with empty require_roles.') @@ -287,15 +300,21 @@ def require_login(require_roles=set(), def decorator(func): @functools.wraps(func) def wrapper(*args, **kwargs): - if not user_matches_roles(require_roles, require_all): - if g.current_user is None: - # We don't need to log at a higher level, as this is very common. - # Many browsers first try to see whether authentication is needed - # at all, before sending the password. - log.debug('Unauthenticated acces to %s attempted.', func) - else: - log.warning('User %s is authenticated, but does not have required roles %s to ' - 'access %s', g.current_user['user_id'], require_roles, func) + if g.current_user is None: + # We don't need to log at a higher level, as this is very common. + # Many browsers first try to see whether authentication is needed + # at all, before sending the password. + log.debug('Unauthenticated acces to %s attempted.', func) + abort(403) + + if require_roles and not g.current_user.matches_roles(require_roles, require_all): + log.warning('User %s is authenticated, but does not have required roles %s to ' + 'access %s', g.current_user['user_id'], require_roles, func) + abort(403) + + if require_cap and not g.current_user.has_cap(require_cap): + log.warning('User %s is authenticated, but does not have required capability %s to ' + 'access %s', g.current_user.user_id, require_cap, func) abort(403) return func(*args, **kwargs) @@ -352,6 +371,23 @@ def user_has_role(role, user: UserClass=None): return user.has_role(role) +def user_has_cap(capability: str, user: UserClass=None) -> bool: + """Returns True iff the user is logged in and has the given capability.""" + + assert capability + + if user is None: + user = g.get('current_user') + + if user is None: + return False + + if not isinstance(user, UserClass): + raise TypeError(f'user should be instance of UserClass, not {type(user)}') + + return user.has_cap(capability) + + def user_matches_roles(require_roles=set(), require_all=False): """Returns True iff the user's roles matches the query. diff --git a/pillar/auth/__init__.py b/pillar/auth/__init__.py index 00139124..e2b02a9b 100644 --- a/pillar/auth/__init__.py +++ b/pillar/auth/__init__.py @@ -16,6 +16,14 @@ from ..api.utils import authentication log = logging.getLogger(__name__) +# Mapping from user role to capabilities obtained by users with that role. +CAPABILITIES = collections.defaultdict(**{ + 'subscriber': {'subscriber', 'home-project'}, + 'demo': {'subscriber', 'home-project'}, + 'admin': {'subscriber', 'home-project', 'video-encoding', 'admin', + 'view-pending-nodes', 'edit-project-node-types'}, +}, default_factory=frozenset) + class UserClass(flask_login.UserMixin): def __init__(self, token: typing.Optional[str]): @@ -30,6 +38,7 @@ class UserClass(flask_login.UserMixin): self.roles: typing.List[str] = [] self.groups: typing.List[str] = [] # NOTE: these are stringified object IDs. self.group_ids: typing.List[bson.ObjectId] = [] + self.capabilities: typing.Set[str] = set() @classmethod def construct(cls, token: str, db_user: dict) -> 'UserClass': @@ -48,6 +57,7 @@ class UserClass(flask_login.UserMixin): user.objectid = str(db_user['_id']) user.gravatar = utils.gravatar(user.email) user.groups = [str(g) for g in user.group_ids] + user.collect_capabilities() return user @@ -71,6 +81,12 @@ class UserClass(flask_login.UserMixin): except KeyError: return default + def collect_capabilities(self): + """Constructs the capabilities set given the user's current roles.""" + + self.capabilities = set().union(*(CAPABILITIES.get(role, frozenset()) + for role in self.roles)) + def has_role(self, *roles): """Returns True iff the user has one or more of the given roles.""" @@ -79,6 +95,14 @@ class UserClass(flask_login.UserMixin): return bool(set(self.roles).intersection(set(roles))) + def has_cap(self, *capabilities: typing.Iterable[str]) -> bool: + """Returns True iff the user has one or more of the given capabilities.""" + + if not self.capabilities: + return False + + return bool(set(self.capabilities).intersection(set(capabilities))) + def matches_roles(self, require_roles=set(), require_all=False) -> bool: @@ -113,6 +137,8 @@ class AnonymousUser(flask_login.AnonymousUserMixin, UserClass): def has_role(self, *roles): return False + def has_cap(self, *capabilities): + return False def _load_user(token) -> typing.Union[UserClass, AnonymousUser]: diff --git a/pillar/web/users/routes.py b/pillar/web/users/routes.py index e8bece1a..e1470c80 100644 --- a/pillar/web/users/routes.py +++ b/pillar/web/users/routes.py @@ -205,6 +205,8 @@ def settings_billing(): @blueprint.route('/u//edit', methods=['GET', 'POST']) @login_required def users_edit(user_id): + from pillar.auth import UserClass + if not current_user.has_role('admin'): return abort(403) api = system_util.pillar_api() @@ -221,7 +223,9 @@ def users_edit(user_id): else: form.roles.data = user.roles form.email.data = user.email - return render_template('users/edit_embed.html', user=user, form=form) + + user_ob = UserClass.construct('', db_user=user.to_dict()) + return render_template('users/edit_embed.html', user=user_ob, form=form) def _users_edit(form, user, api): diff --git a/src/styles/_search.sass b/src/styles/_search.sass index c2879e14..bff0203e 100644 --- a/src/styles/_search.sass +++ b/src/styles/_search.sass @@ -96,7 +96,7 @@ $search-hit-width_grid: 100px #search-container display: flex border-radius: 0 - min-height: 500px + min-height: 600px background-color: white +container-behavior @@ -782,4 +782,3 @@ $search-hit-width_grid: 100px color: white background-color: $color-primary border-color: transparent - diff --git a/src/templates/_macros/_menu.jade b/src/templates/_macros/_menu.jade index 60402898..d7671c80 100644 --- a/src/templates/_macros/_menu.jade +++ b/src/templates/_macros/_menu.jade @@ -4,7 +4,7 @@ | {% if current_user.has_role('demo') %} | {% set subscription = 'demo' %} -| {% elif current_user.has_role('subscriber') %} +| {% elif current_user.has_cap('subscriber') %} | {% set subscription = 'subscriber' %} | {% else %} | {% set subscription = 'none' %} diff --git a/src/templates/errors/403_embed.jade b/src/templates/errors/403_embed.jade index 981f5dc9..498094d6 100644 --- a/src/templates/errors/403_embed.jade +++ b/src/templates/errors/403_embed.jade @@ -8,7 +8,7 @@ | {% if current_user.is_authenticated %} | {% if current_user.has_role('demo') %} | {% set subscription = 'demo' %} - | {% elif current_user.has_role('subscriber') %} + | {% elif current_user.has_cap('subscriber') %} | {% set subscription = 'subscriber' %} | {% else %} | {% set subscription = 'none' %} diff --git a/src/templates/nodes/custom/comment/list_embed.jade b/src/templates/nodes/custom/comment/list_embed.jade index a14e3487..398f2dac 100644 --- a/src/templates/nodes/custom/comment/list_embed.jade +++ b/src/templates/nodes/custom/comment/list_embed.jade @@ -44,7 +44,7 @@ | {# * User is authenticated, but has no subscription or 'POST' permission #} .comment-reply-form .comment-reply-field.sign-in - | {% if current_user.has_role('subscriber') or current_user.has_role('demo') %} + | {% if current_user.has_cap('subscriber') %} i.pi-lock | Only project members can comment. | {% else %} diff --git a/src/templates/projects/edit_node_types.jade b/src/templates/projects/edit_node_types.jade index b85294e0..d0c07794 100644 --- a/src/templates/projects/edit_node_types.jade +++ b/src/templates/projects/edit_node_types.jade @@ -18,7 +18,7 @@ span#project-edit-title When we add support for new node types in the future, it means we allow the creation of new items (such as textures). - | {% if current_user.has_role('admin') %} + | {% if current_user.has_cap('edit-project-node-types') %} ul.list-generic | {% for node_type in project.node_types %} li diff --git a/src/templates/projects/index_dashboard.jade b/src/templates/projects/index_dashboard.jade index 47b12d66..97553be4 100644 --- a/src/templates/projects/index_dashboard.jade +++ b/src/templates/projects/index_dashboard.jade @@ -39,7 +39,7 @@ meta(name="twitter:image", content="{{ url_for('static', filename='assets/img/ba span ({{ projects_shared|length }}) | {% endif %} - | {% if (current_user.has_role('subscriber') or current_user.has_role('admin')) %} + | {% if current_user.has_cap('subscriber') %} li.create( data-url="{{ url_for('projects.create') }}") a#project-create( @@ -73,7 +73,7 @@ meta(name="twitter:image", content="{{ url_for('static', filename='assets/img/ba li.when(title="{{ project._created }}") {{ project._created | pretty_date }} li.edit a(href="{{ url_for('projects.edit', project_url=project.url) }}") Edit - | {% if project.status == 'pending' and current_user.is_authenticated and current_user.has_role('admin') %} + | {% if project.status == 'pending' and current_user.has_cap('view-pending-nodes') %} li.pending Not Published | {% endif %} @@ -113,7 +113,7 @@ meta(name="twitter:image", content="{{ url_for('static', filename='assets/img/ba li.who by {{ project.user.full_name }} li.edit a(href="{{ url_for('projects.edit', project_url=project.url) }}") Edit - | {% if project.status == 'pending' and current_user.is_authenticated and current_user.has_role('admin') %} + | {% if project.status == 'pending' and current_user.has_cap('view-pending-nodes') %} li.pending Not Published | {% endif %} diff --git a/src/templates/projects/sharing.jade b/src/templates/projects/sharing.jade index dc8fb743..2db20b27 100644 --- a/src/templates/projects/sharing.jade +++ b/src/templates/projects/sharing.jade @@ -11,7 +11,7 @@ span#project-edit-title #node-edit-container #node-edit-form .col-md-6 - | {% if (project.user == current_user.objectid or current_user.has_role('admin')) %} + | {% if (project.user == current_user.objectid or current_user.has_cap('admin')) %} .sharing-users-search .form-group input#user-select.form-control( @@ -44,7 +44,7 @@ span#project-edit-title span.sharing-users-extra {{user['username']}} .sharing-users-action | {# Only allow deletion if we are: admin, project owners, or current_user in the team #} - | {% if current_user.has_role('admin') or (project.user == current_user.objectid) or (current_user.objectid == user['_id']) %} + | {% if current_user.has_cap('admin') or (project.user == current_user.objectid) or (current_user.objectid == user['_id']) %} | {% if project.user == user['_id'] %} span @@ -70,7 +70,7 @@ span#project-edit-title | {% endblock %} | {% block footer_scripts %} -| {% if (project.user == current_user.objectid or current_user.has_role('admin')) %} +| {% if (project.user == current_user.objectid or current_user.has_cap('admin')) %} script(src="{{ url_for('static_pillar', filename='assets/js/vendor/jquery.autocomplete-0.22.0.min.js') }}", async=true) script. $(document).ready(function() { diff --git a/src/templates/projects/view.jade b/src/templates/projects/view.jade index fc2e7763..f5d93240 100644 --- a/src/templates/projects/view.jade +++ b/src/templates/projects/view.jade @@ -177,7 +177,7 @@ link(href="{{ url_for('static_pillar', filename='assets/css/project-main.css', v i.pi-more-vertical ul.dropdown-menu - | {% if current_user.has_role('admin') %} + | {% if current_user.has_cap('admin') %} li.button-featured a#item_featured( href="javascript:void(0);", diff --git a/src/templates/users/edit_embed.jade b/src/templates/users/edit_embed.jade index fd6afbdc..35ee7850 100644 --- a/src/templates/users/edit_embed.jade +++ b/src/templates/users/edit_embed.jade @@ -59,6 +59,18 @@ | {% endfor %} + .form-group.capabilities + label Capabilities + | {% if user.capabilities %} + ul + | {% for cap in user.capabilities|sort %} + li {{ cap }} + | {% endfor %} + | {% else %} + p + i.pi-cancel + | none + | {% endif %} a#button-cancel.btn.btn-default(href="#", data-user-id='{{user._id}}') Cancel diff --git a/tests/test_api/test_auth.py b/tests/test_api/test_auth.py index d4ec5d26..e2c06b30 100644 --- a/tests/test_api/test_auth.py +++ b/tests/test_api/test_auth.py @@ -526,7 +526,6 @@ class PermissionComputationTest(AbstractPillarTest): class RequireRolesTest(AbstractPillarTest): def test_no_roles_required(self): - from flask import g from pillar.api.utils.authorization import require_login called = [False] @@ -604,6 +603,40 @@ class RequireRolesTest(AbstractPillarTest): self.assertFalse(user_has_role('admin', make_user(None))) self.assertFalse(user_has_role('admin', None)) + def test_cap_required(self): + from pillar.api.utils.authorization import require_login + + called = [False] + + @require_login(require_cap='subscriber') + def call_me(): + called[0] = True + + with self.app.test_request_context(): + self.login_api_as(ObjectId(24 * 'a'), ['succubus']) + self.assertRaises(Forbidden, call_me) + self.assertFalse(called[0]) + + with self.app.test_request_context(): + self.login_api_as(ObjectId(24 * 'a'), ['admin']) + call_me() + self.assertTrue(called[0]) + + def test_invalid_combinations(self): + from pillar.api.utils.authorization import require_login + + with self.assertRaises(TypeError): + require_login(require_roles=['abc', 'def']) + + with self.assertRaises(TypeError): + require_login(require_cap={'multiple', 'caps'}) + + with self.assertRaises(ValueError): + require_login(require_roles=set(), require_all=True) + + with self.assertRaises(ValueError): + require_login(require_roles={'admin'}, require_cap='hey') + class UserCreationTest(AbstractPillarTest): @responses.activate