Add a365 network gsa enable|disable|status - #497
Lala Sushant Srivastava (lasrivas) wants to merge 9 commits into
Conversation
The documented subnet-injection flow ends with Enable-SubnetInjection, which takes the id of the Power Platform environment to link. Agent 365 provisions a managed environment per tenant and does not publish its id, so admins cannot finish the flow. These subcommands replace that final step: the platform resolves the environment server-side and performs the link. The policy systemId read stays here rather than in the platform. It is a plain ARM GET against a resource the admin already owns, and doing it client-side with the admin's own az login avoids giving the platform a delegated ARM consent grant it otherwise has no need for. Co-authored-by: Copilot <[email protected]>
Global Secure Access is a per-environment Power Platform setting, and Agent 365 does not publish the id of the managed environment it provisions, so the admin surfaces that take an environment id cannot reach it. The platform resolves the environment and applies the change; these commands carry no environment identifier at all. Two things that are not obvious from the diff: Power Platform applies the change asynchronously but issues no operation id for it, so unlike vnet there is no handle to poll. The CLI converges by re-reading the setting, which is why status takes no --operation-id. NotConfigured is reported distinctly from Disabled. A tenant that has never set the value has not turned it off, and the distinction changes what an admin should do next. Co-authored-by: Copilot <[email protected]>
GsaService asked for a token with a login hint but no tenant, so the authority stayed `common`. The Windows broker ignores the hint in that case and returns whichever account Windows prefers; the resulting UPN mismatch is only logged at Debug, so a tenant-wide setting could be applied to the wrong tenant without any visible warning. Passing the tenant also arms the existing mismatch self-heal in AuthenticationService, which is inert while tenantId is null. Resolve both tenant and user from a single `az account show` via IAzureCliService - the same source `vnet link` already uses - rather than adding a --tenant-id option the user would have to keep in sync with their az context. No az login, or an account with no tenant, now fails with a clear message instead of silently guessing. Co-authored-by: Copilot <[email protected]>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved tenant-targeting, cancellation, validation, and VNet polling issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds tenant-level GSA enable, disable, and status commands alongside supporting VNet networking functionality.
Changes:
- Adds GSA services, models, authentication, and convergence polling.
- Registers network commands, handlers, and dependency injection.
- Adds tests, documentation, and release notes.
File summaries
| File | Reviewed change |
|---|---|
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/VNetLinkServiceTests.cs |
VNet service tests |
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GsaServiceTests.cs |
GSA service tests |
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ArmApiServiceTests.cs |
ARM policy lookup tests |
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NetworkCommandTests.cs |
Network command tests |
src/Microsoft.Agents.A365.DevTools.Cli/Services/VNetLinkService.cs |
VNet operations and polling |
src/Microsoft.Agents.A365.DevTools.Cli/Services/IVNetLinkService.cs |
VNet service contract |
src/Microsoft.Agents.A365.DevTools.Cli/Services/IGsaService.cs |
GSA service contract |
src/Microsoft.Agents.A365.DevTools.Cli/Services/GsaService.cs |
GSA API operations and polling |
src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs |
Enterprise policy resolution |
src/Microsoft.Agents.A365.DevTools.Cli/Program.cs |
Command and service registration |
src/Microsoft.Agents.A365.DevTools.Cli/Models/VNetModels.cs |
VNet models |
src/Microsoft.Agents.A365.DevTools.Cli/Models/GsaModels.cs |
GSA response models |
src/Microsoft.Agents.A365.DevTools.Cli/Constants/CommandNames.cs |
Network command constant |
src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs |
Network, VNet, and GSA command tree |
docs/commands/README.md |
Command index entries |
docs/commands/network.md |
VNet command documentation |
docs/commands/network-gsa.md |
GSA command documentation |
CHANGELOG.md |
Release notes |
Review details
Suppressed comments (7)
src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs:260
- The new enable/disable handlers are not exercised through
Command.InvokeAsync; the tests only parsegsa enableand callReportGsaAsyncdirectly. A wiring regression could leaveSetAsyncuncalled or lose the handler's exit-code propagation while all tests stay green; add invocation tests for both verbs covering service calls and success/failure results.
var result = await gsaService.SetAsync(enabled, ct);
context.ExitCode = await ReportGsaAsync(logger, gsaService, result, wait, enabled, ct);
src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs:287
- The new GSA status handler is also only covered by parse/tree assertions; no test invokes it to verify that
GetStatusAsyncis called and that null responses produce exit code 1. Add an invocation test for the success and request-error branches so handler wiring is covered, not justLogGsaStatus/ReportGsaAsync.
var status = await gsaService.GetStatusAsync(ct);
if (status == null)
{
context.ExitCode = 1;
return;
}
LogGsaStatus(logger, status);
context.ExitCode = 0;
src/Microsoft.Agents.A365.DevTools.Cli/Commands/NetworkCommand.cs:111
--tenant-idis anOption<string?>; an explicitly supplied empty or whitespace value enters this branch and is silently replaced with the currentaztenant. That makes a malformed explicit target indistinguishable from omission and can apply a tenant-wide link to the wrong tenant. Detect whether the option was supplied and reject blank values with a targeted error before falling back.
if (string.IsNullOrWhiteSpace(tenantId))
{
var account = await azureCliService.GetCurrentAccountAsync();
tenantId = account?.TenantId;
if (string.IsNullOrWhiteSpace(tenantId))
src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs:328
- When the command token is cancelled during the policy GET or response read, RetryHelper rethrows the cancellation, but this catch converts it to
null.LinkAsyncthen reports an ordinary policy-resolution failure instead of honoring Ctrl+C; rethrowOperationCanceledExceptionwhenctis cancelled before the catch-all.
catch (Exception ex)
{
if (NetworkHelper.IsConnectionResetByProxy(ex))
_logger.LogWarning(NetworkHelper.ConnectionResetWarning);
else
src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs:276
policyArmIdis CLI-controlled and is concatenated directly into the ARM URL after only a whitespace check. A value containing query, fragment, or path-traversal syntax can alter the resource orapi-versionportion of the request; validate it as a well-formed/subscriptions/.../providers/Microsoft.PowerPlatform/enterprisePolicies/...resource ID before constructing this URL.
var url = $"{ArmBaseUrl}{policyArmId}?api-version={apiVersion}";
src/Microsoft.Agents.A365.DevTools.Cli/Services/ArmApiService.cs:269
tenantIdis a required non-nullable parameter, but it is passed toEnsureArmHeadersAsyncwithout a whitespace guard. A blank value can fall through to common-tenant authentication and defeat the tenant-targeted ARM read; reject it before the first use.
string tenantId,
CancellationToken ct = default)
{
if (string.IsNullOrWhiteSpace(policyArmId))
throw new ArgumentException("Policy ARM id is required.", nameof(policyArmId));
if (!await EnsureArmHeadersAsync(tenantId, ct))
src/Microsoft.Agents.A365.DevTools.Cli/Services/GsaService.cs:120
- The account lookup validates only
TenantId; anaz account showresult with a missing user name is passed as an emptyuserId, which disables the user hint and lets the broker select another cached account. For a tenant-wide setting, reject a missing user name before token acquisition so the selected tenant and user are both enforced.
if (account is null || string.IsNullOrWhiteSpace(account.TenantId))
{
_logger.LogError("Could not determine your Azure tenant. Run 'az login' and try again.");
return null;
- Files reviewed: 18/18 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Rick Brighenti (rbrighenti)
left a comment
There was a problem hiding this comment.
Requesting changes. The ARM URL construction lets the bearer token be sent to an arbitrary host and needs fixing before merge.
The PR also adds network vnet link/unlink/status and ARM enterprise policy resolution, which aren't in the title. Please retitle, or split the VNet part out, since most of the findings are there.
Carries the vnet fixes from #494 (this branch contains that change set) and applies the same treatment to the gsa subcommands. Pin the enterprise-policy ARM id to its expected shape before concatenating it onto the ARM base URL. The base has no trailing slash and the ARM bearer token is a default request header, so `--policy-arm-id "@evil.example/x"` produced `https://[email protected]/x` -- userinfo, not host -- and sent the token to the attacker. Also: - Treat `NotStarted` as in-flight, matching what network.md documents. - Acquire the Agent 365 token for the resolved tenant rather than the signed-in default. `vnet unlink` and `vnet status` gain `--tenant-id` so they can do the same; the gsa commands already resolve the az-login tenant themselves. - Confirm before `vnet link --swap`, `vnet unlink`, `gsa enable` and `gsa disable`, with `--yes` for automation. Plain `vnet link` is not gated: a different existing link is reported as a conflict rather than replaced. - Reject an explicitly blank `--tenant-id` instead of silently falling back. - Use `CommandNames.Network` rather than a literal. - Drive every handler through `InvokeAsync` in tests. The previous doc comment claimed `ReportAsync` covered them, but tenant resolution, confirmation, service calls and exit codes were untested. - Stop the VNet tests shelling out to `az account show` via `AzCliHelper`'s static cache, using the repo's existing `loginHintResolver` seam. - Trim the CHANGELOG entries and reference #494 and #497. Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate correctness and timeout/cancellation issues remain unresolved, along with changelog nits.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (3)
Resolved since last review (5)
This token request omits the tenant that the command selected for the ARM read, so…network.mddocumentsNotStartedas an in-flight status, but this predicate only treats… This only checks thatgsa enable --waitparses; none of the three newSetHandlerblocks is…CommandNames.Networkwas added for centralized command names, but this registration still… This Unreleased entry is several sentences and includes implementation details and rationale; it…
…ogin prerequisite The --wait ceiling only bounded the gap between completed polls, so a poll starting just inside the budget could run to the HttpClient's own timeout and overshoot by minutes. The ceiling is now armed on the token each request is made with; a timeout mid-request reports the last known state, while a caller's Ctrl+C still propagates. ArmApiService's broad catch swallowed the OperationCanceledException that RetryHelper deliberately rethrows, so Ctrl+C during the policy read surfaced as "could not read the policy" and link carried on as though the policy did not exist. Both test helpers configured GetAccessTokenAsync without a matcher for its 8th parameter, the CancellationToken, pinning the setup to ct == default. Any call carrying a real token missed the setup and returned null, which the services report as a failed token acquisition -- so a test could not exercise any cancellation path at all. This is why the two new cancellation tests initially failed for the wrong reason. docs: the az login prerequisite claimed it was used "only to read the enterprise policy". It is actually the source of two defaults, the tenant and the signed-in account, and --tenant-id overrides only the first. Tokens are never borrowed from Azure CLI -- both the ARM read and the Agent 365 call acquire their own. Co-authored-by: Copilot <[email protected]>
Every convergence poll re-entered SendAsync, which shells out to `az account show` and returns null on any CLI hiccup. A transient failure part-way through --wait therefore aborted with "could not determine your Azure tenant" even though the tenant had been resolved successfully moments earlier. The account is now resolved once and reused; a failed resolution is not cached, so an admin who runs `az login` after the first attempt does not have to restart the process. WaitForStatusAsync had the same unbounded in-flight poll as the vnet wait: the stopwatch only bounded the gap between completed polls, so a poll starting inside the budget could run to the HttpClient's timeout. The ceiling is now armed on the request token, and a caller's Ctrl+C is still distinguished from a timeout. Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Reuse one Azure CLI account snapshot for confirmation and authentication to prevent tenant mismatches and transient failures.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
This branch was cut to include PR #494's virtual network work, so the two PRs duplicated roughly 1400 lines and every VNet review finding had to be answered twice. GSA never depended on any of it: GsaService talks to the platform directly and shares only the tenant resolution and confirmation helpers in NetworkCommand, which both features need. Removed VNetLinkService, IVNetLinkService, VNetModels and their tests, reverted ArmApiService and its tests to main, dropped the vnet subcommand tree along with ReportAsync and LogStatus, and removed the vnet entries from the CHANGELOG and the docs index. NetworkCommand.CreateCommand and its test helper lose the IVNetLinkService parameter. The two branches now both define the network root command and the shared helpers, so whichever merges second will conflict there. That is a smaller price than reviewing the same code on two PRs. 2063 passed, 0 failed, 12 skipped.
|
The virtual network change set has been removed from this PR, so it now carries only the GSA work. This branch was originally cut to include #494, which meant roughly 1400 lines appeared on both PRs and every VNet review finding had to be answered twice. GSA never depended on any of it: Removed in d923c2a: Review the VNet code on #494 instead; it is unchanged there. One consequence worth flagging: both branches now define the 2063 passed, 0 failed, 12 skipped. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Several moderate service reliability and error-handling issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (5)
Avoid duplicate account lookup causing failures or tenant mismatch Reuse account snapshot to prevent applying settings to the wrong tenant Validate tenant before caching Azure account · New Handle HttpClient timeouts as documented failures · New Correct inaccurate Azure CLI account lookup prerequisites · New
The set handler read az account show to name the tenant in the confirmation prompt, and GsaService read it again to pick the tenant it authenticated against. az account show reflects mutable local state, so the two reads could disagree: the command could confirm tenant A and apply the tenant-wide setting to tenant B, and a transient CLI failure between them could fail a command whose tenant had already been resolved. GsaService is now told which account to act as. It no longer depends on IAzureCliService, and the account cache goes with it. That cache had its own bug: ??= stored any non-null account before its tenant was validated, and AzureAccountInfo defaults TenantId to string.Empty, so a blank-tenant account was cached permanently and never retried after a later az login. SendAsync also rethrew every OperationCanceledException. HttpClients own timeout surfaces as one with no token cancelled, so a bare enable, disable or status threw at the caller instead of returning the documented null. It now rethrows only when the supplied token is actually cancelled, which still covers both a Ctrl+C and the wait ceiling firing on its linked token. Also fixed the network-gsa.md prerequisite, which claimed nothing is read from Azure and repeated half a sentence after an earlier edit. 2065 passed, 0 failed, 12 skipped.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical and moderate service-handling issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (5)
CreateAuthenticatedClient built the client with HttpClient's default handler ownership, so a caller that holds one handler and builds a client per request lost the handler to the first client's disposal. GsaService is exactly that shape, so the second poll of WaitForStatusAsync would have failed with ObjectDisposedException against any handler that honours Dispose. A supplied handler now stays owned by whoever supplied it.
CreateAuthenticatedClient built the client with HttpClient's default handler ownership, so a caller that holds one handler and builds a client per request lost the handler to the first client's disposal. VNetLinkService is exactly that shape, so the second poll of WaitForCompletionAsync would have failed with ObjectDisposedException against any handler that honours Dispose. A supplied handler now stays owned by whoever supplied it. Found on #497, which carries the identical factory; not flagged here.



Adds
a365 network gsa enable|disable|status, which turns Global Secure Access on or off for the tenant's Agent 365 environment.Global Secure Access is a per-environment Power Platform setting, so configuring it normally needs the environment's id. Agent 365 does not publish that id, so the CLI asks the platform to apply the setting to the environment it resolves for the tenant. The platform half shipped in bic/MCP-Platform#3686.
Design
docs/superpowers/specs/2026-09-15-gsa-environment-setting-design.md.Notable decisions
NotConfiguredis reported distinctly fromDisabled. A tenant that has never set the value has not turned it off, and the distinction changes what an admin does next, sostatussays so in as many words.--waitpolls for the requested value, with a ten-minute ceiling armed on the request token so an in-flight poll cannot overshoot it.--tenant-id. The commands authenticate against the tenant of your currentaz login. The command resolves that account once and hands it to the service, so--waitcannot confirm one tenant and change another, and a transientazfailure part-way through a wait does not abort a change that already succeeded.enableanddisableprompt before applying, naming the tenant, and take--yesto skip.statusis read-only and does not prompt.Scope
This PR no longer carries the virtual network change set. It was originally branched to include #494's work, which duplicated roughly 1400 lines across the two PRs and meant every VNet review finding had to be answered twice. GSA never depended on it —
GsaServicetalks to the platform directly and shares only the tenant-resolution and confirmation helpers inNetworkCommand. Removed in d923c2a.Both branches now define the
networkroot command and those shared helpers, so whichever merges second will conflict there. That is a smaller price than reviewing the same code twice.Review rounds
Three rounds of review are folded in. The first (16ef241) covered ARM-shape validation,
NotStartedas in-flight, tenant-targeted tokens,--yesconfirmation onenableanddisable, handler-body test coverage and the CHANGELOG entry. The second (71a6570, ae03dfe) armed the--waitceiling on the request token and corrected theaz loginprerequisite in the docs.Third round (94e71aa, a15fac1)
az account showto name the tenant in the confirmation prompt, thenGsaServiceresolved it again to authenticate. Anaz account setbetween the two, or any mutation of CLI state mid-run, meant confirming tenant A and changing tenant B.IGsaServicenow takes the resolvedAzureAccountInfoas its first parameter, which also let the service drop itsIAzureCliServicedependency and its account cache -- that cache was itself unsound, becauseAzureAccountInfo.TenantIddefaults tostring.Emptyrather than null, so a??=cached a blank-tenant account forever instead of retrying.SendAsyncno longer rethrows HttpClient's own timeout as cancellation. It surfaces as anOperationCanceledExceptionwith no token cancelled, so a bareenable,disableorstatusthrew at the caller instead of returning the documented null and logging the failure. Now rethrows only when the supplied token is actually cancelled, which still covers Ctrl+C and the wait ceiling firing on its linked token.HttpClientno longer disposes a handler it does not own.HttpClientFactory.CreateAuthenticatedClientbuilt the client asnew HttpClient(handler), which defaults todisposeHandler: true, so a service that holds one handler as a field and builds a client per request lost that handler to the first client's disposal. Every later request, including every poll after the first, would have failed withObjectDisposedExceptionagainst any handler with real disposal semantics. A supplied handler now stays owned by the supplier.docs/commands/network-gsa.mdprerequisite paragraph rewritten; the split in d923c2a had left a duplicated fragment mid-sentence.Tests
Full suite: 2068 passed, 0 failed, 12 skipped.