review commentstball/proxifier

4 files changed, 47 insertions, 7 deletions

diff --git a/.travis.yml b/.travis.yml
index c208c42edc..78303d7591 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -118,7 +118,6 @@ matrix:
   - rvm: 2.2
     sudo: required
     dist: trusty
-    cache:
     before_install:
       - sudo apt-get update
       - sudo apt-get -y install squid3 git curl
diff --git a/chef-config/lib/chef-config/config.rb b/chef-config/lib/chef-config/config.rb
index 74b9982e16..113bf481ff 100644
--- a/chef-config/lib/chef-config/config.rb
+++ b/chef-config/lib/chef-config/config.rb
@@ -729,8 +729,8 @@ module ChefConfig
     # users don't have to
     def self.export_proxies
       export_proxy("http", http_proxy, http_proxy_user, http_proxy_pass) if http_proxy
-      export_proxy("https", https_proxy, https_proxy_user, https_proxy_pass ) if https_proxy
-      export_proxy("ftp", ftp_proxy, ftp_proxy_user, ftp_proxy_pass ) if ftp_proxy
+      export_proxy("https", https_proxy, https_proxy_user, https_proxy_pass) if https_proxy
+      export_proxy("ftp", ftp_proxy, ftp_proxy_user, ftp_proxy_pass) if ftp_proxy
       export_no_proxy("no_proxy", no_proxy) if no_proxy
     end
 
@@ -743,6 +743,7 @@ module ChefConfig
     #   hostport = hostname:port or scheme://hostname:port
     #   user = username
     #   pass = password
+    # @api private
     def self.export_proxy(scheme, path, user, pass)
       path = "#{scheme}://#{path}" unless path.include?('://')
       # URI.split returns the following parts:
@@ -751,7 +752,7 @@ module ChefConfig
       # URI::Generic.build requires an integer for the port, but URI::split gives
       # returns a string for the port.
       parts[3] = parts[3].to_i if parts[3]
-      if user
+      if user && !user.empty?
         userinfo = URI.encode(URI.encode(user), '@:')
         if pass
           userinfo << ":#{URI.encode(URI.encode(pass), '@:')}"
@@ -760,10 +761,11 @@ module ChefConfig
       end
 
       path = URI::Generic.build(parts).to_s
-      ENV["#{scheme}_proxy"] = path unless ENV["#{scheme}_proxy"]
+      ENV["#{scheme}_proxy".downcase] = path unless ENV["#{scheme}_proxy".downcase]
       ENV["#{scheme}_proxy".upcase] = path unless ENV["#{scheme}_proxy".upcase]
     end
 
+    # @api private
     def self.export_no_proxy(value)
       ENV['no_proxy'] = value unless ENV['no_proxy']
       ENV['NO_PROXY'] = value unless ENV['NO_PROXY']
diff --git a/chef-config/spec/unit/config_spec.rb b/chef-config/spec/unit/config_spec.rb
index 91902fc1f9..0a3dca5b5a 100644
--- a/chef-config/spec/unit/config_spec.rb
+++ b/chef-config/spec/unit/config_spec.rb
@@ -591,7 +591,7 @@ RSpec.describe ChefConfig::Config do
       end
     end
 
-    context "when http_proxy, proxy_pass and proxy_user are set" do
+    context "when https_proxy, proxy_pass and proxy_user are set" do
       before do
         ChefConfig::Config.https_proxy = https_proxy
         ChefConfig::Config.https_proxy_user = proxy_user
@@ -604,7 +604,7 @@ RSpec.describe ChefConfig::Config do
       end
     end
 
-    context "when http_proxy, proxy_pass and proxy_user are set" do
+    context "when ftp_proxy, proxy_pass and proxy_user are set" do
       before do
         ChefConfig::Config.ftp_proxy = ftp_proxy
         ChefConfig::Config.ftp_proxy_user = proxy_user
@@ -617,6 +617,41 @@ RSpec.describe ChefConfig::Config do
       end
     end
 
+    shared_examples "no user pass" do
+      it "does not populate the user or password" do
+        expect(ENV).to receive(:[]=).with('http_proxy', "http://localhost:7979")
+        expect(ENV).to receive(:[]=).with('HTTP_PROXY', "http://localhost:7979")
+        ChefConfig::Config.export_proxies
+      end
+    end
+
+    context "when proxy_pass and proxy_user are passed as empty strings" do
+      before do
+        ChefConfig::Config.http_proxy = http_proxy
+        ChefConfig::Config.http_proxy_user = ""
+        ChefConfig::Config.http_proxy_pass = proxy_pass
+      end
+      include_examples "no user pass"
+    end
+
+    context "when proxy_pass and proxy_user are not provided" do
+      before do
+        ChefConfig::Config.http_proxy = http_proxy
+      end
+      include_examples "no user pass"
+    end
+
+    context "when the proxy is provided without a scheme" do
+      before do
+        ChefConfig::Config.http_proxy = "localhost:1111"
+      end
+      it "automatically adds the scheme to the proxy url" do
+        expect(ENV).to receive(:[]=).with('http_proxy', "http://localhost:1111")
+        expect(ENV).to receive(:[]=).with('HTTP_PROXY', "http://localhost:1111")
+        ChefConfig::Config.export_proxies
+      end
+    end
+
     shared_examples "no export" do
       it "does not export any proxy settings" do
         ChefConfig::Config.export_proxies
diff --git a/spec/unit/mixin/proxified_socket_spec.rb b/spec/unit/mixin/proxified_socket_spec.rb
index d999d09235..88f71ae48b 100644
--- a/spec/unit/mixin/proxified_socket_spec.rb
+++ b/spec/unit/mixin/proxified_socket_spec.rb
@@ -61,6 +61,8 @@ describe Chef::Mixin::ProxifiedSocket do
 
   context "when https_proxy is set" do
     before do
+      # I'm purposefully setting both of these because we prefer the https
+      # variable
       ENV['https_proxy'] = https_uri
       ENV['http_proxy'] = http_uri
     end
@@ -69,6 +71,8 @@ describe Chef::Mixin::ProxifiedSocket do
     include_examples "proxified socket"
 
     context "when no_proxy is set" do
+      # This is testing that no_proxy is also provided to Proxified
+      # when it is set
       before do
         ENV['no_proxy'] = no_proxy_spec
       end