development/vscode
development/vscode
1
0
Fork 0
mirror of https://github.com/Microsoft/vscode synced 2024-09-28 23:42:26 +00:00
Code Issues Packages Projects Releases Wiki Activity

Workaround MSAL behavior (#228289)

Browse source 
* Workaround MSAL behavior

The main change this makes is around what scopes are being requested.

Due to an MSAL or Identity issue, if you request a resource like `FOO/user_impersonation` and then `email`... the 2nd call does not use Graph and instead uses FOO and FOO may not have an `email` scope available. To work around this, if we detect that all scopes being requested are [OIDC scopes](https://learn.microsoft.com/en-us/entra/identity-platform/scopes-oidc#openid-connect-scopes) then we tack on `User.Read` to make sure that what gets returned is in fact from Graph. This prevents an infinite loop that was happening before. MSAL/Identity should fix this behavior, but this works for now.

Additionally, MSAL does already tack on OIDC scopes to all requests so I removed the logic that adds those.

Couple small things:
* Make sure MSAL logs get logged (trace)
* Use a Sequencer to make sure acquireToken calls are done sequentially just in case.

* more comment
This commit is contained in:
Tyler James Leonhardt 2024-09-11 17:29:29 -07:00 committed by GitHub
parent 614d30678f
commit b8be82f239
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 47 additions and 22 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

33
extensions/microsoft-authentication/src/common/scopeData.ts
View file

@ -6,6 +6,9 @@
const DEFAULT_CLIENT_ID = 'aebc6443-996d-45c2-90f0-388ff96faa56';
const DEFAULT_TENANT = 'organizations';
const OIDC_SCOPES = ['openid', 'email', 'profile', 'offline_access'];
const GRAPH_TACK_ON_SCOPE = 'User.Read';
export class ScopeData {
/**
@ -38,22 +41,10 @@ export class ScopeData {
constructor(readonly originalScopes: readonly string[] = []) {
const modifiedScopes = [...originalScopes];
if (!modifiedScopes.includes('openid')) {
modifiedScopes.push('openid');
}
if (!modifiedScopes.includes('email')) {
modifiedScopes.push('email');
}
if (!modifiedScopes.includes('profile')) {
modifiedScopes.push('profile');
}
if (!modifiedScopes.includes('offline_access')) {
modifiedScopes.push('offline_access');
}
modifiedScopes.sort();
this.allScopes = modifiedScopes;
this.scopeStr = modifiedScopes.join(' ');
this.scopesToSend = this.originalScopes.filter(s => !s.startsWith('VSCODE_'));
this.scopesToSend = this.getScopesToSend(modifiedScopes);
this.clientId = this.getClientId(this.allScopes);
this.tenant = this.getTenantId(this.allScopes);
}
@ -75,4 +66,20 @@ export class ScopeData {
return prev;
}, undefined) ?? DEFAULT_TENANT;
}
private getScopesToSend(scopes: string[]) {
const scopesToSend = scopes.filter(s => !s.startsWith('VSCODE_'));
const set = new Set(scopesToSend);
for (const scope of OIDC_SCOPES) {
set.delete(scope);
}
// If we only had OIDC scopes, we need to add a tack-on scope to make the request valid
// by forcing Identity into treating this as a Graph token request.
if (!set.size) {
scopesToSend.push(GRAPH_TACK_ON_SCOPE);
}
return scopesToSend;
}
}

17
extensions/microsoft-authentication/src/common/test/scopeData.test.ts
View file

@ -9,24 +9,29 @@ import { ScopeData } from '../scopeData';
suite('ScopeData', () => {
test('should include default scopes if not present', () => {
const scopeData = new ScopeData(['custom_scope']);
assert.deepStrictEqual(scopeData.allScopes, ['custom_scope', 'email', 'offline_access', 'openid', 'profile']);
assert.deepStrictEqual(scopeData.allScopes, ['custom_scope']);
});
test('should not duplicate default scopes if already present', () => {
const scopeData = new ScopeData(['openid', 'email', 'profile', 'offline_access']);
assert.deepStrictEqual(scopeData.allScopes, ['email', 'offline_access', 'openid', 'profile']);
const scopeData = new ScopeData(['custom_scope', 'openid', 'email', 'profile', 'offline_access']);
assert.deepStrictEqual(scopeData.allScopes, ['custom_scope', 'email', 'offline_access', 'openid', 'profile']);
});
test('should sort the scopes alphabetically', () => {
const scopeData = new ScopeData(['profile', 'email', 'openid', 'offline_access']);
assert.deepStrictEqual(scopeData.allScopes, ['email', 'offline_access', 'openid', 'profile']);
const scopeData = new ScopeData(['custom_scope', 'profile', 'email', 'openid', 'offline_access']);
assert.deepStrictEqual(scopeData.allScopes, ['custom_scope', 'email', 'offline_access', 'openid', 'profile']);
});
test('should create a space-separated string of all scopes', () => {
const scopeData = new ScopeData(['custom_scope']);
const scopeData = new ScopeData(['custom_scope', 'openid', 'email', 'offline_access', 'profile']);
assert.strictEqual(scopeData.scopeStr, 'custom_scope email offline_access openid profile');
});
test('should add TACK ON scope if all scopes are OIDC scopes', () => {
const scopeData = new ScopeData(['openid', 'email', 'offline_access', 'profile']);
assert.deepStrictEqual(scopeData.scopesToSend, ['email', 'offline_access', 'openid', 'profile', 'User.Read']);
});
test('should filter out internal VS Code scopes for scopesToSend', () => {
const scopeData = new ScopeData(['custom_scope', 'VSCODE_CLIENT_ID:some_id']);
assert.deepStrictEqual(scopeData.scopesToSend, ['custom_scope']);

19
extensions/microsoft-authentication/src/node/cachedPublicClientApplication.ts
View file

@ -3,7 +3,7 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/
import { PublicClientApplication, AccountInfo, Configuration, SilentFlowRequest, AuthenticationResult, InteractiveRequest } from '@azure/msal-node';
import { PublicClientApplication, AccountInfo, Configuration, SilentFlowRequest, AuthenticationResult, InteractiveRequest, LogLevel } from '@azure/msal-node';
import { Disposable, Memento, SecretStorage, LogOutputChannel, window, ProgressLocation, l10n, EventEmitter } from 'vscode';
import { raceCancellationAndTimeoutError } from '../common/async';
import { SecretStorageCachePlugin } from '../common/cachePlugin';
@ -12,6 +12,7 @@ import { ICachedPublicClientApplication } from '../common/publicClientCache';
export class CachedPublicClientApplication implements ICachedPublicClientApplication {
private _pca: PublicClientApplication;
private _sequencer = new Sequencer();
private _accounts: AccountInfo[] = [];
private readonly _disposable: Disposable;
@ -28,6 +29,7 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica
loggerOptions: {
correlationId: `${this._clientId}] [${this._authority}`,
loggerCallback: (level, message, containsPii) => this._loggerOptions.loggerCallback(level, message, containsPii),
logLevel: LogLevel.Trace
}
},
cache: {
@ -82,9 +84,11 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica
}
async acquireTokenSilent(request: SilentFlowRequest): Promise<AuthenticationResult> {
this._logger.debug(`[acquireTokenSilent] [${this._clientId}] [${this._authority}] [${request.scopes.join(' ')}]`);
const result = await this._pca.acquireTokenSilent(request);
this._logger.debug(`[acquireTokenSilent] [${this._clientId}] [${this._authority}] [${request.scopes.join(' ')}] [${request.account.username}] starting...`);
const result = await this._sequencer.queue(() => this._pca.acquireTokenSilent(request));
this._logger.debug(`[acquireTokenSilent] [${this._clientId}] [${this._authority}] [${request.scopes.join(' ')}] [${request.account.username}] got result`);
if (result.account && !result.fromCache) {
this._logger.debug(`[acquireTokenSilent] [${this._clientId}] [${this._authority}] [${request.scopes.join(' ')}] [${request.account.username}] firing event due to change`);
this._onDidAccountsChangeEmitter.fire({ added: [], changed: [result.account], deleted: [] });
}
return result;
@ -146,3 +150,12 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica
this._logger.debug(`[update] [${this._clientId}] [${this._authority}] CachedPublicClientApplication update complete`);
}
}
export class Sequencer {
private current: Promise<unknown> = Promise.resolve(null);
queue<T>(promiseTask: () => Promise<T>): Promise<T> {
return this.current = this.current.then(() => promiseTask(), () => promiseTask());
}
}