Added CertificateSubject feature to xDSCWebService DSC Resource #379

rcarpenter79 · 2017-12-07T12:13:04Z

Changed CertificateThumbprint from mandatory to use a default value AllowUnencryptedTraffic.
CertificateSubject can take a regex expression.
If multiple certificates are found then return the one with the latest not after date.
If no certificate subject is found set thumbprint to 'AllowUnencryptedTraffic'.

Fixes #57
Fixes #205
Fixes #418

This change is

codecov-io · 2017-12-07T12:46:37Z

Codecov Report

Merging #379 into dev will increase coverage by 6%.
The diff coverage is 98%.

@@         Coverage Diff          @@
##            dev   #379    +/-   ##
====================================
+ Coverage    67%    73%    +6%     
====================================
  Files        27     27            
  Lines      3939   3992    +53     
  Branches      4      4            
====================================
+ Hits       2670   2954   +284     
+ Misses     1265   1034   -231     
  Partials      4      4

mgreenegit · 2018-01-05T11:41:10Z

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 849 at r2 (raw file):

        # Should execution stop if no certificate is found?
        Write-Warning -Message "Certificate not found with subject containing $Subject and using template $TemplateName."
        return 'AllowUnencryptedTraffic'

I think it would be better for the configuration to fail if no cert is found than to set the machine not in the desired state with a warning.

Comments from Reviewable

mgreenegit · 2018-01-05T11:41:17Z

Reviewed 1 of 2 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

mgreenegit · 2018-01-05T11:42:36Z

I was just looking at submitting the same type of changes. It actually used to work this way (back in 2014). Thank you for submitting!!! I submitted feedback.

rcarpenter79 · 2018-01-05T14:20:56Z

Review status: 1 of 3 files reviewed at latest revision, 1 unresolved discussion.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 849 at r2 (raw file):

Previously, mgreenegit (Michael Greene) wrote…

I think it would be better for the configuration to fail if no cert is found than to set the machine not in the desired state with a warning.

I was in two minds about that behavior. I have made the suggested change.

Comments from Reviewable

mgreenegit · 2018-01-05T20:00:19Z

attn @kwirkykat

kwirkykat · 2018-01-24T19:49:48Z

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1

@@ -190,7 +232,11 @@ function Set-TargetResource
        $language = 'en'
    }

-    $os = [System.Environment]::OSVersion.Version
+    #$os = [System.Environment]::OSVersion.Version
+    $os = Get-CimInstance -ClassName Win32_OperatingSystem | Select-Object -Property `


Why are you changing how we retrieve the OS version?

I made this change to mock the OS version for testing.
Is there another way to do this without changing the code?

kwirkykat · 2018-01-24T19:51:15Z

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1

@@ -319,10 +365,7 @@ function Set-TargetResource
    }
    else
    {
-        if($AcceptSelfSignedCertificates -and ($AcceptSelfSignedCertificates -eq $false))


Why was this check removed?
This seems like a breaking change.
Previously we would not run this command if $AcceptSelfSignedCertificates was null or true.

The If block will never be true so the code would never run.
I think I made a mistake here as I intended to comment out the line so it would be brought up to discuss the purpose.
I will change the If block to trigger only when $AcceptSelfSignedCertificates is $false

I had second thoughts on making this change.
Wouldn't fixing the If statement result in a breaking change?
Would it be better to put it back how it was and add a comment for it to be checked if it's still needed?

kwirkykat · 2018-01-24T19:59:07Z

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1


-    $webAdminSrvMgr = [Microsoft.Web.Administration.ServerManager]::OpenRemote("127.0.0.1")


The call to open the remote port is gone?

This was replaced with New-Object -TypeName Microsoft.Web.Administration.ServerManager so it can be mocked.

kwirkykat · 2018-01-24T22:16:55Z

@BerheAbrha or @Indhukrishna Can you take a look at this PR?

BerheAbrha · 2018-01-30T20:52:43Z

User need to explicitly opt-out to allow unencrypted traffic for security reasons. I am not sure if making this optional is a good idea security wise. @TravisEz13 could you please advise on this.

BerheAbrha · 2018-01-30T21:10:42Z

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1

+            $_.Oid.FriendlyName -eq 'Certificate Template Name'
+        }.Format($false) -eq $TemplateName
+    } | Sort-Object -Property 'NotAfter' -Descending | Select-Object -First 1
+


If more than one cert is found which satisfy the criteria. I think It should error out instead of choosing one based on the expiry date. The reason being, the same cert have to be trusted by all clients connecting to this server and we can't tell which cert is trusted by all clients connecting to this PullServer.

Yes, there is a requirement that the user explicitly say that they intended for the endpoint to AllowUnencryptedTraffic

rcarpenter79 · 2018-02-23T18:22:07Z

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 840 at r3 (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

Yes, there is a requirement that the user explicitly say that they intended for the endpoint to AllowUnencryptedTraffic

I have updated the function to only allow one certificate to be returned.

Comments from Reviewable

TravisEz13 · 2018-03-01T20:03:48Z

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.schema.mof

@@ -2,8 +2,10 @@
 class MSFT_xDSCWebService : OMI_BaseResource
 {
  [Key] string EndpointName;
-  [required, Description("Can take the value AllowUnencryptedTraffic for setting up a non SSL based endpoint")]
+  [write , Description("Can take the value AllowUnencryptedTraffic for setting up a non SSL based endpoint")]


We still need some mechanism to require the user to explicitly opt-out of being secure

TravisEz13 · 2018-04-26T22:19:01Z

@kwirkykat Can you drive this review?

johlju · 2018-05-10T14:37:43Z

@rcarpenter79 are you able to continue the work on this PR? Happy to help you get this reviewed. Could you please rebase this PR against dev so that we get the latest changes?

rcarpenter79 · 2018-05-12T20:36:44Z

I have re-based my branch and fixed some tests that were failing. One test is now failing but in an unrelated module. What is the recommended action?
I have checked the original files and they aren't different to the files in my branch.

johlju · 2018-05-14T08:13:17Z

@rcarpenter79 Not sure the rebase worked as expected because there a lot of changes this PR want to do that is already part of dev, like that changes in the README.md.
Did you use git rebase? See how to rebase here https://github.com/PowerShell/DscResources/blob/master/GettingStartedWithGitHub.md#resolve-merge-conflicts.

Let's start first with the rebase again, and then see if the problem with the tests still persists.

rcarpenter79 · 2018-05-15T11:59:22Z

I think I have fixed the rebase now as it's only showing the files I modified and added.

johlju · 2018-05-16T10:52:17Z

Thanks! Rebase looking good. 🙂 Could you look at the review comments, please go into Reviewable and write 'Done' when they are resolved. When these review comments are resolved I will review again.

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 236 at r3 (raw file):

Previously, rcarpenter79 wrote…

I made this change to mock the OS version for testing.
Is there another way to do this without changing the code?

Could you instead move the original string into a helper function that wraps the original way of getting the OS version? That wrapper function could be changed to use another method in the future, if needed.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 49 at r5 (raw file):

'CertificateThumbprint must contain a certificate thumbprint or "AllowUnencryptedTraffic".

Maybe: 'CertificateThumbprint must contain a certificate thumbprint, or "AllowUnencryptedTraffic" to opt-out from being secure.'

Throughout. Or even better add this as a localized string so it can be reused.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 138 at r5 (raw file):

    else
    {
        $Certificate = ([Array](Get-ChildItem -Path 'Cert:\LocalMachine\My\')).Where{$_.Thumbprint -eq $webBinding.CertificateHash}

This will only work if the thumbprint is set? What should happen if only subject is set?

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 575 at r5 (raw file):

                }
            }
            'CertificateSubject'

Shouldn't this also check if $CertificateThumbPrint -eq 'AllowUnencryptedTraffic'?

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 866 at r5 (raw file):

    )

    Add-Type -AssemblyName "Microsoft.Web.Administration, Version=7.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL"

Using version here, would that break this resource in the future if only a newer version is available?

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 913 at r5 (raw file):

    )

    [Array] $CertificatesFromTemplates = (Get-ChildItem -Path $Store).Where{$_.Extensions.Oid.Value -contains '1.3.6.1.4.1.311.20.2'}

Could we add a comment that describes what the OID number used here means?

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.schema.mof, line 5 at r4 (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

We still need some mechanism to require the user to explicitly opt-out of being secure

Can we use a new Boolean parameter to opt-out - default is secure, and throws if no certificate is specified. To opt-out this new parameter must be set to $true in the configuration. This would remove the use of the string 'AllowUnencryptedTraffic' on the parameter CertificateThumbPrint. This would be a breaking change for configurations that have opt-out.

Comments from Reviewable

rcarpenter79 · 2018-05-16T15:08:24Z

I think I should have addressed everything.

Review status: 0 of 4 files reviewed at latest revision, 9 unresolved discussions.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 849 at r2 (raw file):

Previously, rcarpenter79 wrote…

I was in two minds about that behavior. I have made the suggested change.

Done.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 236 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could you instead move the original string into a helper function that wraps the original way of getting the OS version? That wrapper function could be changed to use another method in the future, if needed.

Done.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 840 at r3 (raw file):

Previously, rcarpenter79 wrote…

I have updated the function to only allow one certificate to be returned.

Done.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 49 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'CertificateThumbprint must contain a certificate thumbprint or "AllowUnencryptedTraffic".
Maybe: 'CertificateThumbprint must contain a certificate thumbprint, or "AllowUnencryptedTraffic" to opt-out from being secure.'

Throughout. Or even better add this as a localized string so it can be reused.

Done.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 138 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This will only work if the thumbprint is set? What should happen if only subject is set?

I'm returning the current configuration in the Get-TargetResource method.
If the website is configured it will find the certificate hash and then get the subject and template name from the certificate. If the website isn't configured then nothing will be returned.
I'm not sure ignore the current configuration if 'AllowUnencryptedTraffic' is set.
An alternative would be to only return 'AllowUnencryptedTraffic' if the current certificate hash is null and 'AllowUnencryptedTraffic' has been specified.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 575 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Shouldn't this also check if $CertificateThumbPrint -eq 'AllowUnencryptedTraffic'?

CertificateSubject and CertificateThumbprint are in different parameter sets. If both are specified it should error. I don't think I added tests for that so I'll add that them while I'm at it.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 866 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Using version here, would that break this resource in the future if only a newer version is available?

You're right. Hard coding a version isn't sensible. I will lookup the version instead in a function so I can Mock it for testing when the assembly isn't present.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 913 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add a comment that describes what the OID number used here means?

I've added the comment and refactored the code to allow for certificates that use 'Certificate Template Information' as well as Certificate Template Name.
I've removed the use of the Oid in favour of using the FriendlyName for consistency.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.schema.mof, line 5 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Can we use a new Boolean parameter to opt-out - default is secure, and throws if no certificate is specified. To opt-out this new parameter must be set to $true in the configuration. This would remove the use of the string 'AllowUnencryptedTraffic' on the parameter CertificateThumbPrint. This would be a breaking change for configurations that have opt-out.

I put a check in place to make sure something was specified for CertificateThumbprint.
I agree that an opt out parameter would probably be better.
Would it be better to raise that as an enhancement for a future code update?

Comments from Reviewable

rcarpenter79 · 2018-05-17T09:52:14Z

There is definitely something odd about the xWindowsProcess Integration Tests.
It has failed on log files again.
Can I ignore it or do I have to make a change to trigger another build?

johlju · 2018-05-17T11:05:55Z

You can close and reopen the PR to kick off the tests again. But this time you can make a change, there are a file that is missing new line at the end; https://ci.appveyor.com/project/PowerShell/xpsdesiredstateconfiguration/build/6.0.1074.0?fullLog=true#L55

johlju · 2018-05-18T13:41:11Z

Reviewed 1 of 3 files at r5, 2 of 3 files at r6, 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 236 at r3 (raw file):

Previously, rcarpenter79 wrote…

Done.

It's currently a (reported) bug with Reviewable that writing 'Done.' with fulls top make me not able to acknowledge the review comments. :/ Please write 'Done' here instead 🙂

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 138 at r5 (raw file):

Previously, rcarpenter79 wrote…

I'm returning the current configuration in the Get-TargetResource method.
If the website is configured it will find the certificate hash and then get the subject and template name from the certificate. If the website isn't configured then nothing will be returned.
I'm not sure ignore the current configuration if 'AllowUnencryptedTraffic' is set.
An alternative would be to only return 'AllowUnencryptedTraffic' if the current certificate hash is null and 'AllowUnencryptedTraffic' has been specified.

It feels like I want it to return $null if no certificate was set. Feels strange to return 'AllowUnencryptedTraffic' as the thumbprint. Here it is another good example that this should have been a boolean instead to opt-out from being secure. Unsecure = $true would have been better in by opinion.
Let's leave it as is for now.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 6 at r7 (raw file):

$Culture

I think this should be $script:culture according to style guideline. Throughout.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 132 at r7 (raw file):

$Output

$output (lowe-case 'o'). Throughout.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 155 at r7 (raw file):

$Certificate

$certificate (lower-case 'c').

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 880 at r7 (raw file):

$GacAssemblyVersion

$gacAssemblyVersion (lower-case 'g'). Throughout.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 932 at r7 (raw file):

$FilteredCertificates

$filteredCertificates lower-case. We should use camelCase for variables except parameters that uses PascalCase.

Throughout. To many to comment on each 🙂 Though, you only need to do your code changes, but wouldn't mind the other changed as well.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.schema.mof, line 5 at r4 (raw file):

Previously, rcarpenter79 wrote…

I put a check in place to make sure something was specified for CertificateThumbprint.
I agree that an opt out parameter would probably be better.
Would it be better to raise that as an enhancement for a future code update?

Leave it as is for now and I will raise the question and see if we should change this now.

DSCResources/MSFT_xDSCWebService/en-US/MSFT_xDSCWebService.psd1, line 5 at r7 (raw file):

    ThrowCertificateThumbprint       = CertificateThumbprint must contain a certificate thumbprint, or "AllowUnencryptedTraffic" to opt-out from being secure.
    ThrowUseSecurityBestPractice     = Error: Cannot use best practice security settings with unencrypted traffic. Please set UseSecurityBestPractices to $false or use a certificate to encrypt pull server traffic.
    FindCertificateBySubjectMultiple = More than one certificate found with subject containing {0} and using template {1}.

Nitpick (non-blocking): Maybe use quotes around template name since it can be spaces in the names? This and below if so.

Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 26 at r7 (raw file):

$DSCResourceName

camelCase here as well. Throughout the tests.

Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 168 at r7 (raw file):

            Context -Name 'DSC Web Service is installed without certificate' -Fixture {
                $Result = Get-TargetResource @TestParameters

Does Pester catches this if it throws an error if this is run in the Context-block and not in the It-block? Should we add this call to each It-block instead?

Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 239 at r7 (raw file):

'should

We usually write 'Should...' (upper 'S'). Throughout.

Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 264 at r7 (raw file):

-Exactly 1

-Exactly -Times 1. Throughout.

Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 1038 at r7 (raw file):

            }
            It 'should return $false if Module is present in Web.config and not expected to be installed.' {
# This test is failing with existing code.

Is this debug/todo comments? This and the one below?

Comments from Reviewable

johlju · 2018-05-18T13:42:40Z

@TravisEz13 @kwirkykat @mgreenegit Could you acknowledge any review comments you have pending or comment if there are still issues. Thanks!

@TravisEz13 What do you think about we add an Unsecure boolean property to the schema instead of opting-out by adding the string 'AllowUnencryptedTraffic' as the thumbprint? That would be a breaking change only for those configurations that has opted-out from being secure.

johlju · 2018-05-21T09:39:58Z

Awesome work fixing some much style issue. Thank you! 😃

Reviewed 3 of 3 files at r8.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

a discussion (no related file):
Could you please add an descriptive entry, for each change/issue, to the Unreleased section of the change log in the file README.md? If an entry resolves an issue please reference the issue.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 246 at r8 (raw file):

[Bool]

Suggest we set all [Bool] to [Boolean]. That is mostly used throughout the DSC Resource Kit. Unless there is a particular reason to use [Bool]?

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 309 at r8 (raw file):

    $esedatabase = "$DatabasePath\Devices.edb"

    $script:culture = Get-Culture

Should this be $script:culture? Or is is it meant to be $culture? This and the one below.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 787 at r8 (raw file):

    param
    (
        [Parameter(Mandatory = $true)]

Just want to verify. By setting these parameters to mandatory, since there are no integration tests, doe the unit test verify that these are always called with both properties? Same for all other helper functions that have the mandatory property changed for the parameters.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.schema.mof, line 5 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Leave it as is for now and I will raise the question and see if we should change this now.

Would you mind creating an issue for this enhancement?

Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 1038 at r7 (raw file):

Previously, rcarpenter79 wrote…

The test fails with existing code. The module IISSelfSignedCertModule(32bit) is present in the web.config and the test should be false.
I'm expecting the function to return $false but instead it returns $true.
Should I simply fix this function and make the test active again?

Please submit an issue for this. After that, if you want to resolve the problem then please do. 🙂 Do you think it is a breaking change?

Comments from Reviewable

rcarpenter79 · 2018-05-21T16:07:24Z

Review status: all files reviewed at latest revision, 9 unresolved discussions.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 246 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[Bool]
Suggest we set all [Bool] to [Boolean]. That is mostly used throughout the DSC Resource Kit. Unless there is a particular reason to use [Bool]?

There's no specific reason to use Bool. I always use the type accelerator, where available, over the .Net class.
I'll change it back to be consistent with the rest of the resource kit.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 309 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should this be $script:culture? Or is is it meant to be $culture? This and the one below.

Done

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 787 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Just want to verify. By setting these parameters to mandatory, since there are no integration tests, doe the unit test verify that these are always called with both properties? Same for all other helper functions that have the mandatory property changed for the parameters.

I checked each occurrence of each function to confirm every parameter is specified when the function is called.
Your comment made me think what would happen if the function was passed a null value or empty string as this would now cause an error that wouldn't have happened previously.
I checked each function to see where the data came from. Half the functions were passing mandatory parameters from the main Get, Set and Test resources.
The remaining 4 all pass a path derived from Get-Website. If this fails then the other functions will not be called.
I don't expect these changes to cause any difference in behaviour and each function requires each parameter in order to return a sensible value.
If you prefer to leave this alone I'll remove the Mandatory value for the original functions.

Comments from Reviewable

johlju · 2018-05-22T07:21:46Z

@rcarpenter79 I think you forgot to push the changes? Can't see any commits between my comment and your comments. Or am I missing something here? 🤔

rcarpenter79 · 2018-05-22T10:32:08Z

I ran out of time. I have pushed the latest changes now.

Review status: 1 of 5 files reviewed at latest revision, 9 unresolved discussions.

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

Could you please add an descriptive entry, for each change/issue, to the Unreleased section of the change log in the file README.md? If an entry resolves an issue please reference the issue.

Done

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 246 at r8 (raw file):

Previously, rcarpenter79 wrote…

There's no specific reason to use Bool. I always use the type accelerator, where available, over the .Net class.
I'll change it back to be consistent with the rest of the resource kit.

Done

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.schema.mof, line 5 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Would you mind creating an issue for this enhancement?

Raised as #419.

Tests/Unit/MSFT_xDSCWebService.Tests.ps1, line 1038 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Please submit an issue for this. After that, if you want to resolve the problem then please do. 🙂 Do you think it is a breaking change?

Raised as #418.
I don't think this is a breaking change because I don't think existing code could work with it the way it currently is.
I have made a change and it's now passing my test.

Comments from Reviewable

rcarpenter79 · 2018-05-22T10:32:25Z

Done

Review status: 1 of 5 files reviewed at latest revision, 9 unresolved discussions.

Comments from Reviewable

johlju · 2018-05-22T11:24:31Z

Reviewed 4 of 4 files at r9.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 309 at r8 (raw file):

Previously, rcarpenter79 wrote…

Done

Want to double-check. Not sure if done means this is correct as-is or if this was supposed to be changed? 🙂

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 787 at r8 (raw file):

Previously, rcarpenter79 wrote…

I checked each occurrence of each function to confirm every parameter is specified when the function is called.
Your comment made me think what would happen if the function was passed a null value or empty string as this would now cause an error that wouldn't have happened previously.
I checked each function to see where the data came from. Half the functions were passing mandatory parameters from the main Get, Set and Test resources.
The remaining 4 all pass a path derived from Get-Website. If this fails then the other functions will not be called.
I don't expect these changes to cause any difference in behaviour and each function requires each parameter in order to return a sensible value.
If you prefer to leave this alone I'll remove the Mandatory value for the original functions.

LGTM. I prefer having these as mandatory if it works.

Comments from Reviewable

rcarpenter79 · 2018-05-22T11:36:14Z

Review status: 4 of 5 files reviewed at latest revision, 5 unresolved discussions.

DSCResources/MSFT_xDSCWebService/MSFT_xDSCWebService.psm1, line 309 at r8 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Want to double-check. Not sure if done means this is correct as-is or if this was supposed to be changed? 🙂

I misunderstood. I thought you were referencing the style guidelines.
You're right. This variable assignment isn't correct compared with the original assignment at the top of the script.
I fixed it, which is handy as the flaky tests failed again.

Comments from Reviewable

johlju · 2018-05-22T12:43:23Z

Awaiting confirmation from other reviewers for a while before merging.

Reviewed 1 of 1 files at r10.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

Comments from Reviewable

johlju · 2018-05-22T12:46:09Z

@rcarpenter79 Think I found the problem with the tests, I hope. Gonna merge #421 once the tests passes.

johlju · 2018-05-22T13:21:31Z

@rcarpenter79 Can you please rebase now after PR #421 was merged. Hope the tests will all pass now! 🙂

johlju · 2018-05-22T13:58:26Z

After rebase for the merging of a fix for randomly failing tests.

Reviewed 1 of 1 files at r11.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

…fied by subject and template name. Removed the Mandatory parameter from CertificateThumbprint and a check to validate that if CertificateSubject is not specified then CertificateThumbprint must be specified and not null. Made some changes to some lines to allow for code to be mocked during testing. Added unit testing for the module.

Reverted OS change from using Cim-Instance back to use the OSVersion method but moved this to a function. Changed the way the Microsoft.Web.Administration was loaded so that it wouldn't only load a specific version but can still mock the ServerManager object. Made a change to Find-CertificateThumbprintWithSubjectAndTemplateName so it will also work with certificates that use the Oid Certificate Template Information and removed references to the Oid from the code in favour of using the friendly name.

…ervice.psd1.

Fixed the issue (#418) in Test-WebConfigModulesSetting so it would return $false if a module exists and shouldn't. Renamed [Bool] back to [Boolean] to keep with the rest of the resource kit. Updated README.md unreleased section.

kwirkykat reviewed Jan 24, 2018

View reviewed changes

BerheAbrha reviewed Jan 30, 2018

View reviewed changes

TravisEz13 reviewed Mar 1, 2018

View reviewed changes

johlju added the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label May 10, 2018

johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels May 16, 2018

johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels May 16, 2018

johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels May 18, 2018

johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels May 22, 2018

johlju added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed needs review The pull request needs a code review. labels May 22, 2018

johlju mentioned this pull request May 22, 2018

MSFT_xDSCWebService Add a parameter for AllowUnencryptedTraffic instead of re-using CertificateThumbPrint #419

Open

johlju added needs review The pull request needs a code review. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels May 22, 2018

johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels May 22, 2018

johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels May 22, 2018

Richard Carpenter and others added 6 commits May 22, 2018 15:20

Added required line ending to MSFT_xDSCWebService/en-US/MSFT_xDSCWebS…

4f0c67d

…ervice.psd1.

Changed code to meet style guidelines.

fe70a39

Fixed incorrect variable assignment to global variable.

de1bc07

johlju merged commit 169a2ba into dsccommunity:dev May 24, 2018

johlju removed the needs review The pull request needs a code review. label May 24, 2018


		$webAdminSrvMgr = [Microsoft.Web.Administration.ServerManager]::OpenRemote("127.0.0.1")

Added CertificateSubject feature to xDSCWebService DSC Resource #379

Added CertificateSubject feature to xDSCWebService DSC Resource #379

Conversation

rcarpenter79 commented Dec 7, 2017 • edited by johlju Loading

codecov-io commented Dec 7, 2017 • edited Loading

Codecov Report

mgreenegit commented Jan 5, 2018

mgreenegit commented Jan 5, 2018

mgreenegit commented Jan 5, 2018

rcarpenter79 commented Jan 5, 2018

mgreenegit commented Jan 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwirkykat commented Jan 24, 2018

BerheAbrha commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcarpenter79 commented Feb 23, 2018

Choose a reason for hiding this comment

TravisEz13 commented Apr 26, 2018

johlju commented May 10, 2018

rcarpenter79 commented May 12, 2018

johlju commented May 14, 2018

rcarpenter79 commented May 15, 2018

johlju commented May 16, 2018

rcarpenter79 commented May 16, 2018

rcarpenter79 commented May 17, 2018

johlju commented May 17, 2018

johlju commented May 18, 2018

johlju commented May 18, 2018

johlju commented May 21, 2018

rcarpenter79 commented May 21, 2018

johlju commented May 22, 2018 • edited Loading

rcarpenter79 commented May 22, 2018

rcarpenter79 commented May 22, 2018

johlju commented May 22, 2018

rcarpenter79 commented May 22, 2018

johlju commented May 22, 2018

johlju commented May 22, 2018

johlju commented May 22, 2018

johlju commented May 22, 2018

rcarpenter79 commented Dec 7, 2017 •

edited by johlju

Loading

codecov-io commented Dec 7, 2017 •

edited

Loading

johlju commented May 22, 2018 •

edited

Loading