Repository navigation
21801: Fix pass/fail count in assessment - #1597
Sandeep Jha (sandeepjha000) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The targeted correction resolves the undefined-variable defect without changing assessment criteria; tests were not run.
0 open findings
What changed in this PR
Fixes assessment 21801’s pass/fail calculation for phishing-resistant authentication methods.
Changes:
- Counts the existing
$phishResistantUserscollection instead of an undefined variable. - Preserves the requirement that all assessed users must be compliant.
| File | Description |
|---|---|
src/powershell/tests/Test-Assessment.21801.ps1 |
Corrects the compliant-user count using @($phishResistantUsers).Count. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sandeep Jha (sandeepjha000)
added
the
ready for review
There was a problem hiding this comment.
LGTM,
However this test is not limiting the results to a reasonable count. In case of a lot of users, this will generate large table and potentially breaking the generated report.
Please address that following the guidelines we took regarding "All guest must have a sponsor" assessment.
|
Anton Staykov (@astaykov) Are you approving this PR as it is currently or are you asking for limiting the results? |
Anton Staykov (astaykov)
left a comment
Anton Staykov (astaykov)
left a comment
There was a problem hiding this comment.
This test is not limiting the results to a reasonable count. In case of a lot of users, this will generate large table and potentially breake the generated report.
Please address that following the guidelines we took regarding "All guest must have a sponsor" assessment.
Aleksandar Nikolić (alexandair)
left a comment
Aleksandar Nikolić (alexandair)
left a comment
There was a problem hiding this comment.
Recommendation: calculate compliance over the complete result set, then limit only the displayed detail rows. Show total, compliant, and noncompliant counts; prioritize users who need remediation; disclose when detail rows are omitted. Do not add a SQL LIMIT before the pass/fail calculation, because that could hide noncompliant users. Using StringBuilder alone would not bound report size. Add a test with more rows than the chosen display limit and a noncompliant user beyond that limit.
Anton Staykov (@astaykov) 's review asks for this limit. The earlier approval does not mean that request is resolved. The guest-sponsor implementation in this exact base (Test-Assessment.21877.ps1#L96) also has an unbounded detail loop; do not assume that copying its current code supplies the requested cap.
Assessment 21801 used an undefined variable when counting users with phishing-resistant authentication methods. As a result, it could report failure even when all users had a qualifying method.
Use the existing $phishResistantUsers collection for the count, ensuring the assessment passes when every user is compliant and continues to fail when any users are not.