From 9f80c0f318e149bd311c462d9b750edfdecf8d06 Mon Sep 17 00:00:00 2001 From: Vitor Baptista Date: Fri, 14 Feb 2014 21:09:22 -0300 Subject: [PATCH] [#1505] Fix bug when non-sysadmins invited users to organizations The problem was with the .user_invite() auth function. Instead of adding 'id' to the data_dict, I was adding it to the context. Fixed now. I've also changed it to require a data_dict, and a data_dict['group_id'] (as they're required anyway). While I'm at it, I rewrote the tests into the new testing style. --- ckan/logic/auth/create.py | 4 +- ckan/new_tests/logic/action/test_create.py | 92 ++++++++++++++++++++++ ckan/new_tests/logic/auth/test_create.py | 29 +++++++ ckan/tests/logic/test_action.py | 69 ---------------- ckan/tests/logic/test_auth.py | 10 --- 5 files changed, 123 insertions(+), 81 deletions(-) create mode 100644 ckan/new_tests/logic/action/test_create.py create mode 100644 ckan/new_tests/logic/auth/test_create.py diff --git a/ckan/logic/auth/create.py b/ckan/logic/auth/create.py index 0c7be8de7a4..e9082fc6b77 100644 --- a/ckan/logic/auth/create.py +++ b/ckan/logic/auth/create.py @@ -121,8 +121,8 @@ def user_create(context, data_dict=None): 'create users')} return {'success': True} -def user_invite(context, data_dict=None): - context['id'] = context.get('group_id') +def user_invite(context, data_dict): + data_dict['id'] = data_dict['group_id'] return group_member_create(context, data_dict) def _check_group_auth(context, data_dict): diff --git a/ckan/new_tests/logic/action/test_create.py b/ckan/new_tests/logic/action/test_create.py new file mode 100644 index 00000000000..c6bb410377b --- /dev/null +++ b/ckan/new_tests/logic/action/test_create.py @@ -0,0 +1,92 @@ +'''Unit tests for ckan/logic/auth/create.py. + +''' + +import mock +import nose.tools + +import ckan.new_tests.helpers as helpers +import ckan.new_tests.factories as factories +import ckan.model +import ckan.logic + + +class TestUserInvite(object): + + def setup(self): + helpers.reset_db() + + @mock.patch('ckan.lib.mailer.send_invite') + def test_user_invite(self, send_invite): + invited_user = self._invite_user_to_group() + + assert invited_user is not None + + @mock.patch('ckan.lib.mailer.send_invite') + def test_user_invite_creates_pending_user(self, _): + invited_user = self._invite_user_to_group() + + assert invited_user.is_pending() + + @mock.patch('ckan.lib.mailer.send_invite') + def test_user_invite_creates_user_with_valid_username(self, _): + email = 'user$%+abc@email.com' + invited_user = self._invite_user_to_group(email) + + assert invited_user.name.startswith('user---abc'), invited_user + + @mock.patch('ckan.lib.mailer.send_invite') + def test_user_invite_assigns_user_to_group_in_expected_role(self, _): + role = 'admin' + invited_user = self._invite_user_to_group(role=role) + + group_ids = invited_user.get_group_ids(capacity=role) + assert len(group_ids) == 1, group_ids + + @mock.patch('ckan.lib.mailer.send_invite') + def test_user_invite_sends_invite(self, send_invite): + invited_user = self._invite_user_to_group() + + assert send_invite.called + assert send_invite.call_args[0][0].id == invited_user.id + + @mock.patch('ckan.lib.mailer.send_invite') + @mock.patch('random.SystemRandom') + def test_user_invite_works_even_if_tried_username_already_exists(self, SystemRandom, _): + SystemRandom.return_value.random.side_effect = [1000, 1000, 1000, 2000, 3000, 4000, 5000] + + for _ in range(3): + invited_user = self._invite_user_to_group(email='same@email.com') + assert invited_user is not None, invited_user + + @mock.patch('ckan.lib.mailer.send_invite') + @nose.tools.raises(ckan.logic.ValidationError) + def test_user_invite_requires_email(self, _): + self._invite_user_to_group(email=None) + + @mock.patch('ckan.lib.mailer.send_invite') + @nose.tools.raises(ckan.logic.ValidationError) + def test_user_invite_requires_role(self, _): + self._invite_user_to_group(role=None) + + @mock.patch('ckan.lib.mailer.send_invite') + @nose.tools.raises(ckan.logic.ValidationError) + def test_user_invite_requires_group_id(self, _): + self._invite_user_to_group(group={'id': None}) + + def _invite_user_to_group(self, email='user@email.com', group=None, role='member'): + user = factories.User() + group = group or factories.Group(user=user) + + context = { + 'user': user['name'] + } + params = { + 'email': email, + 'group_id': group['id'], + 'role': role + } + + result = helpers.call_action('user_invite', context, **params) + + return ckan.model.User.get(result['id']) diff --git a/ckan/new_tests/logic/auth/test_create.py b/ckan/new_tests/logic/auth/test_create.py new file mode 100644 index 00000000000..43add56edc7 --- /dev/null +++ b/ckan/new_tests/logic/auth/test_create.py @@ -0,0 +1,29 @@ +'''Unit tests for ckan/logic/auth/create.py. + +''' + +import mock + +import ckan.new_tests.helpers as helpers +import ckan.new_tests.factories as factories + + +class TestCreate(object): + def setup(self): + helpers.reset_db() + + @mock.patch('ckan.logic.auth.create.group_member_create') + def test_user_invite_prepares_data_dict_and_delegates_to_group_member_create(self, group_member_create): + user = factories.User() + context = { + 'user': user['name'], + 'model': None, + 'auth_user_obj': user + } + data_dict = {'group_id': 42} + group_member_create_data_dict = data_dict.copy() + group_member_create_data_dict['id'] = data_dict['group_id'] + + helpers.call_auth('user_invite', context=context, **data_dict) + + group_member_create.assert_called_with(context, group_member_create_data_dict) diff --git a/ckan/tests/logic/test_action.py b/ckan/tests/logic/test_action.py index 316d6533a5e..df50e42f5ac 100644 --- a/ckan/tests/logic/test_action.py +++ b/ckan/tests/logic/test_action.py @@ -594,75 +594,6 @@ def test_12_user_update_errors(self): for expected_message in test_call['messages']: assert expected_message[1] in ''.join(res_obj['error'][expected_message[0]]) - @mock.patch('ckan.lib.mailer.send_invite') - def test_user_invite(self, send_invite): - email_username = 'invited_user$ckan' - email = '%s@email.com' % email_username - organization_name = 'an_organization' - CreateTestData.create_groups([{'name': organization_name}]) - role = 'member' - organization = model.Group.get(organization_name) - params = {'email': email, - 'group_id': organization.id, - 'role': role} - postparams = '%s=1' % json.dumps(params) - extra_environ = {'Authorization': str(self.sysadmin_user.apikey)} - - res = self.app.post('/api/action/user_invite', params=postparams, - extra_environ=extra_environ) - - res_obj = json.loads(res.body) - user = model.User.get(res_obj['result']['id']) - assert res_obj['success'] is True, res_obj - assert user.email == email, (user.email, email) - assert user.is_pending(), user - expected_username = email_username.replace('$', '-') - assert user.name.startswith(expected_username), (user.name, - expected_username) - group_ids = user.get_group_ids(capacity=role) - assert organization.id in group_ids, (group_ids, organization.id) - assert send_invite.called - assert send_invite.call_args[0][0].id == res_obj['result']['id'] - - @mock.patch('ckan.lib.mailer.mail_user') - def test_user_invite_without_email_raises_error(self, mail_user): - user_dict = {} - postparams = '%s=1' % json.dumps(user_dict) - extra_environ = {'Authorization': str(self.sysadmin_user.apikey)} - - res = self.app.post('/api/action/user_invite', params=postparams, - extra_environ=extra_environ, - status=StatusCodes.STATUS_409_CONFLICT) - - res_obj = json.loads(res.body) - assert res_obj['success'] is False, res_obj - assert 'email' in res_obj['error'], res_obj - - @mock.patch('random.SystemRandom') - def test_user_invite_should_work_even_if_tried_username_already_exists(self, system_random_mock): - patcher = mock.patch('ckan.lib.mailer.mail_user') - patcher.start() - email = 'invited_user@email.com' - organization_name = 'an_organization' - CreateTestData.create_groups([{'name': organization_name}]) - role = 'member' - organization = model.Group.get(organization_name) - params = {'email': email, - 'group_id': organization.id, - 'role': role} - postparams = '%s=1' % json.dumps(params) - extra_environ = {'Authorization': str(self.sysadmin_user.apikey)} - - system_random_mock.return_value.random.side_effect = [1000, 1000, 2000, 3000] - - for _ in range(2): - res = self.app.post('/api/action/user_invite', params=postparams, - extra_environ=extra_environ) - - res_obj = json.loads(res.body) - assert res_obj['success'] is True, res_obj - patcher.stop() - def test_user_delete(self): name = 'normal_user' CreateTestData.create_user(name) diff --git a/ckan/tests/logic/test_auth.py b/ckan/tests/logic/test_auth.py index e44cf60efd3..2be34db1634 100644 --- a/ckan/tests/logic/test_auth.py +++ b/ckan/tests/logic/test_auth.py @@ -61,16 +61,6 @@ def create_user(cls, name): class TestAuthUsers(TestAuth): - @mock.patch('ckan.logic.auth.create.group_member_create') - def test_invite_user_prepares_context_and_delegates_to_group_member_create(self, group_member_create): - context = {'group_id': 42} - group_member_create_context = context - group_member_create_context['id'] = context['group_id'] - - new_authz.is_authorized_boolean('user_invite', context) - - group_member_create.assert_called(group_member_create_context, None) - def test_only_sysadmins_can_delete_users(self): username = 'username' user = {'id': username}