Repository navigation
Add a365 network gsa enable|disable|status - #497
Lala Sushant Srivastava (lasrivas) wants to merge 1 commit into
Conversation
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://management.azure.com@evil.example/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 <223556219+Copilot@users.noreply.github.com>
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…
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)
|
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
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. 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.
Krishnadheeraj (DheerajPannala)
left a comment
There was a problem hiding this comment.
Thanks, the account-once threading is a real fix and the GSA flow reads well. I checked the contract against the platform change this calls (MCP-Platform PR 3686, merged) and built and ran the suite on this branch and merged onto current main (builds; all tests pass). Details are inline; the summary:
Before merge
--waitcan report the old value as success (inline onNetworkCommand.cs): the platform'sstatusresponse never setspending, so after a timed-out wait the command prints the old value and exits 0 with no "still being applied" hint. The tests covering this path feedpending: trueinto status responses, which the platform does not send.- Scopes (please confirm): the platform rejects any token without the exact scope (
AgentTools.Gsa.Manage.Allfor enable/disable,AgentTools.Gsa.Read.Allfor status) with403 insufficient_scope. The token is{Agent 365 Tools app}/.defaultfrom the Azure PowerShell public client (no client id is passed), so both scopes need to be exposed on the Agent 365 Tools app and pre-authorized for that client, in every cloud the CLI targets. - Rebase onto
main: needed for #478 (cloud-aware endpoints; inline onGsaService.cs). Also, this branch still carries #494's original commit1318bd50plusd923c2a, which deletes those files. A squash merge nets that out, but with a merge commit (enabled in this repo) the two branches share1318bd50as a common ancestor, so whichever merges second hits modify/delete conflicts on the VNet files, and resolving them toward this branch deletes the VNet feature. Rebasing ontomain(ideally dropping that pair) removes the shared ancestor. - Interaction with #494: with squash merges, whichever lands second conflicts in
NetworkCommand.cs,Program.cs,NetworkCommandTests.cs,docs/commands/README.mdandCHANGELOG.md(reproduced in both orders). The semantics also diverge under onenetworkparent:vnethas--tenant-idand does not confirm a plainlink;gsahas no--tenant-idand confirms both verbs.
Verified and looks right
- Resolving the
azaccount once and passing it down removes the prompt/token tenant split, and the tests pin it. - HttpClient's own timeout returns the documented
null, while a caller's cancellation and the wait ceiling still propagate. disposeHandler: falseinHttpClientFactoryis the right ownership model.- Routes and shapes match the platform (
POST /agents/gsa/enable|disable,GET /agents/gsa/status;status/pending/reason;error), andNotConfiguredstays distinct fromDisabled.
Nit: Program.cs creates the logger with the literal "network", and CommandNames.Network exists.
Rebuild the GSA-only change set on current main, dropping shared VNet history. Require --wait to observe the requested value, reject unreadable success responses and unsupported clouds, retry transient reads, and distinguish scope errors from role failures. Clarify prompts, pending output, prerequisites and release notes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0ea31405-e8a4-41a7-b9ec-13e2f66d65a7
a15fac1 to
c7ef788
Compare
There was a problem hiding this comment.
🟡 Changes recommended
GSA can ignore config-only sovereign-cloud authority overrides, trigger unrelated authentication before cloud validation, and execute mutations as an administrator other than the confirmed user.
3 open issues
1 resolved since last review
Review effort: Balanced
| rootCommand.AddCommand(NetworkCommand.CreateCommand( | ||
| networkLogger, azureCliService, gsaService, confirmationProvider)); |
| services.AddSingleton<IGsaService>(provider => new GsaService( | ||
| provider.GetRequiredService<ILogger<GsaService>>(), | ||
| provider.GetRequiredService<IAuthenticationService>(), | ||
| provider.GetRequiredService<IAgent365ToolingService>().Environment)); |
| var authToken = await _authService.GetAccessTokenAsync( | ||
| audience, account.TenantId, userId: account.User.Name, ct: cancellationToken, | ||
| authorityHost: authorityHost); |



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. Agent 365 does not publish that environment's 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.
Behavior
NotConfiguredstays distinct fromDisabled.--wait, an accepted change returns 0 and labels the requested and current values separately.--wait, success means the requested value was observed. Timeout or failed verification returns 1, explicitly states that the change was accepted, and points toa365 network gsa status. The ten-minute ceiling also bounds in-flight requests and retry backoff.--yes; status is read-only.required_scopeseparately from administrator-role failures.Usage and prerequisites: network-gsa.md, including
az login --tenant <id> --allow-no-subscriptionsfor administrators without Azure subscriptions. An optional--tenant-idexpansion is intentionally not included.Integration
Rebuilt the GSA-only net change directly on current main (
2a3245d), removing the original VNet commit and its later deletion from this branch's history. This avoids the shared-ancestor modify/delete problem with #494. Whichever PR lands second must still preserve both subcommand trees when resolving additive command-registration, test, and documentation conflicts. The docs index usesnetwork gsa, not a duplicatenetworkrow.Platform deployment prerequisite
The Agent 365 Tools API must expose
AgentTools.Gsa.Manage.AllandAgentTools.Gsa.Read.Alland preauthorize them for the CLI's Microsoft client application. This registration/deployment prerequisite cannot be confirmed by local unit tests and still needs platform-owner confirmation. No platform app registrations were changed by this PR.Verification
tests.projrequired an unavailable traversal SDK, so the equivalent full solution suite was run instead.