Skip to content
This repository was archived by the owner on Jul 28, 2025. It is now read-only.

Commit ab39df3

Browse files
authored
Fix ServicePrincipalId usage and Microsoft Graph authentication (#862)
1 parent 6b9c29b commit ab39df3

File tree

2 files changed

+49
-42
lines changed

2 files changed

+49
-42
lines changed

src/deploy-cromwell-on-azure/Deployer.cs

Lines changed: 48 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1729,38 +1729,49 @@ private async Task AssignMeAsRbacClusterAdminToManagedClusterAsync(ContainerServ
17291729
var adminGroupObjectIds = managedCluster.Data.AadProfile?.AdminGroupObjectIds ?? [];
17301730
adminGroupObjectIds = adminGroupObjectIds.Count != 0 ? adminGroupObjectIds : (string.IsNullOrWhiteSpace(configuration.AadGroupIds) ? [] : configuration.AadGroupIds.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries).Select(Guid.Parse).ToList());
17311731

1732+
Azure.ResourceManager.Authorization.Models.RoleManagementPrincipalType type;
1733+
IEnumerable<Guid> principalIds;
1734+
string admins;
1735+
Func<Exception, Exception> transformException = default;
1736+
17321737
if (adminGroupObjectIds.Count == 0)
17331738
{
1734-
var user = await GetUserObjectAsync();
1735-
1736-
if (user is not null)
1739+
admins = "deployer user";
1740+
transformException = new(e =>
17371741
{
1738-
await AssignRoleToResourceAsync(
1739-
[new Guid(user.Id)],
1740-
Azure.ResourceManager.Authorization.Models.RoleManagementPrincipalType.User,
1741-
managedCluster,
1742-
roleDefinitionId,
1743-
message.Replace(@"{Admins}", "deployer user"),
1744-
transformException: e =>
1745-
{
1746-
if (e is RequestFailedException ex && ex.Status == 403 && "AuthorizationFailed".Equals(ex.ErrorCode, StringComparison.OrdinalIgnoreCase))
1747-
{
1748-
return new System.ComponentModel.WarningException("Insufficient authorization for role assignment. Skipping role assignment to AKS cluster resource scope.", e);
1749-
}
1742+
if (e is RequestFailedException ex && ex.Status == 403 && "AuthorizationFailed".Equals(ex.ErrorCode, StringComparison.OrdinalIgnoreCase))
1743+
{
1744+
return new System.ComponentModel.WarningException("Insufficient authorization for role assignment. Skipping role assignment to AKS cluster resource scope.", e);
1745+
}
17501746

1751-
return e;
1752-
});
1747+
return e;
1748+
});
1749+
1750+
if (string.IsNullOrWhiteSpace(configuration.ServicePrincipalId))
1751+
{
1752+
principalIds = [new(configuration.ServicePrincipalId)];
1753+
type = Azure.ResourceManager.Authorization.Models.RoleManagementPrincipalType.ServicePrincipal;
1754+
}
1755+
else
1756+
{
1757+
var user = (await GetUserObjectAsync()) ?? throw new System.ComponentModel.WarningException($"The {admins} could not be determined. Skipping role assignment to AKS cluster resource scope.");
1758+
principalIds = [new(user.Id)];
1759+
type = Azure.ResourceManager.Authorization.Models.RoleManagementPrincipalType.User;
17531760
}
17541761
}
17551762
else
17561763
{
1757-
await AssignRoleToResourceAsync(
1758-
adminGroupObjectIds,
1759-
Azure.ResourceManager.Authorization.Models.RoleManagementPrincipalType.Group,
1760-
managedCluster,
1761-
roleDefinitionId,
1762-
message.Replace(@"{Admins}", "designated group"));
1764+
admins = "designated groups";
1765+
principalIds = adminGroupObjectIds;
1766+
type = Azure.ResourceManager.Authorization.Models.RoleManagementPrincipalType.Group;
17631767
}
1768+
1769+
await AssignRoleToResourceAsync(
1770+
principalIds,
1771+
type,
1772+
managedCluster,
1773+
roleDefinitionId,
1774+
message.Replace(@"{Admins}", admins));
17641775
}
17651776

17661777
private Task<StorageAccountResource> CreateStorageAccountAsync()
@@ -2216,6 +2227,7 @@ private Task<KeyVaultResource> CreateKeyVaultAsync(string vaultName, UserAssigne
22162227

22172228
private async Task<Microsoft.Graph.Models.User> GetUserObjectAsync()
22182229
{
2230+
// TODO: async blocking
22192231
if (_me is null)
22202232
{
22212233
Dictionary<Uri, string> nationalClouds = new(
@@ -2226,27 +2238,22 @@ private Task<KeyVaultResource> CreateKeyVaultAsync(string vaultName, UserAssigne
22262238
new(ArmEnvironment.AzureGovernment.Endpoint, GraphClientFactory.USGOV_Cloud), // TODO: when should we return GraphClientFactory.USGOV_DOD_Cloud?
22272239
]);
22282240

2229-
string baseUrl;
2241+
using var httpClient = GraphClientFactory.Create(nationalCloud: nationalClouds.TryGetValue(cloudEnvironment.ArmEnvironment.Endpoint, out var value) ? value : GraphClientFactory.Global_Cloud);
2242+
var scope = new UriBuilder(httpClient.BaseAddress) { Path = ".default" };
2243+
using var client = new GraphServiceClient(httpClient, tokenCredential, scopes: [scope.Uri.AbsoluteUri], baseUrl: httpClient.BaseAddress.AbsoluteUri);
2244+
2245+
try
22302246
{
2231-
using var client = GraphClientFactory.Create(nationalCloud: nationalClouds.TryGetValue(cloudEnvironment.ArmEnvironment.Endpoint, out var value) ? value : GraphClientFactory.Global_Cloud);
2232-
baseUrl = client.BaseAddress.AbsoluteUri;
2247+
_me = await client.Me.GetAsync(cancellationToken: cts.Token);
22332248
}
2249+
catch (AuthenticationFailedException afe)
22342250
{
2235-
using var client = new GraphServiceClient(tokenCredential, baseUrl: baseUrl);
2236-
2237-
try
2238-
{
2239-
_me = await client.Me.GetAsync(cancellationToken: cts.Token);
2240-
}
2241-
catch (Azure.Identity.AuthenticationFailedException)
2242-
{
2243-
_me = null;
2244-
}
2245-
catch (Microsoft.Graph.Models.ODataErrors.ODataError ex) when ("BadRequest".Equals(ex.Error?.Code, StringComparison.OrdinalIgnoreCase) && ex.Message.Contains("/me", StringComparison.OrdinalIgnoreCase))
2246-
{
2247-
// "/me request is only valid with delegated authentication flow."
2248-
_me = null;
2249-
}
2251+
Console.WriteLine($"AuthenticationFailedException: {afe.Message}");
2252+
}
2253+
catch (Microsoft.Graph.Models.ODataErrors.ODataError ex) when ("BadRequest".Equals(ex.Error?.Code, StringComparison.OrdinalIgnoreCase) && ex.Message.Contains("/me", StringComparison.OrdinalIgnoreCase))
2254+
{
2255+
Console.WriteLine($"ODataError: {ex.Message}");
2256+
// "/me request is only valid with delegated authentication flow."
22502257
}
22512258
}
22522259

0 commit comments

Comments
 (0)