Fixes #22325 - permit additional params #407

lzap · 2018-01-19T10:41:17Z

Discovery counterpart for theforeman/foreman#5183

theforeman-bot · 2018-01-19T10:41:23Z

lzap · 2018-01-19T10:42:06Z

app/controllers/concerns/foreman/controller/parameters/discovered_host.rb

@@ -6,6 +6,7 @@ module Foreman::Controller::Parameters::DiscoveredHost
    def discovered_host_params_filter
      Foreman::ParameterFilter.new(::Host::Discovered).tap do |filter|
        filter.permit :discovery_rule_id
+        filter.permit :quick_submit


This is wrong, I want to permit :quick_submit outside of host/discovered_host hash, how to do this in a plugin? @tbrisker any idea?

You don't need to permit it since you delete it before accessing the discovered_host_params_host hash

lzap · 2018-01-19T10:42:25Z

app/controllers/discovered_hosts_controller.rb

@@ -57,6 +57,7 @@ def edit
    @host = ::ForemanDiscovery::HostConverter.to_managed(@host, true, false, discovered_host_params_host) unless @host.nil?
    setup_host_class_variables
    @override_taxonomy = true
+    # need to permit this one but don't know how
    if params[:quick_submit]


This is the line which causes breakages.

you can add something like quick = params.delete(:quick_submit) before accessing discovered_host_params_host above, so the parameter will be removed before the filter is accessed, not sure why permitting isn't working correctly but this parameter doesn't need to be available to mass assignment anyways

tbrisker · 2018-01-22T16:34:54Z

test/functional/discovered_hosts_controller_test.rb

@@ -295,11 +295,11 @@ def test_no_dns_rebuild_if_dns_pending
    hostgroup = prepare_hostgroup_for_dns_rebuild(host)
    Nic::Managed.any_instance.expects(:rebuild_dns).never
    Host::Managed.any_instance.stubs(:skip_orchestration?).returns(false)
-    put :update, params: {:commit => "Update", :id => host.id,


You no longer need to remove commit param, it is passed by default from some forms and is now ignored in core.

lzap · 2018-01-25T15:28:19Z

Tests are green with develop and with @tbrisker patch as well.

tbrisker

one comment inline, also I don't think any of the changes to test/functional/discovered_hosts_controller_test.rb are needed anymore since they are all just removing the commit parameter or whitespace changes

tbrisker · 2018-01-25T15:59:36Z

app/controllers/concerns/foreman/controller/parameters/discovered_host.rb

@@ -6,6 +6,7 @@ module Foreman::Controller::Parameters::DiscoveredHost
    def discovered_host_params_filter
      Foreman::ParameterFilter.new(::Host::Discovered).tap do |filter|
        filter.permit :discovery_rule_id
+        filter.permit :quick_submit


You don't need to permit it since you delete it before accessing the discovered_host_params_host hash

lzap · 2018-01-30T15:04:15Z

Thanks. All green :-)

tbrisker

Code looks good, hopefully there aren't any other issues not covered by tests.

tbrisker · 2018-01-30T15:14:00Z

test/functional/foreman_discovery/concerns/hosts_controller_extensions_test.rb

-          discovered_host_params = {
-            'discovered_host' => { 'architecture_id' => arch_id }
-          }
+          params = { 'host' => { 'architecture_id' => arch_id } }


How does the page actually send the params? this should match that, so there might also be a change needed to the view?

Oh that was a leftover when I was playing around. It's discovered_host, reverted.

tbrisker

Tested locally with my patch enabled, this fails with:

ForemanDiscovery::Concerns::HostsControllerTest::hosts controller requests from discovered_hosts url#test_0001_get "host" params from "discovered_hosts" params [/home/tbrisker/gits/foreman_discovery/test/functional/foreman_discovery/concerns/hosts_controller_extensions_test.rb:18]:
Expected /"1073012829"/ to match "\n<!DOCTYPE html>\n<html lang=\"en\" class=\"login-pf no-js\">\n  <head>\n    <meta charset=\"utf-8\">\n    <meta http-equiv=\"X-UA-Compatible\" content=\"IE=edge,chrome=1\">\n    <title></title>\n    <meta name=\"description\" content=\"\">\n    <meta name=\"viewport\" content=\"user-scalable=no, width=device-width, initial-scale=1.0, maximum-scale=1.0\"/>\n    \n\n    <link rel=\"shortcut icon\" type=\"image/x-icon\" href=\"/assets/favicon-3c4b227479d0b22ade7a9ad5483b48b6f14ed265befe5e29ffcc4fbb63cbcc1a.ico\" />\n\n    \n    <link rel=\"stylesheet\" media=\"screen\" href=\"/assets/application-72a8737f925e3365fec197b85a1d6108709021e5158d95f30322378037ad0e9d.css\" data-turbolinks-track=\"true\" />\n    \n\n    \n    \n    \n    <script src=\"/assets/locale/en/app-50e5b54131d2201e1269360f1d901902339f758f889eb9c0a7fc6dda1d9dc8e1.js\" data-turbolinks-track=\"true\"></script>\n    <script src=\"/assets/application-b5f6270517a2b797c4f8b742d97ffa6fd0ac22226919fb7faf67ca709f29f246.js\" data-turbolinks-track=\"true\"></script>\n    \n    \n\n    <script type=\"text/javascript\">\n      var URL_PREFIX = '';\n\n        var AUTH_TOKEN = null\n\n\n      \n    </script>\n\n    \n  </head>\n\n  <body>\n    <!--[if lte IE 9]>\n      You are using an unsupported browser.\n    <![endif]-->\n      \n  <div id=\"main\">\n  <div id=\"content\" >\n    <div id=\"notifications\" data-flash=\"[]\"><script defer=\"defer\">\n//<![CDATA[\n$(tfm.reactMounter.mount('ToastNotifications', '#notifications', []));\n//]]>\n</script></div>\n    <div class=\"row\">\n      <div class=\"title_filter col-md-4\">\n        \n        &nbsp;\n      </div>\n      <div id=\"title_action\" class=\"col-md-8\">\n        <div class=\"btn-toolbar pull-right\"></div>\n      </div>\n    </div>\n\n    <div class='col-md-offset-4 col-md-4'>\n  <div class=\"alert alert-warning \"><span class=\"pficon pficon-warning-triangle-o \"></span> <strong>Invalid parameter: discovered_host </strong><span class=\"text\">Please verify that the parameter name is valid and the values are the correct type.</span><div class=\"alert-actions\"><hr><a class=\"btn btn-default\" data-id=\"aid_not_defined\" href=\"/\">Back</a></div></div>\n</div>\n\n  </div>\n</div>\n\n\n  </body>\n</html>\n\n".

lzap · 2018-02-01T09:54:23Z

Hohoho! A bug fixed. Tested on both branches now.

tbrisker · 2018-02-01T13:26:57Z

app/controllers/foreman_discovery/concerns/hosts_controller_extensions.rb

@@ -11,7 +11,7 @@ module HostsControllerExtensions
      # etc.. expect a params[:host] to work.
      def set_discovered_params
        return if params[:discovered_host].nil?
-        params[:host] ||= params[:discovered_host]
+        params[:host] ||= params.delete(:discovered_host)


I found a case where both host and discovered_host were present, but fixed it in core theforeman/foreman@d7d633d

tbrisker

LGTM, Thanks @lzap !

theforeman-bot added Needs testing Not yet reviewed labels Jan 19, 2018

lzap commented Jan 19, 2018

View reviewed changes

tbrisker reviewed Jan 22, 2018

View reviewed changes

lzap force-pushed the permit-22325 branch from 7c2e356 to 9986475 Compare January 25, 2018 15:28

tbrisker reviewed Jan 25, 2018

View reviewed changes

lzap force-pushed the permit-22325 branch from 9986475 to ff953ae Compare January 30, 2018 15:04

tbrisker approved these changes Jan 30, 2018

View reviewed changes

theforeman-bot removed the Not yet reviewed label Jan 30, 2018

tbrisker reviewed Jan 30, 2018

View reviewed changes

lzap force-pushed the permit-22325 branch from ff953ae to 0427af9 Compare January 30, 2018 15:49

tbrisker requested changes Jan 31, 2018

View reviewed changes

theforeman-bot added the Waiting on contributor label Jan 31, 2018

Fixes #22325 - permit additional params

9100aad

lzap force-pushed the permit-22325 branch from 0427af9 to 9100aad Compare February 1, 2018 09:51

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Feb 1, 2018

tbrisker reviewed Feb 1, 2018

View reviewed changes

tbrisker approved these changes Feb 1, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Feb 1, 2018

tbrisker merged commit cf57c78 into theforeman:develop Feb 1, 2018

lzap deleted the permit-22325 branch February 2, 2018 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #22325 - permit additional params #407

Fixes #22325 - permit additional params #407

lzap commented Jan 19, 2018

theforeman-bot commented Jan 19, 2018

lzap Jan 19, 2018

tbrisker Jan 25, 2018

lzap Jan 19, 2018

tbrisker Jan 22, 2018

tbrisker Jan 22, 2018

lzap commented Jan 25, 2018

tbrisker left a comment

tbrisker Jan 25, 2018

lzap commented Jan 30, 2018

tbrisker left a comment

tbrisker Jan 30, 2018

lzap Jan 30, 2018

tbrisker left a comment

lzap commented Feb 1, 2018

tbrisker Feb 1, 2018

tbrisker left a comment

Fixes #22325 - permit additional params #407

Fixes #22325 - permit additional params #407

Conversation

lzap commented Jan 19, 2018

theforeman-bot commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented Jan 25, 2018

tbrisker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented Jan 30, 2018

tbrisker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbrisker left a comment

Choose a reason for hiding this comment

lzap commented Feb 1, 2018

Choose a reason for hiding this comment

tbrisker left a comment

Choose a reason for hiding this comment