Skip to content

Fix missing permission check#1465

Open
nicolonsky wants to merge 5 commits intomaester365:mainfrom
nicolonsky:main
Open

Fix missing permission check#1465
nicolonsky wants to merge 5 commits intomaester365:mainfrom
nicolonsky:main

Conversation

@nicolonsky
Copy link
Contributor

Description

Fixes #1422.

Contribution Checklist

Before submitting this PR, please confirm you have completed the following:

  • 📖 Read the guidelines for contributing to this repository.
  • 🧪 Ensure the build and unit tests pass by running /powershell/tests/pester.ps1 on your local system.

@nicolonsky nicolonsky requested a review from a team as a code owner February 25, 2026 22:08
@SamErde SamErde requested a review from Copilot February 26, 2026 02:44
@SamErde SamErde added maester-test Related to a Maester test intune Microsoft Intune labels Feb 26, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses issue #1422 by adding a permission check to the MT.1100 test (Test-MtIntuneDiagnosticSettings) to gracefully handle cases where the user lacks Azure RBAC permissions to read Intune diagnostic settings. When a 403 (Forbidden) response is received from the Azure ARM API, the test now skips with an appropriate message instead of failing.

Changes:

  • Added permission check that detects 403 status code and throws UnauthorizedAccessException
  • Added catch block to handle UnauthorizedAccessException and skip test with custom reason
  • Added documentation explaining the Azure RBAC prerequisites and providing PowerShell snippets to create a custom role

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
powershell/public/maester/intune/Test-MtIntuneDiagnosticSettings.ps1 Added 403 status code check and exception handling to skip test when Azure RBAC permissions are insufficient
powershell/public/maester/intune/Test-MtIntuneDiagnosticSettings.md Added prerequisites section documenting required Azure RBAC permissions and remediation steps

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

SamErde and others added 3 commits March 5, 2026 13:07
…gs.ps1

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…gs.md

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…gs.ps1

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +73 to +75
} catch [System.UnauthorizedAccessException] {
Add-MtTestResultDetail -SkippedBecause Custom -SkippedCustomReason 'Insufficient permissions to read Intune diagnostic settings in Azure.'
return $null
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using throw/catch [UnauthorizedAccessException] for expected authorization failures is inconsistent with other Azure diagnostic-settings tests (e.g., Test-MtCisaDiagnosticSettings uses Add-MtTestResultDetail -SkippedBecause NotAuthorized directly after checking StatusCode -ne '200'). Consider replacing the exception-based control flow with a direct Add-MtTestResultDetail -SkippedBecause NotAuthorized path (and optionally Write-Verbose the response content) to keep skip reasons consistent across reports.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need custom catch and write messages for each type?

While it's helpful to distinguish, I feel there could be more types of exceptions and our tests will have more error handling code than the actual business logic.

I'm thinking we should just leave it with a generic catch all and the error message itself will have the details.

For this specific test, I think it's okay to leave it since it's already written.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Copilot sometimes takes defensive code into edge case scenarios!

…gs.ps1

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

intune Microsoft Intune maester-test Related to a Maester test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🪲 MT.1100 (Intune Diagnostic Settings) fails without showing Azure ARM permissions error

4 participants