From a2741956eaa146e81cbf74141708294dd2721da8 Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Thu, 8 Sep 2022 12:46:21 +1000 Subject: [PATCH 1/6] add Flake8 config and clean up to make it happy --- .flake8 | 19 +++++++++++++++++++ ckanext/security/logic/auth.py | 5 +++++ ckanext/security/plugin/__init__.py | 10 +++++----- ckanext/security/plugin/flask_plugin.py | 2 +- ckanext/security/views.py | 3 +-- 5 files changed, 31 insertions(+), 8 deletions(-) create mode 100644 .flake8 diff --git a/.flake8 b/.flake8 new file mode 100644 index 0000000..bc5b99c --- /dev/null +++ b/.flake8 @@ -0,0 +1,19 @@ +[flake8] +# @see https://flake8.pycqa.org/en/latest/user/configuration.html?highlight=.flake8 + +exclude = + ckan + +# Extended output format. +format = pylint + +# Show the source of errors. +show_source = True +statistics = True + +max-complexity = 10 +max-line-length = 127 + +# List ignore rules one per line. +ignore = + C901 diff --git a/ckanext/security/logic/auth.py b/ckanext/security/logic/auth.py index a1ad4a1..15cccbe 100644 --- a/ckanext/security/logic/auth.py +++ b/ckanext/security/logic/auth.py @@ -2,17 +2,22 @@ Action functions, all sysadmin-only ''' + def security_throttle_user_reset(context, data_dict): return {'success': False} + def security_throttle_address_reset(context, data_dict): return {'success': False} + def security_throttle_user_show(context, data_dict): return {'success': False} + def security_throttle_address_show(context, data_dict): return {'success': False} + def security_reset_totp(context, data_dict): return {'success': False} diff --git a/ckanext/security/plugin/__init__.py b/ckanext/security/plugin/__init__.py index fc0f311..d46f4ef 100644 --- a/ckanext/security/plugin/__init__.py +++ b/ckanext/security/plugin/__init__.py @@ -1,3 +1,5 @@ +# encoding: utf-8 + import logging import ckan.plugins as p @@ -10,12 +12,10 @@ ) from ckanext.security.logic import auth, action -try: - tk.requires_ckan_version("2.9") -except tk.CkanVersionException: - from ckanext.security.plugin.pylons_plugin import MixinPlugin -else: +if tk.check_ckan_version("2.9"): from ckanext.security.plugin.flask_plugin import MixinPlugin +else: + from ckanext.security.plugin.pylons_plugin import MixinPlugin log = logging.getLogger(__name__) diff --git a/ckanext/security/plugin/flask_plugin.py b/ckanext/security/plugin/flask_plugin.py index 6b6ecee..e067f66 100644 --- a/ckanext/security/plugin/flask_plugin.py +++ b/ckanext/security/plugin/flask_plugin.py @@ -17,4 +17,4 @@ def get_blueprint(self): # ICLick def get_commands(self): - return cli.get_commands() \ No newline at end of file + return cli.get_commands() diff --git a/ckanext/security/views.py b/ckanext/security/views.py index 7c4948a..08725c7 100644 --- a/ckanext/security/views.py +++ b/ckanext/security/views.py @@ -2,10 +2,9 @@ import logging -from ckan.views import user from ckanext.security import utils from ckan.lib import helpers -from flask import Blueprint, make_response, request +from flask import Blueprint, make_response from functools import wraps from ckan.plugins import toolkit as tk From ef44283a3d753d42e6f89fda22c1f5f3f2b10640 Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Thu, 8 Sep 2022 16:16:22 +1000 Subject: [PATCH 2/6] enhance user privacy if 'ckan.public_user_details' is False, #53 - user_list is only visible to admins - Individual user profiles are only visible to admins or themselves - Organisation and group user lists are only visible to the admins of that org/group --- ckanext/security/logic/auth.py | 95 +++++++++++++++++++++++++++++ ckanext/security/plugin/__init__.py | 9 ++- 2 files changed, 103 insertions(+), 1 deletion(-) diff --git a/ckanext/security/logic/auth.py b/ckanext/security/logic/auth.py index 15cccbe..ee26c0e 100644 --- a/ckanext/security/logic/auth.py +++ b/ckanext/security/logic/auth.py @@ -2,6 +2,10 @@ Action functions, all sysadmin-only ''' +from ckan import authz, model +from ckan.logic import auth as logic_auth +from ckan.plugins.toolkit import _, asbool, auth_allow_anonymous_access + def security_throttle_user_reset(context, data_dict): return {'success': False} @@ -21,3 +25,94 @@ def security_throttle_address_show(context, data_dict): def security_reset_totp(context, data_dict): return {'success': False} + + +# User privacy enhancements + + +def user_list(context, data_dict=None): + """Check whether access to the user list is authorised. + Restricted to admins. + """ + return {'success': _requester_is_admin(context)} + + +@auth_allow_anonymous_access +def user_show(context, data_dict): + """Check whether access to individual user details is authorised. + Restricted to admins or self. + """ + if _requester_is_admin(context): + return {'success': True} + requester = context.get('user') + id = data_dict.get('id', None) + if id: + user_obj = model.User.get(id) + else: + user_obj = data_dict.get('user_obj', None) + if user_obj: + return {'success': requester in [user_obj.name, user_obj.id]} + + return {'success': False} + + +@auth_allow_anonymous_access +def group_show(context, data_dict): + """Check whether access to a group is authorised. + If it's just the group metadata, this requires no privileges, + but if user details have been requested, it requires a group admin. + """ + user = context.get('user') + group = logic_auth.get_group_object(context, data_dict) + if group.state == 'active' and \ + not asbool(data_dict.get('include_users', False)) and \ + data_dict.get('object_type', None) != 'user': + return {'success': True} + authorized = authz.has_user_permission_for_group_or_org( + group.id, user, 'update') + if authorized: + return {'success': True} + else: + return {'success': False, + 'msg': _('User %s not authorized to read group %s') % (user, group.id)} + + +def _requester_is_admin(context): + """Check whether the current user has admin privileges in some group + or organisation. + This is based on the 'update' privilege; see eg + ckan.logic.auth.update.group_edit_permissions. + """ + requester = context.get('user') + return _has_user_permission_for_some_group(requester, 'admin') + + +def _has_user_permission_for_some_group(user_name, permission): + """Check if the user has the given permission for any group. + """ + user_id = authz.get_user_id_for_username(user_name, allow_none=True) + if not user_id: + return False + roles = authz.get_roles_with_permission(permission) + + if not roles: + return False + # get any groups the user has with the needed role + q = model.Session.query(model.Member) \ + .filter(model.Member.table_name == 'user') \ + .filter(model.Member.state == 'active') \ + .filter(model.Member.capacity.in_(roles)) \ + .filter(model.Member.table_id == user_id) + group_ids = [] + for row in q.all(): + group_ids.append(row.group_id) + # if not in any groups has no permissions + if not group_ids: + return False + + # see if any of the groups are active + q = model.Session.query(model.Group) \ + .filter(model.Group.state == 'active') \ + .filter(model.Group.id.in_(group_ids)) + + return bool(q.count()) diff --git a/ckanext/security/plugin/__init__.py b/ckanext/security/plugin/__init__.py index d46f4ef..e2e6b71 100644 --- a/ckanext/security/plugin/__init__.py +++ b/ckanext/security/plugin/__init__.py @@ -86,7 +86,7 @@ def get_actions(self): # BEGIN Hooks for IAuthFunctions def get_auth_functions(self): - return { + auth_functions = { 'security_throttle_user_reset': auth.security_throttle_user_reset, 'security_throttle_address_reset': @@ -98,6 +98,13 @@ def get_auth_functions(self): 'security_reset_totp': auth.security_reset_totp, } + if not tk.asbool(tk.config.get('ckan.auth.public_user_details', True)): + auth_functions.update({ + 'user_list': auth.user_list, + 'user_show': auth.user_show, + 'group_show': auth.group_show, + }) + return auth_functions # END Hooks for IAuthFunctions # ITemplateHelpers From 2d30613d539a1a451b378ace195872fcfda9d966 Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Wed, 14 Sep 2022 14:42:56 +1000 Subject: [PATCH 3/6] use chained auth functions so other plugins can still add their own rules, #53 --- .flake8 | 1 + ckanext/security/logic/auth.py | 59 ++++++++++++++++++----------- ckanext/security/plugin/__init__.py | 9 ++--- 3 files changed, 40 insertions(+), 29 deletions(-) diff --git a/.flake8 b/.flake8 index bc5b99c..ee7a7cc 100644 --- a/.flake8 +++ b/.flake8 @@ -17,3 +17,4 @@ max-line-length = 127 # List ignore rules one per line. ignore = C901 + W503 diff --git a/ckanext/security/logic/auth.py b/ckanext/security/logic/auth.py index ee26c0e..8bc5a67 100644 --- a/ckanext/security/logic/auth.py +++ b/ckanext/security/logic/auth.py @@ -4,7 +4,8 @@ from ckan import authz, model from ckan.logic import auth as logic_auth -from ckan.plugins.toolkit import _, asbool, auth_allow_anonymous_access +from ckan.plugins.toolkit import _, asbool, config, \ + auth_allow_anonymous_access, chained_auth_function def security_throttle_user_reset(context, data_dict): @@ -30,48 +31,60 @@ def security_reset_totp(context, data_dict): # User privacy enhancements -def user_list(context, data_dict=None): +@chained_auth_function +def user_list(next_auth, context, data_dict=None): """Check whether access to the user list is authorised. Restricted to admins. """ - return {'success': _requester_is_admin(context)} + if asbool(config.get('ckan.auth.public_user_details', True)) \ + or _requester_is_admin(context): + return next_auth(context, data_dict) + else: + return {'success': False} +@chained_auth_function @auth_allow_anonymous_access -def user_show(context, data_dict): +def user_show(next_auth, context, data_dict): """Check whether access to individual user details is authorised. Restricted to admins or self. """ - if _requester_is_admin(context): - return {'success': True} - requester = context.get('user') - id = data_dict.get('id', None) - if id: - user_obj = model.User.get(id) + if asbool(config.get('ckan.auth.public_user_details', True)) \ + or _requester_is_admin(context): + authorized = True else: - user_obj = data_dict.get('user_obj', None) - if user_obj: - return {'success': requester in [user_obj.name, user_obj.id]} + requester = context.get('user') + id = data_dict.get('id', None) + if id: + user_obj = model.User.get(id) + else: + user_obj = data_dict.get('user_obj', None) + authorized = user_obj and requester in [user_obj.name, user_obj.id] + if authorized: + return next_auth(context, data_dict) return {'success': False} +@chained_auth_function @auth_allow_anonymous_access -def group_show(context, data_dict): +def group_show(next_auth, context, data_dict): """Check whether access to a group is authorised. If it's just the group metadata, this requires no privileges, but if user details have been requested, it requires a group admin. """ - user = context.get('user') - group = logic_auth.get_group_object(context, data_dict) - if group.state == 'active' and \ - not asbool(data_dict.get('include_users', False)) and \ - data_dict.get('object_type', None) != 'user': - return {'success': True} - authorized = authz.has_user_permission_for_group_or_org( - group.id, user, 'update') + if asbool(config.get('ckan.auth.public_user_details', True)): + authorized = True + else: + user = context.get('user') + group = logic_auth.get_group_object(context, data_dict) + authorized = ( + group.state == 'active' + and not asbool(data_dict.get('include_users', False)) + and data_dict.get('object_type', None) != 'user' + ) or authz.has_user_permission_for_group_or_org(group.id, user, 'update') if authorized: - return {'success': True} + return next_auth(context, data_dict) else: return {'success': False, 'msg': _('User %s not authorized to read group %s') % (user, group.id)} diff --git a/ckanext/security/plugin/__init__.py b/ckanext/security/plugin/__init__.py index e2e6b71..c845ca9 100644 --- a/ckanext/security/plugin/__init__.py +++ b/ckanext/security/plugin/__init__.py @@ -97,13 +97,10 @@ def get_auth_functions(self): auth.security_throttle_address_show, 'security_reset_totp': auth.security_reset_totp, + 'user_list': auth.user_list, + 'user_show': auth.user_show, + 'group_show': auth.group_show, } - if not tk.asbool(tk.config.get('ckan.auth.public_user_details', True)): - auth_functions.update({ - 'user_list': auth.user_list, - 'user_show': auth.user_show, - 'group_show': auth.group_show, - }) return auth_functions # END Hooks for IAuthFunctions From cde161727f4292c4a0798367e0034afa7f3afb76 Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Tue, 27 Sep 2022 08:00:46 +1000 Subject: [PATCH 4/6] adjust documentation comments to mention the 'public_user_details' flag, #53 --- ckanext/security/logic/auth.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ckanext/security/logic/auth.py b/ckanext/security/logic/auth.py index 8bc5a67..3c5a85a 100644 --- a/ckanext/security/logic/auth.py +++ b/ckanext/security/logic/auth.py @@ -34,7 +34,7 @@ def security_reset_totp(context, data_dict): @chained_auth_function def user_list(next_auth, context, data_dict=None): """Check whether access to the user list is authorised. - Restricted to admins. + Restricted to admins if 'ckan.auth.public_user_details' is False. """ if asbool(config.get('ckan.auth.public_user_details', True)) \ or _requester_is_admin(context): @@ -47,7 +47,7 @@ def user_list(next_auth, context, data_dict=None): @auth_allow_anonymous_access def user_show(next_auth, context, data_dict): """Check whether access to individual user details is authorised. - Restricted to admins or self. + Restricted to admins or self if 'ckan.auth.public_user_details' is False. """ if asbool(config.get('ckan.auth.public_user_details', True)) \ or _requester_is_admin(context): @@ -71,7 +71,8 @@ def user_show(next_auth, context, data_dict): def group_show(next_auth, context, data_dict): """Check whether access to a group is authorised. If it's just the group metadata, this requires no privileges, - but if user details have been requested, it requires a group admin. + but if user details have been requested and + 'ckan.auth.public_user_details' is False, it requires a group admin. """ if asbool(config.get('ckan.auth.public_user_details', True)): authorized = True From 6ebe1e95fd6d6efacf305a275430a9378d42135f Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Tue, 27 Sep 2022 08:01:00 +1000 Subject: [PATCH 5/6] make variable name consistent, #53 --- ckanext/security/logic/auth.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ckanext/security/logic/auth.py b/ckanext/security/logic/auth.py index 3c5a85a..9d8b8b9 100644 --- a/ckanext/security/logic/auth.py +++ b/ckanext/security/logic/auth.py @@ -77,18 +77,18 @@ def group_show(next_auth, context, data_dict): if asbool(config.get('ckan.auth.public_user_details', True)): authorized = True else: - user = context.get('user') + requester = context.get('user') group = logic_auth.get_group_object(context, data_dict) authorized = ( group.state == 'active' and not asbool(data_dict.get('include_users', False)) and data_dict.get('object_type', None) != 'user' - ) or authz.has_user_permission_for_group_or_org(group.id, user, 'update') + ) or authz.has_user_permission_for_group_or_org(group.id, requester, 'update') if authorized: return next_auth(context, data_dict) else: return {'success': False, - 'msg': _('User %s not authorized to read group %s') % (user, group.id)} + 'msg': _('User %s not authorized to read group %s') % (requester, group.id)} def _requester_is_admin(context): From d9b9d3d263a2934aebc9ebcb0c0ac5c94c163037 Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Thu, 22 Dec 2022 14:26:04 +1000 Subject: [PATCH 6/6] add documentation of user profile access controls, #53 --- README.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/README.md b/README.md index 3162599..6413e96 100644 --- a/README.md +++ b/README.md @@ -13,6 +13,7 @@ A CKAN extension to hold various security improvements for CKAN, including: disclose whether or not that email address exists in the DB * Two Factor Authentication is enforced for all users * Preventing upload/linking of certain file types for resources +* Stricter access rules to view user profiles **Please note**: * This extension has been used and tested against CKAN version 2.7.x on git tag 2.5.0 and earlier @@ -88,6 +89,16 @@ You can also achieve this by adding the detected mime type to your blacklist dir Links are only checked based on the extension in the url, we do not request the file at the linked url to infer the mime type. +### Stricter access rules to view user profiles + +If `ckan.auth.public_user_details` is set to False, then access to user profiles will be restricted beyond the CKAN default of 'any authenticated user'. + +* 'user_list' permission (affecting the 'user_list' and 'user_autocomplete' APIs) will be restricted to admins (sysadmins, org admins, and group admins). This is necessary in order for admins to add users to their group/org. +* 'user_show' permission will be restricted to admins, plus the owner of the requested user profile. +* 'group_show' permission (affecting the 'group_show' and 'organization_show' APIs) will be restricted to admins of the specific requested group, if the call requests user details with the 'include_users' parameter. If user details are not requested, calls are unrestricted. + +If `ckan.auth.public_user_details` is absent or True, no restrictions are imposed. + ## Requirements * The server-side session storage requires the session middleware in CKAN core to be moved near the end of the middleware stack. An example changeset (relevant to CKAN 2.9.3) for this is provided in [ckanext-security.patch](ckanext-security.patch). The installed CKAN core codebase will need to have this patch applied (or similar changes made if not using 2.9.3). * A running Redis instance to store brute force protection tokens configured with a maxmemory and maxmemory-policy=lru so it overwrites the least recently used item rather than running out of space. This instance should be a different instance from the one used for Harvest items to avoid data loss. [Redis LRU-Cache documentation](https://redis.io/topics/lru-cache).