fix: resolve principal ID in resource tenant for preflight role check#7174
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the Bicep preflight role-assignment permission check to correctly identify the current principal in the subscription’s resource tenant (fixing false warnings for B2B/guest users), and expands the warning text to include the RBAC Administrator role.
Changes:
- Resolve the principal object ID using the subscription’s resource-tenant (
sub.TenantId) before running the RBACassignedTo()role-assignment query. - Fall back to the existing
CurrentPrincipalIdbehavior when resource-tenant resolution fails. - Update the warning message to include Role Based Access Control Administrator alongside Owner/User Access Administrator.
You can also share your feedback on Copilot code review. Take the survey.
be7b90b to
03790ab
Compare
a0e0c84 to
ad636db
Compare
For B2B/guest users, CurrentPrincipalId returns the home-tenant object ID, which differs from the guest identity in the resource tenant. The assignedTo() role assignment filter cannot resolve cross-tenant identities, causing false positive permission warnings during preflight. This tactical fix resolves the principal ID via Graph /me against the subscription's resource tenant (sub.TenantId) instead of the user-access tenant (sub.UserAccessTenantId), so the role assignment query uses the correct guest identity. Falls back to the default provider on failure. Also adds 'Role Based Access Control Administrator' to the warning message as a valid role for the required permission. Fixes #7169 See #7173 for the broader CurrentPrincipalId fix across all callers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ad636db to
b1b6cd2
Compare
tg-msft
left a comment
There was a problem hiding this comment.
Thanks for digging deep into this!
wbreza
left a comment
There was a problem hiding this comment.
Code Review — PR #7174
✅ What Looks Good
- Per-role RBAC evaluation correctly fixes a semantic mismatch with Azure RBAC — Actions−NotActions per role, then union — addressing a real false-positive scenario (Contributor + UAA)
- Credential tenant forwarding is clean, backward-compatible (all existing sites pass
""), and well-documented with clear precedence (stored tenant → options override) - ABAC condition detection is a thoughtful proactive addition that catches a real deployment failure mode
- Excellent PR description — clearly documents four distinct problems, the solution for each, and testing coverage
- Good doc comments on new types and functions
Summary
| Priority | Count |
|---|---|
| Medium | 4 |
| Low | 1 |
| Total | 5 |
Overall Assessment: Approve — core logic changes are sound and well-motivated. Medium findings are testing gaps and a latent API design concern, not blocking bugs.
Review performed with GitHub Copilot CLI
weikanglim
left a comment
There was a problem hiding this comment.
Signing off on auth changes -- they look good.
|
/azp run azure-dev - cli |
|
Azure Pipelines successfully started running 1 pipeline(s). |
- Fix per-action conditional tracking: track unconditional grants per required action instead of a single global flag, so a later unconditional role clears the conditional flag even when a conditional role granted the same action first (JeffreyCA, wbreza) - Return PermissionCheckResult by value instead of pointer: idiomatic for a 2-bool struct, eliminates nil checks and GC pressure (wbreza) - Add /* tenantID */ comment on empty string constants passed to newAzdCredential call sites in manager.go (tg-msft) - Enhance credential test spy to verify expected tenant ID value, not just option count (wbreza) - Add 3 new tests for checkActionsFromRoles: conditional-then-unconditional ordering, all-conditional, and multi-action mixed conditionality Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
- Add Unreleased section to CHANGELOG covering merged PRs from 2026-03-19/20: - feat: AI-assisted error troubleshooting category selection (Azure#7216) - feat: CopilotService gRPC extension framework service (Azure#7172) - fix: lifecycle hooks silently not firing in azd up workflow (Azure#7218) - fix: azd update for Linux/macOS shell script and Homebrew (Azure#7213) - fix: azd update on Windows with backup/restore safety (Azure#7166) - fix: azd up --debug/--no-prompt positional arg error (Azure#7212) - fix: PromptSubscription not respecting AZD_DEMO_MODE (Azure#7193) - fix: preflight role check for B2B/guest users (Azure#7174) - Add AZD_DEMO_MODE note to PromptSubscription in extension-framework.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Azure#7174) * fix: resolve principal ID in resource tenant for preflight role check For B2B/guest users, CurrentPrincipalId returns the home-tenant object ID, which differs from the guest identity in the resource tenant. The assignedTo() role assignment filter cannot resolve cross-tenant identities, causing false positive permission warnings during preflight. This tactical fix resolves the principal ID via Graph /me against the subscription's resource tenant (sub.TenantId) instead of the user-access tenant (sub.UserAccessTenantId), so the role assignment query uses the correct guest identity. Falls back to the default provider on failure. Also adds 'Role Based Access Control Administrator' to the warning message as a valid role for the required permission. Fixes Azure#7169 See Azure#7173 for the broader CurrentPrincipalId fix across all callers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * address PR Azure#7174 review feedback - Fix per-action conditional tracking: track unconditional grants per required action instead of a single global flag, so a later unconditional role clears the conditional flag even when a conditional role granted the same action first (JeffreyCA, wbreza) - Return PermissionCheckResult by value instead of pointer: idiomatic for a 2-bool struct, eliminates nil checks and GC pressure (wbreza) - Add /* tenantID */ comment on empty string constants passed to newAzdCredential call sites in manager.go (tg-msft) - Enhance credential test spy to verify expected tenant ID value, not just option count (wbreza) - Add 3 new tests for checkActionsFromRoles: conditional-then-unconditional ordering, all-conditional, and multi-action mixed conditionality Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Azure#7174) * fix: resolve principal ID in resource tenant for preflight role check For B2B/guest users, CurrentPrincipalId returns the home-tenant object ID, which differs from the guest identity in the resource tenant. The assignedTo() role assignment filter cannot resolve cross-tenant identities, causing false positive permission warnings during preflight. This tactical fix resolves the principal ID via Graph /me against the subscription's resource tenant (sub.TenantId) instead of the user-access tenant (sub.UserAccessTenantId), so the role assignment query uses the correct guest identity. Falls back to the default provider on failure. Also adds 'Role Based Access Control Administrator' to the warning message as a valid role for the required permission. Fixes Azure#7169 See Azure#7173 for the broader CurrentPrincipalId fix across all callers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * address PR Azure#7174 review feedback - Fix per-action conditional tracking: track unconditional grants per required action instead of a single global flag, so a later unconditional role clears the conditional flag even when a conditional role granted the same action first (JeffreyCA, wbreza) - Return PermissionCheckResult by value instead of pointer: idiomatic for a 2-bool struct, eliminates nil checks and GC pressure (wbreza) - Add /* tenantID */ comment on empty string constants passed to newAzdCredential call sites in manager.go (tg-msft) - Enhance credential test spy to verify expected tenant ID value, not just option count (wbreza) - Add 3 new tests for checkActionsFromRoles: conditional-then-unconditional ordering, all-conditional, and multi-action mixed conditionality Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Azure#7174) * fix: resolve principal ID in resource tenant for preflight role check For B2B/guest users, CurrentPrincipalId returns the home-tenant object ID, which differs from the guest identity in the resource tenant. The assignedTo() role assignment filter cannot resolve cross-tenant identities, causing false positive permission warnings during preflight. This tactical fix resolves the principal ID via Graph /me against the subscription's resource tenant (sub.TenantId) instead of the user-access tenant (sub.UserAccessTenantId), so the role assignment query uses the correct guest identity. Falls back to the default provider on failure. Also adds 'Role Based Access Control Administrator' to the warning message as a valid role for the required permission. Fixes Azure#7169 See Azure#7173 for the broader CurrentPrincipalId fix across all callers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * address PR Azure#7174 review feedback - Fix per-action conditional tracking: track unconditional grants per required action instead of a single global flag, so a later unconditional role clears the conditional flag even when a conditional role granted the same action first (JeffreyCA, wbreza) - Return PermissionCheckResult by value instead of pointer: idiomatic for a 2-bool struct, eliminates nil checks and GC pressure (wbreza) - Add /* tenantID */ comment on empty string constants passed to newAzdCredential call sites in manager.go (tg-msft) - Enhance credential test spy to verify expected tenant ID value, not just option count (wbreza) - Add 3 new tests for checkActionsFromRoles: conditional-then-unconditional ordering, all-conditional, and multi-action mixed conditionality Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Azure#7174) * fix: resolve principal ID in resource tenant for preflight role check For B2B/guest users, CurrentPrincipalId returns the home-tenant object ID, which differs from the guest identity in the resource tenant. The assignedTo() role assignment filter cannot resolve cross-tenant identities, causing false positive permission warnings during preflight. This tactical fix resolves the principal ID via Graph /me against the subscription's resource tenant (sub.TenantId) instead of the user-access tenant (sub.UserAccessTenantId), so the role assignment query uses the correct guest identity. Falls back to the default provider on failure. Also adds 'Role Based Access Control Administrator' to the warning message as a valid role for the required permission. Fixes Azure#7169 See Azure#7173 for the broader CurrentPrincipalId fix across all callers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * address PR Azure#7174 review feedback - Fix per-action conditional tracking: track unconditional grants per required action instead of a single global flag, so a later unconditional role clears the conditional flag even when a conditional role granted the same action first (JeffreyCA, wbreza) - Return PermissionCheckResult by value instead of pointer: idiomatic for a 2-bool struct, eliminates nil checks and GC pressure (wbreza) - Add /* tenantID */ comment on empty string constants passed to newAzdCredential call sites in manager.go (tg-msft) - Enhance credential test spy to verify expected tenant ID value, not just option count (wbreza) - Add 3 new tests for checkActionsFromRoles: conditional-then-unconditional ordering, all-conditional, and multi-action mixed conditionality Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Azure#7174) * fix: resolve principal ID in resource tenant for preflight role check For B2B/guest users, CurrentPrincipalId returns the home-tenant object ID, which differs from the guest identity in the resource tenant. The assignedTo() role assignment filter cannot resolve cross-tenant identities, causing false positive permission warnings during preflight. This tactical fix resolves the principal ID via Graph /me against the subscription's resource tenant (sub.TenantId) instead of the user-access tenant (sub.UserAccessTenantId), so the role assignment query uses the correct guest identity. Falls back to the default provider on failure. Also adds 'Role Based Access Control Administrator' to the warning message as a valid role for the required permission. Fixes Azure#7169 See Azure#7173 for the broader CurrentPrincipalId fix across all callers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * address PR Azure#7174 review feedback - Fix per-action conditional tracking: track unconditional grants per required action instead of a single global flag, so a later unconditional role clears the conditional flag even when a conditional role granted the same action first (JeffreyCA, wbreza) - Return PermissionCheckResult by value instead of pointer: idiomatic for a 2-bool struct, eliminates nil checks and GC pressure (wbreza) - Add /* tenantID */ comment on empty string constants passed to newAzdCredential call sites in manager.go (tg-msft) - Enhance credential test spy to verify expected tenant ID value, not just option count (wbreza) - Add 3 new tests for checkActionsFromRoles: conditional-then-unconditional ordering, all-conditional, and multi-action mixed conditionality Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Azure#7174) * fix: resolve principal ID in resource tenant for preflight role check For B2B/guest users, CurrentPrincipalId returns the home-tenant object ID, which differs from the guest identity in the resource tenant. The assignedTo() role assignment filter cannot resolve cross-tenant identities, causing false positive permission warnings during preflight. This tactical fix resolves the principal ID via Graph /me against the subscription's resource tenant (sub.TenantId) instead of the user-access tenant (sub.UserAccessTenantId), so the role assignment query uses the correct guest identity. Falls back to the default provider on failure. Also adds 'Role Based Access Control Administrator' to the warning message as a valid role for the required permission. Fixes Azure#7169 See Azure#7173 for the broader CurrentPrincipalId fix across all callers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * address PR Azure#7174 review feedback - Fix per-action conditional tracking: track unconditional grants per required action instead of a single global flag, so a later unconditional role clears the conditional flag even when a conditional role granted the same action first (JeffreyCA, wbreza) - Return PermissionCheckResult by value instead of pointer: idiomatic for a 2-bool struct, eliminates nil checks and GC pressure (wbreza) - Add /* tenantID */ comment on empty string constants passed to newAzdCredential call sites in manager.go (tg-msft) - Enhance credential test spy to verify expected tenant ID value, not just option count (wbreza) - Add 3 new tests for checkActionsFromRoles: conditional-then-unconditional ordering, all-conditional, and multi-action mixed conditionality Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Problem
Three issues with the preflight role assignment permission check:
1. MSAL credential ignores target tenant for B2B/guest users
azdCredential.GetToken()never forwarded the tenant ID to MSAL'sAcquireTokenSilent. For B2B/guest users, this caused MSAL to return a cached home-tenant token even when a resource-tenant credential was requested. Graph/methen returned the home-tenant oid instead of the guest oid in the resource tenant.2. RBAC evaluation merges NotActions globally across roles
HasRequiredPermissionscollected Actions and NotActions from all role definitions into a single set, then evaluated them together. This is incorrect — Azure evaluates Actions−NotActions per role definition, then takes the union. The result: Contributor'sNotActions: [Microsoft.Authorization/*/Write]cancelled User Access Administrator'sActions: [Microsoft.Authorization/roleAssignments/*], producing a false positive warning.3. No detection of ABAC conditions on role assignments
Role assignments can have ABAC conditions (e.g., restricting which role definitions can be assigned). The check treated conditional grants the same as unconditional ones, missing cases where the deployment would fail due to condition restrictions.
4. Warning message missing 'RBAC Administrator' role (#7169)
The warning only mentioned 'User Access Administrator' and 'Owner', but the built-in 'Role Based Access Control Administrator' role also grants
Microsoft.Authorization/roleAssignments/write.Solution
Tactical fix scoped to the preflight check. The broader
CurrentPrincipalIdfix across all callers is tracked in #7173.Credential tenant forwarding (
auth/azd_credential.go,auth/manager.go)azdCredentialnow stores atenantIDand forwards it to MSAL viaWithTenantID()inAcquireTokenSilentoptions.TenantID(e.g., from CAE challenges) overrides the stored tenantnewAzdCredentialcall sites pass""(no behavior change); only the cross-tenant path now passes the target tenantPer-role RBAC evaluation + ABAC detection (
azapi/permissions.go)HasRequiredPermissionsnow returnsPermissionCheckResult{HasPermission, Conditional}instead ofboolResource-tenant principal resolution (
bicep/bicep_provider.go)resolveResourceTenantPrincipalId()that calls Graph/meagainst the subscription's resource tenant (sub.TenantId) to get the correct guest oidTesting
azd_credential_test.go(3 cases: stored tenant, override, empty)permissions_test.go(8 single-role cases + multi-role union test)Fixes #7169
See #7173