-
-
Notifications
You must be signed in to change notification settings - Fork 123
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
Fix privileges handling #253
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small misunderstanding to correct because of how GRANT request works. Except that point, it's what i had in mind. Good job :-)
parsed_result = parse_mysql_batch_result(test_sql_results) | ||
Chef::Log.debug(parsed_result) | ||
parsed_result.each do |r| | ||
desired_privs.each do |p| | ||
key = p.to_s.capitalize.tr(' ', '_').gsub('Replication_', 'Repl_').gsub('Create_temporary_tables', 'Create_tmp_table').gsub('Show_databases', 'Show_db') | ||
key = "#{key}_priv" | ||
incorrect_privs = true if r[key] != 'Y' | ||
privs_to_grant << get_clean_grant_right_name(p) if r[key] != 'Y' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, it's not working with grant. You cannot add right one by one, like you can do with revoke (you can revoke only 1 right on 4). You must set all rights you want within one GRANT. Not that easy to understand... But it's like that.
So if one right is missing, you have to add all the rights you want in the GRANT request. That's why we used here a boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean that 2 subsequent queries, for ex. :
"GRANT SELECT"
followed by
"GRANT UPDATE"
won't work as intended ?
What will be the final state ? Only the last one will be enforced ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I not sure to properly get your point, this code only executes one GRANT request.
repair_sql << ' REQUIRE SSL' if new_resource.require_ssl | ||
repair_sql << ' REQUIRE X509' if new_resource.require_x509 | ||
repair_sql << ' WITH GRANT OPTION' if new_resource.grant_option | ||
grant_statement = "GRANT #{privs_to_grant.join(',')}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as before...
Generated by 🚫 Danger |
As there seems to be some confusion on this is there any chance @dud225 you could get an integration test for this one so we can confirm the current behaviour works and your desired behaviour works (and we do not break it in future) I see you did the spec tests as well but I think a functional test could be really useful here Also apologies for the delay coming back to you, |
Hi, I am closing this due to a lack of response, however please do re-open this PR with tests for the behaviour that is not working/ this pr resolves and we can look to get this merged in Thanks Sous Chefs |
Description
This patch fixes the handling of privileges by enabling the user to specify privileges containing spaces.
Issues Resolved
Fixes #243