Fixes #37772 - Add controller guardrails for multi-CV params on hosts and activation keys

[jeremylenz]

What are the changes introduced in this pull request?

Previously, the API hosts and activation keys controllers would allow invalid inputs for content_view_environments and content_view_environment_ids. This would result in failing silently and reassigning your host to have no content view environments. Bad.

Now we don't do that anymore. Also, I added quite a few tests.

Considerations taken when implementing this change?

Ended up doing some refactoring that made sense here, of the with_candlepin_names method. Turns out ContentViewEnvironment has a label field that we should have been using all along.

What are the testing steps for this pull request?

See https://community.theforeman.org/t/foreman-3-12-release-candidate-feedback/39258/5?u=jeremylenz

do stuff like that and make sure it errors correctly.

[jeremylenz]

ugh I really should do the same for activation keys too.. 🤔

[jeremylenz]

The generate_info method already enforces this same logic when updating the CVE. So this enabled candlepin_name to get a lot simpler.

[parthaa]

how about
alias_method :candlepin_name, :label

[jeremylenz]

rake aborted!
NameError: undefined method `label' for class `#<Class:0x0000563816dc4e80>'
/home/runner/work/katello/katello/katello/app/models/katello/content_view_environment.rb:59:in `alias_method'

[jeremylenz]

I tried alias_method :candlepin_name, :label and alias_method :label, :candlepin_name and neither worked. I changed it back.

[jeremylenz]

The ! here was mutating params[:content_view_environment_ids] and causing the error message to be incorrect with uncompacted arrays like No content view environments found with ids: [nil].

[parthaa]

Nice work!. Overall LGTM. Just a couple of suggestions.

[jeremylenz]

🍏

[parthaa]

Looks good suggested patch. Basically added a single_assignment? check

diff --git a/app/controllers/katello/api/v2/activation_keys_controller.rb b/app/controllers/katello/api/v2/activation_keys_controller.rb
index fd0ae762d1..0d15610b92 100644
--- a/app/controllers/katello/api/v2/activation_keys_controller.rb
+++ b/app/controllers/katello/api/v2/activation_keys_controller.rb
@@ -81,7 +81,7 @@ module Katello
     param :id, :number, :desc => N_("ID of the activation key"), :required => true
     def update
       if @content_view_environments.present? || update_cves?
-        if @content_view_environments.length == 1
+        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
@@ -332,9 +332,13 @@ module Katello
       end
     end
 
-    def update_cves?
+    def single_assignment?
       (params.key?(:environment_id) && params.key?(:content_view_id)) || # single
-        (params.key?(:environment) && params.key?(:content_view)) ||
+        (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

[parthaa]

Tested hammer updates/ Activation key page UI updates with the patch. Seems to work well :)

[parthaa]

APJ.