Skip to content

Add GraphMissingPermissionsGuidancePlugin. Closes #88 - #2038

Open
waldekmastykarz wants to merge 10 commits into
dotnet:mainfrom
waldekmastykarz:detect-missing-graph-permissions
Open

waldekmastykarz wants to merge 10 commits into
dotnet:mainfrom
waldekmastykarz:detect-missing-graph-permissions

Conversation

@waldekmastykarz

@waldekmastykarz waldekmastykarz commented Oct 31, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #88

Adds GraphMissingPermissionsGuidancePlugin, a guidance plugin that explains Microsoft Graph 403 Forbidden responses caused by missing permissions.

How it works

  • Runs in AfterResponseAsync for watched Microsoft Graph requests that return 403, and for 403 responses inside $batch responses.
  • Reads delegated scopes (scp) or application roles (roles) from the bearer token on the request.
  • Retrieves the permissions that allow calling the endpoint from the Microsoft Graph DevX permissions API (the same service used by the existing GraphMinimalPermissions* plugins). Results are cached in memory per scope type + method + tokenized URL. Lookups time out after 3s; batch lookups run in parallel, and concurrent lookups for the same key are coalesced (failed lookups are retried later).
  • Skips responses generated by plugins (mocks).
  • Logs per request:
    • Failed when the token has none of the accepted permissions: lists the token's permissions, the least-privileged permission to add and higher-privileged alternatives (or unranked alternatives when DevX doesn't mark a least-privileged one), and instructions for delegated (app registration + requested scopes + consent) vs. application (app registration + admin consent + new token) permissions.
    • Warning when the token already has an accepted permission (403 is likely caused by something else: resource access, license/role, policy), when the token can't be read, or when the required permissions can't be determined.
  • For delegated /users/{id} requests that don't target the signed-in user (compared with the token's oid/upn/preferred_username), self-only permissions (User.Read, User.ReadWrite) aren't treated as sufficient, and the message explains they only cover the signed-in user (use /me).
  • Delegated tokens for personal Microsoft accounts (tid = consumers tenant) use DelegatedPersonal permissions.
  • For delegated requests to another user's mailbox, calendar or contacts, the guidance notes that the data must be shared and the app needs the matching .Shared permission (or application permissions).
  • When the URL can't be parsed (e.g. parenthesized key syntax /users('id')), logs a tip that the analysis couldn't run. Fixing GetTokenizedUrl for that syntax is tracked separately.
  • The access token is decoded only when there's a 403 to analyze.
  • No configuration.

Example:

 fail  ╰ GraphMissingPermissionsGuidancePlugin: Request failed with 403 Forbidden because the access token doesn't have the permissions required to call GET /me/messages. Delegated permissions on the token: openid, User.Read. Add the least privileged permission: Mail.ReadBasic. Alternatively, use one of the following higher-privileged permissions: Mail.Read, Mail.ReadWrite. To fix it, add the permission to the delegated permissions of your app registration in Microsoft Entra ID, include it in the scopes your app requests when acquiring the access token, and make sure the user (or an administrator, for permissions that require admin consent) consents to it.

Other changes

  • Extracted JWT permission parsing into GraphUtils.GetPermissionsFromAuthorizationHeader and reused it in GraphMinimalPermissionsGuidancePlugin (no behavior change).
  • Exposed the existing RespondedByPlugin flag on IProxySession so the plugin skips mocked responses.
  • ProxyUtils.IsGraphUrl now recognizes dod-graph.microsoft.us (fixes DoD-cloud detection for all Graph plugins).
  • Enabled the plugin in the microsoft-graph.json and m365.json presets.
  • Added the plugin to the skill reference docs.
  • Added hermetic tests (stubbed DevX API) covering delegated/application tokens, sufficient permissions, missing token, unknown endpoints, caching, lookup coalescing and retry, self-only permissions, unranked permissions, personal accounts, other-user sharing note, unparsable URLs, mocked responses, DoD host, non-403 responses and $batch.

Adds a guidance plugin that inspects Microsoft Graph responses with
403 Forbidden (including 403s inside $batch responses), compares the
permissions on the access token with the permissions required for the
endpoint (retrieved from the Microsoft Graph DevX permissions API and
cached in memory), and logs which permission to add and how.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 31, 2026 11:08
@waldekmastykarz
waldekmastykarz requested a review from a team as a code owner October 31, 2026 11:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The awaited DevX lookup can delay Graph response delivery and cause client timeouts, while DoD-cloud requests receive no guidance and mocked 403 responses can trigger misleading diagnoses.

3 open issues
What changed in this PR

Adds GraphMissingPermissionsGuidancePlugin to help explain Microsoft Graph 403 responses and recommend permission and consent changes, addressing issue #88.

Changes:

  • Adds cached permission lookups and guidance for individual requests and failed batch requests.
  • Shares JWT permission parsing with the existing minimal-permissions guidance plugin.
  • Enables the plugin in Graph and Microsoft 365 presets, documents it, and adds tests with a stubbed permissions API.
File Description
skills/​dev-proxy/​references/​plugin-catalog.md Lists the new guidance plugin.
skills/​dev-proxy/​references/​configuration.md Documents the plugin in the configuration reference.
DevProxy/​config/​microsoft-graph.json Enables the plugin in the Graph preset.
DevProxy/​config/​m365.json Enables the plugin for Graph URLs in the Microsoft 365 preset.
DevProxy.Plugins/​Utils/​GraphUtils.cs Extracts shared JWT permission parsing.
DevProxy.Plugins/​Reporting/​GraphMinimalPermissionsGuidancePlugin.cs Uses the shared permission parser.
DevProxy.Plugins/​Guidance/​GraphMissingPermissionsGuidancePlugin.cs Implements 403 analysis, caching, and permission guidance.
DevProxy.Integration.Tests/​GraphMissingPermissionsGuidancePluginTests.cs Covers token types, guidance, caching, skipped responses, and batches.

Review effort: Balanced

- Skip responses generated by plugins (expose RespondedByPlugin on IProxySession)
- Recognize dod-graph.microsoft.us in ProxyUtils.IsGraphUrl
- Lower DevX lookup timeout to 3s and analyze batch requests in parallel

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Parallel analysis can trigger duplicate DevX requests for the same uncached key, requiring in-flight lookup coalescing that preserves retries and isolates caller cancellation.

1 open issue
3 resolved since last review

Review effort: Balanced

Refs dotnet#88

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Permission guidance can incorrectly rank unmarked permissions and treat self-only User.Read as sufficient for another user’s profile, producing misleading recommendations.

No open issues 🎉

2 previously missed issues

In code that hasn't changed since last review

Medium severity Recommend User.ReadBasic.All for delegated access to other users

DevProxy.Plugins/​Guidance/​GraphMissingPermissionsGuidancePlugin.cs:170

For a delegated GET /users/{other-user-id}, DevX includes User.Read, but that permission only grants access to the signed-in user's profile. A token containing only User.Read therefore triggers the “another reason” warning instead of guidance to add User.ReadBasic.All. Account for self-only permissions in both the sufficiency check and the least-privilege recommendation, preserving them when the request targets the signed-in user.

Medium severity Do not rank permissions when least-privilege metadata is missing

DevProxy.Plugins/​Guidance/​GraphMissingPermissionsGuidancePlugin.cs:185

When DevX returns permissions without a least-privilege marker, this promotes the first result to “least privileged” without establishing its ranking. For example, a response ordered as User.Read.All, User.ReadBasic.All with both flags false recommends the broader permission and labels the narrower one as higher-privileged. Report the accepted permissions as unranked alternatives when the metadata cannot identify the least-privileged choice.

1 resolved since last review

Review effort: Balanced

Treat self-only delegated permissions (User.Read, User.ReadWrite) as
insufficient for /users/{id} requests and list permissions as unranked
alternatives when none is marked least privileged.

Refs dotnet#88

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@waldekmastykarz

Copy link
Copy Markdown
Collaborator Author

Addressed the 2 previously missed issues from the latest Copilot review in c5317ce:

  • Self-only delegated permissions for other users: for delegated /users/... requests, User.Read/User.ReadWrite no longer count as sufficient and aren't recommended. A User.Read-only token calling /users/{id} now gets guidance to add User.ReadBasic.All, plus a note that the self-only permissions only allow access to the signed-in user and that /me works for that case. /me requests are unaffected.
  • Missing least-privilege metadata: if DevX doesn't mark any permission as least privileged, the plugin now lists the accepted permissions as unranked alternatives ("Add any of the following permissions: …") instead of promoting the first one.

Added tests for both.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unconditionally excluding self-only permissions for /users/{id} can misdiagnose requests targeting the signed-in user and recommend unnecessary directory-wide permissions, requiring identity-aware guidance and coverage.

1 open issue

Review effort: Balanced

Refs dotnet#88

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Human assessment is needed because valid parenthesized Graph URLs can trigger a parsing failure that silently suppresses permissions guidance for watched 403 responses.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

Refs dotnet#88

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@waldekmastykarz

Copy link
Copy Markdown
Collaborator Author

Addressed the note in the latest Copilot review summary (parenthesized Graph URLs such as /users('id') make GraphUtils.GetTokenizedUrl fail, which silently skips guidance) in 9b45753:

  • When tokenizing the URL fails, for a single request or a $batch inner request, the plugin now logs a visible tip saying it couldn't analyze missing permissions for that URL and pointing to the Graph API reference, instead of only writing a debug message. Added tests for single and batch requests.
  • Out of scope: fixing GetTokenizedUrl itself to support parenthesized key syntax. It's shared with the GraphMinimalPermissions* plugins and will be tracked in a separate follow-up issue.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Unpublished findings concerning unnecessary JWT processing and incorrect self-only permission guidance require human assessment before approval.

No open issues 🎉

Review effort: Balanced

Refs dotnet#88

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@waldekmastykarz

Copy link
Copy Markdown
Collaborator Author

Re: the latest Copilot review summary (unpublished findings):

  • Unnecessary JWT processing: fixed in da0ed08. The access token is now decoded only when there's a 403 to analyze (a direct 403 or a batch with forbidden inner responses), and user identity claims are read only for delegated tokens. Before, every watched Graph response decoded the token twice.
  • Self-only permission guidance: the summary doesn't give specifics, so I requested another review to get the details.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Human assessment is needed because an unpublished finding indicates personal-account requests may receive unusable work/school permission recommendations.

No open issues 🎉

Review effort: Balanced

Refs dotnet#88

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@waldekmastykarz

Copy link
Copy Markdown
Collaborator Author

Re: the latest Copilot review summary (an unpublished finding that personal-account requests may get unusable work/school recommendations). Fixed in aa6d0b0: delegated tokens issued to personal Microsoft accounts (tid = consumers tenant 9188040d-6c67-4c5b-b112-36a304b66dad) now look up scopeType=DelegatedPersonal permissions from the DevX API instead of DelegatedWork. Added a test.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

An unpublished moderate-severity finding involving misleading permission guidance for access to another user warrants human assessment.

No open issues 🎉

Review effort: Balanced

…s guidance

Refs dotnet#88

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@waldekmastykarz

Copy link
Copy Markdown
Collaborator Author

Re: the latest Copilot review summary (an unpublished finding about misleading guidance for access to another user). Addressed in 64ff737: for delegated /users/{id}/... requests that don't target the signed-in user (or when the identity is unknown), the DevX API lists permissions like Mail.Read/Calendars.Read, but these only cover data the signed-in user can access. The guidance (both the missing-permission failure and the 'another reason' warning) now adds a note: to access another user's data, that user must share it with the signed-in user (using .Shared permissions where available), or the app must use application permissions. Added tests for the other-user and signed-in-user cases.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The guidance incorrectly presents sharing as a prerequisite for all nested /users/{id} resources and must limit that advice to mailbox/calendar access.

1 open issue

Review effort: Balanced

Refs dotnet#88

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Human assessment is needed because the permission guard can incorrectly treat self-only delegated permissions as sufficient for another user’s request, producing misleading guidance.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@waldekmastykarz

Copy link
Copy Markdown
Collaborator Author

Re: the latest Copilot review summary (an unpublished finding that self-only delegated permissions may be treated as sufficient for another user's request). Not changing this. For other-user mailbox, calendar and contacts requests, a token that has e.g. Mail.Read gets the 'likely failed for another reason' warning, and that warning now includes the note that the other user must share the data and the app needs the matching .Shared permission (or application permissions). That covers this case. For /users/{id} profile requests, User.Read/User.ReadWrite are already excluded unless the ID matches the signed-in user. This theme has come up repeatedly in recent rounds, so I'm treating it as addressed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Detect missing permissions

2 participants