User locking

[Keeper-of-the-Keys]

SUMMARY

Adds support for user locking as discussed in #701

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

user

[Keeper-of-the-Keys]

I suspect that the host_all option in user_is_locked can be dropped due to where in the code path the function is called.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.35%. Comparing base (e9845b0) to head (4f0069d).
Report is 1 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (e9845b0) and HEAD (4f0069d). Click for more details.

HEAD has 124 uploads less than BASE

Flag	BASE (e9845b0)	HEAD (4f0069d)
sanity	12	0
units	27	0
integration	112	27

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #702       +/-   ##
===========================================
- Coverage   79.48%   66.35%   -13.14%     
===========================================
  Files          32       17       -15     
  Lines        2823     2648      -175     
  Branches      703      680       -23     
===========================================
- Hits         2244     1757      -487     
- Misses        397      673      +276     
- Partials      182      218       +36     

Flag	Coverage Δ
integration	66.20% <100.00%> (-10.23%)	⬇️
sanity	?
units	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[laurent-indermuehle]

About the host_all. I would say, it depends. Can we lock roles too? What if multiple accounts with the same name are present?
I think the simpler way to know for sure what we are doing is by adding integration tests for all possible scenarii. This way we will see if the accounts locking works the same between MySQL, MariaDB and their differents supported version.

[Keeper-of-the-Keys]

@laurent-indermuehle can you point me to where I would need to write the integration test (and an example from which I can extend to these functions)?

At the moment user_is_locked is called from inside the loop on hosts (for host in hosts:) so unless there is some reason that you can forsee to call the function outside the loop I think I can drop that code (I initially modelled the function on user_exists but based on the actual usage I don't think the hosts_all arg and conditional is needed).

As for roles - I don't know I did not go into that much depth.

[laurent-indermuehle]

Ok let's forget roles for now. We can focus on normal accounts.

I would create a file called: tests/integrations/targets/test_mysql_user/tasks/test_user_locking.yml
Then you must add it to tests/integrations/targets/test_mysql_user/tasks/main.yml at the bottom.

To get a good example, I would look at tests/integrations/targets/test_mysql_user/tasks/test_column_case_sensitive.yml as it is one of the latest we wrote. It uses failed_when instead of assert for better log outputs.
But you can get examples in any other tasks file in that directory.

Then, to either run tests locally (the fastest) or here in GitHub Action (the easiest), you can use our guides: https://github.com/ansible-collections/community.mysql/blob/main/TESTING.md

[Keeper-of-the-Keys]

@laurent-indermuehle
If I want to add the locked attribute to the doc to fix the sanity checks what version should I put in for version_added?

[laurent-indermuehle]

I'm preparing a PR myself. I think next version will be 3.13

[Keeper-of-the-Keys]

Should I also add an example or just attribute documentation?

[Andersson007]

@Keeper-of-the-Keys thanks for the PR!
Could you also please:

• Add a changelog fragment
• Add test cases with check_mode: true and check if the changes haven't been made

[Andersson007]

Suggested change

	locked=dict(type='bool', default='no'),
	locked=dict(type='bool', default='false'),

it's officially recommended

[Andersson007]

Suggested change

	- The default is C(false)
	default: false

[Andersson007]

Suggested change

	locked: yes
	locked: yes

true/false is a recommended way since some time ago, officially. it's not critical here, but in modules docs and files it must be true/false

[Andersson007]

Should I also add an example or just attribute documentation?

Yes, please add a separate dedicated example

[Keeper-of-the-Keys]

Hey I just realized why the integration tests were failing - in user_is_locked I was assuming that ACCOUNT LOCK was the last option in the query.

In the last commit I changed .endswith('ACCOUNT LOCK') to .find('ACCOUNT LOCK') >0, I also removed the host_all flag from the function since that was not really in use.

If this is desired and one of you has time to give me some guidance I don't mind also trying to fix the codecov complaint but it is not obvious to me where that lives.

[Keeper-of-the-Keys]

@laurent-indermuehle @Andersson007 Some of the integration tests seem to be failing on unrelated errors, at least based on me clicking some at random they seem to fail on test_mysql_variables) is there an issue with that test?

Earlier I had errors where users were not unlocked but now it seems like issues unrelated to my changes.

[laurent-indermuehle]

@Keeper-of-the-Keys
Many errors are ignored (the blue-green text ...ignored). Don't fall into the trap of reading those (this is why I prefer failed_when over assert!).

I see those 3 errors is the latest integration tests:

TASK [test_mysql_role : Subtract privileges that are not in the current privileges, which 
should be a no-op] ***
fatal: [testhost]: FAILED! => {"changed": false, "msg": "(1133, \"Can't find any 
matching row in the user table\")"}

TASK [test_mysql_user : Mysql_user Lock user | create locked | Assert that test user is locked] ***
fatal: [testhost]: FAILED! => {"changed": false, "failed_when_result": "The conditional check 
'locked_user_creation.query_result[0][0] is not search('ACCOUNT LOCK')' failed. The error 
was: error while evaluating conditional (locked_user_creation.query_result[0][0] is not 
search('ACCOUNT LOCK')): 'dict object' has no attribute 'query_result'", "msg": "Cannot 
execute SQL 'SHOW CREATE USER 'mysql_locked_user'@'%'' args [None]: (1133, \"Can't
find any matching row in the user table\")"}

TASK [test_mysql_variables : Get server version] *******************************
An exception occurred during task execution. To see the full traceback, use -vvv. The 
error was: pymysql.err.OperationalError: (1141, "There is no such grant defined for 
user 'role2' on host '%'")

I only read (Ⓐstable-2.17, DB: mariadb 10.11.8, connector: pymysql 1.1.1), maybe for MySQL the error related to roles are different/absent?

[Keeper-of-the-Keys]

@laurent-indermuehle I believe I resolved my tests, I am puzzled by the role error you show, user_mod() does not do anything with roles see here.

As an aside I would suggest that the calls to user_mod that have long sequences of None, None in the calling arguments should use named args for everything, I can add that to this PR:

--- a/plugins/modules/mysql_role.py
+++ b/plugins/modules/mysql_role.py
@@ -930,11 +930,12 @@ class Role():
                                                       set_default_role_all=set_default_role_all)
 
         if privs:
-            result = user_mod(self.cursor, self.name, self.host,
-                              None, None, None, None, None, None, None,
-                              privs, append_privs, subtract_privs, None, None,
-                              self.module, None, None, role=True,
-                              maria_role=self.is_mariadb)
+            result = user_mod(cursor=self.cursor, user=self.name, host=self.host,
+                              host_all=None, password=None, encrypted=None, plugin=None,
+                              plugin_auth_string=None, plugin_hash_string=None, salt=None,
+                              new_priv=privs, append_privs=append_privs, subtract_privs=subtract_privs,
+                              attributes=None, tls_requires=None, module=self.module, password_expire=None,
+                              password_expire_interval=None, role=True, maria_role=self.is_mariadb)
             changed = result['changed']
 
         if admin:
--- a/plugins/modules/mysql_user.py
+++ b/plugins/modules/mysql_user.py
@@ -594,13 +594,15 @@ def main():
                     result = user_mod(cursor, user, host, host_all, password, encrypted,
                                       plugin, plugin_hash_string, plugin_auth_string, salt,
                                       priv, append_privs, subtract_privs, attributes, tls_requires, module,
-                                      password_expire, password_expire_interval, locked)
+                                      password_expire, password_expire_interval, locked=locked)
 
                 else:
-                    result = user_mod(cursor, user, host, host_all, None, encrypted,
-                                      None, None, None, None,
-                                      priv, append_privs, subtract_privs, attributes, tls_requires, module,
-                                      password_expire, password_expire_interval, locked)
+                    result = user_mod(cursor=cursor, user=user, host=host, host_all=host_all, password=None,
+                                      encrypted=encrypted, plugin=None, plugin_hash_string=None, plugin_auth_string=None,
+                                      salt=None, new_priv=priv, append_privs=append_privs, subtract_privs=subtract_privs,
+                                      attributes=attributes, tls_requires=tls_requires, module=module,
+                                      password_expire=password_expire, password_expire_interval=password_expire_interval,
+                                      locked=locked)

[laurent-indermuehle]

@Keeper-of-the-Keys we should avoid mixing different things in the same PR.

Regarding the error about roles. I'm getting the exact same error on my PR and locally. It's only happening with MariaDB: pymysql.err.OperationalError: (1141, "There is no such grant defined for user 'PUBLIC' on host '%'"). Maybe Docker did change something in there library/MariaDB containers !?! I'll continue to investigate.

You PR start to look good! You are doing a great job so far! I'll try to review your changes today.

[Andersson007]

@Keeper-of-the-Keys thanks! the following is still relevant. See my comment below about handling module.check_mode

• Add test cases with check_mode: true and check if the changes haven't been made

[Keeper-of-the-Keys]

@Andersson007 I added a test case for locking/unlocking should there also be a testcase where no action is needed? (ie. locked: true on an already locked account?

Which I assume would fail if changed?

[laurent-indermuehle]

Yes please @Keeper-of-the-Keys. Tests should cover every possible scenarios.
Yes, having a test that checks for idempotence is a common usage!

[Keeper-of-the-Keys]

@laurent-indermuehle done

[laurent-indermuehle]

Great.
Now I think we must do what you suggested earlier about the mysql_role module. Because you added a new argument (locked) in the middle of the argument chain, you need to adapt the calls to user_mod in plugins/modules/mysql_role.py line 933 @Keeper-of-the-Keys.

[Keeper-of-the-Keys]

@laurent-indermuehle sure I can do that, but unless I am very much mistaken it is not needed because the role arguments are already named, my suggestion to make everything named was mainly because I had to copy the function declaration and check all the positional arguments to make sure that I was correct.

(Also once we switch to named all the Nones that are default=None can be dropped)

[Keeper-of-the-Keys]

@laurent-indermuehle @Andersson007 Is there anything further that I can do to help push this over the edge?

[laurent-indermuehle]

@Keeper-of-the-Keys sorry but you haven't finished, there is errors regarding roles to fix. Maybe you can't see the CI results? For me they are displayed just above the field "Add a comment" in this conversation and starts with "Plugins CI / Integration(...)".
Let me know if you can't see them.

[Keeper-of-the-Keys]

@laurent-indermuehle sorry I was under the impression that that issue was happening regardless of this PR which is why I ignored the CI, I made the change as requested but as I pointed out user_mod for a role does nothing and mysql_role does not call user_add so I am at a loss to explain why this PR would be the cause of that failure but I'll look again.

[Keeper-of-the-Keys]

I think I figured it out, just pushed the fix, user_is_locked was running always and would fail for roles.

[Keeper-of-the-Keys]

@laurent-indermuehle As far as I can tell the issue with test_mysql_role is resolved.

As an aside - any idea why search on the CI output does not work? I need to scroll through the output to find the failed tests every time and when I try to search for a string I know is there (like test_mysql_role or no-op) it say 0 results unless that bit of output is showing, which was a contributing factor to me having more difficulty interacting with the CI tests.

(I'm using Firefox)

[laurent-indermuehle]

@Keeper-of-the-Keys great investigation! And Thank you for your time!

I feel your pain about the output. I have difficulties too (using Vivaldi). I think you have to let it finish loading before you can search. Searching too soon make it stop loading.
Alternatively, you can download the output, or "view raw log". You lose the colors, but at least you're able the search !

[laurent-indermuehle]

I've found one issue:

TASK [test_mysql_user : Mysql_user Lock user | create locked | Unlock test user
 check_mode: true] ***
fatal: [testhost]: FAILED! => {"changed": false, "msg": "Unsupported parameters 
for (community.mysql.mysql_user) module: check_mode. Supported parameters
 include: append_privs, attributes, ca_cert, check_hostname, check_implicit_admin,
 client_cert, client_key, column_case_sensitive, config_file, connect_timeout, 
encrypted, force_context, host, host_all, locked, login_host, login_password, 
login_port, login_unix_socket, login_user, name, password, password_expire, 
password_expire_interval, plugin, plugin_auth_string, plugin_hash_string, priv, 
resource_limits, salt, session_vars, sql_log_bin, state, subtract_privs, tls_requires, 
update_password (ssl_ca, ssl_cert, ssl_key, user)."}

I'm off now, I'll continue tomorrow. Feel free to ping me in case you need my help.