[#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

@@ -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):

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'])

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)

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)

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}