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

Fixes #4477 - Host[group] form only show authorized resources #3369

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@tbrisker
Copy link
Member

commented Mar 27, 2016

Previously, most dropdowns in the host and hostgroup edit forms
displayed all of the existing resources, including some that a user may
not have been authorized to view.
This commit makes sure only authorized resources are displayed, with the
exception of the current resource - so that editing a host will not
cause changes to its current associations in case the user is not
allowed to see them. This also includes a refactoring of the code to
reduce duplication.
I have also included a change to with_taxonomy_scope_override that
allows its use for relations. This was not previously possible due to
the .unscoped which was used to remove the default scope and has been
replaced with .unscope(:where => :taxonomy) that only removes any
previous taxonomy scopes.

Hostgroup.with_taxonomy_scope_override(@location,@organization).authorized(:view_hostgroups).reorder(:title)
end

def accessible_compute_resources

This comment has been minimized.

Copy link
@dLobatog

dLobatog Mar 30, 2016

Member

Looks like using a form object and storing these helpers there would be easier to understand. Not that I'm advocating for that to be done in this PR, just a random thought.

@dLobatog

This comment has been minimized.

Copy link
Member

commented Mar 30, 2016

LGTM in principle - I want to look a bit more in detail tomorrow though.

Any ideas on how to test this better? Perhaps moving more logic to the helpers and testing the helpers provide the proper filtered data (.authorized and with_taxonomy_scope...) is a way to ensure this doesn't break again.

@@ -121,7 +122,7 @@
:help_inline => _("Include this host within Foreman reporting")
%>
<div id='model_name'>
<%= select_f f, :model_id, Model.all, :id, :to_label, { :include_blank => true }, {:label => _("Hardware Model")} unless @host.compute_resource_id%>
<%= select_f f, :model_id, Model.authorized(:view_models), :id, :to_label, { :include_blank => true }, {:label => _("Hardware Model")} unless @host.compute_resource_id%>

This comment has been minimized.

Copy link
@dLobatog

dLobatog Mar 31, 2016

Member

accessible_models ? If it's in the helper, we can at least write a simple, cheap, unit test that expects .authorized on Model and "ensure" at least we notice if we break it.

@@ -3,7 +3,7 @@
:text => obj.errors[:puppetclasses].map { |e| "<li>#{e}</li>" }.join.html_safe %>
<% end %>

<%= render("config_groups/config_groups_selection", :obj => obj, :type => obj_type(obj)) if (controller_name != 'config_groups' && ConfigGroup.count > 0) %>
<%= render("config_groups/config_groups_selection", :obj => obj, :type => obj_type(obj)) if (controller_name != 'config_groups' && ConfigGroup.authorized(:view_config_groups).exists?) %>

This comment has been minimized.

Copy link
@dLobatog

dLobatog Mar 31, 2016

Member

accessible_config_groups?

@dLobatog

This comment has been minimized.

Copy link
Member

commented Mar 31, 2016

@tbrisker Nice job! I'm just wary it's a bit brittle and dependent on whether new contributions will respect this convention though. How does it sound to:

  • Write a dynamic accessible_xxxxx for a list of attributes, that just calls XXXXXX.with_taxonomy_scope_override(@location, @organization).authorized(:view_XXXXXX). Maybe for a few attributes you'd have to write the method definition manually instead of dynamically.
  • Use accessible_xxxxxx in host/hostgroup forms
  • Unit test accessible_xxxxxx methods (they'd be in a helper?) by expecting them to call .authorize.
  • (extra 🎱) - check that the form does not contain any model calls without .authorized

?

@tbrisker

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2016

  • Refactor to use generic functions
  • Write tests for generic functions
@dLobatog

This comment has been minimized.

Copy link
Member

commented Apr 28, 2016

I've set this to waiting on contributor because of the missing tests

@tbrisker tbrisker force-pushed the tbrisker:4477 branch from 1ce98b6 to 353b3cb May 11, 2016

@tbrisker tbrisker changed the title Fixes #4477 - Host[group] form only show resources user is allowed to see Fixes #4477 - Host[group] form only show authorized resources May 11, 2016

@tbrisker

This comment has been minimized.

Copy link
Member Author

commented May 11, 2016

@dLobatog tests added, let me know if you think anything more needs testing. Also updated commit message to be more informative.

# need to .unscoped or default_scope {with_taxonomy_scope} overrides inner_method
unscoped.with_taxonomy_scope(loc, org, inner_method)
# need to .unscope in case default_scope {with_taxonomy_scope} overrides inner_method
unscope(:where => :taxonomy).with_taxonomy_scope(loc, org, inner_method)

This comment has been minimized.

Copy link
@tbrisker

tbrisker May 11, 2016

Author Member

Rails 4 lets us remove just the one problematic scope and leave the rest in place.

Fixes #4477 - Host[group] form only show authorized resources
Previously, most dropdowns in the host and hostgroup edit forms
displayed all of the existing resources, including some that a user may
not have been authorized to view.
This commit makes sure only authorized resources are displayed, with the
exception of the current resource - so that editing a host will not
cause changes to its current associations in case the user is not
allowed to see them. This also includes a refactoring of the code to
reduce duplication.
I have also included a change to `with_taxonomy_scope_override` that
allows its use for relations. This was not previously possible due to
the `.unscoped` which was used to remove the default scope and has been
replaced with `.unscope(:where => :taxonomy)` that only removes any
previous taxonomy scopes.

@tbrisker tbrisker force-pushed the tbrisker:4477 branch from 353b3cb to 5937087 Jun 15, 2016

@dLobatog

This comment has been minimized.

Copy link
Member

commented Jun 27, 2016

@tbrisker Thanks for the update - I'll remove webpack/assets/stylesheets/application.css on merge :)

@dLobatog

This comment has been minimized.

Copy link
Member

commented Jun 27, 2016

Merged as a4d69f8, thanks @tbrisker !

@dLobatog dLobatog closed this Jun 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.