Added the ability to enable icmp rules on Windows Firewalls

- Added more checks in the unit tests
- Updated unit tests to check for ICMP rules

salt/modules/win_firewall.py

@@ -116,13 +116,18 @@ def add_rule(name, localport, protocol='tcp', action='allow', dir='in'):
     .. code-block:: bash
 
         salt '*' firewall.add_rule 'test' '8080' 'tcp'
+        salt '*' firewall.add_rule 'test' '1' 'icmpv4'
+
     '''
     cmd = ['netsh', 'advfirewall', 'firewall', 'add', 'rule',
            'name={0}'.format(name),
            'protocol={0}'.format(protocol),
            'dir={0}'.format(dir),
-           'localport={0}'.format(localport),
            'action={0}'.format(action)]
+
+    if 'icmpv4' not in protocol and 'icmpv6' not in protocol:
+        cmd.append('localport={0}'.format(localport))
+
     ret = __salt__['cmd.run'](cmd, python_shell=False)
     if isinstance(ret, six.string_types):
         return ret.strip() == 'Ok.'
@@ -146,8 +151,11 @@ def delete_rule(name, localport, protocol='tcp', dir='in'):
     cmd = ['netsh', 'advfirewall', 'firewall', 'delete', 'rule',
            'name={0}'.format(name),
            'protocol={0}'.format(protocol),
-           'dir={0}'.format(dir),
-           'localport={0}'.format(localport)]
+           'dir={0}'.format(dir)]
+
+    if 'icmpv4' not in protocol and 'icmpv6' not in protocol:
+        cmd.append('localport={0}'.format(localport))
+
     ret = __salt__['cmd.run'](cmd, python_shell=False)
     if isinstance(ret, six.string_types):
         return ret.endswith('Ok.')

tests/unit/modules/win_firewall_test.py

@@ -10,7 +10,8 @@
 from salttesting import TestCase, skipIf
 from salttesting.mock import (
     MagicMock,
-    patch
+    patch,
+    call
 )
 
 from salttesting.helpers import ensure_in_syspath
@@ -41,6 +42,7 @@ def test_get_config(self):
         mock_cmd = MagicMock(return_value='')
         with patch.dict(win_firewall.__salt__, {'cmd.run': mock_cmd}):
             self.assertDictEqual(win_firewall.get_config(), {})
+            mock_cmd.assert_called_once_with(['netsh', 'advfirewall', 'show', 'allprofiles'], python_shell=False)
 
     # 'disable' function tests: 1
 
@@ -51,6 +53,8 @@ def test_disable(self):
         mock_cmd = MagicMock(return_value='Ok.')
         with patch.dict(win_firewall.__salt__, {'cmd.run': mock_cmd}):
             self.assertTrue(win_firewall.disable())
+            mock_cmd.assert_called_once_with(['netsh', 'advfirewall', 'set', 'allprofiles', 'state', 'off'],
+                                             python_shell=False)
 
     # 'enable' function tests: 1
 
@@ -61,6 +65,8 @@ def test_enable(self):
         mock_cmd = MagicMock(return_value='Ok.')
         with patch.dict(win_firewall.__salt__, {'cmd.run': mock_cmd}):
             self.assertTrue(win_firewall.enable())
+            mock_cmd.assert_called_once_with(['netsh', 'advfirewall', 'set', 'allprofiles', 'state', 'on'],
+                                             python_shell=False)
 
     # 'get_rule' function tests: 1
 
@@ -75,6 +81,12 @@ def test_get_rule(self):
 
             self.assertFalse(win_firewall.get_rule())
 
+            calls = [
+                call(['netsh', 'advfirewall', 'firewall', 'show', 'rule', 'name=all'], python_shell=False),
+                call(['netsh', 'advfirewall', 'firewall', 'show', 'rule', 'name=all'], python_shell=False)
+            ]
+            mock_cmd.assert_has_calls(calls)
+
     # 'add_rule' function tests: 1
 
     def test_add_rule(self):
@@ -84,6 +96,42 @@ def test_add_rule(self):
         mock_cmd = MagicMock(return_value='Ok.')
         with patch.dict(win_firewall.__salt__, {'cmd.run': mock_cmd}):
             self.assertTrue(win_firewall.add_rule("test", "8080"))
+            mock_cmd.assert_called_once_with(['netsh', 'advfirewall', 'firewall', 'add', 'rule', 'name=test',
+                                              'protocol=tcp', 'dir=in', 'action=allow', 'localport=8080'],
+                                             python_shell=False)
+
+    def test_add_rule_icmp4(self):
+        '''
+        Test if it add a new firewall rule
+        '''
+        mock_cmd = MagicMock(return_value='Ok.')
+        with patch.dict(win_firewall.__salt__, {'cmd.run': mock_cmd}):
+            self.assertTrue(win_firewall.add_rule("test", "1", protocol='icmpv4'))
+            mock_cmd.assert_called_once_with(['netsh', 'advfirewall', 'firewall', 'add', 'rule', 'name=test',
+                                              'protocol=icmpv4', 'dir=in', 'action=allow'],
+                                             python_shell=False)
+
+    def test_add_rule_icmp6(self):
+        '''
+        Test if it add a new firewall rule
+        '''
+        mock_cmd = MagicMock(return_value='Ok.')
+        with patch.dict(win_firewall.__salt__, {'cmd.run': mock_cmd}):
+            self.assertTrue(win_firewall.add_rule("test", "1", protocol='icmpv6'))
+            mock_cmd.assert_called_once_with(['netsh', 'advfirewall', 'firewall', 'add', 'rule', 'name=test',
+                                              'protocol=icmpv6', 'dir=in', 'action=allow'],
+                                             python_shell=False)
+
+    def test_add_rule_icmp4_any(self):
+        '''
+        Test if it add a new firewall rule
+        '''
+        mock_cmd = MagicMock(return_value='Ok.')
+        with patch.dict(win_firewall.__salt__, {'cmd.run': mock_cmd}):
+            self.assertTrue(win_firewall.add_rule("test", "1", protocol='icmpv4:any,any'))
+            mock_cmd.assert_called_once_with(['netsh', 'advfirewall', 'firewall', 'add', 'rule', 'name=test',
+                                              'protocol=icmpv4:any,any', 'dir=in', 'action=allow'],
+                                             python_shell=False)
 
     # 'delete_rule' function tests: 1
 
@@ -95,6 +143,41 @@ def test_delete_rule(self):
         with patch.dict(win_firewall.__salt__, {'cmd.run': mock_cmd}):
             self.assertTrue(win_firewall.delete_rule("test", "8080", "tcp",
                                                      "in"))
+            mock_cmd.assert_called_once_with(['netsh', 'advfirewall', 'firewall', 'delete', 'rule', 'name=test',
+                                              'protocol=tcp', 'dir=in', 'localport=8080'], python_shell=False)
+
+    def test_delete_rule_icmp4(self):
+        '''
+        Test if it deletes a new firewall rule
+        '''
+        mock_cmd = MagicMock(return_value='Ok.')
+        with patch.dict(win_firewall.__salt__, {'cmd.run': mock_cmd}):
+            self.assertTrue(win_firewall.delete_rule("test", "1", protocol='icmpv4'))
+            mock_cmd.assert_called_once_with(['netsh', 'advfirewall', 'firewall', 'delete', 'rule', 'name=test',
+                                              'protocol=icmpv4', 'dir=in'],
+                                             python_shell=False)
+
+    def test_delete_rule_icmp6(self):
+        '''
+        Test if it deletes a new firewall rule
+        '''
+        mock_cmd = MagicMock(return_value='Ok.')
+        with patch.dict(win_firewall.__salt__, {'cmd.run': mock_cmd}):
+            self.assertTrue(win_firewall.delete_rule("test", "1", protocol='icmpv6'))
+            mock_cmd.assert_called_once_with(['netsh', 'advfirewall', 'firewall', 'delete', 'rule', 'name=test',
+                                              'protocol=icmpv6', 'dir=in'],
+                                             python_shell=False)
+
+    def test_delete_rule_icmp4_any(self):
+        '''
+        Test if it deletes a new firewall rule
+        '''
+        mock_cmd = MagicMock(return_value='Ok.')
+        with patch.dict(win_firewall.__salt__, {'cmd.run': mock_cmd}):
+            self.assertTrue(win_firewall.delete_rule("test", "1", protocol='icmpv4:any,any'))
+            mock_cmd.assert_called_once_with(['netsh', 'advfirewall', 'firewall', 'delete', 'rule', 'name=test',
+                                              'protocol=icmpv4:any,any', 'dir=in'],
+                                             python_shell=False)
 
 
 if __name__ == '__main__':