Fixes #36919 - Add URL validation to HTTP Proxy URL field.

[chris1984]

Before PR:

After PR:

To Test:

• Apply PR
• Create an HTTP Proxy with a nonfqdn value
• Hit Test Connection

[lfu]

Could we have a clear error message, something like: Please enter a valid proxy URL.

[chris1984]

@lfu PR updated, here is the new screenshot

[lfu]

raise URI::InvalidURIError, _("Please enter a valid proxy URL. ") if !uri.is_a?(URI::HTTP) || uri.host.nil?

[chris1984]

Thanks for the catch @lfu, updated it

[ekohl]

If you raise an exception, you can't properly see it in the form. Normally we use valiations to display errors inline. In fact, we already have that:

foreman/app/models/http_proxy.rb

Line 21 in 7493355

validates :url, :format => { :with => /\Ahttps?:\/\// }, :presence => true

So why is this not working? Is the validation somehow skipped?

[ekohl]

To a bit more guiding: I suspect that you can change this bit:

foreman/app/controllers/http_proxies_controller.rb

Lines 26 to 33 in 7493355

	def test_connection
	http_proxy = HttpProxy.new(http_proxy_params)
	http_proxy.test_connection(params[:test_url])
	render :json => {:status => 'success', :message => _("HTTP Proxy connection successful.")}, :status => :ok
	rescue => e
	render :json => {:status => 'failure', :message => e.message}, :status => :unprocessable_entity
	end

To

  def test_connection
    http_proxy = HttpProxy.new(http_proxy_params)
    if http_proxy.invalid?
      # TODO: convert http_proxy.errors to some exception somehow
    end
    http_proxy.test_connection(params[:test_url])

    render :json => {:status => 'success', :message => _("HTTP Proxy connection successful.")}, :status => :ok
  rescue => e
    render :json => {:status => 'failure', :message => e.message}, :status => :unprocessable_entity
  end

See https://guides.rubyonrails.org/active_record_validations.html#valid-questionmark-and-invalid-questionmark & https://guides.rubyonrails.org/active_record_validations.html#validations-overview-errors as well

The main benefit is that you reuse the already existing validations. It may error out if the name is not specified though, which not be what you want.

[chris1984]

To a bit more guiding: I suspect that you can change this bit:

foreman/app/controllers/http_proxies_controller.rb

Lines 26 to 33 in 7493355

	def test_connection
	http_proxy = HttpProxy.new(http_proxy_params)
	http_proxy.test_connection(params[:test_url])
	render :json => {:status => 'success', :message => _("HTTP Proxy connection successful.")}, :status => :ok
	rescue => e
	render :json => {:status => 'failure', :message => e.message}, :status => :unprocessable_entity
	end

To

  def test_connection
    http_proxy = HttpProxy.new(http_proxy_params)
    if http_proxy.invalid?
      # TODO: convert http_proxy.errors to some exception somehow
    end
    http_proxy.test_connection(params[:test_url])

    render :json => {:status => 'success', :message => _("HTTP Proxy connection successful.")}, :status => :ok
  rescue => e
    render :json => {:status => 'failure', :message => e.message}, :status => :unprocessable_entity
  end

See https://guides.rubyonrails.org/active_record_validations.html#valid-questionmark-and-invalid-questionmark & https://guides.rubyonrails.org/active_record_validations.html#validations-overview-errors as well

The main benefit is that you reuse the already existing validations. It may error out if the name is not specified though, which not be what you want.

Ok will go with this approach

[chris1984]

@lfu updated, @ekohl how does this look?

[chris1984]

@ekohl when you get a chance can this one get a review, I have added tests.

[chris1984]

@ekohl can this one get back on your radar to review? It should be a quick one and I did tests as you asked.

[ianballou]

Working well for me! I see the right error when I pass in an invalid proxy URL. The proxy test still works for valid proxies as well, and fails for invalid proxies with valid URLs.

[ianballou]

@ekohl I'd like to merge this, but your previous change request is blocking it.