From bd860dc58758356f36518586d1b70e1079700ba2 Mon Sep 17 00:00:00 2001 From: Ian Murray Date: Wed, 1 Aug 2012 13:05:44 +0100 Subject: [PATCH 1/5] [#2784] Hand control of user's sensitive data to user_dictize. Only a sysadmin or the owner of the account may view a user's apikey or reset_key. This does not change behaviour of the actions; just moves the logic form the actions to the model dictize layer. --- ckan/lib/dictization/model_dictize.py | 10 ++- ckan/logic/action/create.py | 9 ++- ckan/logic/action/get.py | 7 -- ckan/tests/lib/test_dictization.py | 98 ++++++++++++++++++++++++++- 4 files changed, 112 insertions(+), 12 deletions(-) diff --git a/ckan/lib/dictization/model_dictize.py b/ckan/lib/dictization/model_dictize.py index 17ac70a0205..b1d354bd794 100644 --- a/ckan/lib/dictization/model_dictize.py +++ b/ckan/lib/dictization/model_dictize.py @@ -2,6 +2,7 @@ from pylons import config from sqlalchemy.sql import select import datetime +import ckan.authz import ckan.model import ckan.misc import ckan.logic as logic @@ -351,7 +352,6 @@ def user_list_dictize(obj_list, context, for obj in obj_list: user_dict = user_dictize(obj, context) - user_dict.pop('apikey') result_list.append(user_dict) return sorted(result_list, key=sort_key, reverse=reverse) @@ -373,6 +373,14 @@ def user_dictize(user, context): result_dict['number_of_edits'] = user.number_of_edits() result_dict['number_administered_packages'] = user.number_administered_packages() + requester = context['user'] + + if not (ckan.authz.Authorizer().is_sysadmin(unicode(requester)) or + requester == user.name): + # If not sysadmin or the same user, strip sensible info + result_dict.pop('apikey', None) + result_dict.pop('reset_key', None) + model = context['model'] session = model.Session diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index 507acbb7d3f..d67590fc872 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -684,10 +684,17 @@ def user_create(context, data_dict): if not context.get('defer_commit'): model.repo.commit() + # Construct the user dict before changing the context. + # + # TODO: I don't know what the need for changing the context is, probably + # caching of the domain object. But it doesn't seem right given that + # usually context['user'] contains the user who made the request. + user_dict = model_dictize.user_dictize(user, context) + context['user'] = user context['id'] = user.id log.debug('Created user %s' % str(user.name)) - return model_dictize.user_dictize(user, context) + return user_dict ## Modifications for rest api diff --git a/ckan/logic/action/get.py b/ckan/logic/action/get.py index cc7d14e037d..da5bc3475e9 100644 --- a/ckan/logic/action/get.py +++ b/ckan/logic/action/get.py @@ -522,7 +522,6 @@ def user_list(context, data_dict): for user in query.all(): result_dict = model_dictize.user_dictize(user[0], context) - del result_dict['apikey'] users_list.append(result_dict) return users_list @@ -818,7 +817,6 @@ def user_show(context, data_dict): ''' model = context['model'] - user = context['user'] id = data_dict.get('id',None) provided_user = data_dict.get('user_obj',None) @@ -836,11 +834,6 @@ def user_show(context, data_dict): user_dict = model_dictize.user_dictize(user_obj,context) - if not (Authorizer().is_sysadmin(unicode(user)) or user == user_obj.name): - # If not sysadmin or the same user, strip sensible info - del user_dict['apikey'] - del user_dict['reset_key'] - revisions_q = model.Session.query(model.Revision ).filter_by(author=user_obj.name) diff --git a/ckan/tests/lib/test_dictization.py b/ckan/tests/lib/test_dictization.py index ae95f0220ee..71c70835bd6 100644 --- a/ckan/tests/lib/test_dictization.py +++ b/ckan/tests/lib/test_dictization.py @@ -13,6 +13,7 @@ activity_dictize, package_to_api1, package_to_api2, + user_dictize, ) from ckan.lib.dictization.model_save import (package_dict_save, resource_dict_save, @@ -871,7 +872,8 @@ def test_16_group_dictized(self): group = model.Session.query(model.Group).filter_by(name=u'help').one() context = {"model": model, - "session": model.Session} + "session": model.Session, + "user": None} group_dictized = group_dictize(group, context) @@ -897,8 +899,7 @@ def test_16_group_dictized(self): 'fullname': None, 'name': u'annafan', 'number_administered_packages': 1L, - 'number_of_edits': 0L, - 'reset_key': None}], + 'number_of_edits': 0L}], 'name': u'help', 'display_name': u'help', 'image_url': u'', @@ -1068,3 +1069,94 @@ def test_21_package_dictization_with_deleted_group(self): assert_not_in('test-group-2', [ g['name'] for g in result['groups'] ]) assert_in('test-group-1', [ g['name'] for g in result['groups'] ]) + def test_22_user_dictize_as_sysadmin(self): + '''Sysadmins should be allowed to see certain sensitive data.''' + context = { + 'model': model, + 'session': model.Session, + 'user': 'testsysadmin', + } + + user = model.User.by_name('tester') + + user_dict = user_dictize(user, context) + + # Check some of the non-sensitive data + assert 'name' in user_dict + assert 'about' in user_dict + + # Check sensitive data is available + assert 'apikey' in user_dict + assert 'reset_key' in user_dict + + # Passwords should never be available + assert 'password' not in user_dict + + def test_23_user_dictize_as_same_user(self): + '''User should be able to see their own sensitive data.''' + context = { + 'model': model, + 'session': model.Session, + 'user': 'tester', + } + + user = model.User.by_name('tester') + + user_dict = user_dictize(user, context) + + # Check some of the non-sensitive data + assert 'name' in user_dict + assert 'about' in user_dict + + # Check sensitive data is available + assert 'apikey' in user_dict + assert 'reset_key' in user_dict + + # Passwords should never be available + assert 'password' not in user_dict + + def test_24_user_dictize_as_other_user(self): + '''User should not be able to see other's sensitive data.''' + context = { + 'model': model, + 'session': model.Session, + 'user': 'annafan', + } + + user = model.User.by_name('tester') + + user_dict = user_dictize(user, context) + + # Check some of the non-sensitive data + assert 'name' in user_dict + assert 'about' in user_dict + + # Check sensitive data is available + assert 'apikey' not in user_dict + assert 'reset_key' not in user_dict + + # Passwords should never be available + assert 'password' not in user_dict + + def test_25_user_dictize_as_anonymous(self): + '''Anonymous should not be able to see other's sensitive data.''' + context = { + 'model': model, + 'session': model.Session, + 'user': '', + } + + user = model.User.by_name('tester') + + user_dict = user_dictize(user, context) + + # Check some of the non-sensitive data + assert 'name' in user_dict + assert 'about' in user_dict + + # Check sensitive data is available + assert 'apikey' not in user_dict + assert 'reset_key' not in user_dict + + # Passwords should never be available + assert 'password' not in user_dict From 7fe6c76a5408e41c5693c7ab4b4217e1547f9da4 Mon Sep 17 00:00:00 2001 From: Ian Murray Date: Wed, 1 Aug 2012 13:53:40 +0100 Subject: [PATCH 2/5] [#2784] Allow users to see their apikey when creating or updating themselves. --- ckan/lib/dictization/model_dictize.py | 3 +- ckan/logic/action/create.py | 17 +++++---- ckan/tests/functional/api/test_user.py | 50 ++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 7 deletions(-) diff --git a/ckan/lib/dictization/model_dictize.py b/ckan/lib/dictization/model_dictize.py index b1d354bd794..58c4fc6c5f3 100644 --- a/ckan/lib/dictization/model_dictize.py +++ b/ckan/lib/dictization/model_dictize.py @@ -376,7 +376,8 @@ def user_dictize(user, context): requester = context['user'] if not (ckan.authz.Authorizer().is_sysadmin(unicode(requester)) or - requester == user.name): + requester == user.name or + context.get('keep_sensitive_data', False)): # If not sysadmin or the same user, strip sensible info result_dict.pop('apikey', None) result_dict.pop('reset_key', None) diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index d67590fc872..26d013356f1 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -684,13 +684,18 @@ def user_create(context, data_dict): if not context.get('defer_commit'): model.repo.commit() - # Construct the user dict before changing the context. + # A new context is required for dictizing the newly constructed user in + # order that all the new user's data is returned, in particular, the + # api_key. # - # TODO: I don't know what the need for changing the context is, probably - # caching of the domain object. But it doesn't seem right given that - # usually context['user'] contains the user who made the request. - user_dict = model_dictize.user_dictize(user, context) - + # The context is copied so as not to clobber the caller's context dict. + user_dictize_context = context.copy() + user_dictize_context['keep_sensitive_data'] = True + user_dict = model_dictize.user_dictize(user, user_dictize_context) + + # TODO: I don't know what the need for changing the context is here, + # probably caching of the domain object. But it doesn't seem right given + # that usually context['user'] contains the user who made the request. context['user'] = user context['id'] = user.id log.debug('Created user %s' % str(user.name)) diff --git a/ckan/tests/functional/api/test_user.py b/ckan/tests/functional/api/test_user.py index 13ce706f2f4..06dd35dd7fc 100644 --- a/ckan/tests/functional/api/test_user.py +++ b/ckan/tests/functional/api/test_user.py @@ -1,5 +1,6 @@ from nose.tools import assert_equal +import ckan.logic as logic from ckan import model from ckan.lib.create_test_data import CreateTestData from ckan.tests import TestController as ControllerTestCase @@ -50,3 +51,52 @@ def test_autocomplete_limit(self): print response.json assert_equal(len(response.json), 1) +class TestUserActions(object): + + @classmethod + def setup_class(cls): + CreateTestData.create() + + @classmethod + def teardown_class(cls): + model.repo.rebuild_db() + + def test_user_create_simple(self): + '''Simple creation of a new user by a non-sysadmin user.''' + context = { + 'model': model, + 'session': model.Session, + 'user': 'tester' + } + data_dict = { + 'name': 'a-new-user', + 'email': 'a.person@example.com', + 'password': 'supersecret', + } + + user_dict = logic.get_action('user_create')(context, data_dict) + + assert_equal(user_dict['name'], 'a-new-user') + assert 'email' in user_dict + assert 'apikey' in user_dict + assert 'password' not in user_dict + + def test_user_update_simple(self): + '''Simple update of a user by themselves.''' + context = { + 'model': model, + 'session': model.Session, + 'user': 'annafan', + } + + data_dict = { + 'id': 'annafan', + 'email': 'anna@example.com', + } + + user_dict = logic.get_action('user_update')(context, data_dict) + + assert_equal(user_dict['email'], 'anna@example.com') + assert 'apikey' in user_dict + assert 'password' not in user_dict + From 21ca66c77e9a618cec24709430e73c4409c1ac2c Mon Sep 17 00:00:00 2001 From: Ian Murray Date: Wed, 1 Aug 2012 15:06:27 +0100 Subject: [PATCH 3/5] [#2784] User email is treated as sensitive data --- ckan/controllers/user.py | 8 +++++++- ckan/lib/dictization/model_dictize.py | 1 + ckan/tests/lib/test_dictization.py | 9 ++++++--- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/ckan/controllers/user.py b/ckan/controllers/user.py index 4a0578a3dee..ca85c85597b 100644 --- a/ckan/controllers/user.py +++ b/ckan/controllers/user.py @@ -388,12 +388,18 @@ def request_reset(self): def perform_reset(self, id): context = {'model': model, 'session': model.Session, - 'user': c.user} + 'user': c.user, + 'keep_sensitive_data': True} data_dict = {'id': id} try: user_dict = get_action('user_show')(context, data_dict) + + # Be a little paranoid, and get rid of sensitive data that's + # not needed. + user_dict.pop('apikey', None) + user_dict.pop('reset_key', None) user_obj = context['user_obj'] except NotFound, e: abort(404, _('User not found')) diff --git a/ckan/lib/dictization/model_dictize.py b/ckan/lib/dictization/model_dictize.py index 58c4fc6c5f3..cda8b7f69c9 100644 --- a/ckan/lib/dictization/model_dictize.py +++ b/ckan/lib/dictization/model_dictize.py @@ -381,6 +381,7 @@ def user_dictize(user, context): # If not sysadmin or the same user, strip sensible info result_dict.pop('apikey', None) result_dict.pop('reset_key', None) + result_dict.pop('email', None) model = context['model'] session = model.Session diff --git a/ckan/tests/lib/test_dictization.py b/ckan/tests/lib/test_dictization.py index 71c70835bd6..a7806087b13 100644 --- a/ckan/tests/lib/test_dictization.py +++ b/ckan/tests/lib/test_dictization.py @@ -894,7 +894,6 @@ def test_16_group_dictized(self): 'users': [{'about': u'I love reading Annakarenina. My site: anna.com', 'display_name': u'annafan', 'capacity' : 'public', - 'email': None, 'email_hash': 'd41d8cd98f00b204e9800998ecf8427e', 'fullname': None, 'name': u'annafan', @@ -1088,6 +1087,7 @@ def test_22_user_dictize_as_sysadmin(self): # Check sensitive data is available assert 'apikey' in user_dict assert 'reset_key' in user_dict + assert 'email' in user_dict # Passwords should never be available assert 'password' not in user_dict @@ -1111,6 +1111,7 @@ def test_23_user_dictize_as_same_user(self): # Check sensitive data is available assert 'apikey' in user_dict assert 'reset_key' in user_dict + assert 'email' in user_dict # Passwords should never be available assert 'password' not in user_dict @@ -1131,9 +1132,10 @@ def test_24_user_dictize_as_other_user(self): assert 'name' in user_dict assert 'about' in user_dict - # Check sensitive data is available + # Check sensitive data is not available assert 'apikey' not in user_dict assert 'reset_key' not in user_dict + assert 'email' not in user_dict # Passwords should never be available assert 'password' not in user_dict @@ -1154,9 +1156,10 @@ def test_25_user_dictize_as_anonymous(self): assert 'name' in user_dict assert 'about' in user_dict - # Check sensitive data is available + # Check sensitive data is not available assert 'apikey' not in user_dict assert 'reset_key' not in user_dict + assert 'email' not in user_dict # Passwords should never be available assert 'password' not in user_dict From d7408ff17d167d753b08acd62747d661d664e833 Mon Sep 17 00:00:00 2001 From: amercader Date: Fri, 3 Aug 2012 16:25:48 +0100 Subject: [PATCH 4/5] [#2784] Set user domain object in 'user_obj' in context instead of 'user' --- ckan/logic/action/create.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/ckan/logic/action/create.py b/ckan/logic/action/create.py index 26d013356f1..7d9b8812e26 100644 --- a/ckan/logic/action/create.py +++ b/ckan/logic/action/create.py @@ -693,10 +693,7 @@ def user_create(context, data_dict): user_dictize_context['keep_sensitive_data'] = True user_dict = model_dictize.user_dictize(user, user_dictize_context) - # TODO: I don't know what the need for changing the context is here, - # probably caching of the domain object. But it doesn't seem right given - # that usually context['user'] contains the user who made the request. - context['user'] = user + context['user_obj'] = user context['id'] = user.id log.debug('Created user %s' % str(user.name)) return user_dict From f67c571be2c0653a9ae3d67a9a35e61a7329541e Mon Sep 17 00:00:00 2001 From: tobes Date: Wed, 1 Aug 2012 21:01:09 +0100 Subject: [PATCH 5/5] [#2755] Don't call user_show twice --- ckan/controllers/user.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/ckan/controllers/user.py b/ckan/controllers/user.py index ca85c85597b..13e60a93441 100644 --- a/ckan/controllers/user.py +++ b/ckan/controllers/user.py @@ -107,10 +107,6 @@ def read(self, id=None): abort(401, _('Not authorized to see this page')) context['with_related'] = True - try: - user_dict = get_action('user_show')(context,data_dict) - except NotFound: - h.redirect_to(controller='user', action='login', id=None) self._setup_template_variables(context, data_dict)