don't surface error on gets and remove microsoft secret migration. Fixes #146553

This commit is contained in:
Tyler Leonhardt 2022-04-14 12:18:25 -07:00
parent ed1eaf32f8
commit 5919378269
No known key found for this signature in database
GPG key ID: D9BFA0BA8AD9F6F7
6 changed files with 51 additions and 112 deletions

View file

@ -6,11 +6,8 @@
// keytar depends on a native module shipped in vscode, so this is
// how we load it
import * as vscode from 'vscode';
import * as nls from 'vscode-nls';
import { Log } from './logger';
const localize = nls.loadMessageBundle();
export class Keychain {
constructor(
private readonly context: vscode.ExtensionContext,
@ -24,11 +21,6 @@ export class Keychain {
} catch (e) {
// Ignore
this.Logger.error(`Setting token failed: ${e}`);
const troubleshooting = localize('troubleshooting', "Troubleshooting Guide");
const result = await vscode.window.showErrorMessage(localize('keychainWriteError', "Writing login information to the keychain failed with error '{0}'.", e.message), troubleshooting);
if (result === troubleshooting) {
vscode.env.openExternal(vscode.Uri.parse('https://code.visualstudio.com/docs/editor/settings-sync#_troubleshooting-keychain-issues'));
}
}
}

View file

@ -10,7 +10,6 @@ import * as vscode from 'vscode';
import * as nls from 'vscode-nls';
import { v4 as uuid } from 'uuid';
import fetch, { Response } from 'node-fetch';
import { Keychain } from './keychain';
import Logger from './logger';
import { toBase64UrlEncoding } from './utils';
import { sha256 } from './env/node/sha256';
@ -125,10 +124,6 @@ export class AzureActiveDirectoryService {
let sessions = await this._tokenStorage.getAll();
Logger.info(`Got ${sessions.length} stored sessions`);
if (!sessions.length) {
sessions = await this.migrate();
}
const refreshes = sessions.map(async session => {
Logger.trace(`Read the following stored session with scopes: ${session.scope}`);
const scopes = session.scope.split(' ');
@ -813,32 +808,6 @@ export class AzureActiveDirectoryService {
}
}
private async migrate() {
Logger.info('Attempting to migrate stored sessions.');
const migrated = this._context.globalState.get<{ migrated: boolean }>('microsoft-better-storage-layout-migrated');
if (migrated?.migrated) {
return [];
}
await this._context.globalState.update('microsoft-better-storage-layout-migrated', { migrated: true });
const keychain = new Keychain(this._context);
const storedData = await keychain.getToken();
if (!storedData) {
Logger.info('No stored sessions found.');
return [];
}
try {
const sessions = JSON.parse(storedData) as IStoredSession[];
Logger.info(`Migrated ${sessions.length} stored sessions.`);
return sessions;
} catch (e) {
Logger.info('Failed to parse stored sessions. Migrating no sessions.');
return [];
} finally {
await keychain.deleteToken();
}
}
//#endregion
//#region static methods

View file

@ -1,60 +0,0 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/
import * as vscode from 'vscode';
import Logger from './logger';
import * as nls from 'vscode-nls';
const localize = nls.loadMessageBundle();
const SERVICE_ID = `microsoft.login`;
export class Keychain {
constructor(private context: vscode.ExtensionContext) { }
async setToken(token: string): Promise<void> {
try {
return await this.context.secrets.store(SERVICE_ID, token);
} catch (e) {
Logger.error(`Setting token failed: ${e}`);
// Temporary fix for #94005
// This happens when processes write simulatenously to the keychain, most
// likely when trying to refresh the token. Ignore the error since additional
// writes after the first one do not matter. Should actually be fixed upstream.
if (e.message === 'The specified item already exists in the keychain.') {
return;
}
const troubleshooting = localize('troubleshooting', "Troubleshooting Guide");
const result = await vscode.window.showErrorMessage(localize('keychainWriteError', "Writing login information to the keychain failed with error '{0}'.", e.message), troubleshooting);
if (result === troubleshooting) {
vscode.env.openExternal(vscode.Uri.parse('https://code.visualstudio.com/docs/editor/settings-sync#_troubleshooting-keychain-issues'));
}
}
}
async getToken(): Promise<string | null | undefined> {
try {
return await this.context.secrets.get(SERVICE_ID);
} catch (e) {
// Ignore
Logger.error(`Getting token failed: ${e}`);
return Promise.resolve(undefined);
}
}
async deleteToken(): Promise<void> {
try {
return await this.context.secrets.delete(SERVICE_ID);
} catch (e) {
// Ignore
Logger.error(`Deleting token failed: ${e}`);
return Promise.resolve(undefined);
}
}
}

View file

@ -37,11 +37,22 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
public abstract getSecretStoragePrefix(): Promise<string>;
protected abstract withKeytar(): Promise<KeytarModule>;
/**
* An optional method that subclasses can implement to assist in surfacing
* Keytar load errors to the user in a friendly way.
*/
protected abstract surfaceKeytarLoadError?: (err: any) => void;
//#endregion
async getPassword(service: string, account: string): Promise<string | null> {
const keytar = await this.withKeytar();
let keytar: KeytarModule;
try {
keytar = await this.withKeytar();
} catch (e) {
// for get operations, we don't want to surface errors to the user
return null;
}
const password = await keytar.getPassword(service, account);
if (password) {
@ -70,7 +81,14 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
}
async setPassword(service: string, account: string, password: string): Promise<void> {
const keytar = await this.withKeytar();
let keytar: KeytarModule;
try {
keytar = await this.withKeytar();
} catch (e) {
this.surfaceKeytarLoadError?.(e);
throw e;
}
const MAX_SET_ATTEMPTS = 3;
// Sometimes Keytar has a problem talking to the keychain on the OS. To be more resilient, we retry a few times.
@ -119,7 +137,13 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
}
async deletePassword(service: string, account: string): Promise<boolean> {
const keytar = await this.withKeytar();
let keytar: KeytarModule;
try {
keytar = await this.withKeytar();
} catch (e) {
this.surfaceKeytarLoadError?.(e);
throw e;
}
const didDelete = await keytar.deletePassword(service, account);
if (didDelete) {
@ -130,13 +154,25 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
}
async findPassword(service: string): Promise<string | null> {
const keytar = await this.withKeytar();
let keytar: KeytarModule;
try {
keytar = await this.withKeytar();
} catch (e) {
// for get operations, we don't want to surface errors to the user
return null;
}
return keytar.findPassword(service);
}
async findCredentials(service: string): Promise<Array<{ account: string; password: string }>> {
const keytar = await this.withKeytar();
let keytar: KeytarModule;
try {
keytar = await this.withKeytar();
} catch (e) {
// for get operations, we don't want to surface errors to the user
return [];
}
return keytar.findCredentials(service);
}

View file

@ -36,14 +36,13 @@ export class CredentialsNativeMainService extends BaseCredentialsMainService {
return this._keytarCache;
}
try {
this._keytarCache = await import('keytar');
// Try using keytar to see if it throws or not.
await this._keytarCache.findCredentials('test-keytar-loads');
} catch (e) {
this.windowsMainService.sendToFocused('vscode:showCredentialsError', e.message ?? e);
throw e;
}
this._keytarCache = await import('keytar');
// Try using keytar to see if it throws or not.
await this._keytarCache.findCredentials('test-keytar-loads');
return this._keytarCache;
}
protected override surfaceKeytarLoadError = (err: any) => {
this.windowsMainService.sendToFocused('vscode:showCredentialsError', err.message ?? err);
};
}

View file

@ -10,6 +10,9 @@ import { IProductService } from 'vs/platform/product/common/productService';
import { BaseCredentialsMainService, KeytarModule } from 'vs/platform/credentials/common/credentialsMainService';
export class CredentialsWebMainService extends BaseCredentialsMainService {
// Since we fallback to the in-memory credentials provider, we do not need to surface any Keytar load errors
// to the user.
protected surfaceKeytarLoadError?: (err: any) => void;
constructor(
@ILogService logService: ILogService,