dsccommunity / SecurityPolicyDsc

A wrapper around secedit.exe to configure local security policies
MIT License
177 stars 53 forks source link

Ensure that returned value includes specified name #152

Closed michaeltlombardi closed 3 years ago

michaeltlombardi commented 3 years ago

Prior to this commit, invoking the AccountPolicy resource with the Get method would always return $Null for the Name property; though this property is itself never used by the resource, integrating tools are unable to tell as the Name property is also the Key (as defined in the MOF) for this resource; meaning when comparing the existing state of the resource it will always cause a mismatch between the specified Name and the "actual" representation of the resource on the machine, as reported by the Get-TargetResource function.

This commit modifies the Get-TargetResource function for the AccountPolicy DSC resource to always return the specified Name for the resource in order to prevent churn in secondary tools such as Puppet, Ansible, or Chef.

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list


This change is Reviewable

michaeltlombardi commented 3 years ago

Looking over the test suite, I didn't see anything specific to add for this change - I'm happy to cover this in an additional unit test if the maintainers think it necessary (It returns the specified name on the result object in the Get-TargetResource/General operation tests section or something).

Regarding the failing tests, as best I can tell right now, those are failures with the test framework? HQRM tests are failing because something in the framework is passing $Null to the Path parameter of something (see this failure). Similarly, there's a configuration issue at play during the unit tests.

I don't have the familiarity to troubleshoot these at this time, but I'm open to pairing on the problem with a maintainer!

gaelcolas commented 3 years ago

@X-Guardian have you had a chance to look at that?

gaelcolas commented 3 years ago

OH, I see... @michaeltlombardi I think it's a version pinning issue since the release of Pester 5 and ModuleBuilder 1.7.0

In requiredModules.psd1 can you try unpinning ModuleBuilder by setting to latest, and pinning Pester to 4.10.1 ?

Then on a clean PS session, remove the whole output module and build.ps1 again.

gaelcolas commented 3 years ago

leave it to me, there's a bit more to do.

gaelcolas commented 3 years ago

Hopefully #153 will fix your build issue after I merge & you rebase.

codecov[bot] commented 3 years ago

Codecov Report

Merging #152 into master will increase coverage by 0%. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #152   +/-   ##
=====================================
  Coverage      89%    89%           
=====================================
  Files           5      5           
  Lines         574    575    +1     
=====================================
+ Hits          516    517    +1     
  Misses         58     58           
gaelcolas commented 3 years ago

The tests are fixed with #153 and I rebased this branch.

I will let @X-Guardian to review this small change as he knows this resource better than I do.

michaeltlombardi commented 3 years ago

Thanks so much, @gaelcolas!