Skip to content

Commit 07f1613

Browse files
biswapmbiswapm
committed
feat: add V1/V2 migration safety guards for add-mcp-servers, permissions mcp, and list-configured
- add-mcp-servers: LogWarning when updated/added server resolves to legacy ATG audience (null, api://, or explicit ATG AppId), so operators know to re-run after V2 endpoint is live - setup permissions mcp: validate all manifest scopes are known (V1 pattern, V2 value, or McpServersMetadata.Read.All) before writing blueprint; returns false with LogError on unknown scopes - list-configured: derive and display VERSION column (V1/V2/Unknown) from each server's scope
1 parent d134dbe commit 07f1613

4 files changed

Lines changed: 282 additions & 0 deletions

File tree

‎src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopCommand.cs‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -387,7 +387,13 @@ private static Command CreateListConfiguredSubcommand(ILogger logger, IConfigSer
387387
if (string.IsNullOrWhiteSpace(audience)) audience = mappedAudience ?? "";
388388
}
389389

390+
// Derive protocol version from scope
391+
var version = McpConstants.IsV1Scope(scope) ? "V1"
392+
: string.Equals(scope, McpConstants.V2ScopeValue, StringComparison.OrdinalIgnoreCase) ? "V2"
393+
: "Unknown";
394+
390395
logger.LogInformation(" {Name}", serverName);
396+
logger.LogInformation(" Version: {Version}", version);
391397
logger.LogInformation(" URL: {Url}", serverUrl);
392398

393399
if (!string.IsNullOrWhiteSpace(scope))
@@ -780,6 +786,16 @@ private static (List<object> updatedServers, int addedCount, int updatedCount) U
780786
updatedServers.Add(updatedServerObject);
781787
updatedCount++;
782788
logger.LogInformation("Updated existing server: {Server}", existingServerName);
789+
790+
// Warn when the resolved audience is still the legacy ATG AppId (V1 entry)
791+
var resolvedAudience = string.IsNullOrWhiteSpace(audience) ||
792+
audience.StartsWith("api://", StringComparison.OrdinalIgnoreCase)
793+
? McpConstants.Agent365ToolsProdAppId
794+
: audience;
795+
if (string.Equals(resolvedAudience, McpConstants.Agent365ToolsProdAppId, StringComparison.OrdinalIgnoreCase))
796+
{
797+
logger.LogWarning("{Server} uses legacy ATG audience. Re-run add-mcp-servers after V2 endpoint is live.", existingServerName);
798+
}
783799
}
784800
else
785801
{
@@ -839,6 +855,16 @@ private static (List<object> updatedServers, int addedCount, int updatedCount) U
839855

840856
var serverObject = ManifestHelper.CreateCompleteServerObject(serverName, serverName, url, scope, audience);
841857
updatedServers.Add(serverObject);
858+
859+
// Warn when the resolved audience is still the legacy ATG AppId (V1 entry)
860+
var resolvedAudienceForNew = string.IsNullOrWhiteSpace(audience) ||
861+
audience.StartsWith("api://", StringComparison.OrdinalIgnoreCase)
862+
? McpConstants.Agent365ToolsProdAppId
863+
: audience;
864+
if (string.Equals(resolvedAudienceForNew, McpConstants.Agent365ToolsProdAppId, StringComparison.OrdinalIgnoreCase))
865+
{
866+
logger.LogWarning("{Server} uses legacy ATG audience. Re-run add-mcp-servers after V2 endpoint is live.", serverName);
867+
}
842868
}
843869

844870
return (updatedServers, addedCount, updatedCount);

‎src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -399,6 +399,23 @@ public static async Task<bool> ConfigureMcpPermissionsAsync(
399399
return true;
400400
}
401401

402+
// Validate all scopes are known: V1 pattern, V2 value, or metadata scope
403+
var unknownScopes = scopesByAudience.Values
404+
.SelectMany(s => s)
405+
.Where(s =>
406+
!McpConstants.IsV1Scope(s) &&
407+
!string.Equals(s, McpConstants.V2ScopeValue, StringComparison.OrdinalIgnoreCase) &&
408+
!string.Equals(s, "McpServersMetadata.Read.All", StringComparison.OrdinalIgnoreCase))
409+
.Distinct(StringComparer.OrdinalIgnoreCase)
410+
.ToList();
411+
412+
if (unknownScopes.Count > 0)
413+
{
414+
foreach (var unknownScope in unknownScopes)
415+
logger.LogError("Unknown scope '{Scope}'. Re-run: a365 develop add-mcp-servers.", unknownScope);
416+
return false;
417+
}
418+
402419
var specs = scopesByAudience
403420
.Select(kvp => new ResourcePermissionSpec(
404421
kvp.Key, "Agent 365 Tools", kvp.Value, SetInheritable: true))

‎src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/DevelopCommandV1V2MigrationTests.cs‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT License.
3+
4+
using Microsoft.Agents.A365.DevTools.Cli.Constants;
5+
using Xunit;
6+
7+
namespace Microsoft.Agents.A365.DevTools.Cli.Tests.Commands;
8+
9+
/// <summary>
10+
/// Unit tests for V1/V2 migration logic in DevelopCommand:
11+
/// - ATG audience resolution used by the add-mcp-servers legacy warning (Item 1)
12+
/// - VERSION column derivation used by list-configured (Item 3)
13+
/// </summary>
14+
public class DevelopCommandV1V2MigrationTests
15+
{
16+
// ── Item 1: audience resolution (ATG fallback logic) ────────────────────
17+
18+
[Theory]
19+
[InlineData(null, true)] // missing → ATG
20+
[InlineData("", true)] // empty → ATG
21+
[InlineData(" ", true)] // whitespace → ATG
22+
[InlineData("api://mcp-mailtools", true)] // legacy api:// → ATG
23+
[InlineData("API://UPPER", true)] // api:// case-insensitive → ATG
24+
[InlineData("ea9ffc3e-8a23-4a7d-836d-234d7c7565c1", true)] // explicit ATG AppId → ATG
25+
[InlineData("EA9FFC3E-8A23-4A7D-836D-234D7C7565C1", true)] // ATG AppId upper-case → ATG
26+
[InlineData("05879165-0320-489e-b644-f72b33f3edf0", false)] // per-server GUID → not ATG
27+
[InlineData("2cc60bb0-1024-48c8-95f0-1fce211a04d8", false)] // different per-server GUID → not ATG
28+
public void AudienceResolution_MatchesAtgFallbackRules(string? rawAudience, bool expectsAtg)
29+
{
30+
// This mirrors the resolution logic in UpsertMcpServersInManifest:
31+
// resolved = (null/empty/api://) ? ATG AppId : rawAudience
32+
var resolved = string.IsNullOrWhiteSpace(rawAudience) ||
33+
rawAudience.StartsWith("api://", StringComparison.OrdinalIgnoreCase)
34+
? McpConstants.Agent365ToolsProdAppId
35+
: rawAudience;
36+
37+
var isAtg = string.Equals(resolved, McpConstants.Agent365ToolsProdAppId,
38+
StringComparison.OrdinalIgnoreCase);
39+
40+
Assert.Equal(expectsAtg, isAtg);
41+
}
42+
43+
// ── Item 3: VERSION column derivation ───────────────────────────────────
44+
45+
[Theory]
46+
[InlineData("McpServers.Mail.All", "V1")]
47+
[InlineData("McpServers.Calendar.All", "V1")]
48+
[InlineData("mcpservers.teams.all", "V1")] // case-insensitive V1
49+
[InlineData("Tools.ListInvoke.All", "V2")]
50+
[InlineData("tools.listinvoke.all", "V2")] // case-insensitive V2
51+
[InlineData("Unknown.ScopeValue", "Unknown")]
52+
[InlineData("", "Unknown")]
53+
[InlineData(null, "Unknown")]
54+
public void VersionDerivation_FromScope_ReturnsExpectedColumn(string? scope, string expectedVersion)
55+
{
56+
// This mirrors the VERSION derivation logic in CreateListConfiguredSubcommand:
57+
// version = IsV1Scope → "V1" : scope == V2ScopeValue → "V2" : "Unknown"
58+
var version = McpConstants.IsV1Scope(scope) ? "V1"
59+
: string.Equals(scope, McpConstants.V2ScopeValue, StringComparison.OrdinalIgnoreCase) ? "V2"
60+
: "Unknown";
61+
62+
Assert.Equal(expectedVersion, version);
63+
}
64+
65+
[Fact]
66+
public void V1ScopePattern_DoesNotMatchV2Scope()
67+
{
68+
Assert.False(McpConstants.IsV1Scope(McpConstants.V2ScopeValue),
69+
"V2 scope (Tools.ListInvoke.All) must not match the V1 pattern");
70+
}
71+
72+
[Fact]
73+
public void V2ScopeValue_DoesNotMatchV1Pattern()
74+
{
75+
Assert.False(McpConstants.IsV1Scope("Tools.ListInvoke.All"));
76+
}
77+
78+
[Fact]
79+
public void MetadataScope_IsNeither_V1_Nor_V2_ByDesign()
80+
{
81+
const string metadataScope = "McpServersMetadata.Read.All";
82+
83+
// "McpServersMetadata.Read.All" does NOT start with "McpServers." (note the dot):
84+
// char 11 is 'M' not '.' — so IsV1Scope returns false.
85+
Assert.False(McpConstants.IsV1Scope(metadataScope),
86+
"McpServersMetadata.Read.All does not match the McpServers.*.All V1 pattern");
87+
88+
Assert.False(string.Equals(metadataScope, McpConstants.V2ScopeValue, StringComparison.OrdinalIgnoreCase),
89+
"McpServersMetadata.Read.All is not the V2 scope");
90+
}
91+
}

‎src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PermissionsSubcommandTests.cs‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -464,6 +464,154 @@ public async Task ConfigureMcpPermissionsAsync_WithMissingManifest_ShouldHandleG
464464
because: "McpServersMetadata.Read.All is always included even when the ToolingManifest is missing, so the method proceeds to configure permissions and returns true (pending admin consent)");
465465
}
466466

467+
[Fact]
468+
public async Task ConfigureMcpPermissionsAsync_UnknownScope_ReturnsFalse()
469+
{
470+
// Arrange — manifest with a scope that is neither V1 (McpServers.*.All),
471+
// V2 (Tools.ListInvoke.All), nor the metadata scope
472+
var tempDir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
473+
Directory.CreateDirectory(tempDir);
474+
try
475+
{
476+
var manifestPath = Path.Combine(tempDir, "ToolingManifest.json");
477+
File.WriteAllText(manifestPath, """
478+
{
479+
"mcpServers": [
480+
{
481+
"mcpServerName": "mcp_WeirdServer",
482+
"scope": "Unknown.ScopeValue.NotRecognized",
483+
"audience": "99999999-0000-0000-0000-000000000000"
484+
}
485+
]
486+
}
487+
""");
488+
489+
var config = new Agent365Config
490+
{
491+
TenantId = "00000000-0000-0000-0000-000000000000",
492+
AgentBlueprintId = "blueprint-123",
493+
DeploymentProjectPath = tempDir
494+
};
495+
496+
// Act
497+
var result = await PermissionsSubcommand.ConfigureMcpPermissionsAsync(
498+
"config.json",
499+
_mockLogger,
500+
_mockConfigService,
501+
_mockExecutor,
502+
_mockGraphApiService,
503+
_mockBlueprintService,
504+
config,
505+
false);
506+
507+
// Assert — unknown scope blocks the operation
508+
result.Should().BeFalse("unknown scopes must be rejected to prevent misconfigured blueprints");
509+
}
510+
finally
511+
{
512+
Directory.Delete(tempDir, recursive: true);
513+
}
514+
}
515+
516+
[Fact]
517+
public async Task ConfigureMcpPermissionsAsync_V1AndMetadataScopes_AreKnownAndProceed()
518+
{
519+
// Arrange — manifest with only valid V1 scopes (should pass validation and attempt permissions)
520+
var tempDir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
521+
Directory.CreateDirectory(tempDir);
522+
try
523+
{
524+
var manifestPath = Path.Combine(tempDir, "ToolingManifest.json");
525+
File.WriteAllText(manifestPath, $$"""
526+
{
527+
"mcpServers": [
528+
{
529+
"mcpServerName": "mcp_MailTools",
530+
"scope": "McpServers.Mail.All",
531+
"audience": "{{Microsoft.Agents.A365.DevTools.Cli.Constants.McpConstants.Agent365ToolsProdAppId}}"
532+
}
533+
]
534+
}
535+
""");
536+
537+
var config = new Agent365Config
538+
{
539+
TenantId = "00000000-0000-0000-0000-000000000000",
540+
AgentBlueprintId = "blueprint-123",
541+
DeploymentProjectPath = tempDir
542+
};
543+
544+
// Act — proceeds past validation; may fail at Graph API (no real connection)
545+
// but must NOT return false due to unknown-scope validation
546+
Func<Task> act = () => PermissionsSubcommand.ConfigureMcpPermissionsAsync(
547+
"config.json",
548+
_mockLogger,
549+
_mockConfigService,
550+
_mockExecutor,
551+
_mockGraphApiService,
552+
_mockBlueprintService,
553+
config,
554+
false);
555+
556+
// Assert — passes scope validation (any exception is from Graph/blueprint, not scope guard)
557+
await act.Should().NotThrowAsync<InvalidOperationException>(
558+
"V1 scopes are known and must pass scope validation");
559+
}
560+
finally
561+
{
562+
Directory.Delete(tempDir, recursive: true);
563+
}
564+
}
565+
566+
[Fact]
567+
public async Task ConfigureMcpPermissionsAsync_V2Scope_IsKnownAndPassesValidation()
568+
{
569+
// Arrange — manifest with V2 scope (Tools.ListInvoke.All) should pass validation
570+
var tempDir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
571+
Directory.CreateDirectory(tempDir);
572+
try
573+
{
574+
var manifestPath = Path.Combine(tempDir, "ToolingManifest.json");
575+
File.WriteAllText(manifestPath, """
576+
{
577+
"mcpServers": [
578+
{
579+
"mcpServerName": "mcp_TeamsServer",
580+
"scope": "Tools.ListInvoke.All",
581+
"audience": "2cc60bb0-1024-48c8-95f0-1fce211a04d8"
582+
}
583+
]
584+
}
585+
""");
586+
587+
var config = new Agent365Config
588+
{
589+
TenantId = "00000000-0000-0000-0000-000000000000",
590+
AgentBlueprintId = "blueprint-123",
591+
DeploymentProjectPath = tempDir
592+
};
593+
594+
// Act
595+
Func<Task> act = () => PermissionsSubcommand.ConfigureMcpPermissionsAsync(
596+
"config.json",
597+
_mockLogger,
598+
_mockConfigService,
599+
_mockExecutor,
600+
_mockGraphApiService,
601+
_mockBlueprintService,
602+
config,
603+
false);
604+
605+
// Assert — V2 scope is known; validation passes (any failure is from Graph, not scope guard)
606+
await act.Should().NotThrowAsync<InvalidOperationException>(
607+
"V2 scope Tools.ListInvoke.All is known and must pass scope validation");
608+
}
609+
finally
610+
{
611+
Directory.Delete(tempDir, recursive: true);
612+
}
613+
}
614+
467615
#endregion
468616

469617
#region ConfigureBotPermissionsAsync Tests

0 commit comments

Comments
 (0)