From 4c4ec6c89b900648aa9559a49d66d69cc4e7fa7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Tue, 8 Mar 2016 17:36:21 +0100 Subject: [PATCH] Fixed authorization issue. Authorization wasn't properly checked, allowing more than allowed. --- pillar/application/__init__.py | 6 ++-- pillar/application/utils/authorization.py | 38 +++++++++++++---------- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/pillar/application/__init__.py b/pillar/application/__init__.py index c21527c4..6a78e119 100644 --- a/pillar/application/__init__.py +++ b/pillar/application/__init__.py @@ -138,8 +138,7 @@ from modules.file_storage import generate_link def before_returning_item_permissions(response): # Run validation process, since GET on nodes entry point is public validate_token() - if not check_permissions(response, 'GET', append_allowed_methods=True): - return abort(403) + check_permissions(response, 'GET', append_allowed_methods=True) def before_returning_resource_permissions(response): for item in response['_items']: @@ -254,8 +253,7 @@ def project_node_type_has_method(response): if not node_type: return abort(404) # Check permissions and append the allowed_methods to the node_type - if not check_permissions(node_type, 'GET', append_allowed_methods=True): - return abort(403) + check_permissions(node_type, 'GET', append_allowed_methods=True) # def before_returning_notifications(response): # for item in response['_items']: diff --git a/pillar/application/utils/authorization.py b/pillar/application/utils/authorization.py index d475159c..8b0c7fe0 100644 --- a/pillar/application/utils/authorization.py +++ b/pillar/application/utils/authorization.py @@ -1,20 +1,22 @@ +import logging + from flask import g from flask import request from flask import url_for from flask import abort from application import app +log = logging.getLogger(__name__) + def check_permissions(resource, method, append_allowed_methods=False): """Check user permissions to access a node. We look up node permissions from world to groups to users and match them with the computed user permissions. - If there is not match, we return 403. + If there is not match, we raise 403. """ if method != 'GET' and append_allowed_methods: raise ValueError("append_allowed_methods only allowed with 'GET' method") - allowed_methods = [] - current_user = g.get('current_user', None) if 'permissions' in resource: @@ -54,30 +56,34 @@ def check_permissions(resource, method, append_allowed_methods=False): elif resource_permissions and not computed_permissions: computed_permissions = resource_permissions + if not computed_permissions: + log.info('No permissions available to compute for %s on resource %r', + method, resource.get('node_type', resource)) + abort(403) + + # Accumulate allowed methods from the user, group and world level. + allowed_methods = set() + if current_user: # If the user is authenticated, proceed to compare the group permissions for permission in computed_permissions['groups']: if permission['group'] in current_user['groups']: - allowed_methods += permission['methods'] - if method in permission['methods'] and not append_allowed_methods: - return + allowed_methods.update(permission['methods']) for permission in computed_permissions['users']: if current_user['user_id'] == permission['user']: - allowed_methods += permission['methods'] - if method in permission['methods'] and not append_allowed_methods: - return + allowed_methods.update(permission['methods']) # Check if the node is public or private. This must be set for non logged # in users to see the content. For most BI projects this is on by default, # while for private project this will not be set at all. if 'world' in computed_permissions: - allowed_methods += computed_permissions['world'] - if method in computed_permissions['world'] and not append_allowed_methods: - return + allowed_methods.update(computed_permissions['world']) - if append_allowed_methods and method in allowed_methods: - resource['allowed_methods'] = list(set(allowed_methods)) - return resource + permission_granted = method in allowed_methods + if permission_granted: + if append_allowed_methods: + resource['allowed_methods'] = list(set(allowed_methods)) + return - return None + abort(403)