win_firewall_rule: Implement idempotency, check-mode and diff support (#23162)

* win_firewall_rule: Small idempotency fix

This PR includes the following changes:
- an idempotency fix when `profile: any`
- better difference output to debug idempotency issues
- documentation fixes (remove `required: false`)
- Parameter handling fixes
- RDP example that matches default RDP rule
- Renamed parameter 'enable' to 'enabled' (kept alias)
- Renamed parameter 'profile' to 'profiles' (kept alias)

* Rewrite module completely

The logic is still intact, but various changes with a single goal:

- Make the module idempotent
- Implement check-mode
- Implement diff-mode
- Adapted integration tests

This fixes #18807 and #23455.

* Change casing to lowercase

* Improve the logic wrt. diff

lib/ansible/modules/windows/win_firewall_rule.ps1

@@ -20,12 +20,30 @@
 # WANT_JSON
 # POWERSHELL_COMMON
 
+# TODO: Reimplement this using Powershell cmdlets
+
+$ErrorActionPreference = "Stop"
+
 function convertToNetmask($maskLength) {
-    [IPAddress] $ip = 0;
+    [IPAddress] $ip = 0
     $ip.Address = ([UInt32]::MaxValue) -shl (32 - $maskLength) -shr (32 - $maskLength)
     return $ip.IPAddressToString
 }
 
+function ConvertTo-TitleCase($string) {
+    return (Get-Culture).TextInfo.ToTitleCase($string.ToLower())
+}
+
+function ConvertTo-SortedKV($object, $unsupported = @()) {
+    $output = ""
+    foreach($item in $object.GetEnumerator() | Sort -Property Name) {
+        if (($item.Name -notin $unsupported) -and ($item.Value -ne $null)) {
+            $output += "$($item.Name): $($item.Value)`n"
+        }
+    }
+    return $output
+}
+
 function preprocessAndCompare($key, $outputValue, $fwsettingValue) {
     if ($key -eq 'RemoteIP') {
         if ($outputValue -eq $fwsettingValue) {
@@ -54,351 +72,382 @@ function preprocessAndCompare($key, $outputValue, $fwsettingValue) {
             }
         }
     }
-    elseif ($key -eq 'Profiles') {
-        if (($fwsettingValue -eq "any") -and ($outputValue -eq "Domain,Private,Public")) {
-            return $true
-        }
-    }
 
     return $false
 }
 
 function getFirewallRule ($fwsettings) {
-    try {
+    $diff = $false
+    $result = @{
+        changed = $false
+        identical = $false
+        exists = $false
+        failed = $false
+        msg = @()
+        multiple = $false
+    }
 
-        #$output = Get-NetFirewallRule -name $($fwsettings.'Rule Name');
+    try {
-        $rawoutput=@(netsh advfirewall firewall show rule name="$($fwsettings.'Rule Name')" verbose)
+        $command = "netsh advfirewall firewall show rule name=`"$($fwsettings.'Rule Name')`" verbose"
-        if (!($rawoutput -eq 'No rules match the specified criteria.')){
+        #$output = Get-NetFirewallRule -name $($fwsettings.'Rule Name')
-            $rawoutput | Where {$_ -match '^([^:]+):\s*(\S.*)$'} | Foreach -Begin {
+        $result.output = Invoke-Expression $command | Where { $_  }
-                    $FirstRun = $true;
+        $rc = $LASTEXITCODE
-                    $HashProps = @{};
+        if ($rc -eq 1) {
+            $result.msg += @("No rule '$name' could be found")
+        } elseif ($rc -eq 0) {
+            # Process command output
+            $result.output | Where {$_ -match '^([^:]+):\s*(\S.*)$'} | ForEach -Begin {
+                    $FirstRun = $true
+                    $HashProps = @{}
                 } -Process {
-                    if (($Matches[1] -eq 'Rule Name') -and (!($FirstRun))) {
+                    if (($Matches[1] -eq 'Rule Name') -and (-not $FirstRun)) {
-                        #$output=New-Object -TypeName PSCustomObject -Property $HashProps;
+                        $output = $HashProps
-                        $output=$HashProps;
+                        $HashProps = @{}
-                        $HashProps = @{};
+                    }
-                    };
+                    $HashProps.$($Matches[1]) = $Matches[2]
-                    $HashProps.$($Matches[1]) = $Matches[2];
+                    $FirstRun = $false
-                    $FirstRun = $false;
                 } -End {
-                #$output=New-Object -TypeName PSCustomObject -Property $HashProps;
+                    $output = $HashProps
-                $output=$HashProps;
                 }
-        }
+            if ($($output|measure).count -gt 0) {
-        $exists=$false;
+                $diff = $false
-        $correct=$true;
+                $result.exists = $true
-        $diff=$false;
+                #$result.msg += @("The rule '$($fwsettings.'Rule Name')' exists.")
-        $multi=$false;
+                if ($($output|measure).count -gt 1) {
-        $correct=$false;
+                    $result.multiple = $true
-        $difference=@();
+                    $result.msg += @("The rule '$($fwsettings.'Rule Name')' has multiple entries.")
-        $msg=@();
+                    $result.diff = @{}
-        if ($($output|measure).count -gt 0) {
+                    $result.diff.after = ConvertTo-SortedKV $fwsettings
-            $exists=$true;
+                    $result.diff.before = ConvertTo-SortedKV $rule $unsupported
-            $msg += @("The rule '" + $fwsettings.'Rule Name' + "' exists.");
+                    if ($result.diff.after -ne $result.diff.before ) {
-            if ($($output|measure).count -gt 1) {
+                        $diff = $true
-                $multi=$true
+                    }
-                $msg += @("The rule '" + $fwsettings.'Rule Name' + "' has multiple entries.");
+                } else {
-                ForEach($rule in $output.GetEnumerator()) {
+                    if ($diff_support) {
+                        $result.diff = @{}
+                        $result.diff.after = ConvertTo-SortedKV $fwsettings
+                        $result.diff.before = ConvertTo-SortedKV $output $unsupported
+                    }
                     ForEach($fwsetting in $fwsettings.GetEnumerator()) {
-                        if ( $rule.$fwsetting -ne $fwsettings.$fwsetting) {
+                        if ($output.$($fwsetting.Key) -ne $fwsettings.$($fwsetting.Key)) {
-                            $diff=$true;
+                            if ((preprocessAndCompare -key $fwsetting.Key -outputValue $output.$($fwsetting.Key) -fwsettingValue $fwsettings.$($fwsetting.Key))) {
-                            #$difference+=@($fwsettings.$($fwsetting.Key));
+                                Continue
-                            $difference+=@("output:$rule.$fwsetting,fwsetting:$fwsettings.$fwsetting");
+                            } elseif (($fwsetting.Key -eq 'DisplayName') -and ($output."Rule Name" -eq $fwsettings.$($fwsetting.Key))) {
-                        };
+                                Continue
-                    };
+                            } elseif (($fwsetting.Key -eq 'Program') -and ($output.$($fwsetting.Key) -eq (Expand-Environment($fwsettings.$($fwsetting.Key))))) {
-                    if ($diff -eq $false) {
+                                # Ignore difference caused by expanded environment variables
-                        $correct=$true
+                                Continue
-                    };
+                            } else {
-                };
+                                $diff = $true
-            } else {
+                                Break
-                ForEach($fwsetting in $fwsettings.GetEnumerator()) {
+                            }
-                    if ($output.$($fwsetting.Key) -ne $fwsettings.$($fwsetting.Key)) {
+                        }
-                        if ((preprocessAndCompare -key $fwsetting.Key -outputValue $output.$($fwsetting.Key) -fwsettingValue $fwsettings.$($fwsetting.Key))) {
+                    }
-                            Continue
+                }
-                        } elseif (($fwsetting.Key -eq 'DisplayName') -and ($output."Rule Name" -eq $fwsettings.$($fwsetting.Key))) {
+                if (-not $diff) {
-                            Continue
+                    $result.identical = $true
-                        } else {
+                }
-                            $diff=$true;
+                if ($result.identical) {
-                            $difference+=@($fwsettings.$($fwsetting.Key));
+                    $result.msg += @("The rule '$name' exists and is identical")
-                        };
+                } else {
-                    };
+                    $result.msg += @("The rule '$name' exists but has different values")
-                };
+                }
-                if ($diff -eq $false) {
-                    $correct=$true
-                };
-            };
-            if ($correct) {
-                $msg += @("An identical rule exists");
-            } else {
-                $msg += @("The rule exists but has different values");
             }
         } else {
-            $msg += @("No rule could be found");
+            $result.failed = $true
-        };
-        $result = @{
-            failed = $false
-            exists = $exists
-            identical = $correct
-            multiple = $multi
-            difference = $difference
-            msg = $msg
         }
-    } catch [Exception]{
+    } catch [Exception] {
-        $result = @{
+        $result.failed = $true
-            failed = $true
+        $result.error = $_.Exception.Message
-            error = $_.Exception.Message
+    }
-            msg = $msg
-        }
-    };
     return $result
-};
+}
 
 function createFireWallRule ($fwsettings) {
-    $msg=@()
+    $result = @{
-    $execString="netsh advfirewall firewall add rule"
+        changed = $false
+        failed = $false
+        msg = @()
+    }
 
+    $command = "netsh advfirewall firewall add rule"
     ForEach ($fwsetting in $fwsettings.GetEnumerator()) {
-        if ($fwsetting.key -eq 'Direction') {
+        if ($fwsetting.value -ne $null) {
-            $key='dir'
+            switch($fwsetting.key) {
-        } elseif ($fwsetting.key -eq 'Rule Name') {
+                "Direction" { $option = "dir" }
-            $key='name'
+                "Rule Name" { $option = "name" }
-        } elseif ($fwsetting.key -eq 'Enabled') {
+                "Enabled" { $option = "enable" }
-            $key='enable'
+                "Profiles" { $option = "profile" }
-        } elseif ($fwsetting.key -eq 'Profiles') {
+                "InterfaceTypes" { $option = "interfacetype" }
-            $key='profile'
+                "Security" { $option = "security" }
-        } else {
+                "Edge traversal" { $option = "edge" }
-            $key=$($fwsetting.key).ToLower()
+                default { $option = $($fwsetting.key).ToLower() }
-        };
+            }
-        $execString+=" ";
+            $command += " $option='$($fwsetting.value)'"
-        $execString+=$key;
+        }
-        $execString+="=";
+    }
-        $execString+='"';
+
-        $execString+=$fwsetting.value;
-        $execString+='"';
-    };
     try {
-        #$msg+=@($execString);
+        $rc = 0
-        $output=$(Invoke-Expression $execString| ? {$_});
+        if (-not $check_mode) {
-        $msg+=@("Created firewall rule $name");
+            $result.output = Invoke-Expression $command | Where { $_ }
-
+            $rc = $LASTEXITCODE
-        $result=@{
+        }
-            failed = $false
+        if ($rc -eq 0) {
-            output=$output
+            if ($diff_support) {
-            changed=$true
+                $result.diff = @{}
-            msg=$msg
+                $result.diff.after = ConvertTo-SortedKV $fwsettings
-        };
+                $result.diff.before= ""
-
+            }
+            $result.changed = $true
+            $result.msg += @("Created firewall rule '$name'")
+        } else {
+            $result.failed = $true
+            $result.msg += @("Create command '$command' failed with rc=$rc")
+        }
     } catch [Exception]{
-        $msg=@("Failed to create the rule")
+        $result.error = $_.Exception.Message
-        $result=@{
+        $result.failed = $true
-            output=$output
+        $result.msg = @("Failed to create the rule '$name'")
-            failed=$true
+    }
-            error=$_.Exception.Message
-            msg=$msg
-        };
-    };
     return $result
-};
+}
 
 function removeFireWallRule ($fwsettings) {
-    $msg=@()
+    $result = @{
+        changed = $false
+        failed = $false
+        msg = @()
+    }
+
+    $command = "netsh advfirewall firewall delete rule name='$($fwsettings.'Rule Name')'"
     try {
-        $rawoutput=@(netsh advfirewall firewall delete rule name="$($fwsettings.'Rule Name')")
+        $rc = 0
-        $rawoutput | Where {$_ -match '^([^:]+):\s*(\S.*)$'} | Foreach -Begin {
+        if (-not $check_mode) {
-                $FirstRun = $true;
+            $result.output = Invoke-Expression $command | Where { $_ }
-                $HashProps = @{};
+            $rc = $LASTEXITCODE
-            } -Process {
+            $result.output | Where {$_ -match '^([^:]+):\s*(\S.*)$'} | Foreach -Begin {
-                if (($Matches[1] -eq 'Rule Name') -and (!($FirstRun))) {
+                    $FirstRun = $true
-                    $output=$HashProps;
+                    $HashProps = @{}
-                    $HashProps = @{};
+                } -Process {
-                };
+                    if (($Matches[1] -eq 'Rule Name') -and (-not $FirstRun)) {
-                $HashProps.$($Matches[1]) = $Matches[2];
+                        $result.output = $HashProps
-                $FirstRun = $false;
+                        $HashProps = @{}
-            } -End {
+                    }
-                $output=$HashProps;
+                    $HashProps.$($Matches[1]) = $Matches[2]
-            };
+                    $FirstRun = $false
-        $msg+=@("Removed the rule")
+                } -End {
-        $result=@{
+                    $result.output = $HashProps
-            failed=$false
+                }
-            changed=$true
-            msg=$msg
-            output=$output
-        };
-    } catch [Exception]{
-        $msg+=@("Failed to remove the rule")
-        $result=@{
-            failed=$true
-            error=$_.Exception.Message
-            msg=$msg
         }
-    };
+        if ($rc -eq 0 -or $rc -eq 1) {
+            if ($diff_support) {
+                $result.diff = @{}
+                $result.diff.after = ""
+                $result.diff.before = ConvertTo-SortedKV $fwsettings
+            }
+            $result.changed = $true
+            $result.msg += @("Removed the rule '$name'")
+        } else {
+            $result.failed = $true
+            $result.msg += @("Remove command '$command' failed with rc=$rc")
+        }
+    } catch [Exception]{
+        $result.error = $_.Exception.Message
+        $result.failed = $true
+        $result.msg += @("Failed to remove the rule '$name'")
+    }
     return $result
 }
 
-# Mount Drives
+# FIXME: Unsupported keys
-$change=$false;
+#$unsupported = @("Grouping", "Rule source")
-$fail=$false;
+$unsupported = @("Rule source")
-$msg=@();
-$fwsettings=@{}
 
-# Variabelise the arguments
+$result = @{
-$params=Parse-Args $args;
+    changed = $false
+    fwsettings = @{}
+    msg = @()
+}
+
+$params = Parse-Args $args -supports_check_mode $true
+$check_mode = Get-AnsibleParam -obj $params -name "_ansible_check_mode" -type "bool" -default $false
+$diff_support = Get-AnsibleParam -obj $params -name "_ansible_diff" -type "bool" -default $false
 
 $name = Get-AnsibleParam -obj $params -name "name" -failifempty $true
-$direction = Get-AnsibleParam -obj $params -name "direction" -failifempty $true -validateSet "in","out"
+$description = Get-AnsibleParam -obj $params -name "description" -type "str"
-$action = Get-AnsibleParam -obj $params -name "action" -failifempty $true -validateSet "allow","block","bypass"
+$direction = Get-AnsibleParam -obj $params -name "direction" -type "str" -failifempty $true -validateset "in","out"
-$program = Get-AnsibleParam -obj $params -name "program"
+$action = Get-AnsibleParam -obj $params -name "action" -type "str" -failifempty $true -validateset "allow","block","bypass"
-$service = Get-AnsibleParam -obj $params -name "service" -default "any"
+$program = Get-AnsibleParam -obj $params -name "program" -type "str"
-$description = Get-AnsibleParam -obj $params -name "description"
+$service = Get-AnsibleParam -obj $params -name "service" -type "str"
-$enable = ConvertTo-Bool (Get-AnsibleParam -obj $params -name "enable" -default "true")
+$enabled = Get-AnsibleParam -obj $params -name "enabled" -type "bool" -default $true -aliases "enable"
-$winprofile = Get-AnsibleParam -obj $params -name "profile" -default "any"
+$profiles = Get-AnsibleParam -obj $params -name "profiles" -type "str" -default "domain,private,public" -aliases "profile"
-$localip = Get-AnsibleParam -obj $params -name "localip" -default "any"
+$localip = Get-AnsibleParam -obj $params -name "localip" -type "str" -default "any"
-$remoteip = Get-AnsibleParam -obj $params -name "remoteip" -default "any"
+$remoteip = Get-AnsibleParam -obj $params -name "remoteip" -type "str" -default "any"
-$localport = Get-AnsibleParam -obj $params -name "localport" -default "any"
+$localport = Get-AnsibleParam -obj $params -name "localport" -type "str"
-$remoteport = Get-AnsibleParam -obj $params -name "remoteport" -default "any"
+$remoteport = Get-AnsibleParam -obj $params -name "remoteport" -type "str"
-$protocol = Get-AnsibleParam -obj $params -name "protocol" -default "any"
+$protocol = Get-AnsibleParam -obj $params -name "protocol" -type "str" -default "any"
+$edge = Get-AnsibleParam -obj $params -name "edge" -type "str" -default "no" -validateset "no","yes","deferapp","deferuser"
+$interfacetypes = Get-AnsibleParam -obj $params -name "interfacetypes" -type "str" -default "any"
+$security = Get-AnsibleParam -obj $params -name "security" -type "str" -default "notrequired"
 
-$state = Get-AnsibleParam -obj $params -name "state" -failifempty $true -validateSet "present","absent"
+$state = Get-AnsibleParam -obj $params -name "state" -type "str" -default "present" -validateset "present","absent"
-$force = ConvertTo-Bool (Get-AnsibleParam -obj $params -name "force" -default "false")
+$force = Get-AnsibleParam -obj $params -name "force" -type "bool" -default $false
 
 # Check the arguments
-If ($enable -eq $true) {
+if ($enabled) {
-    $fwsettings.Add("Enabled", "yes");
+    $result.fwsettings.Add("Enabled", "Yes")
-} Else {
-    $fwsettings.Add("Enabled", "no");
-};
-
-$fwsettings.Add("Rule Name", $name)
-#$fwsettings.Add("displayname", $name)
-
-$state = $state.ToString().ToLower()
-If ($state -eq "present"){
-    $fwsettings.Add("Direction", $direction)
-    $fwsettings.Add("Action", $action)
-};
-
-If ($description) {
-    $fwsettings.Add("Description", $description);
-}
-
-If ($program) {
-    $fwsettings.Add("Program", $program);
-}
-
-$fwsettings.Add("LocalIP", $localip);
-$fwsettings.Add("RemoteIP", $remoteip);
-$fwsettings.Add("LocalPort", $localport);
-$fwsettings.Add("RemotePort", $remoteport);
-$fwsettings.Add("Service", $service);
-$fwsettings.Add("Protocol", $protocol);
-$fwsettings.Add("Profiles", $winprofile)
-
-$output=@()
-$capture=getFirewallRule ($fwsettings);
-if ($capture.failed -eq $true) {
-    $msg+=$capture.msg;
-    $result=New-Object psobject @{
-        changed=$false
-        failed=$true
-        error=$capture.error
-        msg=$msg
-    };
-    Exit-Json $result;
 } else {
-    $diff=$capture.difference
+    $result.fwsettings.Add("Enabled", "No")
-    $msg+=$capture.msg;
-    $identical=$capture.identical;
-    $multiple=$capture.multiple;
 }
 
+$result.fwsettings.Add("Rule Name", $name)
+#$result.fwsettings.Add("displayname", $name)
 
-switch ($state){
+if ($state -eq "present") {
-    "present" {
+    $result.fwsettings.Add("Direction", $(ConvertTo-TitleCase($direction)))
-        if ($capture.exists -eq $false) {
+    $result.fwsettings.Add("Action", $(ConvertTo-TitleCase $action))
-            $capture=createFireWallRule($fwsettings);
+}
-            $msg+=$capture.msg;
-            $change=$true;
-            if ($capture.failed -eq $true){
-                $result=New-Object psobject @{
-                    failed=$capture.failed
-                    error=$capture.error
-                    output=$capture.output
-                    changed=$change
-                    msg=$msg
-                    difference=$diff
-                    fwsettings=$fwsettings
-                };
-                Exit-Json $result;
-            }
-        } elseif ($capture.identical -eq $false) {
-            if ($force -eq $true) {
-                $capture=removeFirewallRule($fwsettings);
-                $msg+=$capture.msg;
-                $change=$true;
-                if ($capture.failed -eq $true){
-                    $result=New-Object psobject @{
-                        failed=$capture.failed
-                        error=$capture.error
-                        changed=$change
-                        msg=$msg
-                        output=$capture.output
-                        fwsettings=$fwsettings
-                    };
-                    Exit-Json $result;
-                }
-                $capture=createFireWallRule($fwsettings);
-                $msg+=$capture.msg;
-                $change=$true;
-                if ($capture.failed -eq $true){
-                    $result=New-Object psobject @{
-                        failed=$capture.failed
-                        error=$capture.error
-                        changed=$change
-                        msg=$msg
-                        difference=$diff
-                        fwsettings=$fwsettings
-                    };
-                    Exit-Json $result;
-                }
 
-            } else {
+if ($description -ne $null) {
-                $fail=$true
+    $result.fwsettings.Add("Description", $description)
-                $msg+=@("There was already a rule $name with different values, use force=True to overwrite it");
+}
+
+if ($program -ne $null) {
+    $result.fwsettings.Add("Program", $program)
+}
+
+$result.fwsettings.Add("LocalIP", $localip)
+$result.fwsettings.Add("RemoteIP", $remoteip)
+
+if ($localport -ne $null) {
+    $result.fwsettings.Add("LocalPort", $localport)
+}
+
+if ($remoteport -ne $null) {
+    $result.fwsettings.Add("RemotePort", $remoteport)
+}
+
+if ($service -ne $null) {
+    $result.fwsettings.Add("Service", $(ConvertTo-TitleCase($service)))
+}
+
+if ($protocol -eq "Any") {
+    $result.fwsettings.Add("Protocol", $protocol)
+} else {
+    $result.fwsettings.Add("Protocol", $protocol.toupper())
+}
+
+if ($profiles -eq "Any") {
+    $result.fwsettings.Add("Profiles", "Domain,Private,Public")
+} else {
+    $result.fwsettings.Add("Profiles", $(ConvertTo-TitleCase($profiles)))
+}
+
+$result.fwsettings.Add("Edge traversal", $(ConvertTo-TitleCase($edge)))
+
+if ($interfacetypes -ne $null) {
+    $result.fwsettings.Add("InterfaceTypes", $(ConvertTo-TitleCase($interfacetypes)))
+}
+
+switch($security) {
+    "Authenticate" { $security = "Authenticate" }
+    "AuthDynEnc" { $security = "AuthDynEnc" }
+    "AuthEnc" { $security = "AuthEnc" }
+    "AuthNoEncap" { $security = "AuthNoEncap" }
+    "NotRequired" { $security = "NotRequired" }
+}
+$result.fwsettings.Add("Security", $security)
+
+# FIXME: Define unsupported options
+#$result.fwsettings.Add("Grouping", "")
+#$result.fwsettings.Add("Rule source", "Local Setting")
+
+$get = getFirewallRule($result.fwsettings)
+$result.msg += $get.msg
+
+if ($get.failed) {
+    $result.error = $get.error
+    $result.output = $get.output
+    Fail-Json $result $result.msg
+}
+
+$result.diff = $get.diff
+
+if ($state -eq "present") {
+    if (-not $get.exists) {
+
+        $create = createFireWallRule($result.fwsettings)
+        $result.msg += $create.msg
+        $result.diff = $create.diff
+
+        if ($create.failed) {
+            $result.error = $create.error
+            $result.output = $create.output
+            Fail-Json $result $result.msg
+        }
+
+        $result.changed = $true
+
+    } elseif (-not $get.identical) {
+        # FIXME: This ought to use netsh advfirewall firewall set instead !
+        if ($force) {
+
+            $remove = removeFirewallRule($result.fwsettings)
+            # NOTE: We retain the diff output from $get.diff here
+            $result.msg += $remove.msg
+
+            if ($remove.failed) {
+                $result.error = $remove.error
+                $result.output = $remove.output
+                Fail-Json $result $result.msg
             }
-        } elseif ($capture.identical -eq $true) {
+
-            $msg+=@("Firewall rule $name was already created");
+            $create = createFireWallRule($result.fwsettings)
-        };
+            # NOTE: We retain the diff output from $get.diff here
-    }
+            $result.msg += $create.msg
-    "absent" {
+
-        if ($capture.exists -eq $true) {
+            if ($create.failed) {
-            $capture=removeFirewallRule($fwsettings);
+                $result.error = $create.error
-            $msg+=$capture.msg;
+                $result.output = $create.output
-            $change=$true;
+                Fail-Json $result $result.msg
-            if ($capture.failed -eq $true){
-                $result=New-Object psobject @{
-                    failed=$capture.failed
-                    error=$capture.error
-                    changed=$change
-                    msg=$msg
-                    output=$capture.output
-                    fwsettings=$fwsettings
-                };
-                Exit-Json $result;
             }
+
+            $result.changed = $true
+
         } else {
-            $msg+=@("Firewall rule $name did not exist");
+
-        };
+            $result.msg += @("There was already a rule '$name' with different values, use the 'force' parameter to overwrite it")
+            Fail-Json $result $result.msg
+
+        }
+    } else {
+
+        $result.msg += @("Firewall rule '$name' was already created")
+
     }
-};
 
+} elseif ($state -eq "absent") {
 
-$result=New-Object psobject @{
+    if ($get.exists) {
-    failed=$fail
-    changed=$change
-    msg=$msg
-    difference=$diff
-    fwsettings=$fwsettings
-};
 
+        $remove = removeFirewallRule($result.fwsettings)
+        $result.diff = $remove.diff
+        $result.msg += $remove.msg
 
-Exit-Json $result;
+        if ($remove.failed) {
+            $result.error = $remove.error
+            $result.output = $remove.output
+            Fail-Json $result $result.msg
+        }
+
+        $result.changed = $true
+
+    } else {
+
+        $result.msg += @("Firewall rule '$name' did not exist")
+
+    }
+}
+
+Exit-Json $result

lib/ansible/modules/windows/win_firewall_rule.py

@@ -29,100 +29,98 @@ version_added: "2.0"
 author: Timothy Vandenbrande
 short_description: Windows firewall automation
 description:
-    - allows you to create/remove/update firewall rules
+    - Allows you to create/remove/update firewall rules
 options:
-    enable:
+    enabled:
         description:
-            - is this firewall rule enabled or disabled
+            - Is this firewall rule enabled or disabled
-        default: true
+        default: 'yes'
-        required: false
+        choices: [ 'no', 'yes' ]
+        aliases: [ 'enable' ]
     state:
         description:
-            - should this rule be added or removed
+            - Should this rule be added or removed
         default: "present"
-        required: true
         choices: ['present', 'absent']
     name:
         description:
-            - the rules name
+            - The rules name
-        default: null
         required: true
     direction:
         description:
-            - is this rule for inbound or outbound traffic
+            - Is this rule for inbound or outbound traffic
-        default: null
         required: true
         choices: ['in', 'out']
     action:
         description:
-            - what to do with the items this rule is for
+            - What to do with the items this rule is for
-        default: null
         required: true
         choices: ['allow', 'block', 'bypass']
     description:
         description:
-            - description for the firewall rule
+            - Description for the firewall rule
-        default: null
-        required: false
     localip:
         description:
-            - the local ip address this rule applies to
+            - The local ip address this rule applies to
         default: 'any'
-        required: false
     remoteip:
         description:
-            - the remote ip address/range this rule applies to
+            - The remote ip address/range this rule applies to
         default: 'any'
-        required: false
     localport:
         description:
-            - the local port this rule applies to
+            - The local port this rule applies to
-        default: 'any'
-        required: false
     remoteport:
         description:
-            - the remote port this rule applies to
+            - The remote port this rule applies to
-        default: 'any'
-        required: false
     program:
         description:
-            - the program this rule applies to
+            - The program this rule applies to
-        default: null
-        required: false
     service:
         description:
-            - the service this rule applies to
+            - The service this rule applies to
-        default: 'any'
-        required: false
     protocol:
         description:
-            - the protocol this rule applies to
+            - The protocol this rule applies to
         default: 'any'
-        required: false
+    profiles:
-    profile:
         description:
-            - the profile this rule applies to, e.g. Domain,Private,Public
+            - The profile this rule applies to
-        default: 'any'
+        default: 'domain,private,public'
-        required: false
+        aliases: [ 'profile' ]
     force:
         description:
-            - Enforces the change if a rule with different values exists
+            - Replace any existing rule by removing it first.
-        default: false
+        default: 'no'
-        required: false
+        choices: [ 'no', 'yes' ]
-
+notes:
-
+- The implementation uses C(netsh advfirewall) underneath, a pure-Powershell
+  reimplementation would be more powerful.
+- Modifying existing firewall rules is not possible, the module does allow
+  replacing complete rules based on name, but that works by removing the
+  existing rule completely, and recreating it with provided information
+  (when using C(force)).
 '''
 
 EXAMPLES = r'''
-- name: Firewall rule to allow smtp on TCP port 25
+- name: Firewall rule to allow SMTP on TCP port 25
-  action: win_firewall_rule
+  win_firewall_rule:
-  args:
+    name: SMTP
-      name: smtp
+    localport: 25
-      enable: yes
+    action: allow
-      state: present
+    direction: in
-      localport: 25
+    protocol: tcp
-      action: allow
+    state: present
-      direction: In
+    enabled: yes
-      protocol: TCP
 
+- name: Firewall rule to allow RDP on TCP port 3389
+  win_firewall_rule:
+    name: Remote Desktop
+    localport: 3389
+    action: allow
+    direction: in
+    protocol: tcp
+    profiles: private
+    state: present
+    enabled: yes
 '''

test/integration/targets/win_firewall_rule/tasks/main.yml

@@ -3,7 +3,7 @@
     name: http
     state: absent
     action: "{{ item }}"
-    direction: In
+    direction: in
   with_items:
      - allow
      - block
@@ -11,90 +11,86 @@
 - name: Add firewall rule
   win_firewall_rule:
     name: http
-    enable: yes
+    enabled: yes
     state: present
     localport: 80
     action: allow
-    direction: In
+    direction: in
-    protocol: TCP
+    protocol: tcp
   register: add_firewall_rule
 
 - name: Check that creating new firewall rule succeeds with a change
   assert:
     that:
-    - add_firewall_rule.failed == false
     - add_firewall_rule.changed == true
 
 - name: Add same firewall rule (again)
   win_firewall_rule:
     name: http
-    enable: yes
+    enabled: yes
     state: present
     localport: 80
     action: allow
-    direction: In
+    direction: in
-    protocol: TCP
+    protocol: tcp
   register: add_firewall_rule_again
 
 - name: Check that creating same firewall rule succeeds without a change
   assert:
     that:
-    - add_firewall_rule_again.failed == false
     - add_firewall_rule_again.changed == false
 
 - name: Remove firewall rule
   win_firewall_rule:
     name: http
-    enable: yes
+    enabled: yes
     state: absent
     localport: 80
     action: allow
-    direction: In
+    direction: in
-    protocol: TCP
+    protocol: tcp
   register: remove_firewall_rule
 
 - name: Check that removing existing firewall rule succeeds with a change
   assert:
     that:
-    - remove_firewall_rule.failed == false
     - remove_firewall_rule.changed == true
 
 - name: Remove absent firewall rule
   win_firewall_rule:
     name: http
-    enable: yes
+    enabled: yes
     state: absent
     localport: 80
     action: allow
-    direction: In
+    direction: in
-    protocol: TCP
+    protocol: tcp
   register: remove_absent_firewall_rule
 
 - name: Check that removing non existing firewall rule succeeds without a change
   assert:
     that:
-    - remove_absent_firewall_rule.failed == false
     - remove_absent_firewall_rule.changed == false
 
 - name: Add firewall rule
   win_firewall_rule:
     name: http
-    enable: yes
+    enabled: yes
     state: present
     localport: 80
     action: allow
-    direction: In
+    direction: in
-    protocol: TCP
+    protocol: tcp
 
 - name: Add different firewall rule
   win_firewall_rule:
     name: http
-    enable: yes
+    enabled: yes
     state: present
     localport: 80
     action: block
-    direction: In
+    direction: in
-    protocol: TCP
+    protocol: tcp
   ignore_errors: yes
   register: add_different_firewall_rule_without_force
 
@@ -103,143 +99,136 @@
     that:
     - add_different_firewall_rule_without_force.failed == true
     - add_different_firewall_rule_without_force.changed == false
-    - add_different_firewall_rule_without_force.difference == ["block"]
 
 - name: Add different firewall rule with force setting
   win_firewall_rule:
     name: http
-    enable: yes
+    enabled: yes
     state: present
     localport: 80
     action: block
-    direction: In
+    direction: in
-    protocol: TCP
+    protocol: tcp
     force: yes
   register: add_different_firewall_rule_with_force
 
 - name: Check that creating different firewall rule with enabling force setting succeeds
   assert:
     that:
-    - add_different_firewall_rule_with_force.failed == false
     - add_different_firewall_rule_with_force.changed == true
-    - add_different_firewall_rule_with_force.difference == ["block"]
 
 - name: Add firewall rule when remoteip is range
   win_firewall_rule:
     name: http
-    enable: yes
+    enabled: yes
     state: present
     localport: 80
     remoteip: 192.168.0.1-192.168.0.5
     action: allow
-    direction: In
+    direction: in
-    protocol: TCP
+    protocol: tcp
     force: yes
 
 - name: Add same firewall rule when remoteip is range (again)
   win_firewall_rule:
     name: http
-    enable: yes
+    enabled: yes
     state: present
     localport: 80
     remoteip: 192.168.0.1-192.168.0.5
     action: allow
-    direction: In
+    direction: in
-    protocol: TCP
+    protocol: tcp
   register: add_firewall_rule_with_range_remoteip_again
 
 - name: Check that creating same firewall rule when remoteip is range succeeds without a change
   assert:
     that:
-    - add_firewall_rule_with_range_remoteip_again.failed == false
     - add_firewall_rule_with_range_remoteip_again.changed == false
 
 - name: Add firewall rule when remoteip in CIDR notation
   win_firewall_rule:
     name: http
-    enable: yes
+    enabled: yes
     state: present
     localport: 80
     remoteip: 192.168.0.0/24
     action: allow
-    direction: In
+    direction: in
-    protocol: TCP
+    protocol: tcp
     force: yes
 
 - name: Add same firewall rule when remoteip in CIDR notation (again)
   win_firewall_rule:
     name: http
-    enable: yes
+    enabled: yes
     state: present
     localport: 80
     remoteip: 192.168.0.0/24
     action: allow
-    direction: In
+    direction: in
-    protocol: TCP
+    protocol: tcp
   register: add_firewall_rule_with_cidr_remoteip_again
 
 - name: Check that creating same firewall rule succeeds without a change when remoteip in CIDR notation
   assert:
     that:
-    - add_firewall_rule_with_cidr_remoteip_again.failed == false
     - add_firewall_rule_with_cidr_remoteip_again.changed == false
 
 - name: Add firewall rule when remoteip contains a netmask
   win_firewall_rule:
     name: http
-    enable: yes
+    enabled: yes
     state: present
     localport: 80
     remoteip: 192.168.0.0/255.255.255.0
     action: allow
-    direction: In
+    direction: in
-    protocol: TCP
+    protocol: tcp
     force: yes
 
 - name: Add same firewall rule when remoteip contains a netmask (again)
   win_firewall_rule:
     name: http
-    enable: yes
+    enabled: yes
     state: present
     localport: 80
     remoteip: 192.168.0.0/255.255.255.0
     action: allow
-    direction: In
+    direction: in
-    protocol: TCP
+    protocol: tcp
   register: add_firewall_rule_remoteip_contains_netmask_again
 
 - name: Check that creating same firewall rule succeeds without a change when remoteip contains a netmask
   assert:
     that:
-    - add_firewall_rule_remoteip_contains_netmask_again.failed == false
     - add_firewall_rule_remoteip_contains_netmask_again.changed == false
 
 - name: Add firewall rule when remoteip is IPv4
   win_firewall_rule:
     name: http
-    enable: yes
+    enabled: yes
     state: present
     localport: 80
     remoteip: 192.168.0.1
     action: allow
-    direction: In
+    direction: in
-    protocol: TCP
+    protocol: tcp
     force: yes
 
 - name: Add same firewall rule when remoteip is IPv4 (again)
   win_firewall_rule:
     name: http
-    enable: yes
+    enabled: yes
     state: present
     localport: 80
     remoteip: 192.168.0.1
     action: allow
-    direction: In
+    direction: in
-    protocol: TCP
+    protocol: tcp
   register: add_firewall_rule_with_ipv4_remoteip_again
 
 - name: Check that creating same firewall rule when remoteip is IPv4 succeeds without a change
   assert:
     that:
-    - add_firewall_rule_with_ipv4_remoteip_again.failed == false
     - add_firewall_rule_with_ipv4_remoteip_again.changed == false