Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mysql_user: reinitialize the privs list in privileges_unpack() #137

Conversation

parseword
Copy link

SUMMARY

In some scenarios, privileges_unpack() called privs.append() inside a loop without first emptying or reinitializing the privs list from the prior iteration. This could result in an invalid GRANT statement, which incorrectly included privileges from a previously-built GRANT statement.

Reinitialize privs on each pass of the loop to prevent this from occurring.

Fixes #136

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

mysql_user

In some scenarios, `privileges_unpack()` called `privs.append()` inside a loop
without first emptying or reinitializing the `privs` list from the prior
iteration. This could result in an invalid `GRANT` statement, which incorrectly
included privileges from a previously-built `GRANT` statement.

Reinitialize `privs` on each pass of the loop to prevent this from occurring.
@codecov
Copy link

codecov bot commented Apr 4, 2021

Codecov Report

Merging #137 (929240e) into main (ba791bf) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #137   +/-   ##
=======================================
  Coverage   76.61%   76.61%           
=======================================
  Files          20       20           
  Lines        1770     1770           
  Branches      436      436           
=======================================
  Hits         1356     1356           
  Misses        268      268           
  Partials      146      146           
Impacted Files Coverage Δ
plugins/modules/mysql_user.py 81.15% <100.00%> (ø)

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 ba791bf...929240e. Read the comment docs.

@parseword parseword changed the title [WIP] mysql_user: reinitialize the privs list in privileges_unpack() mysql_user: reinitialize the privs list in privileges_unpack() Apr 4, 2021
@parseword
Copy link
Author

Ready for review

@Andersson007
Copy link
Collaborator

@parseword thanks for the PR!
We have CI tests for the module stored in tests/integration/targets/test_mysql_user/tasks/. Could you please cover the case there (just add the task(s) to somewhere which failed before the fix and now works)

@parseword
Copy link
Author

That's wading into uncharted territory for me, but I'll give it a shot tomorrow. I think I can put something together by cribbing from the test_priv_append.yml task.

@Andersson007
Copy link
Collaborator

ok, if any questions, feel free to ask

@Jorge-Rodriguez
Copy link
Contributor

@parseword if you feel more confident covering this bug fix with a unit test instead, we have those stored in tests/unit/plugins/modules

@Andersson007
Copy link
Collaborator

@parseword do we have any time estimates? I'm asking because We're gonna release 2.0.0 next week.
We could wait until you finish this.
If you need more time, no problem, no rush, we could release without this fix and include it in 2.0.1 shortly after. What do you think?

Copy link
Contributor

@Jorge-Rodriguez Jorge-Rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parseword Can you add tests for this?

@Jorge-Rodriguez
Copy link
Contributor

@parseword The code patched in this PR has been moved to /plugins/module_utils/user.py

@Jorge-Rodriguez
Copy link
Contributor

I reviewed this more closely and after we removed the static check for valid privileges this issue has disappeared as we never read the privs list in the privileges_unpack function. My comment on issue #136 still applies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mysql_user: privileges_unpack() bug when handling some column-level privileges
3 participants