Fixes #5929 - Taxonomy filter obey permissions

[ares]

With this patch you can assign permissions like assign_organizations and
assign_locations to particular user so that they can then assign taxonomies
only from set of taxonomies granted by their filters.

[ares]

Don't test yet, we may need similar change for other objects

[dmitri-d]

A test for this would be good.

[dmitri-d]

nm, found tests, my apologies.

[dmitri-d]

lgtm.

[domcleal]

Why skip if the user isn't associated to any?

[ares]

If user is not restricted to any taxonomy (let's say org in this case) it means he can assign any organization. Imagine the opposite, global user (without taxonomy restriction) would be allowed to assign limited set of organizations. In that case all such users should probably have to be assigned all organizations otherwise they wouldn't be able to work with objects which they can see and have permissions to modify. This would be huge change for existing installations. Also it seems consistent with the fact that object assigned to no taxonomy is considered global.

[domcleal]

Is the note about "Don't test yet, we may need similar change for other objects" still valid?

@isratrade please review and test this, thanks.

[ares]

Feel free to test, I updated the comment

[ares]

ping @isratrade or whoever can find some time to test

[isratrade]

@ares, I'm OK with merging this, but why did you call the method ensure_taxonomies_not_escalated? What do you mean by 'escalated'?

[ares]

It was meant to be consistent with admin flag and roles escalation. Escalation comes from privileges escalation, user could get permissions to work with objects in more taxonomies that was desired.

[ares]

@isratrade is there anything blocking this PR?

[ares]

or anyone else? maybe @witlessbird since you already saw the code

[ares]

ping @isratrade

[isratrade]

@ares, I tested this manually in the console for the following and all works as expected.

1. adding and removing locations and organizations from a domain
   domain.location_ids_changed?
   domain.location_ids_was
   domain.organization_ids_changed?
   domain.organization_ids_was
2. adding and removing locations and organizations from a user
   user.location_ids_changed?
   user.location_ids_was
   user.organization_ids_changed?
   user.organization_ids_was
3. adding and removing roles from a user
   user.role_ids_changed?
   user.role_ids_was

[isratrade]

@ares, I tested in the UI and see that the multiple select for locations is blank if a non-admin user does not have privilege assigned_locations. It's debatable from a UI point of view, whether to hide the locations tab, rather than having the multiple select options blank. However, when I save a domain with the locations tab blank, I get a failure on ensure_taxonomies_not_escalated

The log says

Failed to save: Location ids Invalid locations selection, you must select at least one of yours

[ares]

Thanks @isratrade, changing labels accordingly.

[isratrade]

@ares, I thought that the PUT for location_ids might be blank, but the log shows that it location_ids is populated

[ares]

so you need to set two permissions, view_locations let you see the view_location tab, assign_location gives you ability to change the assignment, but it seems you found an issue in corner case when no taxonomy is selected, I'll have to dig a bit deeper, it should probably unassign all locations in case you've described, right?

[ares]

ah I was wrong, this is expected, if you try to unassign all locations, you'd make the resource available in all locations, but since you're assigned to specific locations, you can't do that, you'd make it available somewhere where you don't have permissions
does it make sense @isratrade?

[ares]

rebased, setting label back to need testing

[isratrade]

@ares, it makes sense, but when testing locally, the current PR doesn't allow a non-admin user with no permission to assign_locations to update a domain. There shouldn't be a validation error, but it there is, it should appear in the UI and not only in the logs which is what I experienced.

[ares]

So it turned out that we don't display the validation, because multiple select can't display it. We could workaround by adding errors to base which is not ideal. I created a redmine issue for this http://projects.theforeman.org/issues/7319 since it's out of scope of this PR.

[isratrade]

@ares, so it appears the validation issue I ran into is an edge case where a user has permission to view_locations and thus sees the locations tab, but doesn't have permission to assign_locations so nothing is populated in the multiple select, right? I'll add a comment to issue #7319.

I'm OK with merging this now.

[ares]

@isratrade that's correct, unfortunately we'll encounter same behaviour when user has both permissions and tries to disassociate all locations which is not that "corner"

[isratrade]

@ares, I verified that it works if the non-admin user does not have view_locations and assign_locations permissions. I will merge this and hopefully issue #7319 can be fixed soon.

[isratrade]

Merged as 9523960. Thanks @ares !