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

[rabbitmq] Remove guest user by default #1420

Merged
merged 1 commit into from
Nov 7, 2017
Merged

Conversation

PrajaktaPurohit
Copy link
Contributor

No description provided.

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Looks good -- I'd prefer one call to rmqctl less if possible, but don't worry about that if it's not simple 😉

execute "#{rmq_ctl} delete_user guest" do
environment (rabbitmq_env)
user opc_username
only_if "#{rmq_ctl} list_users| grep guest", :environment => rabbitmq_env
Copy link
Contributor

Choose a reason for hiding this comment

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

What would happen if we executed this unconditionally? Delete might be idempotent? Just wondering if we can save us a call to rabbitmqctl, since these are quite expensive 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we can save that call, changes made! thank you :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm have you pushed that? My expectation -- if we can save a call -- would be to have this block as

execute "#{rmq_ctl} delete_user guest" do
  environment (rabbitmq_env)
  user opc_username
end

So, no only_if at all. Does that make sense? 😉

Copy link
Contributor

@markan markan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

Looks good to me with or without the change @srenatus recommended.

@PrajaktaPurohit PrajaktaPurohit merged commit c44f90a into master Nov 7, 2017
@tas50 tas50 deleted the praj/SEC-32-2 branch July 1, 2021 04:31
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.

4 participants