Fixes #37772 - Add controller guardrails for multi-CV params (Katello…

…#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

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 = []

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

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

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]

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|

config/initializers/inflections.rb

@@ -19,4 +19,5 @@
   inflect.singular 'bases', 'base'
 
   inflect.acronym 'SCA' # Simple Content Access
+  inflect.acronym 'CV' # Content view
 end

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