search

rabbitmq / chef-cookbook

Development repository for Chef cookbook RabbitMQ
https://supermarket.chef.io/cookbooks/rabbitmq
Apache License 2.0
214 stars 423 forks source link

:clear_permissions action for user provider is not executed in some cases when it should be #529

Closed TheFLHurricane closed 5 years ago

TheFLHurricane commented 5 years ago

Version 5.8.1 of rabbitmq Cookbook

In the user provider, both :set_permissions & :clear_permissions use user_has_permissions? method to determine if the action should run. :set_permissions uses if user_has_permissions? which executes on false and takes no action on true. This works as anticipated.

:clear_permissions however uses unless if user_has_permissions? which executes only on true. However in user_has_permissions? true can only be achieved if passed permissions (perm_list) matches the output of permissions from rabbitmqctl. Since the :clear_permissions should only run on a user that has permissions, the output of permissions from rabbitmqctl will be a string, however perm_list is always nil as passed by :clear_permissions action. Therefor, :clear_permissions will never run.

Ultimately the check being performed is only half right. Also the name of the method user_has_permissions? implies it is only checking if the permissions exist (which is also only half right). The current check returns:

  1. false if permissions do not exist and should not exist (perm_list.nil? && cmd.stdout.empty?)
  2. true if permissions exist and are correct (perm_list == cmd.stdout.split.drop(1))
  3. false for all other scenarios. (Presumably a case where the permissions exist but do not match the expected).

The fix would be to change the method to explicitly check if the permissions are correct, not just exist. This actually works today by simply changing the first return (perm_list.nil? && cmd.stdout.empty?) from false to true (since this implies the user has no permissions and no permissions are expected, which would in fact be correct). Then the :clear_permissions action would need to call if method rather than unless method.

I have made a relatively simple fix for this that I have tested locally (file attached). This edit includes three changes.

  1. Change result of first return (perm_list.nil? && cmd.stdout.empty?) from false to true
  2. Change action :clear_permissions to call if method instead of unless method
  3. Change name of method from user_has_permissions? to user_has_correct_permissions? to accurately reflect result being returned user.txt
michaelklishin commented 5 years ago

Please submit a PR instead of attaching text files. Thank you.

michaelklishin commented 5 years ago

I've edited the description to say "5.8.1" as it is the latest released version at the moment.