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.

Sign up for GitHub

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

Resolve string handling for "https://" or "http://" in update_rhsm_conf #17222

Merged
merged 6 commits into from
Apr 3, 2018
+24 −16
Merged

Resolve string handling for "https://" or "http://" in update_rhsm_conf #17222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ab7f652
Resolve string handling for "https://" or "http://" in update_rhsm_conf
robbmanes Mar 28, 2018
58c65e4
Add tests for RegistrationSystem.update_rhsm_conf with http and https
bdunne Mar 29, 2018
65bfb6a
Reduce duplicate test logic
bdunne Mar 29, 2018
47ddbb4
Test against multiple addresses (hostname, IPv4, IPv6)
bdunne Mar 29, 2018
dafb5f7
myport is not a valid port, 0 is reserved so use that instead
bdunne Mar 29, 2018
963a3f3
Fix issues related to proxy_address
bdunne Mar 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions app/models/registration_system.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,9 @@ def self.update_rhsm_conf(options = {})

return unless option_values[:proxy_address]

write_rhsm_config(:proxy_hostname => option_values[:proxy_address].split(':')[0],
:proxy_port => option_values[:proxy_address].split(':')[1],
proxy_uri = URI.parse(option_values[:proxy_address].include?("://") ? option_values[:proxy_address] : "http://#{option_values[:proxy_address]}")
Copy link
Member

@carbonin carbonin Apr 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like this just punts the issue down the road. Is there no field validation for this? Couldn't we just say "This thing needs to be a valid URI" and give a flash message otherwise?

For example if someone were to forget a / and enter "http:/example.com:123" you would end up in this situation right?

irb(main):007:0> uri = URI.parse("http://http:/example.com:123")
=> #<URI::HTTP http://http/example.com:123>
irb(main):008:0> uri.host
=> "http"
irb(main):009:0> uri.port
=> 80

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree - I tried using Addressable::URI objects initially before I did string checking but am not sure there is error checking by the caller of this method, and testing with that manner threw other ugly exceptions further down the stack that similar didn't appear in the UI or any easy-to-interpret log, so I instead just altered the string validation.

I can look at it deeper after this week, I am onsite with a customer now unfortunately, or we can close this PR in favor of a method that does better error handling.

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 I'm okay with this as is for now. @carbonin and I were just discussing that we may want to open a RFE to the UI to add validation to this field. At least this adds the ability to accept URLs

write_rhsm_config(:proxy_hostname => proxy_uri.host,
:proxy_port => proxy_uri.port,
:proxy_user => option_values[:proxy_username],
:proxy_password => option_values[:proxy_password])
end
Expand Down
35 changes: 21 additions & 14 deletions spec/models/registration_system_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,10 @@
ssl_verify_depth = 3

# an http proxy server to use
proxy_hostname = 192.0.2.0
proxy_hostname = ProxyHostnameValue

# port for http proxy server
proxy_port = myport
proxy_port = 0

# user name for authenticating to an http proxy, if needed
proxy_user = my_dummy_username
Expand All @@ -195,14 +195,9 @@
EOT
end

let(:rhsm_conf) { Tempfile.new(@spec_name.downcase) }
let(:rhsm_conf) { Tempfile.new }

before do
@proxy_args = {:registration_http_proxy_server => "192.0.2.0:myport",
:registration_http_proxy_username => "my_dummy_username",
:registration_http_proxy_password => "my_dummy_password"}

@spec_name = File.basename(__FILE__).split(".rb").first.freeze
stub_const("RegistrationSystem::RHSM_CONFIG_FILE", rhsm_conf.path)
rhsm_conf.write(original_rhsm_conf)
rhsm_conf.close
Expand All @@ -213,10 +208,22 @@
FileUtils.rm_f("#{rhsm_conf.path}.miq_orig")
end

it "will save then update the original config file" do
RegistrationSystem.update_rhsm_conf(@proxy_args)
expect(File.read("#{rhsm_conf.path}.miq_orig")).to eq(original_rhsm_conf)
expect(File.read(rhsm_conf)).to eq(updated_rhsm_conf)
context "will save then update the original config file" do
["", "http://", "https://"].each do |prefix|
["proxy.example.com", "192.0.2.0", "[2001:db8::]"].each do |address|
params = {
:registration_http_proxy_server => "#{prefix}#{address}:0",
:registration_http_proxy_username => "my_dummy_username",
:registration_http_proxy_password => "my_dummy_password"
}

it "with #{params[:registration_http_proxy_server]}" do
RegistrationSystem.update_rhsm_conf(params)
expect(File.read("#{rhsm_conf.path}.miq_orig")).to eq(original_rhsm_conf)
expect(File.read(rhsm_conf)).to eq(updated_rhsm_conf.sub(/ProxyHostnameValue/, address))
end
end
end
end

it "with no proxy server will not update the rhsm config file" do
Expand All @@ -227,10 +234,10 @@
it "with no options will use database valuses" do
MiqDatabase.seed
MiqDatabase.first.update_attributes(
:registration_http_proxy_server => "192.0.2.0:myport"
:registration_http_proxy_server => "192.0.2.0:0"
)
RegistrationSystem.update_rhsm_conf
expect(File.read(rhsm_conf)).to include("proxy_hostname = 192.0.2.0", "proxy_port = myport")
expect(File.read(rhsm_conf)).to include("proxy_hostname = 192.0.2.0", "proxy_port = 0")
end
end
end