diff --git a/changelogs/fragments/100-mysql_user_not_idemponent_when_select_on_columns_granted.yml b/changelogs/fragments/100-mysql_user_not_idemponent_when_select_on_columns_granted.yml new file mode 100644 index 0000000..3c370dc --- /dev/null +++ b/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). diff --git a/plugins/modules/mysql_user.py b/plugins/modules/mysql_user.py index 4e69853..85d817c 100644 --- a/plugins/modules/mysql_user.py +++ b/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)) diff --git a/tests/integration/targets/test_mysql_user/tasks/test_priv_dict.yml b/tests/integration/targets/test_mysql_user/tasks/test_priv_dict.yml index ec7b05e..938bf07 100644 --- a/tests/integration/targets/test_mysql_user/tasks/test_priv_dict.yml +++ b/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: diff --git a/tests/unit/plugins/modules/test_mysql_user.py b/tests/unit/plugins/modules/test_mysql_user.py index 2dae9c5..ee7c9d7 100644 --- a/tests/unit/plugins/modules/test_mysql_user.py +++ b/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 - besides MySQL >= 8.0. + Tests whether 'CREATE USER %s@%s IDENTIFIED BY PASSWORD %s' is supported, + 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