Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

jeremylenz
Copy link
Member

@jeremylenz jeremylenz commented Sep 12, 2024

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 jeremylenz force-pushed the 37772-host-update-guardrails branch 3 times, most recently from 755d9b2 to 5a34691 Compare September 12, 2024 20:28
@jeremylenz
Copy link
Member Author

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

@jeremylenz jeremylenz force-pushed the 37772-host-update-guardrails branch from 5a34691 to ffbebf3 Compare September 13, 2024 14:53
@@ -63,8 +57,7 @@ def default_environment?
end

def candlepin_name
return environment.label if default_environment?
"#{environment.label}/#{content_view.label}"
label
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about
alias_method :candlepin_name, :label

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# 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|
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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].

@jeremylenz jeremylenz changed the title Fixes #37772 - Add guardrails to HostsController Fixes #37772 - Add controller guardrails for multi-CV params on hosts and activation keys Sep 13, 2024
@jeremylenz jeremylenz force-pushed the 37772-host-update-guardrails branch from ffbebf3 to d4ca55d Compare September 13, 2024 16:45
@jeremylenz jeremylenz requested a review from parthaa September 13, 2024 16:49
@jeremylenz jeremylenz force-pushed the 37772-host-update-guardrails branch from 46ecff2 to 534e69d Compare September 17, 2024 17:47
@jeremylenz jeremylenz force-pushed the 37772-host-update-guardrails branch from 534e69d to 8f3df28 Compare September 25, 2024 20:42
@parthaa
Copy link
Contributor

parthaa commented Sep 26, 2024

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

@jeremylenz jeremylenz force-pushed the 37772-host-update-guardrails branch 2 times, most recently from 678abff to 4950a76 Compare September 26, 2024 21:10
@jeremylenz
Copy link
Member Author

🍏

@parthaa
Copy link
Contributor

parthaa commented Sep 30, 2024

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
Copy link
Contributor

parthaa commented Sep 30, 2024

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

  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
@jeremylenz jeremylenz force-pushed the 37772-host-update-guardrails branch from 4a7b979 to 6940cf5 Compare October 1, 2024 12:33
Copy link
Contributor

@parthaa parthaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APJ.

@jeremylenz jeremylenz merged commit 31b5c98 into Katello:master Oct 1, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants