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

Improving physical infrastructure provider hostname validation #16220

Closed
wants to merge 3 commits into from

Conversation

douglasgabriel
Copy link
Member

This PR is able to:

  • Ignore the slash at the end of the IP address to test if the hostname is unique. This because the addresses http://#.#.#.# and http://#.#.#.#/ have to be equals;
  • Add extra validation to test if the hostname format is valid. Where if is a IP address (ipv4 or ipv6) don't accept a path after it.
  • Print a message to user if the validation fails.

image

@douglasgabriel
Copy link
Member Author

@agrare

@agrare
Copy link
Member

agrare commented Oct 17, 2017

Hm we shouldn't accept http:// in the address either, it has to be just an IP addr or DNS name. This seems like it is fixing a small part of a bigger issue, maybe we should be using a regex validation on the UI input form? @Fryguy ?

@douglasgabriel
Copy link
Member Author

I agree with that, actually the protocol and path parts are ignored on the connection validation on the UI, so i've try to extend the behavior of the connection validation to the entity validation. The protocol part is just used to know if is an ip address or a DNS name. Look:

def connection_builder(conf, uri)
      $lxca_log.info "XClarityClient::XClarityBase connection_builder", "Building the url"
      #Building configuration
      hostname = URI.parse(conf.host)
      url = URI::HTTPS.build({ :host     => hostname.scheme ? hostname.host : hostname.path,
                               :port     => conf.port.to_i,
                               :path     => uri,
                               :query    => hostname.query,
                               :fragment => hostname.fragment }).to_s

      $lxca_log.info "XClarityClient::XClarityBase connection_builder", "Creating connection to #{url}"

      @conn = Faraday.new(url: url) do |faraday|
        faraday.request  :url_encoded             # form-encode POST params
        faraday.response :logger                  # log requests to STDOUT -- This line, should be uncommented if you wanna inspect the URL Request
        faraday.use :cookie_jar if conf.auth_type == 'token'
        faraday.adapter  Faraday.default_adapter  # make requests with Net::HTTP
        faraday.ssl[:verify] = conf.verify_ssl == 'PEER'
      end

      authentication(conf) if conf.auth_type == 'token'

      @conn.basic_auth(conf.username, conf.password) if conf.auth_type == 'basic_auth'
      $lxca_log.info "XClarityClient::XclarityBase connection_builder", "Connection created Successfuly"
      @conn
    end

@douglasgabriel
Copy link
Member Author

@AndreyMenezes

errors.add(:hostname, N_("has to be unique per provider type")) if existing_hostnames.include?(hostname.downcase.gsub(/\/$/, ''))
end

def hostname_format_valid?
Copy link
Member

Choose a reason for hiding this comment

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

Make this body more cleaner, remove some white lines.

@douglasgabriel douglasgabriel force-pushed the fix-XCS-404 branch 3 times, most recently from a44efe2 to 14fb252 Compare October 23, 2017 12:46
Copy link
Member

@AndreyMenezes AndreyMenezes left a comment

Choose a reason for hiding this comment

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

LGTM

@chessbyte chessbyte requested a review from Fryguy October 25, 2017 21:46
@agrare
Copy link
Member

agrare commented Nov 14, 2017

I think my question is why didn't putting anything but an IP/hostname fail provider validation anyway when it tried to connect? Is a URI a valid entry for the LXCA provider?

@douglasgabriel
Copy link
Member Author

@agrare if I understand correctly your question: the test fails only if put an IP address and adds a path after it, but if it is a hostname the validation doesn't fails if has some path after it

@agrare
Copy link
Member

agrare commented Nov 16, 2017

@douglasgabriel I'm wondering why it doesn't fail when we try to connect to the provider?

@AndreyMenezes
Copy link
Member

@douglasgabriel This type of validation should be checked with the validate button. We have a button to validade, the save button just should save.


serialize :options

def hostname_uniqueness_valid?
return unless hostname_required?
return unless hostname.present? # Presence is checked elsewhere
# check uniqueness per provider type
existing_hostnames = (self.class.all - [self]).map(&:hostname).compact.map { |hostname| hostname.downcase.gsub(/\/$/, '') }
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be done with an ActiveRecord built-in.

uri = URI.parse(URI.encode(hostname.gsub(/\/$/, ''))) # disregarding the slash at the end of the address
return unless uri.scheme # If has no protocol before hostname, any format is valid
error_msg = "is in a wrong format. Please, remove the path after IP address."
errors.add(:hostname, N_(error_msg)) unless uri.path.to_s.empty? # If the IP address has path after it, the format is invalid
Copy link
Member

Choose a reason for hiding this comment

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

@mzazrivec Will this work with the automated string finder for i18n?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope.

This would work:

error_msg = N_("is in a wrong format ...")
errors.add(:hostname, _(error_msg))

Or even simpler:

error_msg = _("is in a wrong format ...")
errors.add(:hostname, error_msg)

@saulotoledo
Copy link
Member

I added a comment also related to this post here.

lpichler and others added 2 commits December 21, 2017 08:07
- for available fields in report definition
- adding as keys to hash with results (this hash is passed to report engine)
@miq-bot
Copy link
Member

miq-bot commented Dec 21, 2017

Checked commits douglasgabriel/manageiq@a92c6b4~...da46e47 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 2 offenses detected

app/models/customization_script.rb

lib/utils/regex_util.rb

@douglasgabriel
Copy link
Member Author

Once the hostname format is validated, there is no need to remove the slash at the end of the hostname, so i'm replacing this PR with this other #16714

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants