diff --git a/CHANGELOG.md b/CHANGELOG.md index caf715f9..99c9a43d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,8 +40,11 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g - 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. ### Fixed +- `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. +- Consent-required message "You are running as a non-admin user and cannot grant admin consent" replaced with "An administrator must grant tenant-wide consent to proceed" — the message fires when tenant-wide consent for S2S scopes has not yet been granted, not when the caller lacks admin rights. - `setup all --agent-name` re-runs no longer create a duplicate agent registration: the CLI now reads `agentRegistrationId` from `a365.generated.config.json` (when present) and checks for an existing registration before posting a new one. - `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. ### Removed 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 bed3ed86..d267a8b9 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs @@ -324,7 +324,7 @@ private static Command CreateBotSubcommand( var botChecks = GetBotChecks(authValidator); await RequirementsSubcommand.RunChecksOrExitAsync(botChecks, setupConfig, logger, ct); - await ConfigureBotPermissionsAsync( + var success = await ConfigureBotPermissionsAsync( configFile.FullName, logger, configService, @@ -333,6 +333,8 @@ await ConfigureBotPermissionsAsync( graphApiService, blueprintService, false); + if (!success) + context.ExitCode = 1; }); @@ -670,22 +672,36 @@ public static async Task ConfigureBotPermissionsAsync( { var specs = new List(SetupHelpers.GetFixedApiPermissionSpecs(setInheritable: true)); + var localResults = setupResults ?? new SetupResults(); var (_, _, consentGranted, _) = await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync( graphService, blueprintService, setupConfig, setupConfig.AgentBlueprintId!, setupConfig.TenantId, - specs, logger, setupResults, cancellationToken, + specs, logger, localResults, cancellationToken, knownBlueprintSpObjectId: setupConfig.AgentBlueprintServicePrincipalObjectId); await configService.SaveStateAsync(setupConfig); + // != true treats both explicit failure (false) and skipped-due-to-error (null) as failure. + // Consent state is checked before S2S so the non-admin path still shows "consent required" + // rather than the S2S warning (S2S is never attempted when consent isn't granted). + var s2sFailed = localResults.S2SAppRoleGranted != true; + logger.LogInformation(""); - logger.LogInformation("Bot API permissions configured successfully"); + if (!s2sFailed && consentGranted) + logger.LogInformation("Bot API permissions configured successfully"); + else if (!consentGranted) + logger.LogInformation("Bot API permissions configured; admin consent required"); + else + logger.LogWarning( + "Bot API permissions configured, but S2S app role assignment failed. " + + "Re-run 'a365 setup permissions bot' as {Roles} to retry.", + AuthenticationConstants.S2SGrantRequiredRoles); logger.LogInformation(""); if (!iSetupAll) { logger.LogInformation("Next step: Deploy your agent (run 'a365 deploy' if hosting on Azure)"); } - return consentGranted; + return consentGranted && !s2sFailed; } 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 7ba3d456..81362e07 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs @@ -496,17 +496,17 @@ private void LogConsentRequiredAndThrow(Exception inner) { var consentUrl = ClientAppValidationException.BuildAdminConsentUrl(_clientAppId, _tenantId); _logger?.LogWarning("Admin consent has not been granted for this application."); - _logger?.LogWarning("You are running as a non-admin user and cannot grant admin consent."); + _logger?.LogWarning("An administrator must grant tenant-wide consent to proceed."); if (consentUrl != null) { - _logger?.LogWarning("Share this URL with a Global Administrator to grant consent:"); + _logger?.LogWarning("Share this URL with an administrator to grant consent:"); _logger?.LogWarning(" {ConsentUrl}", consentUrl); } _logger?.LogWarning("After consent is granted, re-run the command."); throw new MsalAuthenticationFailedException( consentUrl != null - ? $"Admin consent required. Share this URL with a Global Administrator: {consentUrl}" - : "Admin consent required. A Global Administrator must grant tenant-wide consent for this application.", + ? $"Admin consent required. Share this URL with an administrator: {consentUrl}" + : "Admin consent required. An administrator must grant tenant-wide consent for this application.", inner); } 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 f2aa3e32..102710c7 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 @@ -9,6 +9,7 @@ using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; using System.CommandLine; +using System.Text.Json; using Xunit; namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Commands; @@ -741,5 +742,264 @@ public async Task ConfigureCustomPermissionsAsync_WithEmptyList_SkipsGracefully( // - SetupResults tracking for custom permissions #endregion + + #region ConfigureBotPermissionsAsync — Issue 402 Regression Tests + // Regression tests for the localResults fix: S2S failure was invisible when setupResults=null. + + private Agent365Config ArrangeAdminPath(bool s2sSucceeds) + { + var config = new Agent365Config + { + TenantId = "00000000-0000-0000-0000-000000000000", + AgentBlueprintId = "blueprint-app-id", + AgentBlueprintServicePrincipalObjectId = "blueprint-sp-id" + }; + + _mockGraphApiService.EnsureServicePrincipalForAppIdAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any?>(), Arg.Any()) + .Returns(Task.FromResult("resource-sp-id")); + + _mockGraphApiService.IsCurrentUserAdminAsync( + Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(RoleCheckResult.HasRole)); + + _mockGraphApiService.CreateOrUpdateOauth2PermissionGrantAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any>(), Arg.Any(), Arg.Any?>()) + .Returns(Task.FromResult(true)); + + _mockBlueprintService.SetInheritablePermissionsAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any>(), Arg.Any?>(), Arg.Any()) + .Returns(Task.FromResult<(bool ok, bool alreadyExists, string? error)>((true, false, null))); + + _mockBlueprintService.GrantAppRoleAssignmentAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any>(), Arg.Any?>(), Arg.Any()) + .Returns(Task.FromResult(s2sSucceeds)); + + _mockGraphApiService.GraphGetAsync( + Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any?>()) + .Returns(callInfo => + { + var path = callInfo.ArgAt(1); + return path.Contains("/me") + ? Task.FromResult(JsonDocument.Parse("{\"id\":\"mock-user-id\"}")) + : Task.FromResult(null); + }); + + _mockBlueprintService.VerifyInheritablePermissionsAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any?>()) + .Returns(Task.FromResult<(bool, string[], string?)>((true, Array.Empty(), null))); + + _mockConfigService.SaveStateAsync(Arg.Any()) + .Returns(Task.CompletedTask); + + return config; + } + + [Fact] + public async Task ConfigureBotPermissionsAsync_AdminPath_WhenS2SFailsAndNoExternalSetupResults_ReturnsFalse() + { + // Arrange — admin user, OAuth2 grants succeed, S2S grant fails, no external SetupResults. + var config = ArrangeAdminPath(s2sSucceeds: false); + + // Act — no external SetupResults, mirrors how the CLI handler calls this method. + var result = await PermissionsSubcommand.ConfigureBotPermissionsAsync( + "config.json", + _mockLogger, + _mockConfigService, + _mockExecutor, + config, + _mockGraphApiService, + _mockBlueprintService, + iSetupAll: false, + setupResults: null); + + // Assert + result.Should().BeFalse( + because: "the method must create a local SetupResults so S2S failure is captured " + + "even when called without an external setupResults (the CLI handler path)"); + _mockLogger.Received().Log( + LogLevel.Warning, + Arg.Any(), + Arg.Is(o => (o.ToString() ?? string.Empty).Contains("S2S app role assignment failed")), + Arg.Any(), + Arg.Any>()); + } + + [Fact] + public async Task ConfigureBotPermissionsAsync_WhenGraphAuthFails_DoesNotLogSuccessMessage() + { + // Arrange — all Graph calls return null → phase1Result=null (auth failed); consentGranted=false; S2S not attempted. + var config = new Agent365Config + { + TenantId = "00000000-0000-0000-0000-000000000000", + AgentBlueprintId = "blueprint-app-id" + }; + + _mockGraphApiService.GraphGetAsync( + Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any?>()) + .Returns(Task.FromResult(null)); + + _mockConfigService.SaveStateAsync(Arg.Any()) + .Returns(Task.CompletedTask); + + // Act + var result = await PermissionsSubcommand.ConfigureBotPermissionsAsync( + "config.json", + _mockLogger, + _mockConfigService, + _mockExecutor, + config, + _mockGraphApiService, + _mockBlueprintService, + iSetupAll: false, + setupResults: null); + + // Assert + result.Should().BeFalse(because: "consentGranted=false when Graph auth fails"); + _mockLogger.DidNotReceive().Log( + LogLevel.Information, + Arg.Any(), + Arg.Is(o => (o.ToString() ?? string.Empty).Contains("configured successfully")), + Arg.Any(), + Arg.Any>()); + } + + [Fact] + public async Task ConfigureBotPermissionsAsync_WhenGraphAuthFails_LogsConsentRequiredMessage() + { + // Arrange — all Graph calls return null → phase1Result=null (auth failed); consentGranted=false, S2S not attempted. + var config = new Agent365Config + { + TenantId = "00000000-0000-0000-0000-000000000000", + AgentBlueprintId = "blueprint-app-id" + }; + + _mockGraphApiService.GraphGetAsync( + Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any?>()) + .Returns(Task.FromResult(null)); + + _mockConfigService.SaveStateAsync(Arg.Any()) + .Returns(Task.CompletedTask); + + // Act + var result = await PermissionsSubcommand.ConfigureBotPermissionsAsync( + "config.json", + _mockLogger, + _mockConfigService, + _mockExecutor, + config, + _mockGraphApiService, + _mockBlueprintService, + iSetupAll: false, + setupResults: null); + + // Assert + result.Should().BeFalse(because: "consentGranted=false when Graph auth fails"); + _mockLogger.Received().Log( + LogLevel.Information, + Arg.Any(), + Arg.Is(o => (o.ToString() ?? string.Empty).Contains("admin consent required")), + Arg.Any(), + Arg.Any>()); + } + + [Fact] + public async Task ConfigureBotPermissionsAsync_AdminPath_WhenS2SFails_EmitsS2SWarningAndSuppressesSuccessLog() + { + // Arrange — admin user, OAuth2 grants succeed, S2S grant fails. + var config = ArrangeAdminPath(s2sSucceeds: false); + + // Act + _ = await PermissionsSubcommand.ConfigureBotPermissionsAsync( + "config.json", + _mockLogger, + _mockConfigService, + _mockExecutor, + config, + _mockGraphApiService, + _mockBlueprintService, + iSetupAll: false, + setupResults: null); + + // Assert — S2S failure warning must be logged. + _mockLogger.Received().Log( + LogLevel.Warning, + Arg.Any(), + Arg.Is(o => (o.ToString() ?? string.Empty).Contains("S2S app role assignment failed")), + Arg.Any(), + Arg.Any>()); + + // Assert — success message must not be logged when S2S failed. + _mockLogger.DidNotReceive().Log( + LogLevel.Information, + Arg.Any(), + Arg.Is(o => (o.ToString() ?? string.Empty).Contains("configured successfully")), + Arg.Any(), + Arg.Any>()); + } + + [Fact] + public async Task ConfigureBotPermissionsAsync_AdminPath_WhenAllSucceeds_ReturnsTrueAndLogsSuccess() + { + // Arrange — admin user, all OAuth2 grants succeed, S2S grant succeeds. + var config = ArrangeAdminPath(s2sSucceeds: true); + + // Act + var result = await PermissionsSubcommand.ConfigureBotPermissionsAsync( + "config.json", + _mockLogger, + _mockConfigService, + _mockExecutor, + config, + _mockGraphApiService, + _mockBlueprintService, + iSetupAll: false, + setupResults: null); + + // Assert + result.Should().BeTrue(because: "all grants and S2S succeed — method must return true"); + _mockLogger.Received().Log( + LogLevel.Information, + Arg.Any(), + Arg.Is(o => (o.ToString() ?? string.Empty).Contains("configured successfully")), + Arg.Any(), + Arg.Any>()); + } + + [Fact] + public async Task ConfigureBotPermissionsAsync_AdminPath_WhenS2SFailsWithExternalSetupResults_PopulatesS2SFlagAndReturnsFalse() + { + // Arrange — S2S fails, non-null SetupResults passed in (AllSubcommand path). + var config = ArrangeAdminPath(s2sSucceeds: false); + var externalResults = new SetupResults(); + + // Act + var result = await PermissionsSubcommand.ConfigureBotPermissionsAsync( + "config.json", + _mockLogger, + _mockConfigService, + _mockExecutor, + config, + _mockGraphApiService, + _mockBlueprintService, + iSetupAll: false, + setupResults: externalResults); + + // Assert + result.Should().BeFalse( + because: "S2S failure must propagate via the external SetupResults reference"); + externalResults.S2SAppRoleGranted.Should().BeFalse( + because: "the orchestrator must write S2SAppRoleGranted=false to the caller's SetupResults"); + } + + #endregion }