Fixes #38731 - User taxonomy checks during permission checks #10701

adamruzicka · 2025-09-26T13:48:24Z

TL;DR

Additionally perform resource organization and location checks inside Authorizer, to prevent the RBAC system from allowing users to see resources outside of their organizations and locations.

Background

Foreman currently has two mechanisms, which can be used to control what users can do - the permission system and user's orgnization/location membership. These two mechanisms were originally expected to work together. In Foreman, models come with a set of modules facilitating the plumbing between the model, user's taxonomies (Taxonomix) and the RBAC system (Authorizable) and a default scope. The default scope does two things - it sets up the scope in a way that honors current organization/location selection (coming from Taxonomix) as well as limiting the view to only organizations and locations of the current user. This is the usually followed by a call to .authorized(permission) (relying on Authorizable being included in the model), which loops in the RBAC system and further tightens what the user is allowed to do. While this approach nicely separates concerns, it can give misleading results if the two parts are used separately.

This changes the meaning of what user's membership in an organization or location means. Previously, this relationship only marked the user as a resource to be managed within that organization/location, just as subnets or domains are. With this change we would shift it from "a resource to me managed within the scope" to "an actor which manages other things within the scope".

Changes done here

Resource organization and location checks were added to the RBAC system. This makes the RBAC system (Authorizable, Authorizer and #authorized(permission)) one-stop solution for user permission checking.

From the user's point of view, this change should make dealing with permissions in Foreman more consistent as well as making it more intuitive by making organization and location membership set up firmer boundaries than it did previously.

This could also simplify certain workflows. Currently delegating control over a specific organization to a user involves cloning the default "Organization admin" role, setting organization on it, assigning the role to a user and assigning that user to that organization. After this change, this could be reduced to giving the user the default "Organization admin" role without cloning it and just assigning the user to the organization.

This should also benefit Katello, which in general omits the default scope and which scopes resources to organizations differently than Foreman.

"Steps to reproduce"-styled example

Vanilla Foreman

Have orgnizations org1 and org2
Create a user u
Assign the user to org1
Create domain d1 in org1
Create domain d2 in org2
Create a custom role, do not set any taxonomies on the role, add view_domains permission to it, assign it to the user
In rails console:

User.current = User.find_by(login: 'u')
::Domain.unscoped.authorized(:view_domains).map(&:name)

Actual results:
Both d1 and d2 domains are found.

Expected results:
Only d1 domain is found.

Note: On Foreman resources, there is a default scope that performs the organization and location scoping, hence the unscoped, but it is not part of authorization checks.

Katello-flavor

Have orgnizations org1 and org2
Create a user u
Assign the user to org1
Create product p1 in org1
Create product p2 in org2
Create a custom role, do not set any taxonomies on the role, add view_products permission to it, assign it to the user
In rails console:

User.current = User.find_by(login: 'u')
::Katello::Product.authorized(:view_products).map(&:name)

Actual results:
Both p1 and p2 products are found.

Expected results:
Only p1 product is found.

ofedoren · 2025-09-26T14:14:25Z

Is this a replacement for #10652?

adamruzicka · 2025-09-26T14:25:32Z

Is this a replacement for #10652?

No. 10652 outlines "when is page access allowed" vs "what is shown inside the page". 10652 focuses on the first problem, this PR tightens the second one.

adamruzicka · 2025-09-29T14:59:13Z

Looking at the katello test failures, it seems katello never really relied on the user being in the same org as the resources they manage. Now the question is whether that was the intent or if it is that way just because matching user's orgs to resource's orgs wasn't necessary.

pondrejk · 2025-10-14T09:19:52Z

Checked on stream + packit using the described reproducers, also with hammer:

~]# hammer domain list
---|---------------------------------
ID | NAME                            
---|---------------------------------
3  | o1 domain                       
2  | o2 domain                       
...
---|---------------------------------
[root@ip-10-0-168-113 ~]# hammer -u domain-viewer -p .. domain list
---|----------
ID | NAME     
---|----------
2  | o2 domain
---|----------

pondrejk

ack from user pov

adamlazik1

Question: Does the pipeline here automatically run with the test versions from the PR?

adamlazik1 · 2025-10-20T11:20:23Z

test/unit/query_builder_test.rb

+    test 'joins multiple parts with AND conjunction' do
+      result = QueryBuilder.join('AND', ['name = value', 'status = active'])
+      assert_equal '((name = value) AND (status = active))', result
+    end
+
+    test 'joins multiple parts with OR conjunction' do
+      result = QueryBuilder.join('OR', ['name ~ test*', 'name ~ prod*'])
+      assert_equal '((name ~ test*) OR (name ~ prod*))', result
+    end


Nitpick: one of these tests is probably enough because the form of the conjunction does not impact anything. Or alternatively both asserts can go into single test case.

True, merged a couple and dropped some others.

adamlazik1 · 2025-10-20T11:23:49Z

test/unit/query_builder_test.rb

+    test 'joins three or more parts' do
+      result = QueryBuilder.join('OR', ['a = 1', 'b = 2', 'c = 3'])
+      assert_equal '((a = 1) OR (b = 2) OR (c = 3))', result
+    end


Nitpick no. 2: This assert could also go to the 'joins multiple parts' testcase

adamlazik1 · 2025-10-20T11:26:49Z

test/unit/query_builder_test.rb

+    test 'returns nil for empty array when on_empty is explicitly :nil' do
+      result = QueryBuilder.key_value_in('organization_id', [], :nil)
+      assert_nil result
+    end


Nitpick no. 3: probably redundant test.

adamlazik1 · 2025-10-20T11:55:41Z

app/services/query_builder.rb

+      when :nil
+        nil
+      when :block
+        parenthesize("#{key} = 1 AND #{key} != 1")


Probably doesn't matter, but couldn't we just return '(FALSE)'?

Sadly not, scoped search would treat that just as a string.

But yeah, this should be changed somehow. This might break in some cases

> ForemanTasks::Task.search_for("id = 1 and id != 1").count ActiveRecord::StatementInvalid: PG::InvalidTextRepresentation: ERROR: invalid input syntax for type uuid: "1" LINE 1: ...asks_tasks" WHERE ((("foreman_tasks_tasks"."id" = '1') AND (...

adamlazik1 · 2025-10-20T12:00:17Z

test/unit/query_builder_test.rb

+    test 'builds query with OR conditions' do
+      condition1 = 'name ~ prod*'
+      condition2 = 'name ~ test*'
+
+      result = QueryBuilder.join('OR', [condition1, condition2])
+      expected = '((name ~ prod*) OR (name ~ test*))'
+      assert_equal expected, result
+    end


This test appears to be a duplicate of test 'joins multiple parts with OR conjunction'

adamlazik1 · 2025-10-20T12:09:03Z

test/models/filter_test.rb

+    end
+
+    test 'also covers nested locations' do
+      sub_loc = FactoryBot.create(:organization, parent_id: @loc2.id)


Suggested change

sub_loc = FactoryBot.create(:organization, parent_id: @loc2.id)

sub_loc = FactoryBot.create(:location, parent_id: @loc2.id)

adamlazik1 · 2025-10-20T12:11:48Z

test/models/filter_test.rb

+  context '#build_search_query_from_parts' do
+    test 'joins single part with parentheses' do
+      FactoryBot.build_stubbed(:filter)
+      result = QueryBuilder.join('AND', ['name ~ test*'])
+      assert_equal '(name ~ test*)', result
+    end
+
+    test 'joins multiple parts with AND and parentheses' do
+      FactoryBot.build_stubbed(:filter)
+      parts = ['name ~ test*', 'organization_id = 1', 'location_id = 2']
+      result = QueryBuilder.join('AND', parts)
+      assert_equal '((name ~ test*) AND (organization_id = 1) AND (location_id = 2))', result
+    end
+
+    test 'returns empty string for empty array' do
+      FactoryBot.build_stubbed(:filter)
+      result = QueryBuilder.join('AND', [])
+      assert_nil result
+    end


These appear to be just QueryBuilder tests, unless I am mistaken.

adamlazik1 · 2025-10-20T12:52:06Z

test/integration/org_admin_js_test.rb


      switch_form_tab('Organizations')
-      ensure_selected_option_of_multiselect(@org1.name, select_id: 'domain_organization_ids')
+      # ensure_selected_option_of_multiselect(@org1.name, select_id: 'domain_organization_ids')


Suggested change

# ensure_selected_option_of_multiselect(@org1.name, select_id: 'domain_organization_ids')

Can this be deleted or is it kept here as a hint of sorts?

adamruzicka · 2025-10-20T14:44:39Z

Does the pipeline here automatically run with the test versions from the PR?

It does

adamlazik1

LGTM. The katello counterpart should be merged first so that the tests are green here.

adamruzicka · 2025-10-21T14:15:56Z

Oh come on katello tests, you were green just a moment ago on the other end

adamlazik1 · 2025-10-22T12:05:42Z

I would like to rerun the tests but it appears I don't have permissions to do that.

adamruzicka · 2025-10-24T15:28:15Z

Finally green

adamlazik1 · 2025-10-27T10:21:20Z

Thanks @adamruzicka! Feel free to merge as I am unable to do it myself.

adamruzicka · 2025-10-29T16:32:59Z

squashed to make the check happy

pr-processor bot added Not yet reviewed Waiting on contributor labels Sep 26, 2025

adamruzicka changed the title ~~User tax~~ Fixes #TBFL - User taxonomy checks during permission checks Sep 26, 2025

adamruzicka force-pushed the user-tax branch 4 times, most recently from 7c5e43b to 5918d48 Compare September 29, 2025 09:20

MariaAga mentioned this pull request Sep 29, 2025

Fixes #37987 - Filters remove override, unlimted, and taxonomies #10370

Merged

adamruzicka mentioned this pull request Sep 30, 2025

Adjust tests to authorizer changes in Foreman Katello/katello#11512

Merged

adamruzicka force-pushed the user-tax branch 3 times, most recently from 7803a87 to 93fad2c Compare October 9, 2025 13:00

pondrejk approved these changes Oct 14, 2025

View reviewed changes

pr-processor bot removed the Not yet reviewed label Oct 14, 2025

adamruzicka force-pushed the user-tax branch from 93fad2c to 446ae1c Compare October 16, 2025 16:39

pr-processor bot added the Needs re-review label Oct 16, 2025

adamruzicka force-pushed the user-tax branch 4 times, most recently from 8c0e2cc to bd0cf98 Compare October 17, 2025 14:58

pr-processor bot removed the Waiting on contributor label Oct 17, 2025

adamlazik1 reviewed Oct 20, 2025

View reviewed changes

pr-processor bot removed the Needs re-review label Oct 20, 2025

adamruzicka force-pushed the user-tax branch from bd0cf98 to a1f4279 Compare October 20, 2025 13:40

pr-processor bot added Waiting on contributor Needs re-review labels Oct 20, 2025

pr-processor bot removed the Needs re-review label Oct 20, 2025

adamlazik1 approved these changes Oct 21, 2025

View reviewed changes

pr-processor bot added the Needs re-review label Oct 24, 2025

adamlazik1 approved these changes Oct 24, 2025

View reviewed changes

pr-processor bot removed the Needs re-review label Oct 24, 2025

adamruzicka changed the title ~~Fixes #TBFL - User taxonomy checks during permission checks~~ Fixes #38731 - User taxonomy checks during permission checks Oct 24, 2025

adamruzicka force-pushed the user-tax branch from bc70e9b to 43ff3fa Compare October 24, 2025 13:35

pr-processor bot added the Needs re-review label Oct 24, 2025

adamruzicka force-pushed the user-tax branch from 43ff3fa to 42780de Compare October 24, 2025 14:26

Fixes #38731 - Include user's taxonomies in authorization checks

ebf138b

adamruzicka force-pushed the user-tax branch from 42780de to ebf138b Compare October 29, 2025 16:32

pr-processor bot removed the Waiting on contributor label Oct 29, 2025

stejskalleos approved these changes Oct 30, 2025

View reviewed changes

pr-processor bot removed the Needs re-review label Oct 30, 2025

stejskalleos merged commit 3a2b959 into theforeman:develop Oct 30, 2025
58 of 60 checks passed

adamruzicka deleted the user-tax branch October 30, 2025 08:26

aneta-petrova mentioned this pull request Nov 4, 2025

Update permission documentation for assigning orgs and locs theforeman/foreman-documentation#4456

Merged

10 tasks

This was referenced Nov 28, 2025

Fix CV test with permissions SatelliteQE/robottelo#20361

Merged

Fix 3 tests providing users with Location scopes SatelliteQE/robottelo#20371

Merged

	sub_loc = FactoryBot.create(:organization, parent_id: @loc2.id)
	sub_loc = FactoryBot.create(:location, parent_id: @loc2.id)

Fixes #38731 - User taxonomy checks during permission checks #10701

Fixes #38731 - User taxonomy checks during permission checks #10701

Uh oh!

Conversation

adamruzicka commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TL;DR

Background

Changes done here

"Steps to reproduce"-styled example

Vanilla Foreman

Katello-flavor

Uh oh!

ofedoren commented Sep 26, 2025

Uh oh!

adamruzicka commented Sep 26, 2025

Uh oh!

adamruzicka commented Sep 29, 2025

Uh oh!

pondrejk commented Oct 14, 2025

Uh oh!

pondrejk left a comment

Choose a reason for hiding this comment

Uh oh!

adamlazik1 left a comment

Choose a reason for hiding this comment

Uh oh!

adamlazik1 Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamruzicka commented Oct 20, 2025

Uh oh!

adamlazik1 left a comment

Choose a reason for hiding this comment

Uh oh!

adamruzicka commented Oct 21, 2025

Uh oh!

adamlazik1 commented Oct 22, 2025

Uh oh!

adamruzicka commented Oct 24, 2025

Uh oh!

adamlazik1 commented Oct 27, 2025

Uh oh!

adamruzicka commented Oct 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adamruzicka commented Sep 26, 2025 •

edited

Loading

adamlazik1 Oct 20, 2025 •

edited

Loading