Fixes #13164 - adding view_params permission #3592

ares · 2016-06-15T11:58:04Z

No description provided.

theforeman-bot · 2016-06-15T11:58:08Z

There were the following issues with the commit message:

c321ca8 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

tbrisker · 2016-06-15T12:57:36Z

app/models/concerns/host_params.rb

@@ -37,7 +37,7 @@ def host_params
      return cached_host_params unless cached_host_params.blank?
      hp = host_inherited_params
      # and now read host parameters, override if required
-      host_parameters.each {|p| hp.update Hash[p.name => p.value] }
+      host_parameters.authorized(:view_params).each {|p| hp.update Hash[p.name => p.value] }


nitpick: why not host_parameters.authorized(:view_params).each {|p| hp[p.name] = p.value }?

it's unrelated to my changes but reads much better, will do

ares · 2016-06-15T14:10:55Z

builds on top of #3060 (commits from that PR squashed), I fixed few more things

migration uses fake models to avoid callbacks/validations problems in future
fixed tests that were using the fact that viewing params was unauthorized
re-allow parameter tab to be displayed for host/host groups since there are also smart class parameters to be edited
fixed authorizer to work with Parameter STI
added missing parameter check to methods loading inherited params

tbrisker · 2016-06-21T09:08:11Z

I've noticed several issues by adding a role that allows all the parameter actions filtered by name:

When creating a filter with Parameter permissions, the autocomplete does not work for the search field.
Giving permission to view_params does not allow seeing global parameters in the menu. There seems to be some duplication between the "Parameter" and "Common parameter" resource types. I would prefer if there was only one resource type with the option of specifying "parameter type" filter perhaps, as this is confusing.
In the host edit form, global parameters are displayed under the Inherited Parameters header even if they don't match the filter (even if we leave the common parameter resource, they are not filtered even if user has no option to see global params).
Adding a filter to the common params resource (for example by name) does not allow seeing anything under the global params index page, even if there are global params that match the filter.
A user can create a parameter that does not match the filter. For example, if the user is only allowed to CRUD params matching "name ~ a", he can create a parameter with name "ddd" that will be created (but that he cannot see). Moreover, if there is a validation error (for example, empty value), the user will not see the error..

ares · 2016-06-21T11:17:59Z

When creating a filter with Parameter permissions, the autocomplete does not work for the search field.

I'll look into this.

Giving permission to view_params does not allow seeing global parameters in the menu. There seems to be some duplication between the "Parameter" and "Common parameter" resource types. I would prefer if there was only one resource type with the option of specifying "parameter type" filter perhaps, as this is confusing.

Totally agree but that's out of the scope for this PR. We have 3 permission sets now, one for any lookup keys (smart variables + smart class parameters), one for Global parameters and one for the rest of "global" parameters e.g. DomainParameter.

In the host edit form, global parameters are displayed under the Inherited Parameters header even if they don't match the filter (even if we leave the common parameter resource, they are not filtered even if user has no option to see global params).

Can you reproduce this without the PR as well? It might be different issue since global params use different permission.

Adding a filter to the common params resource (for example by name) does not allow seeing anything under the global params index page, even if there are global params that match the filter.

I think the same as above apply.

A user can create a parameter that does not match the filter. For example, if the user is only allowed to CRUD params matching "name ~ a", he can create a parameter with name "ddd" that will be created (but that he cannot see). Moreover, if there is a validation error (for example, empty value), the user will not see the error..

This is known limitation of permission system. Since it's based on scope search which can only limit persisted data using SQL query we have no way to determine if the object that's being created fits conditions. There was a design for how to solve it but it was not implemented, I don't remember anyone asking it. Validation error might be something specific to parameters though.

ares · 2016-06-21T11:37:53Z

When creating a filter with Parameter permissions, the autocomplete does not work for the search field.

We ignore auto complete on purpose since there's no controller we could use for auto-completion in this case. 1) we don't know what exact type of parameter we'd search for, 2) there's a controller only for global parameters, other are handled by domain/os/hostgroup/host controllers

I'd recommend keeping it as it is and improve permissions first. @orrabin suggested adding params permissions per resource in original PR. Either this or maybe it would make sense to split permissions per parameter type e.g. Domain parameter, OsParameter etc. In such case we could add auto-completion.

tbrisker · 2016-06-21T12:07:19Z

Adding a filter to the common params resource (for example by name) does not allow seeing anything under the global params index page, even if there are global params that match the filter.

I think the same as above apply.

This works correctly on develop. The other one is indeed also broken on develop.

tbrisker · 2016-06-21T12:32:20Z

@ares I'm not very happy with having so many permissions for similar things but I agree this is out of scope for this PR.
Could you please open a refactor issue on redmine to handle merging the permission sets?
As for autocomplete, we could add a controller that only does autocomplete for Parameter, but I guess this could also be part of the aforementioned refactoring (if we have a parameter controller, we could have the global parameters page show all parameter types rather then just globals and allow filtering by type)

ares · 2016-06-22T13:34:08Z

@tbrisker I fixed the common parameter issue, good catch. I opened refactoring issue for permissions refactoring.

tbrisker · 2016-06-22T15:06:00Z

LGTM, I did some manual test with the new permission with various filters and it seems to fix the issue at hand and this seems to work well.
Just a couple of minor points left:

Add the view_params permission to the Site Manager role in the seed and to the existing role like we do for viewer.
Squash the two commits together and add a short explanation to the commit message.

After that, if there are no objections, I believe we are good to merge.

ares · 2016-06-23T08:37:00Z

I've updated seed and migration. I think in these rare cases where PR contains more commits we merge them to one during cherry-pick and just credit authors on merge. I can do it in here if you want, I hope @orrabin is OK with my changes being recorded under her name (which makes her responsible for bugs I left there :-)

ares · 2016-06-23T10:21:58Z

[test] failure is unrelated

A new view_params permission was added for parameters inheriting from Parameter object. The only exception is global parameters which are already handled by filter for CommonParameter resource. This new permissions is also automatically added to viewer and site manager roles. With the patch it's now possible to use granular filters for all parameters that Foreman supports. Contributions from: * Ori Rabin (orrabin) <orabin@redhat.com> * Marek Hulán (ares) <mhulan@redhat.com>

tbrisker · 2016-06-23T12:15:50Z

Merged as c7f55be, with minor modification to commit message. Thanks @orrabin and @ares!

tbrisker · 2016-06-23T13:17:05Z

@ares @orrabin this will need a release note in the 1.13 manual once it is branched.

theforeman-bot added Waiting on contributor Not yet reviewed Needs testing labels Jun 15, 2016

tbrisker reviewed Jun 15, 2016
View reviewed changes

ares force-pushed the fix/13164 branch from 71475c2 to 1e07fc1 Compare June 15, 2016 14:01

theforeman-bot added Needs testing and removed Waiting on contributor labels Jun 15, 2016

ares force-pushed the fix/13164 branch from 1e07fc1 to 5078407 Compare June 15, 2016 14:06

ares changed the title ~~[WIP] Fixes #13164 - adding view_params permission~~ Fixes #13164 - adding view_params permission Jun 15, 2016

orrabin mentioned this pull request Jun 20, 2016

Fixes #13164 - adding view_params permission #3060

Closed

tbrisker self-assigned this Jun 21, 2016

tbrisker added Waiting on contributor and removed Needs testing Not yet reviewed labels Jun 21, 2016

ares force-pushed the fix/13164 branch from 5078407 to 47fd4fc Compare June 22, 2016 13:27

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jun 22, 2016

tbrisker added Waiting on contributor and removed Needs re-review labels Jun 22, 2016

tbrisker removed the Needs testing label Jun 22, 2016

ares force-pushed the fix/13164 branch from 47fd4fc to e89a2b7 Compare June 23, 2016 08:35

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jun 23, 2016

ares force-pushed the fix/13164 branch from e89a2b7 to 6f0c45e Compare June 23, 2016 10:36

tbrisker added Ready to merge and removed Needs re-review Needs testing labels Jun 23, 2016

tbrisker closed this Jun 23, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #13164 - adding view_params permission #3592

Fixes #13164 - adding view_params permission #3592

ares commented Jun 15, 2016

theforeman-bot commented Jun 15, 2016

tbrisker Jun 15, 2016

ares Jun 15, 2016

ares commented Jun 15, 2016

tbrisker commented Jun 21, 2016

ares commented Jun 21, 2016

ares commented Jun 21, 2016

tbrisker commented Jun 21, 2016 •

edited

Loading

tbrisker commented Jun 21, 2016

ares commented Jun 22, 2016

tbrisker commented Jun 22, 2016

ares commented Jun 23, 2016 •

edited

Loading

ares commented Jun 23, 2016

tbrisker commented Jun 23, 2016

tbrisker commented Jun 23, 2016

Fixes #13164 - adding view_params permission #3592

Fixes #13164 - adding view_params permission #3592

Conversation

ares commented Jun 15, 2016

theforeman-bot commented Jun 15, 2016

tbrisker Jun 15, 2016

Choose a reason for hiding this comment

ares Jun 15, 2016

Choose a reason for hiding this comment

ares commented Jun 15, 2016

tbrisker commented Jun 21, 2016

ares commented Jun 21, 2016

ares commented Jun 21, 2016

tbrisker commented Jun 21, 2016 • edited Loading

tbrisker commented Jun 21, 2016

ares commented Jun 22, 2016

tbrisker commented Jun 22, 2016

ares commented Jun 23, 2016 • edited Loading

ares commented Jun 23, 2016

tbrisker commented Jun 23, 2016

tbrisker commented Jun 23, 2016

tbrisker commented Jun 21, 2016 •

edited

Loading

ares commented Jun 23, 2016 •

edited

Loading