From 00cc269a36cc41dc4890e2525770d4bdfe439cf5 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Tue, 30 May 2023 09:39:30 +1000 Subject: [PATCH] Refactor RemoteURI storage and related --- src/gdpr.ts | 10 - .../jupyter/connection/jupyterConnection.ts | 50 ++- .../connection/jupyterConnection.unit.test.ts | 23 +- .../connection/jupyterPasswordConnect.ts | 7 +- .../jupyterPasswordConnect.unit.test.ts | 6 +- .../jupyter/connection/serverSelector.ts | 112 +----- .../jupyter/connection/serverUriStorage.ts | 339 ++++++++++++------ src/kernels/jupyter/jupyterUtils.ts | 27 +- .../jupyter/session/jupyterSessionManager.ts | 17 +- src/kernels/jupyter/types.ts | 3 +- .../common/application/encryptedStorage.ts | 16 +- src/platform/common/application/types.ts | 4 +- src/platform/common/constants.ts | 12 - src/platform/common/utils/localize.ts | 1 - .../userServerUrlProvider.ts | 331 ++++++++++------- src/telemetry.ts | 19 - src/test/datascience/mockEncryptedStorage.ts | 23 -- ...remoteNotebookEditor.vscode.common.test.ts | 44 ++- 18 files changed, 556 insertions(+), 488 deletions(-) delete mode 100644 src/test/datascience/mockEncryptedStorage.ts diff --git a/src/gdpr.ts b/src/gdpr.ts index 4b81ba00e7f7..3af15f441162 100644 --- a/src/gdpr.ts +++ b/src/gdpr.ts @@ -967,16 +967,6 @@ "${include}": [ "${F1}" - ] - } - */ -//Telemetry.SetJupyterURIToUserSpecified -/* __GDPR__ - "DATASCIENCE.SET_JUPYTER_URI_USER_SPECIFIED" : { - "azure": {"classification":"SystemMetaData","purpose":"FeatureInsight","comment":"Was the URI set to an Azure uri.","owner":"donjayamanne"}, - "${include}": [ - "${F1}" - ] } */ diff --git a/src/kernels/jupyter/connection/jupyterConnection.ts b/src/kernels/jupyter/connection/jupyterConnection.ts index d61cc01f8dd0..e22fa4519572 100644 --- a/src/kernels/jupyter/connection/jupyterConnection.ts +++ b/src/kernels/jupyter/connection/jupyterConnection.ts @@ -5,7 +5,7 @@ import { inject, injectable } from 'inversify'; import { noop } from '../../../platform/common/utils/misc'; import { RemoteJupyterServerUriProviderError } from '../../errors/remoteJupyterServerUriProviderError'; import { BaseError } from '../../../platform/errors/types'; -import { createRemoteConnectionInfo } from '../jupyterUtils'; +import { createRemoteConnectionInfo, handleExpiredCertsError, handleSelfCertsError } from '../jupyterUtils'; import { IJupyterServerUri, IJupyterServerUriStorage, @@ -14,6 +14,15 @@ import { IJupyterUriProviderRegistration, JupyterServerProviderHandle } from '../types'; +import { IDataScienceErrorHandler } from '../../errors/types'; +import { IApplicationShell } from '../../../platform/common/application/types'; +import { IConfigurationService } from '../../../platform/common/types'; +import { Telemetry, sendTelemetryEvent } from '../../../telemetry'; +import { JupyterSelfCertsExpiredError } from '../../../platform/errors/jupyterSelfCertsExpiredError'; +import { JupyterInvalidPasswordError } from '../../errors/jupyterInvalidPassword'; +import { RemoteJupyterServerConnectionError } from '../../../platform/errors/remoteJupyterServerConnectionError'; +import { traceError } from '../../../platform/logging'; +import { JupyterSelfCertsError } from '../../../platform/errors/jupyterSelfCertsError'; /** * Creates IJupyterConnection objects for URIs and 3rd party handles/ids. @@ -25,7 +34,11 @@ export class JupyterConnection { private readonly jupyterPickerRegistration: IJupyterUriProviderRegistration, @inject(IJupyterSessionManagerFactory) private readonly jupyterSessionManagerFactory: IJupyterSessionManagerFactory, - @inject(IJupyterServerUriStorage) private readonly serverUriStorage: IJupyterServerUriStorage + @inject(IJupyterServerUriStorage) private readonly serverUriStorage: IJupyterServerUriStorage, + @inject(IDataScienceErrorHandler) + private readonly errorHandler: IDataScienceErrorHandler, + @inject(IApplicationShell) private readonly applicationShell: IApplicationShell, + @inject(IConfigurationService) private readonly configService: IConfigurationService ) {} public async createConnectionInfo(serverHandle: JupyterServerProviderHandle) { @@ -37,9 +50,10 @@ export class JupyterConnection { return createRemoteConnectionInfo(serverHandle, serverUri); } - public async validateRemoteUri( + public async validateJupyterServer( serverHandle: JupyterServerProviderHandle, - serverUri?: IJupyterServerUri + serverUri?: IJupyterServerUri, + doNotDisplayUnActionableMessages?: boolean ): Promise { let sessionManager: IJupyterSessionManager | undefined = undefined; serverUri = serverUri || (await this.getJupyterServerUri(serverHandle)); @@ -50,6 +64,34 @@ export class JupyterConnection { sessionManager = await this.jupyterSessionManagerFactory.create(connection, false); await Promise.all([sessionManager.getRunningKernels(), sessionManager.getKernelSpecs()]); // We should throw an exception if any of that fails. + } catch (err) { + if (JupyterSelfCertsError.isSelfCertsError(err)) { + sendTelemetryEvent(Telemetry.ConnectRemoteSelfCertFailedJupyter); + const handled = await handleSelfCertsError(this.applicationShell, this.configService, err.message); + if (!handled) { + throw err; + } + } else if (JupyterSelfCertsExpiredError.isSelfCertsExpiredError(err)) { + sendTelemetryEvent(Telemetry.ConnectRemoteSelfCertFailedJupyter); + const handled = await handleExpiredCertsError(this.applicationShell, this.configService, err.message); + if (!handled) { + throw err; + } + } else if (err && err instanceof JupyterInvalidPasswordError) { + throw err; + } else if (serverUri && !doNotDisplayUnActionableMessages) { + await this.errorHandler.handleError( + new RemoteJupyterServerConnectionError(serverUri.baseUrl, serverHandle, err) + ); + // Can't set the URI in this case. + throw err; + } else { + traceError( + `Uri verification error ${serverHandle.extensionId}, id=${serverHandle.id}, handle=${serverHandle.handle}`, + err + ); + throw err; + } } finally { connection.dispose(); if (sessionManager) { diff --git a/src/kernels/jupyter/connection/jupyterConnection.unit.test.ts b/src/kernels/jupyter/connection/jupyterConnection.unit.test.ts index 6d1c4263cfcd..ea81243b0dad 100644 --- a/src/kernels/jupyter/connection/jupyterConnection.unit.test.ts +++ b/src/kernels/jupyter/connection/jupyterConnection.unit.test.ts @@ -32,6 +32,8 @@ import { ServiceContainer } from '../../../platform/ioc/container'; import { IServiceContainer } from '../../../platform/ioc/types'; import { JupyterConnectionWaiter } from '../launcher/jupyterConnectionWaiter.node'; import { noop } from '../../../test/core'; +import { IDataScienceErrorHandler } from '../../errors/types'; +import { IApplicationShell } from '../../../platform/common/application/types'; use(chaiAsPromised); suite('Jupyter Connection', async () => { let jupyterConnection: JupyterConnection; @@ -39,6 +41,9 @@ suite('Jupyter Connection', async () => { let sessionManagerFactory: IJupyterSessionManagerFactory; let sessionManager: IJupyterSessionManager; let serverUriStorage: IJupyterServerUriStorage; + let errorHandler: IDataScienceErrorHandler; + let applicationShell: IApplicationShell; + let configService: IConfigurationService; const disposables: IDisposable[] = []; const provider = { extensionId: 'ext', @@ -55,10 +60,16 @@ suite('Jupyter Connection', async () => { sessionManagerFactory = mock(); sessionManager = mock(); serverUriStorage = mock(); + errorHandler = mock(); + applicationShell = mock(); + configService = mock(); jupyterConnection = new JupyterConnection( instance(registrationPicker), instance(sessionManagerFactory), - instance(serverUriStorage) + instance(serverUriStorage), + instance(errorHandler), + instance(applicationShell), + instance(configService) ); (instance(sessionManager) as any).then = undefined; @@ -77,7 +88,7 @@ suite('Jupyter Connection', async () => { when(sessionManager.getKernelSpecs()).thenResolve([]); when(sessionManager.getRunningKernels()).thenResolve([]); - await jupyterConnection.validateRemoteUri(provider, server); + await jupyterConnection.validateJupyterServer(provider, server); verify(sessionManager.getKernelSpecs()).once(); verify(sessionManager.getRunningKernels()).once(); @@ -90,7 +101,7 @@ suite('Jupyter Connection', async () => { when(sessionManager.getRunningKernels()).thenResolve([]); when(registrationPicker.getJupyterServerUri(provider)).thenResolve(server); - await jupyterConnection.validateRemoteUri(provider); + await jupyterConnection.validateJupyterServer(provider); verify(sessionManager.getKernelSpecs()).once(); verify(sessionManager.getRunningKernels()).once(); @@ -103,7 +114,7 @@ suite('Jupyter Connection', async () => { when(sessionManager.getRunningKernels()).thenResolve([]); when(registrationPicker.getJupyterServerUri(anything())).thenReject(new Error('kaboom')); - await assert.isRejected(jupyterConnection.validateRemoteUri(provider)); + await assert.isRejected(jupyterConnection.validateJupyterServer(provider)); verify(sessionManager.getKernelSpecs()).never(); verify(sessionManager.getRunningKernels()).never(); @@ -115,7 +126,7 @@ suite('Jupyter Connection', async () => { when(sessionManager.getKernelSpecs()).thenResolve([]); when(sessionManager.getRunningKernels()).thenReject(new Error('Kaboom kernels failure')); - await assert.isRejected(jupyterConnection.validateRemoteUri(provider, server), 'Kaboom kernels failure'); + await assert.isRejected(jupyterConnection.validateJupyterServer(provider, server), 'Kaboom kernels failure'); verify(sessionManager.getKernelSpecs()).once(); verify(sessionManager.getRunningKernels()).once(); @@ -126,7 +137,7 @@ suite('Jupyter Connection', async () => { when(sessionManager.getKernelSpecs()).thenReject(new Error('Kaboom kernelspec failure')); when(sessionManager.getRunningKernels()).thenResolve([]); - await assert.isRejected(jupyterConnection.validateRemoteUri(provider, server), 'Kaboom kernelspec failure'); + await assert.isRejected(jupyterConnection.validateJupyterServer(provider, server), 'Kaboom kernelspec failure'); verify(sessionManager.getKernelSpecs()).once(); verify(sessionManager.getRunningKernels()).once(); diff --git a/src/kernels/jupyter/connection/jupyterPasswordConnect.ts b/src/kernels/jupyter/connection/jupyterPasswordConnect.ts index f34d78fe6ca3..3bb9f9ecf742 100644 --- a/src/kernels/jupyter/connection/jupyterPasswordConnect.ts +++ b/src/kernels/jupyter/connection/jupyterPasswordConnect.ts @@ -419,10 +419,9 @@ export class JupyterPasswordConnect implements IJupyterPasswordConnect { // Try once and see if it fails with unauthorized. try { - return await this.requestCreator.getFetchMethod()( - url, - this.addAllowUnauthorized(url, allowUnauthorized ? true : false, options) - ); + const requestInit = this.addAllowUnauthorized(url, allowUnauthorized ? true : false, options); + const result = await this.requestCreator.getFetchMethod()(url, requestInit); + return result; } catch (e) { if (e.message.indexOf('reason: self signed certificate') >= 0) { // Ask user to change setting and possibly try again. diff --git a/src/kernels/jupyter/connection/jupyterPasswordConnect.unit.test.ts b/src/kernels/jupyter/connection/jupyterPasswordConnect.unit.test.ts index 4d79806ae987..83b8139bef24 100644 --- a/src/kernels/jupyter/connection/jupyterPasswordConnect.unit.test.ts +++ b/src/kernels/jupyter/connection/jupyterPasswordConnect.unit.test.ts @@ -166,7 +166,7 @@ suite('JupyterPasswordConnect', () => { assert(result, 'Failed to get password'); if (result) { // eslint-disable-next-line - assert.ok((result.requestHeaders as any).Cookie, 'No cookie'); + assert.ok(result.requestHeaders?.Cookie, 'No cookie'); } // Verfiy calls @@ -224,7 +224,7 @@ suite('JupyterPasswordConnect', () => { assert(result, 'Failed to get password'); if (result) { // eslint-disable-next-line - assert.ok((result.requestHeaders as any).Cookie, 'No cookie'); + assert.ok(result.requestHeaders?.Cookie, 'No cookie'); } // Verfiy calls @@ -277,7 +277,7 @@ suite('JupyterPasswordConnect', () => { assert(result, 'Failed to get password'); if (result) { // eslint-disable-next-line - assert.ok((result.requestHeaders as any).Cookie, 'No cookie'); + assert.ok(result.requestHeaders?.Cookie, 'No cookie'); } // Verfiy calls diff --git a/src/kernels/jupyter/connection/serverSelector.ts b/src/kernels/jupyter/connection/serverSelector.ts index 8c91dbd6a543..b65b13a577d5 100644 --- a/src/kernels/jupyter/connection/serverSelector.ts +++ b/src/kernels/jupyter/connection/serverSelector.ts @@ -4,25 +4,9 @@ /* eslint-disable @typescript-eslint/no-use-before-define */ import { inject, injectable } from 'inversify'; -import { IApplicationShell, IWorkspaceService } from '../../../platform/common/application/types'; -import { traceError, traceWarning } from '../../../platform/logging'; -import { DataScience } from '../../../platform/common/utils/localize'; -import { sendTelemetryEvent } from '../../../telemetry'; -import { Telemetry } from '../../../telemetry'; -import { - IJupyterServerUri, - IJupyterServerUriStorage, - IJupyterUriProviderRegistration, - JupyterServerProviderHandle -} from '../types'; -import { IDataScienceErrorHandler } from '../../errors/types'; -import { IConfigurationService, IDisposableRegistry } from '../../../platform/common/types'; -import { handleExpiredCertsError, handleSelfCertsError, jupyterServerHandleToString } from '../jupyterUtils'; +import { IJupyterServerUriStorage, JupyterServerProviderHandle } from '../types'; import { JupyterConnection } from './jupyterConnection'; -import { JupyterSelfCertsError } from '../../../platform/errors/jupyterSelfCertsError'; -import { RemoteJupyterServerConnectionError } from '../../../platform/errors/remoteJupyterServerConnectionError'; -import { JupyterSelfCertsExpiredError } from '../../../platform/errors/jupyterSelfCertsExpiredError'; -import { JupyterInvalidPasswordError } from '../../errors/jupyterInvalidPassword'; +import { traceError } from '../../../platform/logging'; export type SelectJupyterUriCommandSource = | 'nonUser' @@ -33,47 +17,6 @@ export type SelectJupyterUriCommandSource = | 'errorHandler' | 'prompt'; -export async function validateSelectJupyterURI( - jupyterConnection: JupyterConnection, - applicationShell: IApplicationShell, - configService: IConfigurationService, - isWebExtension: boolean, - serverHandle: JupyterServerProviderHandle, - serverUri: IJupyterServerUri -): Promise { - // Double check this server can be connected to. Might need a password, might need a allowUnauthorized - try { - await jupyterConnection.validateRemoteUri(serverHandle, serverUri); - } catch (err) { - traceWarning('Uri verification error', err); - if (JupyterSelfCertsError.isSelfCertsError(err)) { - sendTelemetryEvent(Telemetry.ConnectRemoteSelfCertFailedJupyter); - const handled = await handleSelfCertsError(applicationShell, configService, err.message); - if (!handled) { - return DataScience.jupyterSelfCertFailErrorMessageOnly; - } - } else if (JupyterSelfCertsExpiredError.isSelfCertsExpiredError(err)) { - sendTelemetryEvent(Telemetry.ConnectRemoteSelfCertFailedJupyter); - const handled = await handleExpiredCertsError(applicationShell, configService, err.message); - if (!handled) { - return DataScience.jupyterSelfCertExpiredErrorMessageOnly; - } - } else if (err && err instanceof JupyterInvalidPasswordError) { - return DataScience.passwordFailure; - } else { - // Return the general connection error to show in the validation box - // Replace any Urls in the error message with markdown link. - const urlRegex = /(https?:\/\/[^\s]+)/g; - const errorMessage = (err.message || err.toString()).replace(urlRegex, (url: string) => `[${url}](${url})`); - return ( - isWebExtension || true - ? DataScience.remoteJupyterConnectionFailedWithoutServerWithErrorWeb - : DataScience.remoteJupyterConnectionFailedWithoutServerWithError - )(errorMessage); - } - } -} - /** * Provides the UI for picking a remote server. Multiplexes to one of two implementations based on the 'showOnlyOneTypeOfKernel' experiment. */ @@ -81,58 +24,17 @@ export async function validateSelectJupyterURI( export class JupyterServerSelector { constructor( @inject(IJupyterServerUriStorage) private readonly serverUriStorage: IJupyterServerUriStorage, - @inject(IDataScienceErrorHandler) - private readonly errorHandler: IDataScienceErrorHandler, - @inject(IApplicationShell) private readonly applicationShell: IApplicationShell, - @inject(IConfigurationService) private readonly configService: IConfigurationService, - @inject(JupyterConnection) private readonly jupyterConnection: JupyterConnection, - @inject(IWorkspaceService) readonly workspaceService: IWorkspaceService, - @inject(IDisposableRegistry) readonly disposableRegistry: IDisposableRegistry, - @inject(IJupyterUriProviderRegistration) - private readonly jupyterPickerRegistration: IJupyterUriProviderRegistration + @inject(JupyterConnection) private readonly jupyterConnection: JupyterConnection ) {} public async addJupyterServer(serverHandle: JupyterServerProviderHandle): Promise { - const serverHandleId = jupyterServerHandleToString(serverHandle); - // Double check this server can be connected to. Might need a password, might need a allowUnauthorized - let serverUri: undefined | IJupyterServerUri; try { - serverUri = await this.jupyterPickerRegistration.getJupyterServerUri(serverHandle); - await this.jupyterConnection.validateRemoteUri(serverHandle); + // Double check this server can be connected to. Might need a password, might need a allowUnauthorized + await this.jupyterConnection.validateJupyterServer(serverHandle); } catch (err) { - if (JupyterSelfCertsError.isSelfCertsError(err)) { - sendTelemetryEvent(Telemetry.ConnectRemoteSelfCertFailedJupyter); - const handled = await handleSelfCertsError(this.applicationShell, this.configService, err.message); - if (!handled) { - return; - } - } else if (JupyterSelfCertsExpiredError.isSelfCertsExpiredError(err)) { - sendTelemetryEvent(Telemetry.ConnectRemoteSelfCertFailedJupyter); - const handled = await handleExpiredCertsError(this.applicationShell, this.configService, err.message); - if (!handled) { - return; - } - } else if (err && err instanceof JupyterInvalidPasswordError) { - return; - } else if (serverUri) { - await this.errorHandler.handleError( - new RemoteJupyterServerConnectionError(serverUri.baseUrl, serverHandle, err) - ); - // Can't set the URI in this case. - return; - } else { - traceError( - `Uri verification error ${serverHandle.extensionId}, id=${serverHandle.id}, handle=${serverHandle.handle}`, - err - ); - } + traceError(`Error in validating the Remote Uri ${serverHandle.id}.${serverHandle.handle}`, err); + return; } - await this.serverUriStorage.add(serverHandle); - - // Indicate setting a jupyter URI to a remote setting. Check if an azure remote or not - sendTelemetryEvent(Telemetry.SetJupyterURIToUserSpecified, undefined, { - azure: serverHandleId.toLowerCase().includes('azure') - }); } } diff --git a/src/kernels/jupyter/connection/serverUriStorage.ts b/src/kernels/jupyter/connection/serverUriStorage.ts index ecf1d23d2405..4d89470aa5c9 100644 --- a/src/kernels/jupyter/connection/serverUriStorage.ts +++ b/src/kernels/jupyter/connection/serverUriStorage.ts @@ -1,12 +1,12 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -import { EventEmitter, Memento } from 'vscode'; +import { EventEmitter, Memento, Uri } from 'vscode'; import { inject, injectable, named } from 'inversify'; import { IEncryptedStorage } from '../../../platform/common/application/types'; -import { Settings } from '../../../platform/common/constants'; -import { IMemento, GLOBAL_MEMENTO } from '../../../platform/common/types'; -import { traceError, traceInfoIfCI, traceVerbose } from '../../../platform/logging'; +import { JVSC_EXTENSION_ID, Settings } from '../../../platform/common/constants'; +import { IMemento, GLOBAL_MEMENTO, IExtensionContext, IDisposableRegistry } from '../../../platform/common/types'; +import { traceError, traceInfoIfCI, traceVerbose, traceWarning } from '../../../platform/logging'; import { jupyterServerHandleFromString, jupyterServerHandleToString } from '../jupyterUtils'; import { IJupyterServerUriEntry, @@ -14,12 +14,28 @@ import { IJupyterUriProviderRegistration, JupyterServerProviderHandle } from '../types'; +import * as path from '../../../platform/vscode-path/resources'; +import { IFileSystem } from '../../../platform/common/platform/types'; +import { Disposables } from '../../../platform/common/utils'; + +const MAX_MRU_COUNT = 10; +const JupyterServerRemoteLaunchUriListKey = 'remote-uri-list'; + +type StorageMRUItem = { + displayName: string; + time: number; + serverHandle: JupyterServerProviderHandle; +}; +const JupyterServerUriList = 'jupyter.jupyterServer.uriList'; +const JupyterServerLocalLaunch = 'local'; +const JupyterServerRemoteLaunchUriEqualsDisplayName = 'same'; +const JupyterServerRemoteLaunchNameSeparator = '\n'; /** * Class for storing Jupyter Server URI values, also manages the MRU list of the servers/urls. */ @injectable() -export class JupyterServerUriStorage implements IJupyterServerUriStorage { +export class JupyterServerUriStorage extends Disposables implements IJupyterServerUriStorage { private _onDidChangeUri = new EventEmitter(); public get onDidChange() { return this._onDidChangeUri.event; @@ -32,108 +48,230 @@ export class JupyterServerUriStorage implements IJupyterServerUriStorage { public get onDidAdd() { return this._onDidAddUri.event; } + private readonly migration: MigrateOldMRU; + private readonly storageFile: Uri; constructor( @inject(IEncryptedStorage) private readonly encryptedStorage: IEncryptedStorage, @inject(IMemento) @named(GLOBAL_MEMENTO) private readonly globalMemento: Memento, @inject(IJupyterUriProviderRegistration) - private readonly jupyterPickerRegistration: IJupyterUriProviderRegistration - ) {} + private readonly jupyterPickerRegistration: IJupyterUriProviderRegistration, + @inject(IExtensionContext) private readonly context: IExtensionContext, + @inject(IFileSystem) private readonly fs: IFileSystem, + @inject(IDisposableRegistry) disposables: IDisposableRegistry + ) { + super(); + disposables.push(this); + this.disposables.push(this._onDidAddUri); + this.disposables.push(this._onDidChangeUri); + this.disposables.push(this._onDidRemoveUris); + this._onDidRemoveUris.event( + (e) => this.removeHandles(e.map((item) => item.serverHandle)), + this, + this.disposables + ); + this.storageFile = Uri.joinPath(this.context.globalStorageUri, 'remoteServersMRUList.json'); + // eslint-disable-next-line @typescript-eslint/no-use-before-define + this.migration = new MigrateOldMRU(this.encryptedStorage, this.globalMemento, this.fs, this.storageFile); + } public async update(serverHandle: JupyterServerProviderHandle) { + await this.migration.migrateMRU(); + await this.add(serverHandle); + this._onDidChangeUri.fire(); + } + public async remove(serverHandle: JupyterServerProviderHandle) { + await this.migration.migrateMRU(); const uriList = await this.getAll(); const serverHandleId = jupyterServerHandleToString(serverHandle); - const existingEntry = uriList.find( - (entry) => jupyterServerHandleToString(entry.serverHandle) === serverHandleId - ); - if (!existingEntry) { - throw new Error(`Uri not found for Server Id ${serverHandleId}`); + const newList = uriList.filter((f) => jupyterServerHandleToString(f.serverHandle) !== serverHandleId); + const removedItem = uriList.find((f) => jupyterServerHandleToString(f.serverHandle) === serverHandleId); + if (removedItem) { + await this.updateStore( + newList.map( + (item) => + { + displayName: item.displayName, + time: item.time, + serverHandle: item.serverHandle + } + ) + ); + this._onDidRemoveUris.fire([removedItem]); + this._onDidChangeUri.fire(); } - - await this.addToUriList(existingEntry.serverHandle, existingEntry.displayName || ''); } - private async addToUriList(serverHandle: JupyterServerProviderHandle, displayName: string) { - const serverHandleId = jupyterServerHandleToString(serverHandle); - const uriList = await this.getAll(); - - // Check if we have already found a display name for this server - displayName = - uriList.find((entry) => jupyterServerHandleToString(entry.serverHandle) === serverHandleId)?.displayName || - displayName || - serverHandleId; + public async getAll(): Promise { + await this.migration.migrateMRU(); + let items: StorageMRUItem[] = []; + if (await this.fs.exists(this.storageFile)) { + items = JSON.parse(await this.fs.readFile(this.storageFile)) as StorageMRUItem[]; + } else { + return []; + } + const result = await Promise.all( + items.map(async (item) => { + // This can fail if the URI is invalid + const server: IJupyterServerUriEntry = { + time: item.time, + displayName: item.displayName, + isValidated: true, + serverHandle: item.serverHandle + }; - // Remove this uri if already found (going to add again with a new time) - const editedList = uriList.filter( - (f, i) => - jupyterServerHandleToString(f.serverHandle) !== serverHandleId && - i < Settings.JupyterServerUriListMax - 1 + try { + const info = await this.jupyterPickerRegistration.getJupyterServerUri(item.serverHandle); + server.displayName = info.displayName || server.displayName || new URL(info.baseUrl).hostname; + return server; + } catch (ex) { + server.isValidated = false; + return server; + } + }) ); - // Add this entry into the last. + traceVerbose(`Found ${result.length} saved URIs, ${JSON.stringify(result)}`); + return result; + } + public async clear(): Promise { + const uriList = await this.getAll(); + await this.updateStore([]); + // Notify out that we've removed the list to clean up controller entries, passwords, ect + this._onDidRemoveUris.fire(uriList); + this._onDidChangeUri.fire(); + } + public async get(serverHandle: JupyterServerProviderHandle): Promise { + await this.migration.migrateMRU(); + const savedList = await this.getAll(); + const serverHandleId = jupyterServerHandleToString(serverHandle); + return savedList.find((item) => jupyterServerHandleToString(item.serverHandle) === serverHandleId); + } + public async add(serverHandle: JupyterServerProviderHandle): Promise { + await this.migration.migrateMRU(); + traceInfoIfCI(`setUri: ${serverHandle.id}.${serverHandle.handle}`); + const server = await this.jupyterPickerRegistration.getJupyterServerUri(serverHandle); + let uriList = await this.getAll(); + const id = jupyterServerHandleToString(serverHandle); + const storageItems = uriList + .filter((item) => jupyterServerHandleToString(item.serverHandle) !== id) + .map( + (item) => + { + displayName: item.displayName, + serverHandle, + time: item.time + } + ); const entry: IJupyterServerUriEntry = { - time: Date.now(), serverHandle, - displayName, + time: Date.now(), + displayName: server.displayName, isValidated: true }; - editedList.push(entry); - - // Signal that we added in the entry - await this.updateMemento(editedList); + storageItems.push({ + serverHandle, + time: entry.time, + displayName: server.displayName + }); + await this.updateStore(storageItems); + this._onDidChangeUri.fire(); this._onDidAddUri.fire(entry); - this._onDidChangeUri.fire(); // Needs to happen as soon as we change so that dependencies update synchronously } - public async remove(serverHandle: JupyterServerProviderHandle) { - const uriList = await this.getAll(); - const serverHandleId = jupyterServerHandleToString(serverHandle); - await this.updateMemento(uriList.filter((f) => jupyterServerHandleToString(f.serverHandle) !== serverHandleId)); - const removedItem = uriList.find((f) => jupyterServerHandleToString(f.serverHandle) === serverHandleId); - if (removedItem) { - this._onDidRemoveUris.fire([removedItem]); + /** + * If we're no longer in a handle, then notify the jupyter uri providers as well. + * This will allow them to clean up any state they have. + * E.g. in the case of User userServerUriProvider.ts, we need to clear the old server list + * if the corresponding entry is removed from MRU. + */ + private async removeHandles(serverHandles: JupyterServerProviderHandle[]) { + for (const handle of serverHandles) { + try { + const provider = await this.jupyterPickerRegistration.getProvider(handle.id); + if (provider?.removeHandle) { + await provider.removeHandle(handle.handle); + } + } catch (ex) { + traceWarning(`Failed to get provider for ${handle.id} to delete handle ${handle.handle}`, ex); + } } } - private async updateMemento(editedList: IJupyterServerUriEntry[]) { - // Sort based on time. Newest time first - const sorted = editedList - .sort((a, b) => b.time - a.time) - // We have may stored some old bogus entries in the past. - .filter((item) => item.uri !== Settings.JupyterServerLocalLaunch); - - // Transform the sorted into just indexes. Uris can't show up in - // non encrypted storage (so remove even the display name) - const mementoList = sorted.map((v, i) => { - return { index: i, time: v.time }; - }); + private async updateStore(items: StorageMRUItem[]) { + const itemsToSave = items.slice(0, MAX_MRU_COUNT - 1); + const itemsToRemove = items.slice(MAX_MRU_COUNT); + const dir = path.dirname(this.storageFile); + if (!(await this.fs.exists(dir))) { + await this.fs.createDirectory(dir); + } + await this.fs.writeFile(this.storageFile, JSON.stringify(itemsToSave)); - // Write the uris to the storage in one big blob (max length issues?) - // This is because any part of the URI may be a secret (we don't know it's just token values for instance) - const blob = sorted - .map( - (e) => - `${jupyterServerHandleToString(e.serverHandle)}${Settings.JupyterServerRemoteLaunchNameSeparator}${ - !e.displayName || e.displayName === e.uri - ? Settings.JupyterServerRemoteLaunchUriEqualsDisplayName - : e.displayName - }` + // This is required so the individual publishers of JupyterUris can clean up their state + // I.e. they need to know that these handles are no longer saved in MRU, so they too can clean their state. + this._onDidRemoveUris.fire( + itemsToRemove.map( + (item) => + { + serverHandle: item.serverHandle, + time: item.time, + displayName: item.displayName, + isValidated: false + } ) - .join(Settings.JupyterServerRemoteLaunchUriSeparator); + ); + } +} + +class MigrateOldMRU { + private migration: Promise | undefined; + constructor( + private readonly encryptedStorage: IEncryptedStorage, + private readonly globalMemento: Memento, + private readonly fs: IFileSystem, + private readonly storageFile: Uri + ) {} + async migrateMRU() { + if (!this.migration) { + this.migration = this.migrateMRUImpl(); + } + return this.migration; + } + private async migrateMRUImpl() { + // Do not store the fact that we migrated in memento, + // we do not want such state to be transferred across machines. + if (await this.fs.exists(this.storageFile)) { + return; + } + const items = await this.getMRU(); + if (items.length === 0) { + return; + } + const dir = path.dirname(this.storageFile); + if (!(await this.fs.exists(dir))) { + await this.fs.createDirectory(dir); + } + const storageItems = items.map( + (item) => + { + serverHandle: item.serverHandle, + displayName: item.displayName || '', + time: item.time + } + ); + await Promise.all([this.clear(), this.fs.writeFile(this.storageFile, JSON.stringify(storageItems))]); + } + private async clear(): Promise { await Promise.all([ - this.globalMemento.update(Settings.JupyterServerUriList, mementoList), - this.encryptedStorage.store( - Settings.JupyterServerRemoteLaunchService, - Settings.JupyterServerRemoteLaunchUriListKey, - blob - ) + this.globalMemento.update(JupyterServerUriList, []), + this.encryptedStorage.store(`${JVSC_EXTENSION_ID}.${JupyterServerRemoteLaunchUriListKey}`, undefined) ]); } - public async getAll(): Promise { + + private async getMRU() { // List is in the global memento, URIs are in encrypted storage - const indexes = this.globalMemento.get<{ index: number; time: number }[]>(Settings.JupyterServerUriList); + const indexes = this.globalMemento.get<{ index: number; time: number }[]>(JupyterServerUriList); if (!Array.isArray(indexes) || indexes.length === 0) { return []; } // Pull out the \r separated URI list (\r is an invalid URI character) const blob = await this.encryptedStorage.retrieve( - Settings.JupyterServerRemoteLaunchService, - Settings.JupyterServerRemoteLaunchUriListKey + `${JVSC_EXTENSION_ID}.${JupyterServerRemoteLaunchUriListKey}` ); if (!blob) { return []; @@ -141,11 +279,11 @@ export class JupyterServerUriStorage implements IJupyterServerUriStorage { // Make sure same length const split = blob.split(Settings.JupyterServerRemoteLaunchUriSeparator); const result = await Promise.all( - split.slice(0, Math.min(split.length, indexes.length)).map(async (item, index) => { - const uriAndDisplayName = item.split(Settings.JupyterServerRemoteLaunchNameSeparator); + split.map(async (item, index) => { + const uriAndDisplayName = item.split(JupyterServerRemoteLaunchNameSeparator); const uri = uriAndDisplayName[0]; // Old code (we may have stored a bogus url in the past). - if (uri === Settings.JupyterServerLocalLaunch) { + if (uri === JupyterServerLocalLaunch) { return; } @@ -154,24 +292,15 @@ export class JupyterServerUriStorage implements IJupyterServerUriStorage { const serverHandle = jupyterServerHandleFromString(uri); // 'same' is specified for the display name to keep storage shorter if it is the same value as the URI const displayName = - uriAndDisplayName[1] === Settings.JupyterServerRemoteLaunchUriEqualsDisplayName || - !uriAndDisplayName[1] + uriAndDisplayName[1] === JupyterServerRemoteLaunchUriEqualsDisplayName || !uriAndDisplayName[1] ? uri : uriAndDisplayName[1]; - const server: IJupyterServerUriEntry = { - time: indexes[index].time, + return { + time: indexes[index].time, // Assumption is that during retrieval, indexes and blob will be in sync. displayName, - isValidated: true, + isValidated: false, serverHandle }; - - try { - await this.jupyterPickerRegistration.getJupyterServerUri(serverHandle); - return server; - } catch (ex) { - server.isValidated = false; - return server; - } } catch (ex) { // traceError(`Failed to parse URI ${item}: `, ex); @@ -182,30 +311,4 @@ export class JupyterServerUriStorage implements IJupyterServerUriStorage { traceVerbose(`Found ${result.length} saved URIs, ${JSON.stringify(result)}`); return result.filter((item) => !!item) as IJupyterServerUriEntry[]; } - - public async clear(): Promise { - const uriList = await this.getAll(); - // Clear out memento and encrypted storage - await this.globalMemento.update(Settings.JupyterServerUriList, []); - await this.encryptedStorage.store( - Settings.JupyterServerRemoteLaunchService, - Settings.JupyterServerRemoteLaunchUriListKey, - undefined - ); - - // Notify out that we've removed the list to clean up controller entries, passwords, ect - this._onDidRemoveUris.fire(uriList); - } - public async get(serverHandle: JupyterServerProviderHandle): Promise { - const savedList = await this.getAll(); - const serverHandleId = jupyterServerHandleToString(serverHandle); - return savedList.find((item) => jupyterServerHandleToString(item.serverHandle) === serverHandleId); - } - public async add(serverHandle: JupyterServerProviderHandle): Promise { - traceInfoIfCI(`setUri: ${serverHandle.id}.${serverHandle.handle}`); - const server = await this.jupyterPickerRegistration.getJupyterServerUri(serverHandle); - - // display name is wrong here - await this.addToUriList(serverHandle, server.displayName); - } } diff --git a/src/kernels/jupyter/jupyterUtils.ts b/src/kernels/jupyter/jupyterUtils.ts index ff5a170fc849..dfa301c1c65b 100644 --- a/src/kernels/jupyter/jupyterUtils.ts +++ b/src/kernels/jupyter/jupyterUtils.ts @@ -12,7 +12,7 @@ import { IConfigurationService, IWatchableJupyterSettings, Resource } from '../. import { getFilePath } from '../../platform/common/platform/fs-paths'; import { DataScience } from '../../platform/common/utils/localize'; import { sendTelemetryEvent } from '../../telemetry'; -import { Identifiers, JVSC_EXTENSION_ID, Telemetry } from '../../platform/common/constants'; +import { JVSC_EXTENSION_ID, Telemetry } from '../../platform/common/constants'; import { traceError } from '../../platform/logging'; import { computeHash } from '../../platform/common/crypto'; @@ -129,20 +129,23 @@ export async function computeServerId(serverHandle: JupyterServerProviderHandle) } const OLD_EXTENSION_ID_THAT_DID_NOT_HAVE_EXT_ID_IN_URL = ['ms-toolsai.jupyter', 'ms-toolsai.vscode-ai']; +const REMOTE_URI = 'https://remote/'; +const REMOTE_URI_ID_PARAM = 'id'; +const REMOTE_URI_HANDLE_PARAM = 'uriHandle'; +const REMOTE_URI_EXTENSION_ID_PARAM = 'extensionId'; + export function jupyterServerHandleToString(serverHandle: JupyterServerProviderHandle) { if (OLD_EXTENSION_ID_THAT_DID_NOT_HAVE_EXT_ID_IN_URL.includes(serverHandle.extensionId)) { // Jupyter extension and AzML extension did not have extension id in the generated Id. // Hence lets not store them in the future as well, however // for all other extensions we will (it will only break the MRU for a few set of users using other extensions that contribute Jupyter servers via Jupyter extension). - return `${Identifiers.REMOTE_URI}?${Identifiers.REMOTE_URI_ID_PARAM}=${serverHandle.id}&${ - Identifiers.REMOTE_URI_HANDLE_PARAM - }=${encodeURI(serverHandle.handle)}`; + return `${REMOTE_URI}?${REMOTE_URI_ID_PARAM}=${serverHandle.id}&${REMOTE_URI_HANDLE_PARAM}=${encodeURI( + serverHandle.handle + )}`; } - return `${Identifiers.REMOTE_URI}?${Identifiers.REMOTE_URI_ID_PARAM}=${serverHandle.id}&${ - Identifiers.REMOTE_URI_HANDLE_PARAM - }=${encodeURI(serverHandle.handle)}&${Identifiers.REMOTE_URI_EXTENSION_ID_PARAM}=${encodeURI( - serverHandle.extensionId - )}`; + return `${REMOTE_URI}?${REMOTE_URI_ID_PARAM}=${serverHandle.id}&${REMOTE_URI_HANDLE_PARAM}=${encodeURI( + serverHandle.handle + )}&${REMOTE_URI_EXTENSION_ID_PARAM}=${encodeURI(serverHandle.extensionId)}`; } export function jupyterServerHandleFromString(serverHandleId: string): JupyterServerProviderHandle { @@ -150,9 +153,9 @@ export function jupyterServerHandleFromString(serverHandleId: string): JupyterSe const url: URL = new URL(serverHandleId); // Id has to be there too. - const id = url.searchParams.get(Identifiers.REMOTE_URI_ID_PARAM) || ''; - const uriHandle = url.searchParams.get(Identifiers.REMOTE_URI_HANDLE_PARAM); - let extensionId = url.searchParams.get(Identifiers.REMOTE_URI_EXTENSION_ID_PARAM); + const id = url.searchParams.get(REMOTE_URI_ID_PARAM) || ''; + const uriHandle = url.searchParams.get(REMOTE_URI_HANDLE_PARAM); + let extensionId = url.searchParams.get(REMOTE_URI_EXTENSION_ID_PARAM); extensionId = extensionId || // We know the extension ids for some of the providers. diff --git a/src/kernels/jupyter/session/jupyterSessionManager.ts b/src/kernels/jupyter/session/jupyterSessionManager.ts index 3b66206eb783..e5414e61f8b8 100644 --- a/src/kernels/jupyter/session/jupyterSessionManager.ts +++ b/src/kernels/jupyter/session/jupyterSessionManager.ts @@ -20,7 +20,8 @@ import { IPersistentStateFactory, Resource, IDisplayOptions, - IDisposable + IDisposable, + ReadWrite } from '../../../platform/common/types'; import { Common, DataScience } from '../../../platform/common/utils/localize'; import { SessionDisposedError } from '../../../platform/errors/sessionDisposedError'; @@ -364,19 +365,23 @@ export class JupyterSessionManager implements IJupyterSessionManager { serverSettings = { ...serverSettings, token: '' }; const pwSettings = await this.jupyterPasswordConnect.getPasswordConnectionInfo({ url: connInfo.baseUrl, - isTokenEmpty + isTokenEmpty, + serverHandle: connInfo.serverHandle }); if (pwSettings && pwSettings.requestHeaders) { requestInit = { ...requestInit, headers: pwSettings.requestHeaders }; - cookieString = (pwSettings.requestHeaders as any).Cookie || ''; + cookieString = pwSettings.requestHeaders.Cookie || ''; // Password may have overwritten the base url and token as well if (pwSettings.remappedBaseUrl) { - (serverSettings as any).baseUrl = pwSettings.remappedBaseUrl; - (serverSettings as any).wsUrl = pwSettings.remappedBaseUrl.replace('http', 'ws'); + (serverSettings as ReadWrite).baseUrl = pwSettings.remappedBaseUrl; + (serverSettings as ReadWrite).wsUrl = pwSettings.remappedBaseUrl.replace( + 'http', + 'ws' + ); } if (pwSettings.remappedToken) { - (serverSettings as any).token = pwSettings.remappedToken; + (serverSettings as ReadWrite).token = pwSettings.remappedToken; } } else if (pwSettings) { serverSettings = { ...serverSettings, token: '' }; diff --git a/src/kernels/jupyter/types.ts b/src/kernels/jupyter/types.ts index c414bb5f8b59..31146290e586 100644 --- a/src/kernels/jupyter/types.ts +++ b/src/kernels/jupyter/types.ts @@ -54,7 +54,7 @@ export interface IJupyterServerHelper extends IAsyncDisposable { } export interface IJupyterPasswordConnectInfo { - requestHeaders?: HeadersInit; + requestHeaders?: Record; remappedBaseUrl?: string; remappedToken?: string; } @@ -64,6 +64,7 @@ export interface IJupyterPasswordConnect { getPasswordConnectionInfo(options: { url: string; isTokenEmpty: boolean; + serverHandle: JupyterServerProviderHandle; }): Promise; } diff --git a/src/platform/common/application/encryptedStorage.ts b/src/platform/common/application/encryptedStorage.ts index 8442079bc909..9ec7c2543e1d 100644 --- a/src/platform/common/application/encryptedStorage.ts +++ b/src/platform/common/application/encryptedStorage.ts @@ -18,36 +18,36 @@ export class EncryptedStorage implements IEncryptedStorage { private readonly testingState = new Map(); - public async store(service: string, key: string, value: string | undefined): Promise { + public async store(key: string, value: string | undefined): Promise { // On CI we don't need to use keytar for testing (else it hangs). if (isCI && this.extensionContext.extensionMode !== ExtensionMode.Production) { - this.testingState.set(`${service}#${key}`, value || ''); + this.testingState.set(key, value || ''); return; } if (!value) { try { - await this.extensionContext.secrets.delete(`${service}.${key}`); + await this.extensionContext.secrets.delete(key); } catch (e) { traceError(e); } } else { - await this.extensionContext.secrets.store(`${service}.${key}`, value); + await this.extensionContext.secrets.store(key, value); } } - public async retrieve(service: string, key: string): Promise { + public async retrieve(key: string): Promise { // On CI we don't need to use keytar for testing (else it hangs). if (isCI && this.extensionContext.extensionMode !== ExtensionMode.Production) { - return this.testingState.get(`${service}#${key}`); + return this.testingState.get(key); } try { // eslint-disable-next-line - const val = await this.extensionContext.secrets.get(`${service}.${key}`); + const val = await this.extensionContext.secrets.get(key); return val; } catch (e) { // If we get an error trying to get a secret, it might be corrupted. So we delete it. try { - await this.extensionContext.secrets.delete(`${service}.${key}`); + await this.extensionContext.secrets.delete(key); return; } catch (e) { traceError(e); diff --git a/src/platform/common/application/types.ts b/src/platform/common/application/types.ts index aaff83ad795d..613974589353 100644 --- a/src/platform/common/application/types.ts +++ b/src/platform/common/application/types.ts @@ -1250,6 +1250,6 @@ export interface IVSCodeNotebook { export const IEncryptedStorage = Symbol('IEncryptedStorage'); export interface IEncryptedStorage { - store(service: string, key: string, value: string | undefined): Promise; - retrieve(service: string, key: string): Promise; + store(key: string, value: string | undefined): Promise; + retrieve(key: string): Promise; } diff --git a/src/platform/common/constants.ts b/src/platform/common/constants.ts index 992c27e47091..293261d48b8f 100644 --- a/src/platform/common/constants.ts +++ b/src/platform/common/constants.ts @@ -33,15 +33,7 @@ export namespace HelpLinks { } export namespace Settings { - export const JupyterServerLocalLaunch = 'local'; - export const JupyterServerRemoteLaunch = 'remote'; - export const JupyterServerUriList = 'jupyter.jupyterServer.uriList'; - export const JupyterServerRemoteLaunchUriListKey = 'remote-uri-list'; export const JupyterServerRemoteLaunchUriSeparator = '\r'; - export const JupyterServerRemoteLaunchNameSeparator = '\n'; - export const JupyterServerRemoteLaunchUriEqualsDisplayName = 'same'; - export const JupyterServerRemoteLaunchService = JVSC_EXTENSION_ID; - export const JupyterServerUriListMax = 10; // If this timeout expires, ignore the completion request sent to Jupyter. export const IntellisenseTimeout = 2000; } @@ -86,10 +78,6 @@ export namespace Identifiers { export const PYTHON_VARIABLES_REQUESTER = 'PYTHON_VARIABLES_REQUESTER'; export const MULTIPLEXING_DEBUGSERVICE = 'MULTIPLEXING_DEBUGSERVICE'; export const RUN_BY_LINE_DEBUGSERVICE = 'RUN_BY_LINE_DEBUGSERVICE'; - export const REMOTE_URI = 'https://remote/'; - export const REMOTE_URI_ID_PARAM = 'id'; - export const REMOTE_URI_HANDLE_PARAM = 'uriHandle'; - export const REMOTE_URI_EXTENSION_ID_PARAM = 'extensionId'; } export namespace CodeSnippets { diff --git a/src/platform/common/utils/localize.ts b/src/platform/common/utils/localize.ts index c5939b92774b..75872a51a4f1 100644 --- a/src/platform/common/utils/localize.ts +++ b/src/platform/common/utils/localize.ts @@ -700,7 +700,6 @@ export namespace DataScience { export const localPythonEnvironments = l10n.t('Python Environments...'); export const UserJupyterServerUrlProviderDisplayName = l10n.t('Existing Jupyter Server...'); export const UserJupyterServerUrlProviderDetail = l10n.t('Connect to an existing Jupyter Server'); - export const UserJupyterServerUrlAlreadyExistError = l10n.t('A Jupyter Server with this URL already exists'); export const kernelPickerSelectKernelTitle = l10n.t('Select Kernel'); export const kernelPickerSelectLocalKernelSpecTitle = l10n.t('Select a Jupyter Kernel'); export const kernelPickerSelectPythonEnvironmentTitle = l10n.t('Select a Python Environment'); diff --git a/src/standalone/userJupyterServer/userServerUrlProvider.ts b/src/standalone/userJupyterServer/userServerUrlProvider.ts index fb9b8e06aa48..a4432a034ec6 100644 --- a/src/standalone/userJupyterServer/userServerUrlProvider.ts +++ b/src/standalone/userJupyterServer/userServerUrlProvider.ts @@ -15,7 +15,6 @@ import { window } from 'vscode'; import { JupyterConnection } from '../../kernels/jupyter/connection/jupyterConnection'; -import { validateSelectJupyterURI } from '../../kernels/jupyter/connection/serverSelector'; import { IJupyterServerUri, IJupyterUriProvider, @@ -24,26 +23,40 @@ import { } from '../../kernels/jupyter/types'; import { IExtensionSyncActivationService } from '../../platform/activation/types'; import { IApplicationShell, IClipboard, IEncryptedStorage } from '../../platform/common/application/types'; -import { Identifiers, JVSC_EXTENSION_ID, Settings } from '../../platform/common/constants'; +import { JVSC_EXTENSION_ID, Settings } from '../../platform/common/constants'; import { GLOBAL_MEMENTO, - IConfigurationService, IDisposable, IDisposableRegistry, IMemento, IsWebExtension } from '../../platform/common/types'; import { DataScience } from '../../platform/common/utils/localize'; -import { traceError } from '../../platform/logging'; +import { traceError, traceWarning } from '../../platform/logging'; import { JupyterPasswordConnect } from '../../kernels/jupyter/connection/jupyterPasswordConnect'; -import { jupyterServerHandleFromString, jupyterServerHandleToString } from '../../kernels/jupyter/jupyterUtils'; +import { jupyterServerHandleFromString } from '../../kernels/jupyter/jupyterUtils'; +import { disposeAllDisposables } from '../../platform/common/helpers'; +import { Disposables } from '../../platform/common/utils'; +import { JupyterSelfCertsError } from '../../platform/errors/jupyterSelfCertsError'; +import { JupyterSelfCertsExpiredError } from '../../platform/errors/jupyterSelfCertsExpiredError'; +import { JupyterInvalidPasswordError } from '../../kernels/errors/jupyterInvalidPassword'; export const UserJupyterServerUriListKey = 'user-jupyter-server-uri-list'; const UserJupyterServerUriListMementoKey = '_builtin.jupyterServerUrlProvider.uriList'; +const NewSecretStorageKey = UserJupyterServerUriListKey; +const OldSecretStorageKey = `${JVSC_EXTENSION_ID}.${UserJupyterServerUriListKey}`; +const providerId = '_builtin.jupyterServerUrlProvider'; +type ServerInfoAndHandle = { + serverHandle: JupyterServerProviderHandle; + serverInfo: IJupyterServerUri; +}; @injectable() -export class UserJupyterServerUrlProvider implements IExtensionSyncActivationService, IDisposable, IJupyterUriProvider { - readonly id: string = '_builtin.jupyterServerUrlProvider'; +export class UserJupyterServerUrlProvider + extends Disposables + implements IExtensionSyncActivationService, IDisposable, IJupyterUriProvider +{ + readonly id: string = providerId; readonly extensionId = JVSC_EXTENSION_ID; readonly displayName: string = DataScience.UserJupyterServerUrlProviderDisplayName; readonly detail: string = DataScience.UserJupyterServerUrlProviderDetail; @@ -51,86 +64,55 @@ export class UserJupyterServerUrlProvider implements IExtensionSyncActivationSer onDidChangeHandles: Event = this._onDidChangeHandles.event; private _servers: { serverHandle: JupyterServerProviderHandle; serverInfo: IJupyterServerUri }[] = []; private _cachedServerInfoInitialized: Promise | undefined; - private _localDisposables: Disposable[] = []; + private readonly migration: MigrateOldStorage; constructor( @inject(IClipboard) private readonly clipboard: IClipboard, @inject(IJupyterUriProviderRegistration) private readonly uriProviderRegistration: IJupyterUriProviderRegistration, @inject(IApplicationShell) private readonly applicationShell: IApplicationShell, - @inject(IConfigurationService) private readonly configService: IConfigurationService, @inject(JupyterConnection) private readonly jupyterConnection: JupyterConnection, @inject(IsWebExtension) private readonly isWebExtension: boolean, @inject(IEncryptedStorage) private readonly encryptedStorage: IEncryptedStorage, @inject(IMemento) @named(GLOBAL_MEMENTO) private readonly globalMemento: Memento, - @inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry + @inject(IDisposableRegistry) disposables: IDisposableRegistry ) { - this.disposables.push(this); + super(); + disposables.push(this); + // eslint-disable-next-line @typescript-eslint/no-use-before-define + this.migration = new MigrateOldStorage(this.encryptedStorage, this.globalMemento); } activate() { - this._localDisposables.push(this.uriProviderRegistration.registerProvider(this)); + this.disposables.push(this.uriProviderRegistration.registerProvider(this)); this._servers = []; - this._localDisposables.push( + this.disposables.push( commands.registerCommand('dataScience.ClearUserProviderJupyterServerCache', async () => { - await this.encryptedStorage.store( - Settings.JupyterServerRemoteLaunchService, - UserJupyterServerUriListKey, - '' - ); - await this.globalMemento.update(UserJupyterServerUriListMementoKey, []); + await Promise.all([ + this.encryptedStorage.store(OldSecretStorageKey, undefined), + this.encryptedStorage.store(NewSecretStorageKey, undefined), + this.globalMemento.update(UserJupyterServerUriListMementoKey, undefined) + ]); this._servers = []; this._onDidChangeHandles.fire(); }) ); } - private async loadUserEnteredUrls(): Promise { - if (this._cachedServerInfoInitialized) { - return this._cachedServerInfoInitialized; - } - - this._cachedServerInfoInitialized = new Promise(async (resolve) => { - const serverList = this.globalMemento.get<{ index: number; handle: string }[]>( - UserJupyterServerUriListMementoKey - ); - - const cache = await this.encryptedStorage.retrieve( - Settings.JupyterServerRemoteLaunchService, - UserJupyterServerUriListKey - ); - - if (!cache || !serverList || serverList.length === 0) { - return resolve(); - } - - const encryptedList = cache.split(Settings.JupyterServerRemoteLaunchUriSeparator); - if (encryptedList.length === 0 || encryptedList.length !== serverList.length) { - traceError('Invalid server list, unable to retrieve server info'); - return resolve(); - } - - const servers: { serverHandle: JupyterServerProviderHandle; serverInfo: IJupyterServerUri }[] = []; - - for (let i = 0; i < encryptedList.length; i += 1) { - if (encryptedList[i].startsWith(Identifiers.REMOTE_URI)) { - continue; - } - const serverInfo = this.parseUri(encryptedList[i]); - if (!serverInfo) { - traceError('Unable to parse server info', encryptedList[i]); - } else { - servers.push({ - serverHandle: { extensionId: JVSC_EXTENSION_ID, handle: serverList[i].handle, id: this.id }, - serverInfo - }); + private async loadUserEnteredUrls(ignoreCache?: boolean): Promise { + await this.migration.migrate(); + if (!this._cachedServerInfoInitialized || ignoreCache) { + this._cachedServerInfoInitialized = new Promise(async (resolve) => { + try { + const data = await this.encryptedStorage.retrieve(NewSecretStorageKey); + const servers: ServerInfoAndHandle[] = data && data.length ? JSON.parse(data) : []; + this._servers = servers; + } catch (ex) { + traceError('Failed to load user entered urls', ex); } - } - - this._servers = servers; - - resolve(); - }); + resolve(); + }); + } return this._cachedServerInfoInitialized; } @@ -198,7 +180,7 @@ export class UserJupyterServerUrlProvider implements IExtensionSyncActivationSer input.validationMessage = DataScience.jupyterSelectURIInvalidURI; return; } - const jupyterServerUri = this.parseUri(uri, ''); + const jupyterServerUri = parseUri(uri); if (!jupyterServerUri) { if (inputWasHidden) { input.show(); @@ -208,36 +190,55 @@ export class UserJupyterServerUrlProvider implements IExtensionSyncActivationSer } const serverHandle = { extensionId: JVSC_EXTENSION_ID, handle: uuid(), id: this.id }; - const message = await validateSelectJupyterURI( - this.jupyterConnection, - this.applicationShell, - this.configService, - this.isWebExtension, - serverHandle, - jupyterServerUri - ); + let message = ''; + try { + await this.jupyterConnection.validateJupyterServer(serverHandle, jupyterServerUri, true); + } catch (err) { + traceWarning('Uri verification error', err); + if (JupyterSelfCertsError.isSelfCertsError(err)) { + message = DataScience.jupyterSelfCertFailErrorMessageOnly; + } else if (JupyterSelfCertsExpiredError.isSelfCertsExpiredError(err)) { + message = DataScience.jupyterSelfCertExpiredErrorMessageOnly; + } else if (err && err instanceof JupyterInvalidPasswordError) { + message = DataScience.passwordFailure; + } else { + // Return the general connection error to show in the validation box + // Replace any Urls in the error message with markdown link. + const urlRegex = /(https?:\/\/[^\s]+)/g; + const errorMessage = (err.message || err.toString()).replace( + urlRegex, + (url: string) => `[${url}](${url})` + ); + message = ( + this.isWebExtension || true + ? DataScience.remoteJupyterConnectionFailedWithoutServerWithErrorWeb + : DataScience.remoteJupyterConnectionFailedWithoutServerWithError + )(errorMessage); + } + } if (message) { if (inputWasHidden) { input.show(); } input.validationMessage = message; - } else { - promptingForServerName = true; - // Offer the user a chance to pick a display name for the server - // Leaving it blank will use the URI as the display name - jupyterServerUri.displayName = - (await this.applicationShell.showInputBox({ - title: DataScience.jupyterRenameServer - })) || jupyterServerUri.displayName; - - this._servers.push({ + return; + } + + promptingForServerName = true; + // Offer the user a chance to pick a display name for the server + jupyterServerUri.displayName = + (await this.applicationShell.showInputBox({ + title: DataScience.jupyterRenameServer + })) || new URL(jupyterServerUri.baseUrl).hostname; + + await this.updateMemento({ + add: { serverHandle, serverInfo: jupyterServerUri - }); - await this.updateMemento(); - resolve(serverHandle.handle); - } + } + }); + resolve(serverHandle.handle); }), input.onDidHide(() => { inputWasHidden = true; @@ -248,44 +249,11 @@ export class UserJupyterServerUrlProvider implements IExtensionSyncActivationSer ); input.show(); - }).finally(() => { - disposables.forEach((d) => d.dispose()); - }); - } - - private parseUri(uri: string, displayName?: string): IJupyterServerUri | undefined { - // This is a url that we crafted. It's not a valid Jupyter Server Url. - if (uri.startsWith(Identifiers.REMOTE_URI)) { - return; - } - try { - // Do not call this if we can avoid it, as this logs errors. - jupyterServerHandleFromString(uri); - // This is a url that we crafted. It's not a valid Jupyter Server Url. - return; - } catch (ex) { - // This is a valid Jupyter Server Url. - } - try { - const url = new URL(uri); - - // Special case for URI's ending with 'lab'. Remove this from the URI. This is not - // the location for connecting to jupyterlab - const baseUrl = `${url.protocol}//${url.host}${url.pathname === '/lab' ? '' : url.pathname}`; - - return { - baseUrl: baseUrl, - token: url.searchParams.get('token') || '', - displayName: displayName || url.hostname - }; - } catch (err) { - traceError(`Failed to parse URI ${uri}`, err); - // This should already have been parsed when set, so just throw if it's not right here - return undefined; - } + }).finally(() => disposeAllDisposables(disposables)); } async getServerUri(handle: string): Promise { + await this.loadUserEnteredUrls(); const server = this._servers.find((s) => s.serverHandle.handle === handle); if (!server) { throw new Error('Server not found'); @@ -299,25 +267,116 @@ export class UserJupyterServerUrlProvider implements IExtensionSyncActivationSer } async removeHandle(handle: string): Promise { - this._servers = this._servers.filter((s) => s.serverHandle.handle !== handle); - await this.updateMemento(); + await this.loadUserEnteredUrls(); + await this.updateMemento({ removeHandle: handle }); + } + + private async updateMemento(options: { add: ServerInfoAndHandle } | { removeHandle: string }) { + // Get the latest information, possible another workspace updated with a new server. + await this.loadUserEnteredUrls(true); + if ('add' in options) { + // Remove any duplicates. + this._servers = this._servers.filter((s) => s.serverInfo.baseUrl !== options.add.serverInfo.baseUrl); + this._servers.push(options.add); + } else if ('removeHandle' in options) { + this._servers = this._servers.filter((s) => s.serverHandle.handle !== options.removeHandle); + } + await this.encryptedStorage.store(NewSecretStorageKey, JSON.stringify(this._servers)); this._onDidChangeHandles.fire(); } +} - private async updateMemento() { - const blob = this._servers - .map((e) => `${jupyterServerHandleToString(e.serverHandle)}`) - .join(Settings.JupyterServerRemoteLaunchUriSeparator); - const mementoList = this._servers.map((v, i) => ({ index: i, handle: v.serverHandle.handle })); - await this.globalMemento.update(UserJupyterServerUriListMementoKey, mementoList); - return this.encryptedStorage.store( - Settings.JupyterServerRemoteLaunchService, - UserJupyterServerUriListKey, - blob +const REMOTE_URI = 'https://remote/'; +/** + * This can be removed after a few releases. + */ +class MigrateOldStorage { + private migration?: Promise; + constructor( + @inject(IEncryptedStorage) private readonly encryptedStorage: IEncryptedStorage, + @inject(IMemento) @named(GLOBAL_MEMENTO) private readonly globalMemento: Memento + ) {} + public async migrate() { + if (!this.migration) { + this.migration = this.migrateImpl(); + } + return this.migration; + } + private async migrateImpl() { + const oldStorage = await this.getOldStorage(); + if (oldStorage.length) { + await Promise.all([ + this.encryptedStorage.store(OldSecretStorageKey, undefined), + this.globalMemento.update(UserJupyterServerUriListMementoKey, undefined), + this.encryptedStorage.store(NewSecretStorageKey, JSON.stringify(oldStorage)) + ]); + } + } + private async getOldStorage() { + const serverList = this.globalMemento.get<{ index: number; handle: string }[]>( + UserJupyterServerUriListMementoKey ); + + const cache = await this.encryptedStorage.retrieve(OldSecretStorageKey); + if (!cache || !serverList || serverList.length === 0) { + return []; + } + + const encryptedList = cache.split(Settings.JupyterServerRemoteLaunchUriSeparator); + if (encryptedList.length === 0 || encryptedList.length !== serverList.length) { + traceError('Invalid server list, unable to retrieve server info'); + return []; + } + + const servers: ServerInfoAndHandle[] = []; + + for (let i = 0; i < encryptedList.length; i += 1) { + if (encryptedList[i].startsWith(REMOTE_URI)) { + continue; + } + const serverInfo = parseUri(encryptedList[i]); + if (!serverInfo) { + traceError('Unable to parse server info', encryptedList[i]); + } else { + servers.push({ + serverHandle: { extensionId: JVSC_EXTENSION_ID, handle: serverList[i].handle, id: providerId }, + serverInfo + }); + } + } + + return servers; } +} - dispose(): void { - this._localDisposables.forEach((d) => d.dispose()); +function parseUri(uri: string): IJupyterServerUri | undefined { + // This is a url that we crafted. It's not a valid Jupyter Server Url. + if (uri.startsWith(REMOTE_URI)) { + return; + } + try { + // Do not call this if we can avoid it, as this logs errors. + jupyterServerHandleFromString(uri); + // This is a url that we crafted. It's not a valid Jupyter Server Url. + return; + } catch (ex) { + // This is a valid Jupyter Server Url. + } + try { + const url = new URL(uri); + + // Special case for URI's ending with 'lab'. Remove this from the URI. This is not + // the location for connecting to jupyterlab + const baseUrl = `${url.protocol}//${url.host}${url.pathname === '/lab' ? '' : url.pathname}`; + + return { + baseUrl: baseUrl, + token: url.searchParams.get('token') || '', + displayName: '' // This would have been provided earlier + }; + } catch (err) { + traceError(`Failed to parse URI ${uri}`, err); + // This should already have been parsed when set, so just throw if it's not right here + return undefined; } } diff --git a/src/telemetry.ts b/src/telemetry.ts index 717f7679c3c1..532bccc506c2 100644 --- a/src/telemetry.ts +++ b/src/telemetry.ts @@ -1744,25 +1744,6 @@ export class IEventNamePropertyMapping { } } }; - /** - * Jupyter URI was valid and set to a remote setting. - */ - [Telemetry.SetJupyterURIToUserSpecified]: TelemetryEventInfo<{ - /** - * Was the URI set to an Azure uri. - */ - azure: boolean; - }> = { - owner: 'donjayamanne', - feature: ['KernelPicker'], - source: 'N/A', - properties: { - azure: { - classification: 'SystemMetaData', - purpose: 'FeatureInsight' - } - } - }; /** * Information banner displayed to give the user the option to configure shift+enter for the Interactive Window. */ diff --git a/src/test/datascience/mockEncryptedStorage.ts b/src/test/datascience/mockEncryptedStorage.ts deleted file mode 100644 index bf923c626be7..000000000000 --- a/src/test/datascience/mockEncryptedStorage.ts +++ /dev/null @@ -1,23 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -import { IEncryptedStorage } from '../../platform/common/application/types'; - -/** - * Mock for encrypted storage. Doesn't do anything except hold values in memory (keytar doesn't work without a UI coming up on Mac/Linux) - */ -export class MockEncryptedStorage implements IEncryptedStorage { - private map = new Map(); - public async store(service: string, key: string, value: string | undefined): Promise { - const trueKey = `${service}.${key}`; - if (value) { - this.map.set(trueKey, value); - } else { - this.map.delete(trueKey); - } - } - public async retrieve(service: string, key: string): Promise { - const trueKey = `${service}.${key}`; - return this.map.get(trueKey); - } -} diff --git a/src/test/datascience/notebook/remoteNotebookEditor.vscode.common.test.ts b/src/test/datascience/notebook/remoteNotebookEditor.vscode.common.test.ts index 2e07a42e236a..343b02346146 100644 --- a/src/test/datascience/notebook/remoteNotebookEditor.vscode.common.test.ts +++ b/src/test/datascience/notebook/remoteNotebookEditor.vscode.common.test.ts @@ -4,10 +4,10 @@ /* eslint-disable @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires */ import { assert } from 'chai'; import * as sinon from 'sinon'; -import { commands, CompletionList, Memento, Position, Uri, window } from 'vscode'; -import { IEncryptedStorage, IVSCodeNotebook } from '../../../platform/common/application/types'; +import { commands, CompletionList, Position, Uri, window } from 'vscode'; +import { IVSCodeNotebook } from '../../../platform/common/application/types'; import { traceInfo } from '../../../platform/logging'; -import { GLOBAL_MEMENTO, IDisposable, IMemento } from '../../../platform/common/types'; +import { IDisposable, IExtensionContext } from '../../../platform/common/types'; import { captureScreenShot, IExtensionTestApi, initialize, startJupyterServer, waitForCondition } from '../../common'; import { closeActiveWindows } from '../../initialize'; import { @@ -25,13 +25,14 @@ import { defaultNotebookTestTimeout } from './helper'; import { openNotebook } from '../helpers'; -import { PYTHON_LANGUAGE, Settings } from '../../../platform/common/constants'; +import { PYTHON_LANGUAGE } from '../../../platform/common/constants'; import { IS_REMOTE_NATIVE_TEST, JVSC_EXTENSION_ID_FOR_TESTS } from '../../constants'; import { PreferredRemoteKernelIdProvider } from '../../../kernels/jupyter/connection/preferredRemoteKernelIdProvider'; import { IServiceContainer } from '../../../platform/ioc/types'; import { setIntellisenseTimeout } from '../../../standalone/intellisense/pythonKernelCompletionProvider'; import { IControllerRegistration } from '../../../notebooks/controllers/types'; import { ControllerDefaultService } from './controllerDefaultService'; +import { IFileSystem } from '../../../platform/common/platform/types'; /* eslint-disable @typescript-eslint/no-explicit-any, no-invalid-this */ suite('Remote Execution @kernelCore', function () { @@ -41,10 +42,10 @@ suite('Remote Execution @kernelCore', function () { let vscodeNotebook: IVSCodeNotebook; let ipynbFile: Uri; let serviceContainer: IServiceContainer; - let globalMemento: Memento; - let encryptedStorage: IEncryptedStorage; + let fs: IFileSystem; let controllerRegistration: IControllerRegistration; let controllerDefault: ControllerDefaultService; + let storageFile: Uri; suiteSetup(async function () { if (!IS_REMOTE_NATIVE_TEST()) { @@ -54,10 +55,13 @@ suite('Remote Execution @kernelCore', function () { api = await initialize(); await startJupyterServer(); sinon.restore(); + storageFile = Uri.joinPath( + api.serviceContainer.get(IExtensionContext).globalStorageUri, + 'remoteServersMRUList.json' + ); serviceContainer = api.serviceContainer; vscodeNotebook = api.serviceContainer.get(IVSCodeNotebook); - encryptedStorage = api.serviceContainer.get(IEncryptedStorage); - globalMemento = api.serviceContainer.get(IMemento, GLOBAL_MEMENTO); + fs = api.serviceContainer.get(IFileSystem); controllerRegistration = api.serviceContainer.get(IControllerRegistration); controllerDefault = ControllerDefaultService.create(api.serviceContainer); }); @@ -100,8 +104,11 @@ suite('Remote Execution @kernelCore', function () { }); suiteTeardown(() => closeNotebooksAndCleanUpAfterTests(disposables)); test('MRU and encrypted storage should be updated with remote Uri info', async function () { - const previousList = globalMemento.get<{}[]>(Settings.JupyterServerUriList, []); - const encryptedStorageSpiedStore = sinon.spy(encryptedStorage, 'store'); + const previousContents = await fs + .readFile(storageFile) + .then((b) => JSON.parse(b.toString())) + .catch(() => []); + const fsWriteSpy = sinon.spy(fs, 'writeFile'); const { editor } = await openNotebook(ipynbFile); await waitForKernelToGetAutoSelected(editor, PYTHON_LANGUAGE); await deleteAllCellsAndWait(); @@ -109,19 +116,20 @@ suite('Remote Execution @kernelCore', function () { const cell = editor.notebook.cellAt(0)!; await Promise.all([runAllCellsInActiveNotebook(), waitForExecutionCompletedSuccessfully(cell)]); - // Wait for MRU to get updated & encrypted storage to get updated. - await waitForCondition(async () => encryptedStorageSpiedStore.called, 5_000, 'Encrypted storage not updated'); + // Wait for MRU to get updated + await waitForCondition(async () => fsWriteSpy.called, 5_000, 'Storage not updated'); + let newContents: unknown[]; await waitForCondition( async () => { - const newList = globalMemento.get<{}[]>(Settings.JupyterServerUriList, []); - assert.notDeepEqual(previousList, newList, 'MRU not updated'); + newContents = await fs + .readFile(storageFile) + .then((b) => JSON.parse(b.toString())) + .catch(() => []); + assert.notDeepEqual(newContents, previousContents, 'MRU not updated'); return true; }, 5_000, - () => - `MRU not updated, previously ${JSON.stringify(previousList)}, now ${JSON.stringify( - globalMemento.get<{}[]>(Settings.JupyterServerUriList, []) - )}` + () => `MRU not updated, previously ${JSON.stringify(previousContents)}, now ${JSON.stringify(newContents)}` ); }); test('Use same kernel when re-opening notebook', async function () {