Run client app validation as the ambient operator identity - #491
Closed
Krishnadheeraj (DheerajPannala) wants to merge 1 commit into
Closed
Run client app validation as the ambient operator identity#491Krishnadheeraj (DheerajPannala) wants to merge 1 commit into
Krishnadheeraj (DheerajPannala) wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
documentation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Copilot started reviewing on behalf of
Krishnadheeraj (DheerajPannala)
September 1, 2026 16:49
View session
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes misleading client-app validation failures during setup requirements by distinguishing “confirmed absent” (empty successful Graph result) from “lookup inconclusive” (authorization/throttling/server/transport failures), and by ensuring the application existence probe uses the ambient bootstrap identity rather than authenticating as the app under validation.
Changes:
- Updated
ClientAppValidator.GetClientAppInfoAsyncto useGraphAuthenticationMode.Ambient, retry only on 401, and throw a new “application lookup failed” exception for non-proving failures. - Added
ClientAppValidationException.ApplicationLookupFailed(...)to preserve HTTP status/reason and provide targeted mitigation guidance (including 403 permission guidance). - Expanded unit tests to cover 403/401-retry behavior, malformed success payloads, status-0 failures, and ambient-vs-resolved token acquisition behavior; updated requirement-check messaging and added a changelog entry.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTests.cs | Adds regression tests ensuring Ambient mode does not use the resolved custom-client token, with a counter-test for ResolvedClientApp mode. |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs | Adds/updates coverage for “not found” vs “lookup failed” vs “token revoked”, including 403/429/5xx/status-0/malformed responses and cancellation vs timeout behavior. |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/Requirements/RequirementChecks/ClientAppRequirementCheck.cs | Removes “ensure the app exists” wording from failure details to avoid contradicting inconclusive-lookup scenarios. |
| src/Microsoft.Agents.A365.DevTools.Cli/Services/ClientAppValidator.cs | Switches the application lookup probe to Ambient auth, refines retry/exception behavior, and tightens response-shape validation. |
| src/Microsoft.Agents.A365.DevTools.Cli/Exceptions/ClientAppValidationException.cs | Introduces ApplicationLookupFailed exception factory with preserved status in context and targeted mitigations (notably for 403). |
| CHANGELOG.md | Adds a consumer-facing fixed entry for the corrected “app not found” misdiagnosis during Graph lookup failures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+270
to
+272
| // Re-fetch fresh app info and re-validate to confirm provisioning succeeded. | ||
| // A null result now means only that the app was deleted between the two reads; | ||
| // a failed re-read throws rather than silently keeping the stale verdict. |
Copilot started reviewing on behalf of
Krishnadheeraj (DheerajPannala)
September 1, 2026 23:32
View session
Krishnadheeraj (DheerajPannala)
changed the title
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The fix is well-scoped, aligns with the stated problem/mitigation, and is backed by strong regression and end-to-end test coverage (only minor follow-ups suggested).
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
1391
to
+1394
| using var doc = graphResponse.Json; | ||
| if (doc == null) return null; | ||
| var apps = doc is null ? null : JsonNode.Parse(doc.RootElement.GetRawText()) as JsonObject; | ||
| if (apps?["value"] is not JsonArray values) | ||
| { |
Comment on lines
+1375
to
1389
| if (graphResponse is null || !graphResponse.IsSuccess) | ||
| { | ||
| // 403, 429, 5xx, network failure and token-acquisition failure all leave the app's | ||
| // existence unknown. Reporting them as "not found" sends users to re-create an app | ||
| // that is already there. | ||
| var status = graphResponse is null | ||
| ? "Microsoft Graph application lookup returned no result." | ||
| : graphResponse.StatusCode > 0 | ||
| ? $"Microsoft Graph application lookup failed: HTTP {graphResponse.StatusCode} {graphResponse.ReasonPhrase}".TrimEnd() | ||
| : $"Microsoft Graph application lookup failed before a response was received: {graphResponse.ReasonPhrase}".TrimEnd(); | ||
|
|
||
| _logger.LogDebug("Graph app query failed with {StatusCode} — reporting lookup failure", graphResponse?.StatusCode ?? 0); | ||
| throw ClientAppValidationException.ApplicationLookupFailed( | ||
| clientAppId, tenantId, status, graphResponse?.StatusCode ?? 0); | ||
| } |
Copilot started reviewing on behalf of
Krishnadheeraj (DheerajPannala)
September 1, 2026 23:40
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Two newly added tests can currently pass under missing/invalid authentication headers or unsent requests, weakening the intended regression protection for the ambient-identity fix.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTests.cs:568
- This test can pass even if
GraphGetWithResponseAsyncnever sends a request (e.g., token acquisition fails and it returns theNoAuthfailure), because it only asserts that the custom-app token provider was not used. Queue a success response and assert at least one request was made / the call succeeded so the test actually guards ambient-mode behavior end-to-end.
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+44
to
+60
| if (request.Headers.Authorization?.Parameter == CustomAppToken) | ||
| { | ||
| RequestsAsCustomApp.Add(descriptor); | ||
| return Task.FromResult(new HttpResponseMessage(HttpStatusCode.Forbidden) | ||
| { | ||
| Content = new StringContent( | ||
| """{"error":{"code":"Authorization_RequestDenied","message":"Insufficient privileges to complete the operation."}}""", | ||
| Encoding.UTF8, "application/json") | ||
| }); | ||
| } | ||
|
|
||
| RequestsAsAmbient.Add(descriptor); | ||
| return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) | ||
| { | ||
| Content = new StringContent(BuildBody(url), Encoding.UTF8, "application/json") | ||
| }); | ||
| } |
Krishnadheeraj (DheerajPannala)
force-pushed
the
kpannala-microsoft-fix-custom-app-lookup
branch
from
September 1, 2026 23:44
0e6e483 to
1d42d9b
Compare
Once the bootstrap resolves a tenant-owned client app, every Microsoft Graph call in client app validation authenticated as that app, whose token defaults to User.Read. Reading application and servicePrincipal metadata needs Application.Read.All, so Graph refused all of them with 403 Authorization_RequestDenied. Two failures followed. The existence probe returned null on the 403 and the caller reported the app as not found, directing operators to re-create an app registration that was already present. Every later read failed the same way, so permission resolution returned nothing and a fully configured app was reported as missing all seven required permissions. Route the tenant-directory reads and repairs through the ambient bootstrap identity, the same one that already resolves the app by display name, and reserve a null lookup result for a successful response with no match. Authorization, throttling, server, transport and malformed-response outcomes now report an inconclusive lookup that preserves the HTTP status, and token revocation is reported only when a refreshed attempt is itself a 401. GraphGetAsync and GraphPatchAsync gain an authentication-mode parameter that defaults to the existing behavior, so no other caller changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Krishnadheeraj (DheerajPannala)
force-pushed
the
kpannala-microsoft-fix-custom-app-lookup
branch
from
September 1, 2026 23:57
1d42d9b to
d0ccfb2
Compare
Copilot started reviewing on behalf of
Krishnadheeraj (DheerajPannala)
September 1, 2026 23:57
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of correctness/message issues in the updated lookup/error paths that should be addressed before approval (see stored review comments).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+1423
to
+1427
| var displayName = app["displayName"] is JsonValue nameValue && nameValue.TryGetValue<string>(out var name) | ||
| ? name | ||
| : string.Empty; | ||
|
|
||
| var app = apps[0]!.AsObject(); | ||
| return new ClientAppInfo( | ||
| app["id"]?.GetValue<string>() ?? string.Empty, | ||
| app["displayName"]?.GetValue<string>() ?? string.Empty, | ||
| app["requiredResourceAccess"]?.AsArray()); | ||
| return new ClientAppInfo(objectId, displayName, app["requiredResourceAccess"] as JsonArray); |
Comment on lines
+66
to
+70
| var mitigationSteps = new List<string>(); | ||
| if (statusCode == 403) | ||
| { | ||
| mitigationSteps.Add($"Ask a tenant administrator to grant your account the '{AuthenticationConstants.ApplicationReadAllScope}' Microsoft Graph permission, or run the command as a Global Administrator or Application Administrator."); | ||
| } |
Rick Brighenti (rbrighenti)
approved these changes
Sep 2, 2026
|
Abandoning the changes here, in favor of the fix being combined with #478 where the issue is needed for different clouds. |
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.
Problem
When
setup requirementsfalls back to a tenant-owned custom client app, the resolved app ID is set onGraphApiService.CustomClientAppId. From that point on, every Microsoft Graph call in client app validation authenticated as the app being validated, andEnsureGraphHeadersAsyncdefaults that token request toUser.Read.Reading application and servicePrincipal metadata requires
Application.Read.All, so Graph answered403 Authorization_RequestDeniedto all of them. Two distinct failures followed:nullon the 403, and the caller translated that intoAppNotFound— telling operators their client app did not exist and directing them to re-create an app registration that was already present.PATCHwas attempted and refused.This reproduces in any tenant where the Agent 365 CLI application is not provisioned, so setup falls back to a tenant-owned app. The app is demonstrably present in that situation — the ambient bootstrap lookup already found it by display name on the same
/v1.0/applicationsendpoint, and MSAL had acquired a token as that client.Fix
Existence probe (
GetClientAppInfoAsync):GraphAuthenticationMode.Ambient. A token issued for the app under validation cannot prove that app's own absence, and requestingApplication.Read.Allfrom it would be circular when the app is missing or has not consented that scope.nullis reserved exclusively for a successful response with an emptyvaluearray — the only outcome that proves absence.NoAuth, transport failures and malformed responses throwClientAppValidationException.ApplicationLookupFailed, preserving the HTTP status and reason. A 403 can never be reported asAppNotFound.TokenRevokedis raised only when the refreshed attempt is itself a 401.HttpClienttimeout is a lookup failure, not a Ctrl+C.The rest of the validation flow: the 23 remaining tenant-directory reads and repairs (15 GETs, 6 PATCHes, 2 status-bearing GETs) now route through three small ambient helpers, so the whole flow runs as the ambient operator identity — the same identity that already resolves the app by display name.
GraphGetAsyncandGraphPatchAsyncgained anauthenticationModeparameter defaulting to the existingResolvedClientApp, so no other caller changes behavior.The
wids/scptoken-inspection paths deliberately keep using the custom-app token, since they exist to read claims that only that app's registration carries.ClientAppRequirementCheckno longer appends "ensure the app exists" to every validation failure, which contradicted the new guidance.Not changed
Untouched:
LookupServicePrincipalByAppIdWithResponseAsync,EnsureValidFirstPartyClientAppAsync,GraphAuthenticationModedefaults, first-party token scope validation, service-principal creation, fallback SP propagation, andRequirementsSubcommand(which still sets the resolved client app ID for later operations).Testing
Full CLI test project: 2034 passed, 12 skipped, 0 failed (2046 total, up from 2035). Release build: 0 warnings, 0 errors.
ClientAppValidatorAmbientIdentityTestsexercises the failing scenario end to end with a dual-identity HTTP handler: the ambient token receives 200, a custom-app token receives 403Authorization_RequestDenied. Against the pre-fix code the flow issued 1 ambient call and 8 forbidden ones and failed with "missing required API permissions"; it now issues 9 ambient calls, 0 forbidden, and validation succeeds. Both tests were verified to fail against the unfixed code with a clean rebuild.Unit coverage added for the probe: ambient lookup when
CustomClientAppIdis set; 403 reported as an authorization failure with the status preserved and never asAppNotFound; 401 then 403 on retry reported as a lookup failure rather than revocation; 200-with-empty-result as the only path toAppNotFound; 429 and 5xx preserving status without offering theApplication.Read.Allstep; status 0 preserving the reason phrase; five malformed-response shapes; ambient 401 force-refresh retry; transport failure and timeout; caller cancellation. PlusGraphApiServicetests pinning thatAmbientdoes not request a token forCustomClientAppId, paired with aResolvedClientAppcounter-test that it does.