Skip to content
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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions auth/src/AzureSessionProvider.ts
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>;

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 including scopes in there was to specify the special VSCODE_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 specify VSCODE_CLIENT_ID explicitly.

Could we consider something like an options object? E.g.

export type GetAuthSessionParams {
    tenantId: string;
    behavior: GetSessionBehavior;
    applicationClientId?: string;
}

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I'm not familiar with the special VSCODE_CLIENT_ID? How and why are you using this?
  2. And I agree, I think an options object would be good here. We can add an optional scopes property in there too.
export type GetAuthSessionParams {
    tenantId: string;
    behavior: GetSessionBehavior;
    applicationClientId?: string;
    scopes?: string[];
}

Copy link
Member Author

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with the special VSCODE_CLIENT_ID? How and why are you using this?

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 the VSCODE_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 a getAccessToken function. This takes some optional scopes and I'm passing them on to my getAuthSession function. If I didn't do that, I think the aud 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).

dispose(): void;
};
241 changes: 241 additions & 0 deletions auth/src/VSCodeAzureSessionProvider.ts
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
Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The 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 enum abstraction starts to look a bit better.


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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For AuthScenario.GetSessionPrompt the comment states:

// 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.

But the documentation for the createIfNone option is:

Whether login should be performed if there is no matching session.

So to me, it sounds like enabling createIfNone will do what you're trying to achieve. If there is not matching session, it will prompt. Why do we have to call getSession with silent enabled before hand?

Choose a reason for hiding this comment

The 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 createIfNone, which is what I was trying to avoid with the
silent option. I'd be happy to be proved wrong on that though - this is one of my least favourite parts of the code.

(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.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.)

Yeah, it sounds like it could be a bug. Or at least the docstring for createIfNone is misleading.

}

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);
}
Loading