From c176c7155c3323d5d9b4bb61d3c23ee07330edf9 Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Tue, 1 Oct 2019 10:07:52 -0700 Subject: [PATCH 1/5] fix no pass no auth and add tests --- .../jupyter/jupyterPasswordConnect.ts | 16 +++-- .../jupyter/jupyterSessionManager.ts | 4 +- src/client/datascience/types.ts | 1 + .../datascience/notebook.functional.test.ts | 63 ++++++++++++++++++- .../serverConfigFiles/remoteNoAuth.py | 4 ++ 5 files changed, 78 insertions(+), 10 deletions(-) create mode 100644 src/test/datascience/serverConfigFiles/remoteNoAuth.py diff --git a/src/client/datascience/jupyter/jupyterPasswordConnect.ts b/src/client/datascience/jupyter/jupyterPasswordConnect.ts index 1b2f1f857791..5ba2cb0db843 100644 --- a/src/client/datascience/jupyter/jupyterPasswordConnect.ts +++ b/src/client/datascience/jupyter/jupyterPasswordConnect.ts @@ -53,13 +53,17 @@ export class JupyterPasswordConnect implements IJupyterPasswordConnect { sessionCookieName = sessionResult.sessionCookieName; sessionCookieValue = sessionResult.sessionCookieValue; } + } else { + // If userPassword is undefined or '' then the user didn't pick a password. In this case return back that we should just try to connect + // like a standard connection. Might be the case where there is no token and no password + return { emptyPassword: true, xsrfCookie: '', sessionCookieName: '', sessionCookieValue: '' }; } userPassword = undefined; // If we found everything return it all back if not, undefined as partial is useless if (xsrfCookie && sessionCookieName && sessionCookieValue) { sendTelemetryEvent(Telemetry.GetPasswordSuccess); - return { xsrfCookie, sessionCookieName, sessionCookieValue }; + return { xsrfCookie, sessionCookieName, sessionCookieValue, emptyPassword: false }; } else { sendTelemetryEvent(Telemetry.GetPasswordFailure); return undefined; @@ -69,14 +73,14 @@ export class JupyterPasswordConnect implements IJupyterPasswordConnect { // For HTTPS connections respect our allowUnauthorized setting by adding in an agent to enable that on the request private addAllowUnauthorized(url: string, allowUnauthorized: boolean, options: nodeFetch.RequestInit): nodeFetch.RequestInit { if (url.startsWith('https') && allowUnauthorized) { - const requestAgent = new HttpsAgent({rejectUnauthorized: false}); - return {...options, agent: requestAgent}; + const requestAgent = new HttpsAgent({ rejectUnauthorized: false }); + return { ...options, agent: requestAgent }; } return options; } - private async getUserPassword() : Promise { + private async getUserPassword(): Promise { // First get the proposed URI from the user return this.appShell.showInputBox({ prompt: localize.DataScience.jupyterSelectPasswordPrompt(), @@ -112,7 +116,7 @@ export class JupyterPasswordConnect implements IJupyterPasswordConnect { allowUnauthorized: boolean, xsrfCookie: string, password: string, - fetchFunction: (url: nodeFetch.RequestInfo, init?: nodeFetch.RequestInit) => Promise): Promise<{sessionCookieName: string | undefined; sessionCookieValue: string | undefined}> { + fetchFunction: (url: nodeFetch.RequestInfo, init?: nodeFetch.RequestInit) => Promise): Promise<{ sessionCookieName: string | undefined; sessionCookieValue: string | undefined }> { let sessionCookieName: string | undefined; let sessionCookieValue: string | undefined; // Create the form params that we need @@ -138,7 +142,7 @@ export class JupyterPasswordConnect implements IJupyterPasswordConnect { } } - return {sessionCookieName, sessionCookieValue}; + return { sessionCookieName, sessionCookieValue }; } private getCookies(response: nodeFetch.Response): Map { diff --git a/src/client/datascience/jupyter/jupyterSessionManager.ts b/src/client/datascience/jupyter/jupyterSessionManager.ts index 066631c427dd..d00d4d2ab169 100644 --- a/src/client/datascience/jupyter/jupyterSessionManager.ts +++ b/src/client/datascience/jupyter/jupyterSessionManager.ts @@ -116,11 +116,13 @@ export class JupyterSessionManager implements IJupyterSessionManager { if (connInfo.token === '' || connInfo.token === 'null') { serverSettings = { ...serverSettings, token: '' }; const pwSettings = await this.jupyterPasswordConnect.getPasswordConnectionInfo(connInfo.baseUrl, connInfo.allowUnauthorized ? true : false); - if (pwSettings) { + if (pwSettings && !pwSettings.emptyPassword) { cookieString = this.getSessionCookieString(pwSettings); const requestHeaders = { Cookie: cookieString, 'X-XSRFToken': pwSettings.xsrfCookie }; requestInit = { ...requestInit, headers: requestHeaders }; requiresWebSocket = true; + } else if (pwSettings && pwSettings.emptyPassword) { + serverSettings = { ...serverSettings, token: connInfo.token }; } else { // Failed to get password info, notify the user throw new Error(localize.DataScience.passwordFailure()); diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts index d0856618a417..e737c1b2740f 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -145,6 +145,7 @@ export interface IJupyterDebugger { } export interface IJupyterPasswordConnectInfo { + emptyPassword: boolean; xsrfCookie: string; sessionCookieName: string; sessionCookieValue: string; diff --git a/src/test/datascience/notebook.functional.test.ts b/src/test/datascience/notebook.functional.test.ts index 18c072e5bb31..2a4362142054 100644 --- a/src/test/datascience/notebook.functional.test.ts +++ b/src/test/datascience/notebook.functional.test.ts @@ -9,10 +9,12 @@ import { injectable } from 'inversify'; import * as os from 'os'; import * as path from 'path'; import { Readable, Writable } from 'stream'; +import * as TypeMoq from 'typemoq'; import * as uuid from 'uuid/v4'; import { Disposable, Uri } from 'vscode'; import { CancellationToken, CancellationTokenSource } from 'vscode-jsonrpc'; +import { IApplicationShell } from '../../client/common/application/types'; import { Cancellation, CancellationError } from '../../client/common/cancellation'; import { EXTENSION_ROOT_DIR } from '../../client/common/constants'; import { traceError, traceInfo } from '../../client/common/logger'; @@ -59,8 +61,6 @@ suite('DataScience notebook tests', () => { setup(() => { ioc = new DataScienceIocContainer(); ioc.registerDataScienceTypes(); - jupyterExecution = ioc.serviceManager.get(IJupyterExecution); - processFactory = ioc.serviceManager.get(IProcessServiceFactory); }); teardown(async () => { @@ -199,8 +199,14 @@ suite('DataScience notebook tests', () => { }); } - function runTest(name: string, func: () => Promise, _notebookProc?: ChildProcess) { + //function runTest(name: string, func: () => Promise, _notebookProc?: ChildProcess) { + function runTest(name: string, func: () => Promise, _notebookProc?: ChildProcess, rebindFunc?: () => void) { test(name, async () => { + if (rebindFunc) { + rebindFunc(); + } + jupyterExecution = ioc.serviceManager.get(IJupyterExecution); + processFactory = ioc.serviceManager.get(IProcessServiceFactory); console.log(`Starting test ${name} ...`); if (await jupyterExecution.isNotebookSupported()) { return func(); @@ -282,6 +288,53 @@ suite('DataScience notebook tests', () => { } }); + // Connect to a server that doesn't have a token or password, customers use this and we regressed it once + runTest('Remote No Auth', async () => { + const python = await getNotebookCapableInterpreter(ioc, processFactory); + const procService = await processFactory.create(); + + if (procService && python) { + const connectionFound = createDeferred(); + const configFile = path.join(EXTENSION_ROOT_DIR, 'src', 'test', 'datascience', 'serverConfigFiles', 'remoteNoAuth.py'); + const exeResult = procService.execObservable(python.path, ['-m', 'jupyter', 'notebook', `--config=${configFile}`], { env: process.env, throwOnStdErr: false }); + disposables.push(exeResult); + + exeResult.out.subscribe((output: Output) => { + traceInfo(`remote jupyter output: ${output.out}`); + const connectionURL = getIPConnectionInfo(output.out); + if (connectionURL) { + connectionFound.resolve(connectionURL); + } + }); + + const connString = await connectionFound.promise; + const uri = connString as string; + + // We have a connection string here, so try to connect jupyterExecution to the notebook server + const server = await jupyterExecution.connectToNotebookServer({ uri, useDefaultConfig: true, purpose: '' }); + const notebook = server ? await server.createNotebook(Uri.parse(Identifiers.InteractiveWindowIdentity)) : undefined; + if (!notebook) { + assert.fail('Failed to connect to remote password server'); + } else { + await verifySimple(notebook, `a=1${os.EOL}a`, 1); + } + // Have to dispose here otherwise the process may exit before hand and mess up cleanup. + await server!.dispose(); + } + }, undefined, () => { + const dummyDisposable = { + dispose: () => { return; } + }; + const appShell = TypeMoq.Mock.ofType(); + appShell.setup(a => a.showErrorMessage(TypeMoq.It.isAnyString())).returns((e) => { throw e; }); + appShell.setup(a => a.showInformationMessage(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve('')); + appShell.setup(a => a.showInformationMessage(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns((_a1: string, a2: string, _a3: string) => Promise.resolve(a2)); + appShell.setup(a => a.showInformationMessage(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns((_a1: string, a2: string, _a3: string, _a4: string) => Promise.resolve(a2)); + appShell.setup(a => a.showInputBox(TypeMoq.It.isAny())).returns(() => Promise.resolve('')); + appShell.setup(a => a.setStatusBarMessage(TypeMoq.It.isAny())).returns(() => dummyDisposable); + ioc.serviceManager.rebindInstance(IApplicationShell, appShell.object); + }); + runTest('Remote Password', async () => { const python = await getNotebookCapableInterpreter(ioc, processFactory); const procService = await processFactory.create(); @@ -368,6 +421,8 @@ suite('DataScience notebook tests', () => { }); test('Not installed', async () => { + jupyterExecution = ioc.serviceManager.get(IJupyterExecution); + processFactory = ioc.serviceManager.get(IProcessServiceFactory); // Rewire our data we use to search for processes class EmptyInterpreterService implements IInterpreterService { public get hasInterpreters(): Promise { @@ -1001,6 +1056,8 @@ plt.show()`, }); test('Notebook launch failure', async function () { + jupyterExecution = ioc.serviceManager.get(IJupyterExecution); + processFactory = ioc.serviceManager.get(IProcessServiceFactory); if (!ioc.mockJupyter) { // tslint:disable-next-line: no-invalid-this this.skip(); diff --git a/src/test/datascience/serverConfigFiles/remoteNoAuth.py b/src/test/datascience/serverConfigFiles/remoteNoAuth.py new file mode 100644 index 000000000000..e4dea4f52d0e --- /dev/null +++ b/src/test/datascience/serverConfigFiles/remoteNoAuth.py @@ -0,0 +1,4 @@ +# With these settings you can connect to a server with no token and no password +c.NotebookApp.token = '' +c.NotebookApp.open_browser = False +c.NotebookApp.disable_check_xsrf = True \ No newline at end of file From 434dfaefe942e7e0d369a78f1475b273d70af0e3 Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Tue, 1 Oct 2019 10:11:25 -0700 Subject: [PATCH 2/5] add comment --- src/test/datascience/notebook.functional.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/datascience/notebook.functional.test.ts b/src/test/datascience/notebook.functional.test.ts index 2a4362142054..886909de3351 100644 --- a/src/test/datascience/notebook.functional.test.ts +++ b/src/test/datascience/notebook.functional.test.ts @@ -199,9 +199,9 @@ suite('DataScience notebook tests', () => { }); } - //function runTest(name: string, func: () => Promise, _notebookProc?: ChildProcess) { function runTest(name: string, func: () => Promise, _notebookProc?: ChildProcess, rebindFunc?: () => void) { test(name, async () => { + // Give tests a chance to rebind IOC services before we fetch jupyterExecution and processFactory if (rebindFunc) { rebindFunc(); } From ffab55919b252da2977bc4d4ba857aa85759d51f Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Tue, 1 Oct 2019 10:12:49 -0700 Subject: [PATCH 3/5] add news --- news/2 Fixes/7137.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/2 Fixes/7137.md diff --git a/news/2 Fixes/7137.md b/news/2 Fixes/7137.md new file mode 100644 index 000000000000..952440c7f342 --- /dev/null +++ b/news/2 Fixes/7137.md @@ -0,0 +1 @@ +Fix regression to allow connection to servers with no token and no password and add functional test for this scenario \ No newline at end of file From 308c1e5651e9b73ee437410c8d281107fd3081f5 Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Tue, 1 Oct 2019 13:29:18 -0700 Subject: [PATCH 4/5] update pytest for requirements.txt --- news/requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/news/requirements.txt b/news/requirements.txt index 0dbf42bfb455..66bbf12fe931 100644 --- a/news/requirements.txt +++ b/news/requirements.txt @@ -1,2 +1,2 @@ docopt~=0.6.2 -pytest~=3.4.1 +pytest~=5.2.0 From 448f5fdc1dbd492006da4aef18443ad9466fba6e Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Tue, 1 Oct 2019 13:30:59 -0700 Subject: [PATCH 5/5] >= on advice from brett --- news/requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/news/requirements.txt b/news/requirements.txt index 66bbf12fe931..b5fafa23bfcd 100644 --- a/news/requirements.txt +++ b/news/requirements.txt @@ -1,2 +1,2 @@ docopt~=0.6.2 -pytest~=5.2.0 +pytest>=5.2.0