xDSCWebservice: FIX Firewall Exception

[tmeckel]

Pull Request (PR) description

This PR contains a fix for issue #536 where a (unwanted) firewall exception is automatically created during the deployment of a Pull Server.

This Pull Request (PR) fixes the following issues

• Fixes xDSCWebservice Firewall Exception  #536

Task list

This PR is currently marked as draft because work isn't finished yet. The following task list will be updated.

• Added an entry under the Unreleased section of the change log in
  CHANGELOG.md. Entry should say what was changed, and how that affects
  users (if applicable).
• Resource documentation added/updated in README.md.
• Resource parameter descriptions added/updated in README.md, schema.mof
  and comment-based help.
• Comment-based help added/updated.
• Localization strings added/updated in all localization files as
  appropriate.
• Examples appropriately added/updated.
• Unit tests added/updated. See
  DSC Resource Testing Guidelines.
• Integration tests added/updated (where possible). See
  DSC Resource Testing Guidelines.
• New/changed code adheres to
  DSC Resource Style Guidelines and Best Practices

---

This change is 

[tmeckel]

@mhendric can you give a hint why this meta test is failing??? I already rebased my branch to origin/dev. The error still persists.

https://ci.appveyor.com/project/PowerShell/xpsdesiredstateconfiguration/builds/23660190/job/xng6p5sns1k7c627/tests

at <ScriptBlock>, C:\projects\xpsdesiredstateconfiguration\DscResource.Tests\Meta.Tests.ps1: line 499
499:                         $requiredPssaRulesOutput | Should -Be $null

Strangely enough the test is passed with my other PR #613

https://ci.appveyor.com/project/PowerShell/xpsdesiredstateconfiguration/builds/23660192/job/u5lxm5awv412o4hn/tests

[mhendric]

It looks like it failed because the new function you added, Test-PullServerFirewallConfiguration, returns a type of Boolean, but does not have an OutputType specified in the param header.

    Context Firewall.psm1
1232
      [+] Should pass all error-level PS Script Analyzer rules 47ms
1233
Required PSSA rule(s) did not pass.
1234
The following PSScriptAnalyzer rule 'PSUseOutputTypeCorrectly' errors need to be fixed:
1235
Firewall.psm1 (Line 97): The cmdlet 'Test-PullServerFirewallConfiguration' returns an object of type 'System.Boolean' but this type is not declared in the OutputType attribute.
1236
For instructions on how to run PSScriptAnalyzer on your own machine, please go to https://github.com/powershell/PSScriptAnalyzer
1237
      [-] Should pass all required PS Script Analyzer rules 1.99s
1238
        Expected $null, but got Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.DiagnosticRecord.
1239
        499:                         $requiredPssaRulesOutput | Should -Be $null
1240
        at <ScriptBlock>, C:\projects\xpsdesiredstateconfiguration\DscResource.Tests\Meta.Tests.ps1: line 499
1241
      [+] Should pass all flagged PS Script Analyzer rules 158ms
1242
      [+] Should pass any recently-added, error-level PS Script Analyzer rules 58ms
1243
      [+] Should not suppress any required PS Script Analyzer rules 10ms

[tmeckel]

@mhendric thanks for the quick answer! I added the OutputType declaration.

One additional question: I execute the defined Unit tests for xDSCWebService like so:

Invoke-Pester .\Tests\Unit\MSFT_xDSCWebService.Tests.ps1

How do I execute the meta test, so that I won't push PRs that don't adhere to the defined coding rules in the future?

[mhendric]

@mhendric thanks for the quick answer! I added the OutputType declaration.
One additional question: I execute the defined Unit tests for xDSCWebService like so:
Invoke-Pester .\Tests\Unit\MSFT_xDSCWebService.Tests.ps1
How do I execute the meta test, so that I won't push PRs that don't adhere to the defined coding rules in the future?

I think you just call DscResource.Tests\Meta.Tests.ps1.

[codecov-io]

Codecov Report

Merging #614 into dev will decrease coverage by <1%.
The diff coverage is 40%.

@@         Coverage Diff         @@
##            dev   #614   +/-   ##
===================================
- Coverage    73%    73%   -1%     
===================================
  Files        27     28    +1     
  Lines      4132   4164   +32     
  Branches      4      5    +1     
===================================
+ Hits       3043   3066   +23     
- Misses     1085   1093    +8     
- Partials      4      5    +1

[tmeckel]

@mhendric @PlagueHO @johlju the PR is now ready. Would be nice if one of you guys would do a review.

[mhendric]

Hi @tmeckel , I can review this one for you. However, can you try to get a clean build through first? I'd like to see if the Unit and Integration tests are passing too. Looks like this is failing on Meta tests again (mostly due to lines longer than 80 chars, or trailing whitespace in README.MD).

1879
Context When there are markdown files
1880
VERBOSE: Using markdownlint settings file from repository folder 'C:\projects\xpsdesiredstateconfiguration'.
1881
C:\projects\xpsdesiredstateconfiguration\CHANGELOG.md: 19: MD013/line-length Line length [Expected: 80; Actual: 130]
1882
C:\projects\xpsdesiredstateconfiguration\README.md: 195: MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
1883
C:\projects\xpsdesiredstateconfiguration\README.md: 199: MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
1884
C:\projects\xpsdesiredstateconfiguration\README.md: 191: MD013/line-length Line length [Expected: 80; Actual: 183]
1885
C:\projects\xpsdesiredstateconfiguration\README.md: 195: MD013/line-length Line length [Expected: 80; Actual: 185]
1886
C:\projects\xpsdesiredstateconfiguration\README.md: 197: MD013/line-length Line length [Expected: 80; Actual: 332]
1887
C:\projects\xpsdesiredstateconfiguration\README.md: 199: MD013/line-length Line length [Expected: 80; Actual: 273]
1888
[-] Should not have errors in any markdown files 5.18s
1889
Expected 0, but got 7.
1890
976: $mdErrors | Should -Be 0
1891
at , C:\projects\xpsdesiredstateconfiguration\DscResource.Tests\Meta.Tests.ps1: line 976
1892
1893
Context When uninstalling markdown validation dependencies
1894
[+] Should not throw an error when uninstalling package gulp-concat as a dev-dependency 7.07s

[mhendric]

Reviewed 8 of 14 files at r1.
Reviewable status: 8 of 14 files reviewed, 17 unresolved discussions (waiting on @tmeckel)

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 2 at r1 (raw file):

$script:FireWallRuleDisplayName 

Variable name should use camel case: https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#variable-names-use-camel-case

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 8 at r1 (raw file):

.PARAMETER firewallPort

Parameter name doesn't match actual parameter. Throughout file.

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 22 at r1 (raw file):

$script:netsh = "$env:windir\system32\netsh.exe"

Can this be moved outside of a Function, to the script itself, so you don't have to keep re-declaring it?

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 27 at r1 (raw file):

# remove all existing rules with that displayName

First comment character should be capitalized

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 31 at r1 (raw file):

& $script:netsh advfirewall firewall add rule name=DSCPullServer_IIS_Port dir=in action=allow protocol=TCP localport=$Port | Out-Null

@PlagueHO may ask you to assign the results of this to a $null variable rather than piping to | Out-Null. I personally don't care though :).

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 65 at r1 (raw file):

Where-Object DisplayName -eq "$ruleName" 

Need a FilterScript and ScriptBlock in here.

---

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 2 at r1 (raw file):

Import-Module -Name (Join-Path $PSScriptRoot 'PSWSIISEndpoint.psm1') -Verbose:$false
Import-Module -Name (Join-Path $PSScriptRoot 'UseSecurityBestPractices.psm1') -Verbose:$false
Import-Module -NAme (Join-Path $PSScriptRoot 'Firewall.psm1') -Verbose:$false

Need Named Parameters on these Join-Path's (Path, ChildPath). Also, -NAme in the last one has bad casing.

---

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 378 at r1 (raw file):

 | ForEach-Object {

Need a Named Parameter here (-Process?)

---

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 386 at r1 (raw file):

ForEach-Object { Remove-PullServerFirewallConfiguration -Port $_ }

Needs Named Parameter

---

Tests/Integration/MSFT_xDSCWebService.Integration.tests.ps1, line 14 at r1 (raw file):

Import-Module -Name WebAdministration -ErrorAction:Stop -Force

What do you think about testing for the Windows Feature that would install this module, and installing it if it's missing? You could use the Install-WindowsFeatureAndVerify test helper function.

---

Tests/Integration/MSFT_xDSCWebService.Integration.tests.ps1, line 121 at r1 (raw file):

("Should exist a WebSite called $WebsiteName")

I don't think any of these It labels need to be in parenthesis...

---

Tests/Integration/MSFT_xDSCWebService.Integration.tests.ps1, line 128 at r1 (raw file):

$website.state 

Should .state be capitalized?

---

Tests/Integration/MSFT_xDSCWebService.Integration.tests.ps1, line 168 at r1 (raw file):

 $cnt 

Can you make this a little more descriptive? Like $expectedRuleCount or something?

---

Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 467 at r1 (raw file):

            function Stop-Website {}

Looks like you have one too many line breaks here.

---

Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 1016 at r1 (raw file):

Quoted 4 lines of code…

            function Get-Website {}
            function Get-WebBinding {}

            function Stop-Website {}

Probably don't need a line break in the middle of these.

---

Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 1037 at r1 (raw file):

Quoted 4 lines of code…

            function Get-Website {}
            function Get-WebBinding {}

            function Stop-Website {}

Probably don't need a line break in the middle of these.

---

Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 1089 at r1 (raw file):

Quoted 4 lines of code…

            function Get-Website {}
            function Get-WebBinding {}

            function Stop-Website {}

Probably don't need a line break in the middle of these... throughout file

[tmeckel]

Fixed the meta tests.

Reviewable status: 8 of 14 files reviewed, 17 unresolved discussions (waiting on @mhendric)

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 2 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

$script:FireWallRuleDisplayName 

Variable name should use camel case: https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#variable-names-use-camel-case

Done.

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 8 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

.PARAMETER firewallPort

Parameter name doesn't match actual parameter. Throughout file.

Done.

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 22 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

$script:netsh = "$env:windir\system32\netsh.exe"

Can this be moved outside of a Function, to the script itself, so you don't have to keep re-declaring it?

Moved it to a script global definition:

New-Variable -Name netsh -Value "$env:windir\system32\netsh.exe" -Option ReadOnly -Scope Script -Force

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 27 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

# remove all existing rules with that displayName

First comment character should be capitalized

Done.

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 31 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

& $script:netsh advfirewall firewall add rule name=DSCPullServer_IIS_Port dir=in action=allow protocol=TCP localport=$Port | Out-Null

@PlagueHO may ask you to assign the results of this to a $null variable rather than piping to | Out-Null. I personally don't care though :).

Using the famous $null assignment now :-D

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 65 at r1 (raw file):

DisplayName -eq "$ruleName"
According to the Powershell Cmdlet manual this is perfectly valid because this will spare you to use the dot-notation with the current pipeline object $_.DisplayName

---

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 2 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

Import-Module -Name (Join-Path $PSScriptRoot 'PSWSIISEndpoint.psm1') -Verbose:$false
Import-Module -Name (Join-Path $PSScriptRoot 'UseSecurityBestPractices.psm1') -Verbose:$false
Import-Module -NAme (Join-Path $PSScriptRoot 'Firewall.psm1') -Verbose:$false

Need Named Parameters on these Join-Path's (Path, ChildPath). Also, -NAme in the last one has bad casing.

Done.

---

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 378 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

 | ForEach-Object {

Need a Named Parameter here (-Process?)

Done.

---

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 386 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

ForEach-Object { Remove-PullServerFirewallConfiguration -Port $_ }

Needs Named Parameter

Done.

---

Tests/Integration/MSFT_xDSCWebService.Integration.tests.ps1, line 14 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

Import-Module -Name WebAdministration -ErrorAction:Stop -Force

What do you think about testing for the Windows Feature that would install this module, and installing it if it's missing? You could use the Install-WindowsFeatureAndVerify test helper function.

Great idea. Added the following

if (-not (Install-WindowsFeatureAndVerify -Name Web-Mgmt-Tools))
{
    Write-Error -Message 'Failed to verify for required Windows Feature. Unable to continue ...' -ErrorAction:Stop
}

---

Tests/Integration/MSFT_xDSCWebService.Integration.tests.ps1, line 121 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

("Should exist a WebSite called $WebsiteName")

I don't think any of these It labels need to be in parenthesis...

I adopted this from the Meta.Tests.ps1 and thus I thought it's a good idea :-D

It ('Markdown file ''{0}'' should not have Byte Order Mark (BOM)' -f $filePathOutputName) {

---

Tests/Integration/MSFT_xDSCWebService.Integration.tests.ps1, line 128 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

$website.state 

Should .state be capitalized?

Surprinsingly not. The properties of a website object are lowercase even not camel cased.

---

Tests/Integration/MSFT_xDSCWebService.Integration.tests.ps1, line 168 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

 $cnt 

Can you make this a little more descriptive? Like $expectedRuleCount or something?

Renamed this to $expectedRuleCount

---

Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 467 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

            function Stop-Website {}

Looks like you have one too many line breaks here.

Done.

---

Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 1016 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

            function Get-Website {}
            function Get-WebBinding {}

            function Stop-Website {}

Probably don't need a line break in the middle of these.

Done.

---

Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 1037 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

            function Get-Website {}
            function Get-WebBinding {}

            function Stop-Website {}

Probably don't need a line break in the middle of these.

Done.

---

Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 1089 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

            function Get-Website {}
            function Get-WebBinding {}

            function Stop-Website {}

Probably don't need a line break in the middle of these... throughout file

Done.

[tmeckel]

Reviewable status: 8 of 14 files reviewed, 17 unresolved discussions (waiting on @mhendric)

---

Tests/Integration/MSFT_xDSCWebService.Integration.tests.ps1, line 121 at r1 (raw file):

Previously, tmeckel (Thomas Meckel) wrote…

I adopted this from the Meta.Tests.ps1 and thus I thought it's a good idea :-D

It ('Markdown file ''{0}'' should not have Byte Order Mark (BOM)' -f $filePathOutputName) {

Okay I've to admit in this example the parenthesis are required because they use the -f (Format) operator and not inline string variable expansion. So yeah perhaps you're right that the parenthesis aren't required in my case.

[mhendric]

Hey @tmeckel, sorry for taking so long on this review. It's been a busy last couple weeks.

Reviewable status: 8 of 14 files reviewed, 5 unresolved discussions (waiting on @mhendric and @tmeckel)

---

CHANGELOG.md, line 19 at r2 (raw file):

[issue #536]

Can you make this a full URL like in the entry directly above, and also put it at the end of the entry instead of mid entry?

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 27 at r1 (raw file):

Previously, tmeckel (Thomas Meckel) wrote…

Done.

This doesn't appear to be done.

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 31 at r1 (raw file):

Previously, tmeckel (Thomas Meckel) wrote…

Using the famous $null assignment now :-D

This doesn't appear to be done.

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 65 at r1 (raw file):

Previously, tmeckel (Thomas Meckel) wrote…

DisplayName -eq "$ruleName"
According to the Powershell Cmdlet manual this is perfectly valid because this will spare you to use the dot-notation with the current pipeline object $_.DisplayName

Yeah, it's absolutely valid syntax. I just don't think it aligns with the DSC style guidelines, which suggest using named parameters. What do you think?

https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-parameter-usage-in-function-and-cmdlet-calls

---

Tests/Integration/MSFT_xDSCWebService.Integration.tests.ps1, line 168 at r1 (raw file):

Previously, tmeckel (Thomas Meckel) wrote…

Renamed this to $expectedRuleCount

Thanks. Looks like you have an extra, unnecessary space before the = signs in assignments to this variable though. Can you correct those?

[tmeckel]

No problem ... we all have important things to do aside this project here!

Reviewable status: 7 of 14 files reviewed, 5 unresolved discussions (waiting on @mhendric)

---

CHANGELOG.md, line 19 at r2 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

[issue #536]

Can you make this a full URL like in the entry directly above, and also put it at the end of the entry instead of mid entry?

Done.

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 27 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

This doesn't appear to be done.

Finally :-D

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 31 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

This doesn't appear to be done.

Obviously not ... but now ...

---

DSCResources/MSFT_xDSCWebService/Firewall.psm1, line 65 at r1 (raw file):

Obviously not ... but now ...
I added the required parameters, whereas I've to admit that this is much too verbose for me. But this is my personal taste. If the style guide demands it ... here we go.

---

Tests/Integration/MSFT_xDSCWebService.Integration.tests.ps1, line 168 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

Thanks. Looks like you have an extra, unnecessary space before the = signs in assignments to this variable though. Can you correct those?

Done.

[mhendric]

Reviewed 3 of 6 files at r2, 4 of 4 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

[mhendric]

Hey @tmeckel , this all looks good to me. Just to confirm, are you done working on this? If so I'll get this merged.

[tmeckel]

Hey @tmeckel , this all looks good to me. Just to confirm, are you done working on this? If so I'll get this merged.

Hey @mhendric thanks for the review! Highly appreciated. I'm done working on this, so go ahead with the merge. When you've done with it I can rebase my other PR #613 and prepare it for review as well.