Fix: grant revoked priv

[rsicart]

SUMMARY

Fix: do not revoke GRANT permission when it's already allowed and present in priv parameter.

Partially fixes an accidental behavior which can lock out users having GRANT privileges. If the user is root, you can get locked out of your database.

Does not fix privileges being updated every time.

I suspect that the bug was introduced in #333.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

mysql_user

ADDITIONAL INFORMATION

See #77 and those comments for more details and how to reproduce the problem:

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

$ git checkout HEAD^
$ ansible-test integration test_mysql_user --docker -v
TASK [test_mysql_user : grant all privileges with grant option] *****************************************
changed: [testhost] => {"changed": true, "msg": "Privileges updated: granted ['ALL'], revoked ['DROP', 'FILE', 'REPLICATION SLAVE', 'LOCK TABLES', 'INSERT', 'SELECT', 'SHOW VIEW', 'PROCESS', 'CREATE USER', 'REPLICATION CLIENT', 'SUPER', 'CREATE', 'SHUTDOWN', 'SHOW DATABASES', 'RELOAD', 'EVENT', 'DELETE', 'UPDATE']", "password_changed": false, "user": "db_user2"}

TASK [test_mysql_user : Assert that priv changed] *******************************************************
fatal: [testhost]: FAILED! => {
    "assertion": "\"granted ['ALL', 'GRANT']\" in result.msg",
    "changed": false,
    "evaluated_to": false,
    "msg": "Assertion failed"
}
...
PLAY RECAP **********************************************************************************************
testhost                   : ok=372  changed=150  unreachable=0    failed=1    skipped=21   rescued=0    ignored=7

$ git checkout rsi/fix-grant-revoked-priv
$ ansible-test integration test_mysql_user --docker -v
...
PLAY RECAP **********************************************************************************************
testhost                   : ok=672  changed=286  unreachable=0    failed=0    skipped=54   rescued=0    ignored=17

[codecov]

Codecov Report

Merging #434 (6cb11d9) into main (f1d63e3) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #434      +/-   ##
==========================================
- Coverage   77.85%   77.82%   -0.03%     
==========================================
  Files          27       27              
  Lines        2321     2327       +6     
  Branches      560      562       +2     
==========================================
+ Hits         1807     1811       +4     
- Misses        355      356       +1     
- Partials      159      160       +1     

Impacted Files	Coverage Δ
plugins/module_utils/user.py	84.58% <100.00%> (+0.09%)	⬆️
plugins/modules/mysql_replication.py	69.23% <0.00%> (-0.42%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Andersson007]

@rsicart thanks much for the fix! Looking forward to seeing the CI green to review

CC @betanummeric

[Andersson007]

CC @Jorge-Rodriguez

[rsicart]

Thanks for your reviews @laurent-indermuehle @betanummeric !

Indeed I forgot to put my PR in Draft mode, but your comments helped me a lot. Do not hesitate to continue to suggest or commit if you feel it!

[rsicart]

@Andersson007 @betanummeric @Jorge-Rodriguez @laurent-indermuehle

Pipeline is green but, before removing Draft mode, I'd like to know if that seems normal to you (in my opinion that's expected because of the translation of ALL PRIVILEGES to specific privileges done by mysql server >= 8):

https://github.com/ansible-collections/community.mysql/runs/8112530493?check_suite_focus=true#step:9:7469

TASK [test_mysql_user : Test idempotency (expect ok)] **************************
changed: [testhost] => {"changed": true, "msg": "Privileges updated: granted ['ALL'], revoked ['INNODB_REDO_LOG_ENABLE', 'DROP ROLE', 'REPLICATION CLIENT', 'SYSTEM_VARIABLES_ADMIN', 'AUDIT_ADMIN', 'ROLE_ADMIN', 'RELOAD', 'EVENT', 'APPLICATION_PASSWORD_ADMIN', 'BINLOG_ADMIN', 'INSERT', 'ALTER ROUTINE', 'TABLE_ENCRYPTION_ADMIN', 'CREATE ROLE', 'REPLICATION SLAVE', 'CONNECTION_ADMIN', 'FILE', 'ENCRYPTION_KEY_ADMIN', 'REFERENCES', 'SERVICE_CONNECTION_ADMIN', 'SYSTEM_USER', 'SESSION_VARIABLES_ADMIN', 'SUPER', 'CREATE VIEW', 'DROP', 'SHOW_ROUTINE', 'SHUTDOWN', 'PERSIST_RO_VARIABLES_ADMIN', 'CREATE TEMPORARY TABLES', 'RESOURCE_GROUP_USER', 'REPLICATION_APPLIER', 'CREATE', 'SELECT', 'SHOW VIEW', 'TRIGGER', 'BACKUP_ADMIN', 'INNODB_REDO_LOG_ARCHIVE', 'RESOURCE_GROUP_ADMIN', 'SHOW DATABASES', 'CREATE ROUTINE', 'PROCESS', 'DELETE', 'EXECUTE', 'REPLICATION_SLAVE_ADMIN', 'ALTER', 'UPDATE', 'CREATE USER', 'CREATE TABLESPACE', 'BINLOG_ENCRYPTION_ADMIN', 'CLONE_ADMIN', 'XA_RECOVER_ADMIN', 'LOCK TABLES', 'SET_USER_ID', 'GROUP_REPLICATION_ADMIN', 'INDEX']", "password_changed": false, "user": "db_user2"}

Perhaps we shouldn't revoke anything if 'ALL' in new_priv[db_table], as that will revoke and grant all after that. What do you think?

[betanummeric]

ALL PRIVILEGES is not quite everything:

• on MariaDB, GRANT OPTION is not included in ALL
• on MySQL, GRANT OPTION and PROXY is not included in ALL

We could add something like

if 'ALL' in grant_privs or 'ALL PRIVILEGES' in grant_privs:
  revoke_privs = list({'GRANT', 'PROXY'} & set(revoke_privs))

This should avoid pointless revocations.

[rsicart]

I added what @betanummeric proposed to avoid useless revocations.

It's ready for review, thanks in advance!

[laurent-indermuehle]

Great job! Good usage of Python sets :)

I would prefer multilines "when:" clause in order to be under 80 chars and be more legible. But maybe it's just me.

Also, I think double quote are unnecessary in assert.that. But again, not a big deal.

Apart from the two missing spaces I requested because they failed the sanity test, it looks good to me. I missed the failed unit tests. I don't know why it doesn't pass.

[Andersson007]

@rsicart please add a changelog fragment:)

[rsicart]

@rsicart please add a changelog fragment:)

Yes! I remebered that yesterday before sleep... :man-facepalming:

[Andersson007]

@rsicart thanks for the contribution!
@laurent-indermuehle @betanummeric thanks for reviewing!

[Andersson007]

The bug is critical, so does anyone want to release the collection?

[Andersson007]

I'll release it then, probably today