xFirewall: Modify Set-TargetResource to support both Group/DisplayGroup

[tysonjhayes]

Right now there is a limitation in the code where xFirewall doesn't support setting both Group and DisplayGroup, the code pretty much doesn't let us set Group. This should be changed to allow both.

[PlagueHO]

👍

I could have a go at this change this week if you like? I've have some ideas on the changes that need to be made.

The fiddliest part is working around the issue where a DisplayGroup can't be renamed (https://connect.microsoft.com/PowerShell/feedbackdetail/view/1970765/add-ability-to-change-firewall-displaygroup-in-set-netfirewallrule-cmdlet) or where the Group needs to be renamed on an existing rule - because the only way to manage these two scenarios is to recreate the rule. But I think this can be managed.

[PlagueHO]

Also, I added another request to Windows Connect to allow the Group to be changed in the Set-NetFirewallRule cmdlet:

https://connect.microsoft.com/PowerShell/feedbackdetail/view/2000226/add-ability-to-change-firewall-group-in-set-netfirewallrule-cmdlet

[tysonjhayes]

One of these days we'll get everything open sourced so we can submit pull requests. 👍

[PlagueHO]

Now that would be awesome :) I'll happen one day I'm sure. Still, Connect is a pretty good way to start.

[PlagueHO]

I've done some digging on this one and actually think it requires a slightly different solution that I originally thought. This does still require a change, so I'll summarize the situation here so that it is (hopefully) clear why this resource behaves this way and hopefully clarifies things.

My original thinking was there is no possible way to set the DisplayGroup on an existing or new Firewall rule. It will always be set to the value of the Group parameter that is passed in the New-NetFirewallRule cmdlet and once set can not be changed by any other cmdlet. It can't even be changed using NETSH or the WFAS UI!!

But after researching this I don't think the DisplayGroup is actually never meant to be set. We should ONLY ever allow the Group to be set. The DisplayGroup will always take on one of two values:

1. The same value as the Group.
2. If the group is starts with @ (this means it is a predefined rule) then the display group will be set from the DLL that is specified in the group. For example:
   
   

Another example:
If you run this:
new-netfirewallrule -Name Predefined -DisplayName 'Rule Test' -Group '@%windir%\system32\ine tsrv\iisres.dll,-30503'

You will end up with the DisplayGroup of the rule being pulled from the IISRES.DLL - this is called a predefined rule (and you can't change the properties of this rule in WFAS):

So I think this also explains why the Group can never be changed once assigned - because it could cause some pretty unexpected behavior when changing a non-predefined rule to a predefined rule.

tl;dr;
The solution to all this is:

1. All references to DisplayGroup should be changed to Group because DisplayGroup should never actually be able to be set directly.
2. DisplayGroup should be converted to a READ DSC parameter.
3. Group should be added as a WRITE DSC parameter.
4. If Group is changed on an existing rule the rule will be recreated automatically.

So I'll make the above changes and get those through.

Also, this makes my submissions to Windows Connect invalid as the Cmdlets are actually behaving exactly as they should. Although the documentation on this could be improved :)

[tysonjhayes]

I'm going to call this one good with some of the merges that have been happening.

[PlagueHO]

👍 No argument from me.

[TravisEz13]

Note this was a breaking change.
Not asking for it to be changed back.

[tysonjhayes]

I'm curious why you're noting this now?

[TravisEz13]

I'm bringing configuration forward to use the latest resources and trying to figure out why the configuration broke. I don't see any notes in the review that this was a breaking change.

[tysonjhayes]

Makes sense, I'll try to make sure we're calling that out in the release notes if we ever need to make another breaking change. 👍

[PlagueHO]

Fair enough. It definitely could have been a bit clearer in the release notes 👍