Fixes #36273 - Use proper permission for editing Ansible variable

[pmoravec]

Requires:

• Fixes #36663 - Add permission to modify LookupValue resource foreman#9803

[nofaralfasi]

Hey @pmoravec, I agree with the change you made in this PR. However, the fix only applies to the API. We also need to address the problem in the web UI.
Furthermore, I noticed that there are no tests for this functionality. Could you please add them?

[adamruzicka]

We also need to address the problem in the web UI.

That's true, there are two other places where arguably wrong permission (edit_external_parameters) is used.

Could you please add them?

I'd be willing to let this slide. We can reason whether the permission used is the right one or not, but the whole authorization framework is thoroughly tested in Foreman itself.

[pmoravec]

I see. Sadly I dont have bandwidth to complete the fix as requested - please let somebody else adds a fix for WebUI "variant" and add some tests.

[ofedoren]

Agree on that it should be using :*_ansible_variables here, since as noted @stejskalleos it somehow defined to use lookup_values permissions as well:

foreman_ansible/lib/foreman_ansible/register.rb

Lines 119 to 124 in 5b44890

	permission :edit_ansible_variables,
	{ :lookup_values => [:update],
	:ansible_variables => [:edit, :update],
	:'api/v2/ansible_variables' => [:update],
	:'api/v2/ansible_override_values' => [:create, :destroy] },
	:resource_type => 'AnsibleVariable'

.

The only thing that bothers me, I didn't find any defined permissions on LookupValue model in Foreman itself :/

[nofaralfasi]

The only thing that bothers me, I didn't find any defined permissions on LookupValue model in Foreman itself :/

I see the LookupValue is defined in foreman core. Would it solve the issue if I add the permissions for the LookupValue resource there?

[ofedoren]

Would it solve the issue if I add the permissions for the LookupValue resource there?

It should 🤷

[nofaralfasi]

How to test:

1. Create a non-admin user, with 'Ansible Roles Manager' and View hosts permissions.
2. UI - Login as the new user, and try to create, edit and delete Ansible variables from the host details page.
3. API - Test the following requests using the new user credentials: POST /ansible/api/ansible_override_values, DELETE /ansible/api/ansible_override_values/:id.

Note:

• Even if the user doesn't have the Edit Host permission, he will still be able to modify Ansible variables from the host details page. This functionality existed prior to this PR's changes, and if we want to change that behaviour, it would require disabling the corresponding option within the Specify Matchers section on the ansible_variable edit page.

[nofaralfasi]

The tests are not passing due to the dependency on the merge of theforeman/foreman#9803.

[ofedoren]

Thanks, @nofaralfasi and @pmoravec, can be merged once theforeman/foreman#9803 is in.

[nofaralfasi]

In my opinion, it seems necessary to include the lookup_values permissions (view, create, edit and destroy) in the register.rb file.
Without it, we are not able to use them for the Ansible Roles Manager role.

[ofedoren]

Why is this needed? I mean, I've removed those lines and it still works...

[nofaralfasi]

Did you first remove these permissions from the DB?
From the rails console: Permission.where("name LIKE '%_lookup_values%'").destroy_all
And then re-run the rails server to test it.

[ofedoren]

I've done this and it looks... not quite good.

I've destroyed permissions, but we still need to create them, to use here. Thus, I needed to run db:seed to re-create them (we have defined them in core), but due to Ansible plugin needing these perms to create ARM role, it failed. Thus, after I removed Ansible plugin from loading, I've re-created the permissions, then enabled the plugin and run the server. I was able to change variable values even without this block.

I mean, we already create these perms in the core, why would we try to re-create them here? Seems weird. I've checked what we do with e.g. create_job_invocations, since this permission is used for ARM role as well: it seems REX plugin creates it and Ansible plugin then simply uses it to define a role. As you can see, Ansible plugin doesn't call permission :create_job_invocations, which is right.

Since I'm not so well familiar with the internals of permission logic, @adamruzicka, maybe you can help us out?

[nofaralfasi]

I forgot I need to make a change in the db/seeds.d/020-permissions_list.rb file for the db:seed command to include that file. That's why it didn't work as expected for me.
Therefore, as long as we run the db:seed command, there's no requirement to additionally include these permissions in the register.rb file, right?

[ofedoren]

🤷 that's what I hoped for...

[adamruzicka]

Yeah, we shouldn't redefine permissions. Let's get rid of it, it seemed to work for me when I did it locally

[ofedoren]

But the test failures now show what I was facing as well :/ Not sure why it complains about perms that are being created in DB by Foreman itself, isn't here some issues with load ordering?