UserRightsAssignment: Allow unresolvable SIDs found in local security policy

[ShawnHardwick]

Pull Request (PR) description

Do not attempt to translate identities returned by local security policy when setting the resource. If the identity returned is an unresolvable SID (usually due to deleted/orphaned AD objects), then the resource will throw a translation error.

I also thought of changing the ConvertTo-NTAccount function to never throw but instead log verbosely for all scope types, but felt that the implementation in this PR was more precise.

SecurityPolicyDsc/source/Modules/SecurityPolicyResourceHelper/SecurityPolicyResourceHelper.psm1

Lines 369 to 377 in b29bdf8

	if ($Scope -eq 'Get')
	{
	Write-Verbose -Message ($script:localizedData.ErrorSidTranslation -f $sidId, $Policy)
	$result += $sidId.Value
	}
	else
	{
	throw "$($script:localizedData.ErrorSidTranslation -f $sidId, $Policy)"
	}

The resource will still translate any identities given to the resource by the user.

Neither secedit nor Windows care about the presence of unresolved SIDs in a security policy. During a configure operation (secedit.exe /configure), secedit will happily accept either style in the .inf file. It will also accept and de-duplicate security principals that are specified as an account name and SID in the same line item under the [Privileges] section.

First attempt at using Pester unit testing before. I don't have a way to test locally due to permission restrictions.

This Pull Request (PR) fixes the following issues

• Fixes How to properly handle SID translation failures #78
• Fixes Unresolved SIDs in unmanaged user rights cause a failure #87
• Fixes UserRightsAssignment: 'Could not translate SID' for orphaned SID #158

Task list

• Added an entry to the change log under the Unreleased section of the file CHANGELOG.md.
  Entry should say what was changed and how that affects users (if applicable), and
  reference the issue being resolved (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 Community Testing Guidelines.
• Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
• New/changed code adheres to DSC Community Style Guidelines.

---

This change is 

[codecov]

Codecov Report

Merging #161 (620a4ef) into master (44acc25) will increase coverage by 0%.
The diff coverage is 66%.

@@          Coverage Diff          @@
##           master   #161   +/-   ##
=====================================
  Coverage      89%    89%           
=====================================
  Files           5      5           
  Lines         577    579    +2     
=====================================
+ Hits          517    520    +3     
+ Misses         60     59    -1     

[ShawnHardwick]

I believe this PR is ready for review. It is my understanding that the currently failing CI tests are not related to my PR.

[ShawnHardwick]

Is there anything missing that I need to address to have this reviewed?

[jdwhited]

Can this be reviewed for merge? I would like to utilize this. Thanks!

[gaelcolas]

I've just kicked another build to check, but please make sure all the test pass for starter.
I think some High Quality Resource Modules were not passing.

[gaelcolas]

Ok they're passing.
@bcwilhite could you check this PR please and review the proposal please?

[ShawnHardwick]

Still looking to get this reviewed. :)

[jdwhited]

Can this be reviewed please?

[jdwhited]

Bump.... Would really like to see this merged.

[japatton]

Bump for release.

[ShawnHardwick]

Still looking for someone to review this lol. We've forked this internally, but would see benefit in having this merged.

[gaelcolas]

Will bump this in the #DSC channel of the PowerShell slack/discord

[ghost]

Hi there. Any updates on this? Facing the same problem.

[ic248]

We're hitting the same problem also, would be good to get a fix