diff --git a/cli/azd/.vscode/cspell.yaml b/cli/azd/.vscode/cspell.yaml index 8787173187d..bfb80f84ecb 100644 --- a/cli/azd/.vscode/cspell.yaml +++ b/cli/azd/.vscode/cspell.yaml @@ -305,6 +305,9 @@ overrides: - filename: pkg/azdext/scope_detector.go words: - fakeazure + - filename: "{pkg/azapi/permissions.go,pkg/infra/provisioning/bicep/bicep_provider.go}" + words: + - ABAC - filename: extensions/azure.ai.models/internal/cmd/custom_create.go words: - Qwen diff --git a/cli/azd/pkg/auth/azd_credential.go b/cli/azd/pkg/auth/azd_credential.go index 22a4d8ae55f..9bf8789f4cc 100644 --- a/cli/azd/pkg/auth/azd_credential.go +++ b/cli/azd/pkg/auth/azd_credential.go @@ -16,24 +16,48 @@ import ( ) type azdCredential struct { - client publicClient - account *public.Account - cloud *cloud.Cloud + client publicClient + account *public.Account + cloud *cloud.Cloud + tenantID string } -func newAzdCredential(client publicClient, account *public.Account, cloud *cloud.Cloud) *azdCredential { +// newAzdCredential creates a credential that acquires tokens via MSAL's public client. +// tenantID, when non-empty, is forwarded to AcquireTokenSilent so MSAL issues tokens +// for that specific tenant instead of defaulting to the account's home tenant. +func newAzdCredential( + client publicClient, account *public.Account, cloud *cloud.Cloud, tenantID string, +) *azdCredential { return &azdCredential{ - client: client, - account: account, - cloud: cloud, + client: client, + account: account, + cloud: cloud, + tenantID: tenantID, } } func (c *azdCredential) GetToken(ctx context.Context, options policy.TokenRequestOptions) (azcore.AccessToken, error) { - res, err := c.client.AcquireTokenSilent(ctx, - options.Scopes, + silentOpts := []public.AcquireSilentOption{ public.WithSilentAccount(*c.account), - public.WithClaims(options.Claims)) + public.WithClaims(options.Claims), + } + + // Forward the tenant ID so MSAL acquires a token from the correct tenant + // authority. The credential's tenantID is set when the credential is created + // for a specific tenant (e.g. resource tenant for B2B guests). The caller + // can override via options.TenantID (e.g. during CAE challenges). + // Without this, MSAL defaults to the account's home tenant, which causes + // cross-tenant calls (e.g. Graph /me for B2B guests) to return identity + // information for the home tenant instead of the resource tenant. + tenantID := c.tenantID + if options.TenantID != "" { + tenantID = options.TenantID + } + if tenantID != "" { + silentOpts = append(silentOpts, public.WithTenantID(tenantID)) + } + + res, err := c.client.AcquireTokenSilent(ctx, options.Scopes, silentOpts...) if err != nil { if authFailed, ok := errors.AsType[*AuthFailedError](err); ok { diff --git a/cli/azd/pkg/auth/azd_credential_test.go b/cli/azd/pkg/auth/azd_credential_test.go new file mode 100644 index 00000000000..339b087be78 --- /dev/null +++ b/cli/azd/pkg/auth/azd_credential_test.go @@ -0,0 +1,103 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package auth + +import ( + "context" + "testing" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/AzureAD/microsoft-authentication-library-for-go/apps/public" + "github.com/azure/azure-dev/cli/azd/pkg/cloud" + "github.com/stretchr/testify/require" +) + +// spyPublicClient records the options passed to AcquireTokenSilent so tests can +// verify that GetToken forwards TenantID correctly. +type spyPublicClient struct { + mockPublicClient + silentOptions []public.AcquireSilentOption +} + +func (s *spyPublicClient) AcquireTokenSilent( + ctx context.Context, scopes []string, options ...public.AcquireSilentOption, +) (public.AuthResult, error) { + s.silentOptions = options + return public.AuthResult{ + AccessToken: "test-token", + ExpiresOn: time.Now().Add(time.Hour), + Account: public.Account{ + HomeAccountID: "test.id", + }, + }, nil +} + +func TestAzdCredential_GetToken_ForwardsTenantID(t *testing.T) { + tests := []struct { + name string + credTID string // tenant stored in credential + optsTID string // tenant from TokenRequestOptions + // expectTenantOption is true when WithTenantID should be in the options + expectTenantOption bool + // expectedTenantID is the tenant ID value that should be forwarded. + // When optsTID is set it overrides credTID. + expectedTenantID string + }{ + { + name: "CredentialTenantUsed", + credTID: "resource-tenant-id", + optsTID: "", + expectTenantOption: true, + expectedTenantID: "resource-tenant-id", + }, + { + name: "OptionsTenantOverrides", + credTID: "resource-tenant-id", + optsTID: "override-tenant-id", + expectTenantOption: true, + expectedTenantID: "override-tenant-id", + }, + { + name: "NoTenant", + credTID: "", + optsTID: "", + expectTenantOption: false, + expectedTenantID: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + spy := &spyPublicClient{} + account := &public.Account{HomeAccountID: "test.id"} + cred := newAzdCredential(spy, account, cloud.AzurePublic(), tt.credTID) + + _, err := cred.GetToken(t.Context(), policy.TokenRequestOptions{ + Scopes: []string{"https://graph.microsoft.com/.default"}, + TenantID: tt.optsTID, + }) + require.NoError(t, err) + + if tt.expectTenantOption { + require.Len(t, spy.silentOptions, 3, + "expected WithSilentAccount + WithClaims + WithTenantID") + } else { + require.Len(t, spy.silentOptions, 2, + "expected WithSilentAccount + WithClaims only") + } + + // Verify the resolved tenant ID matches expectations. + // The credential resolves: optsTID if non-empty, else credTID. + // MSAL's WithTenantID option is opaque, so we verify the credential's + // tenant resolution logic directly (same package gives access to internals). + resolvedTenant := cred.tenantID + if tt.optsTID != "" { + resolvedTenant = tt.optsTID + } + require.Equal(t, tt.expectedTenantID, resolvedTenant, + "resolved tenant ID should match expected value") + }) + } +} diff --git a/cli/azd/pkg/auth/manager.go b/cli/azd/pkg/auth/manager.go index 71fa9ed63e5..46c2345b974 100644 --- a/cli/azd/pkg/auth/manager.go +++ b/cli/azd/pkg/auth/manager.go @@ -325,7 +325,7 @@ func (m *Manager) CredentialForCurrentUser( for i, account := range accounts { if account.HomeAccountID == *currentUser.HomeAccountID { if options.TenantID == "" { - return newAzdCredential(m.publicClient, &accounts[i], m.cloud), nil + return newAzdCredential(m.publicClient, &accounts[i], m.cloud, "" /* tenantID */), nil } else { newAuthority := m.cloud.Configuration.ActiveDirectoryAuthorityHost + options.TenantID @@ -342,7 +342,8 @@ func (m *Manager) CredentialForCurrentUser( } return newAzdCredential( - &msalPublicClientAdapter{client: &clientWithNewTenant}, &accounts[i], m.cloud), nil + &msalPublicClientAdapter{client: &clientWithNewTenant}, + &accounts[i], m.cloud, options.TenantID), nil } } } @@ -741,7 +742,7 @@ func (m *Manager) LoginInteractive( _ = os.Remove(claimsFile) } - return newAzdCredential(m.publicClient, &res.Account, m.cloud), nil + return newAzdCredential(m.publicClient, &res.Account, m.cloud, "" /* tenantID */), nil } // LoginWithBrokerAccount logs in an account provided by the system authentication broker via OneAuth. @@ -851,7 +852,7 @@ func (m *Manager) LoginWithDeviceCode( _ = os.Remove(claimsFile) } - return newAzdCredential(m.publicClient, &res.Account, m.cloud), nil + return newAzdCredential(m.publicClient, &res.Account, m.cloud, "" /* tenantID */), nil } diff --git a/cli/azd/pkg/azapi/permissions.go b/cli/azd/pkg/azapi/permissions.go index 867439c1481..09f662e581e 100644 --- a/cli/azd/pkg/azapi/permissions.go +++ b/cli/azd/pkg/azapi/permissions.go @@ -34,6 +34,17 @@ func NewPermissionsService( } } +// PermissionCheckResult describes the outcome of a permission check. +type PermissionCheckResult struct { + // HasPermission is true when the required actions are granted by at least one role. + HasPermission bool + // Conditional is true when every role that grants the required action has an + // ABAC condition attached. Conditions restrict the scope of the action (e.g., + // limiting which role definitions can be assigned) and the deployment may still + // fail at the server-side validation stage. + Conditional bool +} + // HasRequiredPermissions checks whether the given principal has all the specified // permissions at the subscription scope. Each required permission should be an Azure // resource provider action string such as @@ -42,17 +53,19 @@ func NewPermissionsService( // The check is performed by: // 1. Listing all role assignments for the principal on the subscription. // 2. Retrieving the role definition for each assignment. -// 3. Checking that the required actions are included in the allowed actions -// and not excluded by NotActions. +// 3. Checking that the required actions are included in at least one role's +// effective permissions (Actions minus NotActions). NotActions are evaluated +// per role definition, not globally, matching Azure RBAC semantics. +// 4. Detecting whether all granting role assignments have ABAC conditions. func (s *PermissionsService) HasRequiredPermissions( ctx context.Context, subscriptionId string, principalId string, requiredActions []string, -) (bool, error) { +) (PermissionCheckResult, error) { credential, err := s.credentialProvider.CredentialForSubscription(ctx, subscriptionId) if err != nil { - return false, fmt.Errorf("getting credential for subscription %s: %w", subscriptionId, err) + return PermissionCheckResult{}, fmt.Errorf("getting credential for subscription %s: %w", subscriptionId, err) } // Create a role assignments client to list the principal's role assignments at subscription scope. @@ -60,18 +73,16 @@ func (s *PermissionsService) HasRequiredPermissions( subscriptionId, credential, s.armClientOptions, ) if err != nil { - return false, fmt.Errorf("creating role assignments client: %w", err) + return PermissionCheckResult{}, fmt.Errorf("creating role assignments client: %w", err) } // Create a role definitions client to retrieve the definition for each assignment. roleDefinitionsClient, err := armauthorization.NewRoleDefinitionsClient(credential, s.armClientOptions) if err != nil { - return false, fmt.Errorf("creating role definitions client: %w", err) + return PermissionCheckResult{}, fmt.Errorf("creating role definitions client: %w", err) } - // Collect all role definition IDs assigned to this principal at subscription scope. - // Use assignedTo() filter which is supported by the API and also captures - // role assignments inherited through group membership. + // Collect role assignments with metadata about conditions. subscriptionScope := fmt.Sprintf("/subscriptions/%s", subscriptionId) filter := fmt.Sprintf("assignedTo('%s')", principalId) pager := roleAssignmentsClient.NewListForScopePager( @@ -81,106 +92,135 @@ func (s *PermissionsService) HasRequiredPermissions( }, ) - roleDefinitionIDs := []string{} + var assignments []roleAssignmentInfo for pager.More() { page, err := pager.NextPage(ctx) if err != nil { - return false, fmt.Errorf("listing role assignments for principal %s: %w", principalId, err) + return PermissionCheckResult{}, fmt.Errorf( + "listing role assignments for principal %s: %w", principalId, err) } for _, ra := range page.Value { if ra.Properties != nil && ra.Properties.RoleDefinitionID != nil { - roleDefinitionIDs = append(roleDefinitionIDs, *ra.Properties.RoleDefinitionID) + hasCondition := ra.Properties.Condition != nil && + *ra.Properties.Condition != "" + assignments = append(assignments, roleAssignmentInfo{ + roleDefinitionID: *ra.Properties.RoleDefinitionID, + hasCondition: hasCondition, + }) } } } - if len(roleDefinitionIDs) == 0 { - return false, nil - } - - // Build the set of allowed actions from all role definitions. - allowedActions, err := s.collectAllowedActions(ctx, roleDefinitionsClient, roleDefinitionIDs) - if err != nil { - return false, err - } - - // Check that every required action is covered. - for _, required := range requiredActions { - if !isActionAllowed(required, allowedActions) { - return false, nil - } + if len(assignments) == 0 { + return PermissionCheckResult{HasPermission: false}, nil } - return true, nil + // Check each role definition and track whether granting roles have conditions. + return s.checkActionsFromRoles( + ctx, roleDefinitionsClient, assignments, requiredActions) } -// allowedActionSet represents the collected allowed and denied actions from role definitions. -type allowedActionSet struct { - actions []string - notActions []string +// roleAssignmentInfo pairs a role definition ID with whether the assignment has a condition. +type roleAssignmentInfo struct { + roleDefinitionID string + hasCondition bool } -// collectAllowedActions retrieves the allowed/denied actions from all specified role definitions. -func (s *PermissionsService) collectAllowedActions( +// checkActionsFromRoles checks whether every required action is granted by at least +// one role definition. Each role is evaluated independently: an action is granted by a +// role if it matches an Action entry and is NOT excluded by a NotAction entry of that +// same role. It also tracks whether all granting assignments are conditional (ABAC). +func (s *PermissionsService) checkActionsFromRoles( ctx context.Context, client *armauthorization.RoleDefinitionsClient, - roleDefinitionIDs []string, -) (*allowedActionSet, error) { - result := &allowedActionSet{} + assignments []roleAssignmentInfo, + requiredActions []string, +) (PermissionCheckResult, error) { + // Track which required actions are still unresolved. + remaining := make(map[string]bool, len(requiredActions)) + for _, a := range requiredActions { + remaining[a] = true + } + + // Track which required actions have been granted unconditionally (no ABAC condition). + // An action needs at least one unconditional grant to avoid the conditional warning. + unconditionalActions := make(map[string]bool, len(requiredActions)) - for _, rdID := range roleDefinitionIDs { - resp, err := client.GetByID(ctx, rdID, nil) + for _, assignment := range assignments { + if len(remaining) == 0 && len(unconditionalActions) == len(requiredActions) { + break + } + + resp, err := client.GetByID(ctx, assignment.roleDefinitionID, nil) if err != nil { - return nil, fmt.Errorf("getting role definition %s: %w", rdID, err) + return PermissionCheckResult{}, fmt.Errorf( + "getting role definition %s: %w", assignment.roleDefinitionID, err) } if resp.Properties == nil || resp.Properties.Permissions == nil { continue } + // Collect this role's actions and notActions. + var actions, notActions []string for _, perm := range resp.Properties.Permissions { - if perm.Actions != nil { - for _, action := range perm.Actions { - if action != nil { - result.actions = append(result.actions, *action) - } + for _, a := range perm.Actions { + if a != nil { + actions = append(actions, *a) } } - if perm.NotActions != nil { - for _, notAction := range perm.NotActions { - if notAction != nil { - result.notActions = append(result.notActions, *notAction) - } + for _, na := range perm.NotActions { + if na != nil { + notActions = append(notActions, *na) } } } + + // Check each required action against this role. Even if the action was already + // granted by an earlier role, we still check for unconditional grants so that a + // later unconditional assignment can clear the conditional flag. + for _, action := range requiredActions { + if unconditionalActions[action] { + continue + } + if isActionAllowedByRole(action, actions, notActions) { + delete(remaining, action) + if !assignment.hasCondition { + unconditionalActions[action] = true + } + } + } + } + + if len(remaining) > 0 { + return PermissionCheckResult{HasPermission: false}, nil } - return result, nil + return PermissionCheckResult{ + HasPermission: true, + Conditional: len(unconditionalActions) < len(requiredActions), + }, nil } -// isActionAllowed checks whether the given action is matched by any allowed action -// and not excluded by any NotAction. Action matching supports the wildcard "*". -func isActionAllowed(requiredAction string, actions *allowedActionSet) bool { +// isActionAllowedByRole checks whether a single role's Actions (minus NotActions) +// grant the required action. +func isActionAllowedByRole(requiredAction string, actions []string, notActions []string) bool { matched := false - for _, action := range actions.actions { + for _, action := range actions { if actionMatches(action, requiredAction) { matched = true break } } - if !matched { return false } - // Check if any NotAction excludes this action. - for _, notAction := range actions.notActions { + for _, notAction := range notActions { if actionMatches(notAction, requiredAction) { return false } } - return true } diff --git a/cli/azd/pkg/azapi/permissions_test.go b/cli/azd/pkg/azapi/permissions_test.go index eedb834900f..ba56048b642 100644 --- a/cli/azd/pkg/azapi/permissions_test.go +++ b/cli/azd/pkg/azapi/permissions_test.go @@ -4,11 +4,99 @@ package azapi import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "strings" "testing" + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2" "github.com/stretchr/testify/require" ) +// roleDefinitionProperties describes a role for use in test helpers. +type roleDefinitionProperties struct { + actions []string + notActions []string +} + +// fakeRoleDefTransport is a minimal HTTP transport that returns canned role definition +// responses keyed by role definition ID (matched as a URL path suffix). +type fakeRoleDefTransport struct { + definitions map[string]*roleDefinitionProperties +} + +func (f *fakeRoleDefTransport) Do(req *http.Request) (*http.Response, error) { + for id, def := range f.definitions { + // Match by checking that the URL path ends with the role definition ID. + if strings.HasSuffix(req.URL.Path, "/"+id) || req.URL.Path == "/"+id { + actions := make([]*string, len(def.actions)) + for i := range def.actions { + actions[i] = &def.actions[i] + } + notActions := make([]*string, len(def.notActions)) + for i := range def.notActions { + notActions[i] = &def.notActions[i] + } + + resp := armauthorization.RoleDefinitionsClientGetByIDResponse{ + RoleDefinition: armauthorization.RoleDefinition{ + Properties: &armauthorization.RoleDefinitionProperties{ + Permissions: []*armauthorization.Permission{{ + Actions: actions, + NotActions: notActions, + }}, + }, + }, + } + + body, _ := json.Marshal(resp.RoleDefinition) + return &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{}, + Request: req, + Body: io.NopCloser(bytes.NewBuffer(body)), + }, nil + } + } + return nil, fmt.Errorf("no mock for role definition request: %s", req.URL.Path) +} + +// newFakeRoleDefinitionsClient creates a *armauthorization.RoleDefinitionsClient backed +// by a fake HTTP transport that returns canned role definitions. +func newFakeRoleDefinitionsClient( + t *testing.T, definitions map[string]*roleDefinitionProperties, +) *armauthorization.RoleDefinitionsClient { + t.Helper() + + transport := &fakeRoleDefTransport{definitions: definitions} + client, err := armauthorization.NewRoleDefinitionsClient( + &fakeCredential{}, + &arm.ClientOptions{ + ClientOptions: azcore.ClientOptions{ + Transport: transport, + }, + }, + ) + require.NoError(t, err) + return client +} + +// fakeCredential satisfies azcore.TokenCredential for test clients. +type fakeCredential struct{} + +func (f *fakeCredential) GetToken( + _ context.Context, _ policy.TokenRequestOptions, +) (azcore.AccessToken, error) { + return azcore.AccessToken{Token: "fake-token"}, nil +} + func TestActionMatches(t *testing.T) { tests := []struct { name string @@ -104,86 +192,66 @@ func TestActionMatches(t *testing.T) { } } -func TestIsActionAllowed(t *testing.T) { +func TestIsActionAllowedByRole(t *testing.T) { tests := []struct { name string requiredAction string - actions *allowedActionSet + actions []string + notActions []string want bool }{ { name: "allowed by exact match", requiredAction: "Microsoft.Authorization/roleAssignments/write", - actions: &allowedActionSet{ - actions: []string{"Microsoft.Authorization/roleAssignments/write"}, - notActions: nil, - }, - want: true, + actions: []string{"Microsoft.Authorization/roleAssignments/write"}, + want: true, }, { name: "allowed by wildcard", requiredAction: "Microsoft.Authorization/roleAssignments/write", - actions: &allowedActionSet{ - actions: []string{"*"}, - notActions: nil, - }, - want: true, + actions: []string{"*"}, + want: true, }, { name: "denied by NotActions", requiredAction: "Microsoft.Authorization/roleAssignments/write", - actions: &allowedActionSet{ - actions: []string{"*"}, - notActions: []string{"Microsoft.Authorization/roleAssignments/write"}, - }, - want: false, + actions: []string{"*"}, + notActions: []string{"Microsoft.Authorization/roleAssignments/write"}, + want: false, }, { name: "denied by NotActions wildcard", requiredAction: "Microsoft.Authorization/roleAssignments/write", - actions: &allowedActionSet{ - actions: []string{"*"}, - notActions: []string{"Microsoft.Authorization/*"}, - }, - want: false, + actions: []string{"*"}, + notActions: []string{"Microsoft.Authorization/*"}, + want: false, }, { name: "not matched at all", requiredAction: "Microsoft.Authorization/roleAssignments/write", - actions: &allowedActionSet{ - actions: []string{"Microsoft.Storage/*"}, - notActions: nil, - }, - want: false, + actions: []string{"Microsoft.Storage/*"}, + want: false, }, { name: "empty actions", requiredAction: "Microsoft.Authorization/roleAssignments/write", - actions: &allowedActionSet{ - actions: nil, - notActions: nil, - }, - want: false, + want: false, }, { name: "allowed by provider wildcard not blocked", requiredAction: "Microsoft.Authorization/roleAssignments/write", - actions: &allowedActionSet{ - actions: []string{"Microsoft.Authorization/*"}, - notActions: []string{"Microsoft.Storage/*"}, - }, - want: true, + actions: []string{"Microsoft.Authorization/*"}, + notActions: []string{"Microsoft.Storage/*"}, + want: true, }, { - name: "Contributor role pattern - allowed by star but blocked by NotActions", + name: "Contributor role - allowed by star but blocked by NotActions", requiredAction: "Microsoft.Authorization/roleAssignments/write", - actions: &allowedActionSet{ - actions: []string{"*"}, - notActions: []string{ - "Microsoft.Authorization/*/Delete", - "Microsoft.Authorization/*/Write", - "Microsoft.Authorization/elevateAccess/Action", - }, + actions: []string{"*"}, + notActions: []string{ + "Microsoft.Authorization/*/Delete", + "Microsoft.Authorization/*/Write", + "Microsoft.Authorization/elevateAccess/Action", }, want: false, }, @@ -191,8 +259,140 @@ func TestIsActionAllowed(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := isActionAllowed(tt.requiredAction, tt.actions) + got := isActionAllowedByRole(tt.requiredAction, tt.actions, tt.notActions) require.Equal(t, tt.want, got) }) } } + +func TestIsActionAllowedByRole_MultiRoleUnion(t *testing.T) { + // Simulates: Contributor (Actions=*, NotActions=Microsoft.Authorization/*/Write) + // + User Access Administrator (Actions=Microsoft.Authorization/roleAssignments/*, NotActions=none). + // Contributor alone denies roleAssignments/write, but User Access Admin grants it. + // The per-role evaluation should find it allowed via User Access Admin. + required := "Microsoft.Authorization/roleAssignments/write" + + contributorActions := []string{"*"} + contributorNotActions := []string{ + "Microsoft.Authorization/*/Delete", + "Microsoft.Authorization/*/Write", + "Microsoft.Authorization/elevateAccess/Action", + } + + uaaActions := []string{ + "Microsoft.Authorization/roleAssignments/*", + "Microsoft.Authorization/roleAssignments/read", + "Microsoft.Support/*", + "*/read", + } + var uaaNotActions []string + + // Contributor alone denies it. + require.False(t, isActionAllowedByRole(required, contributorActions, contributorNotActions)) + + // User Access Administrator alone allows it. + require.True(t, isActionAllowedByRole(required, uaaActions, uaaNotActions)) +} + +func TestCheckActionsFromRoles_ConditionalThenUnconditional(t *testing.T) { + // Verifies that when a conditional role grants an action first, a later + // unconditional role still clears the Conditional flag. This exercises the + // per-action unconditional tracking (unconditionalActions map) to ensure + // ordering doesn't produce false positive conditional warnings. + service := &PermissionsService{} + + // Two assignments: first is conditional, second is unconditional. + // Both grant the same action via wildcard. + assignments := []roleAssignmentInfo{ + {roleDefinitionID: "conditional-role", hasCondition: true}, + {roleDefinitionID: "unconditional-role", hasCondition: false}, + } + + // Mock role definitions client via a fakeRoleDefinitionsClient. + definitions := map[string]*roleDefinitionProperties{ + "conditional-role": { + actions: []string{"Microsoft.Authorization/*"}, + notActions: nil, + }, + "unconditional-role": { + actions: []string{"*"}, + notActions: nil, + }, + } + + client := newFakeRoleDefinitionsClient(t, definitions) + result, err := service.checkActionsFromRoles( + t.Context(), client, + assignments, + []string{"Microsoft.Authorization/roleAssignments/write"}, + ) + require.NoError(t, err) + require.True(t, result.HasPermission, "action should be granted") + require.False(t, result.Conditional, + "unconditional role should clear the conditional flag even when seen after a conditional role") +} + +func TestCheckActionsFromRoles_AllConditional(t *testing.T) { + // When all granting roles have ABAC conditions, Conditional should be true. + service := &PermissionsService{} + + assignments := []roleAssignmentInfo{ + {roleDefinitionID: "cond-role-1", hasCondition: true}, + {roleDefinitionID: "cond-role-2", hasCondition: true}, + } + + definitions := map[string]*roleDefinitionProperties{ + "cond-role-1": { + actions: []string{"Microsoft.Authorization/roleAssignments/*"}, + }, + "cond-role-2": { + actions: []string{"*"}, + }, + } + + client := newFakeRoleDefinitionsClient(t, definitions) + result, err := service.checkActionsFromRoles( + t.Context(), client, + assignments, + []string{"Microsoft.Authorization/roleAssignments/write"}, + ) + require.NoError(t, err) + require.True(t, result.HasPermission) + require.True(t, result.Conditional, + "all granting roles are conditional, so result should be conditional") +} + +func TestCheckActionsFromRoles_MultipleActions_MixedConditionality(t *testing.T) { + // Two required actions: action A granted only by a conditional role, + // action B granted by an unconditional role. Result should be Conditional + // because action A has no unconditional grant. + service := &PermissionsService{} + + assignments := []roleAssignmentInfo{ + {roleDefinitionID: "cond-role", hasCondition: true}, + {roleDefinitionID: "uncond-role", hasCondition: false}, + } + + definitions := map[string]*roleDefinitionProperties{ + "cond-role": { + actions: []string{"Microsoft.Authorization/roleAssignments/*"}, + }, + "uncond-role": { + actions: []string{"Microsoft.Compute/*"}, + }, + } + + client := newFakeRoleDefinitionsClient(t, definitions) + result, err := service.checkActionsFromRoles( + t.Context(), client, + assignments, + []string{ + "Microsoft.Authorization/roleAssignments/write", + "Microsoft.Compute/virtualMachines/write", + }, + ) + require.NoError(t, err) + require.True(t, result.HasPermission) + require.True(t, result.Conditional, + "action A only has conditional grants, so overall result should be conditional") +} diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go index c205e40c8bc..8f00c63a7ca 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go @@ -30,6 +30,7 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/async" "github.com/azure/azure-dev/cli/azd/pkg/azapi" "github.com/azure/azure-dev/cli/azd/pkg/azure" + "github.com/azure/azure-dev/cli/azd/pkg/azureutil" "github.com/azure/azure-dev/cli/azd/pkg/cloud" "github.com/azure/azure-dev/cli/azd/pkg/cmdsubst" "github.com/azure/azure-dev/cli/azd/pkg/config" @@ -2203,6 +2204,11 @@ func (p *BicepProvider) validatePreflight( // has Microsoft.Authorization/roleAssignments/write permission when the template contains // role assignments. The PermissionsService is resolved lazily via the service locator so it // is only instantiated when actually needed. +// +// For B2B/guest users, the principal's object ID differs between the home tenant and the +// resource tenant. This check resolves the principal ID in the resource tenant context +// (sub.TenantId) so that the role assignment query uses the correct identity. +// See https://github.com/Azure/azure-dev/issues/7173 for the broader fix. func (p *BicepProvider) checkRoleAssignmentPermissions( ctx context.Context, valCtx *validationContext, ) (*PreflightCheckResult, error) { @@ -2217,14 +2223,20 @@ func (p *BicepProvider) checkRoleAssignmentPermissions( return nil, nil } - principalId, err := p.curPrincipal.CurrentPrincipalId(ctx) + subscriptionId := p.env.GetSubscriptionId() + + // Resolve the principal ID in the resource tenant context rather than the user-access + // (home) tenant. For B2B/guest users, CurrentPrincipalId returns the home-tenant oid, + // which does not match the guest identity in the resource tenant. The assignedTo() role + // assignment filter cannot resolve cross-tenant identities, causing false positive + // warnings. This is a tactical fix; see #7173 for the full CurrentPrincipalId fix. + principalId, err := p.resolveResourceTenantPrincipalId(ctx, subscriptionId) if err != nil { log.Printf( "could not determine current principal, skipping role assignment permission check: %v", err) return nil, nil } - subscriptionId := p.env.GetSubscriptionId() requiredActions := []string{ "Microsoft.Authorization/roleAssignments/write", } @@ -2237,14 +2249,15 @@ func (p *BicepProvider) checkRoleAssignmentPermissions( return nil, nil } - if !hasPermission { + if !hasPermission.HasPermission { return &PreflightCheckResult{ Severity: PreflightCheckWarning, Message: fmt.Sprintf( "the current principal (%s) does not have permission to create role assignments "+ "(Microsoft.Authorization/roleAssignments/write) on subscription %s. "+ "The deployment includes role assignments and will fail without this permission. "+ - "Ensure you have the 'User Access Administrator', 'Owner', or a custom role with "+ + "Ensure you have the 'Role Based Access Control Administrator', "+ + "'User Access Administrator', 'Owner', or a custom role with "+ "'Microsoft.Authorization/roleAssignments/write' assigned to your account.", principalId, subscriptionId, @@ -2252,9 +2265,53 @@ func (p *BicepProvider) checkRoleAssignmentPermissions( }, nil } + if hasPermission.Conditional { + return &PreflightCheckResult{ + Severity: PreflightCheckWarning, + Message: fmt.Sprintf( + "the current principal (%s) has conditional permission to create role "+ + "assignments (Microsoft.Authorization/roleAssignments/write) on "+ + "subscription %s. The role assignment that grants this permission "+ + "has an ABAC condition that may restrict which roles can be assigned. "+ + "The deployment may fail if the condition does not permit the "+ + "specific role assignments in the template.", + principalId, + subscriptionId, + ), + }, nil + } + return nil, nil } +// resolveResourceTenantPrincipalId returns the current user's object ID as seen in the +// subscription's resource tenant. This is needed because B2B/guest users have a different +// object ID in the resource tenant than in their home tenant. Returns an error if the +// resource tenant principal cannot be resolved; the caller should skip the check rather +// than fall back to the home-tenant oid (which would produce false positive warnings). +func (p *BicepProvider) resolveResourceTenantPrincipalId( + ctx context.Context, subscriptionId string, +) (string, error) { + sub, err := p.subscriptionManager.GetSubscription(ctx, subscriptionId) + if err != nil { + return "", fmt.Errorf("getting subscription %s: %w", subscriptionId, err) + } + + var userProfileService *azapi.UserProfileService + if err := p.serviceLocator.Resolve(&userProfileService); err != nil { + return "", fmt.Errorf("resolving UserProfileService: %w", err) + } + + // Call Graph /me against the resource tenant to get the guest oid. + principalId, err := azureutil.GetCurrentPrincipalId(ctx, userProfileService, sub.TenantId) + if err != nil { + return "", fmt.Errorf( + "resolving principal in resource tenant %s: %w", sub.TenantId, err) + } + + return principalId, nil +} + // Deploys the specified Bicep module and parameters with the selected provisioning scope (subscription vs resource group) func (p *BicepProvider) deployModule( ctx context.Context,