Add manual tenant sign-in fallback for Conditional Access tenants#1511
Add manual tenant sign-in fallback for Conditional Access tenants#1511CoffeeKanzler wants to merge 10 commits into
Conversation
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Adds support for the existing azure.tenant setting when authenticating via VS Code’s built-in Microsoft authentication provider, enabling tenant-directed sign-in and tenant-scoped session requests (notably for auth-record export) consistent with az login --tenant ....
Changes:
- Introduces
src/utils/azureTenantSetting.tsto normalize/readazure.tenant, constructVSCODE_TENANT:<tenant>scopes, and provide tenant fallback selection logic. - Passes configured tenant through sign-in flows (login command + subscription provider context) and applies tenant-scoped auth session requests during auth-record export.
- Adds configuration contribution/localization for
azure.tenantand adds unit tests validating tenant normalization/scope behavior and logIn tenant forwarding.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/azureTenantSetting.test.ts | Adds tests for tenant normalization, tenant scope construction, tenant fallback logic, and tenant forwarding in logIn. |
| src/utils/azureTenantSetting.ts | Implements helpers for reading/normalizing azure.tenant, appending VSCODE_TENANT scope, and choosing effective tenant ID for auth. |
| src/services/VSCodeAzureSubscriptionProvider.ts | Adds tenant-aware signIn override and tenant fallback in subscription context generation. |
| src/exportAuthRecord.ts | Applies tenant-scoped session request for export and uses configured tenant in tenant resolution logic. |
| src/commands/accounts/logIn.ts | Passes configured azure.tenant into provider signIn. |
| package.nls.json | Adds localized description string for azure.tenant. |
| package.json | Contributes the azure.tenant setting to VS Code Settings UI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function getTenantId(session: unknown, context?: IActionContext): string | undefined { | ||
| return extractTenantIdFromIdToken(session, context) ?? getConfiguredAzureTenant(); | ||
| } |
bwateratmsft
left a comment
There was a problem hiding this comment.
The Azure Resources extension supports multi-tenant authentication on purpose. You can filter out tenants you aren't interested in looking at with the Accounts and Tenants view and the subscription filter. I don't see why this is necessary.
|
You can not login into some tenants that enforce that via conditional login. You NEED to specifiy the tenant manually to start the Auth workflow via the Microsoft Authenticator |
|
Does the "Sign in to Tenant (Directory)..." command work? |
|
No sadly it does not. The flow from my understanding is: Azure: Sign In Sign in to Tenant getAccounts() requires an account already registered with the VS Code Microsoft authentication provider. If the initial generic authentication failed, there is no account to use for tenant discovery. This creates a circular dependency: Tenant-specific authentication is required to satisfy Conditional Access and sign into tenant requires generic authentication and tenant discovery before it can request tenant-specific authentication |
|
That makes sense. Perhaps an input box in (side note, I wish Azure auth wasn't so nightmarishly overcomplicated...) |
|
Yeah that sounds like a better Idea than my current solution. I can look into that and rewrite this. |
| validateInput: value => value?.trim() ? undefined : localize('enterTenantIdValidation', 'Enter a tenant ID or domain.'), | ||
| }); | ||
|
|
||
| return tenantId ? { tenantId: tenantId.trim() } as TenantIdAndAccount : undefined; | ||
| } |
| return unauthenticatedTenants | ||
| .sort((a, b) => (a.displayName ?? '').localeCompare(b.displayName ?? '')) | ||
| .map(tenant => ({ | ||
| label: tenant.displayName ?? '', |
| signIn: (tenant?: TenantIdAndAccount) => Promise<boolean>; | ||
| }): AzureSubscriptionProvider { | ||
| return { | ||
| onRefreshSuggested: (() => ({ dispose: () => { /* no-op */ } })) as unknown as vscode.Event<RefreshSuggestedEvent>, |
| export async function setManualSignInTenant(tenant: string): Promise<void> { | ||
| if (!ext.context) { | ||
| testingManualSignInTenant = normalizeTenant(tenant); | ||
| return; | ||
| } |
|
I'll rework the description and work on the findings later this week probably. For now my tests are successful but I'll have to work in the additions of Copilot |
| /*--------------------------------------------------------------------------------------------- | ||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ |
| return unauthenticatedTenants | ||
| .sort((a, b) => (a.displayName ?? '').localeCompare(b.displayName ?? '')) | ||
| .map(tenant => ({ | ||
| label: tenant.displayName ?? tenant.defaultDomain ?? tenant.tenantId, |
| try { | ||
| const picks = await getPicks(subscriptionProvider, account); | ||
| if (picks.length) { | ||
| const pick = await context.ui.showQuickPick(picks, { | ||
| placeHolder: localize('selectTenantPlaceholder', 'Select a Tenant (Directory) to Sign In To'), | ||
| matchOnDescription: true, | ||
| }); | ||
| return { tenant: pick.data, isManual: false }; | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- Sort the tenant quick pick by the same display/domain/id fallback used for the label, so tenants without a display name no longer clump at the top. - Add a test for the manual-entry fallback when discovery returns no tenants (account exists but has no unauthenticated tenants).
|
@bwateratmsft One open design question I'd like your call on before this is final: When a user manually enters a tenant and sign-in succeeds, we persist it ( The thing I'm unsure about is its lifecycle: it's currently never cleared. The one case that can actually wedge things is a stale/revoked manual tenant — if the user later loses access to it, account-level enumeration keeps trying to get a token for it. Clearing it on a successful discovered sign-in doesn't help there (you can't reach discovery if enumeration is already blocked). Options I see: (a) leave it as-is, (b) add an explicit "Clear manual tenant" affordance, or (c) self-heal by dropping it if its silent token acquisition fails. Do you have a preference, or would you rather keep this PR to the bootstrap fix and handle lifecycle separately? |
bwateratmsft
left a comment
There was a problem hiding this comment.
Once an account is signed in (to a specific tenant), does it still hit the endpoint that CA is blocking? The MSAL broker might be "sticky" enough to remember the tenant for us once the account has been signed in, removing the need to persist the tenant ID ourselves. Not certain, but worth trying.
There was a problem hiding this comment.
We should make the changes in the auth package in microsoft/vscode-azuretools rather than duplicating the code there. Potentially we add a tenant or tenant ID arg to that function? @alexweininger thoughts?
There was a problem hiding this comment.
@CoffeeKanzler thanks for opening the PR!
getPicks here duplicates the auth package's existing picker, can we instead just delegate to the auth package's signInToTenant(provider) and only fall back to the manual tenant input box when it throws NotSignedInError? That avoids the duplication and any auth-package changes. Also, do we actually need the manualSignInTenant persistence + getSubscriptionContext override?
For example: main...alexweininger/sign-in-to-tenant-manual-entry
There was a problem hiding this comment.
Happy to refactor the picker to delegate to the auth package's signInToTenant(provider) with an input-box fallback on NotSignedInError, @alexweininger.
On whether we still need the manualSignInTenant persistence + getSubscriptionContext override: I tested exactly this. The manual sign-in itself succeeds, but resources never populate afterward.
The reason: subscription enumeration goes through getSubscriptionContext({ account, tenantId: undefined }) → listTenants(), and with tenantId undefined the silent token request hits the common/organizations
endpoint. So no subscriptions come back and the tree stays empty. The override re-injects the signed-in tenant into that account-level context, which scopes the silent token correctly and makes resources load.
It has to be persisted (not just in-memory) because the same enumeration re-runs on every reload.
@bwateratmsft — to your MSAL-stickiness question, that's what I was hoping too, but in practice the broker is only sticky for the interactive sign-in, not for the silent enumeration token. Without the persisted
tenant, a reload gives an empty tree.
So my proposal: Refactor the delegation (drop the duplicated picker) and keep the tenant persistence + context override (necessary for resources to actually load).
If you'd rather that re-scoping live in the auth package instead like e.g. a "default/sticky tenant" the provider applies to account-level requests then I'd need to further study the auth package after I am back.
Heads up on timing: I'm traveling in about an hour and won't be back until Friday at the earliest, so apologies in advance for the slow follow-up. I'll pick this up as soon as I'm back.
There was a problem hiding this comment.
Sorry to go back and forth on it, but now that we know we need to persist that value, then I think it's best kept as a setting like your original design.
- Drop the
manualSignInTenantglobalState, use an advanced setting instead. - Move the discovery fix to
getTenantsForAccount(we already override it, and it gets theaccount): when ARM/tenantsreturns empty/throws for a conditional-access user, injectAzureTenant { tenantId: <setting>, account }. The normal per-tenant flow then lists subscriptions via that tenant's session, so we can drop thegetSubscriptionContextoverride entirely. - Keep using the setting for the initial
signIn({ tenantId }).
summary: one setting + the existing public override, no globalState, no internal coupling. Clients consume our discovered subscriptions via the Resources API, so this covers the whole extension family too.
Address review feedback from microsoft#1511: - Delegate the tenant picker to the auth package's signInToTenant and fall back to a tenant ID/domain input box only on NotSignedInError. - Drop the manualSignInTenant globalState; persist the manually entered tenant in a new azure.tenant setting (machine scope) instead. - Move the conditional-access discovery fix into getTenantsForAccount: when tenant discovery throws or returns no tenants, inject a synthetic AzureTenant from the setting so the normal per-tenant subscription flow runs against the tenant-scoped session. This removes the getSubscriptionContext override entirely. Claude-Session: https://claude.ai/code/session_01Cqc27MN7o36f4avwMAyj3M
No description provided.