Fixes #39088 - Do not tie policies to hosts by adamruzicka · Pull Request #605 · theforeman/foreman_openscap

adamruzicka · 2026-02-18T14:05:15Z

No description provided.

JSON.fast_generate is deprecated and will be removed in json 3.0.0, just use JSON.generate

adamruzicka · 2026-02-18T14:08:53Z

          end

-          @asset = ForemanOpenscap::Helper::get_asset(params[:cname], policy_id)
+          @host = ForemanOpenscap::Helper::find_host_by_name_or_uuid(params[:cname])


An alternative would be to leave the asset find_or_create, but not attach the policy to it if it is already provided by the host's hostgroup or its ancestors. Since this would be more expensive at runtime and would create orphaned records, I decided against it.

This also changes the behaviour in an edge case where the host has neither a hostgroup nor a policy assigned and a report comes in. Previously this would assign the policy to the host directly. In this case, I'd say Foreman is the source of truth about assigned policies and incoming reports should not change that.

adamruzicka · 2026-02-18T14:11:02Z

+                  .where(policy_id: policy_ids)
+
+        scope.each do |asset_policy|
+          # Composite primary keys are supported in rails >=7.1, since we're on 7.0, raw SQL will have to do


An alternative would be pulling in a composite_primary_keys gem, but eh

on arf report creation

Copilot

Pull request overview

This PR addresses #39088 by stopping ARF uploads from implicitly coupling (creating/maintaining) host-level OpenSCAP policy assignments when policies are provided via hostgroups, and adds a cleanup migration to remove existing host policy assignments that shadow hostgroup assignments.

Changes:

Update ARF upload flow to resolve a Host directly (instead of fetching/updating an Asset) and create reports against that host.
Add a migration to remove host-level AssetPolicy rows that duplicate policies already provided by hostgroups, and prune now-empty host assets.
Extend/adjust functional coverage around ARF upload behavior for hostgroup-provided policies.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
`app/controllers/api/v2/compliance/arf_reports_controller.rb`	Switch ARF upload pre-processing from asset lookup to direct host lookup and use `@host` through the create flow.
`app/models/foreman_openscap/arf_report.rb`	Change `create_arf` to accept a `host` directly; minor JSON generation change for `fixes`.
`test/functional/api/v2/compliance/arf_reports_controller_test.rb`	Update stubbing to match new host-resolution logic and add a regression test for hostgroup-provided policies.
`db/migrate/20260218133925_drop_host_policy_assignments_shadowing_hostgroup_policy_assignments.rb`	Data migration to delete host policy assignments that are redundant with hostgroup inheritance and prune empty host assets.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-18T15:21:52Z

+    host = ForemanOpenscap::Helper.stubs(:find_host_by_name_or_uuid).returns(@host)
+    host.stubs(:openscap_proxy).returns(nil)


In this test, host = ForemanOpenscap::Helper.stubs(:find_host_by_name_or_uuid)... assigns a Mocha expectation to host, so host.stubs(:openscap_proxy) won’t stub the Host instance and can raise (or silently do nothing). Stub @host.openscap_proxy instead (or drop the stub entirely since openscap_proxy_url is provided).

Suggested change

host = ForemanOpenscap::Helper.stubs(:find_host_by_name_or_uuid).returns(@host)

host.stubs(:openscap_proxy).returns(nil)

ForemanOpenscap::Helper.stubs(:find_host_by_name_or_uuid).returns(@host)

@host.stubs(:openscap_proxy).returns(nil)

Copilot · 2026-02-18T15:21:52Z

    assert_response :success
  end

+  test "should create host asset and tie to policy when policy is from hostgroup" do


Test name says it "should create host asset" but the assertions verify that a host-level asset is not created. Please rename the test to match the expected behavior (or adjust assertions if the intent is different).

Suggested change

test "should create host asset and tie to policy when policy is from hostgroup" do

test "should not create host asset but tie report to hostgroup policy when policy is from hostgroup" do

Yeah, that's what I get for creating a test for the original behaviour first and then tweaking it

Copilot · 2026-02-18T15:21:53Z

+      # Take ids of the hostgroup and all its children, all of these provide the policies extracted above
+      hostgroup_ids = hg_asset.hostgroup.subtree_ids


hg_asset.hostgroup can be nil if the polymorphic assetable record was deleted (there are migrations cleaning up orphaned asset_policies, but not orphaned assets). Calling subtree_ids on nil would break the migration. Consider skipping assets with missing hostgroups or joining against existing hostgroups to ensure hg_asset.hostgroup is present before calling subtree_ids.

Suggested change

# Take ids of the hostgroup and all its children, all of these provide the policies extracted above

hostgroup_ids = hg_asset.hostgroup.subtree_ids

# Skip assets with missing hostgroups to avoid calling subtree_ids on nil

hostgroup = hg_asset.hostgroup

next if hostgroup.nil?

# Take ids of the hostgroup and all its children, all of these provide the policies extracted above

hostgroup_ids = hostgroup.subtree_ids

Copilot · 2026-02-18T15:21:53Z

+
+      scope.each do |asset_policy|
+        # Composite primary keys are supported in rails >=7.1, since we're on 7.0, raw SQL will have to do
+        ActiveRecord::Base.connection.execute("DELETE FROM foreman_openscap_asset_policies WHERE asset_id = #{asset_policy.asset_id} AND policy_id = #{asset_policy.policy_id}")
+      end


This does a per-row connection.execute DELETE with string interpolation. Even if IDs come from the DB, it’s inefficient (N deletes) and bypasses quoting/sanitization. Prefer a set-based delete (e.g., where(asset_id: ..., policy_id: ...).delete_all or a single DELETE ... WHERE asset_id IN (...) AND policy_id IN (...)) and use bind params / quoting helpers if raw SQL is necessary.

Suggested change

scope.each do |asset_policy|

# Composite primary keys are supported in rails >=7.1, since we're on 7.0, raw SQL will have to do

ActiveRecord::Base.connection.execute("DELETE FROM foreman_openscap_asset_policies WHERE asset_id = #{asset_policy.asset_id} AND policy_id = #{asset_policy.policy_id}")

end

asset_policy_keys = scope.pluck(:asset_id, :policy_id)

next if asset_policy_keys.empty?

# Composite primary keys are supported in rails >=7.1, since we're on 7.0, raw SQL will have to do.

# Use a single, set-based DELETE with proper quoting for (asset_id, policy_id) tuples.

connection = ActiveRecord::Base.connection

tuples_sql = asset_policy_keys.map { |asset_id, policy_id|

"(#{connection.quote(asset_id)}, #{connection.quote(policy_id)})"

}.join(', ')

connection.execute("DELETE FROM foreman_openscap_asset_policies WHERE (asset_id, policy_id) IN (#{tuples_sql})")

Prefer a set-based delete (e.g., where(asset_id: ..., policy_id: ...).delete_all or a single DELETE ... WHERE asset_id IN (...) AND policy_id IN (...))

Except we want to delete specific asset_id-policy_id combinations, which this wouldn't really do.

The code suggestion (which does something different) would probably work if we added some sort of batching to prevent the query from growing too big to be processed.

Copilot · 2026-02-18T15:21:53Z

+    # Prune host assets which do not have any policies assigned to them
+    scope = ForemanOpenscap::Asset.where(assetable_type: 'Host::Base').left_outer_joins(:asset_policies)
+    scope = scope.where('foreman_openscap_asset_policies.asset_id IS NULL')
+    scope.destroy_all


destroy_all will instantiate each Asset and run callbacks, which can be slow for large datasets in a data cleanup migration. If callbacks aren’t required here, prefer delete_all (or batched deletes) for pruning host assets with no policies.

Suggested change

scope.destroy_all

scope.delete_all

adamlazik1 · 2026-02-23T13:21:13Z

  end

+  test "should not create host asset but tie report to hostgroup policy when policy is from hostgroup" do
+    reports_cleanup


Unrelated to PR, asking to broaden my understanding of ruby tests: Wouldn't reports be deleted automatically as a teardown action? My understanding is that objects in the testing db do not persist.

We do have that none, but that may not have always been the case. I dropped it from the new test I added here, we should revisit this and do a cleanup in a followup pr.

adamlazik1

Code LGTM, functionality verified on packit.

adamlazik1 · 2026-02-25T12:19:51Z

Thanks @adamruzicka !

Fix json warnings in tests

ff44e9d

JSON.fast_generate is deprecated and will be removed in json 3.0.0, just use JSON.generate

pr-processor Bot added Not yet reviewed Waiting on contributor labels Feb 18, 2026

adamruzicka commented Feb 18, 2026

View reviewed changes

pr-processor Bot removed the Not yet reviewed label Feb 18, 2026

adamruzicka commented Feb 18, 2026

View reviewed changes

Fixes #39088 - Do not tie policies to hosts

adb708c

on arf report creation

adamruzicka force-pushed the host-policy branch from e820b32 to adb708c Compare February 18, 2026 14:48

pr-processor Bot added the Needs re-review label Feb 18, 2026

ofedoren requested a review from Copilot February 18, 2026 15:02

Copilot started reviewing on behalf of ofedoren February 18, 2026 15:05 View session

Copilot AI reviewed Feb 18, 2026

View reviewed changes

pr-processor Bot added Needs re-review and removed Needs re-review labels Feb 18, 2026

adamlazik1 reviewed Feb 23, 2026

View reviewed changes

adamruzicka added 2 commits February 24, 2026 11:22

Fixes #39088 - Address comments

aed9560

Fixes #39088 - Batched deletes

fede920

adamruzicka force-pushed the host-policy branch from 7e45d96 to fede920 Compare February 24, 2026 10:22

pr-processor Bot added Needs re-review and removed Needs re-review labels Feb 24, 2026

adamlazik1 approved these changes Feb 25, 2026

View reviewed changes

adamlazik1 merged commit 9b4995a into theforeman:master Feb 25, 2026
11 of 12 checks passed

adamruzicka deleted the host-policy branch March 13, 2026 13:36

		host = ForemanOpenscap::Helper.stubs(:find_host_by_name_or_uuid).returns(@host)
		host.stubs(:openscap_proxy).returns(nil)

	test "should create host asset and tie to policy when policy is from hostgroup" do
	test "should not create host asset but tie report to hostgroup policy when policy is from hostgroup" do

		# Take ids of the hostgroup and all its children, all of these provide the policies extracted above
		hostgroup_ids = hg_asset.hostgroup.subtree_ids

-      # Take ids of the hostgroup and all its children, all of these provide the policies extracted above
-      hostgroup_ids = hg_asset.hostgroup.subtree_ids
+      # Skip assets with missing hostgroups to avoid calling subtree_ids on nil
+      hostgroup = hg_asset.hostgroup
+      next if hostgroup.nil?
+      # Take ids of the hostgroup and all its children, all of these provide the policies extracted above
+      hostgroup_ids = hostgroup.subtree_ids

-      scope.each do |asset_policy|
-        # Composite primary keys are supported in rails >=7.1, since we're on 7.0, raw SQL will have to do
-        ActiveRecord::Base.connection.execute("DELETE FROM foreman_openscap_asset_policies WHERE asset_id = #{asset_policy.asset_id} AND policy_id = #{asset_policy.policy_id}")
-      end
+      asset_policy_keys = scope.pluck(:asset_id, :policy_id)
+      next if asset_policy_keys.empty?
+      # Composite primary keys are supported in rails >=7.1, since we're on 7.0, raw SQL will have to do.
+      # Use a single, set-based DELETE with proper quoting for (asset_id, policy_id) tuples.
+      connection = ActiveRecord::Base.connection
+      tuples_sql = asset_policy_keys.map { |asset_id, policy_id|
+        "(#{connection.quote(asset_id)}, #{connection.quote(policy_id)})"
+      }.join(', ')
+      connection.execute("DELETE FROM foreman_openscap_asset_policies WHERE (asset_id, policy_id) IN (#{tuples_sql})")

Conversation

adamruzicka commented Feb 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

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!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

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!

Copilot AI Feb 18, 2026

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!

adamlazik1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamlazik1 commented Feb 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants