mysql_user, mysql_role: add argument subtract_privs to revoke privileges explicitly

[betanummeric]

SUMMARY

fixes #331

Add an argument subtract_privs (boolean, default false, mutual conflict with append_privs) to mysql_user and mysql_role. If true, the privileges specified by the priv argument get revoked and unspecified privileges remain unchanged.

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

mysql_user and mysql_role

[codecov]

Codecov Report

Merging #333 (517c67b) into main (641894e) will decrease coverage by 0.03%.
The diff coverage is 84.44%.

@@            Coverage Diff             @@
##             main     #333      +/-   ##
==========================================
- Coverage   78.24%   78.20%   -0.04%     
==========================================
  Files          27       27              
  Lines        2248     2267      +19     
  Branches      527      550      +23     
==========================================
+ Hits         1759     1773      +14     
- Misses        333      335       +2     
- Partials      156      159       +3     

Impacted Files	Coverage Δ
plugins/modules/mysql_user.py	81.44% <50.00%> (-1.54%)	⬇️
plugins/modules/mysql_role.py	87.37% <60.00%> (-0.53%)	⬇️
plugins/module_utils/user.py	87.07% <91.66%> (+0.09%)	⬆️
plugins/modules/mysql_replication.py	69.41% <0.00%> (+0.24%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 641894e...517c67b. Read the comment docs.

[betanummeric]

Hi @Andersson007, @rsicart, @Jorge-Rodriguez, @bmalynovytch,
Currently, when an invalid privilege is given along with subtract_privs: yes, it is just ignored because it is not found among the existing privileges. How should the module behave: ignore those or fail with an error message?

When granting a privilege, the invalid priv string error comes from trying out the statement and catching the mysql/mariadb error. This is not possible here, because it is never attempted to remove the invalid privilege. If an error is desired, a check needs to be added in the module.

The integration tests are currently red because they assume an error but the invalid privilege is ignored.

[rsicart]

Good job, thanks!

I think ignoring and documenting this behavior would be enough.

What does the rest of the team think about that?

[Andersson007]

Good job, thanks!

I think ignoring and documenting this behavior would be enough.

What does the rest of the team think about that?

Hi all, sorry for the later reply - was on PTO, +1 to ignoring and documenting

[Andersson007]

@betanummeric if you're OK with our opinion, feel free to proceed as i don't think we'll get more feedback. So please go on:)

[betanummeric]

@Andersson007 I'm ok with documenting and ignoring. As I thought this would be your opinion, I already implemented that, see commit ea48464 and the two after.

The integration tests only fail for mysql 8 at test_mysql_user/tasks/test_privs.yml:153 (see here) with "msg": "Error granting privileges, invalid priv string: ". The privilege string is empty, it looks like somewhere in the module the privileges were lost. I'm not sure that this is related to my changes.

Apart from that, I am done with my changes.

[Andersson007]

@betanummeric i've taken a look at the changes and have realized that i need to start my day with it:)

Before that:

I'm not sure that this is related to my changes.

I ran the tests locally without the changes and against MySQL 8 - it works and this test (that fails) had been there before this PR, so the failings are probably related to the changes.

[betanummeric]

The error described in my comment above is fixed with commit 29885d1. The issue was that the difference between requested and existing privileges only was the WITH GRANT OPTION. The module tried to do as little as possible, which resulted in invalid SQL syntax: GRANT ON *.* to user@host WITH GRANT OPTION (missing privileges).
I chose to fix it by adding all existing privileges to the empty list. It would also work with adding all requested privileges, any subset of them or just USAGE. That should make no difference here.

[betanummeric]

I have to correct myself: The placeholder privileges for only adding the grant option make a difference indeed, because privileges are first revoked, then granted. I changed the placeholder to USAGE and I hope the integration test succeed this time.

Finding this bug was tricky because the module is not very verbose about what it does. I added some more output in commit 37fa66a, because I first couldn't reproduce the bug. The added output will only show the added/revoked privileges of the last db/table, it is by far not comprehensive.

[Andersson007]

@betanummeric sorry for the delay, busy days.., i'll try to review it tomorrow morning, thanks for your work!

[Andersson007]

@betanummeric solid refactoring, thanks:)

Also thanks for adding the comments (they make things much clearer) and good test coverage

LGTM, one formatting thing from me

I believe one day we should split all of those ancient long core functions into several smaller ones doing one thing, etc. and cover them with unit tests in future.

Let's wait for @rsicart 's review

[Andersson007]

@betanummeric , completely forgot about version_added, could you please add it to the options so that users will see since what version it is available.

[rsicart]

Thanks for contributing @betanummeric !

And thanks for reviewing @Andersson007 , indeed that's a good thing to have a double review when a PR is related to critical parts of code, like this one :)

All changes seem coherent and logical to me, thanks for commenting the code, that helped a lot. Integration tests are also clear and easy to read.

I needed some time to understand why, when creating a user/role with privs and substract_privs, we set privs to None. Do you confirm that that's because:

• we can't substract privs from a non existing user/role
• otherwise that would create a user/role with privs

Good job!
LGTM 👍

[betanummeric]

Thanks for the feedback!

@rsicart
When a user/role needs to be created and subtract_privs: true is set, we need to set priv = None because otherwise there would follow a REVOKE USAGE ON *.* FROM the_just_created_user_or_role, even if '*.*': 'USAGE' is not in priv. privs would be granted to the user/role, which is clearly against the purpose of subtract_privs. I see no purpose in revoking privileges from a freshly created user/role, so editing privileges is skipped by setting priv = None.

[Andersson007]

@betanummeric thank you for the contribution!
@rsicart thank you for reviewing!

I think we can release the collection soon. As it's a time consuming process, would be great to have that blocker #341 merged as well. I'd be grateful for reviews.

UPDATE: there's another bugfix which is ready for review #322