mysql_user: when grant select on columns, the module always report the state has changed (#100)

* mysql_user: fix the module is not idempotent when there is SELECT on columns granted

* add changelog fragment

* fix

* Add unit tests for has_select_on_col function

* Add unit tests for sort_column_order function

* Add unit tests for handle_select_on_col function

* Update a comment

changelogs/fragments/100-mysql_user_not_idemponent_when_select_on_columns_granted.yml

@@ -0,0 +1,2 @@
+bugfixes:
+- mysql_user - the module is not idempotent when SELECT on columns granted (https://github.com/ansible-collections/community.mysql/issues/99).

plugins/modules/mysql_user.py

@@ -768,6 +768,17 @@ def privileges_get(cursor, user, host):
             raise InvalidPrivsError('unable to parse the MySQL grant string: %s' % grant[0])
         privileges = res.group(1).split(",")
         privileges = [pick(x.strip()) for x in privileges]
+
+        # Handle cases when there's GRANT SELECT (colA, ...) in privileges.
+        # To this point, the privileges list can look like
+        # ['SELECT (`A`', '`B`)', 'INSERT'] that is incorrect (SELECT statement is splitted).
+        # Columns should also be sorted to compare it with desired privileges later.
+        # Determine if there's a case similar to the above:
+        start, end = has_select_on_col(privileges)
+        # If not, either start and end will be None
+        if start is not None:
+            privileges = handle_select_on_col(privileges, start, end)
+
         if "WITH GRANT OPTION" in res.group(7):
             privileges.append('GRANT')
         if 'REQUIRE SSL' in res.group(7):
@@ -777,6 +788,102 @@ def privileges_get(cursor, user, host):
     return output
 
 
+def has_select_on_col(privileges):
+    """Check if there is a statement like SELECT (colA, colB)
+    in the privilege list.
+
+    Return (start index, end index).
+    """
+    # Determine elements of privileges where
+    # columns are listed
+    start = None
+    end = None
+    for n, priv in enumerate(privileges):
+        if 'SELECT (' in priv:
+            # We found the start element
+            start = n
+
+        if start is not None and ')' in priv:
+            # We found the end element
+            end = n
+            break
+
+    if start is not None and end is not None:
+        # if the privileges list consist of, for example,
+        # ['SELECT (A', 'B), 'INSERT'], return indexes of related elements
+        return start, end
+    else:
+        # If start and end position is the same element,
+        # it means there's expression like 'SELECT (A)',
+        # so no need to handle it
+        return None, None
+
+
+def handle_select_on_col(privileges, start, end):
+    """Handle cases when the SELECT (colA, ...) is in the privileges list."""
+    # When the privileges list look like ['SELECT (colA,', 'colB)']
+    # (Notice that the statement is splitted)
+    if start != end:
+        output = list(privileges[:start])
+
+        select_on_col = ', '.join(privileges[start:end + 1])
+
+        select_on_col = sort_column_order(select_on_col)
+
+        output.append(select_on_col)
+
+        output.extend(privileges[end + 1:])
+
+    # When it look like it should be, e.g. ['SELECT (colA, colB)'],
+    # we need to be sure, the columns is sorted
+    else:
+        output = list(privileges)
+        output[start] = sort_column_order(output[start])
+
+    return output
+
+
+def sort_column_order(statement):
+    """Sort column order in SELECT (colA, colB, ...) grants.
+
+    MySQL changes columns order like below:
+    ---------------------------------------
+    mysql> GRANT SELECT (testColA, testColB), INSERT ON `testDb`.`testTable` TO 'testUser'@'localhost';
+    Query OK, 0 rows affected (0.04 sec)
+
+    mysql> flush privileges;
+    Query OK, 0 rows affected (0.00 sec)
+
+    mysql> SHOW GRANTS FOR testUser@localhost;
+    +---------------------------------------------------------------------------------------------+
+    | Grants for testUser@localhost                                                               |
+    +---------------------------------------------------------------------------------------------+
+    | GRANT USAGE ON *.* TO 'testUser'@'localhost'                                                |
+    | GRANT SELECT (testColB, testColA), INSERT ON `testDb`.`testTable` TO 'testUser'@'localhost' |
+    +---------------------------------------------------------------------------------------------+
+
+    We should sort columns in our statement, otherwise the module always will return
+    that the state has changed.
+    """
+    # 1. Extract stuff inside ()
+    # 2. Split
+    # 3. Sort
+    # 4. Put between () and return
+
+    # "SELECT (colA, colB) => "colA, colB"
+    columns = statement.split('(')[1].rstrip(')')
+
+    # "colA, colB" => ["colA", "colB"]
+    columns = columns.split(',')
+
+    for i, col in enumerate(columns):
+        col = col.strip()
+        columns[i] = col.strip('`')
+
+    columns.sort()
+    return 'SELECT (%s)' % ', '.join(columns)
+
+
 def privileges_unpack(priv, mode):
     """ Take a privileges string, typically passed as a parameter, and unserialize
     it into a dictionary, the same format as privileges_get() above. We have this
@@ -819,6 +926,12 @@ def privileges_unpack(priv, mode):
         else:
             output[pieces[0]] = pieces[1].upper().split(',')
             privs = output[pieces[0]]
+
+        # Handle cases when there's GRANT SELECT (colA, ...) in privs.
+        start, end = has_select_on_col(output[pieces[0]])
+        if start is not None:
+            output[pieces[0]] = handle_select_on_col(output[pieces[0]], start, end)
+
         new_privs = frozenset(privs)
         if not new_privs.issubset(VALID_PRIVS):
             raise InvalidPrivsError('Invalid privileges specified: %s' % new_privs.difference(VALID_PRIVS))

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

@@ -16,6 +16,7 @@
       loop:
       - data1
       - data2
+      - data3
 
     - name: Create user with privileges
       mysql_user:
@@ -37,6 +38,61 @@
           - "'GRANT SELECT ON `data1`.*' in result.stdout"
           - "'GRANT SELECT ON `data2`.*' in result.stdout"
 
+    # Issue https://github.com/ansible-collections/community.mysql/issues/99
+    - name: Create test table test_table_issue99
+      mysql_query:
+        <<: *mysql_params
+        query: "CREATE TABLE IF NOT EXISTS data3.test_table_issue99 (a INT, b INT, c INT)"
+
+    - name: Grant select on a column
+      mysql_user:
+        <<: *mysql_params
+        name: '{{ user_name_3 }}'
+        priv:
+          'data3.test_table_issue99': 'SELECT (a)'
+      register: result
+
+    - assert:
+        that:
+        - result is changed
+
+    - name: Grant select on the column again
+      mysql_user:
+        <<: *mysql_params
+        name: '{{ user_name_3 }}'
+        priv:
+          'data3.test_table_issue99': 'SELECT (a)'
+      register: result
+
+    - assert:
+        that:
+        - result is not changed
+
+
+    - name: Grant select on columns
+      mysql_user:
+        <<: *mysql_params
+        name: '{{ user_name_3 }}'
+        priv:
+          'data3.test_table_issue99': 'SELECT (a, b),INSERT'
+      register: result
+
+    - assert:
+        that:
+        - result is changed
+
+    - name: Grant select on columns again
+      mysql_user:
+        <<: *mysql_params
+        name: '{{ user_name_3 }}'
+        priv:
+          'data3.test_table_issue99': 'SELECT (a, b),INSERT'
+      register: result
+
+    - assert:
+        that:
+        - result is not changed
+
     ##########
     # Clean up
     - name: Drop test databases
@@ -47,6 +103,7 @@
       loop:
       - data1
       - data2
+      - data3
 
     - name: Drop test user
       mysql_user:

tests/unit/plugins/modules/test_mysql_user.py

@@ -1,9 +1,16 @@
+# -*- coding: utf-8 -*-
+
 from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
 import pytest
 
-from ansible_collections.community.mysql.plugins.modules.mysql_user import supports_identified_by_password
+from ansible_collections.community.mysql.plugins.modules.mysql_user import (
+    handle_select_on_col,
+    has_select_on_col,
+    sort_column_order,
+    supports_identified_by_password,
+)
 from ..utils import dummy_cursor_class
 
 
@@ -26,8 +33,58 @@ from ..utils import dummy_cursor_class
 )
 def test_supports_identified_by_password(function_return, cursor_output, cursor_ret_type):
     """
-    Tests whether 'CREATE USER %s@%s IDENTIFIED BY PASSWORD %s' is supported, which is currently supported by everything
+    Tests whether 'CREATE USER %s@%s IDENTIFIED BY PASSWORD %s' is supported,
-    besides MySQL >= 8.0.
+    which is currently supported by everything besides MySQL >= 8.0.
     """
     cursor = dummy_cursor_class(cursor_output, cursor_ret_type)
     assert supports_identified_by_password(cursor) == function_return
+
+
+@pytest.mark.parametrize(
+    'input_list,output_tuple',
+    [
+        (['INSERT', 'DELETE'], (None, None)),
+        (['SELECT', 'UPDATE'], (None, None)),
+        (['INSERT', 'UPDATE', 'INSERT', 'DELETE'], (None, None)),
+        (['just', 'a', 'random', 'text'], (None, None)),
+        (['SELECT (A, B, C)'], (0, 0)),
+        (['UPDATE', 'SELECT (A, B, C)'], (1, 1)),
+        (['INSERT', 'SELECT (A', 'B)'], (1, 2)),
+        (['SELECT (A', 'B)', 'UPDATE'], (0, 1)),
+        (['INSERT', 'SELECT (A', 'B)', 'UPDATE'], (1, 2)),
+    ]
+)
+def test_has_select_on_col(input_list, output_tuple):
+    """Tests has_select_on_col function."""
+    assert has_select_on_col(input_list) == output_tuple
+
+
+@pytest.mark.parametrize(
+    'input_,output',
+    [
+        ('SELECT (A)', 'SELECT (A)'),
+        ('SELECT (`A`)', 'SELECT (A)'),
+        ('SELECT (A, B)', 'SELECT (A, B)'),
+        ('SELECT (`A`, `B`)', 'SELECT (A, B)'),
+        ('SELECT (B, A)', 'SELECT (A, B)'),
+        ('SELECT (`B`, `A`)', 'SELECT (A, B)'),
+        ('SELECT (`B`, `A`, C)', 'SELECT (A, B, C)'),
+    ]
+)
+def test_sort_column_order(input_, output):
+    """Tests sort_column_order function."""
+    assert sort_column_order(input_) == output
+
+
+@pytest.mark.parametrize(
+    'privileges,start,end,output',
+    [
+        (['UPDATE', 'SELECT (C, B, A)'], 1, 1, ['UPDATE', 'SELECT (A, B, C)']),
+        (['INSERT', 'SELECT (A', 'B)'], 1, 2, ['INSERT', 'SELECT (A, B)']),
+        (['SELECT (`A`', 'B)', 'UPDATE'], 0, 1, ['SELECT (A, B)', 'UPDATE']),
+        (['INSERT', 'SELECT (`B`', 'A', 'C)', 'UPDATE'], 1, 3, ['INSERT', 'SELECT (A, B, C)', 'UPDATE']),
+    ]
+)
+def test_handle_select_on_col(privileges, start, end, output):
+    """Tests handle_select_on_col function."""
+    assert handle_select_on_col(privileges, start, end) == output