Rabbitmq user permission fixes (#49404)

* Simplify permission changing code for rabbitmq_user module

* Add check for multiple permission dicts for same host to rabbitmq_user module

* Add docstring for _get_permission method of rabbitmq_user

* Fix method that compares vhost permissions in rabbitmq_user

* Add tests for rabbitmq_user module

* Add helper function for simulating collections.Counter functionality

lib/ansible/module_utils/common/collections.py

@@ -39,3 +39,18 @@ def is_sequence(seq, include_strings=False):
         return False
 
     return isinstance(seq, Sequence)
+
+
+def count(seq):
+    """Returns a dictionary with the number of appearances of each element of the iterable.
+
+    Resembles the collections.Counter class functionality. It is meant to be used when the
+    code is run on Python 2.6.* where collections.Counter is not available. It should be
+    deprecated and replaced when support for Python < 2.7 is dropped.
+    """
+    if not is_iterable(seq):
+        raise Exception('Argument provided  is not an iterable')
+    counters = dict()
+    for elem in seq:
+        counters[elem] = counters.get(elem, 0) + 1
+    return counters

lib/ansible/modules/messaging/rabbitmq/rabbitmq_user.py

@@ -117,9 +117,9 @@ EXAMPLES = '''
         write_priv: .*
     state: present
 '''
-import operator
 
 from ansible.module_utils.basic import AnsibleModule
+from ansible.module_utils.common.collections import count
 
 
 class RabbitMqUser(object):
@@ -173,6 +173,7 @@ class RabbitMqUser(object):
         return False
 
     def _get_permissions(self):
+        """Get permissions of the user from RabbitMQ."""
         perms_out = [perm for perm in self._exec(['list_user_permissions', self.username], True) if perm.strip()]
 
         perms_list = list()
@@ -211,28 +212,28 @@ class RabbitMqUser(object):
         self._exec(['set_user_tags', self.username] + self.tags)
 
     def set_permissions(self):
-        for permission in self._permissions:
-            if permission not in self.permissions:
-                cmd = ['clear_permissions', '-p']
-                cmd.append(permission['vhost'])
-                cmd.append(self.username)
-                self._exec(cmd)
-        for permission in self.permissions:
-            if permission not in self._permissions:
-                cmd = ['set_permissions', '-p']
-                cmd.append(permission['vhost'])
-                cmd.append(self.username)
-                cmd.append(permission['configure_priv'])
-                cmd.append(permission['write_priv'])
-                cmd.append(permission['read_priv'])
-                self._exec(cmd)
+        permissions_to_clear = [permission for permission in self._permissions if permission not in self.permissions]
+        permissions_to_add = [permission for permission in self.permissions if permission not in self._permissions]
+        for permission in permissions_to_clear:
+            cmd = 'clear_permissions -p {vhost} {username}'.format(username=self.username,
+                                                                   vhost=permission['vhost'])
+            self._exec(cmd.split(' '))
+        for permission in permissions_to_add:
+            cmd = ('set_permissions -p {vhost} {username} {configure_priv} {write_priv} {read_priv}'
+                   .format(username=self.username, **permission))
+            self._exec(cmd.split(' '))
 
     def has_tags_modifications(self):
         return set(self.tags) != set(self._tags)
 
     def has_permissions_modifications(self):
-        sort_key_fetch = operator.itemgetter('vhost')
-        return sorted(self._permissions, key=sort_key_fetch) != sorted(self.permissions, key=sort_key_fetch)
+        def to_permission_tuple(vhost_permission_dict):
+            return vhost_permission_dict['vhost'], vhost_permission_dict
+
+        def permission_dict(vhost_permission_list):
+            return dict(map(to_permission_tuple, vhost_permission_list))
+
+        return permission_dict(self._permissions) != permission_dict(self.permissions)
 
 
 def main():
@@ -268,8 +269,12 @@ def main():
     node = module.params['node']
     update_password = module.params['update_password']
 
+    if permissions:
+        vhosts = map(lambda permission: permission.get('vhost', '/'), permissions)
+        if any(map(lambda count: count > 1, count(vhosts).values())):
+            module.fail_json(msg="Error parsing permissions: You can't have two permission dicts for the same vhost")
         bulk_permissions = True
-    if not permissions:
+    else:
         perm = {
             'vhost': vhost,
             'configure_priv': configure_priv,
@@ -283,7 +288,6 @@ def main():
                                  node, bulk_permissions=bulk_permissions)
 
     result = dict(changed=False, user=username, state=state)
-
     if rabbitmq_user.get():
         if state == 'absent':
             rabbitmq_user.delete()

test/units/modules/messaging/rabbitmq/test_rabbimq_user.py

@@ -0,0 +1,134 @@
+# -*- coding: utf-8 -*-
+
+from ansible.modules.messaging.rabbitmq import rabbitmq_user
+from units.compat.mock import patch
+from units.modules.utils import AnsibleExitJson, AnsibleFailJson, ModuleTestCase, set_module_args
+
+
+class TestRabbitMQUserModule(ModuleTestCase):
+    def setUp(self):
+        super(TestRabbitMQUserModule, self).setUp()
+        self.module = rabbitmq_user
+
+    def tearDown(self):
+        super(TestRabbitMQUserModule, self).tearDown()
+
+    def _assert(self, exc, attribute, expected_value, msg=""):
+        value = exc.message[attribute] if hasattr(exc, attribute) else exc.args[0][attribute]
+        assert value == expected_value, msg
+
+    def test_without_required_parameters(self):
+        """Failure must occurs when all parameters are missing"""
+        with self.assertRaises(AnsibleFailJson):
+            set_module_args({})
+            self.module.main()
+
+    def test_permissions_with_same_vhost(self):
+        set_module_args({
+            'user': 'someuser',
+            'password': 'somepassword',
+            'state': 'present',
+            'permissions': [{'vhost': '/'}, {'vhost': '/'}],
+        })
+        with patch('ansible.module_utils.basic.AnsibleModule.get_bin_path') as get_bin_path:
+            get_bin_path.return_value = '/rabbitmqctl'
+            try:
+                self.module.main()
+            except AnsibleFailJson as e:
+                self._assert(e, 'failed', True)
+                self._assert(e, 'msg',
+                             "Error parsing permissions: You can't have two permission dicts for the same vhost")
+
+    @patch('ansible.module_utils.basic.AnsibleModule.get_bin_path')
+    @patch('ansible.modules.messaging.rabbitmq.rabbitmq_user.RabbitMqUser.get')
+    @patch('ansible.modules.messaging.rabbitmq.rabbitmq_user.RabbitMqUser.check_password')
+    @patch('ansible.modules.messaging.rabbitmq.rabbitmq_user.RabbitMqUser.has_tags_modifications')
+    @patch('ansible.modules.messaging.rabbitmq.rabbitmq_user.RabbitMqUser.has_permissions_modifications')
+    def test_password_changes_only_when_needed(self, has_permissions_modifications, has_tags_modifications,
+                                               check_password, get, get_bin_path):
+        set_module_args({
+            'user': 'someuser',
+            'password': 'somepassword',
+            'state': 'present',
+            'update_password': 'always',
+        })
+        get.return_value = True
+        get_bin_path.return_value = '/rabbitmqctl'
+        check_password.return_value = True
+        has_tags_modifications.return_value = False
+        has_permissions_modifications.return_value = False
+        try:
+            self.module.main()
+        except AnsibleExitJson as e:
+            self._assert(e, 'changed', False)
+            self._assert(e, 'state', 'present')
+
+    @patch('ansible.module_utils.basic.AnsibleModule.get_bin_path')
+    @patch('ansible.modules.messaging.rabbitmq.rabbitmq_user.RabbitMqUser._exec')
+    @patch('ansible.modules.messaging.rabbitmq.rabbitmq_user.RabbitMqUser._get_permissions')
+    @patch('ansible.modules.messaging.rabbitmq.rabbitmq_user.RabbitMqUser.has_tags_modifications')
+    def test_same_permissions_not_changing(self, has_tags_modifications, _get_permissions, _exec, get_bin_path):
+        set_module_args({
+            'user': 'someuser',
+            'password': 'somepassword',
+            'state': 'present',
+            'permissions': [{'vhost': '/', 'configure_priv': '.*', 'write_priv': '.*', 'read_priv': '.*'}],
+        })
+        _get_permissions.return_value = [{'vhost': '/', 'configure_priv': '.*', 'write_priv': '.*', 'read_priv': '.*'}]
+        _exec.return_value = ['someuser\t[]']
+        get_bin_path.return_value = '/rabbitmqctl'
+        has_tags_modifications.return_value = False
+        try:
+            self.module.main()
+        except AnsibleExitJson as e:
+            self._assert(e, 'changed', False)
+            self._assert(e, 'state', 'present')
+
+    @patch('ansible.module_utils.basic.AnsibleModule.get_bin_path')
+    @patch('ansible.modules.messaging.rabbitmq.rabbitmq_user.RabbitMqUser._exec')
+    @patch('ansible.modules.messaging.rabbitmq.rabbitmq_user.RabbitMqUser._get_permissions')
+    @patch('ansible.modules.messaging.rabbitmq.rabbitmq_user.RabbitMqUser.set_permissions')
+    @patch('ansible.modules.messaging.rabbitmq.rabbitmq_user.RabbitMqUser.has_tags_modifications')
+    def test_permissions_are_fixed(self, has_tags_modifications, set_permissions, _get_permissions, _exec, get_bin_path):
+        set_module_args({
+            'user': 'someuser',
+            'password': 'somepassword',
+            'state': 'present',
+            'permissions': [{'vhost': '/', 'configure_priv': '.*', 'write_priv': '.*', 'read_priv': '.*'}],
+        })
+        set_permissions.return_value = None
+        _get_permissions.return_value = []
+        _exec.return_value = ['someuser\t[]']
+        get_bin_path.return_value = '/rabbitmqctl'
+        has_tags_modifications.return_value = False
+        try:
+            self.module.main()
+        except AnsibleExitJson as e:
+            self._assert(e, 'changed', True)
+            self._assert(e, 'state', 'present')
+            assert set_permissions.call_count == 1
+
+    @patch('ansible.module_utils.basic.AnsibleModule.get_bin_path')
+    @patch('ansible.modules.messaging.rabbitmq.rabbitmq_user.RabbitMqUser._exec')
+    @patch('ansible.modules.messaging.rabbitmq.rabbitmq_user.RabbitMqUser._get_permissions')
+    @patch('ansible.modules.messaging.rabbitmq.rabbitmq_user.RabbitMqUser.set_permissions')
+    @patch('ansible.modules.messaging.rabbitmq.rabbitmq_user.RabbitMqUser.has_tags_modifications')
+    def test_permissions_are_fixed_with_different_host(self, has_tags_modifications, set_permissions, _get_permissions,
+                                                       _exec, get_bin_path):
+        set_module_args({
+            'user': 'someuser',
+            'password': 'somepassword',
+            'state': 'present',
+            'permissions': [{'vhost': '/', 'configure_priv': '.*', 'write_priv': '.*', 'read_priv': '.*'}],
+        })
+        set_permissions.return_value = None
+        _get_permissions.return_value = [{'vhost': 'monitoring', 'configure_priv': '.*', 'write_priv': '.*', 'read_priv': '.*'}]
+        _exec.return_value = ['someuser\t[]']
+        get_bin_path.return_value = '/rabbitmqctl'
+        has_tags_modifications.return_value = False
+        try:
+            self.module.main()
+        except AnsibleExitJson as e:
+            self._assert(e, 'changed', True)
+            self._assert(e, 'state', 'present')
+            assert set_permissions.call_count == 1