From ffbebf3e3cda938e5e2fee3fc0be349232f14141 Mon Sep 17 00:00:00 2001
From: Jeremy Lenz <jlenz@redhat.com>
Date: Thu, 12 Sep 2024 13:28:49 -0400
Subject: [PATCH] Fixes #37772 - Add controller guardrails for multi-CV params 
  also some light refactoring around candlepin names   and content view
 environments

---
 .../api/v2/activation_keys_controller.rb      |  9 ++-
 .../api/v2/hosts_controller_extensions.rb     | 11 +++-
 .../api/v2/multi_cv_params_handling.rb        | 24 ++++++++
 .../update_cv_repository_cert_guard.rb        |  2 +-
 .../katello/content_view_environment.rb       | 21 +++----
 config/initializers/inflections.rb            |  1 +
 .../api/v2/activation_keys_controller_test.rb | 60 +++++++++++++++++++
 .../api/v2/hosts_controller_test.rb           | 53 ++++++++++++++++
 test/models/content_view_environment_test.rb  | 23 ++++++-
 9 files changed, 183 insertions(+), 21 deletions(-)
 create mode 100644 app/controllers/katello/concerns/api/v2/multi_cv_params_handling.rb

diff --git a/app/controllers/katello/api/v2/activation_keys_controller.rb b/app/controllers/katello/api/v2/activation_keys_controller.rb
index 9d2430a965d..2d39d5149fd 100644
--- a/app/controllers/katello/api/v2/activation_keys_controller.rb
+++ b/app/controllers/katello/api/v2/activation_keys_controller.rb
@@ -2,6 +2,7 @@ module Katello
   class Api::V2::ActivationKeysController < Api::V2::ApiController  # rubocop:disable Metrics/ClassLength
     include Katello::Concerns::FilteredAutoCompleteSearch
     include Katello::Concerns::Api::V2::ContentOverridesController
+    include Katello::Concerns::Api::V2::MultiCVParamsHandling
     before_action :verify_presence_of_organization_or_environment, :only => [:index]
     before_action :find_optional_organization, :only => [:index, :create, :show]
     before_action :find_authorized_katello_resource, :only => [:show, :update, :destroy, :available_releases,
@@ -286,7 +287,7 @@ def find_cve_for_single
                                                              content_view_id: content_view_id).first
       if cve.blank?
         fail HttpErrors::NotFound, _("Couldn't find content view environment with content view ID '%{cv}'"\
-                                    " or environment ID '%{env}'") % { cv: content_view_id, env: environment_id }
+                                    " and environment ID '%{env}'") % { cv: content_view_id, env: environment_id }
       end
       @content_view_environments = [cve]
     end
@@ -297,9 +298,13 @@ def find_content_view_environments
         find_cve_for_single
       elsif params[:content_view_environments] || params[:content_view_environment_ids]
         @content_view_environments = ::Katello::ContentViewEnvironment.fetch_content_view_environments(
-              labels: params[:content_view_environments],
+              candlepin_names: params[:content_view_environments],
               ids: params[:content_view_environment_ids],
               organization: @organization || @activation_key&.organization)
+        if @content_view_environments.blank?
+          handle_errors(candlepin_names: params[:content_view_environments],
+                        ids: params[:content_view_environment_ids])
+        end
       end
       @organization ||= @content_view_environments.first&.organization
     end
diff --git a/app/controllers/katello/concerns/api/v2/hosts_controller_extensions.rb b/app/controllers/katello/concerns/api/v2/hosts_controller_extensions.rb
index 86fd203b61e..d59cfb8fee7 100644
--- a/app/controllers/katello/concerns/api/v2/hosts_controller_extensions.rb
+++ b/app/controllers/katello/concerns/api/v2/hosts_controller_extensions.rb
@@ -3,6 +3,7 @@ module Concerns
     module Api::V2::HostsControllerExtensions
       extend ActiveSupport::Concern
       include ForemanTasks::Triggers
+      include Katello::Concerns::Api::V2::MultiCVParamsHandling
 
       module Overrides
         def action_permission
@@ -46,11 +47,15 @@ def set_content_view_environments
           return if content_facet_attributes.blank? || @host&.content_facet.blank? ||
             (cve_params[:content_view_id].present? && cve_params[:lifecycle_environment_id].present?)
           cves = ::Katello::ContentViewEnvironment.fetch_content_view_environments(
-            labels: cve_params[:content_view_environments],
+            candlepin_names: cve_params[:content_view_environments],
             ids: cve_params[:content_view_environment_ids],
             organization: @organization || @host&.organization)
-
-          @host.content_facet.content_view_environments = cves if cves.present?
+          if cves.present?
+            @host.content_facet.content_view_environments = cves
+          else
+            handle_errors(candlepin_names: cve_params[:content_view_environments],
+              ids: cve_params[:content_view_environment_ids])
+          end
         end
 
         def cve_params
diff --git a/app/controllers/katello/concerns/api/v2/multi_cv_params_handling.rb b/app/controllers/katello/concerns/api/v2/multi_cv_params_handling.rb
new file mode 100644
index 00000000000..94a635bfa47
--- /dev/null
+++ b/app/controllers/katello/concerns/api/v2/multi_cv_params_handling.rb
@@ -0,0 +1,24 @@
+module Katello
+  module Concerns
+    module Api::V2::MultiCVParamsHandling
+      extend ActiveSupport::Concern
+      include ::Katello::Api::V2::ErrorHandling
+
+      def handle_errors(candlepin_names: [], ids: [])
+        if candlepin_names.present?
+          fail HttpErrors::UnprocessableEntity, "No content view environments found with names: #{candlepin_names.join(',')}"
+        elsif ids.present?
+          fail HttpErrors::UnprocessableEntity, "No content view environments found with ids: #{ids}"
+        end
+      rescue HttpErrors::UnprocessableEntity => error
+        respond_for_exception(
+          error,
+          :status => :unprocessable_entity,
+          :text => error.message,
+          :errors => [error.message],
+          :with_logging => true
+        )
+      end
+    end
+  end
+end
diff --git a/app/lib/actions/pulp3/repository/update_cv_repository_cert_guard.rb b/app/lib/actions/pulp3/repository/update_cv_repository_cert_guard.rb
index 3b258b5739e..a517517482c 100644
--- a/app/lib/actions/pulp3/repository/update_cv_repository_cert_guard.rb
+++ b/app/lib/actions/pulp3/repository/update_cv_repository_cert_guard.rb
@@ -1,7 +1,7 @@
 module Actions
   module Pulp3
     module Repository
-      class UpdateCvRepositoryCertGuard < Pulp3::Abstract
+      class UpdateCVRepositoryCertGuard < Pulp3::Abstract
         def plan(repository, smart_proxy)
           root = repository.root
           cv_repositories = root.repositories - [root.library_instance]
diff --git a/app/models/katello/content_view_environment.rb b/app/models/katello/content_view_environment.rb
index bf5823f0a6b..f4398bc19bc 100644
--- a/app/models/katello/content_view_environment.rb
+++ b/app/models/katello/content_view_environment.rb
@@ -36,13 +36,7 @@ def self.for_content_facets(content_facets)
     end
 
     def self.with_candlepin_name(cp_name, organization: Organization.current)
-      lce_name, cv_name = cp_name.split('/')
-      if cv_name.blank? && lce_name == 'Library'
-        return default.find_by(
-          environment: ::Katello::KTEnvironment.library.where(organization: organization)&.first
-        )
-      end
-      joins(:environment, :content_view).where("#{Katello::KTEnvironment.table_name}.label" => lce_name, "#{Katello::ContentView.table_name}.label" => cv_name).first
+      joins(:environment, :content_view).where("#{Katello::KTEnvironment.table_name}.organization_id" => organization, label: cp_name).first
     end
 
     # retrieve the owning environment for this content view environment.
@@ -63,8 +57,7 @@ def default_environment?
     end
 
     def candlepin_name
-      return environment.label if default_environment?
-      "#{environment.label}/#{content_view.label}"
+      label
     end
 
     def priority(content_object)
@@ -75,16 +68,16 @@ def priority(content_object)
       end
     end
 
-    def self.fetch_content_view_environments(labels: [], ids: [], organization:)
+    def self.fetch_content_view_environments(candlepin_names: [], ids: [], organization:)
       # Must do maps here to ensure CVEs remain in the same order.
       # Using ActiveRecord .where will return them in a different order.
       if ids.present?
-        ids.map! do |id|
+        cves = ids.map do |id|
           ::Katello::ContentViewEnvironment.find_by(id: id)
         end
-        ids.compact
-      elsif labels.present?
-        environment_names = labels.map(&:strip)
+        cves.compact
+      elsif candlepin_names.present?
+        environment_names = candlepin_names.map(&:strip)
         environment_names.map! do |name|
           with_candlepin_name(name, organization: organization)
         end
diff --git a/config/initializers/inflections.rb b/config/initializers/inflections.rb
index d61dc95dac3..28462b6db5f 100644
--- a/config/initializers/inflections.rb
+++ b/config/initializers/inflections.rb
@@ -19,4 +19,5 @@
   inflect.singular 'bases', 'base'
 
   inflect.acronym 'SCA' # Simple Content Access
+  inflect.acronym 'CV' # Content view
 end
diff --git a/test/controllers/api/v2/activation_keys_controller_test.rb b/test/controllers/api/v2/activation_keys_controller_test.rb
index 26040cec028..020409e63a0 100644
--- a/test/controllers/api/v2/activation_keys_controller_test.rb
+++ b/test/controllers/api/v2/activation_keys_controller_test.rb
@@ -207,6 +207,46 @@ def test_create_with_description
       assert_equal key_description, response['description']
     end
 
+    def test_create_with_content_view_environments_param
+      cve = katello_content_view_environments(:library_dev_view_library)
+      cve.update(organization: @organization)
+      content_view_environments = ['published_dev_view_library']
+      ActivationKey.any_instance.expects(:reload)
+      assert_sync_task(::Actions::Katello::ActivationKey::Create) do |activation_key|
+        assert_equal content_view_environments, activation_key.content_view_environments.map(&:candlepin_name), [cve.candlepin_name]
+        assert_valid activation_key
+      end
+
+      post :create, params: {
+        :organization_id => @organization.id,
+        :content_view_environments => content_view_environments,
+        :activation_key => {:name => 'new key'}
+      }
+
+      assert_response :success
+      assert_template 'katello/api/v2/common/create'
+    end
+
+    def test_create_with_content_view_environment_ids_param
+      cve = katello_content_view_environments(:library_dev_view_library)
+      cve.update(organization: @organization)
+      content_view_environment_ids = [cve.id]
+      ActivationKey.any_instance.expects(:reload)
+      assert_sync_task(::Actions::Katello::ActivationKey::Create) do |activation_key|
+        assert_equal content_view_environment_ids, activation_key.content_view_environments.map(&:id)
+        assert_valid activation_key
+      end
+
+      post :create, params: {
+        :organization_id => @organization.id,
+        :content_view_environment_ids => content_view_environment_ids,
+        :activation_key => {:name => 'new key'}
+      }
+
+      assert_response :success
+      assert_template 'katello/api/v2/common/create'
+    end
+
     test_attributes :pid => 'a9e756e1-886d-4f0d-b685-36ce4247517d'
     def test_should_not_create_with_no_hosts_limit
       post :create, params: {
@@ -217,6 +257,16 @@ def test_should_not_create_with_no_hosts_limit
       assert_match 'Validation failed: Max hosts cannot be nil', @response.body
     end
 
+    def test_should_not_create_with_invalid_content_view_environments_param
+      post :create, params: { :organization_id => @organization.id, :content_view_environments => ['foo'] }
+      assert_response :unprocessable_entity
+    end
+
+    def test_should_not_create_with_invalid_content_view_environment_ids_param
+      post :create, params: { :organization_id => @organization.id, :content_view_environment_ids => ['foo'] }
+      assert_response :unprocessable_entity
+    end
+
     test_attributes :pid => 'c018b177-2074-4f1a-a7e0-9f38d6c9a1a6'
     def test_should_not_create_with_invalid_hosts_limit
       post :create, params: {
@@ -300,6 +350,16 @@ def test_should_not_update_with_invalid_release
       assert_response :bad_request
     end
 
+    def test_should_not_update_with_invalid_content_view_environments_param
+      put :update, params: { :organization_id => @organization.id, :id => @activation_key.id, :content_view_environments => ['foo'] }
+      assert_response :unprocessable_entity
+    end
+
+    def test_should_not_update_with_invalid_content_view_environment_ids_param
+      put :update, params: { :organization_id => @organization.id, :id => @activation_key.id, :content_view_environment_ids => ['foo'] }
+      assert_response :unprocessable_entity
+    end
+
     test_attributes :pid => 'ec225dad-2d27-4b37-989d-1ba2c7f74ac4'
     def test_update_auto_attach
       new_auto_attach = !@activation_key.auto_attach
diff --git a/test/controllers/api/v2/hosts_controller_test.rb b/test/controllers/api/v2/hosts_controller_test.rb
index 406cd1f78ab..ec87d471ca0 100644
--- a/test/controllers/api/v2/hosts_controller_test.rb
+++ b/test/controllers/api/v2/hosts_controller_test.rb
@@ -142,6 +142,59 @@ def test_host_update_with_cv_only
         :content_view_id => @cv2.id
       }
     }, session: set_session_user
+    assert_response :unprocessable_entity
+  end
+
+  def test_set_content_view_environments_with_valid_content_view_environs_param
+    Katello::Host::SubscriptionFacet.any_instance.expects(:backend_update_needed?).returns(false)
+    ::Host::Managed.any_instance.expects(:update_candlepin_associations)
+    host = FactoryBot.create(:host, :with_content, :with_subscription,
+                              :content_view => @content_view, :lifecycle_environment => @environment)
+    ::Katello::ContentViewEnvironment.expects(:fetch_content_view_environments).returns([katello_content_view_environments(:library_default_view_environment)])
+    put :update, params: {
+      :id => host.id,
+      :content_facet_attributes => {
+        :content_view_environments => ["Library"]
+      }
+    }, session: set_session_user
+    assert_response :success
+  end
+
+  def test_set_content_view_environments_with_valid_ids_param
+    Katello::Host::SubscriptionFacet.any_instance.expects(:backend_update_needed?).returns(false)
+    ::Host::Managed.any_instance.expects(:update_candlepin_associations)
+    host = FactoryBot.create(:host, :with_content, :with_subscription,
+                              :content_view => @content_view, :lifecycle_environment => @environment)
+    put :update, params: {
+      :id => host.id,
+      :content_facet_attributes => {
+        :content_view_environment_ids => [@cv4.content_view_environments.first.id]
+      }
+    }, session: set_session_user
+    assert_response :success
+  end
+
+  def test_set_content_view_environments_with_invalid_ids_param
+    host = FactoryBot.create(:host, :with_content, :with_subscription,
+                              :content_view => @content_view, :lifecycle_environment => @environment)
+    put :update, params: {
+      :id => host.id,
+      :content_facet_attributes => {
+        :content_view_environment_ids => ["invalid string"]
+      }
+    }, session: set_session_user
+    assert_response :unprocessable_entity
+  end
+
+  def test_set_content_view_environments_with_invalid_content_view_environs_param
+    host = FactoryBot.create(:host, :with_content, :with_subscription,
+                              :content_view => @content_view, :lifecycle_environment => @environment)
+    put :update, params: {
+      :id => host.id,
+      :content_facet_attributes => {
+        :content_view_environments => ["invalid string"]
+      }
+    }, session: set_session_user
     assert_response 422
   end
 
diff --git a/test/models/content_view_environment_test.rb b/test/models/content_view_environment_test.rb
index 393eb1f3b32..e3e607e7847 100644
--- a/test/models/content_view_environment_test.rb
+++ b/test/models/content_view_environment_test.rb
@@ -34,7 +34,28 @@ def test_with_candlepin_name
       dev = katello_environments(:dev)
       view = katello_content_views(:library_dev_view)
       cve = Katello::ContentViewEnvironment.where(:environment_id => dev, :content_view_id => view).first
-      assert_equal cve, ContentViewEnvironment.with_candlepin_name('dev_label/published_dev_view')
+      assert_equal cve, ContentViewEnvironment.with_candlepin_name('published_dev_view_dev', organization: dev.organization)
+    end
+
+    def test_fetch_content_view_environments_candlepin_names
+      dev = katello_environments(:dev)
+      view = katello_content_views(:library_dev_view)
+      cve = Katello::ContentViewEnvironment.where(:environment_id => dev, :content_view_id => view).first
+      assert_equal [cve], ContentViewEnvironment.fetch_content_view_environments(candlepin_names: ['published_dev_view_dev'], organization: dev.organization)
+    end
+
+    def test_fetch_content_view_environments_ids
+      dev = katello_environments(:dev)
+      view = katello_content_views(:library_dev_view)
+      cve = Katello::ContentViewEnvironment.where(:environment_id => dev, :content_view_id => view).first
+      assert_equal [cve], ContentViewEnvironment.fetch_content_view_environments(ids: [cve.id], organization: dev.organization)
+    end
+
+    def test_fetch_content_view_environments_invalid_ids_does_not_mutate_array
+      dev = katello_environments(:dev)
+      input_ids = [0, 999]
+      assert_equal [], ContentViewEnvironment.fetch_content_view_environments(ids: input_ids, organization: dev.organization)
+      assert_equal [0, 999], input_ids # should not have a map! which mutates the input array
     end
   end
 end