login tracking [Temporarily frozen] #62

Jorge-Rodriguez · 2020-11-17T11:00:44Z

SUMMARY

Introduce support for MySQL >= 8.0.19 failed login tracking and account locking
Fixes #49

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

mysql_user

codecov · 2020-11-17T11:10:48Z

Codecov Report

Attention: Patch coverage is 5.26316% with 54 lines in your changes missing coverage. Please review.

Project coverage is 15.64%. Comparing base (31bd2b5) to head (f43676c).
Report is 229 commits behind head on main.

Files with missing lines	Patch %	Lines
plugins/modules/mysql_user.py	5.26%	54 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (31bd2b5) and HEAD (f43676c). Click for more details.

HEAD has 97 uploads less than BASE

Flag BASE (31bd2b5) HEAD (f43676c)

100 3

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #62       +/-   ##
===========================================
- Coverage   75.07%   15.64%   -59.43%     
===========================================
  Files          12        8        -4     
  Lines        1645     1016      -629     
  Branches      423      246      -177     
===========================================
- Hits         1235      159     -1076     
- Misses        267      854      +587     
+ Partials      143        3      -140

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

Andersson007

@Jorge-Rodriguez it looks very good! I'll try to make time to review ASAP. Thanks!

plugins/modules/mysql_user.py

Andersson007

@Jorge-Rodriguez in general LGTM, thanks very much for doing this!

changelogs/fragments/49-mysql_user_login_tracking.yml

plugins/modules/mysql_user.py

Andersson007 · 2020-11-23T10:13:02Z

@martinwalle could you please take a look? If you don't know python much, could you look, at least, at tests/integration/targets/test_mysql_user/tasks/issue-49.yml which contains examples of how to use the feature. Maybe EXAMPLES block of module's file would be also helpful, and the option's description in the DOCUMENTATION section.

@bmildren @bmalynovytch could you also please take a look?

martinwalle · 2020-11-30T08:43:29Z

@martinwalle could you please take a look? If you don't know python much, could you look, at least, at tests/integration/targets/test_mysql_user/tasks/issue-49.yml which contains examples of how to use the feature. Maybe EXAMPLES block of module's file would be also helpful, and the option's description in the DOCUMENTATION section.

@bmildren @bmalynovytch could you also please take a look?

Hello @Andersson007 ,

I read the tests and the arguments is good for me.
May I test it to validate ?

Sincerely,
Martin.

Jorge-Rodriguez · 2020-11-30T14:38:11Z

Just need to add the conditional assertions for versions that do not support this feature

Andersson007 · 2020-12-01T05:15:06Z

@martinwalle could you please take a look? If you don't know python much, could you look, at least, at tests/integration/targets/test_mysql_user/tasks/issue-49.yml which contains examples of how to use the feature. Maybe EXAMPLES block of module's file would be also helpful, and the option's description in the DOCUMENTATION section.
@bmildren @bmalynovytch could you also please take a look?

Hello @Andersson007 ,

I read the tests and the arguments is good for me.
May I test it to validate ?

Sincerely,
Martin.

Hey @martinwalle !
It would be great, if you try the changes manually, though I'm not sure @Jorge-Rodriguez has done all what he intended.
@Jorge-Rodriguez when it's ready for review, please put something here. And thanks for steadily working on improving mysql-specific support!

Jorge-Rodriguez · 2020-12-01T11:57:34Z

@Andersson007 @martinwalle only test fixes are missing, the functional code is ready for manual testing.

Andersson007 · 2020-12-04T06:16:55Z

@Jorge-Rodriguez thanks for the information! Would be happy to review once the tests are green
@martinwalle so feel free to try the changes

martinwalle · 2020-12-07T14:54:11Z

@Jorge-Rodriguez @Andersson007
I tested mysql_user with these vars :

 account_locking:
      FAILED_LOGIN_ATTEMPTS: 3
      PASSWORD_LOCK_TIME: 5

  account_locking:
      FAILED_LOGIN_ATTEMPTS: 8
      PASSWORD_LOCK_TIME: 1

   account_locking:
      FAILED_LOGIN_ATTEMPTS: 1
      PASSWORD_LOCK_TIME: UNBOUNDED

    account_locking:
      FAILED_LOGIN_ATTEMPTS: 1
      PASSWORD_LOCK_TIME: 32768
	  
	  {"changed": false, "msg": "Account locking values are out of the valid range (0-32767)"}

All is fine. It is working as expected.
I also test the failed-login. And it worked !
Nice work :)

Andersson007 · 2021-01-15T11:14:09Z

@martinwalle thanks for testing! Sorry for the delayed response, I've been waiting for Jorge's feedback.
@Jorge-Rodriguez any updates on this?

Jorge-Rodriguez · 2021-01-16T13:14:47Z

Sorry everyone. it's been truly hectic lately and I haven't found time to fix the tests.
For what it's worth, there should be nothing functionally wrong with the tests, technically speaking, the failures are valid because the tests are run on platforms that don't support the feature.
I'll be adding conditionals to those tests soon.

Cheers

Jorge-Rodriguez · 2021-01-31T07:32:16Z

The rework needed for these tests is tangentially related to #91 so I'm looking that here as a reminder that however we decide to handle that will probably affect and require a refactor of this.

Co-authored-by: Andrew Klychkov <[email protected]>

Andersson007 · 2021-02-07T16:01:05Z

@Jorge-Rodriguez thanks for your effort!
I'll look at the PR in detail ASAP (hope during the next several days, am in a business trip now).
Do you think there's no way to improve the coverage (see the marked places)? If no, no problem, it can be often hard to reach.

Jorge-Rodriguez · 2021-02-07T17:28:37Z

Do you think there's no way to improve the coverage (see the marked places)? If no, no problem, it can be often hard to reach.

One branch won't be reached until we introduce MariaDB integration tests, other issues have me puzzled because I'd swear they should be covered.

I'm thinking of tackling that major MySQL/MariaDB refactor next. It makes sense to me to handle that before introducing more bug fixes in the codebase, though I'd appreciate a thumbs up on that from @bmalynovytch as well.

Andersson007

@Jorge-Rodriguez thanks for the great work!
I found one typo in the fragment and there's the user_password_update_test.yml file - I don't see where it's included, could you please point out if i missed it?

Andersson007 · 2021-02-08T14:47:58Z

changelogs/fragments/49-mysql_user_login_tracking.yml

@@ -0,0 +1,2 @@
+minor_changes:
+- mysql_user - add the ``account_locking`` option sot support login attempt tracking and account locking feature (https://github.com/ansible-collections/community.mysql/issues/49).


Suggested change

- mysql_user - add the ``account_locking`` option sot support login attempt tracking and account locking feature (https://github.com/ansible-collections/community.mysql/issues/49).

- mysql_user - add the ``account_locking`` option to support login attempt tracking and account locking feature (https://github.com/ansible-collections/community.mysql/issues/49).

the coverage looks very good

bmalynovytch · 2021-02-08T15:59:48Z

I'm thinking of tackling that major MySQL/MariaDB refactor next. It makes sense to me to handle that before introducing more bug fixes in the codebase, though I'd appreciate a thumbs up on that from @bmalynovytch as well.

Thumbs up ! 👍 👍 👍

Andersson007 · 2021-02-10T12:36:31Z

@Jorge-Rodriguez should I wait for this PR merged or I can release community.mysql? I could wait a couple of days, no problem.

Jorge-Rodriguez · 2021-02-11T05:33:26Z

@Jorge-Rodriguez should I wait for this PR merged or I can release community.mysql? I could wait a couple of days, no problem.

@Andersson007 if you give me a day or two, I'll get those two issues you spotted ready and we can merge.

Andersson007 · 2021-02-11T05:51:05Z

@Jorge-Rodriguez ok, no problem, take your time! I'd release it on Sanday

Andersson007 · 2021-02-11T10:57:01Z

@Jorge-Rodriguez thank you for the great and regular contribution! You did and are doing really a lot!

As a gesture of trust, we're happy to give you commit priviledge in the community.mysql collection repository.
It's a great responsibility, so please, first, read the Committer Guidelines carefully.

I personally found the following rule essential: "Don't merge your PRs" and everything will be fine;)

If you have any questions, feel free to ask me directly via aaklychkov at mail dot ru (and I'd be happy to have your email to contact you if really needed).
Also, not to miss important announcements, would be great to subscribe to Changes impacting Collection Contributors and Maintainers issue, and Bullhorn newsletter (its Issue is here).

If you don't like to have this priviledge, please, let me know.

Thanks for the great contribution!:)

Jorge-Rodriguez · 2021-02-11T16:08:48Z

@Andersson007 It's a privilege and proud moment for me to receive the commit privilege on the repo.

I'll be sure to take the Guidelines to heart and use the privilege responsibly.

Many thanks.

Jorge-Rodriguez · 2021-02-11T16:20:53Z

@Andersson007 The password update tests show that I might have broken something in the user_mod function.
If I haven't managed to push a fix by Sunday, do not wait any longer. We'll merge this on the next release.

Cheers!

Andersson007 · 2021-02-11T16:36:27Z

@Andersson007 It's a privilege and proud moment for me to receive the commit privilege on the repo.

I'll be sure to take the Guidelines to heart and use the privilege responsibly.

Many thanks.

It's a pleasure to work with you, thanks!

If I haven't managed to push a fix by Sunday, do not wait any longer. We'll merge this on the next release.

Take your time, I'll release the collection on Sunday and then again once you complete the PR, thanks!

Andersson007 · 2021-02-14T06:41:34Z

@Jorge-Rodriguez I've just surprisingly found out that there's nothing new to release since 1.2.0:))
so take your time

Jorge-Rodriguez · 2021-02-25T11:13:04Z

I just found out that the reason the tests fail is unrelated to this feature and it has to do with the priv option.

As it turns out, if a user is granted ALL privileges like so:

- mysql_user:
       [...]
        priv: '*.*:ALL'
        state: present

The current privilege check is an execution of the SQL statement: SHOW GRANTS FOR <user>@<host> and the return value of such query when the privilege is ALL is not the keyword ALL but rather the list of all privileges, such as:

{'*.*': ['SELECT', 'INSERT', 'UPDATE', 'DELETE', 'CREATE', 'DROP', 'RELOAD', 'SHUTDOWN', 'PROCESS', 'FILE', 'REFERENCES', 'INDEX', 'ALTER', 'SHOW DATABASES',  
'SUPER', 'CREATE TEMPORARY TABLES', 'LOCK TABLES', 'EXECUTE', 'REPLICATION SLAVE', 'REPLICATION CLIENT', 'CREATE VIEW', 'SHOW VIEW', 'CREATE ROUTINE', 'ALTER  
ROUTINE', 'CREATE USER', 'EVENT', 'TRIGGER', 'CREATE TABLESPACE', 'CREATE ROLE', 'DROP ROLE', 'APPLICATION_PASSWORD_ADMIN', 'AUDIT_ADMIN', 'BACKUP_ADMIN',     
'BINLOG_ADMIN', 'BINLOG_ENCRYPTION_ADMIN', 'CLONE_ADMIN', 'CONNECTION_ADMIN', 'ENCRYPTION_KEY_ADMIN', 'GROUP_REPLICATION_ADMIN', 'INNODB_REDO_LOG_ARCHIVE',    
'INNODB_REDO_LOG_ENABLE', 'PERSIST_RO_VARIABLES_ADMIN', 'REPLICATION_APPLIER', 'REPLICATION_SLAVE_ADMIN', 'RESOURCE_GROUP_ADMIN', 'RESOURCE_GROUP_USER',       
'ROLE_ADMIN', 'SERVICE_CONNECTION_ADMIN', 'SESSION_VARIABLES_ADMIN', 'SET_USER_ID', 'SHOW_ROUTINE', 'SYSTEM_USER', 'SYSTEM_VARIABLES_ADMIN',                   
'TABLE_ENCRYPTION_ADMIN', 'XA_RECOVER_ADMIN']}

As such, the set of all privileges and the set containing the ALL keyword differ and as such it triggers a change in the module.

This is an idempotence problem. @bmalynovytch @bmildren @Andersson007, I think this is related to #89, we're not currently working on this bug and I'd appreciate your input in prioritizing that bugfix, this PR and issue #101 which are all somewhat related.

Andersson007 · 2021-02-26T07:22:25Z

@Jorge-Rodriguez if CI fails again and it's unrelated, feel free to comment the problem spot in the tests. Please also put the link to the idempotency issue there and something like TODO: uncomment when fixed or something (the aim is not to forget to uncomment).

When CI is green, I'd be happy to make a final review.

Andersson007 · 2021-02-26T07:24:29Z

TASK [test_mysql_user : create database using user2 and new password] **********
task path: /root/ansible_collections/community/mysql/tests/output/.tmp/integration/test_mysql_user-71m392nq-ÅÑŚÌβŁÈ/tests/integration/targets/test_mysql_user/tasks/test_user_password_update.yml:123
<testhost> ESTABLISH LOCAL CONNECTION FOR USER: root
<testhost> EXEC /bin/sh -c 'echo ~root && sleep 0'
<testhost> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /root/.ansible/tmp `"&& mkdir "` echo /root/.ansible/tmp/ansible-tmp-1614324082.3479342-24793-98910581994041 `" && echo ansible-tmp-1614324082.3479342-24793-98910581994041="` echo /root/.ansible/tmp/ansible-tmp-1614324082.3479342-24793-98910581994041 `" ) && sleep 0'
Using module file /root/ansible_collections/community/mysql/plugins/modules/mysql_db.py
<testhost> PUT /root/.ansible/tmp/ansible-local-200364ojg1vjm/tmp4o9kz8bc TO /root/.ansible/tmp/ansible-tmp-1614324082.3479342-24793-98910581994041/AnsiballZ_mysql_db.py
<testhost> EXEC /bin/sh -c 'chmod u+x /root/.ansible/tmp/ansible-tmp-1614324082.3479342-24793-98910581994041/ /root/.ansible/tmp/ansible-tmp-1614324082.3479342-24793-98910581994041/AnsiballZ_mysql_db.py && sleep 0'
<testhost> EXEC /bin/sh -c '/usr/bin/python3.6 /root/.ansible/tmp/ansible-tmp-1614324082.3479342-24793-98910581994041/AnsiballZ_mysql_db.py && sleep 0'
<testhost> EXEC /bin/sh -c 'rm -f -r /root/.ansible/tmp/ansible-tmp-1614324082.3479342-24793-98910581994041/ > /dev/null 2>&1 && sleep 0'
ok: [testhost] => {
    "changed": false,
    "db": "data",
    "db_list": [
        "data"
    ],
    "executed_commands": [],
    "invocation": {
        "module_args": {
            "ca_cert": null,
            "check_hostname": null,
            "check_implicit_admin": false,
            "client_cert": null,
            "client_key": null,
            "collation": "",
            "config_file": "/root/.my.cnf",
            "config_overrides_defaults": false,
            "connect_timeout": 30,
            "dump_extra_args": null,
            "encoding": "",
            "force": false,
            "hex_blob": false,
            "ignore_tables": [],
            "login_host": "127.0.0.1",
            "login_password": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER",
            "login_port": 3307,
            "login_unix_socket": null,
            "login_user": "db_user2",
            "master_data": 0,
            "name": [
                "data"
            ],
            "quick": true,
            "restrict_config_file": false,
            "single_transaction": false,
            "skip_lock_tables": false,
            "state": "present",
            "target": null,
            "unsafe_login_password": false,
            "use_shell": false
        }
    }
}

TASK [test_mysql_user : assert output message that database is created with new password] ***
task path: /root/ansible_collections/community/mysql/tests/output/.tmp/integration/test_mysql_user-71m392nq-ÅÑŚÌβŁÈ/tests/integration/targets/test_mysql_user/tasks/test_user_password_update.yml:133
fatal: [testhost]: FAILED! => {
    "assertion": "result is changed",
    "changed": false,
    "evaluated_to": false,
    "msg": "Assertion failed"
}

Andersson007 · 2021-02-26T07:35:27Z

The assertion for this task fails

    - name: create database using user2 and new password
      mysql_db:
        login_user: '{{ user_name_2 }}'
        login_password: '{{ user_password_1 }}'
        login_host: '{{ mysql_host }}'
        login_port: '{{ mysql_primary_port }}'
        name: '{{ db_name }}'
        state: present
      register: result

    - name: assert output message that database is created with new password
      assert:
        that:
          - result is changed

    - name: remove database
      mysql_db:

Maybe it already exists. Try to remove it before this invocation as well

Andersson007 · 2021-02-26T07:38:41Z

I just looked at one. Maybe there are other assertions fail including that one as you mentioned.
Feel free to comment the unrelated spot.

Jorge-Rodriguez · 2021-02-26T10:33:09Z

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

+        <<: *mysql_params
+        name: '{{ user_name_1 }}'
+        password: '{{ user_password_1 }}'
+        priv: '*.*:ALL'


@bmildren Do you know why we're granting ALL privileges here? It doesn't seem like we need that for these tests and it's causing the tests to fail because of #77

Don't wait the responce too much, it's quite inefficient at least for the last 6 months.
I think Ben probably doesn't know the answer anyway (if you look at git blame and see Ben there- he moved the stuff from community.general to this repo)

I think Ben probably doesn't know the answer anyway (if you look at git blame and see Ben there- he moved the stuff from community.general to this repo)

That already kind of gives me the answer I needed. My guess is this particular set of tests were moved here from community.general where they had already been forgotten. I re-added them on this PR as per your comment and they got hit by issue #77.

I do not think we need the privileges option for this particular battery of tests to verify what it's supposed to check so I'm going to go ahead and remove those lines. Then I'll see if there are any further failing tests and what can be done about them if so.

@Andersson007 I'll request your review again once the test dance is done.

@Jorge-Rodriguez sounds good, thanks!

bmalynovytch · 2021-03-01T10:19:55Z

While I completely agree the need of this PR, I think it should be done after we handled MySQL / MariaDB differences (#103) , as this only introduces changes related to MySQL and it will require heavy rewrite after splitting MySQL / MariaDB code base.

Andersson007 · 2023-01-20T08:19:56Z

@Jorge-Rodriguez as your Handle differences PR was merged, will you be able to finish this one?

Jorge-Rodriguez · 2023-01-20T09:01:57Z

@Andersson007 yeah, I'll take this.

Andersson007 reviewed Nov 17, 2020

View reviewed changes

plugins/modules/mysql_user.py Outdated Show resolved Hide resolved

Andersson007 reviewed Nov 23, 2020

View reviewed changes

changelogs/fragments/49-mysql_user_login_tracking.yml Outdated Show resolved Hide resolved

plugins/modules/mysql_user.py Show resolved Hide resolved

Jorge-Rodriguez and others added 17 commits February 7, 2021 10:08

Introduce account locking functionality

be0244e

Fix assertions

9ffdeb0

Fix parsing of versions with dashes

5646003

Make dictionary comprehension 2.6 compatible

46d7f41

Fix version added

da9d82e

Fix parsing of versions with dashes

0532ebd

Add account locking warnings on user add

2f26a66

Fix assertions for versions that do not support account locking

6d795ea

Fix version added

aaf7020

Co-authored-by: Andrew Klychkov <[email protected]>

Log account locking warnings only when locking is requested

9730eeb

Add changelog fragment

c2d5b97

Update changelog

800d9a5

Update module documentation

9a303ac

Check account_locking values

de75697

Remove reference to unused msg

58d21f6

Fix docstring

0502359

Fix key error

f41a9ce

Andersson007 reviewed Feb 8, 2021

View reviewed changes

Andersson007 mentioned this pull request Feb 9, 2021

Release plan #31

Closed

Jorge-Rodriguez added 2 commits February 11, 2021 09:18

Fix changelog typo

7bcf810

Include the password update tests in the play.

d3d68e4

Fix command quotes

f43676c

Andersson007 closed this Feb 26, 2021

Andersson007 reopened this Feb 26, 2021

Jorge-Rodriguez mentioned this pull request Feb 26, 2021

mysql_user: Granting all privileges at a global level always results in a change in MySQL 8.0 #77

Open

Jorge-Rodriguez commented Feb 26, 2021

View reviewed changes

Jorge-Rodriguez marked this pull request as draft March 27, 2021 08:16

Jorge-Rodriguez changed the title ~~login tracking~~ login tracking [Temporarily frozen] Mar 27, 2021

		@@ -0,0 +1,2 @@
		minor_changes:
		- mysql_user - add the ``account_locking`` option sot support login attempt tracking and account locking feature (https://github.com/ansible-collections/community.mysql/issues/49).

login tracking [Temporarily frozen] #62

Are you sure you want to change the base?

login tracking [Temporarily frozen] #62

Conversation

Jorge-Rodriguez commented Nov 17, 2020

SUMMARY

ISSUE TYPE

COMPONENT NAME

codecov bot commented Nov 17, 2020 • edited Loading

Codecov Report

Andersson007 left a comment

Choose a reason for hiding this comment

Andersson007 left a comment

Choose a reason for hiding this comment

Andersson007 commented Nov 23, 2020 • edited Loading

martinwalle commented Nov 30, 2020

Jorge-Rodriguez commented Nov 30, 2020

Andersson007 commented Dec 1, 2020

Jorge-Rodriguez commented Dec 1, 2020

Andersson007 commented Dec 4, 2020

martinwalle commented Dec 7, 2020 • edited Loading

Andersson007 commented Jan 15, 2021

Jorge-Rodriguez commented Jan 16, 2021

Jorge-Rodriguez commented Jan 31, 2021

Andersson007 commented Feb 7, 2021

Jorge-Rodriguez commented Feb 7, 2021 • edited Loading

Andersson007 left a comment

Choose a reason for hiding this comment

Andersson007 Feb 8, 2021

Choose a reason for hiding this comment

Andersson007 Feb 8, 2021

Choose a reason for hiding this comment

bmalynovytch commented Feb 8, 2021

Andersson007 commented Feb 10, 2021

Jorge-Rodriguez commented Feb 11, 2021

Andersson007 commented Feb 11, 2021

Andersson007 commented Feb 11, 2021

Jorge-Rodriguez commented Feb 11, 2021

Jorge-Rodriguez commented Feb 11, 2021

Andersson007 commented Feb 11, 2021

Andersson007 commented Feb 14, 2021

Jorge-Rodriguez commented Feb 25, 2021 • edited Loading

Andersson007 commented Feb 26, 2021

Andersson007 commented Feb 26, 2021

Andersson007 commented Feb 26, 2021 • edited Loading

Andersson007 commented Feb 26, 2021

Jorge-Rodriguez Feb 26, 2021

Choose a reason for hiding this comment

Andersson007 Feb 26, 2021 • edited Loading

Choose a reason for hiding this comment

Jorge-Rodriguez Feb 26, 2021

Choose a reason for hiding this comment

Andersson007 Mar 1, 2021

Choose a reason for hiding this comment

bmalynovytch commented Mar 1, 2021

Andersson007 commented Jan 20, 2023

Jorge-Rodriguez commented Jan 20, 2023

codecov bot commented Nov 17, 2020 •

edited

Loading

Andersson007 commented Nov 23, 2020 •

edited

Loading

martinwalle commented Dec 7, 2020 •

edited

Loading

Jorge-Rodriguez commented Feb 7, 2021 •

edited

Loading

Jorge-Rodriguez commented Feb 25, 2021 •

edited

Loading

Andersson007 commented Feb 26, 2021 •

edited

Loading

Andersson007 Feb 26, 2021 •

edited

Loading