Skip to content

Commit 637004b

Browse files
bhaarathmsclaude
andcommitted
fix: address PR review - array copy safety, retry handling, test coverage
- GetConsentRedirectUris returns defensive copies to prevent mutation of shared static arrays - SetWebConsentRedirectUrisAsync now uses RetryHelper for transient Graph failures, consistent with publicClient redirect URI handling - Add because: clauses to web consent URI test assertions documenting the admin consent portal contract - Add test for web redirect URI update throw path Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 979a2b1 commit 637004b

2 files changed

Lines changed: 35 additions & 7 deletions

File tree

src/Microsoft.Agents.A365.DevTools.Cli/Services/EntraAppProvisioner.cs

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,8 @@ internal static string[] GetConsentRedirectUris(string environment)
3737
{
3838
return environment?.ToLowerInvariant() switch
3939
{
40-
"test" or "preprod" or "ppe" => TestPreProdConsentRedirectUris,
41-
_ => ProdConsentRedirectUris,
40+
"test" or "preprod" or "ppe" => [.. TestPreProdConsentRedirectUris],
41+
_ => [.. ProdConsentRedirectUris],
4242
};
4343
}
4444

@@ -122,7 +122,10 @@ private async Task SetWebConsentRedirectUrisAsync(
122122
var resolvedEnvironment = environment ?? Environment.GetEnvironmentVariable("A365_ENVIRONMENT") ?? "prod";
123123
var consentUris = GetConsentRedirectUris(resolvedEnvironment);
124124

125-
var success = await _graphApiService.UpdateAppRedirectUrisAsync(tenantId, objectId, consentUris, ct);
125+
var success = await _retryHelper.ExecuteWithRetryAsync(
126+
async retryCt => await _graphApiService.UpdateAppRedirectUrisAsync(tenantId, objectId, consentUris, retryCt),
127+
result => !result,
128+
cancellationToken: ct);
126129
if (success)
127130
{
128131
_logger.LogDebug(
@@ -131,7 +134,7 @@ private async Task SetWebConsentRedirectUrisAsync(
131134
}
132135
else
133136
{
134-
var msg = $"Failed to set web redirect URIs on {roleDisplay} app '{appName}'.";
137+
var msg = $"Failed to set web redirect URIs on {roleDisplay} app '{appName}' after retries.";
135138
_logger.LogWarning(msg);
136139
warnings?.Add(msg);
137140
}

src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/EntraAppProvisionerTests.cs

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,9 @@ public async Task CreatePublicClientsAppAsync_ProdEnvironment_SetsPublicClientAn
186186
{
187187
"https://admin.cloud.microsoft/?ref=tools/consent",
188188
},
189-
opt => opt.WithStrictOrdering());
189+
opt => opt.WithStrictOrdering(),
190+
because: "Prod consent redirect URIs are required by the admin.cloud.microsoft " +
191+
"consent portal. Changing these URIs breaks the admin consent flow.");
190192
}
191193

192194
[Theory]
@@ -237,7 +239,10 @@ public async Task CreatePublicClientsAppAsync_TestPreProdEnvironment_SetsTestPre
237239
"https://sdf.admin.cloud.microsoft/?ref=tools/consent",
238240
"https://ignite.admin.cloud.microsoft/?ref=tools/consent",
239241
},
240-
opt => opt.WithStrictOrdering());
242+
opt => opt.WithStrictOrdering(),
243+
because: "Test/PreProd consent redirect URIs target the sdf and ignite admin " +
244+
"consent portals. Changing these URIs breaks the admin consent flow " +
245+
"in non-production environments.");
241246
}
242247

243248
[Fact]
@@ -295,7 +300,7 @@ public async Task CreatePublicClientsAppAsync_WhenWebRedirectUriUpdateReturnsFal
295300
var result = await _provisioner.CreatePublicClientsAppAsync(ServerName, TenantId, serviceTreeId: null, warnings);
296301

297302
result.ClientId.Should().Be(AppClientId);
298-
warnings.Should().ContainSingle().Which.Should().Be($"Failed to set web redirect URIs on Public Clients app '{ServerName}-PublicClients'.");
303+
warnings.Should().ContainSingle().Which.Should().Be($"Failed to set web redirect URIs on Public Clients app '{ServerName}-PublicClients' after retries.");
299304
}
300305

301306
[Fact]
@@ -315,4 +320,24 @@ public async Task CreatePublicClientsAppAsync_WhenRedirectUriUpdateThrows_Return
315320
result.AppName.Should().Be($"{ServerName}-PublicClients");
316321
warnings.Should().ContainSingle().Which.Should().Be("Failed to set redirect URIs on Public Clients app: Graph blew up");
317322
}
323+
324+
[Fact]
325+
public async Task CreatePublicClientsAppAsync_WhenWebRedirectUriUpdateThrows_ReturnsIdsAndAppendsExceptionWarning()
326+
{
327+
_graph.CreateEntraAppAsync(TenantId, Arg.Any<string>(), serviceTreeId: Arg.Any<string?>(), Arg.Any<CancellationToken>())
328+
.Returns(Task.FromResult<(string ObjectId, string ClientId)?>((AppObjectId, AppClientId)));
329+
_graph.UpdateAppPublicClientRedirectUrisAsync(
330+
TenantId, AppObjectId, Arg.Any<string[]>(), Arg.Any<CancellationToken>())
331+
.Returns(Task.FromResult(true));
332+
_graph.UpdateAppRedirectUrisAsync(
333+
TenantId, AppObjectId, Arg.Any<string[]>(), Arg.Any<CancellationToken>())
334+
.Returns<Task<bool>>(_ => throw new InvalidOperationException("Graph consent update failed"));
335+
336+
var warnings = new List<string>();
337+
var result = await _provisioner.CreatePublicClientsAppAsync(ServerName, TenantId, serviceTreeId: null, warnings);
338+
339+
result.ClientId.Should().Be(AppClientId);
340+
result.ObjectId.Should().Be(AppObjectId);
341+
warnings.Should().ContainSingle().Which.Should().Be("Failed to set redirect URIs on Public Clients app: Graph consent update failed");
342+
}
318343
}

0 commit comments

Comments
 (0)