From 31b5c98ba80fbe8ae8e13eb902529fe6dc81bf6a Mon Sep 17 00:00:00 2001
From: Jeremy Lenz <jlenz@redhat.com>
Date: Tue, 1 Oct 2024 09:14:05 -0400
Subject: [PATCH] Fixes #37772 - Add controller guardrails for multi-CV params
 (#11139)

also some light refactoring around candlepin names
  and content view environments
Refs #37772 - allow for clearing of CVEs on AK
Refs #37772 - don't clear CVEs on AngularJS update
---
 .../api/v2/activation_keys_controller.rb      |  45 +++++-
 .../api/v2/hosts_controller_extensions.rb     |   9 +-
 .../api/v2/multi_cv_params_handling.rb        |  24 +++
 .../update_cv_repository_cert_guard.rb        |   2 +-
 .../katello/content_view_environment.rb       |  15 +-
 config/initializers/inflections.rb            |   1 +
 .../api/v2/activation_keys_controller_test.rb | 140 ++++++++++++++++++
 .../api/v2/hosts_controller_test.rb           |  64 +++++++-
 test/models/content_view_environment_test.rb  |  23 ++-
 9 files changed, 296 insertions(+), 27 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 6bc022642de..cd5b0ddecb8 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,
@@ -65,7 +66,7 @@ def index
     param_group :activation_key
     def create
       @activation_key = ActivationKey.new(activation_key_params) do |activation_key|
-        activation_key.content_view_environments = @content_view_environments if @content_view_environments
+        activation_key.content_view_environments = @content_view_environments if update_cves?
         activation_key.organization = @organization
         activation_key.user = current_user
       end
@@ -79,8 +80,8 @@ def create
     param_group :activation_key
     param :id, :number, :desc => N_("ID of the activation key"), :required => true
     def update
-      if @content_view_environments.present?
-        if @content_view_environments.length == 1
+      if @content_view_environments.present? || update_cves?
+        if single_assignment? && @content_view_environments.length == 1
           @activation_key.assign_single_environment(
             content_view: @content_view_environments.first.content_view,
             lifecycle_environment: @content_view_environments.first.lifecycle_environment
@@ -294,7 +295,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
@@ -305,13 +306,43 @@ 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],
-              ids: params[:content_view_environment_ids],
-              organization: @organization || @activation_key&.organization)
+          labels: 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
+      handle_blank_cve_params
       @organization ||= @content_view_environments.first&.organization
     end
 
+    def handle_blank_cve_params
+      if params.key?(:environment) && params.key?(:content_view)
+        return # AngularJS sends nested environment and content_view params, but with blank _id values
+      end
+      # Activation keys do not require CVEs to be associated. So it's possible the user intends to clear them.
+      if params.key?(:environment_id) && params[:environment_id].blank? && params.key?(:content_view_id) && params[:content_view_id].blank?
+        @content_view_environments = []
+      elsif params.key?(:content_view_environments) && params[:content_view_environments].blank?
+        @content_view_environments = []
+      elsif params.key?(:content_view_environment_ids) && params[:content_view_environment_ids].blank?
+        @content_view_environments = []
+      end
+    end
+
+    def single_assignment?
+      (params.key?(:environment_id) && params.key?(:content_view_id)) ||
+      (params.key?(:environment) && params.key?(:content_view))
+    end
+
+    def update_cves?
+      single_assignment? ||
+        params.key?(:content_view_environments) || # multi
+        params.key?(:content_view_environment_ids)
+    end
+
     def find_host_collections
       ids = params[:activation_key][:host_collection_ids] if params[:activation_key]
       @host_collections = []
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..2268b1b18e9 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
@@ -49,8 +50,12 @@ def set_content_view_environments
             labels: 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..99e6c654e49 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)
@@ -79,10 +72,10 @@ def self.fetch_content_view_environments(labels: [], 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
+        cves.compact
       elsif labels.present?
         environment_names = labels.map(&:strip)
         environment_names.map! do |name|
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..4091c21da9d 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,96 @@ 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
+
+    def test_update_with_cleared_cves
+      cve = katello_content_view_environments(:library_dev_view_library)
+      cve.update(organization: @organization)
+      assert_sync_task(::Actions::Katello::ActivationKey::Update) do |activation_key, _activation_key_params|
+        assert_valid activation_key
+      end
+      assert_operator @activation_key.content_view_environments.size, :>, 0
+
+      put :update, params: {
+        :organization_id => @organization.id,
+        :id => @activation_key.id,
+        :content_view_environments => []
+      }
+
+      assert_response :success
+      assert_equal 0, @activation_key.content_view_environments.size
+    end
+
+    def test_update_with_cleared_cve_ids
+      cve = katello_content_view_environments(:library_dev_view_library)
+      cve.update(organization: @organization)
+      assert_sync_task(::Actions::Katello::ActivationKey::Update) do |activation_key, _activation_key_params|
+        assert_valid activation_key
+      end
+      assert_operator @activation_key.content_view_environments.size, :>, 0
+
+      put :update, params: {
+        :organization_id => @organization.id,
+        :id => @activation_key.id,
+        :content_view_environment_ids => []
+      }
+
+      assert_response :success
+      assert_equal 0, @activation_key.content_view_environments.size
+    end
+
+    def test_update_with_cleared_cv_lce_ids
+      cve = katello_content_view_environments(:library_dev_view_library)
+      cve.update(organization: @organization)
+      assert_sync_task(::Actions::Katello::ActivationKey::Update) do |activation_key, _activation_key_params|
+        assert_valid activation_key
+      end
+      assert_operator @activation_key.content_view_environments.size, :>, 0
+
+      put :update, params: {
+        :organization_id => @organization.id,
+        :id => @activation_key.id,
+        :content_view_id => nil,
+        :environment_id => nil
+      }
+
+      assert_response :success
+      assert_equal 0, @activation_key.content_view_environments.size
+    end
+
+    def test_update_from_angularjs_does_not_clear_cves
+      cve = katello_content_view_environments(:library_dev_view_library)
+      cve.update(organization: @organization)
+      assert_sync_task(::Actions::Katello::ActivationKey::Update) do |activation_key, _activation_key_params|
+        assert_valid activation_key
+      end
+      assert_operator @activation_key.content_view_environments.size, :>, 0
+
+      put :update, params: {
+        :organization_id => @organization.id,
+        :id => @activation_key.id,
+        :content_view_id => nil,
+        :environment_id => nil,
+        :content_view => {
+          :id => @view.id
+        },
+        :environment => {
+          :id => @library.id
+        }
+      }
+
+      assert_response :success
+      assert_operator @activation_key.content_view_environments.size, :>, 0
+    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..17503cb4bbc 100644
--- a/test/controllers/api/v2/hosts_controller_test.rb
+++ b/test/controllers/api/v2/hosts_controller_test.rb
@@ -78,20 +78,21 @@ def test_host_contents_environments_param
     Setting[:allow_multiple_content_views] = true
     ::Host::Managed.any_instance.stubs(:update_candlepin_associations)
     host = FactoryBot.create(:host, :with_content, :with_subscription, :with_operatingsystem,
-                              :content_view => @content_view, :lifecycle_environment => @environment)
+                              :content_view => @content_view, :lifecycle_environment => @environment, :organization => @environment.organization)
     Katello::Host::SubscriptionFacet.any_instance.expects(:backend_update_needed?).returns(false)
     orig_cves = host.content_facet.content_view_environment_ids.to_a
+    target_cves = [::Katello::ContentViewEnvironment.where(:content_view_id => @cv4.id,
+      :environment_id => @dev.id).first, ::Katello::ContentViewEnvironment.where(:content_view_id => @cv3.id,
+      :environment_id => @dev.id).first]
+    target_cves_ids = target_cves.map(&:id)
     put :update, params: {
       :id => host.id,
       :content_facet_attributes => {
-        :content_view_environments => ["#{@dev.label}/#{@cv4.label}", "#{@dev.label}/#{@cv3.label}"]
+        :content_view_environments => target_cves.map(&:label)
       }
     }, session: set_session_user
     assert_response :success
     host.content_facet.reload
-    target_cves_ids = [::Katello::ContentViewEnvironment.where(:content_view_id => @cv4.id,
-      :environment_id => @dev.id).first, ::Katello::ContentViewEnvironment.where(:content_view_id => @cv3.id,
-      :environment_id => @dev.id).first].map(&:id)
     assert_equal 2, host.content_facet.content_view_environment_ids.count
     refute_equal orig_cves, host.content_facet.content_view_environment_ids
     assert_equal_arrays target_cves_ids, host.content_facet.content_view_environments.ids
@@ -142,6 +143,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..663e8fcfcbd 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(labels: ['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