[BUG] Fix duplicate policy creation bug & dynamic group compliance

[AlfredSchreuder]

Pull Request (PR) description

• Fix duplicate policy creation bug, when existing policy was found in tenant, still a new policy was created.
• Fix dynamic group compliance, when dynamic group was found, returned incompliant due to empty parameters Members & GroupAsMembers. Edited so that those params are only added to the comparison when it is a static group.

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 parameter descriptions added/updated in the schema.mof.
• Resource documentation added/updated in README.md.
• Resource settings.json file contains all required permissions.
• Examples appropriately added/updated.
• Unit tests added/updated.
• New/changed code adheres to DSC Community Style Guidelines.

[FabienTschanz]

I do like the general approach with checking if there are multiple policies, so this should probably also be added in the resource generator.

[FabienTschanz]

I'm also not fond of switching the ErrorAction to stop and wrapping it in a try/catch block. Try/catch adds massive underlying logic and degrades performance (even though it might not impact us that hard), whereas the previous SilentlyContinue action with a $null result was perfectly enough to indicate that a policy was not found.

[AlfredSchreuder]

The previous code did not check compliance of the existing policy found by Name when the Id was not specified. It always resulted in false and created a new policy. I felt like the SilentlyContinue ignored alot of Verbose logging and did not tell me what went right and what went wrong. After implementing this code I allways got the right Verbose logging to tell me what the script did find or did not find.

[AlfredSchreuder]

Example of old code with no Id specified:

Example of old code with wrong Id specified:

These examples were applicable for all the files changed. And with this fix it also found the policies in both cases and check compliancy or updated the existing policies.

[FabienTschanz]

This seems more like an internal bug because the log output tells us that there was a policy found but it still created a new policy, which is incorrect. This should not be the case.

[AlfredSchreuder]

It should not, but it does, even when compliant. I tested this multiple times on all different policies. After the changes compliant policies actually returned True or when imcompliant changed the existing policies found by name. Feel free to test it yourself.

[FabienTschanz]

Why is this even necessary? Now there is two times the same block:

$getValue = Get-MgBetaDeviceManagementConfigurationPolicy -All -Filter "Name eq '$DisplayName'"
if ($getValue.Length -gt 1)
{
    throw "Duplicate Intune Account Protection Policy for Windows10 named $DisplayName exist in tenant"
}

[AlfredSchreuder]

As cases described in the below comment, I believe it is usefull because it are 2 different cases. The code used to result in described cases is integrated from the MSFT_AADConditionalAccessPolicy which had the behaviour that this policies lacked.

[FabienTschanz]

I still believe that only the if check should be added to the modules and not really other changes. I'll check some things and report back.

[AlfredSchreuder]

Actually this block that is present 2 times handles the cases where multiple policies are present with the same Name. This was not present in the old code. Maybe the If statement was enough to handle the other cases, I don't know, I copied/used/integrated working code to make these policies show the expected behaviour.

[FabienTschanz]

@AlfredSchreuder Please take a look at my other comments as well. The capitalized i of Id is now fixed, but the other comments need to be addressed as well.

[AlfredSchreuder]

@FabienTschanz Don't you see my comments on your questions/comments? My bad, apparantly I had to submit the review to actually show the comments, GitHub nooby here.

[FabienTschanz]

@FabienTschanz Don't you see my comments on your questions/comments? My bad, apparantly I had to submit the review to actually show the comments, GitHub nooby here.

No worries, everybody trips somewhere. Now they are visible. I'll report back once I'm done testing.

[AlfredSchreuder]

@FabienTschanz Regarding your testing and open conversations/comments, originally I changed the code according the the MSFT_AADConditionalAccessPolicy handling all below cases, I prefer the changed policies to show the same behaviour, like I showed in the screenshots the old code did not:

• Id specified and found -> compliant or update
• Id specified and not found by Id, but found by Name -> compliant or update
• Id specified and not found by Id, but found by Name and multiple policies with same Name -> Error, cant decide which policy to check
• Id not specified and found by Name -> compliant or update
• Id not specified and found by Name, but multiple policies with same Name -> Error, cant decide which policy to check
• All cases above are not applicable and didn't found by Id or Name -> Error, no matchable policy to check

[FabienTschanz]

I would like to suggest a combined approach of what we both would like to achieve.
Edit: I tend to disagree that the new messages are better, they are okay for one resource, but not if specified in multiple policies. Then you don't know with which resource you're currently dealing with.

In the module, when first checking for the id, instead of directly calling the Graph command, let's use the following code (example of LAPS):

# Retrieve policy general settings
$policy = $null
if (-not [System.String]::IsNullOrEmpty($Identity))
{
    $policy = Get-MgBetaDeviceManagementConfigurationPolicy -DeviceManagementConfigurationPolicyId $Identity -ErrorAction SilentlyContinue
}

Second, let's update the "fetch by name" like the following:

if (-not [System.String]::IsNullOrEmpty($DisplayName))
{
    $policy = Get-MgBetaDeviceManagementConfigurationPolicy `
        -Filter "Name eq '$DisplayName' and templateReference/TemplateId eq '$templateReferenceId'" `
        -ErrorAction SilentlyContinue

    if ($policy.Length -gt 1)
    {
        throw "Duplicate Account Protection LAPS Policy named $DisplayName exist in tenant"
    }
}

This requires a small change in the catch block of the Get-TargetResource function:

catch
{
    New-M365DSCLogEntry -Message 'Error retrieving data:' `
        -Exception $_ `
        -Source $($MyInvocation.MyCommand.Source) `
        -TenantId $TenantId `
        -Credential $Credential

    # Necessary to rethrow caught exception regarding duplicate policies
    if ($_.Exception.Message -like "Duplicate*")
    {
        throw $_
    }

    $nullResult = Clear-M365DSCAuthenticationParameter -BoundParameters $nullResult
    return $nullResult
}

What do you think? Is that a way we can go?

[AlfredSchreuder]

I already changed the messages in the last commit, feel free to change the code to whatever you prefer it to be. I spent alot of time to get the expected behaviour I wanted/expected. If you want to write it in your way/style, I am fine with that as long as it has the expected behaviour on all cases as described. Another contributor wrote the code for the MSFT_AADConditionalAccessPolicy module which has the expected behaviour, so I just implemented that.

[FabienTschanz]

We appreciate the time and effort you put in. It's in our best interest to fix the issues with minimal changes. I'm simply trying to integrate your proposed changes, which I deem a bit too excessive, in a version that still fixes everything with a bit reduced impact and code changes.
@ykuijs What's your take on it? Which direction should we continue, or do you have any other input?