-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
auth: Add AzureSessionProvider and related types #1722
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
import { type TenantIdDescription } from "@azure/arm-resources-subscriptions"; | ||
import { type AuthenticationSession, type Event } from "vscode"; | ||
|
||
export type SignInStatus = "Initializing" | "SigningIn" | "SignedIn" | "SignedOut"; | ||
|
||
export type AzureAuthenticationSession = AuthenticationSession & { | ||
tenantId: string; | ||
}; | ||
|
||
export type DefinedTenant = TenantIdDescription & Required<Pick<TenantIdDescription, "tenantId" | "displayName">>; | ||
|
||
export enum GetSessionBehavior { | ||
Silent, | ||
PromptIfRequired, | ||
} | ||
|
||
export type AzureSessionProvider = { | ||
signIn(): Promise<void>; | ||
signInStatus: SignInStatus; | ||
tenants: DefinedTenant[]; | ||
isSignedInToTenant(tenantId: string): boolean; | ||
signInStatusChangeEvent: Event<SignInStatus>; | ||
getAuthSession(tenantId: string, behavior: GetSessionBehavior, scopes?: string[]): Promise<AzureAuthenticationSession | undefined>; | ||
dispose(): void; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,241 @@ | ||
import { type TenantIdDescription } from "@azure/arm-resources-subscriptions"; | ||
import { AuthenticationGetSessionOptions, AuthenticationSession, Event, EventEmitter, Disposable as VSCodeDisposable, authentication } from "vscode"; | ||
import { AzureAuthenticationSession, AzureSessionProvider, DefinedTenant, GetSessionBehavior, SignInStatus } from "./AzureSessionProvider"; | ||
import { NotSignedInError } from "./NotSignedInError"; | ||
import { getConfiguredAuthProviderId, getConfiguredAzureEnv } from "./utils/configuredAzureEnv"; | ||
import { getSubscriptionClient } from "./utils/resourceManagement"; | ||
|
||
enum AuthScenario { | ||
Initialization, | ||
SignIn, | ||
GetSessionSilent, | ||
GetSessionPrompt, | ||
} | ||
Comment on lines
+8
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although these are only used within this class, I'd love to remove this extra layer of abstraction. I want to avoid adding layers of logic/abstraction on top of the VS Code API as much as we can avoid. I think for now it's fine, but I'm exploring refactoring this to avoid using these. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Introducing this was really a ripple effect of the "silent first" approach you commented on below. I agree, ideally we'd pass a get-session options object around, but when the receiving code converts that into two calls with different options values, the |
||
|
||
type TenantSignInStatus = { | ||
tenant: DefinedTenant; | ||
isSignedIn: boolean; | ||
}; | ||
|
||
export class VSCodeAzureSessionProvider extends VSCodeDisposable implements AzureSessionProvider { | ||
private readonly initializePromise: Promise<void>; | ||
private handleSessionChanges: boolean = true; | ||
private tenantSignInStatuses: TenantSignInStatus[] = []; | ||
|
||
public readonly onSignInStatusChangeEmitter = new EventEmitter<SignInStatus>(); | ||
public signInStatusValue: SignInStatus = "Initializing"; | ||
|
||
public constructor() { | ||
const disposable = authentication.onDidChangeSessions(async (e) => { | ||
// Ignore events for non-microsoft providers | ||
if (e.provider.id !== getConfiguredAuthProviderId()) { | ||
return; | ||
} | ||
|
||
// Ignore events that we triggered. | ||
if (!this.handleSessionChanges) { | ||
return; | ||
} | ||
|
||
// Silently check authentication status and tenants | ||
await this.signInAndUpdateTenants(AuthScenario.Initialization); | ||
}); | ||
|
||
super(() => { | ||
this.onSignInStatusChangeEmitter.dispose(); | ||
disposable.dispose(); | ||
}); | ||
|
||
this.initializePromise = this.initialize(); | ||
} | ||
|
||
public get signInStatus(): SignInStatus { | ||
return this.signInStatusValue; | ||
} | ||
|
||
public get signInStatusChangeEvent(): Event<SignInStatus> { | ||
return this.onSignInStatusChangeEmitter.event; | ||
} | ||
|
||
public get tenants(): DefinedTenant[] { | ||
return this.tenantSignInStatuses.map(s => s.tenant); | ||
} | ||
|
||
public isSignedInToTenant(tenantId: string): boolean { | ||
return this.tenantSignInStatuses.some(s => s.tenant.tenantId === tenantId && s.isSignedIn); | ||
} | ||
|
||
private async initialize(): Promise<void> { | ||
await this.signInAndUpdateTenants(AuthScenario.Initialization); | ||
} | ||
|
||
/** | ||
* Sign in to Azure interactively, i.e. prompt the user to sign in even if they have an active session. | ||
* This allows the user to choose a different account or tenant. | ||
*/ | ||
public async signIn(): Promise<void> { | ||
await this.initializePromise; | ||
|
||
const newSignInStatus = "SigningIn"; | ||
if (newSignInStatus !== this.signInStatusValue) { | ||
this.signInStatusValue = newSignInStatus; | ||
this.onSignInStatusChangeEmitter.fire(this.signInStatusValue); | ||
} | ||
|
||
await this.signInAndUpdateTenants(AuthScenario.SignIn); | ||
} | ||
|
||
private async signInAndUpdateTenants(authScenario: AuthScenario): Promise<void> { | ||
// Initially, try to get a session using the 'organizations' tenant/authority: | ||
// https://learn.microsoft.com/en-us/entra/identity-platform/msal-client-application-configuration#authority | ||
// This allows the user to sign in to the Microsoft provider and list tenants, | ||
// but the resulting session will not allow tenant-level operations. For that, | ||
// we need to get a session for a specific tenant. | ||
const scopes = [getDefaultScope(getConfiguredAzureEnv().resourceManagerEndpointUrl)]; | ||
const getSessionResult = await this.getArmSession("organizations", authScenario, scopes); | ||
if (getSessionResult === undefined) { | ||
if (this.tenantSignInStatuses.length > 0 || this.signInStatusValue !== "SignedOut") { | ||
this.tenantSignInStatuses = []; | ||
this.signInStatusValue = "SignedOut"; | ||
this.onSignInStatusChangeEmitter.fire(this.signInStatusValue); | ||
} | ||
|
||
return; | ||
} | ||
|
||
// Get the tenants | ||
const allTenants = await getTenants(getSessionResult); | ||
|
||
const signInStatusesPromises = allTenants.map<Promise<TenantSignInStatus>>(async (t) => { | ||
const session = await this.getArmSession(t.tenantId, AuthScenario.Initialization, scopes); | ||
return { | ||
tenant: t, | ||
isSignedIn: session !== undefined, | ||
}; | ||
}); | ||
|
||
const newTenantSignInStatuses = await Promise.all(signInStatusesPromises); | ||
const tenantsChanged = !areStringCollectionsEqual( | ||
this.tenantSignInStatuses.map(s => s.tenant.tenantId), | ||
newTenantSignInStatuses.map(s => s.tenant.tenantId)); | ||
|
||
// Get the overall sign-in status. If the user has access to any tenants they are signed in. | ||
const newSignInStatus = newTenantSignInStatuses.length > 0 ? "SignedIn" : "SignedOut"; | ||
const signInStatusChanged = newSignInStatus !== this.signInStatusValue; | ||
|
||
// Update the state and fire event if anything has changed. | ||
this.tenantSignInStatuses = newTenantSignInStatuses; | ||
this.signInStatusValue = newSignInStatus; | ||
if (signInStatusChanged || tenantsChanged) { | ||
this.onSignInStatusChangeEmitter.fire(this.signInStatusValue); | ||
} | ||
} | ||
|
||
/** | ||
* Get the current Azure session, silently if possible. | ||
* @returns The current Azure session, if available. If the user is not signed in, or there are no tenants, | ||
* an error is thrown. | ||
*/ | ||
public async getAuthSession(tenantId: string, behavior: GetSessionBehavior, scopes?: string[]): Promise<AzureAuthenticationSession | undefined> { | ||
await this.initializePromise; | ||
if (this.signInStatusValue !== "SignedIn") { | ||
throw new NotSignedInError(); | ||
} | ||
|
||
const tenantSignInStatus = this.tenantSignInStatuses.find(s => s.tenant.tenantId === tenantId); | ||
if (!tenantSignInStatus) { | ||
throw new Error(`User does not have access to tenant ${tenantId}`); | ||
} | ||
|
||
// Get a session for a specific tenant. | ||
scopes = scopes || [getDefaultScope(getConfiguredAzureEnv().resourceManagerEndpointUrl)]; | ||
const behaviourScenarios: Record<GetSessionBehavior, AuthScenario> = { | ||
[GetSessionBehavior.Silent]: AuthScenario.GetSessionSilent, | ||
[GetSessionBehavior.PromptIfRequired]: AuthScenario.GetSessionPrompt, | ||
}; | ||
|
||
const session = await this.getArmSession(tenantId, behaviourScenarios[behavior], scopes); | ||
tenantSignInStatus.isSignedIn = session !== undefined; | ||
|
||
return session; | ||
} | ||
|
||
private async getArmSession( | ||
tenantId: string, | ||
authScenario: AuthScenario, | ||
scopes: string[], | ||
): Promise<AzureAuthenticationSession | undefined> { | ||
this.handleSessionChanges = false; | ||
try { | ||
scopes = addTenantIdScope(scopes, tenantId); | ||
|
||
let options: AuthenticationGetSessionOptions; | ||
let silentFirst = false; | ||
switch (authScenario) { | ||
case AuthScenario.Initialization: | ||
case AuthScenario.GetSessionSilent: | ||
options = { createIfNone: false, clearSessionPreference: false, silent: true }; | ||
break; | ||
case AuthScenario.SignIn: | ||
options = { createIfNone: true, clearSessionPreference: true, silent: false }; | ||
break; | ||
case AuthScenario.GetSessionPrompt: | ||
// the 'createIfNone' option cannot be used with 'silent', but really we want both | ||
// flags here (i.e. create a session silently, but do create one if it doesn't exist). | ||
// To allow this, we first try to get a session silently. | ||
silentFirst = true; | ||
options = { createIfNone: true, clearSessionPreference: false, silent: false }; | ||
break; | ||
Comment on lines
+182
to
+188
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For
But the documentation for the
So to me, it sounds like enabling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pretty sure I tried that. However, I think it pops up a confirmation dialog if you enable (It also might be a feature request on the VS Code side, but I'm not sure I've got a comprehensive idea of what the options/behaviour I'm asking for would be.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, it sounds like it could be a bug. Or at least the docstring for |
||
} | ||
|
||
let session: AuthenticationSession | undefined; | ||
if (silentFirst) { | ||
// The 'silent' option is incompatible with most other options, so we completely replace the options object here. | ||
session = await authentication.getSession(getConfiguredAuthProviderId(), scopes, { silent: true }); | ||
} | ||
|
||
if (!session) { | ||
session = await authentication.getSession(getConfiguredAuthProviderId(), scopes, options); | ||
} | ||
|
||
if (!session) { | ||
return undefined; | ||
} | ||
|
||
return Object.assign(session, { tenantId }); | ||
} finally { | ||
this.handleSessionChanges = true; | ||
} | ||
} | ||
} | ||
|
||
function getDefaultScope(endpointUrl: string): string { | ||
// Endpoint URL is that of the audience, e.g. for ARM in the public cloud | ||
// it would be "https://management.azure.com". | ||
return endpointUrl.endsWith("/") ? `${endpointUrl}.default` : `${endpointUrl}/.default`; | ||
} | ||
|
||
async function getTenants(session: AuthenticationSession): Promise<DefinedTenant[]> { | ||
const { client } = await getSubscriptionClient(session); | ||
|
||
const results: TenantIdDescription[] = []; | ||
for await (const tenant of client.tenants.list()) { | ||
results.push(tenant); | ||
} | ||
|
||
return results.filter(isDefinedTenant); | ||
} | ||
|
||
function isDefinedTenant(tenant: TenantIdDescription): tenant is DefinedTenant { | ||
return tenant.tenantId !== undefined && tenant.displayName !== undefined; | ||
} | ||
|
||
function areStringCollectionsEqual(values1: string[], values2: string[]): boolean { | ||
return values1.sort().join(",") === values2.sort().join(","); | ||
} | ||
|
||
function addTenantIdScope(scopes: string[], tenantId: string): string[] { | ||
const scopeSet = new Set<string>(scopes); | ||
scopeSet.add(`VSCODE_TENANT:${tenantId}`); | ||
return Array.from(scopeSet); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth reconsidering the parameters for
getAuthSession
. For my purposes at least, the only reason for includingscopes
in there was to specify the specialVSCODE_CLIENT_ID
value.The way I've done it here, it's a bit weird that the special
TENANT_ID
scope is abstracted away, but consumers are expected specifyVSCODE_CLIENT_ID
explicitly.Could we consider something like an
options
object? E.g.If there's a need to specify unconstrained
scopes
it could be added later. However, if we add it now and it causes confusion ("what scopes do I actually need to specify?"), it'll be hard to remove.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VSCODE_CLIENT_ID
? How and why are you using this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there are scenarios where my team has needed to exclude the ARM scope, and add in something else. I will research into exactly what that was and report back. I want to make sure we can cover that scenario too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not yet, but I'm experimenting with using the session credentials to access
graph
endpoints for listing and creating applications and service principals. The default "Visual Studio Code" application does not have the required delegated permissions to do that. You can use theVSCODE_CLIENT_ID
scope to specify a different application.Speaking of that, when I said above I wasn't using the
scopes
option I think I was wrong. When creating an authentication provider needed to initialise the graph client, you need to implement agetAccessToken
function. This takes some optionalscopes
and I'm passing them on to mygetAuthSession
function. If I didn't do that, I think theaud
claim in the resulting access token would be wrong, and requests would be rejected by the graph endpoints.Perhaps the scenarios you're referring to above are similar (e.g. accessing graph rather than ARM).