mysql_user: Fixed change detection with append_privs (#69) (#72)

* mysql_user: Fixed change detection with append_privs (#69)

Prior to this change, mysql_user with append_privs would attempt to make
a change even if the current privileges were a superset of the new
privileges (shouldn't require any action).

* Fixed unrelated mysql_replication doc causing failures in CI

* Added fragments and check_mode tests

* Expanded priv append tests to cover additional case

changelogs/fragments/72-mysql_user_change_detection.yml

@@ -0,0 +1,2 @@
+bugfixes:
+- mysql_user - fixed change detection when using append_privs (https://github.com/ansible-collections/community.mysql/pull/72).

plugins/modules/mysql_replication.py

@@ -82,6 +82,7 @@ options:
     description:
     - Same as mysql variable.
     type: bool
+    default: false
   master_ssl_ca:
     description:
     - Same as mysql variable.
@@ -106,6 +107,7 @@ options:
     description:
     - Whether the host uses GTID based replication or not.
     type: bool
+    default: false
   master_use_gtid:
     description:
     - Configures the slave to use the MariaDB Global Transaction ID.

plugins/modules/mysql_user.py

@@ -665,7 +665,14 @@ def user_mod(cursor, user, host, host_all, password, encrypted,
             # and in the new privileges, then we need to see if there's a difference.
             db_table_intersect = set(new_priv.keys()) & set(curr_priv.keys())
             for db_table in db_table_intersect:
+
+                # If appending privileges, only the set difference between new privileges and current privileges matter.
+                # The symmetric difference isn't relevant for append because existing privileges will not be revoked.
+                if append_privs:
+                    priv_diff = set(new_priv[db_table]) - set(curr_priv[db_table])
+                else:
                     priv_diff = set(new_priv[db_table]) ^ set(curr_priv[db_table])
+
                 if len(priv_diff) > 0:
                     msg = "Privileges updated"
                     if module.check_mode:

tests/integration/targets/test_mysql_user/defaults/main.yml

@@ -9,10 +9,12 @@ db_name: 'data'
 user_name_1: 'db_user1'
 user_name_2: 'db_user2'
 user_name_3: 'db_user3'
+user_name_4: 'db_user4'
 
 user_password_1: 'gadfFDSdtTU^Sdfuj'
 user_password_2: 'jkFKUdfhdso78yi&td'
 user_password_3: 'jkFKUdfhdso78yi&tk'
+user_password_4: 's2R#7pLV31!ZJrXPa3'
 
 root_password: 'zevuR6oPh7'
 

tests/integration/targets/test_mysql_user/tasks/main.yml

@@ -261,6 +261,11 @@
     # Tests for the priv parameter with dict value (https://github.com/ansible/ansible/issues/57533)
     - include: test_priv_dict.yml
 
+    # Test that append_privs will not attempt to make a change where current privileges are a superset of new privileges
+    # (https://github.com/ansible-collections/community.mysql/issues/69)
+    - include: test_priv_append.yml enable_check_mode=no
+    - include: test_priv_append.yml enable_check_mode=yes
+
     # Tests for the TLS requires dictionary
     - include: tls_requirements.yml
 

tests/integration/targets/test_mysql_user/tasks/test_priv_append.yml

@@ -0,0 +1,114 @@
+# Test code to ensure that appending privileges will not result in unnecessary changes when the current privileges
+# are a superset of the new privileges that have been defined.
+- vars:
+    mysql_parameters: &mysql_params
+      login_user: '{{ mysql_user }}'
+      login_password: '{{ mysql_password }}'
+      login_host: 127.0.0.1
+      login_port: '{{ mysql_primary_port }}'
+
+  block:
+
+    - name: Create test databases
+      mysql_db:
+        <<: *mysql_params
+        name: '{{ item }}'
+        state: present
+      loop:
+      - data1
+      - data2
+
+    - name: Create a user with an initial set of privileges
+      mysql_user:
+        <<: *mysql_params
+        name: '{{ user_name_4 }}'
+        password: '{{ user_password_4 }}'
+        priv: 'data1.*:SELECT,INSERT/data2.*:SELECT,DELETE'
+        state: present
+
+    - name: Run command to show privileges for user (expect privileges in stdout)
+      command: "{{ mysql_command }} -e \"SHOW GRANTS FOR '{{ user_name_4 }}'@'localhost'\""
+      register: result
+
+    - name: Assert that the initial set of privileges matches what is expected
+      assert:
+        that:
+          - "'GRANT SELECT, INSERT ON `data1`.*' in result.stdout"
+          - "'GRANT SELECT, DELETE ON `data2`.*' in result.stdout"
+
+    - name: Append privileges that are a subset of the current privileges, which should be a no-op
+      mysql_user:
+        <<: *mysql_params
+        name: '{{ user_name_4 }}'
+        password: '{{ user_password_4 }}'
+        priv: 'data1.*:SELECT/data2.*:SELECT'
+        append_privs: yes
+        state: present
+      check_mode: '{{ enable_check_mode }}'
+      register: result
+
+    - name: Assert that there wasn't a change in permissions
+      assert:
+        that:
+          - "result.changed == false"
+
+    - name: Run command to show privileges for user (expect privileges in stdout)
+      command: "{{ mysql_command }} -e \"SHOW GRANTS FOR '{{ user_name_4 }}'@'localhost'\""
+      register: result
+
+    - name: Assert that the permissions still match what was originally granted
+      assert:
+        that:
+          - "'GRANT SELECT, INSERT ON `data1`.*' in result.stdout"
+          - "'GRANT SELECT, DELETE ON `data2`.*' in result.stdout"
+
+    - name: Append privileges that are not included in the current set of privileges to test that privileges are updated
+      mysql_user:
+        <<: *mysql_params
+        name: '{{ user_name_4 }}'
+        password: '{{ user_password_4 }}'
+        priv: 'data1.*:DELETE/data2.*:SELECT'
+        append_privs: yes
+        state: present
+      check_mode: '{{ enable_check_mode }}'
+      register: result
+
+    - name: Assert that there was a change because permissions were added to data1.*
+      assert:
+        that:
+          - "result.changed == true"
+
+    - name: Run command to show privileges for user (expect privileges in stdout)
+      command: "{{ mysql_command }} -e \"SHOW GRANTS FOR '{{ user_name_4 }}'@'localhost'\""
+      register: result
+
+    - name: Assert that the permissions were changed as expected if check_mode is set to 'no'
+      assert:
+        that:
+          - "'GRANT SELECT, INSERT, DELETE ON `data1`.*' in result.stdout"
+          - "'GRANT SELECT, DELETE ON `data2`.*' in result.stdout"
+      when: enable_check_mode == 'no'
+
+    - name: Assert that the permissions were not actually changed if check_mode is set to 'yes'
+      assert:
+        that:
+          - "'GRANT SELECT, INSERT ON `data1`.*' in result.stdout"
+          - "'GRANT SELECT, DELETE ON `data2`.*' in result.stdout"
+      when: enable_check_mode == 'yes'
+
+    ##########
+    # Clean up
+    - name: Drop test databases
+      mysql_db:
+        <<: *mysql_params
+        name: '{{ item }}'
+        state: present
+      loop:
+      - data1
+      - data2
+
+    - name: Drop test user
+      mysql_user:
+        <<: *mysql_params
+        name: '{{ user_name_4 }}'
+        state: absent