Fixes #30394 - allow non-admins deal with untaxed filters #8422

ezr-ondrej · 2021-03-31T21:15:16Z

Prior this non-admin user would have to have assigned Role without
taxonomies (global role) to be able to manipulate filters.
This allows manipulating Filters to any User with Filter perms.

Filters with taxonomies mean they apply to taxonomy. But given they have
taxonomies relations, they are expected to be taxable in our permission
model. All taxable resources have to have the same taxonomies as Filter
have.

Some filters doesn't have taxonomies as their underlying resource
doesn't have taxonomies. That mean they were unable to be touched by
non-admins prior this patch.

This also renames the taxable checks to express beter what they mean.

theforeman-bot · 2021-03-31T21:15:31Z

Issues: #30394

ezr-ondrej · 2021-03-31T21:17:30Z

Alternative approach would be checking the role's taxonomies for Filter manipulation, but I believe this is what we want.

ezr-ondrej · 2021-04-07T08:58:01Z

app/models/filter.rb


-    if organizations.empty? && locations.empty?
+    if organizations.blank? && locations.blank?


this is wrong

app/services/authorizer.rb

app/models/filter.rb

ares

As discussed, please split the actual fix from the refactoring to two commits, otherwise makes sense. Left one nit re empty? vs blank?. Also the logging seems to be debuggig leftover :-)

ares · 2021-04-07T08:51:04Z

app/models/filter.rb


-    if organizations.empty? && locations.empty?
+    if organizations.blank? && locations.blank?


can this ever be nil? even '' is empty

I don't think so. build_taxonomy_search_string guarantees to return a string.

I've rolled this back, these are not org and loc, but organizations and locations.

ares · 2021-04-07T08:53:43Z

app/services/authorizer.rb

-      find_collection(subject.class, :permission => permission).
-        where(:id => subject.id).any?
+      col = find_collection(subject.class, :permission => permission)
+      Foreman::Logging.logger('permissions').debug col.collect(&:id)


perhaps use a block for the message so col.collect(&:id) would only be evaluated if the logger is allowed and debug level set (performance nit)

I've just removed that.

ezr-ondrej · 2021-04-07T10:07:02Z

I've split this into two commits to have the rename method separate.

ekohl · 2021-04-07T11:52:38Z

app/models/filter.rb

+    orgs = if resource_taxable_by_organization?
+             build_taxonomy_search_string('organization')
+           else
+             ''
+           end
+    locs = if resource_taxable_by_location?
+             build_taxonomy_search_string('location')
+           else
+             ''
+           end


Now that I see it here, should build_taxonomy_search_string instead return '' if it's not taxable?

looks much cleaner now :)

ezr-ondrej · 2021-04-15T19:10:17Z

I've fixed tests, so I believe it is ready :)

tbrisker

LGTM, looks like all concerns have been addressed. any other comments @ares @ekohl ?

ares

I was just about to test this. Code looks good now, thanks for splitting to two commits, please keep that when merging. I haven't tested, let me know if you need hand with that. Otherwise feel free to merge.

ekohl · 2021-06-01T15:20:44Z

No objections from my side.

tbrisker · 2021-06-02T07:22:59Z

@ares if you have the reproducer ready for testing that would be helpful :)

ares · 2021-06-03T15:06:41Z

This seems to cause problems when there is already a filter with Filter permissions that's scoped to the org/loc. From now on, it's invalid and I don't have a way to fix that from UI, because I no longer see the org/loc tabs for this filter. We may need a migration for this.

Otherwise works as expected.

ezr-ondrej · 2021-06-09T09:55:00Z

Thanks @ares! Great catch! If you could verify the migration fixes the issue for you, it would be great :)

db/migrate/20210609093404_drop_override_taxonomies_from_filter.rb

Prior this non-admin user would have to have assigned Role without taxonomies (global role) to be able to manipulate filters. This allows manipulating Filters to any User with Filter perms. Filters with taxonomies mean they apply to taxonomy. But given they have taxonomies relations, they are expected to be taxable in our permission model. All taxable resources have to have the same taxonomies as Filter have. Some filters doesn't have taxonomies as their underlying resource doesn't have taxonomies. That mean they were unable to be touched by non-admins prior this patch. This also drops current taxonomy relations in migration and force flip the `Override` flag to false for Filter resource filters.

This renames the taxable check methods on Filter to better express what they mean. We want to know it the resource is taxable, not if it has search on taxonomy.

tbrisker

LGTM, any other comments @ares ?

ares

Thanks, the migration passes. Merging!

tbrisker · 2021-06-23T09:52:03Z

Cherry pick in GH-8616

theforeman-bot added Needs testing Not yet reviewed labels Mar 31, 2021

ezr-ondrej force-pushed the filters_not_taxable branch from 5b80234 to 3b5afb0 Compare March 31, 2021 23:33

ezr-ondrej commented Apr 7, 2021

View reviewed changes

app/services/authorizer.rb Outdated Show resolved Hide resolved

ekohl reviewed Apr 7, 2021

View reviewed changes

app/models/filter.rb Outdated Show resolved Hide resolved

ekohl reviewed Apr 7, 2021

View reviewed changes

app/models/filter.rb Show resolved Hide resolved

ares requested changes Apr 7, 2021

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Apr 7, 2021

ezr-ondrej force-pushed the filters_not_taxable branch from 3b5afb0 to bcfb7a1 Compare April 7, 2021 09:58

theforeman-bot added Needs re-review and removed Waiting on contributor labels Apr 7, 2021

ezr-ondrej force-pushed the filters_not_taxable branch from bcfb7a1 to be6e3de Compare April 7, 2021 10:02

ezr-ondrej force-pushed the filters_not_taxable branch from be6e3de to 8b1c6da Compare April 7, 2021 10:54

ekohl reviewed Apr 7, 2021

View reviewed changes

ezr-ondrej force-pushed the filters_not_taxable branch 3 times, most recently from 60def6a to 5df1bcd Compare April 15, 2021 13:44

ezr-ondrej assigned tbrisker Apr 21, 2021

ezr-ondrej requested a review from ares May 24, 2021 11:22

tbrisker previously approved these changes Jun 1, 2021

View reviewed changes

theforeman-bot removed the Needs re-review label Jun 1, 2021

ares reviewed Jun 1, 2021

View reviewed changes

ezr-ondrej dismissed tbrisker’s stale review via fd6180d June 9, 2021 09:54

ezr-ondrej force-pushed the filters_not_taxable branch from 5df1bcd to fd6180d Compare June 9, 2021 09:54

ezr-ondrej requested a review from ares June 9, 2021 09:55

tbrisker reviewed Jun 9, 2021

View reviewed changes

db/migrate/20210609093404_drop_override_taxonomies_from_filter.rb Outdated Show resolved Hide resolved

theforeman-bot added the Waiting on contributor label Jun 9, 2021

ezr-ondrej force-pushed the filters_not_taxable branch from 61d00d0 to cbbf0bd Compare June 9, 2021 12:34

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jun 9, 2021

ezr-ondrej added 2 commits June 9, 2021 14:47

Refs #30394 - rename taxable checks

cab4d3a

This renames the taxable check methods on Filter to better express what they mean. We want to know it the resource is taxable, not if it has search on taxonomy.

ezr-ondrej force-pushed the filters_not_taxable branch from cbbf0bd to cab4d3a Compare June 9, 2021 12:47

tbrisker approved these changes Jun 10, 2021

View reviewed changes

theforeman-bot removed the Needs re-review label Jun 10, 2021

ares approved these changes Jun 18, 2021

View reviewed changes

ares merged commit 4476429 into theforeman:develop Jun 18, 2021

ezr-ondrej deleted the filters_not_taxable branch June 22, 2021 08:48

ezr-ondrej added the Needs cherrypick This should be cherrypicked to the stable branches or branches label Jun 23, 2021

tbrisker removed the Needs cherrypick This should be cherrypicked to the stable branches or branches label Jun 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #30394 - allow non-admins deal with untaxed filters #8422

Fixes #30394 - allow non-admins deal with untaxed filters #8422

ezr-ondrej commented Mar 31, 2021

theforeman-bot commented Mar 31, 2021

ezr-ondrej commented Mar 31, 2021

ezr-ondrej Apr 7, 2021

ares left a comment

ares Apr 7, 2021

ekohl Apr 7, 2021

ezr-ondrej Apr 7, 2021

ares Apr 7, 2021

ezr-ondrej Apr 7, 2021

ezr-ondrej commented Apr 7, 2021

ekohl Apr 7, 2021

ezr-ondrej Apr 11, 2021

ezr-ondrej commented Apr 15, 2021

tbrisker left a comment

ares left a comment

ekohl commented Jun 1, 2021

tbrisker commented Jun 2, 2021

ares commented Jun 3, 2021

ezr-ondrej commented Jun 9, 2021

tbrisker left a comment

ares left a comment

tbrisker commented Jun 23, 2021


		if organizations.empty? && locations.empty?
		if organizations.blank? && locations.blank?

Fixes #30394 - allow non-admins deal with untaxed filters #8422

Fixes #30394 - allow non-admins deal with untaxed filters #8422

Conversation

ezr-ondrej commented Mar 31, 2021

theforeman-bot commented Mar 31, 2021

ezr-ondrej commented Mar 31, 2021

Choose a reason for hiding this comment

ares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezr-ondrej commented Apr 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezr-ondrej commented Apr 15, 2021

tbrisker left a comment

Choose a reason for hiding this comment

ares left a comment

Choose a reason for hiding this comment

ekohl commented Jun 1, 2021

tbrisker commented Jun 2, 2021

ares commented Jun 3, 2021

ezr-ondrej commented Jun 9, 2021

tbrisker left a comment

Choose a reason for hiding this comment

ares left a comment

Choose a reason for hiding this comment

tbrisker commented Jun 23, 2021