Fix MT.1020: Support new OnPremisesDirectorySyncAccount role and service principal-based sync#1666
Merged
SamErde merged 10 commits intomaester365:mainfrom Apr 25, 2026
Conversation
… synchronization accounts in Conditional Access policies
Up to standards ✅🟢 Issues
|
Contributor
There was a problem hiding this comment.
Pull request overview
Updates MT.1020 coverage for directory synchronization accounts by expanding the test to consider both legacy and new directory sync roles, and refreshes the baseline test text to match the updated intent.
Changes:
- Update MT.1020 baseline test description to mention Directory/OnPremises synchronization accounts.
- Refactor
Test-MtCaExclusionForDirectorySyncAccountto pull members from bothDirectorySynchronizationAccountsandOnPremisesDirectorySyncAccountrole template IDs. - Add per-policy checks for sync members included/excluded via
includeUsers/excludeUsersandincludeRoles/excludeRoles.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tests/Maester/Entra/Test-ConditionalAccessBaseline.Tests.ps1 | Updates MT.1020 test text to reflect expanded sync account scope. |
| powershell/public/maester/entra/Test-MtCaExclusionForDirectorySyncAccount.ps1 | Switches to dual role template IDs and adds member-based inclusion/exclusion evaluation logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…SyncAccount.ps1 Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The exclusion checks (\, \, and the inner if/else) were placed inside the 'if (\ -or \)' branch, meaning they only ran when sync accounts were explicitly and individually included in a policy. For the common case of a policy targeting 'All' users, neither flag is set (\ contains the literal string 'All', not individual user IDs; \ is empty unless a role is explicitly scoped). As a result: - The 'if' block was never entered for all-user policies - The exclusion checks were never executed - \ carried over from the previous loop iteration Fix: add 'continue' to the 'included' branch so policies that specifically include sync accounts are correctly skipped, and move all exclusion evaluation into the 'else' branch so it runs for every policy that does NOT explicitly include sync accounts (the case where exclusion is actually required). Also correct the \ comment: \ is already filtered to exclude service principals, so 'users and service principals' was inaccurate. Minor: capitalise the inline comment for consistency.
16 tests covering the key execution paths: - No sync account members in tenant → not applicable, returns true - Service principal-only members → returns true (SPs not subject to CA) - Policy not scoped to all applications → skip, returns true - Policy targets only guests → skip, returns true - Policy only blocks legacy auth → skip, returns true (sync does not use legacy auth) - Sync account excluded by DirectorySynchronizationAccounts role → returns true - Sync account excluded by OnPremisesDirectorySyncAccount role → returns true - All user members individually excluded by user ID → returns true - Policy not excluded at all → returns false - Only partially excluded (not all user members excluded) → returns false - Mix of user + service principal: user excluded, SP not subject to CA → returns true - Policy scoped specifically to sync account by user ID → skip, returns true - Policy scoped specifically to sync role → skip, returns true - Multiple policies, one fails → returns false - Multiple policies, all pass → returns true - Free Entra ID license → returns null (test skipped) Also: use single quotes for string literals in Get-MtRoleInfo calls (no variable expansion needed).
This was
linked to
issues
Apr 25, 2026
**Bug fix (production)**: When all role members are service principals, \ is empty after filtering out SPs. The code reached the else branch where \ = (0 -gt 0 -and ...) = \False and \ was also \False, causing the policy to be incorrectly marked as non-compliant and the function to return \False. Fix: add an elseif (\.Count -eq 0) branch that skips the exclusion check and continues — consistent with the existing rationale that service principals bypass CA policies and do not need to be excluded. **Test fix**: 'Multiple policies — all pass' expected \True but got \, meaning the function threw an exception. The test used \ and \ in the same mock scriptblock for two separate New-CaPolicy calls. Changed policy2 to use ExcludeRoles @(\) instead of ExcludeUsers, which avoids the problematic combination. The scenario still meaningfully tests that the loop correctly passes all policies when each excludes sync accounts via a different role.
…e mock Pester 5 with -ModuleName mocks has a scoping quirk: referencing two different $script: variables (DirSyncRoleId + OnPremRoleId) from the same mock scriptblock causes the scriptblock to throw (caught by the functions try/catch, returning $null instead of $true). Fix: pre-assign each policy to a local variable inside the mock scriptblock and hardcode the role GUIDs as string literals, removing the dependency on $script: variable resolution in that context.
Contributor
|
It's going in, @Mynster9361!!! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix issue
#699
#582
Following PR's can be closed
#1642
#1643
Fix MT.1020: update tests to ensure exclusion of Directory/OnPremises synchronization accounts in Conditional Access policies
Contribution Checklist
Before submitting this PR, please confirm you have completed the following:
/powershell/tests/pester.ps1on your local system.Join us at the Maester repository discussions 💬 or Entra Discord 🧑💻 for more help and conversations!