Fix privilege changing everytime

[rsicart]

SUMMARY

Compare privileges from before and after manipulation to determine if a change was made to user privileges.

For the moment, privileges_equal() function is very naive. We'll see if we need to do a deeper comparison or use an already existing tool for the job (f.e. deepdiff).

Fixes #77

See #77 for more details.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

mysql_user

[codecov]

Codecov Report

Merging #438 (1005108) into main (3670b2a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #438   +/-   ##
=======================================
  Coverage   77.82%   77.83%           
=======================================
  Files          27       27           
  Lines        2327     2328    +1     
  Branches      562      562           
=======================================
+ Hits         1811     1812    +1     
  Misses        356      356           
  Partials      160      160           

Impacted Files	Coverage Δ
plugins/module_utils/user.py	84.61% <100.00%> (+0.03%)	⬆️

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 for the fix!
Could you also please:

• Add a changelog fragment
• Fix the sanity issues

ERROR: Found 2 pep8 issue(s) which need to be resolved:
ERROR: tests/unit/plugins/module_utils/test_mysql_user.py:107:161: E501: line too long (1286 > 160 characters)
ERROR: tests/unit/plugins/module_utils/test_mysql_user.py:108:161: E501: line too long (1286 > 160 characters)

UPDATE: sorry, didn't notice that it's a draft

[rsicart]

@Andersson007 ready for review :)

all reviewers are welcome!

[rsicart]

BTW I'll add the changelog fragment this morning...

[Andersson007]

@rsicart great job, it's cool to see changes covered with integration and units!

[Andersson007]

cc @betanummeric @laurent-indermuehle could you also please take a look?

[laurent-indermuehle]

I'm very happy with this contribution which removes multiple "when: mysql... when: mariadb..." conditions. And a bug!

But the method privileges_equal(before_privs, after_privs) is only a comparison (==). And the only thing the unit test tests is the Python comparison operator. Which we should not test at our level.

What if future version of MySQL and MariaDB changes the way they handle privileges? We will have new bugs without knowing.

I would prefer to replace the call to privileges_equal with a simple == in the module, remove the unit test and the method. Then replace this with integration tests that validate that grants are added, revoked, entirely, partially and with all supported database engine.

[rsicart]

But the method privileges_equal(before_privs, after_privs) is only a comparison (==). And the only thing the unit test tests is the Python comparison operator. Which we should not test at our level.

Yes, before beginning this PR I thought I'd need a more complex function, comparing key by key, list by list, or something like that. Finally it seems that a comparison makes it.

What if future version of MySQL and MariaDB changes the way they handle privileges? We will have new bugs without knowing.

That's what happened with 'ALL PRIVILEGES' translation.

I would prefer to replace the call to privileges_equal with a simple == in the module, remove the unit test and the method.

I can do that if you prefer, it makes sense to me also.

Then replace this with integration tests that validate that grants are added, revoked, entirely, partially and with all supported database engine.

I think there's already some tests for privilege replace, append and subtract. Don't they cover already what you comment? Perhaps that should be a new issue to improve integration test suite.

[laurent-indermuehle]

Then replace this with integration tests that validate that grants are added, revoked, entirely, partially and with all supported database engine.

I think there's already some tests for privilege replace, append and subtract. Don't they cover already what you comment? Perhaps that should be a new issue to improve integration test suite.

My bad, I didn't checked the 21 (!!!) files inside tests/integration/targets/test_mysql_user/tasks.
Many cases are covered already and we can always add more tests later if issues arise.

lgtm! Thanks for the modifications!

[betanummeric]

I like the validation by comparing the existing privileges before and after the grant/revoke. 👍

For the record, MariaDB is also very chaotic when it comes to privileges (see https://mariadb.com/kb/en/grant/):

• aliased privileges
  • ALL results in ALL PRIGVILEGES
  • REPLICATION REPLICA results in REPLICATION SLAVE
• USAGE grants nothing and can be ignored (only sometimes necessary for syntactic reasons)

BINLOG MONITOR
New name for REPLICATION CLIENT from MariaDB 10.5.2, (REPLICATION CLIENT still supported as an alias for compatibility purposes). Permits running SHOW commands related to the binary log, in particular the SHOW BINLOG STATUS and SHOW BINARY LOGS statements. Unlike REPLICATION CLIENT prior to MariaDB 10.5, SHOW REPLICA STATUS isn't included in this privilege, and REPLICA MONITOR is required.

The SUPER privilege has been split into multiple smaller privileges from MariaDB 10.5.2 to allow for more fine-grained privileges, although it remains an alias for these smaller privileges.

But implementing special logic for this mess would only make sense to avoid an unwanted revocation, like we did in PR #434 for the ALL privilege. The module revokes privileges before granting others, so we need to be careful not to create a time where a wanted privilege is removed.

[rsicart]

Thank you @Andersson007 @laurent-indermuehle @betanummeric for reviewing! :)

[Andersson007]

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

[Andersson007]

1. Should we backport this? If yes, @rsicart feel free to put the backport-1 and backport-2 labels
2. I was fast and announced that it was in the latest release which is not true but anyway, we should release the collection (after backporting the fix if needed).

[rsicart]

1. Should we backport this?

I'm not sure @Andersson007, this PR fixes code introduced by #333 which was not backported.

[Andersson007]

1. Should we backport this?

I'm not sure @Andersson007, this PR fixes code introduced by #333 which was not backported.

@rsicart ah, ok, doesn't need then, thanks for the info

[laurent-indermuehle]

I tried to patch stable-2 with your changes and all integrations tests pass. But there is less tests in stable-2. I would not be confident to backport this.

[betanummeric]

stable-1 subtracts existing from requested privileges and grants those assuming a change, so in theory it is affected by the false change bug as well. But I would say it's too much effort to backport.

[Andersson007]

I tried to patch stable-2 with your changes and all integrations tests pass. But there is less tests in stable-2. I would not be confident to backport this.

@laurent-indermuehle if the related code was introduced in #333, we must not backport the fix as it relates to a new feature introduced in 3.. (we don't backport new features, so the related code isn't present in stable-2 and stable-1

[Andersson007]

i wouldn't touch the other branches if it's not impossible to backport it with just git cherry-pick -x <commitNum>

[Andersson007]

sorry, forgot to draft a release for 3.5.0 last time, just did it.
Does anyone want to release 3.5.1 ?

[rsicart]

I'll do it.