Skip to content

Commit 96cfb9d

Browse files
sellakumaransellakumaran
committed
Refine app role assignment logic and improve test clarity
- Leave S2SAppRoleGranted as null if blueprint SP is unresolved or service is unavailable, clarifying "not attempted" vs "not applicable" - Deduplicate and sanitize app role names before assignment to prevent redundant POSTs - Track assigned role IDs within a single operation to avoid duplicate assignments - Add test assertions and resource disposal for improved test reliability and clarity - Enhance consent URL test with explicit reasoning for scope encoding
1 parent f59b001 commit 96cfb9d

4 files changed

Lines changed: 135 additions & 114 deletions

File tree

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -175,8 +175,7 @@ internal static class BatchPermissionsOrchestrator
175175
var s2sScopes = permScopes.Concat(AuthenticationConstants.RequiredS2SGrantScopes).ToArray();
176176
if (!string.IsNullOrWhiteSpace(phase1Result?.BlueprintSpObjectId))
177177
await PerformS2SGrantsAsync(blueprintService, tenantId, phase1Result.BlueprintSpObjectId, specs, s2sScopes, logger, setupResults, ct);
178-
else if (setupResults is not null)
179-
setupResults.S2SAppRoleGranted = true;
178+
// else: blueprint SP was not resolved — leave S2SAppRoleGranted = null (not attempted)
180179

181180
logger.LogInformation("");
182181
if (grantsOk)
@@ -840,8 +839,7 @@ private record BlueprintPermissionsResult(
840839
var s2sScopes = permScopes.Concat(AuthenticationConstants.RequiredS2SGrantScopes).ToArray();
841840
if (blueprintService is not null && !string.IsNullOrWhiteSpace(phase1Result.BlueprintSpObjectId))
842841
await PerformS2SGrantsAsync(blueprintService, tenantId, phase1Result.BlueprintSpObjectId, specs, s2sScopes, logger, setupResults, ct);
843-
else
844-
setupResults.S2SAppRoleGranted = true; // M-003: no service or unresolved SP — mark not applicable
842+
// else: blueprint service unavailable or SP not resolved — leave S2SAppRoleGranted = null (not attempted)
845843

846844
return (allGrantsOk, phase1Result.BlueprintSpObjectId);
847845
}

‎src/Microsoft.Agents.A365.DevTools.Cli/Services/AgentBlueprintService.cs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -970,7 +970,11 @@ public virtual async Task<bool> GrantAppRoleAssignmentAsync(
970970
IEnumerable<string>? requiredScopes = null,
971971
CancellationToken ct = default)
972972
{
973-
var roleNames = appRoleNames?.ToList() ?? new List<string>();
973+
// De-dup upfront: duplicate names map to the same role ID and would cause a redundant POST.
974+
var roleNames = appRoleNames?
975+
.Where(n => !string.IsNullOrWhiteSpace(n))
976+
.Distinct(StringComparer.OrdinalIgnoreCase)
977+
.ToList() ?? new List<string>();
974978
if (roleNames.Count == 0) return true;
975979

976980
try
@@ -1067,6 +1071,7 @@ public virtual async Task<bool> GrantAppRoleAssignmentAsync(
10671071
if (resp.IsSuccess)
10681072
{
10691073
_logger.LogDebug("App role '{RoleName}' assigned to blueprint SP {BpSpId}.", roleName, blueprintSpObjectId);
1074+
existingRoleIds.Add(appRoleId); // prevent duplicate POST if same ID appears again
10701075
}
10711076
else
10721077
{

‎src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersConsentUrlTests.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,8 @@ public void BuildCombinedConsentUrl_AlwaysIncludesAllThreeFixedResources()
220220

221221
url.Should().Contain(Uri.EscapeDataString($"{ConfigConstants.MessagingBotApiIdentifierUri}/{ConfigConstants.MessagingBotApiAdminConsentScope}"),
222222
because: "scope URIs are Uri.EscapeDataString-encoded in the query string — required by AAD for adminconsent");
223-
url.Should().Contain(Uri.EscapeDataString($"{ConfigConstants.ObservabilityApiIdentifierUri}/{ConfigConstants.ObservabilityApiOtelWriteScope}"));
223+
url.Should().Contain(Uri.EscapeDataString($"{ConfigConstants.ObservabilityApiIdentifierUri}/{ConfigConstants.ObservabilityApiOtelWriteScope}"),
224+
because: "scope URIs are Uri.EscapeDataString-encoded in the query string — required by AAD for adminconsent");
224225
url.Should().Contain(Uri.EscapeDataString($"{PowerPlatformConstants.PowerPlatformApiIdentifierUri}/{PowerPlatformConstants.PermissionNames.ConnectivityConnectionsRead}"));
225226
}
226227

‎src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/AgentBlueprintServiceTests.cs‎

Lines changed: 125 additions & 108 deletions
Original file line numberDiff line numberDiff line change
@@ -476,165 +476,182 @@ public async Task GrantAppRoleAssignmentAsync_WhenResourceSpNotFound_ReturnsFals
476476
{
477477
// Arrange
478478
var (service, handler) = CreateServiceWithFakeHandler();
479-
// SP lookup returns empty value array
480-
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
479+
using (handler)
481480
{
482-
Content = new StringContent("{\"value\":[]}")
483-
});
481+
// SP lookup returns empty value array
482+
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
483+
{
484+
Content = new StringContent("{\"value\":[]}")
485+
});
484486

485-
// Act
486-
var result = await service.GrantAppRoleAssignmentAsync(
487-
"tenant-id", "blueprint-sp-id", "resource-app-id",
488-
new[] { "Agent365.Observability.OtelWrite" });
487+
// Act
488+
var result = await service.GrantAppRoleAssignmentAsync(
489+
"tenant-id", "blueprint-sp-id", "resource-app-id",
490+
new[] { "Agent365.Observability.OtelWrite" });
489491

490-
// Assert
491-
result.Should().BeFalse();
492+
// Assert
493+
result.Should().BeFalse();
494+
}
492495
}
493496

494497
[Fact]
495498
public async Task GrantAppRoleAssignmentAsync_WhenRoleNotFoundOnResourceSp_ReturnsFalse()
496499
{
497500
// Arrange
498501
var (service, handler) = CreateServiceWithFakeHandler();
499-
// SP lookup succeeds
500-
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
501-
{
502-
Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}")
503-
});
504-
// Resource SP has no matching app roles
505-
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
506-
{
507-
Content = new StringContent("{\"appRoles\":[]}")
508-
});
509-
// Existing assignments
510-
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
502+
using (handler)
511503
{
512-
Content = new StringContent("{\"value\":[]}")
513-
});
504+
// SP lookup succeeds
505+
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
506+
{
507+
Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}")
508+
});
509+
// Resource SP has no matching app roles
510+
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
511+
{
512+
Content = new StringContent("{\"appRoles\":[]}")
513+
});
514+
// Existing assignments
515+
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
516+
{
517+
Content = new StringContent("{\"value\":[]}")
518+
});
514519

515-
// Act
516-
var result = await service.GrantAppRoleAssignmentAsync(
517-
"tenant-id", "blueprint-sp-id", "resource-app-id",
518-
new[] { "Agent365.Observability.OtelWrite" });
520+
// Act
521+
var result = await service.GrantAppRoleAssignmentAsync(
522+
"tenant-id", "blueprint-sp-id", "resource-app-id",
523+
new[] { "Agent365.Observability.OtelWrite" });
519524

520-
// Assert
521-
result.Should().BeFalse();
525+
// Assert
526+
result.Should().BeFalse();
527+
}
522528
}
523529

524530
[Fact]
525531
public async Task GrantAppRoleAssignmentAsync_WhenRoleAlreadyAssigned_ReturnsTrueWithoutPost()
526532
{
527533
// Arrange
528534
var (service, handler) = CreateServiceWithFakeHandler();
529-
// SP lookup
530-
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
531-
{
532-
Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}")
533-
});
534-
// Resource SP app roles
535-
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
536-
{
537-
Content = new StringContent("{\"appRoles\":[{\"value\":\"Agent365.Observability.OtelWrite\",\"id\":\"role-id-1\"}]}")
538-
});
539-
// Existing assignments — role already assigned
540-
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
535+
using (handler)
541536
{
542-
Content = new StringContent("{\"value\":[{\"resourceId\":\"resource-sp-id\",\"appRoleId\":\"role-id-1\"}]}")
543-
});
544-
// No POST should be queued — if the handler is called a 4th time it returns 404
537+
// SP lookup
538+
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
539+
{
540+
Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}")
541+
});
542+
// Resource SP app roles
543+
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
544+
{
545+
Content = new StringContent("{\"appRoles\":[{\"value\":\"Agent365.Observability.OtelWrite\",\"id\":\"role-id-1\"}]}")
546+
});
547+
// Existing assignments — role already assigned
548+
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
549+
{
550+
Content = new StringContent("{\"value\":[{\"resourceId\":\"resource-sp-id\",\"appRoleId\":\"role-id-1\"}]}")
551+
});
552+
// No POST should be queued — if the handler is called a 4th time it returns 404
545553

546-
// Act
547-
var result = await service.GrantAppRoleAssignmentAsync(
548-
"tenant-id", "blueprint-sp-id", "resource-app-id",
549-
new[] { "Agent365.Observability.OtelWrite" });
554+
// Act
555+
var result = await service.GrantAppRoleAssignmentAsync(
556+
"tenant-id", "blueprint-sp-id", "resource-app-id",
557+
new[] { "Agent365.Observability.OtelWrite" });
550558

551-
// Assert
552-
result.Should().BeTrue();
559+
// Assert
560+
result.Should().BeTrue();
561+
}
553562
}
554563

555564
[Fact]
556565
public async Task GrantAppRoleAssignmentAsync_WhenPostSucceeds_ReturnsTrue()
557566
{
558567
// Arrange
559568
var (service, handler) = CreateServiceWithFakeHandler();
560-
// SP lookup
561-
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
562-
{
563-
Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}")
564-
});
565-
// Resource SP app roles
566-
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
567-
{
568-
Content = new StringContent("{\"appRoles\":[{\"value\":\"Agent365.Observability.OtelWrite\",\"id\":\"role-id-1\"}]}")
569-
});
570-
// Existing assignments — none
571-
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
572-
{
573-
Content = new StringContent("{\"value\":[]}")
574-
});
575-
// POST succeeds
576-
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.Created)
569+
using (handler)
577570
{
578-
Content = new StringContent("{\"id\":\"assignment-id\"}")
579-
});
571+
// SP lookup
572+
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
573+
{
574+
Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}")
575+
});
576+
// Resource SP app roles
577+
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
578+
{
579+
Content = new StringContent("{\"appRoles\":[{\"value\":\"Agent365.Observability.OtelWrite\",\"id\":\"role-id-1\"}]}")
580+
});
581+
// Existing assignments — none
582+
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
583+
{
584+
Content = new StringContent("{\"value\":[]}")
585+
});
586+
// POST succeeds
587+
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.Created)
588+
{
589+
Content = new StringContent("{\"id\":\"assignment-id\"}")
590+
});
580591

581-
// Act
582-
var result = await service.GrantAppRoleAssignmentAsync(
583-
"tenant-id", "blueprint-sp-id", "resource-app-id",
584-
new[] { "Agent365.Observability.OtelWrite" });
592+
// Act
593+
var result = await service.GrantAppRoleAssignmentAsync(
594+
"tenant-id", "blueprint-sp-id", "resource-app-id",
595+
new[] { "Agent365.Observability.OtelWrite" });
585596

586-
// Assert
587-
result.Should().BeTrue();
597+
// Assert
598+
result.Should().BeTrue();
599+
}
588600
}
589601

590602
[Fact]
591603
public async Task GrantAppRoleAssignmentAsync_WhenPostFails_ReturnsFalse()
592604
{
593605
// Arrange
594606
var (service, handler) = CreateServiceWithFakeHandler();
595-
// SP lookup
596-
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
597-
{
598-
Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}")
599-
});
600-
// Resource SP app roles
601-
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
602-
{
603-
Content = new StringContent("{\"appRoles\":[{\"value\":\"Agent365.Observability.OtelWrite\",\"id\":\"role-id-1\"}]}")
604-
});
605-
// Existing assignments — none
606-
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
607-
{
608-
Content = new StringContent("{\"value\":[]}")
609-
});
610-
// POST fails
611-
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.Forbidden)
607+
using (handler)
612608
{
613-
Content = new StringContent("{\"error\":{\"code\":\"Authorization_RequestDenied\"}}")
614-
});
609+
// SP lookup
610+
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
611+
{
612+
Content = new StringContent("{\"value\":[{\"id\":\"resource-sp-id\"}]}")
613+
});
614+
// Resource SP app roles
615+
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
616+
{
617+
Content = new StringContent("{\"appRoles\":[{\"value\":\"Agent365.Observability.OtelWrite\",\"id\":\"role-id-1\"}]}")
618+
});
619+
// Existing assignments — none
620+
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.OK)
621+
{
622+
Content = new StringContent("{\"value\":[]}")
623+
});
624+
// POST fails
625+
handler.QueueResponse(new HttpResponseMessage(HttpStatusCode.Forbidden)
626+
{
627+
Content = new StringContent("{\"error\":{\"code\":\"Authorization_RequestDenied\"}}")
628+
});
615629

616-
// Act
617-
var result = await service.GrantAppRoleAssignmentAsync(
618-
"tenant-id", "blueprint-sp-id", "resource-app-id",
619-
new[] { "Agent365.Observability.OtelWrite" });
630+
// Act
631+
var result = await service.GrantAppRoleAssignmentAsync(
632+
"tenant-id", "blueprint-sp-id", "resource-app-id",
633+
new[] { "Agent365.Observability.OtelWrite" });
620634

621-
// Assert
622-
result.Should().BeFalse();
635+
// Assert
636+
result.Should().BeFalse();
637+
}
623638
}
624639

625640
[Fact]
626641
public async Task GrantAppRoleAssignmentAsync_WithEmptyRoleNames_ReturnsTrue()
627642
{
628643
// Arrange
629644
var (service, handler) = CreateServiceWithFakeHandler();
645+
using (handler)
646+
{
647+
// Act — no HTTP calls should be made for an empty role list
648+
var result = await service.GrantAppRoleAssignmentAsync(
649+
"tenant-id", "blueprint-sp-id", "resource-app-id",
650+
Array.Empty<string>());
630651

631-
// Act — no HTTP calls should be made for an empty role list
632-
var result = await service.GrantAppRoleAssignmentAsync(
633-
"tenant-id", "blueprint-sp-id", "resource-app-id",
634-
Array.Empty<string>());
635-
636-
// Assert
637-
result.Should().BeTrue();
652+
// Assert
653+
result.Should().BeTrue();
654+
}
638655
}
639656
}
640657

0 commit comments

Comments
 (0)