diff --git a/.claude/agents/pr-code-reviewer.md b/.claude/agents/pr-code-reviewer.md index 54e86eff..a6f13a91 100644 --- a/.claude/agents/pr-code-reviewer.md +++ b/.claude/agents/pr-code-reviewer.md @@ -215,6 +215,56 @@ For each changed file, analyze: - **Fix pattern**: replace `context.ExitCode = 1;` with `context.ExitCode = ex.ExitCode;` so the exit code is always authoritative from the exception definition - **Real example (PR #406, Comments 5 & 6)**: `ConfigFileNotFoundException.ExitCode => 2`, but two catch blocks in `AllSubcommand.cs` hardcoded `context.ExitCode = 1`. Scripts expecting exit code 2 on configuration errors received 1 instead. + **E. Static scope array emptied or scopes removed → verify callers and document intent** + When the diff sets a static `string[]` scope array to `[]` or removes entries from it (e.g., `RequiredPermissionGrantScopes`, `RequiredS2SGrantScopes`): + - Run `Grep` for every reference to the array across non-test source files + - For each call site that passes the array as `scopes` to `EnsureGraphHeadersAsync` or equivalent: + - Read the `hasScopes = scopes?.Any() == true` guard logic — confirm the empty-array fallback path still acquires a valid token via `GetGraphAccessTokenAsync` (the standard `AuthenticationService` path) + - Confirm the operations that previously needed those scopes are still covered by `RequiredClientAppPermissions` or by a PowerShell fallback + - Check that the XML doc on the constant explains WHY it is empty and names the fallback behavior + - Severity: **MEDIUM** if XML doc doesn't explain empty state; **HIGH** if fallback path would fail in practice + + **F. Runtime `Contains()` on a static compile-time array → verify element IS in the array** + When the diff adds `SomeStaticArray.Contains(SomeConstant)` as a guard condition: + - Read `SomeStaticArray`'s initializer — confirm whether `SomeConstant` is actually present + - If `SomeConstant` is intentionally absent (disabled feature, future path), the condition is always `false`: + - Verify the `else` branch handles the always-false case correctly and critical functionality is not silently skipped + - Require a comment that makes the always-false state explicit AND states the consequence (e.g., "this means agent user cleanup is always skipped until create-instance is re-enabled") + - Severity: **MEDIUM** if consequence is limited scope; **HIGH** if critical functionality (e.g., cleanup of real resources) is silently disabled with no user-visible warning + - **Real example (PR #409)**: `RequiredClientAppPermissions.Contains(AgentIdUserReadWriteAllScope)` is always `false` because that scope is intentionally absent — agent user queries and cleanup always skip silently + + **G. XML/inline comments claiming permission scope semantics → verify against actual scope requirements** + When the diff adds or modifies `///` XML comments (especially in test files) that describe what a permission scope covers (e.g., "umbrella — includes SP creation", "covers X, Y, Z"): + - Cross-check the claim against `RequiredClientAppPermissions` and the PR's own changes + - Specifically: if the PR adds a *separate* scope for an operation (e.g., `AgentIdentityBlueprintPrincipal.Create` for SP creation), any comment claiming that operation is "covered by the umbrella" is now wrong + - Flag **MEDIUM** for inaccurate scope coverage claims in comments — these mislead future reviewers and generate follow-up PR comments + - **Real example (PR #409)**: XML comment said `AgentIdentityBlueprint.ReadWrite.All` "includes SP creation" but the same PR requires `AgentIdentityBlueprintPrincipal.Create` separately for SP creation + + **H. JWT claim decoded → verify the token was issued by the app registration that has the claim configured** + When the diff adds code that decodes a JWT and reads a claim that is configured *per app registration* + (e.g., `wids`, `roles`, `groups`, any custom optional claim from Entra Token Configuration): + - Trace backward from the decoder to the token-acquisition call. Identify which clientId issues the token. + - If acquisition goes through `AuthenticationService.GetAccessTokenAsync(...)` without an explicit + `clientId` argument, the token is from the PowerShell well-known clientId + (`AuthenticationConstants.PowershellClientId`) — **not** the custom client app. Optional claims + configured on the custom client app's manifest will NOT be in this token. + - The correct path for tokens that must carry custom-app optional claims is + `IMicrosoftGraphTokenProvider.GetMgGraphAccessTokenAsync(..., clientAppId: CustomClientAppId, ...)`. + - Flag **HIGH** when the decoder relies on a claim that the issuing app doesn't have configured. + The bug is silent: the decoder returns "claim absent → Unknown / DoesNotHaveRole", and the caller + proceeds with the conservative path (e.g., admin treated as non-admin → admin URL printed even + for actual admins). Functional but degraded UX, easy to miss in passing tests because mocks + usually return a hand-crafted JWT regardless of which auth path is used. + - Also apply this rule to **empty static scope arrays** (related to Rule E): when an empty + `IEnumerable` is passed to a helper that routes "no scopes" through the legacy + `AuthenticationService` path, the token will be from the PowerShell clientId. If any downstream + code reads custom-app optional claims, the routing is wrong. + - **Real example (PR #409)**: `CheckDirectoryRoleAsync` decoded `wids` from a token acquired via + `GetGraphAccessTokenAsync` → `AuthenticationService` → PowerShell clientId. `wids` was + configured on the custom client app only, so the JWT never carried it and the method always + returned `Unknown`. Fix: route through `_tokenProvider` with `CustomClientAppId` and a minimal + scope (`User.Read`). + **D2. Cross-method "catch-returns-null / caller-sets-ExitCode" pattern — same exit-code problem, harder to spot** Rule 9-D catches *inline* catch blocks. This variant spans two methods and is easy to miss: ```csharp diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ed958348..448bfc30 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,6 +13,7 @@ jobs: dotnet-sdk: name: .NET SDK runs-on: ubuntu-latest + timeout-minutes: 20 permissions: contents: write defaults: @@ -52,7 +53,7 @@ jobs: run: dotnet build tests.proj --no-restore --configuration Release - name: Run tests - run: dotnet test tests.proj --no-build --configuration Release --verbosity normal + run: dotnet test tests.proj --no-build --configuration Release --verbosity normal --blame-hang --blame-hang-timeout 5min - name: Pack NuGet packages id: pack diff --git a/.gitignore b/.gitignore index e769f62b..af194ac0 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,6 @@ # Internal working documents docs/plans/ +docs/min-permissions/ docs/Permissions-Review.md docs/Testing.md scripts/skills/ diff --git a/CHANGELOG.md b/CHANGELOG.md index c1acd0cc..1d68df41 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,7 +41,14 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g - Messaging endpoint row added to `a365 setup all` summary output, with "registered"/"reused"/"skipped (non-M365)"/"manual config required"/"failed" states. When registration can't complete, the summary surfaces an "Action Required" entry with the Teams Developer Portal URL so the user knows exactly what to do next. - Defensive fallback when the server rejects the new request with a known contract-mismatch signature — the CLI logs `"Automated messaging endpoint registration is not available for this tenant yet. You'll need to configure it manually."` and directs the user to the Teams Developer Portal. Same user-facing path is reused when registration fails because the signed-in user is not a blueprint owner. +### Changed +- Agent identity creation now uses Blueprint app-only credentials (`AgentIdentity.CreateAsManager`, auto-granted to all Blueprint apps). The custom CLI app no longer requires `AgentIdentity.Create.All` or `DelegatedPermissionGrant.ReadWrite.All`. Administrators can remove these permissions from the CLI app registration. See [Custom client app registration](https://learn.microsoft.com/microsoft-agent-365/developer/custom-client-app-registration) for the updated permission list. +- `setup all` now retries agent identity creation and blueprint token acquisition with exponential back-off (delay doubles up to a 60-second cap; agent identity retries up to 5 times, blueprint token up to 12 times — worst case is several minutes per call when Entra replication lag is severe) when Entra replication lag causes transient 401/AADSTS errors on fresh blueprint setups. Retry progress is logged at `Debug` level only. + ### Fixed +- `setup all` with `--authmode obo --aiteammate` no longer exits with an error. `obo` is the default for AI Teammate agents and is accepted with a warning; `--authmode s2s` and `--authmode both` remain incompatible with `--aiteammate`. +- `setup all` summary now shows **Messaging endpoint — not configured** (with a link to the Teams Developer Portal to register the endpoint manually) instead of **failed — see Action Required** when the messaging endpoint URL is absent from config. Real failures (bad URL, contract mismatch) continue to show the error path. +- `setup blueprint` no longer silently loses the blueprint client secret on re-runs when `agentBlueprintClientSecret` was previously `null` in `a365.generated.config.json`. Null dynamic properties are now omitted from the generated config file instead of being written as explicit nulls (fixes macOS secret-loss regression introduced in the issue #408 fix). - Error messages for commands run without required configuration no longer expose internal file paths. `setup all`, `cleanup`, and `create-instance` without `--agent-name` now show actionable guidance with the exact command to run. `develop addpermissions` and `develop gettoken` without `--app-id` now prompt for the application ID directly. - `setup all` no longer inherits stale resource IDs when the user switches tenants between runs (`az logout` + `az login` to a different tenant). The CLI detects the tenant change before loading configuration, silently backs up files from the previous run, and prompts the user to re-run with `--agent-name` for a clean setup in the new tenant. - `setup permissions bot` no longer emits "Bot API permissions configured successfully" when any S2S app-role assignment fails; shows a warning with retry instructions instead. @@ -50,6 +57,7 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g - `setup all` now skips agent registration with a clear warning when the agent identity ID is not available, instead of silently sending an invalid request. Retry with `a365 setup all --agent-registration-only` once the identity is ready. - `setup permissions bot` now returns a non-zero exit code when an S2S app role assignment fails, so callers and scripts can detect the failure. - `setup all --agent-registration-only` reliability fixes: stored IDs are now correctly read in bootstrap (`--agent-name`) mode; falls back to a Graph API lookup when `agenticAppId` is missing; skips identity, permission, and project-settings steps that don't apply. +- `setup permissions bot` help text and final "Next step" log no longer suggest the non-existent `a365 deploy` command; both now point at `a365 publish` (the actual next command in the workflow). ### Removed - `a365 config` command family (`config init`, `config display`, `config permissions`) — replaced by `a365 setup all --agent-name` and `a365 setup permissions custom`. diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs index d326d7e1..0c57295a 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs @@ -179,9 +179,16 @@ public static Command CreateCommand( } if (authMode is not null && aiTeammateFlag == true) { - logger.LogError("--authmode is not supported with --aiteammate — AI Teammate agents automatically use OBO via agent user identity."); - context.ExitCode = 1; - return; + if (authMode == "obo") + { + logger.LogWarning("--authmode obo is redundant with --aiteammate — AI Teammate agents always use OBO. Flag ignored."); + } + else + { + logger.LogError("--authmode {AuthMode} is not supported with --aiteammate — AI Teammate agents always use OBO via agent user identity.", authMode); + context.ExitCode = 1; + return; + } } // Generate correlation ID at workflow entry point @@ -782,7 +789,7 @@ internal static async Task ExecuteMessagingEndpointStepAsync(SetupContext ctx) { ctx.Logger.LogWarning("Messaging endpoint registration skipped: agent blueprint ID is missing (the blueprint step likely failed)."); ctx.Results.MessagingEndpointResult = Models.EndpointRegistrationResult.Failed; - ctx.Results.MessagingEndpointFailureReason = "BlueprintMissing"; + ctx.Results.MessagingEndpointFailureReason = MessagingEndpointFailureReasons.BlueprintMissing; ctx.Results.MessagingEndpoint = ctx.Config.MessagingEndpoint; ctx.Results.Warnings.Add("Messaging endpoint: agent blueprint ID is missing, so endpoint registration was not attempted. Resolve the blueprint creation failure first, then re-run 'a365 setup blueprint --endpoint-only --m365'."); return; @@ -816,7 +823,7 @@ internal static async Task ExecuteMessagingEndpointStepAsync(SetupContext ctx) // setup should continue; surface the failure in the summary as a warning. ctx.Logger.LogWarning("Messaging endpoint registration skipped: {Message}", ex.Message); ctx.Results.MessagingEndpointResult = Models.EndpointRegistrationResult.Failed; - ctx.Results.MessagingEndpointFailureReason = "Other"; + ctx.Results.MessagingEndpointFailureReason = MessagingEndpointFailureReasons.Other; ctx.Results.MessagingEndpoint = ctx.Config.MessagingEndpoint; ctx.Results.Warnings.Add($"Messaging endpoint: {ex.Message}"); } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs index 96c49c8f..534186dc 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs @@ -790,7 +790,7 @@ public static async Task EnsureDelegatedConsentWithRetriesAsync( { logger.LogError(ex, "Error during delegated consent: {Message}", ex.Message); logger.LogError("Common causes:"); - logger.LogError(" 1. Insufficient permissions - You need Application.ReadWrite.All and DelegatedPermissionGrant.ReadWrite.All"); + logger.LogError(" 1. Insufficient permissions - You need AgentIdentityBlueprint.ReadWrite.All consented on your client app"); logger.LogError(" 2. Not a Global Administrator or similar privileged role"); logger.LogError(" 3. Azure CLI authentication expired - Run 'az login' and retry"); logger.LogError(" 4. Network connectivity issues"); @@ -1021,7 +1021,9 @@ public static async Task EnsureDelegatedConsentWithRetriesAsync( // Explicit scopes — NOT .default. Using .default bundles all consented scopes including // AgentIdentityBlueprint.*, which Entra rejects for POST /v1.0/servicePrincipals // ("backing application must be in the local tenant"). - // AgentIdentityBlueprintPrincipal.Create is the correct scope per Agent ID team (Kyle Marsh). + // AgentIdentityBlueprintPrincipal.Create is required for POST /v1.0/serviceprincipals/graph.agentIdentityBlueprintPrincipal. + // AgentIdentityBlueprint.ReadWrite.All is the umbrella for blueprint app operations but does NOT cover + // blueprint SP creation — that is a separate resource requiring Principal.Create (confirmed Run 1, 2026-05-08). logger.LogDebug("Acquiring blueprint httpClient token — scope: AgentIdentityBlueprintPrincipal.Create, loginHint: {LoginHint}", blueprintLoginHint ?? "(none)"); var graphToken = await AcquireMsalGraphTokenAsync(tenantId, setupConfig.ClientAppId, logger, ct, scope: AuthenticationConstants.AgentIdentityBlueprintPrincipalCreateScope, @@ -1512,7 +1514,7 @@ await retryHelper.ExecuteWithRetryAsync( ficError = ficCreateResult?.ErrorMessage ?? "Federated Identity Credential creation failed"; logger.LogWarning("[WARN] Federated Identity Credential creation failed - you may need to create it manually in Entra ID"); - logger.LogWarning(" Ensure the client app has 'AgentIdentityBlueprint.UpdateAuthProperties.All' permission consented."); + logger.LogWarning(" Ensure the client app has 'AgentIdentityBlueprint.ReadWrite.All' permission consented."); } } else if (!useManagedIdentity) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs index fefd0ebc..c703319a 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs @@ -428,12 +428,24 @@ private static async Task ExecuteAgentIdentityAndRegistrationAsync( } else { - // Agent identity creation via delegated flow (AgentIdentity.Create.All). - // Agent ID Developer role is sufficient — client credentials are not required. + // Agent identity creation via blueprint client credentials (app-only). + // AgentIdentity.CreateAsManager is auto-granted to Blueprint apps — no custom app permission required. ctx.Logger.LogInformation("Creating agent identity..."); - var agentId = await ctx.GraphApiService.CreateAgentIdentityDelegatedAsync( + if (string.IsNullOrWhiteSpace(ctx.Config.AgentBlueprintClientSecret)) + { + ctx.Results.AgentIdentityFailed = true; + ctx.Logger.LogError("Blueprint client secret is not available. Re-run 'a365 setup blueprint' to create it."); + return; + } + + var plainSecret = SecretProtectionHelper.UnprotectSecret( + ctx.Config.AgentBlueprintClientSecret, + ctx.Config.AgentBlueprintClientSecretProtected, + ctx.Logger); + var agentId = await ctx.GraphApiService.CreateAgentIdentityAsync( ctx.Config.TenantId!, ctx.Config.AgentBlueprintId!, + plainSecret, agentIdentityDisplayName, ctx.CancellationToken); @@ -451,8 +463,8 @@ private static async Task ExecuteAgentIdentityAndRegistrationAsync( else if (!ctx.Results.AgentIdentityFailed) { ctx.Results.AgentIdentityFailed = true; - ctx.Results.Warnings.Add("Agent identity creation failed. Ensure you have the Agent ID Developer or Agent ID Administrator role in this tenant."); - ctx.Logger.LogWarning("Agent identity creation failed. Ensure you have the Agent ID Developer or Agent ID Administrator role in this tenant."); + ctx.Results.Warnings.Add("Agent identity creation failed. Ensure blueprint setup completed and the client secret is available."); + ctx.Logger.LogWarning("Agent identity creation failed. Ensure blueprint setup completed and the client secret is available."); } } } @@ -628,18 +640,14 @@ internal static async Task GrantAgentIdentityPermissionsAsync( return; } - var agentIdentitySpObjectId = await ctx.GraphApiService.EnsureServicePrincipalForAppIdAsync( - ctx.Config.TenantId!, - ctx.Config.AgenticAppId!, - ctx.CancellationToken, - Constants.AuthenticationConstants.RequiredPermissionGrantScopes); + // AgenticAppId is the SP object ID returned by CreateAgentIdentityAsync and stored in config. + var agentIdentitySpObjectId = ctx.Config.AgenticAppId; if (string.IsNullOrWhiteSpace(agentIdentitySpObjectId)) { ctx.Logger.LogWarning( - "Could not resolve service principal for agent identity ({AgentId}). " + - "Permissions must be granted manually in the Entra portal.", - ctx.Config.AgenticAppId); + "Agent identity ID not found in config. " + + "Permissions must be granted manually in the Entra portal."); return; } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs index d267a8b9..c986e9b0 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs @@ -245,7 +245,7 @@ private static Command CreateBotSubcommand( "Configure Messaging Bot API OAuth2 grants and inheritable permissions\n" + "Minimum required permissions: Global Administrator\n\n" + "Prerequisites: Blueprint and MCP permissions (run 'a365 setup permissions mcp' first)\n" + - "Next step: Deploy your agent (run 'a365 deploy' if hosting on Azure)"); + "Next step: Run 'a365 publish' to package your agent for upload to the Microsoft 365 Admin Center"); var agentNameOption = new Option( ["--agent-name", "-n"], @@ -699,7 +699,7 @@ public static async Task ConfigureBotPermissionsAsync( logger.LogInformation(""); if (!iSetupAll) { - logger.LogInformation("Next step: Deploy your agent (run 'a365 deploy' if hosting on Azure)"); + logger.LogInformation("Next step: Run 'a365 publish' to package your agent for upload to the Microsoft 365 Admin Center."); } return consentGranted && !s2sFailed; } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs index 86fab033..0c95db8a 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs @@ -553,12 +553,12 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, boo else { var oboAlso = isBothMode && results.AgentIdentityPermissionsGranted; - var label = oboAlso ? "granted S2S app roles + developer-scoped delegated" : "granted S2S app roles"; + var label = oboAlso ? "granted S2S app roles + developer-scoped delegated on agent identity" : "granted S2S app roles"; logger.LogInformation(DryRunRow(permGrantStep, "Permission Grants") + label); } } else if (results.AgentIdentityPermissionsGranted) - logger.LogInformation(DryRunRow(permGrantStep, "Permission Grants") + "granted developer-scoped delegated"); + logger.LogInformation(DryRunRow(permGrantStep, "Permission Grants") + "granted developer-scoped delegated on agent identity"); else if (pendingDelegatedAction) logger.LogWarning(DryRunRow(permGrantStep, "Permission Grants") + "PENDING — see Action Required"); else if (results.BatchPermissionsPhase2Completed) @@ -612,14 +612,18 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, boo break; case Models.EndpointRegistrationResult.Failed: default: - if (string.Equals(results.MessagingEndpointFailureReason, "NotOwner", StringComparison.Ordinal)) + if (string.Equals(results.MessagingEndpointFailureReason, MessagingEndpointFailureReasons.NotOwner, StringComparison.Ordinal)) { logger.LogWarning(DryRunRow(endpointStep, "Messaging endpoint") + "failed (not blueprint owner) — see Action Required"); } - else if (string.Equals(results.MessagingEndpointFailureReason, "BlueprintMissing", StringComparison.Ordinal)) + else if (string.Equals(results.MessagingEndpointFailureReason, MessagingEndpointFailureReasons.BlueprintMissing, StringComparison.Ordinal)) { logger.LogWarning(DryRunRow(endpointStep, "Messaging endpoint") + "not attempted (blueprint creation failed) — see Action Required"); } + else if (string.Equals(results.MessagingEndpointFailureReason, MessagingEndpointFailureReasons.NotConfigured, StringComparison.Ordinal)) + { + logger.LogWarning(DryRunRow(endpointStep, "Messaging endpoint") + "not configured — register manually in the Teams Developer Portal"); + } else { logger.LogWarning(DryRunRow(endpointStep, "Messaging endpoint") + "failed — see Action Required"); @@ -745,7 +749,7 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, boo if (messagingEndpointFailureRequired) { actionCount++; - if (string.Equals(results.MessagingEndpointFailureReason, "NotOwner", StringComparison.Ordinal)) + if (string.Equals(results.MessagingEndpointFailureReason, MessagingEndpointFailureReasons.NotOwner, StringComparison.Ordinal)) { logger.LogInformation(" {N}. Messaging endpoint — you are not an owner of the blueprint, so automated", actionCount); logger.LogInformation(" registration was refused by the server. To complete this step, either:"); @@ -758,13 +762,18 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, boo logger.LogInformation(" the endpoint step (no need to re-run the full setup):"); logger.LogInformation(" a365 setup blueprint --endpoint-only --m365"); } - else if (string.Equals(results.MessagingEndpointFailureReason, "BlueprintMissing", StringComparison.Ordinal)) + else if (string.Equals(results.MessagingEndpointFailureReason, MessagingEndpointFailureReasons.BlueprintMissing, StringComparison.Ordinal)) { logger.LogInformation(" {N}. Messaging endpoint — not attempted because agent blueprint creation did not", actionCount); logger.LogInformation(" complete. Resolve the blueprint step (see errors above), then re-run just"); logger.LogInformation(" the endpoint step:"); logger.LogInformation(" a365 setup blueprint --endpoint-only --m365"); } + else if (string.Equals(results.MessagingEndpointFailureReason, MessagingEndpointFailureReasons.NotConfigured, StringComparison.Ordinal)) + { + logger.LogInformation(" {N}. Messaging endpoint — register manually in the Teams Developer Portal:", actionCount); + logger.LogInformation(" {Url}", ConfigConstants.TeamsDeveloperPortalConfigureEndpointUrl); + } else { logger.LogInformation(" {N}. Messaging endpoint — registration failed; see the error above for details.", actionCount); @@ -826,7 +835,7 @@ public static void DisplaySetupSummary(SetupResults results, ILogger logger, boo { nextStepLines.Add(() => { - logger.LogInformation(" Ensure 'AgentIdentityBlueprint.UpdateAuthProperties.All' is consented, then:"); + logger.LogInformation(" Ensure 'AgentIdentityBlueprint.ReadWrite.All' is consented, then:"); logger.LogInformation(" a365 setup blueprint"); }); } @@ -1180,7 +1189,7 @@ public static async Task EnsureResourcePermissionsAsync( { throw new SetupValidationException( $"Failed to create/update OAuth2 permission grant from blueprint {config.AgentBlueprintId} to {resourceName} {resourceAppId}. " + - "This may be due to insufficient permissions. Ensure you have DelegatedPermissionGrant.ReadWrite.All permission."); + "This may be due to insufficient permissions. Ensure you have AgentIdentityBlueprint.ReadWrite.All permission consented on your client app."); } // 3. Set inheritable permissions (for agent blueprints) @@ -1193,16 +1202,20 @@ public static async Task EnsureResourcePermissionsAsync( logger.LogInformation(" - Configuring inheritable permissions: blueprint {Blueprint} to resourceAppId {ResourceAppId} scopes [{Scopes}]", config.AgentBlueprintId, resourceAppId, string.Join(' ', scopes)); - // Use custom client app auth for inheritable permissions - Azure CLI doesn't support this operation. - // Reuse permissionGrantScopes (which already includes AgentIdentityBlueprint.UpdateAuthProperties.All) - // so all Graph PowerShell calls in this method share a single Connect-MgGraph session/cache entry. + // Use custom client app auth for inheritable permissions — Azure CLI doesn't support this operation. + // permissionGrantScopes is intentionally empty in the current permission set (the operation that + // previously required AgentIdentityBlueprint.UpdateAuthProperties.All is now covered by the + // AgentIdentityBlueprint.ReadWrite.All umbrella in RequiredClientAppPermissions). When the scope + // collection is empty, SetInheritablePermissionsAsync falls through to the standard token path + // which already carries every required scope; passing the array here keeps a single call shape + // regardless of whether the set is empty or repopulated in a future revision. var (ok, alreadyExists, err) = await blueprintService.SetInheritablePermissionsAsync( config.TenantId, config.AgentBlueprintId, resourceAppId, scopes, requiredScopes: permissionGrantScopes, ct); if (!ok && !alreadyExists) { throw new SetupValidationException($"Failed to set inheritable permissions: {err}. " + - "Ensure you have AgentIdentityBlueprint.UpdateAuthProperties.All permission in your custom client app."); + "Ensure you have AgentIdentityBlueprint.ReadWrite.All permission consented on your custom client app."); } if (alreadyExists) @@ -1391,8 +1404,7 @@ public static async Task EnsureResourcePermissionsAsync( if (string.IsNullOrWhiteSpace(setupConfig.MessagingEndpoint)) { logger.LogWarning("MessagingEndpoint not configured. Skipping endpoint registration."); - logger.LogWarning("Configure 'messagingEndpoint' in a365.config.json and re-run 'a365 setup blueprint' to register the endpoint."); - return (Models.EndpointRegistrationResult.Failed, "Other"); + return (Models.EndpointRegistrationResult.Failed, MessagingEndpointFailureReasons.NotConfigured); } if (!Uri.TryCreate(setupConfig.MessagingEndpoint, UriKind.Absolute, out var messagingEndpointUri) || diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs index 2e4d924e..cffee253 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs @@ -166,27 +166,14 @@ public static string[] GetRequiredRedirectUris(string clientAppId) /// public const string AgentIdentityBlueprintPrincipalCreateScope = "AgentIdentityBlueprintPrincipal.Create"; - /// - /// Delegated scope required to delete an Agent Blueprint. - /// Per the Agent ID permissions reference, this is the correct scope for Delete operations. - /// - public const string AgentIdentityBlueprintDeleteRestoreAllScope = "AgentIdentityBlueprint.DeleteRestore.All"; - /// /// Delegated scope required to delete an Agent Identity (service principal). /// Per the Agent ID permissions reference, DELETE /beta/servicePrincipals/{id} for agent identities - /// requires this scope — NOT AgentIdentityBlueprint.DeleteRestore.All, which is blueprint-only. + /// requires this scope — blueprint deletion is covered by the AgentIdentityBlueprint.ReadWrite.All + /// umbrella so no separate blueprint-delete scope is defined here. /// public const string AgentIdentityDeleteRestoreAllScope = "AgentIdentity.DeleteRestore.All"; - /// - /// Delegated scope required to add or remove federated identity credentials and password credentials - /// on an Agent Blueprint. Per the Agent ID permissions reference, covers keyCredentials, - /// passwordCredentials, and federatedIdentityCredentials. Requires Global Administrator or - /// Agent ID Administrator role. - /// - public const string AgentIdentityBlueprintAddRemoveCredsAllScope = "AgentIdentityBlueprint.AddRemoveCreds.All"; - /// /// Delegated scope for full read/write access to an Agent Blueprint. /// Includes all granular update permissions (UpdateAuthProperties, AddRemoveCreds, UpdateBranding). @@ -205,6 +192,22 @@ public static string[] GetRequiredRedirectUris(string clientAppId) /// public const string ApplicationReadWriteAllScope = "Application.ReadWrite.All"; + /// + /// Delegated scope for reading application and service principal details. + /// Narrower replacement for Directory.Read.All — covers SP lookups by appId + /// (GET /v1.0/servicePrincipals?$filter=appId eq '...') without granting + /// broad directory read access. + /// + public const string ApplicationReadAllScope = "Application.Read.All"; + + /// + /// Delegated scope for creating and managing agent user accounts. + /// Agent-specific replacement for User.ReadWrite.All — covers agent user creation, + /// usageLocation update, and license assignment without granting broad write + /// access to all users in the tenant. + /// + public const string AgentIdUserReadWriteAllScope = "AgentIdUser.ReadWrite.All"; + /// /// Required delegated permissions for the custom client app used by a365 CLI. /// These permissions enable the CLI to manage Entra ID applications and agent blueprints. @@ -215,24 +218,13 @@ public static string[] GetRequiredRedirectUris(string clientAppId) /// public static readonly string[] RequiredClientAppPermissions = new[] { - "AgentIdentityBlueprintPrincipal.Create", // Required for POST /v1.0/serviceprincipals/graph.agentIdentityBlueprintPrincipal (blueprint SP creation) - "AgentIdentityBlueprint.ReadWrite.All", - "AgentIdentityBlueprint.UpdateAuthProperties.All", - "AgentIdentityBlueprint.AddRemoveCreds.All", // Required for passwordCredentials and FICs during setup and cleanup - "DelegatedPermissionGrant.ReadWrite.All", - "Directory.Read.All", - "AgentInstance.ReadWrite.All", // Required for DELETE /beta/agentRegistry/agentInstances (CleanupCommand) + "AgentIdentityBlueprint.ReadWrite.All", // Umbrella — covers blueprint creation, UpdateAuthProperties, AddRemoveCreds, DeleteRestore sub-scopes + "AgentIdentityBlueprintPrincipal.Create", // Required for POST /v1.0/serviceprincipals/graph.agentIdentityBlueprintPrincipal — separate from ReadWrite.All (different resource) "AgentRegistration.ReadWrite.All", // Required for POST/DELETE /beta/copilot/agentRegistrations (agent registration) - // AgentIdentity.ReadWrite.All removed — no code requests it as a token scope. - // Delete uses AgentIdentity.DeleteRestore.All. Read uses AgentIdentity.Read.All. - // AgentIdentity.Create.All is a delegated scope used by CreateAgentIdentityDelegatedAsync - // (POST /beta/servicePrincipals/Microsoft.Graph.AgentIdentity). Requires Agent ID Developer role. - "AgentIdentityBlueprint.DeleteRestore.All", // Required for 'a365 cleanup' to delete the Agent Blueprint application "AgentIdentity.Read.All", // Required for GET /beta/servicePrincipals/microsoft.graph.agentIdentity?$filter=agentIdentityBlueprintId (idempotency check) "AgentIdentity.DeleteRestore.All", // Required for 'a365 cleanup' to delete the Agent Identity service principal + "Application.Read.All", // Narrower replacement for Directory.Read.All — covers SP lookups by appId "User.Read", // Required for /me endpoint to resolve the signed-in user's object ID for blueprint owner/sponsor assignment - "User.ReadWrite.All", // Required for agent user creation, usage location update, and license assignment - // Note: RoleManagementReadDirectoryScope is excluded because Directory.Read.All covers the needed read operations. }; /// @@ -246,28 +238,25 @@ public static string[] GetRequiredRedirectUris(string clientAppId) .ToArray(); /// - /// Required scopes for all PowerShell-based Microsoft Graph operations (OAuth2 grants, - /// service principal lookups, and inheritable permissions). - /// Using a single unified set ensures Connect-MgGraph authenticates once and the resulting - /// token is reused from the in-process cache for all downstream Graph operations. - /// All scopes require admin consent and are included in RequiredClientAppPermissions. + /// Explicit delegated scopes passed to EnsureGraphHeadersAsync for permission-grant operations. + /// Intentionally empty: the operations that previously needed explicit scopes here + /// (DelegatedPermissionGrant.ReadWrite.All for oauth2 grant CRUD, + /// AgentIdentityBlueprint.UpdateAuthProperties.All for inheritable permissions) are now + /// covered by the AgentIdentityBlueprint.ReadWrite.All umbrella in RequiredClientAppPermissions. + /// An empty array causes EnsureGraphHeadersAsync to route through the standard token path + /// (GetGraphAccessTokenAsync / AuthenticationService), which already carries all required scopes. + /// Validated end-to-end across all 4 setup variants (PR #409). /// - public static readonly string[] RequiredPermissionGrantScopes = new[] - { - "DelegatedPermissionGrant.ReadWrite.All", - "AgentIdentityBlueprint.UpdateAuthProperties.All", - }; + public static readonly string[] RequiredPermissionGrantScopes = []; /// - /// Additional scopes required only on Global Administrator paths that grant S2S app role - /// assignments. Kept separate from so that - /// non-admin flows (deploy, setup permissions) do not request an admin-only scope and - /// trigger unexpected consent prompts. + /// Additional scopes for S2S app role assignment calls in BatchPermissionsOrchestrator. + /// Intentionally empty: AppRoleAssignment.ReadWrite.All was removed from the required + /// permission set. Global Admins can assign app roles without that scope (admin bypass); + /// developers receive a 403 and fall back to PowerShell instructions as intended. + /// Validated across admin and developer paths (PR #409). /// - public static readonly string[] RequiredS2SGrantScopes = new[] - { - "AppRoleAssignment.ReadWrite.All", - }; + public static readonly string[] RequiredS2SGrantScopes = []; /// /// Entra roles that can perform S2S app role assignments programmatically. @@ -297,9 +286,8 @@ public static string[] GetRequiredRedirectUris(string clientAppId) /// public static readonly string[] BlueprintInteractiveAuthScopes = new[] { - $"{MicrosoftGraphResourceUri}/AgentIdentityBlueprintPrincipal.Create", $"{MicrosoftGraphResourceUri}/AgentIdentityBlueprint.ReadWrite.All", - $"{MicrosoftGraphResourceUri}/AgentIdentityBlueprint.UpdateAuthProperties.All", + $"{MicrosoftGraphResourceUri}/AgentIdentityBlueprintPrincipal.Create", $"{MicrosoftGraphResourceUri}/User.Read" }; diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ErrorCodes.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ErrorCodes.cs index 91cd3e23..8ef6ee98 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ErrorCodes.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ErrorCodes.cs @@ -10,8 +10,6 @@ public static class ErrorCodes public const string DeploymentScopesFailed = "DEPLOYMENT_SCOPES_FAILED"; public const string DeploymentMcpFailed = "DEPLOYMENT_MCP_FAILED"; public const string HighPrivilegeScopeDetected = "HIGH_PRIVILEGE_SCOPE_DETECTED"; - public const string NodeBuildFailed = "NODE_BUILD_FAILED"; - public const string NodeDependencyInstallFailed = "NODE_DEPENDENCY_INSTALL_FAILED"; public const string NodeProjectNotFound = "NODE_PROJECT_NOT_FOUND"; public const string RetryExhausted = "RETRY_EXHAUSTED"; public const string SetupValidationFailed = "SETUP_VALIDATION_FAILED"; diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/MessagingEndpointFailureReasons.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/MessagingEndpointFailureReasons.cs new file mode 100644 index 00000000..730e8373 --- /dev/null +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/MessagingEndpointFailureReasons.cs @@ -0,0 +1,25 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +namespace Microsoft.Agents.A365.DevTools.Cli.Constants; + +/// +/// String literals assigned to SetupResults.MessagingEndpointFailureReason when the +/// messaging endpoint step does not complete successfully. The values are persisted in setup +/// summary output, so the exact strings must not change without considering downstream +/// consumers that grep for them. +/// +public static class MessagingEndpointFailureReasons +{ + /// Signed-in user is not a blueprint owner; Teams Graph returned a 403-wrapped-as-400. + public const string NotOwner = "NotOwner"; + + /// Blueprint creation itself failed, so endpoint registration was never attempted. + public const string BlueprintMissing = "BlueprintMissing"; + + /// Messaging endpoint URL was absent from config; we direct the user to the Teams Developer Portal. + public const string NotConfigured = "NotConfigured"; + + /// Any other failure (validation exception, contract mismatch, unexpected error). + public const string Other = "Other"; +} diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Exceptions/NodeBuildFailedException.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Exceptions/NodeBuildFailedException.cs deleted file mode 100644 index fac3c974..00000000 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Exceptions/NodeBuildFailedException.cs +++ /dev/null @@ -1,57 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -using Microsoft.Agents.A365.DevTools.Cli.Constants; - -namespace Microsoft.Agents.A365.DevTools.Cli.Exceptions; - -/// -/// Thrown when the build process of a Node.js project fails. -/// -public sealed class NodeBuildFailedException : Agent365Exception -{ - public override int ExitCode => 1; - public override bool IsUserError => true; - - public NodeBuildFailedException(string projectDirectory, string? npmErrorOutput) - : base( - errorCode: ErrorCodes.NodeBuildFailed, - issueDescription: "Failed to build the Node.js project using 'npm run build'.", - errorDetails: BuildDetails(projectDirectory, npmErrorOutput), - mitigationSteps: new List - { - "Run 'npm run build' locally in the project directory and fix any TypeScript/webpack/build errors.", - "Verify that the 'build' script is defined correctly in package.json.", - "If the build depends on environment variables or private packages, ensure those are configured on the machine running 'a365 deploy'.", - "After resolving the build issues, rerun 'a365 deploy'." - }, - context: new Dictionary - { - ["ProjectDirectory"] = projectDirectory - }) - { - } - - private static List BuildDetails(string projectDirectory, string? npmErrorOutput) - { - var details = new List - { - $"Project directory: {projectDirectory}", - }; - - if (!string.IsNullOrWhiteSpace(npmErrorOutput)) - { - details.Add("npm build error output (truncated):"); - details.Add($" {TrimError(npmErrorOutput)}"); - } - - return details; - } - - private static string TrimError(string error) - { - const int maxLen = 400; - error = error.Trim(); - return error.Length <= maxLen ? error : error[..maxLen] + " ..."; - } -} \ No newline at end of file diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Exceptions/NodeDependencyInstallException.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Exceptions/NodeDependencyInstallException.cs deleted file mode 100644 index a2ba9d5a..00000000 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Exceptions/NodeDependencyInstallException.cs +++ /dev/null @@ -1,57 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -using Microsoft.Agents.A365.DevTools.Cli.Constants; - -namespace Microsoft.Agents.A365.DevTools.Cli.Exceptions; - -/// -/// Thrown when installation of Node.js dependencies fails. -/// -public class NodeDependencyInstallException : Agent365Exception -{ - public override int ExitCode => 1; - public override bool IsUserError => true; - - public NodeDependencyInstallException(string projectDirectory, string? npmErrorOutput) - : base( - errorCode: ErrorCodes.NodeDependencyInstallFailed, - issueDescription: "Failed to install Node.js dependencies for the project.", - errorDetails: BuildDetails(projectDirectory, npmErrorOutput), - mitigationSteps: new List - { - "Run 'npm install' (or 'npm ci') locally in the project directory and fix any errors.", - "Check that your internet connection and npm registry access are working.", - "If you use a private registry or npm auth, ensure those settings are configured on the machine running 'a365 deploy'.", - "After fixing the issue, rerun 'a365 deploy'." - }, - context: new Dictionary - { - ["ProjectDirectory"] = projectDirectory - }) - { - } - - private static List BuildDetails(string projectDirectory, string? npmErrorOutput) - { - var details = new List - { - $"Project directory: {projectDirectory}", - }; - - if (!string.IsNullOrWhiteSpace(npmErrorOutput)) - { - details.Add("npm error output (truncated):"); - details.Add($" {TrimError(npmErrorOutput)}"); - } - - return details; - } - - private static string TrimError(string error) - { - const int maxLen = 400; - error = error.Trim(); - return error.Length <= maxLen ? error : error[..maxLen] + " ..."; - } -} \ No newline at end of file diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/A365CreateInstanceRunner.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/A365CreateInstanceRunner.cs index d7a79dac..431f1bd4 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/A365CreateInstanceRunner.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/A365CreateInstanceRunner.cs @@ -282,7 +282,7 @@ string GetConfig(string name) => _logger.LogError(""); _logger.LogError("RECOMMENDED ACTIONS:"); _logger.LogError(" 1. Wait 5-10 minutes and run: a365 setup createinstance --step user"); - _logger.LogError(" 2. Verify User.ReadWrite.All permission is granted"); + _logger.LogError(" 2. Verify AgentIdUser.ReadWrite.All permission is granted"); _logger.LogError(" 3. Check Azure AD audit logs for detailed error information"); return false; } @@ -568,7 +568,7 @@ string GetConfig(string name) => _logger.LogError(""); _logger.LogError("RECOMMENDED ACTIONS:"); _logger.LogError(" 1. Ensure you have completed MSAL sign-in (the CLI authenticates via browser or device code, not Azure CLI)"); - _logger.LogError(" 2. Verify your custom client app has the required delegated scopes (User.ReadWrite.All)"); + _logger.LogError(" 2. Verify your custom client app has the required delegated scopes (AgentIdUser.ReadWrite.All)"); _logger.LogError(" 3. Re-run the command"); return (false, null); } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs index baecd0a2..5806ca05 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs @@ -87,9 +87,9 @@ public virtual async Task DeleteAgentBlueprintAsync( { _logger.LogInformation("Deleting agent blueprint application: {BlueprintId}", blueprintId); - var requiredScopes = new[] { AuthenticationConstants.AgentIdentityBlueprintDeleteRestoreAllScope }; + var requiredScopes = new[] { AuthenticationConstants.AgentIdentityBlueprintReadWriteAllScope }; - _logger.LogInformation("Acquiring access token with AgentIdentityBlueprint.DeleteRestore.All scope..."); + _logger.LogInformation("Acquiring access token with AgentIdentityBlueprint.ReadWrite.All scope..."); _logger.LogInformation("An authentication dialog will appear to complete sign-in."); var deletePath = $"/beta/applications/{blueprintId}/microsoft.graph.agentIdentityBlueprint"; @@ -176,21 +176,34 @@ public virtual async Task> GetAgentInstancesFor string blueprintId, CancellationToken cancellationToken = default) { - var requiredScopes = new[] { AuthenticationConstants.AgentIdentityReadAllScope }; + var spScopes = new[] { AuthenticationConstants.AgentIdentityReadAllScope }; var encodedId = Uri.EscapeDataString(blueprintId); // Fetch agent identity SPs and agent users for this blueprint sequentially to avoid races on shared HTTP headers var spItems = await FetchAllPagesAsync( tenantId, $"/beta/servicePrincipals/microsoft.graph.agentIdentity?$filter=agentIdentityBlueprintId eq '{encodedId}'&$select=id,displayName", - requiredScopes, + spScopes, cancellationToken); - var userItems = await FetchAllPagesAsync( - tenantId, - $"/beta/users/microsoft.graph.agentUser?$filter=agentIdentityBlueprintId eq '{encodedId}'&$select=id,identityParentId", - requiredScopes, - cancellationToken); + // Agent user query requires AgentIdUser.ReadWrite.All, which is intentionally absent from + // RequiredClientAppPermissions until create-instance is re-enabled. This means agent user + // cleanup is also disabled — no agent users exist while create-instance is off, so this is safe. + List userItems; + if (AuthenticationConstants.RequiredClientAppPermissions.Contains(AuthenticationConstants.AgentIdUserReadWriteAllScope)) + { + var userScopes = new[] { AuthenticationConstants.AgentIdUserReadWriteAllScope }; + userItems = await FetchAllPagesAsync( + tenantId, + $"/beta/users/microsoft.graph.agentUser?$filter=agentIdentityBlueprintId eq '{encodedId}'&$select=id,identityParentId", + userScopes, + cancellationToken); + } + else + { + _logger.LogDebug("Skipping agent user query — AgentIdUser.ReadWrite.All not in required permissions (create-instance not enabled)"); + userItems = new List(); + } // Build lookup: identityParentId (SP object ID) -> user object ID var userBySpId = new Dictionary(StringComparer.OrdinalIgnoreCase); foreach (var user in userItems) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ConfigService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ConfigService.cs index 757ab61d..e6f99020 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/ConfigService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/ConfigService.cs @@ -720,7 +720,12 @@ private void MergeDynamicProperties(Agent365Config config, JsonElement stateData var jsonName = GetJsonPropertyName(prop); var value = prop.GetValue(config); - result[jsonName] = value; + // Omit null values: JsonIgnoreCondition.WhenWritingNull in DefaultIgnoreCondition does NOT + // apply to dictionary values (dotnet/runtime#30690), so we filter here to avoid writing + // "agentBlueprintClientSecret": null and similar fields that would then re-merge as null + // on the next LoadAsync, silently losing previously-written values. + if (value != null) + result[jsonName] = value; } return result; diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/FederatedCredentialService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/FederatedCredentialService.cs index e2cb4410..c324ab3f 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/FederatedCredentialService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/FederatedCredentialService.cs @@ -53,7 +53,7 @@ public async Task> GetFederatedCredentialsAsync( tenantId, $"/beta/applications/{blueprintObjectId}/federatedIdentityCredentials", cancellationToken, - scopes: [AuthenticationConstants.AgentIdentityBlueprintAddRemoveCredsAllScope]); + scopes: [AuthenticationConstants.AgentIdentityBlueprintReadWriteAllScope]); JsonDocument? doc; if (primaryDoc != null && primaryDoc.RootElement.TryGetProperty("value", out var valueCheck) && valueCheck.GetArrayLength() > 0) @@ -69,7 +69,7 @@ public async Task> GetFederatedCredentialsAsync( tenantId, $"/beta/applications/microsoft.graph.agentIdentityBlueprint/{blueprintObjectId}/federatedIdentityCredentials", cancellationToken, - scopes: [AuthenticationConstants.AgentIdentityBlueprintAddRemoveCredsAllScope]); + scopes: [AuthenticationConstants.AgentIdentityBlueprintReadWriteAllScope]); } if (doc == null) @@ -267,7 +267,7 @@ public async Task CreateFederatedCredentialAsyn endpoint, payload, cancellationToken, - scopes: [AuthenticationConstants.AgentIdentityBlueprintAddRemoveCredsAllScope]); + scopes: [AuthenticationConstants.AgentIdentityBlueprintReadWriteAllScope]); if (response.IsSuccess) { @@ -401,7 +401,7 @@ public async Task DeleteFederatedCredentialAsync( _logger.LogDebug("Deleting federated credential: {CredentialId} from blueprint: {ObjectId}", credentialId, blueprintObjectId); - var ficScope = AuthenticationConstants.AgentIdentityBlueprintAddRemoveCredsAllScope; + var ficScope = AuthenticationConstants.AgentIdentityBlueprintReadWriteAllScope; // Try the standard endpoint first var endpoint = $"/beta/applications/{blueprintObjectId}/federatedIdentityCredentials/{credentialId}"; diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs index 708be6b9..f727016a 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/GraphApiService.cs @@ -157,37 +157,68 @@ public GraphApiService(ILogger logger, CommandExecutor executor private async Task EnsureGraphHeadersAsync(string tenantId, bool forceRefresh = false, IEnumerable? scopes = null, CancellationToken ct = default) { - // Authentication Strategy: - // 1. If specific scopes required AND token provider configured: Use MSAL with delegated scopes (WAM/browser/device-code) - // 2. Otherwise: Use MSAL via AuthenticationService (WAM/browser/device-code, persistent cache) + // Authentication strategy: + // + // 1. Token provider with CustomClientAppId resolved (steady-state): route through + // `_tokenProvider.GetMgGraphAccessTokenAsync(..., CustomClientAppId, ...)` so the + // JWT carries per-app-registration optional claims (like `wids`) configured on the + // custom client app. Default the scope to User.Read when the caller didn't pass any + // (some callers pass an empty array as a "no extra scopes" signal — they still need + // a custom-app token, not a PowerShell well-known token). + // + // 2. Token provider but CustomClientAppId not yet resolved (bootstrap phase, before + // config is loaded): fall through to the legacy AuthenticationService path. The + // bootstrap config resolver uses this to look up the custom client app by display + // name before its ID is known. Routing through `_tokenProvider` with a null clientId + // would prompt PowerShell `Connect-MgGraph` and hang on user input. + // + // 3. No token provider (test/legacy scenarios only): same legacy fallback. Operations + // that need claims from the custom-app JWT (e.g. CheckDirectoryRoleAsync) must guard + // for both `_tokenProvider == null` and an empty `CustomClientAppId` themselves. + // // All paths go through MSAL — no az CLI subprocess involved. string? token; - - if (scopes != null && _tokenProvider != null) - { - // Use token provider with delegated scopes (interactive browser auth with caching) - _logger.LogDebug("Acquiring Graph token with specific scopes via token provider: {Scopes}", string.Join(", ", scopes)); + bool hasScopes = scopes?.Any() == true; + bool hasCustomApp = !string.IsNullOrWhiteSpace(CustomClientAppId); + + // Use the token provider when the caller passed explicit scopes (the call site is asking + // for a scoped token — clientId can be null until config resolves; production callers that + // pass scopes do so post-bootstrap) OR when CustomClientAppId is set (the only condition + // that makes a default-scope token meaningful — without it the token would have no link to + // the custom app's optional claims). The remaining case (no scopes AND no CustomClientAppId + // — the bootstrap phase config-resolver lookup) falls through to the legacy path below. + if (_tokenProvider != null && (hasScopes || hasCustomApp)) + { + var effectiveScopes = hasScopes ? scopes! : [AuthenticationConstants.UserReadScope]; + _logger.LogDebug( + "Acquiring Graph token via token provider (clientId: {AppId}, scopes: {Scopes})", + CustomClientAppId, string.Join(", ", effectiveScopes)); var loginHint = await ResolveLoginHintAsync(); - token = await _tokenProvider.GetMgGraphAccessTokenAsync(tenantId, scopes, false, CustomClientAppId, ct, loginHint, forceRefresh); + token = await _tokenProvider.GetMgGraphAccessTokenAsync(tenantId, effectiveScopes, false, CustomClientAppId, ct, loginHint, forceRefresh); if (string.IsNullOrWhiteSpace(token)) { - _logger.LogError("Failed to acquire Graph token with scopes: {Scopes}", string.Join(", ", scopes)); + _logger.LogError("Failed to acquire Graph token from token provider (scopes: {Scopes})", + string.Join(", ", effectiveScopes)); return false; } - _logger.LogDebug("Successfully acquired Graph token with specific scopes (cached or new)"); + _logger.LogDebug("Successfully acquired Graph token from token provider (cached or new)"); } - else if (scopes != null && _tokenProvider == null) + else if (hasScopes && _tokenProvider == null) { - // Scopes required but no token provider - this is a configuration issue - _logger.LogError("Token provider is not configured, but specific scopes are required: {Scopes}", string.Join(", ", scopes)); + // Specific scopes required but no token provider — configuration issue. + _logger.LogError("Token provider is not configured, but specific scopes are required: {Scopes}", string.Join(", ", scopes!)); return false; } else { - // Default path: acquire via AuthenticationService (MSAL, persistent disk cache). + // Bootstrap or legacy fallback: token comes from the PowerShell well-known clientId + // via AuthenticationService. Reached when CustomClientAppId hasn't been resolved + // yet (initial app lookup) or when no token provider is configured (tests). Token + // does NOT carry custom-app optional claims — callers that depend on those must + // not reach this branch. token = await GetGraphAccessTokenAsync(tenantId, forceRefresh: forceRefresh, ct: ct); if (string.IsNullOrWhiteSpace(token)) @@ -839,9 +870,9 @@ private async Task CreateOrUpdateOauth2PermissionGrantCoreAsync( if (attempt < maxRetries - 1) { var delaySecs = (int)Math.Min(baseDelaySeconds * Math.Pow(2, attempt), 60); - _logger.LogWarning( + _logger.LogDebug( "Service principal not yet replicated to grants endpoint — retrying in {Delay}s (attempt {Attempt}/{Max})...", - delaySecs, attempt + 1, maxRetries - 1); + delaySecs, attempt + 1, maxRetries); await Task.Delay(TimeSpan.FromSeconds(delaySecs), ct); } } @@ -1063,8 +1094,10 @@ private async Task CreateOrUpdateOauth2PermissionGrantCoreAsync( /// /// Checks whether the currently signed-in user holds the Global Administrator role, /// which is required to grant tenant-wide admin consent interactively. - /// Uses only — works for both admin and non-admin users. - /// Returns (non-blocking) if the check cannot be completed. + /// Role detection is performed by decoding the wids claim from the MSAL access token + /// (see ), so this does not call Graph and works without + /// any directory-read scope. Returns (non-blocking) + /// when the claim is absent or token acquisition fails. /// public virtual async Task IsCurrentUserAdminAsync( string tenantId, @@ -1076,8 +1109,10 @@ private async Task CreateOrUpdateOauth2PermissionGrantCoreAsync( /// /// Checks whether the currently signed-in user holds the Agent ID Administrator role, /// which is required to create or update inheritable permissions on agent blueprints. - /// Uses only — works for both admin and non-admin users. - /// Returns (non-blocking) if the check cannot be completed. + /// Role detection is performed by decoding the wids claim from the MSAL access token + /// (see ), so this does not call Graph and works without + /// any directory-read scope. Returns (non-blocking) + /// when the claim is absent or token acquisition fails. /// public virtual async Task IsCurrentUserAgentIdAdminAsync( string tenantId, @@ -1089,49 +1124,88 @@ private async Task CreateOrUpdateOauth2PermissionGrantCoreAsync( /// /// Returns if the role is confirmed active, /// if confirmed absent, or - /// if the check itself failed (e.g. network error, - /// throttling, auth failure) — in which case the caller should attempt the operation - /// anyway and let the API surface the real error. - /// Queries /me/transitiveMemberOf/microsoft.graph.directoryRole, which requires only - /// User.Read and succeeds for both admin and non-admin users. - /// Note: PIM-eligible-but-not-activated assignments are not considered active. + /// if the check could not be completed — in + /// which case the caller should attempt the operation anyway and let the API surface the + /// real error. + /// + /// Implementation decodes the wids claim from the MSAL access token (no Graph call). + /// wids is a JWT array of role template GUIDs for the directly-assigned directory roles + /// the user holds. The optional claim must be configured on the app registration's access + /// token; when absent we return . + /// + /// Limitations: + /// - Only directly-assigned roles appear in wids. Roles assigned via Entra + /// role-assignable groups are NOT reflected and will return DoesNotHaveRole. + /// - PIM-eligible-but-not-activated assignments are not active and are correctly excluded. + /// PIM-active assignments do appear in wids. /// private async Task CheckDirectoryRoleAsync(string tenantId, string roleTemplateId, CancellationToken ct) { + // Decode the wids claim from the MSAL access token instead of calling Graph. + // wids contains role template GUIDs for directory roles the user directly holds. + // Limitation: group-based role assignments are not reflected in wids. + // If wids is absent (optional claim not configured on the app registration), + // we return Unknown and the caller proceeds without role validation. try { - // /me/transitiveMemberOf is a directory query — Directory.Read.All is required. - // User.Read is insufficient and would return Unknown for most users. - IEnumerable? scopes = _tokenProvider != null - ? [AuthenticationConstants.DirectoryReadAllScope] - : null; - - string? nextUrl = "/v1.0/me/transitiveMemberOf/microsoft.graph.directoryRole?$select=roleTemplateId"; - - while (nextUrl != null) + // CRITICAL: token MUST be issued for the custom client app (CustomClientAppId), + // not the PowerShell well-known clientId that GetGraphAccessTokenAsync would use. + // The `wids` optional claim is configured per-app-registration on the access token — + // the custom client app has it (see custom-client-app-registration.md, Step 5); + // the PowerShell well-known app does not. A token from the PowerShell app would never + // include `wids`, and this method would always return Unknown, which would in turn + // cause BatchPermissionsOrchestrator to treat real Global Admins as non-admins. + // + // We request User.Read here purely to satisfy the token request — the scopes grant + // is irrelevant for the role check; the claim set on the issued token is what matters. + if (_tokenProvider == null) { - using var doc = await GraphGetAsync(tenantId, nextUrl, ct, scopes); + _logger.LogDebug("Role check skipped — no token provider configured (cannot acquire a custom-app token that would carry wids)."); + return Models.RoleCheckResult.Unknown; + } + // We don't guard on CustomClientAppId being null here. Production callers + // (BatchPermissionsOrchestrator, BlueprintSubcommand, SetupHelpers) all invoke role + // checks AFTER the bootstrap phase has resolved CustomClientAppId. Tests pass the + // token provider directly and don't always set CustomClientAppId — that's fine + // because the mocked provider returns a hand-crafted JWT regardless of clientId. + var loginHint = await ResolveLoginHintAsync(); + var token = await _tokenProvider.GetMgGraphAccessTokenAsync( + tenantId, + [AuthenticationConstants.UserReadScope], + useDeviceCode: false, + clientAppId: CustomClientAppId, + ct: ct, + loginHint: loginHint, + forceRefresh: false); + if (string.IsNullOrWhiteSpace(token)) + return Models.RoleCheckResult.Unknown; - if (doc == null) - return Models.RoleCheckResult.Unknown; + var parts = token.Split('.'); + if (parts.Length < 2) + return Models.RoleCheckResult.Unknown; - if (!doc.RootElement.TryGetProperty("value", out var roles)) - { - _logger.LogWarning("Unexpected Graph response shape — 'value' property missing from transitiveMemberOf response."); - return Models.RoleCheckResult.Unknown; - } + var payload = parts[1]; + payload = payload.Replace('-', '+').Replace('_', '/'); + payload = (payload.Length % 4) switch + { + 2 => payload + "==", + 3 => payload + "=", + _ => payload + }; - if (roles.EnumerateArray().Any(r => - r.TryGetProperty("roleTemplateId", out var id) && - string.Equals(id.GetString(), roleTemplateId, StringComparison.OrdinalIgnoreCase))) - return Models.RoleCheckResult.HasRole; + var json = System.Text.Encoding.UTF8.GetString(Convert.FromBase64String(payload)); + using var doc = JsonDocument.Parse(json); - nextUrl = doc.RootElement.TryGetProperty("@odata.nextLink", out var nextLink) - ? nextLink.GetString() - : null; + if (!doc.RootElement.TryGetProperty("wids", out var wids)) + { + _logger.LogDebug("wids claim absent from token — role check skipped (add wids as optional claim in Token Configuration on the app registration)"); + return Models.RoleCheckResult.Unknown; } - return Models.RoleCheckResult.DoesNotHaveRole; + return wids.EnumerateArray().Any(w => + string.Equals(w.GetString(), roleTemplateId, StringComparison.OrdinalIgnoreCase)) + ? Models.RoleCheckResult.HasRole + : Models.RoleCheckResult.DoesNotHaveRole; } catch (Exception ex) { @@ -1515,7 +1589,7 @@ public virtual async Task DeleteAgentInstanceAsync( using var httpClient = HttpClientFactory.CreateAuthenticatedClient(correlationId: effectiveCorrelationId); var tokenEndpoint = $"https://login.microsoftonline.com/{tenantId}/oauth2/v2.0/token"; - const int maxRetries = 5; + const int maxRetries = 12; const int baseDelaySeconds = 5; for (int attempt = 0; attempt < maxRetries; attempt++) @@ -1540,10 +1614,13 @@ public virtual async Task DeleteAgentInstanceAsync( var errorContent = await response.Content.ReadAsStringAsync(ct); - // AADSTS7000215 means the credential exists in AAD but is not yet visible on this - // replica — same eventual consistency window as object replication. Retry with backoff. - var isCredentialPropagationLag = response.StatusCode == System.Net.HttpStatusCode.Unauthorized - && errorContent.Contains("AADSTS7000215", StringComparison.OrdinalIgnoreCase); + // AADSTS7000215: credential exists but not yet visible on this STS replica. + // AADSTS700016: blueprint app itself not yet visible on this STS replica. + // Both are eventual-consistency propagation lag — retry with backoff. + // AADSTS700016 / AADSTS7000215 are returned as 400 or 401 depending on the STS replica. + var isCredentialPropagationLag = + (errorContent.Contains("AADSTS7000215", StringComparison.OrdinalIgnoreCase) + || errorContent.Contains("AADSTS700016", StringComparison.OrdinalIgnoreCase)); if (!isCredentialPropagationLag || attempt == maxRetries - 1) { @@ -1557,9 +1634,9 @@ public virtual async Task DeleteAgentInstanceAsync( } var delaySecs = Math.Min(baseDelaySeconds * (int)Math.Pow(2, attempt), 60); - _logger.LogInformation( - "Blueprint credential not yet propagated (AADSTS7000215) — retrying in {Delay}s (attempt {Attempt} of {Max})...", - delaySecs, attempt + 1, maxRetries - 1); + _logger.LogDebug( + "Blueprint app or credentials not yet propagated (AADSTS7000215/AADSTS700016) — retrying in {Delay}s (attempt {Attempt} of {Max})...", + delaySecs, attempt + 1, maxRetries); await Task.Delay(TimeSpan.FromSeconds(delaySecs), ct); } @@ -1753,60 +1830,62 @@ public virtual async Task DeleteAgentInstanceAsync( }; } - using var content = new StringContent( - body.ToJsonString(), - System.Text.Encoding.UTF8, - "application/json"); - - using var response = await httpClient.PostAsync( - "https://graph.microsoft.com/beta/serviceprincipals/Microsoft.Graph.AgentIdentity", - content, - ct); + const int maxAttempts = 5; + const int baseDelaySeconds = 5; + const string agentIdentityUrl = "https://graph.microsoft.com/beta/serviceprincipals/Microsoft.Graph.AgentIdentity"; - // Some tenants reject sponsor binding — retry without it. - if (!response.IsSuccessStatusCode && response.StatusCode == System.Net.HttpStatusCode.BadRequest - && !string.IsNullOrWhiteSpace(currentUserId)) + for (int attempt = 0; attempt < maxAttempts; attempt++) { - _logger.LogDebug("Agent identity creation with sponsor failed (400); retrying without sponsor."); - body.Remove("sponsors@odata.bind"); - using var content2 = new StringContent(body.ToJsonString(), System.Text.Encoding.UTF8, "application/json"); - using var response2 = await httpClient.PostAsync( - "https://graph.microsoft.com/beta/serviceprincipals/Microsoft.Graph.AgentIdentity", - content2, - ct); - - if (!response2.IsSuccessStatusCode) + // StringContent is a one-shot stream — must be recreated each attempt. + using var content = new StringContent(body.ToJsonString(), System.Text.Encoding.UTF8, "application/json"); + using var response = await httpClient.PostAsync(agentIdentityUrl, content, ct); + + // Some tenants reject sponsor binding — remove and retry immediately. + if (!response.IsSuccessStatusCode && response.StatusCode == System.Net.HttpStatusCode.BadRequest + && body.ContainsKey("sponsors@odata.bind")) { - var err = await response2.Content.ReadAsStringAsync(ct); - _logger.LogError("Failed to create agent identity: {Status} - {Error}", response2.StatusCode, err); - return null; + _logger.LogDebug("Agent identity creation with sponsor failed (400); retrying without sponsor."); + body.Remove("sponsors@odata.bind"); + continue; } - var json2 = await response2.Content.ReadAsStringAsync(ct); - using var doc2 = JsonDocument.Parse(json2); - var id2 = doc2.RootElement.GetProperty("id").GetString(); - _logger.LogInformation("Agent identity created (ID: {Id})", id2); - return id2; - } + if (response.IsSuccessStatusCode) + { + var json = await response.Content.ReadAsStringAsync(ct); + using var doc = JsonDocument.Parse(json); + var id = doc.RootElement.GetProperty("id").GetString(); + _logger.LogInformation("Agent identity created (ID: {Id})", id); + return id; + } - if (!response.IsSuccessStatusCode) - { var err = await response.Content.ReadAsStringAsync(ct); - _logger.LogError("Failed to create agent identity: {Status} - {Error}", response.StatusCode, err); - if (err.Contains("Authorization_RequestDenied", StringComparison.OrdinalIgnoreCase) || - err.Contains("calling identity type", StringComparison.OrdinalIgnoreCase)) + + // Authorization_IdentityNotFound: blueprint SP not yet propagated to the agent identity + // service — same eventual consistency window as SP replication. Retry with backoff. + var isIdentityNotFound = response.StatusCode == System.Net.HttpStatusCode.Unauthorized + && err.Contains("Authorization_IdentityNotFound", StringComparison.OrdinalIgnoreCase); + + if (!isIdentityNotFound || attempt == maxAttempts - 1) { - _logger.LogError("Authorization denied. Ensure the blueprint application has " + - "Application.ReadWrite.All and AgentIdentity.Create.OwnedBy application permissions."); + _logger.LogError("Failed to create agent identity: {Status} - {Error}", response.StatusCode, err); + if (err.Contains("Authorization_RequestDenied", StringComparison.OrdinalIgnoreCase) || + err.Contains("calling identity type", StringComparison.OrdinalIgnoreCase)) + { + _logger.LogError("Authorization denied. Ensure the blueprint application has the " + + "AgentIdentity.CreateAsManager app role (auto-granted to Blueprint apps). " + + "Re-run 'a365 setup blueprint' to recreate the blueprint if setup was incomplete."); + } + return null; } - return null; + + var delaySecs = Math.Min(baseDelaySeconds * (int)Math.Pow(2, attempt), 60); + _logger.LogDebug( + "Blueprint identity not yet propagated (Authorization_IdentityNotFound) — retrying in {Delay}s (attempt {Attempt} of {Max})...", + delaySecs, attempt + 1, maxAttempts); + await Task.Delay(TimeSpan.FromSeconds(delaySecs), ct); } - var json = await response.Content.ReadAsStringAsync(ct); - using var doc = JsonDocument.Parse(json); - var id = doc.RootElement.GetProperty("id").GetString(); - _logger.LogInformation("Agent identity created (ID: {Id})", id); - return id; + return null; } catch (Exception ex) { diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs index 81362e07..4d275bd7 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs @@ -260,10 +260,10 @@ private static void RegisterPersistentCache(IPublicClientApplication app, ILogge } catch (Exception ex) { - // Cache registration failure is non-fatal - authentication will still work, - // but users may see more prompts during multi-step operations - logger?.LogDebug(ex, "Failed to register persistent token cache"); - logger?.LogWarning("Failed to register persistent token cache. Authentication prompts may be repeated."); + // Cache registration failure is non-fatal — authentication still works and + // the user can do nothing to remediate (no D-Bus/Keychain on headless Linux is + // the common cause), so this stays at Debug rather than surfacing as a warning. + logger?.LogDebug(ex, "Failed to register persistent token cache; auth prompts may be repeated within this session."); } } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/PlatformDetector.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/PlatformDetector.cs index 38473bc2..22f3e849 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/PlatformDetector.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/PlatformDetector.cs @@ -66,7 +66,7 @@ public Models.ProjectPlatform Detect(string projectPath) return Models.ProjectPlatform.Python; } - _logger.LogWarning("Could not detect project platform in: {Path}", projectPath); + _logger.LogDebug("Could not detect project platform in: {Path}", projectPath); return Models.ProjectPlatform.Unknown; } } diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Services/TeamsGraphBackendConfigurator.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Services/TeamsGraphBackendConfigurator.cs index 18e08008..cc3779b5 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/TeamsGraphBackendConfigurator.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/TeamsGraphBackendConfigurator.cs @@ -344,10 +344,10 @@ private static string ClassifyFailureReason(string? errorContent) if (!string.IsNullOrEmpty(errorContent) && errorContent.Contains("not the owner", StringComparison.OrdinalIgnoreCase)) { - return "NotOwner"; + return MessagingEndpointFailureReasons.NotOwner; } - return "Other"; + return MessagingEndpointFailureReasons.Other; } private static bool ResponseDetailsContains(string? content, string substring) diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/design.md b/src/Microsoft.Agents.A365.DevTools.Cli/design.md index 6f688846..d105e444 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/design.md +++ b/src/Microsoft.Agents.A365.DevTools.Cli/design.md @@ -15,7 +15,6 @@ Microsoft.Agents.A365.DevTools.Cli/ │ ├── ConfigCommand.cs # a365 config (init, display) │ ├── SetupCommand.cs # a365 setup (blueprint + messaging endpoint) │ ├── CreateInstanceCommand.cs # a365 create-instance (identity, licenses, notifications) -│ ├── DeployCommand.cs # a365 deploy │ ├── CleanupCommand.cs # a365 cleanup (delete resources) │ ├── QueryEntraCommand.cs # a365 query-entra (blueprint-scopes, instance-scopes) │ ├── DevelopCommand.cs # a365 develop (development utilities) @@ -24,12 +23,7 @@ Microsoft.Agents.A365.DevTools.Cli/ │ └── SetupSubcommands/ # Setup workflow components ├── Services/ # Business logic services │ ├── ConfigService.cs # Configuration management -│ ├── DeploymentService.cs # Multiplatform Azure deployment -│ ├── PlatformDetector.cs # Automatic platform detection -│ ├── IPlatformBuilder.cs # Platform builder interface -│ ├── DotNetBuilder.cs # .NET project builder -│ ├── NodeBuilder.cs # Node.js project builder -│ ├── PythonBuilder.cs # Python project builder +│ ├── PlatformDetector.cs # Automatic platform detection (used by publish) │ ├── TeamsGraphBackendConfigurator.cs # Messaging endpoint (Teams Graph backend config) │ ├── GraphApiService.cs # Graph API interactions │ ├── AuthenticationService.cs # MSAL.NET authentication @@ -340,11 +334,13 @@ Commands supporting `--dry-run` skip checks entirely — the `RunChecksOrExitAsy --- -## Multiplatform Deployment Architecture +## Multiplatform Project Detection -### Platform Detection - -The `PlatformDetector` service auto-detects project type from files: +The `PlatformDetector` service auto-detects project type from files. Used by +`a365 publish` and friends to drive platform-specific behavior. (Azure App Service +deployment was previously handled by a now-removed `a365 deploy` command — agent +hosting is now external to the CLI; the developer ships their agent to whatever +host they choose and points `messagingEndpoint` at it.) ```csharp public enum ProjectPlatform @@ -361,47 +357,6 @@ public enum ProjectPlatform Detection priority: .NET > Node.js > Python > Unknown -### Platform Builder Interface - -```csharp -public interface IPlatformBuilder -{ - Task ValidateEnvironmentAsync(); // Check required tools installed - Task CleanAsync(string projectDir); // Clean build artifacts - Task BuildAsync(string projectDir, string outputPath, bool verbose); - Task CreateManifestAsync(string projectDir, string publishPath); -} -``` - -### Deployment Pipeline - -```mermaid -flowchart TD - A[Platform Detection] --> B[Environment Validation] - B --> C[Clean Build Artifacts] - C --> D[Platform-Specific Build] - D --> E[Create Oryx Manifest] - E --> F[Package ZIP] - F --> G[Deploy to Azure App Service] -``` - -1. **Platform Detection** - Auto-detect project type from files -2. **Environment Validation** - Check required tools (dotnet/node/python) -3. **Clean** - Remove previous build artifacts -4. **Build** - Platform-specific build process -5. **Manifest Creation** - Generate Azure Oryx manifest -6. **Package** - Create deployment ZIP -7. **Deploy** - Upload to Azure App Service - -### Restart Mode (`--restart` flag) - -For quick iteration after manual changes to the `publish/` folder: - -```bash -a365 deploy # Full pipeline: steps 1-7 -a365 deploy --restart # Quick mode: steps 6-7 only (packaging + deploy) -``` - --- ## Permissions Architecture @@ -465,7 +420,7 @@ var graphApiService = new GraphApiService(...); var rootCommand = new RootCommand("a365 — Microsoft Agent 365 CLI"); rootCommand.AddCommand(SetupCommand.CreateCommand(logger, configService, ...)); -rootCommand.AddCommand(DeployCommand.CreateCommand(logger, configService, ...)); +rootCommand.AddCommand(PublishCommand.CreateCommand(logger, configService, ...)); // ... more commands return await new CommandLineBuilder(rootCommand) diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AllSubcommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AllSubcommandTests.cs index 05adbf88..f822fd11 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AllSubcommandTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AllSubcommandTests.cs @@ -3,6 +3,7 @@ using FluentAssertions; using Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands; +using Microsoft.Agents.A365.DevTools.Cli.Constants; using Microsoft.Agents.A365.DevTools.Cli.Models; using Microsoft.Agents.A365.DevTools.Cli.Services; using Microsoft.Agents.A365.DevTools.Cli.Services.Helpers; @@ -220,7 +221,7 @@ await backend.DidNotReceiveWithAnyArgs().SetBackendConfigurationAsync( EndpointRegistrationResult.Failed, because: "the step must record a distinct failure state so the summary doesn't misreport it as 'skipped (non-M365 agent)' — null is reserved for non-M365"); ctx.Results.MessagingEndpointFailureReason.Should().Be( - "BlueprintMissing", + MessagingEndpointFailureReasons.BlueprintMissing, because: "a dedicated reason code lets the summary point the user at the blueprint step rather than generic retry guidance"); ctx.Results.Warnings.Should().ContainSingle( w => w.Contains("agent blueprint ID is missing", StringComparison.OrdinalIgnoreCase), @@ -276,14 +277,14 @@ public async Task ExecuteMessagingEndpointStepAsync_NotOwnerFailure_SetsFailureR "blueprint-id", "https://example.com/api/messages", "test-correlation-id") - .Returns((EndpointRegistrationResult.Failed, (string?)"NotOwner")); + .Returns((EndpointRegistrationResult.Failed, (string?)MessagingEndpointFailureReasons.NotOwner)); var ctx = BuildMessagingEndpointContext(backend, isM365: true); await AllSubcommand.ExecuteMessagingEndpointStepAsync(ctx); ctx.Results.MessagingEndpointResult.Should().Be(EndpointRegistrationResult.Failed); - ctx.Results.MessagingEndpointFailureReason.Should().Be("NotOwner"); + ctx.Results.MessagingEndpointFailureReason.Should().Be(MessagingEndpointFailureReasons.NotOwner); ctx.Results.MessagingEndpointRegistered.Should().BeFalse(); } @@ -295,14 +296,14 @@ public async Task ExecuteMessagingEndpointStepAsync_GenericFailure_SetsFailureRe "blueprint-id", "https://example.com/api/messages", "test-correlation-id") - .Returns((EndpointRegistrationResult.Failed, (string?)"Other")); + .Returns((EndpointRegistrationResult.Failed, (string?)MessagingEndpointFailureReasons.Other)); var ctx = BuildMessagingEndpointContext(backend, isM365: true); await AllSubcommand.ExecuteMessagingEndpointStepAsync(ctx); ctx.Results.MessagingEndpointResult.Should().Be(EndpointRegistrationResult.Failed); - ctx.Results.MessagingEndpointFailureReason.Should().Be("Other"); + ctx.Results.MessagingEndpointFailureReason.Should().Be(MessagingEndpointFailureReasons.Other); ctx.Results.MessagingEndpointRegistered.Should().BeFalse(); } } diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/MockToolingServerSubcommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/MockToolingServerSubcommandTests.cs index 00b3b6ac..8b1af729 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/MockToolingServerSubcommandTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/MockToolingServerSubcommandTests.cs @@ -259,63 +259,13 @@ public async Task HandleStartServer_WithInvalidPort_LogsError(int invalidPort) Assert.Contains(invalidPort.ToString(), logCall.Message); } - [Fact] - public async Task HandleStartServer_WithNullPort_UsesDefaultPort() - { - // Start on a dedicated OS thread (LongRunning) so Server.Start() cannot starve the thread pool - // when many tests run in parallel. - _ = Task.Factory.StartNew( - () => MockToolingServerSubcommand.HandleStartServer(null, false, false, false, _testLogger, _mockProcessService).GetAwaiter().GetResult(), - CancellationToken.None, - TaskCreationOptions.LongRunning, - TaskScheduler.Default); - - // Poll until the initial log messages appear (up to 2 s). - var deadline = DateTime.UtcNow.AddSeconds(2); - while (!_testLogger.LogCalls.Any() && DateTime.UtcNow < deadline) - await Task.Delay(10); - - // Assert - Should log foreground startup messages with default port - Assert.NotEmpty(_testLogger.LogCalls); - Assert.Contains(_testLogger.LogCalls, call => - call.Level == LogLevel.Information && - call.Message.Contains("Starting Up MockToolingServer.")); - - Assert.Contains(_testLogger.LogCalls, call => - call.Level == LogLevel.Information && - call.Message.Contains("Press Ctrl+C to stop the server.")); - } - - [Theory] - [InlineData(1)] - [InlineData(5309)] - [InlineData(8080)] - [InlineData(65535)] - public async Task HandleStartServer_WithValidPort_LogsStartingMessage(int validPort) - { - // Start on a dedicated OS thread (LongRunning) so Server.Start() cannot starve the thread pool - // when many tests run in parallel. - _ = Task.Factory.StartNew( - () => MockToolingServerSubcommand.HandleStartServer(validPort, false, false, false, _testLogger, _mockProcessService).GetAwaiter().GetResult(), - CancellationToken.None, - TaskCreationOptions.LongRunning, - TaskScheduler.Default); - - // Poll until the initial log messages appear (up to 2 s). - var deadline = DateTime.UtcNow.AddSeconds(2); - while (!_testLogger.LogCalls.Any() && DateTime.UtcNow < deadline) - await Task.Delay(10); - - // Assert - Should log foreground startup messages - Assert.NotEmpty(_testLogger.LogCalls); - Assert.Contains(_testLogger.LogCalls, call => - call.Level == LogLevel.Information && - call.Message.Contains("Starting Up MockToolingServer.")); - - Assert.Contains(_testLogger.LogCalls, call => - call.Level == LogLevel.Information && - call.Message.Contains("Press Ctrl+C to stop the server.")); - } + // Note: Tests that exercised the foreground "Starting Up MockToolingServer." / "Press Ctrl+C" + // log lines were removed because they launched a real Kestrel server via Server.Start() on a + // fire-and-forget LongRunning task and never tore it down. On Linux CI this caused two failures: + // (a) the Theory data included port 1, which requires root on Linux and never binds, and + // (b) parallel tests collided on the leaked port 5309 binding (address already in use). + // Foreground/background routing, dry-run, invalid-port, and verbose flags are still covered by + // the surrounding tests, none of which actually start a server. [Fact] public async Task HandleStartServer_WithInvalidPort_DoesNotAttemptStartup() diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PermissionsSubcommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PermissionsSubcommandTests.cs index 5ed8625b..691b456e 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PermissionsSubcommandTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PermissionsSubcommandTests.cs @@ -38,6 +38,20 @@ public PermissionsSubcommandTests() _mockGraphApiService = Substitute.ForPartsOf(); _mockBlueprintService = Substitute.ForPartsOf(Substitute.For>(), _mockGraphApiService); _mockConfirmationProvider = Substitute.For(); + + // Short-circuit any virtual GraphApiService methods that would otherwise fall through to + // the real MSAL/az-CLI authentication pipeline on a partial mock. Without this, tests + // that drive ConfigureMcpPermissionsAsync into BatchPermissionsOrchestrator hang on + // Linux CI (no cached creds, no interactive auth available). The orchestrator handles + // a null pre-warm response by skipping Phase 1, which is what every test in this class + // wants because none of them exercise real Graph calls. + _mockGraphApiService.GraphGetAsync(Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any?>()) + .Returns((JsonDocument?)null); + _mockGraphApiService.IsCurrentUserAdminAsync(Arg.Any(), Arg.Any()) + .Returns(Microsoft.Agents.A365.DevTools.Cli.Models.RoleCheckResult.Unknown); + _mockGraphApiService.IsCurrentUserAgentIdAdminAsync(Arg.Any(), Arg.Any()) + .Returns(Microsoft.Agents.A365.DevTools.Cli.Models.RoleCheckResult.Unknown); } #region Command Structure Tests @@ -642,8 +656,8 @@ public void BotSubcommand_Description_ShouldNotReferenceNonExistentEndpointComma botSubcommand.Should().NotBeNull(); botSubcommand!.Description.Should().NotContain("a365 setup endpoint", "the 'a365 setup endpoint' command does not exist - endpoint is registered as part of blueprint setup"); - botSubcommand.Description.Should().Contain("a365 deploy", - "after permissions setup, users should deploy their agent code"); + botSubcommand.Description.Should().Contain("a365 publish", + "after permissions setup, the next a365 command in the workflow is 'publish' to package the agent for the Microsoft 365 Admin Center"); } [Fact] diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/SetupCommandTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/SetupCommandTests.cs index e6431039..355ccc42 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/SetupCommandTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/SetupCommandTests.cs @@ -579,18 +579,49 @@ public async Task SetupAll_AuthMode_InvalidValue_ExitsWithCode1() } /// - /// --authmode is only meaningful for blueprint agents. Passing it with --aiteammate true - /// must be rejected because the combination is undefined. + /// --authmode obo with --aiteammate is redundant (obo is the AI Teammate default) but not + /// conflicting. It must emit a warning and continue — not exit with an error before setup runs. + /// --dry-run is used to avoid hitting real Azure auth in the test environment. /// [Fact] - public async Task SetupAll_AuthMode_WithAiteammateTrue_ExitsWithCode1() + public async Task SetupAll_AuthMode_Obo_WithAiteammateTrue_WarnsAndContinues() { _mockConfigService.LoadAsync(Arg.Any(), Arg.Any()).Returns(Task.FromResult(BlueprintConfig())); var parser = new CommandLineBuilder(BuildSetupCommand()).Build(); - var result = await parser.InvokeAsync("all --aiteammate true --authmode obo", new TestConsole()); + var result = await parser.InvokeAsync("all --aiteammate true --authmode obo --dry-run", new TestConsole()); - result.Should().Be(1, because: "--authmode is not supported with --aiteammate"); + result.Should().Be(0, because: "--authmode obo with --aiteammate must warn and continue, not exit with error"); + _mockLogger.DidNotReceive().Log( + LogLevel.Error, + Arg.Any(), + Arg.Is(o => o.ToString()!.Contains("not supported with --aiteammate")), + Arg.Any(), + Arg.Any>()); + _mockLogger.Received().Log( + LogLevel.Warning, + Arg.Any(), + Arg.Is(o => o.ToString()!.Contains("redundant with --aiteammate")), + Arg.Any(), + Arg.Any>()); + } + + /// + /// --authmode s2s and --authmode both are incompatible with --aiteammate because AI Teammate + /// agents always use OBO via agent user identity. These combinations must exit with code 1. + /// + [Theory] + [InlineData("s2s")] + [InlineData("both")] + public async Task SetupAll_AuthMode_NonObo_WithAiteammateTrue_ExitsWithCode1(string authMode) + { + _mockConfigService.LoadAsync(Arg.Any(), Arg.Any()).Returns(Task.FromResult(BlueprintConfig())); + var parser = new CommandLineBuilder(BuildSetupCommand()).Build(); + + var result = await parser.InvokeAsync($"all --aiteammate true --authmode {authMode}", new TestConsole()); + + result.Should().Be(1, + because: $"--authmode {authMode} is incompatible with --aiteammate — AI Teammate agents always use OBO"); _mockLogger.Received().Log( LogLevel.Error, Arg.Any(), diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Agent365ConfigServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Agent365ConfigServiceTests.cs index c8f2afd7..91ac73d3 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Agent365ConfigServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Agent365ConfigServiceTests.cs @@ -315,6 +315,58 @@ public async Task SaveStateAsync_SavesLocallyEvenWhenNoStaticConfigExists() } } + [Fact] + public async Task SaveStateAsync_NullStringProperty_IsOmittedFromJson() + { + // Arrange — agentBlueprintClientSecret is null (the state before the secret is created). + // SaveStateAsync must NOT write "agentBlueprintClientSecret": null because: + // (a) DefaultIgnoreCondition.WhenWritingNull does not apply to dictionary values + // (dotnet/runtime#30690), so the filter must be applied in ExtractDynamicProperties, and + // (b) an explicit null in the file causes MergeDynamicProperties to overwrite any previously + // written secret with null on the next LoadAsync — the self-reinforcing null cycle. + var statePath = Path.Combine(_testDirectory, "a365.generated.config.json"); + var config = new Agent365Config { AgentBlueprintId = "aaa-111" }; + // AgentBlueprintClientSecret is intentionally left null + + // Act + await _service.SaveStateAsync(config, statePath); + + // Assert — key must be absent, not present with a null value. + // Note: parse the JSON object and check keys directly rather than doing a substring search, + // because "agentBlueprintClientSecretProtected" also contains "agentBlueprintClientSecret" + // as a prefix and would produce a false positive on a raw string check. + var json = await File.ReadAllTextAsync(statePath); + var node = System.Text.Json.Nodes.JsonNode.Parse(json)?.AsObject(); + Assert.NotNull(node); + Assert.False(node!.ContainsKey("agentBlueprintClientSecret"), + "agentBlueprintClientSecret must be omitted when null to prevent the self-reinforcing null cycle on re-run"); + Assert.Contains("aaa-111", json); // sanity: non-null values still written + } + + [Fact] + public async Task SaveStateAsync_NonNullStringProperty_IsWrittenToJson() + { + // Arrange — after CreateBlueprintClientSecretAsync sets the secret, SaveStateAsync must + // persist it so the file contains the real value (fixes the macOS null-secret bug, issue #408). + var statePath = Path.Combine(_testDirectory, "a365.generated.config.json"); + var config = new Agent365Config + { + AgentBlueprintId = "aaa-111", + AgentBlueprintClientSecret = "super-secret-value", + AgentBlueprintClientSecretProtected = false + }; + + // Act + await _service.SaveStateAsync(config, statePath); + + // Assert — parse JSON to avoid dependence on serializer whitespace/ordering. + var json = await File.ReadAllTextAsync(statePath); + var node = System.Text.Json.Nodes.JsonNode.Parse(json)?.AsObject(); + Assert.NotNull(node); + Assert.Equal("super-secret-value", node!["agentBlueprintClientSecret"]?.GetValue()); + Assert.Equal(false, node["agentBlueprintClientSecretProtected"]?.GetValue()); + } + #endregion #region ValidateAsync Tests diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs index ebb4d950..44917f92 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs @@ -432,15 +432,8 @@ public async Task GetAgentInstancesForBlueprintAsync_ReturnsFilteredInstances() })) }); - // Response 2: GET /beta/users/microsoft.graph.agentUser?$filter=agentIdentityBlueprintId eq '...' - // Bulk query returns all agent users for the blueprint; correlated via identityParentId - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent(JsonSerializer.Serialize(new - { - value = new[] { new { id = "user-obj-1", identityParentId = "sp-obj-1" } } - })) - }); + // Note: No Response 2 for agent users — AgentIdUser.ReadWrite.All is not in RequiredClientAppPermissions + // (create-instance is not enabled), so the user query is intentionally skipped and AgentUserId is null. // Act var instances = await service.GetAgentInstancesForBlueprintAsync("tenant-id", blueprintId); @@ -449,7 +442,7 @@ public async Task GetAgentInstancesForBlueprintAsync_ReturnsFilteredInstances() instances.Should().HaveCount(1); instances[0].IdentitySpId.Should().Be("sp-obj-1"); instances[0].DisplayName.Should().Be("Instance A"); - instances[0].AgentUserId.Should().Be("user-obj-1"); + instances[0].AgentUserId.Should().BeNull("AgentIdUser.ReadWrite.All is not in RequiredClientAppPermissions when create-instance is disabled"); } } diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs index f3454aec..7140d47f 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/ClientAppValidatorTests.cs @@ -33,16 +33,11 @@ public class ClientAppValidatorTests // Stable test GUIDs for required permissions — must match between SetupPermissionResolution // and SetupAppInfoWithAllPermissions so the validation resolves all permissions as present. - private const string AgentBlueprintPrincipalCreateId = "aaaa0001-0000-0000-0000-000000000000"; private const string AgentBlueprintReadWriteAllId = "aaaa0002-0000-0000-0000-000000000000"; - private const string AgentBlueprintUpdateAuthId = "aaaa0003-0000-0000-0000-000000000000"; - private const string AgentBlueprintAddRemoveCredsId = "aaaa0004-0000-0000-0000-000000000000"; - private const string DelegatedPermissionGrantReadWriteAllId = "aaaa0005-0000-0000-0000-000000000000"; - private const string DirectoryReadAllId = "aaaa0006-0000-0000-0000-000000000000"; - private const string AgentInstanceReadWriteAllId = "aaaa0007-0000-0000-0000-000000000000"; + private const string AgentBlueprintPrincipalCreateId = "aaaa0003-0000-0000-0000-000000000000"; private const string UserReadId = "aaaa0008-0000-0000-0000-000000000000"; - private const string UserReadWriteAllId = "aaaa0009-0000-0000-0000-000000000000"; private const string AgentRegistrationReadWriteAllId = "aaaa000a-0000-0000-0000-000000000000"; + private const string ApplicationReadAllId = "aaaa000b-0000-0000-0000-000000000000"; // Separate SP object ID used only by the consent-grant path (GetConsentedPermissionsAsync) // so it does not conflict with SetupAdminConsentSp / SetupAdminConsentGrantsEmpty. @@ -192,13 +187,13 @@ await Assert.ThrowsAsync( [Fact] public async Task EnsureValidClientAppAsync_WhenAppMissingSomePermissions_ThrowsClientAppValidationException() { - // Only AgentIdentityBlueprintPrincipal.Create present — missing the other required permissions + // Only AgentIdentityBlueprint.ReadWrite.All present — missing the other required permissions var requiredResourceAccess = $$""" [ { "resourceAppId": "{{AuthenticationConstants.MicrosoftGraphResourceAppId}}", "resourceAccess": [ - {"id": "{{AgentBlueprintPrincipalCreateId}}", "type": "Scope"} + {"id": "{{AgentBlueprintReadWriteAllId}}", "type": "Scope"} ] } ] @@ -220,15 +215,10 @@ public async Task EnsureValidClientAppAsync_WhenMissingAgentRegistrationPermissi { "resourceAppId": "{{AuthenticationConstants.MicrosoftGraphResourceAppId}}", "resourceAccess": [ - {"id": "{{AgentBlueprintPrincipalCreateId}}", "type": "Scope"}, {"id": "{{AgentBlueprintReadWriteAllId}}", "type": "Scope"}, - {"id": "{{AgentBlueprintUpdateAuthId}}", "type": "Scope"}, - {"id": "{{AgentBlueprintAddRemoveCredsId}}", "type": "Scope"}, - {"id": "{{DelegatedPermissionGrantReadWriteAllId}}", "type": "Scope"}, - {"id": "{{DirectoryReadAllId}}", "type": "Scope"}, - {"id": "{{AgentInstanceReadWriteAllId}}", "type": "Scope"}, - {"id": "{{UserReadId}}", "type": "Scope"}, - {"id": "{{UserReadWriteAllId}}", "type": "Scope"} + {"id": "{{AgentBlueprintPrincipalCreateId}}", "type": "Scope"}, + {"id": "{{ApplicationReadAllId}}", "type": "Scope"}, + {"id": "{{UserReadId}}", "type": "Scope"} ] } ] @@ -379,16 +369,11 @@ private ClientAppValidator CreateValidatorWithConfirmation(IConfirmationProvider { "resourceAppId": "{{AuthenticationConstants.MicrosoftGraphResourceAppId}}", "resourceAccess": [ - {"id": "{{AgentBlueprintPrincipalCreateId}}", "type": "Scope"}, {"id": "{{AgentBlueprintReadWriteAllId}}", "type": "Scope"}, - {"id": "{{AgentBlueprintUpdateAuthId}}", "type": "Scope"}, - {"id": "{{AgentBlueprintAddRemoveCredsId}}", "type": "Scope"}, - {"id": "{{DelegatedPermissionGrantReadWriteAllId}}", "type": "Scope"}, - {"id": "{{DirectoryReadAllId}}", "type": "Scope"}, - {"id": "{{AgentInstanceReadWriteAllId}}", "type": "Scope"}, + {"id": "{{AgentBlueprintPrincipalCreateId}}", "type": "Scope"}, {"id": "{{AgentRegistrationReadWriteAllId}}", "type": "Scope"}, - {"id": "{{UserReadId}}", "type": "Scope"}, - {"id": "{{UserReadWriteAllId}}", "type": "Scope"} + {"id": "{{ApplicationReadAllId}}", "type": "Scope"}, + {"id": "{{UserReadId}}", "type": "Scope"} ] } ] @@ -423,16 +408,10 @@ private ClientAppValidator CreateValidatorWithConfirmation(IConfirmationProvider "value": [{ "id": "graph-sp-id-123", "oauth2PermissionScopes": [ - {"id": "{{AgentBlueprintPrincipalCreateId}}", "value": "AgentIdentityBlueprintPrincipal.Create"}, {"id": "{{AgentBlueprintReadWriteAllId}}", "value": "AgentIdentityBlueprint.ReadWrite.All"}, - {"id": "{{AgentBlueprintUpdateAuthId}}", "value": "AgentIdentityBlueprint.UpdateAuthProperties.All"}, - {"id": "{{AgentBlueprintAddRemoveCredsId}}", "value": "AgentIdentityBlueprint.AddRemoveCreds.All"}, - {"id": "{{DelegatedPermissionGrantReadWriteAllId}}", "value": "DelegatedPermissionGrant.ReadWrite.All"}, - {"id": "{{DirectoryReadAllId}}", "value": "Directory.Read.All"}, - {"id": "{{AgentInstanceReadWriteAllId}}", "value": "AgentInstance.ReadWrite.All"}, {"id": "{{AgentRegistrationReadWriteAllId}}", "value": "AgentRegistration.ReadWrite.All"}, - {"id": "{{UserReadId}}", "value": "User.Read"}, - {"id": "{{UserReadWriteAllId}}", "value": "User.ReadWrite.All"} + {"id": "{{ApplicationReadAllId}}", "value": "Application.Read.All"}, + {"id": "{{UserReadId}}", "value": "User.Read"} ] }] } @@ -487,8 +466,7 @@ public async Task EnsureValidClientAppAsync_WhenUserDeclinesWithMissingPermissio "value": [{ "id": "graph-sp-id-123", "oauth2PermissionScopes": [ - {"id": "{{AgentBlueprintPrincipalCreateId}}", "value": "AgentIdentityBlueprintPrincipal.Create"}, - {"id": "{{DelegatedPermissionGrantReadWriteAllId}}", "value": "DelegatedPermissionGrant.ReadWrite.All"} + {"id": "{{AgentBlueprintReadWriteAllId}}", "value": "AgentIdentityBlueprint.ReadWrite.All"} ] }] } @@ -860,10 +838,9 @@ private void SetupAppInfoGetEmpty() } /// - /// Sets up the app info GET with all required permissions (8 with GUIDs + AgentIdentity.Create.All, - /// AgentIdentityBlueprint.DeleteRestore.All, and AgentIdentity.DeleteRestore.All via consent grant). - /// The permission GUIDs match those returned by SetupPermissionResolution so validation passes. - /// The three no-GUID scopes are resolved via GetConsentedPermissionsAsync fallback. + /// Sets up the app info GET with all required permissions. The 5 GUID-resolvable permissions + /// match those returned by SetupPermissionResolution; AgentIdentity.Read.All and + /// AgentIdentity.DeleteRestore.All are resolved via the GetConsentedPermissionsAsync fallback. /// private void SetupAppInfoWithAllPermissions(string appId) { @@ -872,16 +849,11 @@ private void SetupAppInfoWithAllPermissions(string appId) { "resourceAppId": "{{AuthenticationConstants.MicrosoftGraphResourceAppId}}", "resourceAccess": [ - {"id": "{{AgentBlueprintPrincipalCreateId}}", "type": "Scope"}, {"id": "{{AgentBlueprintReadWriteAllId}}", "type": "Scope"}, - {"id": "{{AgentBlueprintUpdateAuthId}}", "type": "Scope"}, - {"id": "{{AgentBlueprintAddRemoveCredsId}}", "type": "Scope"}, - {"id": "{{DelegatedPermissionGrantReadWriteAllId}}", "type": "Scope"}, - {"id": "{{DirectoryReadAllId}}", "type": "Scope"}, - {"id": "{{AgentInstanceReadWriteAllId}}", "type": "Scope"}, + {"id": "{{AgentBlueprintPrincipalCreateId}}", "type": "Scope"}, {"id": "{{AgentRegistrationReadWriteAllId}}", "type": "Scope"}, - {"id": "{{UserReadId}}", "type": "Scope"}, - {"id": "{{UserReadWriteAllId}}", "type": "Scope"} + {"id": "{{ApplicationReadAllId}}", "type": "Scope"}, + {"id": "{{UserReadId}}", "type": "Scope"} ] } ] @@ -893,7 +865,7 @@ private void SetupAppInfoWithAllPermissions(string appId) /// /// Sets up the Microsoft Graph SP permission resolution GET (select includes oauth2PermissionScopes). - /// Returns the 7 required permissions with GUIDs matching the test constants. + /// Returns the 5 required permissions with GUIDs matching the test constants. /// private void SetupPermissionResolution() { @@ -903,16 +875,11 @@ private void SetupPermissionResolution() { "id": "graph-sp-id-123", "oauth2PermissionScopes": [ - {"id": "{{AgentBlueprintPrincipalCreateId}}", "value": "AgentIdentityBlueprintPrincipal.Create"}, {"id": "{{AgentBlueprintReadWriteAllId}}", "value": "AgentIdentityBlueprint.ReadWrite.All"}, - {"id": "{{AgentBlueprintUpdateAuthId}}", "value": "AgentIdentityBlueprint.UpdateAuthProperties.All"}, - {"id": "{{AgentBlueprintAddRemoveCredsId}}", "value": "AgentIdentityBlueprint.AddRemoveCreds.All"}, - {"id": "{{DelegatedPermissionGrantReadWriteAllId}}", "value": "DelegatedPermissionGrant.ReadWrite.All"}, - {"id": "{{DirectoryReadAllId}}", "value": "Directory.Read.All"}, - {"id": "{{AgentInstanceReadWriteAllId}}", "value": "AgentInstance.ReadWrite.All"}, + {"id": "{{AgentBlueprintPrincipalCreateId}}", "value": "AgentIdentityBlueprintPrincipal.Create"}, {"id": "{{AgentRegistrationReadWriteAllId}}", "value": "AgentRegistration.ReadWrite.All"}, - {"id": "{{UserReadId}}", "value": "User.Read"}, - {"id": "{{UserReadWriteAllId}}", "value": "User.ReadWrite.All"} + {"id": "{{ApplicationReadAllId}}", "value": "Application.Read.All"}, + {"id": "{{UserReadId}}", "value": "User.Read"} ] } ] diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTests.cs index b0a3306c..18839587 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/GraphApiServiceTests.cs @@ -508,24 +508,67 @@ public async Task GetServicePrincipalDisplayNameAsync_MissingDisplayNameProperty #region IsCurrentUserAdminAsync + // Role checks now decode the wids claim from the MSAL access token instead of calling Graph. + // Tests provide a fake JWT with the appropriate wids array via FakeAuthReturning(). + + private static string BuildJwtWithWids(params string[] wids) + { + var payloadJson = System.Text.Json.JsonSerializer.Serialize(new { wids }); + var b64 = Convert.ToBase64String(System.Text.Encoding.UTF8.GetBytes(payloadJson)) + .TrimEnd('=').Replace('+', '-').Replace('/', '_'); + return $"eyJhbGciOiJub25lIn0.{b64}."; + } + + private static IAuthenticationService FakeAuthReturning(string? token) + { + var mock = Substitute.For(); + mock.GetAccessTokenAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(token!)); + return mock; + } + + private static GraphApiService CreateServiceWithWids(TestHttpMessageHandler handler, params string[] roleWids) + { + var logger = Substitute.For>(); + var executor = Substitute.For(Substitute.For>()); + var tokenProvider = Substitute.For(); + var jwt = BuildJwtWithWids(roleWids); + // CheckDirectoryRoleAsync now requires the token to come from _tokenProvider (the path + // that uses the custom client app's clientId — the only app with the `wids` optional + // claim configured). The previous AuthenticationService-based mock would not be hit by + // the production code path. + tokenProvider.GetMgGraphAccessTokenAsync( + Arg.Any(), Arg.Any>(), Arg.Any(), + Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(jwt); + return new GraphApiService(logger, executor, FakeAuthReturning(jwt), handler, tokenProvider, + loginHintResolver: () => Task.FromResult(null), + retryHelper: new RetryHelper(NullLogger.Instance, maxRetries: 1, baseDelaySeconds: 0)); + } + + private static GraphApiService CreateServiceWithNullAuth(TestHttpMessageHandler handler) + { + var logger = Substitute.For>(); + var executor = Substitute.For(Substitute.For>()); + var tokenProvider = Substitute.For(); + // Simulate token acquisition failure on the token-provider path (the path + // CheckDirectoryRoleAsync now uses). + tokenProvider.GetMgGraphAccessTokenAsync( + Arg.Any(), Arg.Any>(), Arg.Any(), + Arg.Any(), Arg.Any(), Arg.Any()) + .Returns((string?)null); + return new GraphApiService(logger, executor, FakeAuthReturning(null), handler, tokenProvider, + loginHintResolver: () => Task.FromResult(null), + retryHelper: new RetryHelper(NullLogger.Instance, maxRetries: 1, baseDelaySeconds: 0)); + } + [Fact] public async Task IsCurrentUserAdminAsync_UserWithGlobalAdminRole_ReturnsHasRole() { - // Arrange — user holds the Global Administrator role + // Arrange — MSAL token contains wids claim with Global Administrator template ID using var handler = new TestHttpMessageHandler(); - var service = CreateServiceWithTokenProvider(handler); - - var rolesResponse = new - { - value = new[] - { - new { roleTemplateId = "62e90394-69f5-4237-9190-012177145e10" } // Global Administrator - } - }; - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent(JsonSerializer.Serialize(rolesResponse)) - }); + var service = CreateServiceWithWids(handler, "62e90394-69f5-4237-9190-012177145e10"); // Act var result = await service.IsCurrentUserAdminAsync("tenant-123"); @@ -537,15 +580,9 @@ public async Task IsCurrentUserAdminAsync_UserWithGlobalAdminRole_ReturnsHasRole [Fact] public async Task IsCurrentUserAdminAsync_UserWithNoAdminRole_ReturnsDoesNotHaveRole() { - // Arrange — user has no admin roles + // Arrange — MSAL token contains wids claim with no matching role GUIDs using var handler = new TestHttpMessageHandler(); - var service = CreateServiceWithTokenProvider(handler); - - var rolesResponse = new { value = Array.Empty() }; - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent(JsonSerializer.Serialize(rolesResponse)) - }); + var service = CreateServiceWithWids(handler); // empty wids // Act var result = await service.IsCurrentUserAdminAsync("tenant-123"); @@ -555,19 +592,19 @@ public async Task IsCurrentUserAdminAsync_UserWithNoAdminRole_ReturnsDoesNotHave } [Fact] - public async Task IsCurrentUserAdminAsync_GraphFails_ReturnsUnknown() + public async Task IsCurrentUserAdminAsync_TokenAcquisitionFails_ReturnsUnknown() { - // Arrange — Graph call fails (500 causes GraphGetAsync to return null) + // Arrange — token acquisition returns null (auth failure). The role check now decodes + // wids from the access token, so a failed acquisition (rather than a Graph 500) is the + // realistic failure mode. using var handler = new TestHttpMessageHandler(); - var service = CreateServiceWithTokenProvider(handler); - - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.InternalServerError)); + var service = CreateServiceWithNullAuth(handler); // Act var result = await service.IsCurrentUserAdminAsync("tenant-123"); // Assert - result.Should().Be(RoleCheckResult.Unknown, "a failed Graph call should return Unknown, not DoesNotHaveRole"); + result.Should().Be(RoleCheckResult.Unknown, "a failed token acquisition should return Unknown, not DoesNotHaveRole"); } #endregion @@ -706,15 +743,9 @@ private static GraphApiService CreateServiceWithTokenProvider(TestHttpMessageHan [Fact] public async Task IsCurrentUserAgentIdAdminAsync_UserWithNoRelevantRole_ReturnsDoesNotHaveRole() { - // Arrange — user is an Agent ID developer (no admin roles) + // Arrange — MSAL token contains wids with no Agent ID Administrator GUID using var handler = new TestHttpMessageHandler(); - var service = CreateServiceWithTokenProvider(handler); - - var rolesResponse = new { value = Array.Empty() }; - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent(JsonSerializer.Serialize(rolesResponse)) - }); + var service = CreateServiceWithWids(handler); // empty wids // Act var result = await service.IsCurrentUserAgentIdAdminAsync("tenant-123"); @@ -726,21 +757,9 @@ public async Task IsCurrentUserAgentIdAdminAsync_UserWithNoRelevantRole_ReturnsD [Fact] public async Task IsCurrentUserAgentIdAdminAsync_UserWithAgentIdAdminRole_ReturnsHasRole() { - // Arrange — user holds the Agent ID Administrator role + // Arrange — MSAL token contains wids with Agent ID Administrator template ID using var handler = new TestHttpMessageHandler(); - var service = CreateServiceWithTokenProvider(handler); - - var rolesResponse = new - { - value = new[] - { - new { roleTemplateId = "db506228-d27e-4b7d-95e5-295956d6615f" } // Agent ID Administrator - } - }; - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent(JsonSerializer.Serialize(rolesResponse)) - }); + var service = CreateServiceWithWids(handler, "db506228-d27e-4b7d-95e5-295956d6615f"); // Act var result = await service.IsCurrentUserAgentIdAdminAsync("tenant-123"); @@ -752,22 +771,9 @@ public async Task IsCurrentUserAgentIdAdminAsync_UserWithAgentIdAdminRole_Return [Fact] public async Task IsCurrentUserAgentIdAdminAsync_UserWithGlobalAdminRoleOnly_ReturnsDoesNotHaveRole() { - // Arrange — user is a Global Administrator but not an Agent ID Administrator + // Arrange — MSAL token has Global Administrator GUID but not Agent ID Administrator GUID using var handler = new TestHttpMessageHandler(); - var service = CreateServiceWithTokenProvider(handler); - - // User holds Global Administrator only — not Agent ID Administrator - var rolesResponse = new - { - value = new[] - { - new { roleTemplateId = "62e90394-69f5-4237-9190-012177145e10" } // Global Administrator - } - }; - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK) - { - Content = new StringContent(JsonSerializer.Serialize(rolesResponse)) - }); + var service = CreateServiceWithWids(handler, "62e90394-69f5-4237-9190-012177145e10"); // Act var result = await service.IsCurrentUserAgentIdAdminAsync("tenant-123"); @@ -777,19 +783,19 @@ public async Task IsCurrentUserAgentIdAdminAsync_UserWithGlobalAdminRoleOnly_Ret } [Fact] - public async Task IsCurrentUserAgentIdAdminAsync_GraphReturnsNull_ReturnsUnknown() + public async Task IsCurrentUserAgentIdAdminAsync_TokenAcquisitionFails_ReturnsUnknown() { - // Arrange — Graph call fails (null response simulates network/auth error) + // Arrange — token acquisition returns null (auth failure). The role check now decodes + // wids from the access token, so a failed acquisition (rather than a Graph 500) is the + // realistic failure mode. using var handler = new TestHttpMessageHandler(); - var service = CreateServiceWithTokenProvider(handler); - - handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.InternalServerError)); + var service = CreateServiceWithNullAuth(handler); // Act var result = await service.IsCurrentUserAgentIdAdminAsync("tenant-123"); // Assert - result.Should().Be(RoleCheckResult.Unknown, "a failed Graph call should return Unknown, not DoesNotHaveRole"); + result.Should().Be(RoleCheckResult.Unknown, "a failed token acquisition should return Unknown, not DoesNotHaveRole"); } #endregion diff --git a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/InteractiveGraphAuthServiceTests.cs b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/InteractiveGraphAuthServiceTests.cs index a0b3cc40..7551106f 100644 --- a/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/InteractiveGraphAuthServiceTests.cs +++ b/src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/InteractiveGraphAuthServiceTests.cs @@ -17,10 +17,11 @@ public class InteractiveGraphAuthServiceTests /// This test ensures that all required Graph API scopes are present in the RequiredScopes array. /// If any of these scopes are removed, the test will fail to prevent accidental permission reduction. /// - /// These scopes are critical for Agent Blueprint creation and inheritable permissions configuration: - /// - AgentIdentityBlueprintPrincipal.Create: Required for blueprint SP creation (per Agent ID team — Kyle Marsh; ReadWrite.All is higher privilege and not needed) + /// These scopes are critical for Agent Blueprint creation: /// - AgentIdentityBlueprint.ReadWrite.All: Required for Agent Blueprint operations - /// - AgentIdentityBlueprint.UpdateAuthProperties.All: Required for updating blueprint auth properties + /// (umbrella — covers UpdateAuthProperties, AddRemoveCreds, and DeleteRestore) + /// - AgentIdentityBlueprintPrincipal.Create: Required for Agent Blueprint service principal creation + /// (separate scope; NOT covered by the ReadWrite.All umbrella — 403 without it) /// - User.Read: Basic user profile access for authentication context /// [Fact] @@ -29,9 +30,8 @@ public void RequiredScopes_MustContainAllEssentialPermissions() // Arrange var expectedScopes = new[] { - "https://graph.microsoft.com/AgentIdentityBlueprintPrincipal.Create", "https://graph.microsoft.com/AgentIdentityBlueprint.ReadWrite.All", - "https://graph.microsoft.com/AgentIdentityBlueprint.UpdateAuthProperties.All", + "https://graph.microsoft.com/AgentIdentityBlueprintPrincipal.Create", "https://graph.microsoft.com/User.Read" };