Move Find-Certificate Function from CertificateDsc.Common.psm1 to DscResource.Common

[hollanjs]

Move Find-Certificate function, and all function dependencies and tests from CertificateDsc.Common.psm1 to DscResource.Common.

I've found that several DSC code bases are implementing their own methods for obtaining references to certificates, when this should be a standardized functional call.

Moving the function to DscResource.Common would ensure its availability to modules like SharePointDsc, who need to obtain certificate reference when, for example, when using the resource SPTrustedRootAuthority:
SharePointDsc Issue #1417

[hollanjs]

@johlju - Thoughts on this? This is similar to some of the SqlServerDsc functions that we moved over to DscResources.Common. After some initial digging, almost all DSC modules implement their own homebrewed way to obtain references to certificates in the local certificate stores.

[johlju]

I would not mind having that in DscResource.Common. But a thought, there seems to be a lot of certificate related functions in CertificateDsc.Common that could be made as public functions. We could add them to a seperate new module, e.g. PSCertificate (or another name that is not taken on the gallery). The new module can be used by resource like DscResource.Common.

@PlagueHO your thoughts on this? Move them to DscResource.Common?

[PlagueHO]

I'm happy to move this over to DscResource.Common. @hollanjs - do you have a list of the other resources you've encountered that also use similar functions? We could then raise issues in those repos to migrate them across to the function.

[hollanjs]

@PlagueHO - I do not have a list, but I can start one and keep an eye out for other resources like this. I might take some time this weekend to look around the xxxxxxxxDsc.Common.psm1 files in the modules and see if anything sticks out.

[hollanjs]

All the functions below I believe would make good candidates for DSCResource.Common.

@johlju mentioned the possibility of breaking it up into different utilities. I know there are several ways of doing that (nesting modules, creating other modules and declaring as dependencies, breaking apart code by folder structures, etc.), but not sure what might work best, if at all; there is something nice about it being one module. However as the functions grow, it would be good to have some way to visibly organize them.

I categorized some of the functions below as though they could be used in a separate, more specific, modules. I could picture being able to import all of these more specific modules when you import DSCResource.Common, but also being able to pull in a more specific module for finding and getting reference to certificates (ex. DSCResource.Common.Certificates).

I'm also sure there's more utilities buried within some of the modules that can be dug out, or a new solution for an issue all modules are trying to solve on their own. Perhaps categorizing common utilities into separate modules or buckets will allow people to better see what is missing and expand them.

---

Might be useful creating a DSCResource.Common.IPAddress module for IP functions useful to DHCP, DNS, Network DSC modules

• DhcpServerDsc.Common.psm1
  • Get-ValidIPAddress
• NetworksDsc.Common.psm1
  • Get-IPAddressPrefix
  • Convert-CIDRToSubhetMask

---

Might be useful creating a DSCResource.Common.Certificates module for certificate functions useful to Web Service DSC Modules like SharePoint, OfficeOnlineServer, WebAdministration, etc...

• CertificateDsc.Common.psm1
  • Test-CertificatePath
  • Test-Thumbprint
  • Find-Certificate
  • Get-CertificateStorePath
  • Get-CertificateFromCertificateStore

---

Might be useful creating a DSCResource.Common.RegEdit module for registry functions useful to all DSC modules...

• ComputerManagementDsc.Common.psm1

  • Get-RegistryPropertyValue

    SqlServerDsc.Common has alternate version of this same function

• SqlServerDsc.Common.psm1

  • Format-Path

---

Might be useful creating a DSCResource.Common.ThrowException module for identifying and throwing exceptions. Every DSC Module seems to have their own unique way of doing this, as well as pulling in localized data...

• DhcpServerDsc.Common.psm1

  • New-TerminatingError

    WebAdministrationDsc.Common.psm1 has a version of this

• GPRegistryPolicyDsc.Common.psm1

  • New-InvalidArgumentException
  • New-InvalideOperationException
  • New-ObjectNotFoundExceptiona
  • New-InvalidResultException
  • New-NotImplementedException

---

Things for DSCResource.Common

• ActiveDirectoryDsc.Common.psm1

  • Start-ProcessWithTimeout
  • Get-DomainName
  • ConvertTo-TimeSpan

    HyperVDsc.Common has alternate version of this same function

  • ConvertFrom-TimeSpan

    HyperVDsc.Common has alternate version of this same function

  • Convert-PropertyMapToObjectProperties
  • Compare-ResourcePropertyState
  • Add-TypeAssembly
  • Get-CurrentUser

    WebAdministrationDsc.Common has a verion of this function

  • Test-Password
  • Get-ByteContent
  • Resolve-SamAccountName
  • Resolve-SecurityIdentifier
  • Test-DscPropertyState

    WebAdministration.Common has a version of this
    
    DnsServerDsc.Common implementes a similar Test-DscDnsParameterState
    
    GPRegistryPolicyDsc.Common implements a similar Test-DscparameterState

• ComputerManagementDsc.Common.psm1

  • Test-Command

    CertificateDsc.Common has a version called Test-CommandExists

• SqlServerDsc.Common.psm1

  • Copy-ItemWithRobocopy
  • Invoke-InstallationMediaCopy
  • Connect-UncPath
  • Disconnect-UncPath
  • Test-PendingRestart

    needs updates to include other areas of reboot causes

  • Import-Assembly
  • Get-ServiceAccount

• WebAdministrationDsc.Common.psm1

  • Start-ProcessWithtimeout
  • New-CimCredentialInstance
  • New-TerminatingError

• xBitlocker.Common.ps1

  • Add-ToPSBoundParametersFromHashtable
  • Remove-FromPSBoundParametersUsingHashtable
  • Get-OSEdition

[johlju]

Really impressive analysis here!

@johlju mentioned the possibility of breaking it up into different utilities.

Yes, but it is not something we must do. I do like the idea you had of having an 'az' type of module that will import all the rest. I do like the way you categorized them (might chnage IPAddress to Net, and RegEdit to Registry, and ThrowException to ErrorHandling, but minor). But I see it being a lot of work of maintaining several modules and releasing them, that will be the biggest drawback for me. We would keep an eye out for PR's in several modules. See also next thought below.

there is something nice about it being one module. However as the functions grow, it would be good to have some way to visibly organize them.

I agree, and maybe we should keep one module, and automate the documenation part instead (PlatyPS). Publishing that and generating a nice start page with all the commands (https://dbatools.io/commands like) might be what is lacking in discovery?

Get-RegistryPropertyValue

SqlServerDsc should move to the one in DscResource.Common. Just haven't happen yet.

Same for all other modules that have similiar function, should be moved to the DscResource.Common one, and maybe extend the one in DscResource.Common.

In conclusion, looking at the maintaining part (unless someone raise there hand to maintain separate modules) I rather keep one module (for now at least).

[PlagueHO]

Very thorough job @hollanjs ! Great stuff. I think we should probably move this issue over to DscResource.Common and break it into tasks (with the first task to move Find-Certificate). I can see this being a fairly time-consuming process - but worth doing.

Moving to PlatyPS would be ideal as well (I've implemented in some of my other non-DSC resource modules that use Sampler). Would be good to finally implement a PlatyPS task (something I've meant to do... but no time).

[hollanjs]

@PlagueHO - The way I did this in the past with @johlju was we did PRs for both repos at the same time: one to add the function to DscResource.Common, then one to remove it from the old repo (e.x. SqlServerDsc); both PRs executed pretty much at the same time. If multiple functions should be moved to DscResource.Common, should all functions be move to DscResource.Common first, a new version published, and only after that begin updating the other modules to remove them?

[PlagueHO]

@hollanjs - I reckon we do this function by function. Big bang changes are a pain and too risky (not too mention the review headache).

What I mean is we create an issue with a checklist that is used to track the work. E.g
, How we did with the hqrm tracking.

But regarding Find-Certificate, happy to do that using the synchronized PR approach. Just need DscCommon to be updated first as this is a build time to dependency rather than install time - which makes it simple.

[hollanjs]

@PlagueHO - I can create the issue on the DscResource.Common to get this started.

If you would like, you can assign both issues to me to complete and I can get the PRs queued up some time this week/weekend for yours and/or @johlju review.

Can you paste a link here for the HQRM issue you referenced? I would like to check it out to see the checklist you would like to use as a template.

[johlju]

@hollanjs you create a task list by using this

- [ ] Task1
- [x] Task1
- [ ] Task1

It will look like this, which can be checked.

• Task1
• Task1
• Task1

[johlju]

Instead of transferring tis issue I suggest creating an issue in DscResource.Common whith a task list of all functions that should be moved from different repos (your suggestions above) over as new commands, or... create several new issues, one for each command that should be brought over, which can be auto closed by PR's. Either way is good.

[johlju]

@hollanjs you are now assigned. Sorry for taking long to answer - have very busy days for a while now.

[hollanjs]

@johlju - no worries! Same here on my end. I should be able to have a PR for this by end of the weekend.

[hollanjs]

@johlju - I've pretty much got it all working, except for some issues with the tests. I rewrote the tests to use a mocked certificate object rather than it actually needing to create/delete a certificate on a windows machine to use as reference. All is good with the mocked cert objects, however there were tests to ensure Test-Path and Get-ChildItem were called exactly 1 time when the Find-Certificate function is called. Those functions are mocked as well, however, they're not being reported as called when the tests run, so I'm currently working through that.

[PlagueHO]

@hollanjs - thanks for working on this.

It is possible that when we created the Find-Certificate function (the code used to be inline), the tests were not updated properly and calling functions were testing implementation details of Find-Certificate (which they shouldn't do).

Can you check that:

• Any function calling Find-Certificate only mocks Find-Certificate and doesn't try to test implementation details that it should not know about. This was acceptable when Find-Certificate was part of this resource, but won't be if it's moved into an external dependency.
• Note: The tests for Find-Certificate in DscResource.Common can test implementation details (so can mock Test-Path and Get-ChildItem etc).

Thanks for fixing up past laziness in the tests.

[hollanjs]

@PlagueHO - Anytime!

Sorry, just realized I should have been talking about the PR and mock issues in the DscResource.Common thread, as those pertained to the Find-Certificate after it was moved.

I will go through the tests and refactor like you mentioned!

[hollanjs]

@PlagueHO - I've been stuck on something, trying to figure it out for some time. The Set and Test unit tests for DSC_CertificateExport is erroring out ever since the Find-Certificate function has been removed. The unit test has that function mocked, however, for some reason it continues to throw:

...
 Describing DSC_CertificateExport\Set-TargetResource

    Context Certificate is not found
      [-] should not throw exception 39ms
        CommandNotFoundException: Could not find Command Find-Certificate
        at Validate-Command, Z:\DSC\CertificateDsc\output\RequiredModules\Pester\4.10.1\Functions\Mock.ps1: line 914
        at Mock, Z:\DSC\CertificateDsc\output\RequiredModules\Pester\4.10.1\Functions\Mock.ps1: line 215
        at <ScriptBlock>, Z:\DSC\CertificateDsc\tests\Unit\DSC_CertificateExport.Tests.ps1: line 214
        at Invoke-Blocks, Z:\DSC\CertificateDsc\output\RequiredModules\Pester\4.10.1\Functions\SetupTeardown.ps1: line 135
        at Invoke-TestCaseSetupBlocks, Z:\DSC\CertificateDsc\output\RequiredModules\Pester\4.10.1\Functions\SetupTeardown.ps1: line 113
      [-] should call the expected mocks 33ms
        CommandNotFoundException: Could not find Command Find-Certificate
        at Validate-Command, Z:\DSC\CertificateDsc\output\RequiredModules\Pester\4.10.1\Functions\Mock.ps1: line 914
        at Mock, Z:\DSC\CertificateDsc\output\RequiredModules\Pester\4.10.1\Functions\Mock.ps1: line 215
        at <ScriptBlock>, Z:\DSC\CertificateDsc\tests\Unit\DSC_CertificateExport.Tests.ps1: line 214
        at Invoke-Blocks, Z:\DSC\CertificateDsc\output\RequiredModules\Pester\4.10.1\Functions\SetupTeardown.ps1: line 135
        at Invoke-TestCaseSetupBlocks, Z:\DSC\CertificateDsc\output\RequiredModules\Pester\4.10.1\Functions\SetupTeardown.ps1: line 113
...

Here is the mock:

$mockFindCertificate = {
    if ($Thumbprint -eq $certificateThumbprint)
    {
        $validCertificate
    }
}
...
Describe 'DSC_CertificateExport\Set-TargetResource' -Tag 'Set' {
    BeforeEach {
        Mock `
            -CommandName Find-Certificate `
            -MockWith $mockFindCertificate
    }

    Context 'Certificate is not found' {
        Mock `
            -CommandName Export-Certificate

        Mock `
            -CommandName Export-PfxCertificate

        It 'should not throw exception' {
            { Set-TargetResource @validCertificateNotFoundParameters -Verbose } | Should -Not -Throw
        }

        It 'should call the expected mocks' {
            Assert-MockCalled -CommandName Find-Certificate -Exactly -Times 1
            Assert-MockCalled -CommandName Export-Certificate -Exactly -Times 0
            Assert-MockCalled -CommandName Export-PfxCertificate -Exactly -Times 0
        }
    }
...

If you've seen this before, let me know! Any assistance would be greatly appreciated :-)

[johlju]

Is it Pester that cannot find the command Find-Certificate when calling Mock? The command must be available in the session for Mock to work. You have to make sure the module DscResource.Common is imported into the session. But that should normally happen when the module (the resource) is imported.