-
Notifications
You must be signed in to change notification settings - Fork 987
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
Fixes #28093 - Wrong host ownership in UserMailNotifications #7422
Conversation
Wrong list of hosts in "Configuration Management Summary Report" when view_host permission is used with filter "owner = current_user"
Issues: #28093 |
Do it in User.as block so that the conext is restored after.
Place the Host.authorized search in a begin block so when it fails it does not brake sending emails for other users. Also, place it in an User.as block so it executes in the contex of the user being checked
Fix detected coding standard violations
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.
Sorry for the slow response here! just a small suggested improvement inline, otherwise looks good to me!
users.select { |user| Host.authorized_as(user, :view_hosts).find(host.id).present? } | ||
users = users.select do |user| | ||
User.as user do | ||
Host.authorized_as(user, :view_hosts).find(host.id).present? |
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.
Host.authorized_as(user, :view_hosts).find(host.id).present? | |
Host.authorized(:view_hosts).find(host.id).present? |
@@ -6,7 +6,9 @@ class HostMailer < ApplicationMailer | |||
# sends out a summary email of hosts and their metrics (e.g. how many changes failures etc). | |||
def summary(options = {}) | |||
raise ::Foreman::Exception.new(N_("Must specify a valid user with email enabled")) unless (user = User.find(options[:user])) | |||
hosts = Host::Managed.authorized_as(user, :view_hosts, Host) | |||
hosts = User.as user do | |||
Host::Managed.authorized_as(user, :view_hosts, Host) |
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.
Since you are in a user scope, you can use authorized
instead of authorized_as
here and below:
Host::Managed.authorized_as(user, :view_hosts, Host) | |
Host::Managed.authorized(:view_hosts) |
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.
Thanks @emildragu ! I'll merge this now and push another PR to improve on it.
for the record - #7543 |
Wrong list of hosts in "Configuration Management Summary Report" when view_host permission
is used with filter "owner = current_user"