Allow passing multiple ports in windows_firewall

Allow ports to be an integer
Allow ports to be a comma separated list or an array
Workaround a bug in PowerShell where arrays that are converted to json turn into hashes with the count and the value
The quotes around the port are removed because you can't pass '80,8080' in the cmdlet. It has to be a bare 80,8080

Signed-off-by: Tim Smith <tsmith@chef.io>

2 files changed, 62 insertions, 24 deletions

diff --git a/lib/chef/resource/windows_firewall_rule.rb b/lib/chef/resource/windows_firewall_rule.rb
index a7e4609605..466378f615 100644
--- a/lib/chef/resource/windows_firewall_rule.rb
+++ b/lib/chef/resource/windows_firewall_rule.rb
@@ -40,18 +40,19 @@ class Chef
       property :local_address, String,
                description: "The local address the firewall rule applies to."
 
-      property :local_port, String,
+      property :local_port, [String, Integer, Array],
+               coerce: proc { |d| d.is_a?(String) ? d.split(/\s*,\s*/).sort : Array(d).sort },
                description: "The local port the firewall rule applies to."
 
       property :remote_address, String,
                description: "The remote address the firewall rule applies to."
 
-      property :remote_port, String,
+      property :remote_port, [String, Integer, Array],
+               coerce: proc { |d| d.is_a?(String) ? d.split(/\s*,\s*/).sort : Array(d).sort },
                description: "The remote port the firewall rule applies to."
 
       property :direction, [Symbol, String],
-               default: :inbound,
-               equal_to: [:inbound, :outbound],
+               default: :inbound, equal_to: [:inbound, :outbound],
                description: "The direction of the firewall rule. Direction means either inbound or outbound traffic.",
                coerce: proc { |d| d.is_a?(String) ? d.downcase.to_sym : d }
 
@@ -60,14 +61,12 @@ class Chef
                description: "The protocol the firewall rule applies to."
 
       property :firewall_action, [Symbol, String],
-               default: :allow,
-               equal_to: [:allow, :block, :notconfigured],
+               default: :allow, equal_to: [:allow, :block, :notconfigured],
                description: "The action of the firewall rule.",
                coerce: proc { |f| f.is_a?(String) ? f.downcase.to_sym : f }
 
       property :profile, [Symbol, String],
-               default: :any,
-               equal_to: [:public, :private, :domain, :any, :notapplicable],
+               default: :any, equal_to: [:public, :private, :domain, :any, :notapplicable],
                description: "The profile the firewall rule applies to.",
                coerce: proc { |p| p.is_a?(String) ? p.downcase.to_sym : p }
 
@@ -78,8 +77,7 @@ class Chef
                description: "The service the firewall rule applies to."
 
       property :interface_type, [Symbol, String],
-               default: :any,
-               equal_to: [:any, :wireless, :wired, :remoteaccess],
+               default: :any, equal_to: [:any, :wireless, :wired, :remoteaccess],
                description: "The interface type the firewall rule applies to.",
                coerce: proc { |i| i.is_a?(String) ? i.downcase.to_sym : i }
 
@@ -102,9 +100,9 @@ class Chef
           state = Chef::JSONCompat.from_json(output.stdout)
         end
         local_address state["local_address"]
-        local_port state["local_port"]
+        local_port Array(state["local_port"]).sort
         remote_address state["remote_address"]
-        remote_port state["remote_port"]
+        remote_port Array(state["remote_port"]).sort
         direction state["direction"]
         protocol state["protocol"]
         firewall_action state["firewall_action"]
@@ -116,6 +114,8 @@ class Chef
       end
 
       action :create do
+        description "Create a Windows firewall entry."
+
         if current_resource
           converge_if_changed :rule_name, :local_address, :local_port, :remote_address, :remote_port, :direction,
                               :protocol, :firewall_action, :profile, :program, :service, :interface_type, :enabled do
@@ -131,6 +131,8 @@ class Chef
       end
 
       action :delete do
+        description "Delete an existing Windows firewall entry."
+
         if current_resource
           converge_by("delete firewall rule #{new_resource.rule_name}") do
             powershell_out!("Remove-NetFirewallRule -Name '#{new_resource.rule_name}'")
@@ -148,9 +150,9 @@ class Chef
           cmd << " -DisplayName '#{new_resource.rule_name}'" if cmdlet_type == "New"
           cmd << " -Description '#{new_resource.description}'" if new_resource.description
           cmd << " -LocalAddress '#{new_resource.local_address}'" if new_resource.local_address
-          cmd << " -LocalPort '#{new_resource.local_port}'" if new_resource.local_port
+          cmd << " -LocalPort #{new_resource.local_port.join(',')}" if new_resource.local_port
           cmd << " -RemoteAddress '#{new_resource.remote_address}'" if new_resource.remote_address
-          cmd << " -RemotePort '#{new_resource.remote_port}'" if new_resource.remote_port
+          cmd << " -RemotePort #{new_resource.remote_port.join(',')}" if new_resource.remote_port
           cmd << " -Direction '#{new_resource.direction}'" if new_resource.direction
           cmd << " -Protocol '#{new_resource.protocol}'" if new_resource.protocol
           cmd << " -Action '#{new_resource.firewall_action}'" if new_resource.firewall_action
@@ -170,6 +172,7 @@ class Chef
       # # @return [String] current firewall state
       def load_firewall_state(rule_name)
         <<-EOH
+          Remove-TypeData System.Array # workaround for PS bug here: https://bit.ly/2SRMQ8M
           $rule = Get-NetFirewallRule -Name '#{rule_name}'
           $addressFilter = $rule | Get-NetFirewallAddressFilter
           $portFilter = $rule | Get-NetFirewallPortFilter
diff --git a/spec/unit/resource/windows_firewall_rule_spec.rb b/spec/unit/resource/windows_firewall_rule_spec.rb
index b20425e21b..228e94f9c0 100644
--- a/spec/unit/resource/windows_firewall_rule_spec.rb
+++ b/spec/unit/resource/windows_firewall_rule_spec.rb
@@ -53,9 +53,24 @@ describe Chef::Resource::WindowsFirewallRule do
     expect(resource.local_address).to eql("192.168.1.1")
   end
 
+  it "the local_port property accepts integers" do
+    resource.local_port(8080)
+    expect(resource.local_port).to eql([8080])
+  end
+
   it "the local_port property accepts strings" do
     resource.local_port("8080")
-    expect(resource.local_port).to eql("8080")
+    expect(resource.local_port).to eql(["8080"])
+  end
+
+  it "the local_port property accepts comma separated lists without spaces" do
+    resource.local_port("8080,8081")
+    expect(resource.local_port).to eql(%w{8080 8081})
+  end
+
+  it "the local_port property accepts comma separated lists with spaces" do
+    resource.local_port("8080, 8081")
+    expect(resource.local_port).to eql(%w{8080 8081})
   end
 
   it "the remote_address property accepts strings" do
@@ -65,7 +80,27 @@ describe Chef::Resource::WindowsFirewallRule do
 
   it "the remote_port property accepts strings" do
     resource.remote_port("8081")
-    expect(resource.remote_port).to eql("8081")
+    expect(resource.remote_port).to eql(["8081"])
+  end
+
+  it "the remote_port property accepts integers" do
+    resource.remote_port(8081)
+    expect(resource.remote_port).to eql([8081])
+  end
+
+  it "the remote_port property accepts comma separated lists without spaces" do
+    resource.remote_port("8080,8081")
+    expect(resource.remote_port).to eql(%w{8080 8081})
+  end
+
+  it "the remote_port property accepts comma separated lists with spaces" do
+    resource.remote_port("8080, 8081")
+    expect(resource.remote_port).to eql(%w{8080 8081})
+  end
+
+  it "the remote_port property accepts arrays and sorts the array" do
+    resource.remote_port([8081, 8080])
+    expect(resource.remote_port).to eql([8080, 8081])
   end
 
   it "the direction property accepts :inbound and :outbound" do
@@ -162,12 +197,12 @@ describe Chef::Resource::WindowsFirewallRule do
 
   it "aliases :localport to :local_port" do
     resource.localport("80")
-    expect(resource.local_port).to eql("80")
+    expect(resource.local_port).to eql(["80"])
   end
 
   it "aliases :remoteport to :remote_port" do
     resource.remoteport("8080")
-    expect(resource.remote_port).to eql("8080")
+    expect(resource.remote_port).to eql(["8080"])
   end
 
   it "aliases :interfacetype to :interface_type" do
@@ -197,7 +232,7 @@ describe Chef::Resource::WindowsFirewallRule do
 
       it "sets LocalPort" do
         resource.local_port("80")
-        expect(provider.firewall_command("New")).to eql("New-NetFirewallRule -Name 'test_rule' -DisplayName 'test_rule' -Description 'Firewall rule' -LocalPort '80' -Direction 'inbound' -Protocol 'TCP' -Action 'allow' -Profile 'any' -InterfaceType 'any' -Enabled 'true'")
+        expect(provider.firewall_command("New")).to eql("New-NetFirewallRule -Name 'test_rule' -DisplayName 'test_rule' -Description 'Firewall rule' -LocalPort 80 -Direction 'inbound' -Protocol 'TCP' -Action 'allow' -Profile 'any' -InterfaceType 'any' -Enabled 'true'")
       end
 
       it "sets RemoteAddress" do
@@ -207,7 +242,7 @@ describe Chef::Resource::WindowsFirewallRule do
 
       it "sets RemotePort" do
         resource.remote_port("443")
-        expect(provider.firewall_command("New")).to eql("New-NetFirewallRule -Name 'test_rule' -DisplayName 'test_rule' -Description 'Firewall rule' -RemotePort '443' -Direction 'inbound' -Protocol 'TCP' -Action 'allow' -Profile 'any' -InterfaceType 'any' -Enabled 'true'")
+        expect(provider.firewall_command("New")).to eql("New-NetFirewallRule -Name 'test_rule' -DisplayName 'test_rule' -Description 'Firewall rule' -RemotePort 443 -Direction 'inbound' -Protocol 'TCP' -Action 'allow' -Profile 'any' -InterfaceType 'any' -Enabled 'true'")
       end
 
       it "sets Direction" do
@@ -265,7 +300,7 @@ describe Chef::Resource::WindowsFirewallRule do
         resource.service("SomeService")
         resource.interface_type(:remoteaccess)
         resource.enabled(false)
-        expect(provider.firewall_command("New")).to eql("New-NetFirewallRule -Name 'test_rule_the_second' -DisplayName 'test_rule_the_second' -Description 'some other rule' -LocalAddress '192.168.40.40' -LocalPort '80' -RemoteAddress '8.8.4.4' -RemotePort '8081' -Direction 'outbound' -Protocol 'UDP' -Action 'notconfigured' -Profile 'domain' -Program '%WINDIR%\\System32\\lsass.exe' -Service 'SomeService' -InterfaceType 'remoteaccess' -Enabled 'false'")
+        expect(provider.firewall_command("New")).to eql("New-NetFirewallRule -Name 'test_rule_the_second' -DisplayName 'test_rule_the_second' -Description 'some other rule' -LocalAddress '192.168.40.40' -LocalPort 80 -RemoteAddress '8.8.4.4' -RemotePort 8081 -Direction 'outbound' -Protocol 'UDP' -Action 'notconfigured' -Profile 'domain' -Program '%WINDIR%\\System32\\lsass.exe' -Service 'SomeService' -InterfaceType 'remoteaccess' -Enabled 'false'")
       end
     end
 
@@ -286,7 +321,7 @@ describe Chef::Resource::WindowsFirewallRule do
 
       it "sets LocalPort" do
         resource.local_port("80")
-        expect(provider.firewall_command("Set")).to eql("Set-NetFirewallRule -Name 'test_rule' -Description 'Firewall rule' -LocalPort '80' -Direction 'inbound' -Protocol 'TCP' -Action 'allow' -Profile 'any' -InterfaceType 'any' -Enabled 'true'")
+        expect(provider.firewall_command("Set")).to eql("Set-NetFirewallRule -Name 'test_rule' -Description 'Firewall rule' -LocalPort 80 -Direction 'inbound' -Protocol 'TCP' -Action 'allow' -Profile 'any' -InterfaceType 'any' -Enabled 'true'")
       end
 
       it "sets RemoteAddress" do
@@ -296,7 +331,7 @@ describe Chef::Resource::WindowsFirewallRule do
 
       it "sets RemotePort" do
         resource.remote_port("443")
-        expect(provider.firewall_command("Set")).to eql("Set-NetFirewallRule -Name 'test_rule' -Description 'Firewall rule' -RemotePort '443' -Direction 'inbound' -Protocol 'TCP' -Action 'allow' -Profile 'any' -InterfaceType 'any' -Enabled 'true'")
+        expect(provider.firewall_command("Set")).to eql("Set-NetFirewallRule -Name 'test_rule' -Description 'Firewall rule' -RemotePort 443 -Direction 'inbound' -Protocol 'TCP' -Action 'allow' -Profile 'any' -InterfaceType 'any' -Enabled 'true'")
       end
 
       it "sets Direction" do
@@ -354,7 +389,7 @@ describe Chef::Resource::WindowsFirewallRule do
         resource.service("SomeService")
         resource.interface_type(:remoteaccess)
         resource.enabled(false)
-        expect(provider.firewall_command("Set")).to eql("Set-NetFirewallRule -Name 'test_rule_the_second' -Description 'some other rule' -LocalAddress '192.168.40.40' -LocalPort '80' -RemoteAddress '8.8.4.4' -RemotePort '8081' -Direction 'outbound' -Protocol 'UDP' -Action 'notconfigured' -Profile 'domain' -Program '%WINDIR%\\System32\\lsass.exe' -Service 'SomeService' -InterfaceType 'remoteaccess' -Enabled 'false'")
+        expect(provider.firewall_command("Set")).to eql("Set-NetFirewallRule -Name 'test_rule_the_second' -Description 'some other rule' -LocalAddress '192.168.40.40' -LocalPort 80 -RemoteAddress '8.8.4.4' -RemotePort 8081 -Direction 'outbound' -Protocol 'UDP' -Action 'notconfigured' -Profile 'domain' -Program '%WINDIR%\\System32\\lsass.exe' -Service 'SomeService' -InterfaceType 'remoteaccess' -Enabled 'false'")
       end
     end
   end