Skip to content

Commit af4ea03

Browse files
TylerLeonhardtTylerLeonhardt
authored
Use the session access flow (microsoft#165567)
* Use the session access flow At some point I accidentally stopped using `requestSessionAccess`. It helps a ton when you have multiple extensions that are requesting the same scopes. This re-uses it. Also fixed a leaking disposable. * clean up session preference a bit * use new api in one more place
1 parent 6933e6f commit af4ea03

File tree

4 files changed

+75
-40
lines changed

4 files changed

+75
-40
lines changed

‎src/vs/workbench/api/browser/mainThreadAuthentication.ts

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -199,12 +199,6 @@ export class MainThreadAuthentication extends Disposable implements MainThreadAu
199199
return choice === 0;
200200
}
201201

202-
private async setTrustedExtensionAndAccountPreference(providerId: string, accountName: string, extensionId: string, extensionName: string, sessionId: string): Promise<void> {
203-
this.authenticationService.updatedAllowedExtension(providerId, accountName, extensionId, extensionName, true);
204-
this.storageService.store(`${extensionName}-${providerId}`, sessionId, StorageScope.APPLICATION, StorageTarget.MACHINE);
205-
206-
}
207-
208202
private async doGetSession(providerId: string, scopes: string[], extensionId: string, extensionName: string, options: AuthenticationGetSessionOptions): Promise<AuthenticationSession | undefined> {
209203
const sessions = await this.authenticationService.getSessions(providerId, scopes, true);
210204
const supportsMultipleAccounts = this.authenticationService.supportsMultipleAccounts(providerId);
@@ -224,9 +218,12 @@ export class MainThreadAuthentication extends Disposable implements MainThreadAu
224218
if (!options.forceNewSession && sessions.length) {
225219
if (supportsMultipleAccounts) {
226220
if (options.clearSessionPreference) {
227-
this.storageService.remove(`${extensionName}-${providerId}`, StorageScope.APPLICATION);
221+
// Clearing the session preference is usually paired with createIfNone, so just remove the preference and
222+
// defer to the rest of the logic in this function to choose the session.
223+
this.authenticationService.removeSessionPreference(providerId, extensionId, scopes);
228224
} else {
229-
const existingSessionPreference = this.storageService.get(`${extensionName}-${providerId}`, StorageScope.APPLICATION);
225+
// If we have an existing session preference, use that. If not, we'll return any valid session at the end of this function.
226+
const existingSessionPreference = this.authenticationService.getSessionPreference(providerId, extensionId, scopes);
230227
if (existingSessionPreference) {
231228
const matchingSession = sessions.find(session => session.id === existingSessionPreference);
232229
if (matchingSession && this.authenticationService.isAccessAllowed(providerId, matchingSession.account.label, extensionId)) {
@@ -256,17 +253,36 @@ export class MainThreadAuthentication extends Disposable implements MainThreadAu
256253
const session = sessions?.length && !options.forceNewSession && supportsMultipleAccounts
257254
? await this.authenticationService.selectSession(providerId, extensionId, extensionName, scopes, sessions)
258255
: await this.authenticationService.createSession(providerId, scopes, true);
259-
await this.setTrustedExtensionAndAccountPreference(providerId, session.account.label, extensionId, extensionName, session.id);
256+
this.authenticationService.updateAllowedExtension(providerId, session.account.label, extensionId, extensionName, true);
257+
this.authenticationService.updateSessionPreference(providerId, extensionId, session);
260258
return session;
261259
}
262260

263-
// passive flows (silent or default)
261+
// For the silent flows, if we have a session, even though it may not be the user's preference, we'll return it anyway because it might be for a specific
262+
// set of scopes.
263+
const validSession = sessions.find(session => this.authenticationService.isAccessAllowed(providerId, session.account.label, extensionId));
264+
if (validSession) {
265+
// Migration. If we have a valid session, but no preference, we'll set the preference to the valid session.
266+
// TODO: Remove this after in a few releases.
267+
if (!this.authenticationService.getSessionPreference(providerId, extensionId, scopes)) {
268+
if (this.storageService.get(`${extensionName}-${providerId}`, StorageScope.APPLICATION)) {
269+
this.storageService.remove(`${extensionName}-${providerId}`, StorageScope.APPLICATION);
270+
}
271+
this.authenticationService.updateAllowedExtension(providerId, validSession.account.label, extensionId, extensionName, true);
272+
this.authenticationService.updateSessionPreference(providerId, extensionId, validSession);
273+
}
274+
return validSession;
275+
}
264276

265-
const validSession = sessions.find(s => this.authenticationService.isAccessAllowed(providerId, s.account.label, extensionId));
266-
if (!options.silent && !validSession) {
267-
await this.authenticationService.requestNewSession(providerId, scopes, extensionId, extensionName);
277+
// passive flows (silent or default)
278+
if (!options.silent) {
279+
// If there is a potential session, but the extension doesn't have access to it, use the "grant access" flow,
280+
// otherwise request a new one.
281+
sessions.length
282+
? this.authenticationService.requestSessionAccess(providerId, extensionId, extensionName, scopes, sessions)
283+
: await this.authenticationService.requestNewSession(providerId, scopes, extensionId, extensionName);
268284
}
269-
return validSession;
285+
return undefined;
270286
}
271287

272288
async $getSession(providerId: string, scopes: string[], extensionId: string, extensionName: string, options: AuthenticationGetSessionOptions): Promise<AuthenticationSession | undefined> {

‎src/vs/workbench/api/test/browser/extHostAuthentication.test.ts

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -171,9 +171,9 @@ suite('ExtHostAuthentication', () => {
171171
scopes,
172172
{});
173173

174-
assert.strictEqual(session.id, session2?.id);
175-
assert.strictEqual(session.scopes[0], session2?.scopes[0]);
176-
assert.strictEqual(session.accessToken, session2?.accessToken);
174+
assert.strictEqual(session2?.id, session.id);
175+
assert.strictEqual(session2?.scopes[0], session.scopes[0]);
176+
assert.strictEqual(session2?.accessToken, session.accessToken);
177177
});
178178

179179
// should behave the same as createIfNone: false
@@ -312,9 +312,9 @@ suite('ExtHostAuthentication', () => {
312312

313313
// clearing session preference causes us to get the first session
314314
// because it would normally show a quick pick for the user to choose
315-
assert.strictEqual(session.id, session3?.id);
316-
assert.strictEqual(session.scopes[0], session3?.scopes[0]);
317-
assert.strictEqual(session.accessToken, session3?.accessToken);
315+
assert.strictEqual(session3?.id, session.id);
316+
assert.strictEqual(session3?.scopes[0], session.scopes[0]);
317+
assert.strictEqual(session3?.accessToken, session.accessToken);
318318
});
319319

320320
test('silently getting session should return a session (if any) regardless of preference - fixes #137819', async () => {
@@ -347,18 +347,18 @@ suite('ExtHostAuthentication', () => {
347347
'test-multiple',
348348
scopes,
349349
{});
350-
assert.strictEqual(session.id, shouldBeSession1?.id);
351-
assert.strictEqual(session.scopes[0], shouldBeSession1?.scopes[0]);
352-
assert.strictEqual(session.accessToken, shouldBeSession1?.accessToken);
350+
assert.strictEqual(shouldBeSession1?.id, session.id);
351+
assert.strictEqual(shouldBeSession1?.scopes[0], session.scopes[0]);
352+
assert.strictEqual(shouldBeSession1?.accessToken, session.accessToken);
353353

354354
const shouldBeSession2 = await extHostAuthentication.getSession(
355355
extensionDescription,
356356
'test-multiple',
357357
scopes2,
358358
{});
359-
assert.strictEqual(session2.id, shouldBeSession2?.id);
360-
assert.strictEqual(session2.scopes[0], shouldBeSession2?.scopes[0]);
361-
assert.strictEqual(session2.accessToken, shouldBeSession2?.accessToken);
359+
assert.strictEqual(shouldBeSession2?.id, session2.id);
360+
assert.strictEqual(shouldBeSession2?.scopes[0], session2.scopes[0]);
361+
assert.strictEqual(shouldBeSession2?.accessToken, session2.accessToken);
362362
});
363363

364364
//#endregion

‎src/vs/workbench/services/authentication/browser/authenticationService.ts

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -387,7 +387,7 @@ export class AuthenticationService extends Disposable implements IAuthentication
387387
return undefined;
388388
}
389389

390-
async updatedAllowedExtension(providerId: string, accountName: string, extensionId: string, extensionName: string, isAllowed: boolean): Promise<void> {
390+
updateAllowedExtension(providerId: string, accountName: string, extensionId: string, extensionName: string, isAllowed: boolean): void {
391391
const allowList = readAllowedExtensions(this.storageService, providerId, accountName);
392392
const index = allowList.findIndex(extension => extension.id === extensionId);
393393
if (index === -1) {
@@ -396,9 +396,29 @@ export class AuthenticationService extends Disposable implements IAuthentication
396396
allowList[index].allowed = isAllowed;
397397
}
398398

399-
await this.storageService.store(`${providerId}-${accountName}`, JSON.stringify(allowList), StorageScope.APPLICATION, StorageTarget.USER);
399+
this.storageService.store(`${providerId}-${accountName}`, JSON.stringify(allowList), StorageScope.APPLICATION, StorageTarget.USER);
400400
}
401401

402+
//#region Session Preference
403+
404+
updateSessionPreference(providerId: string, extensionId: string, session: AuthenticationSession): void {
405+
// The 3 parts of this key are important:
406+
// * Extension id: The extension that has a preference
407+
// * Provider id: The provider that the preference is for
408+
// * The scopes: The subset of sessions that the preference applies to
409+
this.storageService.store(`${extensionId}-${providerId}-${session.scopes.join(' ')}`, session.id, StorageScope.APPLICATION, StorageTarget.MACHINE);
410+
}
411+
412+
getSessionPreference(providerId: string, extensionId: string, scopes: string[]): string | undefined {
413+
return this.storageService.get(`${extensionId}-${providerId}-${scopes.join(' ')}`, StorageScope.APPLICATION, undefined);
414+
}
415+
416+
removeSessionPreference(providerId: string, extensionId: string, scopes: string[]): void {
417+
this.storageService.remove(`${extensionId}-${providerId}-${scopes.join(' ')}`, StorageScope.APPLICATION);
418+
}
419+
420+
//#endregion
421+
402422
async showGetSessionPrompt(providerId: string, accountName: string, extensionId: string, extensionName: string): Promise<boolean> {
403423
const providerName = this.getLabel(providerId);
404424
const { choice } = await this.dialogService.show(
@@ -413,7 +433,7 @@ export class AuthenticationService extends Disposable implements IAuthentication
413433
const cancelled = choice === 2;
414434
const allowed = choice === 0;
415435
if (!cancelled) {
416-
this.updatedAllowedExtension(providerId, accountName, extensionId, extensionName, allowed);
436+
this.updateAllowedExtension(providerId, accountName, extensionId, extensionName, allowed);
417437
this.removeAccessRequest(providerId, extensionId);
418438
}
419439

@@ -458,10 +478,9 @@ export class AuthenticationService extends Disposable implements IAuthentication
458478
const session = quickPick.selectedItems[0].session ?? await this.createSession(providerId, scopes);
459479
const accountName = session.account.label;
460480

461-
this.updatedAllowedExtension(providerId, accountName, extensionId, extensionName, true);
462-
481+
this.updateAllowedExtension(providerId, accountName, extensionId, extensionName, true);
482+
this.updateSessionPreference(providerId, extensionId, session);
463483
this.removeAccessRequest(providerId, extensionId);
464-
this.storageService.store(`${extensionName}-${providerId}`, session.id, StorageScope.APPLICATION, StorageTarget.MACHINE);
465484

466485
quickPick.dispose();
467486
resolve(session);
@@ -551,9 +570,10 @@ export class AuthenticationService extends Disposable implements IAuthentication
551570
// since this is sync and returns a disposable. So, wait for registration event to fire that indicates the
552571
// provider is now in the map.
553572
await new Promise<void>((resolve, _) => {
554-
this.onDidRegisterAuthenticationProvider(e => {
573+
const dispose = this.onDidRegisterAuthenticationProvider(e => {
555574
if (e.id === providerId) {
556575
provider = this._authenticationProviders.get(providerId);
576+
dispose.dispose();
557577
resolve();
558578
}
559579
});
@@ -594,14 +614,10 @@ export class AuthenticationService extends Disposable implements IAuthentication
594614
id: commandId,
595615
handler: async (accessor) => {
596616
const authenticationService = accessor.get(IAuthenticationService);
597-
const storageService = accessor.get(IStorageService);
598617
const session = await authenticationService.createSession(providerId, scopes);
599618

600-
// Add extension to allow list since user explicitly signed in on behalf of it
601-
this.updatedAllowedExtension(providerId, session.account.label, extensionId, extensionName, true);
602-
603-
// And also set it as the preferred account for the extension
604-
storageService.store(`${extensionName}-${providerId}`, session.id, StorageScope.APPLICATION, StorageTarget.MACHINE);
619+
this.updateAllowedExtension(providerId, session.account.label, extensionId, extensionName, true);
620+
this.updateSessionPreference(providerId, extensionId, session);
605621
}
606622
});
607623

‎src/vs/workbench/services/authentication/common/authentication.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,10 @@ export interface IAuthenticationService {
3737
registerAuthenticationProvider(id: string, provider: IAuthenticationProvider): void;
3838
unregisterAuthenticationProvider(id: string): void;
3939
isAccessAllowed(providerId: string, accountName: string, extensionId: string): boolean | undefined;
40-
updatedAllowedExtension(providerId: string, accountName: string, extensionId: string, extensionName: string, isAllowed: boolean): Promise<void>;
40+
updateAllowedExtension(providerId: string, accountName: string, extensionId: string, extensionName: string, isAllowed: boolean): void;
41+
updateSessionPreference(providerId: string, extensionId: string, session: AuthenticationSession): void;
42+
getSessionPreference(providerId: string, extensionId: string, scopes: string[]): string | undefined;
43+
removeSessionPreference(providerId: string, extensionId: string, scopes: string[]): void;
4144
showGetSessionPrompt(providerId: string, accountName: string, extensionId: string, extensionName: string): Promise<boolean>;
4245
selectSession(providerId: string, extensionId: string, extensionName: string, scopes: string[], possibleSessions: readonly AuthenticationSession[]): Promise<AuthenticationSession>;
4346
requestSessionAccess(providerId: string, extensionId: string, extensionName: string, scopes: string[], possibleSessions: readonly AuthenticationSession[]): void;

0 commit comments

Comments
 (0)