Skip to content

Commit 4d496f6

Browse files
TylerLeonhardtTylerLeonhardt
committed
remove keytar fallback in keychain and add a ton more logging to microsoft auth. ref microsoft#133201 
1 parent 7fd7b8f commit 4d496f6

File tree

6 files changed

+74
-144
lines changed

6 files changed

+74
-144
lines changed

‎extensions/github-authentication/src/common/keychain.ts

Lines changed: 0 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -5,29 +5,12 @@
55

66
// keytar depends on a native module shipped in vscode, so this is
77
// how we load it
8-
import type * as keytarType from 'keytar';
98
import * as vscode from 'vscode';
109
import * as nls from 'vscode-nls';
1110
import { Log } from './logger';
1211

1312
const localize = nls.loadMessageBundle();
1413

15-
function getKeytar(): Keytar | undefined {
16-
try {
17-
return require('keytar');
18-
} catch (err) {
19-
console.log(err);
20-
}
21-
22-
return undefined;
23-
}
24-
25-
export type Keytar = {
26-
getPassword: typeof keytarType['getPassword'];
27-
setPassword: typeof keytarType['setPassword'];
28-
deletePassword: typeof keytarType['deletePassword'];
29-
};
30-
3114
export class Keychain {
3215
constructor(
3316
private readonly context: vscode.ExtensionContext,
@@ -72,25 +55,4 @@ export class Keychain {
7255
return Promise.resolve(undefined);
7356
}
7457
}
75-
76-
async tryMigrate(): Promise<string | null | undefined> {
77-
try {
78-
const keytar = getKeytar();
79-
if (!keytar) {
80-
throw new Error('keytar unavailable');
81-
}
82-
83-
const oldValue = await keytar.getPassword(`${vscode.env.uriScheme}-github.login`, 'account');
84-
if (oldValue) {
85-
this.Logger.trace('Attempting to migrate from keytar to secret store...');
86-
await this.setToken(oldValue);
87-
await keytar.deletePassword(`${vscode.env.uriScheme}-github.login`, 'account');
88-
}
89-
90-
return oldValue;
91-
} catch (_) {
92-
// Ignore
93-
return Promise.resolve(undefined);
94-
}
95-
}
9658
}

‎extensions/github-authentication/src/github.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ export class GitHubAuthenticationProvider implements vscode.AuthenticationProvid
117117
let sessionData: SessionData[];
118118
try {
119119
this._logger.info('Reading sessions from keychain...');
120-
const storedSessions = await this._keychain.getToken() || await this._keychain.tryMigrate();
120+
const storedSessions = await this._keychain.getToken();
121121
if (!storedSessions) {
122122
return [];
123123
}

‎extensions/microsoft-authentication/src/AADHelper.ts

Lines changed: 67 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ export class AzureActiveDirectoryService {
100100
private _tokens: IToken[] = [];
101101
private _refreshTimeouts: Map<string, NodeJS.Timeout> = new Map<string, NodeJS.Timeout>();
102102
private _uriHandler: UriEventHandler;
103-
private _disposables: vscode.Disposable[] = [];
103+
private _disposable: vscode.Disposable;
104104

105105
// Used to keep track of current requests when not using the local server approach.
106106
private _pendingStates = new Map<string, string[]>();
@@ -112,51 +112,59 @@ export class AzureActiveDirectoryService {
112112
constructor(private _context: vscode.ExtensionContext) {
113113
this._keychain = new Keychain(_context);
114114
this._uriHandler = new UriEventHandler();
115-
this._disposables.push(vscode.window.registerUriHandler(this._uriHandler));
115+
this._disposable = vscode.Disposable.from(
116+
vscode.window.registerUriHandler(this._uriHandler),
117+
this._context.secrets.onDidChange(() => this.checkForUpdates()));
116118
}
117119

118120
public async initialize(): Promise<void> {
119-
const storedData = await this._keychain.getToken() || await this._keychain.tryMigrate();
120-
if (storedData) {
121-
try {
122-
const sessions = this.parseStoredData(storedData);
123-
const refreshes = sessions.map(async session => {
124-
if (!session.refreshToken) {
125-
return Promise.resolve();
126-
}
121+
Logger.info('Reading sessions from keychain...');
122+
const storedData = await this._keychain.getToken();
123+
if (!storedData) {
124+
Logger.info('No stored sessions found.');
125+
return;
126+
}
127+
Logger.info('Got stored sessions!');
127128

128-
try {
129-
await this.refreshToken(session.refreshToken, session.scope, session.id);
130-
} catch (e) {
131-
if (e.message === REFRESH_NETWORK_FAILURE) {
132-
const didSucceedOnRetry = await this.handleRefreshNetworkError(session.id, session.refreshToken, session.scope);
133-
if (!didSucceedOnRetry) {
134-
this._tokens.push({
135-
accessToken: undefined,
136-
refreshToken: session.refreshToken,
137-
account: {
138-
label: session.account.label ?? session.account.displayName!,
139-
id: session.account.id
140-
},
141-
scope: session.scope,
142-
sessionId: session.id
143-
});
144-
this.pollForReconnect(session.id, session.refreshToken, session.scope);
145-
}
146-
} else {
147-
await this.removeSession(session.id);
129+
try {
130+
const sessions = this.parseStoredData(storedData);
131+
const refreshes = sessions.map(async session => {
132+
Logger.trace(`Read the following session from the keychain with the following scopes: ${session.scope}`);
133+
if (!session.refreshToken) {
134+
Logger.trace(`Session with the following scopes does not have a refresh token so we will not try to refresh it: ${session.scope}`);
135+
return Promise.resolve();
136+
}
137+
138+
try {
139+
await this.refreshToken(session.refreshToken, session.scope, session.id);
140+
} catch (e) {
141+
// If we aren't connected to the internet, then wait and try to refresh again later.
142+
if (e.message === REFRESH_NETWORK_FAILURE) {
143+
const didSucceedOnRetry = await this.handleRefreshNetworkError(session.id, session.refreshToken, session.scope);
144+
if (!didSucceedOnRetry) {
145+
this._tokens.push({
146+
accessToken: undefined,
147+
refreshToken: session.refreshToken,
148+
account: {
149+
label: session.account.label ?? session.account.displayName!,
150+
id: session.account.id
151+
},
152+
scope: session.scope,
153+
sessionId: session.id
154+
});
155+
this.pollForReconnect(session.id, session.refreshToken, session.scope);
148156
}
157+
} else {
158+
await this.removeSession(session.id);
149159
}
150-
});
160+
}
161+
});
151162

152-
await Promise.all(refreshes);
153-
} catch (e) {
154-
Logger.info('Failed to initialize stored data');
155-
await this.clearSessions();
156-
}
163+
await Promise.all(refreshes);
164+
} catch (e) {
165+
Logger.error(`Failed to initialize stored data: ${e}`);
166+
await this.clearSessions();
157167
}
158-
159-
this._disposables.push(this._context.secrets.onDidChange(() => this.checkForUpdates));
160168
}
161169

162170
private parseStoredData(data: string): IStoredSession[] {
@@ -263,16 +271,16 @@ export class AzureActiveDirectoryService {
263271
private async resolveAccessAndIdTokens(token: IToken): Promise<IMicrosoftTokens> {
264272
if (token.accessToken && (!token.expiresAt || token.expiresAt > Date.now())) {
265273
token.expiresAt
266-
? Logger.info(`Token available from cache, expires in ${token.expiresAt - Date.now()} milliseconds`)
267-
: Logger.info('Token available from cache');
274+
? Logger.info(`Token available from cache (for scopes ${token.scope}), expires in ${token.expiresAt - Date.now()} milliseconds`)
275+
: Logger.info('Token available from cache (for scopes ${token.scope})');
268276
return Promise.resolve({
269277
accessToken: token.accessToken,
270278
idToken: token.idToken
271279
});
272280
}
273281

274282
try {
275-
Logger.info('Token expired or unavailable, trying refresh');
283+
Logger.info(`Token expired or unavailable (for scopes ${token.scope}), trying refresh`);
276284
const refreshedToken = await this.refreshToken(token.refreshToken, token.scope, token.sessionId);
277285
if (refreshedToken.accessToken) {
278286
return {
@@ -301,17 +309,21 @@ export class AzureActiveDirectoryService {
301309
}
302310

303311
async getSessions(scopes?: string[]): Promise<vscode.AuthenticationSession[]> {
312+
Logger.info(`Getting sessions for ${scopes?.join(',') ?? 'all scopes'}...`);
304313
if (!scopes) {
305-
return this.sessions;
314+
const sessions = await this.sessions;
315+
Logger.info(`Got ${sessions.length} sessions for all scopes...`);
316+
return sessions;
306317
}
307318

308319
const orderedScopes = scopes.sort().join(' ');
309320
const matchingTokens = this._tokens.filter(token => token.scope === orderedScopes);
321+
Logger.info(`Got ${matchingTokens.length} sessions for ${scopes?.join(',')}...`);
310322
return Promise.all(matchingTokens.map(token => this.convertToSession(token)));
311323
}
312324

313325
public async createSession(scope: string): Promise<vscode.AuthenticationSession> {
314-
Logger.info('Logging in...');
326+
Logger.info(`Logging in for the following scopes: ${scope}`);
315327
if (!scope.includes('offline_access')) {
316328
Logger.info('Warning: The \'offline_access\' scope was not included, so the generated token will not be able to be refreshed.');
317329
}
@@ -360,7 +372,7 @@ export class AzureActiveDirectoryService {
360372
}
361373
token = await this.exchangeCodeForToken(codeRes.code, codeVerifier, scope);
362374
this.setToken(token, scope);
363-
Logger.info('Login successful');
375+
Logger.info(`Login successful for scopes: ${scope}`);
364376
res.writeHead(302, { Location: '/' });
365377
const session = await this.convertToSession(token);
366378
return session;
@@ -371,7 +383,7 @@ export class AzureActiveDirectoryService {
371383
res.end();
372384
}
373385
} catch (e) {
374-
Logger.error(e.message);
386+
Logger.error(`Error creating session for scopes: ${scope} Error: ${e}`);
375387

376388
// If the error was about starting the server, try directly hitting the login endpoint instead
377389
if (e.message === 'Error listening to server' || e.message === 'Closed' || e.message === 'Timeout waiting for port') {
@@ -387,8 +399,7 @@ export class AzureActiveDirectoryService {
387399
}
388400

389401
public dispose(): void {
390-
this._disposables.forEach(disposable => disposable.dispose());
391-
this._disposables = [];
402+
this._disposable.dispose();
392403
}
393404

394405
private getCallbackEnvironment(callbackUri: vscode.Uri): string {
@@ -550,7 +561,7 @@ export class AzureActiveDirectoryService {
550561
}
551562

552563
private async exchangeCodeForToken(code: string, codeVerifier: string, scope: string): Promise<IToken> {
553-
Logger.info('Exchanging login code for token');
564+
Logger.info(`Exchanging login code for token for scopes: ${scope}`);
554565
try {
555566
const postData = querystring.stringify({
556567
grant_type: 'authorization_code',
@@ -575,21 +586,21 @@ export class AzureActiveDirectoryService {
575586
});
576587

577588
if (result.ok) {
578-
Logger.info('Exchanging login code for token success');
579589
const json = await result.json();
590+
Logger.info(`Exchanging login code for token (for scopes: ${scope}) succeeded!`);
580591
return this.getTokenFromResponse(json, scope);
581592
} else {
582-
Logger.error('Exchanging login code for token failed');
593+
Logger.error(`Exchanging login code for token (for scopes: ${scope}) failed: ${await result.text()}`);
583594
throw new Error('Unable to login.');
584595
}
585596
} catch (e) {
586-
Logger.error(e.message);
597+
Logger.error(`Error exchanging code for token (for scopes ${scope}): ${e}`);
587598
throw e;
588599
}
589600
}
590601

591602
private async refreshToken(refreshToken: string, scope: string, sessionId: string): Promise<IToken> {
592-
Logger.info('Refreshing token...');
603+
Logger.info(`Refreshing token for scopes: ${scope}`);
593604
const postData = querystring.stringify({
594605
refresh_token: refreshToken,
595606
client_id: clientId,
@@ -611,7 +622,7 @@ export class AzureActiveDirectoryService {
611622
body: postData
612623
});
613624
} catch (e) {
614-
Logger.error('Refreshing token failed');
625+
Logger.error(`Refreshing token failed (for scopes: ${scope}) Error: ${e}`);
615626
throw new Error(REFRESH_NETWORK_FAILURE);
616627
}
617628

@@ -620,14 +631,14 @@ export class AzureActiveDirectoryService {
620631
const json = await result.json();
621632
const token = this.getTokenFromResponse(json, scope, sessionId);
622633
this.setToken(token, scope);
623-
Logger.info('Token refresh success');
634+
Logger.info(`Token refresh success for scopes: ${token.scope}`);
624635
return token;
625636
} else {
626637
throw new Error('Bad request.');
627638
}
628639
} catch (e) {
629640
vscode.window.showErrorMessage(localize('signOut', "You have been signed out because reading stored authentication information failed."));
630-
Logger.error(`Refreshing token failed: ${result.statusText}`);
641+
Logger.error(`Refreshing token failed (for scopes: ${scope}): ${result.statusText}`);
631642
throw new Error('Refreshing token failed');
632643
}
633644
}
@@ -668,7 +679,7 @@ export class AzureActiveDirectoryService {
668679
private handleRefreshNetworkError(sessionId: string, refreshToken: string, scope: string, attempts: number = 1): Promise<boolean> {
669680
return new Promise((resolve, _) => {
670681
if (attempts === 3) {
671-
Logger.error('Token refresh failed after 3 attempts');
682+
Logger.error(`Token refresh (for scopes: ${scope}) failed after 3 attempts`);
672683
return resolve(false);
673684
}
674685

‎extensions/microsoft-authentication/src/extension.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@ import * as vscode from 'vscode';
77
import { AzureActiveDirectoryService, onDidChangeSessions } from './AADHelper';
88
import TelemetryReporter from 'vscode-extension-telemetry';
99

10-
export const DEFAULT_SCOPES = 'https://management.core.windows.net/.default offline_access';
11-
1210
export async function activate(context: vscode.ExtensionContext) {
1311
const { name, version, aiKey } = context.extension.packageJSON as { name: string, version: string, aiKey: string };
1412
const telemetryReporter = new TelemetryReporter(name, version, aiKey);

‎extensions/microsoft-authentication/src/keychain.ts

Lines changed: 1 addition & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -3,47 +3,17 @@
33
* Licensed under the MIT License. See License.txt in the project root for license information.
44
*--------------------------------------------------------------------------------------------*/
55

6-
// keytar depends on a native module shipped in vscode, so this is
7-
// how we load it
8-
import * as keytarType from 'keytar';
96
import * as vscode from 'vscode';
107
import Logger from './logger';
118
import * as nls from 'vscode-nls';
129

1310
const localize = nls.loadMessageBundle();
1411

15-
function getKeytar(): Keytar | undefined {
16-
try {
17-
return require('keytar');
18-
} catch (err) {
19-
console.log(err);
20-
}
21-
22-
return undefined;
23-
}
24-
25-
export type Keytar = {
26-
getPassword: typeof keytarType['getPassword'];
27-
setPassword: typeof keytarType['setPassword'];
28-
deletePassword: typeof keytarType['deletePassword'];
29-
};
30-
31-
const OLD_SERVICE_ID = `${vscode.env.uriScheme}-microsoft.login`;
3212
const SERVICE_ID = `microsoft.login`;
33-
const ACCOUNT_ID = 'account';
3413

3514
export class Keychain {
36-
private keytar: Keytar;
37-
38-
constructor(private context: vscode.ExtensionContext) {
39-
const keytar = getKeytar();
40-
if (!keytar) {
41-
throw new Error('System keychain unavailable');
42-
}
43-
44-
this.keytar = keytar;
45-
}
4615

16+
constructor(private context: vscode.ExtensionContext) { }
4717

4818
async setToken(token: string): Promise<void> {
4919

@@ -87,19 +57,4 @@ export class Keychain {
8757
return Promise.resolve(undefined);
8858
}
8959
}
90-
91-
async tryMigrate(): Promise<string | null> {
92-
try {
93-
const oldValue = await this.keytar.getPassword(OLD_SERVICE_ID, ACCOUNT_ID);
94-
if (oldValue) {
95-
await this.setToken(oldValue);
96-
await this.keytar.deletePassword(OLD_SERVICE_ID, ACCOUNT_ID);
97-
}
98-
99-
return oldValue;
100-
} catch (_) {
101-
// Ignore
102-
return Promise.resolve(null);
103-
}
104-
}
10560
}

0 commit comments

Comments
 (0)