fix(MT.1191): skip when break-glass exclusion cannot be verified - #2165
fix(MT.1191): skip when break-glass exclusion cannot be verified#2165earbona23 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe break-glass exclusion test now records missing emergency access accounts as skipped with a custom reason. New Pester tests cover skipped, failed, and passed evaluation outcomes. ChangesBreak-glass verification status
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The unverifiable break-glass condition is now reported as skipped instead of passed, preventing a misleading compliance result. The change is localized and no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description is detailed and covers the problem, fix, linked issue, tests, results, and intentionally unchanged scope. It does not reproduce the template checklist, but it provides the required information in substance. Full details: Linked Issues checkExplanation The implementation directly addresses issue Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
|
Thanks — automated checks are green (CodeRabbit + Codacy). Happy to rebase onto the latest For context: the change is a one-liner that follows the |
MT.1191 was counted as Passed in situations where it had explicitly reported that it could not check anything. When no emergency access accounts are configured in maester-config.json, Test-MtGsaCompliantNetworkBreakGlassExcluded records "break-glass exclusion cannot be verified" and returns $null. The bundled wrapper only asserts when the result is non-null, so the It block completes without executing a single assertion and Pester reports Passed. A control that states its own requirement is unverifiable therefore lands in the passed count, which is the reading most likely to stop someone looking. Add-MtTestResultDetail calls Set-ItResult -Skipped only when -SkippedBecause is supplied. Passing -SkippedBecause Custom with the same message keeps the wording identical and moves the result from Passed to Skipped. That pattern is already used in 153 places in this repository, so nothing new is introduced here. Adds unit tests for the three branches: the unverifiable case now skips, an unexcluded break-glass account still returns false, and an excluded one still returns true. The first test was checked by reverting the one-line fix and confirming it fails -- without that, it would pass either way and prove nothing. Deliberately not changed: the sibling "no enabled Compliant Network policy was found" branch, and the equivalent branches in the other GSA checks. Those are genuinely not applicable rather than unverifiable -- with no such policy there is no lock-out risk -- and they follow the convention established across the GSA family. Happy to revisit separately if maintainers see it differently. Fixes maester365#2114
fb1e5f1 to
5a1950f
Compare
1 participant
Fixes #2114.
The problem
MT.1191 is counted as Passed in exactly the situation where it has just reported that it could not check anything.
When no emergency access accounts are configured in
maester-config.json,Test-MtGsaCompliantNetworkBreakGlassExcludedrecords "break-glass exclusion cannot be verified" and returns$null. The bundled wrapper only asserts when the result is non-null:So the
Itblock completes without executing a single assertion, and Pester reports Passed. A control that states its own requirement is unverifiable lands in the passed count — which is the reading most likely to stop someone from looking.The fix
Add-MtTestResultDetailcallsSet-ItResult -Skippedonly when-SkippedBecauseis supplied. Passing-SkippedBecause Customwith the same message keeps the wording identical and moves the result from Passed to Skipped:That pattern is already used in 153 places in this repository, so this introduces nothing new.
Tests
Adds
powershell/tests/functions/Test-MtGsaCompliantNetworkBreakGlassExcluded.Tests.ps1covering three branches: the unverifiable case now skips, an unexcluded break-glass account still returns$false, and an excluded one still returns$true.The first test was checked by reverting the one-line fix and confirming it fails (
Expected Add-MtTestResultDetail to be called 1 time exactly, but was called 0 times). Without that check it would pass with or without the bug and prove nothing.Full unit suite after the change: 4103 passed, 0 failed (Pester 6.1.0, pwsh 7.6.2, Linux).
Deliberately not changed
The sibling
"No enabled Compliant Network enforcement policy was found"branch, and the equivalent branches in the other GSA checks, are left alone. Those are genuinely not applicable rather than unverifiable — with no such policy there is no lock-out risk — and they follow the convention established across the GSA family. Happy to revisit separately if you see it differently.Summary by CodeRabbit
Bug Fixes
Tests