Add --authmode to setup all: control agent identity permission grants (OBO/S2S/both)

.claude/agents/pr-code-reviewer.md

@@ -186,6 +186,27 @@ For each changed file, analyze:
    - Based on the conditional logic, what specific test scenarios are needed?
    - Generate concrete test code examples
 
+9. **Holistic Related-File Expansion** (for patterns that span unstaged files)
+
+   Do not limit analysis to staged/diff lines. For the following patterns, actively fetch and read related unstaged files before generating findings:
+
+   **A. New property on a model class → check validators (even if not staged)**
+   When the diff adds a new `public` property to any class in `Models/` or `SetupSubcommands/` (e.g., `Agent365Config`, `SetupContext`, `SetupResults`):
+   - Read the full `Validate()` and `ValidateNonDwMinimal()` methods of that class — they may not be in the diff
+   - Also `Grep` for `ValidateAsync` methods in `ConfigService.cs` that take the same model type (see Anti-Pattern #25)
+   - If the new property represents a discrete set of values (e.g., `"obo|s2s|both"`) and no validation exists in any of these methods, flag HIGH (see Anti-Pattern #27)
+
+   **B. New CLI `Option<...>` → read the directory README (even if not staged)**
+   When the diff adds `new Option<...>(` to a command file under `Commands/`:
+   - Read `README.md` in that same directory
+   - If the new option is not documented, flag MEDIUM (missing README update)
+   - Also verify the README does not claim the option is available on commands where it is not wired — check the command class to confirm scope
+
+   **C. Changed observable log message → grep tests for old string**
+   When the diff changes a quoted string in a `LogInformation`/`LogWarning`/`LogError` call:
+   - Run `Grep` for the old string in `src/Tests/**/*.cs`
+   - If test files assert `Contains("old string")` and the old string is gone, the test will silently pass with the wrong message — flag HIGH
+
 ### Step 3: Generate Findings
 
 For each issue found, provide:
@@ -820,6 +841,45 @@ When a required-field check is added, removed, or relaxed in a model's `Validate
   ```
 - **Real example**: Removing `"messagingEndpoint is required when needDeployment is 'no'."` from `Agent365Config.Validate()` without removing the parallel `ValidateRequired(config.MessagingEndpoint, ...)` call in `ConfigService.ValidateAsync`. The fix appeared in `Agent365ConfigTests.cs` and `Agent365Config.cs` but not in `ConfigService.cs`, so `a365 cleanup` still failed with `MessagingEndpoint is required` on bootstrap-path projects.
 
+### 27. New Nullable Config Property Without Validation in `Validate()`
+
+When a new nullable string property is added to a config model class (`Agent365Config`, `SetupContext`, or similar) to represent a discrete set of allowed values (e.g., `obo|s2s|both`), and no validation is added to `Validate()` / `ValidateNonDwMinimal()` / `ValidateAsync()`, the property becomes a silent misconfiguration path: a typo in `a365.config.json` passes through unnoticed and silently disables the feature or skips grant branches.
+
+- **Pattern to catch**:
+  - `[JsonPropertyName("someMode")] public string? SomeMode { get; init; }` added to a model class
+  - No corresponding discrete-value check in `Validate()` / `ValidateNonDwMinimal()` for that property name
+  - Often paired with Anti-Pattern #28 (assignment without whitespace normalization)
+- **Severity**: `high` — incorrect config values silently skip grant/permission branches; user gets no error
+- **Check**: For every new property in a model class diff, read the `Validate()` method of that class (even if not staged) and search for the property name. If absent, flag it.
+- **Fix**: Add a private helper and call it from all `Validate()` overloads:
+  ```csharp
+  private static void ValidateSomeMode(string? value, List<string> errors)
+  {
+      if (value is not null && value is not ("a" or "b" or "c"))
+          errors.Add($"someMode must be 'a', 'b', or 'c' when set (got '{value}').");
+  }
+  ```
+- **Real example (PR #391, Comments 4 & 5)**: `Agent365Config.AuthMode` was added without validation. `a365.config.json` could contain `"authMode": "typo"` and the run would proceed, silently skipping all grant branches.
+
+### 28. Mode/Enum-Like String Property Normalized With `?.ToLowerInvariant()` Instead of `IsNullOrWhiteSpace`
+
+When a string property represents a discrete set of values (e.g., `"obo"`, `"s2s"`, `"both"`), the assignment `AuthMode = authMode?.ToLowerInvariant()` looks correct but lets empty string through: `""?.ToLowerInvariant()` returns `""`, not `null`. An empty string from `a365.config.json` then makes `IsOboMode`/`IsS2sMode`/`IsBothMode` all return `false` — silently skipping all grant branches with no error.
+
+- **Pattern to catch**:
+  - `SomeMode = someMode?.ToLowerInvariant();` where `SomeMode` is a nullable string used in discrete-value comparisons (e.g., `== "obo"`)
+  - Appears in `SetupContext` constructors, settings classes, or model property setters
+- **Severity**: `high` — empty-string config value silently disables all mode-dependent branches
+- **Check**: For every `?.ToLowerInvariant()` assignment in the diff where the left-hand side is compared against a finite set of literals elsewhere, verify the `IsNullOrWhiteSpace` guard is present.
+- **Fix**:
+  ```csharp
+  // Wrong — empty string passes through as a real mode
+  AuthMode = authMode?.ToLowerInvariant();
+
+  // Correct — empty/whitespace becomes null (= use documented default)
+  AuthMode = string.IsNullOrWhiteSpace(authMode) ? null : authMode.Trim().ToLowerInvariant();
+  ```
+- **Real example (PR #391, Comments 7 & 8)**: `SetupContext.AuthMode` and `NonDwBlueprintSetupOrchestrator.effectiveMode` both used `?.ToLowerInvariant()`, allowing `""` to disable OBO without any visible error.
+
 ## Example Invocation
 
 When you receive a request like "Review PR #253", you should:

CHANGELOG.md

@@ -27,6 +27,13 @@ a365 setup admin --config-dir "<path-to-config-dir>"
 ```
 
 ### Added
+- `setup requirements` Global Administrator path: when the well-known CLI client app is not found in a new tenant, Global Admins are prompted to create the app and grant admin consent automatically (enter an app ID or type `C` to create).
+- `--authmode obo|s2s|both` option on `setup all` — controls how the agent identity service principal receives permissions:
+  - `obo` (default): principal-scoped delegated grants (`consentType: "Principal"`); no Global Administrator required.
+  - `s2s`: application role assignments on the agent identity SP; attempted programmatically, falls back to printed PowerShell instructions if the caller lacks Global Administrator.
+  - `both`: applies both OBO delegated grants and S2S app role assignments.
+  - Inheritable permissions (Phase 2a) and AllPrincipals grants (Phase 2b) are always skipped for non-DW agents regardless of `authMode`, to avoid requiring a Global Administrator role.
+  - `authMode` can be persisted in `a365.config.json` to apply on every run without the flag.
 - `--project-path <path>` option on `develop list-configured`, `develop add-mcp-servers`, and `develop remove-mcp-servers` — specify the manifest location without requiring `a365.config.json`.
 - `setup requirements` runs without `a365.config.json` — system checks (PowerShell modules, Frontier enrollment) always run; client app checks run when a config file or Azure CLI session is available.
 - `--agent-name` and `--tenant-id` options added to `setup blueprint`, `setup permissions` (all subcommands), `create-instance`, `publish`, and `query-entra` — all commands can now resolve configuration from Entra ID without requiring `a365.config.json`.
@@ -77,6 +84,8 @@ a365 setup admin --config-dir "<path-to-config-dir>"
 - `ToolingManifest.json` duplicate server detection now falls back to `mcpServerName` when `mcpServerUniqueName` is absent, preventing false duplicate errors for older manifest entries
 
 ### Fixed
+- `setup all` dry-run with `--agent-name` no longer runs az CLI tenant detection — tenant ID is not shown in the plan, so the subprocess was unnecessary
+- `setup all` live summary incorrectly showed `Inheritable Permissions: configured` for non-AI Teammate agents — now shows `skipped (permissions set directly on agent identity)`
 - `AgentBlueprintService.SetInheritablePermissionsAsync` no longer crashes when the Graph PATCH call throws a transient exception (#366) — the exception is caught, logged, and surfaced as a structured error result
 - `cleanup` now returns exit code 1 when no config file and no `--agent-name` are provided, instead of silently reporting success.
 - `AgentBlueprintService.SetInheritablePermissionsAsync` now correctly propagates `OperationCanceledException` when the user cancels (Ctrl+C), instead of masking cancellation as a generic error
@@ -97,6 +106,9 @@ a365 setup admin --config-dir "<path-to-config-dir>"
 - `a365 develop add-mcp-servers` no longer writes the literal string `"null"` as a scope value in `ToolingManifest.json` when the V2 catalog returns `"scope": "null"` — the field is omitted, allowing correct fallback to name-based scope mapping
 - `a365 develop get-token` no longer requests a token with scope `"null"` when a manifest entry has a null scope from the V2 catalog
 - `a365 setup permissions mcp` no longer passes a literal `"default"` string as an AAD resourceAppId — Dataverse custom servers (`McpServers.DataverseCustom.All`, `McpServers.Dataverse.All`) with `"audience": "default"` are now bucketed under the shared ATG AppId, the same as missing or `api://` legacy audiences
+- `a365 setup blueprint` (non-DW) blueprint service principal creation no longer returns 403 — the CLI now uses `POST /v1.0/serviceprincipals/graph.agentIdentityBlueprintPrincipal` (Agent ID-specific endpoint) instead of the generic `/v1.0/servicePrincipals`, which required `Application.ReadWrite.All`
+- `a365 setup all` (non-DW) agent identity idempotency pre-check no longer returns 403 — uses `AgentIdentity.Read.All` scope for `GET /beta/servicePrincipals/microsoft.graph.agentIdentity?$filter=agentIdentityBlueprintId eq '...'`
+- Agent registration endpoint promoted from `/stagingbeta/copilot/agentRegistrations` to `/beta/copilot/agentRegistrations`
 
 ### Removed
 - `a365 deploy` command (`deploy app`, `deploy mcp`) — Azure App Service hosting is no longer managed by the CLI. Provide a `messagingEndpoint` in `a365.config.json` pointing to your externally hosted agent.

src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs

@@ -134,6 +134,14 @@ public static Command CreateCommand(
             description: "Treat this agent as an M365 agent. When set, registers the messaging endpoint via MCP Platform. " +
                         "Default is false (opt-in); omit this flag for non-M365 agents.");
 
+        var authModeOption = new Option<string?>(
+            "--authmode",
+            description: "Authentication pattern for the agent identity (blueprint agents only).\n" +
+                         "  obo  — on-behalf-of (default); principal-scoped delegated grants; no admin consent needed.\n" +
+                         "  s2s  — service-to-service; app permissions on agent identity; Global Admin needed or PowerShell fallback.\n" +
+                         "  both — delegated grants (OBO) and app permissions (S2S).\n" +
+                         "Not supported with --aiteammate true.");
+
         command.AddOption(verboseOption);
         command.AddOption(dryRunOption);
         command.AddOption(skipInfrastructureOption);
@@ -143,6 +151,7 @@ public static Command CreateCommand(
         command.AddOption(m365Option);
         command.AddOption(agentNameOption);
         command.AddOption(tenantIdOption);
+        command.AddOption(authModeOption);
 
         command.SetHandler(async (System.CommandLine.Invocation.InvocationContext context) =>
         {
@@ -159,8 +168,23 @@ public static Command CreateCommand(
             var agentName = context.ParseResult.GetValueForOption(agentNameOption);
             var tenantIdFlag = context.ParseResult.GetValueForOption(tenantIdOption);
             bool isM365 = context.ParseResult.GetValueForOption(m365Option);
+            var authMode = context.ParseResult.GetValueForOption(authModeOption)?.ToLowerInvariant();
             var ct = context.GetCancellationToken();
 
+            // --authmode validation
+            if (authMode is not null && authMode is not ("obo" or "s2s" or "both"))
+            {
+                logger.LogError("Invalid --authmode value '{Value}'. Allowed values: obo, s2s, both.", authMode);
+                context.ExitCode = 1;
+                return;
+            }
+            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;
+            }
+
             // Generate correlation ID at workflow entry point
             var correlationId = HttpClientFactory.GenerateCorrelationId();
             logger.LogDebug("Starting setup all (CorrelationId: {CorrelationId})", correlationId);
@@ -176,13 +200,11 @@ public static Command CreateCommand(
                 {
                     if (dryRun)
                     {
-                        // Dry-run: detect tenant only (no client app lookup needed for display)
-                        var dryRunTenantId = tenantIdFlag;
-                        if (string.IsNullOrWhiteSpace(dryRunTenantId))
-                            dryRunTenantId = await SetupHelpers.ResolveBootstrapTenantIdAsync(null, executor, logger);
+                        // Dry-run: build config from flags only — no az CLI subprocess needed.
+                        // TenantId is not shown in the plan so detection is skipped intentionally.
                         nonDwConfig = new Agent365Config
                         {
-                            TenantId = dryRunTenantId ?? "(unknown — run 'az login' or pass --tenant-id)",
+                            TenantId = tenantIdFlag ?? string.Empty,
                             ClientAppId = string.Empty,
                             AgentIdentityDisplayName = $"{agentName} Identity",
                             AgentBlueprintDisplayName = $"{agentName} Blueprint",
@@ -257,6 +279,18 @@ public static Command CreateCommand(
                 }
             }
 
+            // Validate the effective authMode (flag OR config). The CLI flag was validated above;
+            // this re-check catches an invalid authMode persisted in a365.config.json that was not
+            // caught at load time (e.g. a user manually edited the file with a bad value).
+            var effectiveAuthModeForValidation = authMode ?? nonDwConfig?.AuthMode?.Trim().ToLowerInvariant();
+            if (!string.IsNullOrWhiteSpace(effectiveAuthModeForValidation) &&
+                effectiveAuthModeForValidation is not ("obo" or "s2s" or "both"))
+            {
+                logger.LogError("Invalid authMode value '{Value}' (from --authmode flag or a365.config.json). Allowed values: obo, s2s, both.", effectiveAuthModeForValidation);
+                context.ExitCode = 1;
+                return;
+            }
+
             // AI Teammate (DW) agents are M365 agents by design — auto-enable messaging endpoint.
             // --m365 remains opt-in for blueprint agents (non-DW path).
             if (nonDwConfig is null)
@@ -267,7 +301,8 @@ public static Command CreateCommand(
                 if (dryRun)
                 {
                     var rawArgs = context.ParseResult.Tokens.Select(t => t.Value).ToArray();
-                    NonDwBlueprintSetupOrchestrator.PrintDryRunPlan(nonDwConfig, logger, isBootstrap, rawArgs, skipRequirements, isM365, agentRegistrationOnly);
+                    var effectiveAuthMode = authMode ?? nonDwConfig.AuthMode;
+                    NonDwBlueprintSetupOrchestrator.PrintDryRunPlan(nonDwConfig, logger, isBootstrap, rawArgs, skipRequirements, isM365, agentRegistrationOnly, effectiveAuthMode);
                     return;
                 }
 
@@ -302,6 +337,7 @@ public static Command CreateCommand(
                     agentInstanceOnly: agentRegistrationOnly,
                     isBootstrap: isBootstrap,
                     isM365: isM365,
+                    authMode: authMode ?? nonDwConfig.AuthMode,
                     confirmationProvider: confirmationProvider);
 
                 context.ExitCode = await NonDwBlueprintSetupOrchestrator.ExecuteAsync(nonDwCtx);

src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs

@@ -1215,7 +1215,7 @@ public static async Task<bool> EnsureDelegatedConsentWithRetriesAsync(
         ILogger logger,
         CancellationToken ct)
     {
-        var createSpUrl = $"{Constants.GraphApiConstants.BaseUrl}/v1.0/servicePrincipals";
+        var createSpUrl = $"{Constants.GraphApiConstants.BaseUrl}/v1.0/serviceprincipals/graph.agentIdentityBlueprintPrincipal";
         var spManifestJson = new JsonObject { ["appId"] = appId }.ToJsonString();
         int forbiddenRetries = 0;
         const int maxForbiddenRetries = 3;