From 46d4b3a51f65a933ccaccf53f792e4368beced3f Mon Sep 17 00:00:00 2001 From: Sol Lee Date: Thu, 26 Sep 2024 11:59:59 +0000 Subject: [PATCH] Revert "Merge pull request #1847 from minrk/fetch-events" This reverts commit c6c5dc8fe73f81ca538c47b420b33f317c3aa8ae, reversing changes made to 430a3ea0267fad2aff71c164fb5157d068203abf. --- binderhub/app.py | 3 +- binderhub/base.py | 13 +- binderhub/builder.py | 4 - binderhub/static/js/index.js | 9 +- binderhub/static/js/src/repo.js | 10 +- binderhub/templates/index.html | 2 - binderhub/templates/loading.html | 2 - js/packages/binderhub-client/lib/index.js | 161 +++--------------- js/packages/binderhub-client/package.json | 2 +- .../binderhub-client/tests/index.test.js | 50 +----- package.json | 2 +- 11 files changed, 48 insertions(+), 210 deletions(-) diff --git a/binderhub/app.py b/binderhub/app.py index 2cf9e2b50..881fe5d71 100644 --- a/binderhub/app.py +++ b/binderhub/app.py @@ -964,7 +964,8 @@ def initialize(self, *args, **kwargs): "enable_api_only_mode": self.enable_api_only_mode, } ) - self.tornado_settings["cookie_secret"] = os.urandom(32) + if self.auth_enabled: + self.tornado_settings["cookie_secret"] = os.urandom(32) if self.cors_allow_origin: self.tornado_settings.setdefault("headers", {})[ "Access-Control-Allow-Origin" diff --git a/binderhub/base.py b/binderhub/base.py index 5f198c401..b002bbd95 100644 --- a/binderhub/base.py +++ b/binderhub/base.py @@ -137,20 +137,11 @@ def get_current_user(self): @property def template_namespace(self): - - ns = dict( + return dict( static_url=self.static_url, banner=self.settings["banner_message"], - auth_enabled=self.settings["auth_enabled"], - ) - if self.settings["auth_enabled"]: - ns["xsrf"] = self.xsrf_token.decode("ascii") - ns["api_token"] = self.hub_auth.get_token(self) or "" - - ns.update( - self.settings.get("template_variables", {}), + **self.settings.get("template_variables", {}), ) - return ns def set_default_headers(self): headers = self.settings.get("headers", {}) diff --git a/binderhub/builder.py b/binderhub/builder.py index f420febe7..914eefce0 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -247,10 +247,6 @@ def _get_build_only(self): return build_only - def redirect(self, *args, **kwargs): - # disable redirect to login, which won't work for EventSource - raise HTTPError(403) - @authenticated async def get(self, provider_prefix, _unescaped_spec): """Get a built image for a given spec and repo provider. diff --git a/binderhub/static/js/index.js b/binderhub/static/js/index.js index dbd9639ab..536a4e278 100644 --- a/binderhub/static/js/index.js +++ b/binderhub/static/js/index.js @@ -1,6 +1,7 @@ /* If this file gets over 200 lines of code long (not counting docs / comments), start using a framework */ import ClipboardJS from "clipboard"; +import "event-source-polyfill"; import { BinderRepository } from "@jupyterhub/binderhub-client"; import { updatePathText } from "./src/path"; @@ -60,12 +61,12 @@ async function build(providerSpec, log, fitAddon, path, pathType) { $(".on-build").removeClass("hidden"); const buildToken = $("#build-token").data("token"); - const apiToken = $("#api-token").data("token"); const buildEndpointUrl = new URL("build", BASE_URL); - const image = new BinderRepository(providerSpec, buildEndpointUrl, { - apiToken, + const image = new BinderRepository( + providerSpec, + buildEndpointUrl, buildToken, - }); + ); for await (const data of image.fetch()) { // Write message to the log terminal if there is a message diff --git a/binderhub/static/js/src/repo.js b/binderhub/static/js/src/repo.js index 17a3524b2..e848b49ed 100644 --- a/binderhub/static/js/src/repo.js +++ b/binderhub/static/js/src/repo.js @@ -24,16 +24,8 @@ function setLabels() { */ export function updateRepoText(baseUrl) { if (Object.keys(configDict).length === 0) { - const xsrf = $("#xsrf-token").data("token"); - const apiToken = $("#api-token").data("token"); const configUrl = new URL("_config", baseUrl); - const headers = {}; - if (apiToken && apiToken.length > 0) { - headers["Authorization"] = `Bearer ${apiToken}`; - } else if (xsrf && xsrf.length > 0) { - headers["X-Xsrftoken"] = xsrf; - } - fetch(configUrl, { headers }).then((resp) => { + fetch(configUrl).then((resp) => { resp.json().then((data) => { configDict = data; setLabels(); diff --git a/binderhub/templates/index.html b/binderhub/templates/index.html index cc55b89a7..6260c9c9d 100644 --- a/binderhub/templates/index.html +++ b/binderhub/templates/index.html @@ -3,8 +3,6 @@ {% block head %} - - {{ super() }} {% endblock head %} diff --git a/binderhub/templates/loading.html b/binderhub/templates/loading.html index 4fe8af1b0..dd6369ef2 100644 --- a/binderhub/templates/loading.html +++ b/binderhub/templates/loading.html @@ -14,8 +14,6 @@ - - {{ super() }} diff --git a/js/packages/binderhub-client/lib/index.js b/js/packages/binderhub-client/lib/index.js index f33f35600..9401eea7f 100644 --- a/js/packages/binderhub-client/lib/index.js +++ b/js/packages/binderhub-client/lib/index.js @@ -1,30 +1,8 @@ -import { fetchEventSource } from "@microsoft/fetch-event-source"; +import { NativeEventSource, EventSourcePolyfill } from "event-source-polyfill"; import { EventIterator } from "event-iterator"; -function _getXSRFToken() { - // from @jupyterlab/services - // https://github.com/jupyterlab/jupyterlab/blob/69223102d717f3d3e9f976d32e657a4e2456e85d/packages/services/src/contents/index.ts#L1178-L1184 - let cookie = ""; - try { - cookie = document.cookie; - } catch (e) { - // e.g. SecurityError in case of CSP Sandbox - return null; - } - // extracts the value of the cookie named `_xsrf` - // by picking up everything between `_xsrf=` and the next semicolon or end-of-line - // `\b` ensures word boundaries, so it doesn't pick up `something_xsrf=`... - const xsrfTokenMatch = cookie.match("\\b_xsrf=([^;]*)\\b"); - if (xsrfTokenMatch) { - return xsrfTokenMatch[1]; - } - return null; -} - -/* throw this to close the event stream */ -class EventStreamClose extends Error {} -/* throw this to close the event stream */ -class EventStreamRetry extends Error {} +// Use native browser EventSource if available, and use the polyfill if not available +const EventSource = NativeEventSource || EventSourcePolyfill; /** * Build (and optionally launch) a repository by talking to a BinderHub API endpoint @@ -34,14 +12,10 @@ export class BinderRepository { * * @param {string} providerSpec Spec of the form // to pass to the binderhub API. * @param {URL} buildEndpointUrl API URL of the build endpoint to talk to - * @param {Object} [options] - optional arguments - * @param {string} [options.buildToken] Optional JWT based build token if this binderhub installation requires using build tokens - * @param {boolean} [options.buildOnly] Opt out of launching built image by default by passing `build_only` param - * @param {string} [options.apiToken] Optional Bearer token for authenticating requests + * @param {string} [buildToken] Optional JWT based build token if this binderhub installation requires using build tokens + * @param {boolean} [buildOnly] Opt out of launching built image by default by passing `build_only` param */ - constructor(providerSpec, buildEndpointUrl, options) { - const { apiToken, buildToken, buildOnly } = options || {}; - + constructor(providerSpec, buildEndpointUrl, buildToken, buildOnly) { this.providerSpec = providerSpec; // Make sure that buildEndpointUrl is a real URL - this ensures hostname is properly set if (!(buildEndpointUrl instanceof URL)) { @@ -66,10 +40,8 @@ export class BinderRepository { if (buildOnly) { this.buildUrl.searchParams.append("build_only", "true"); } - this.apiToken = apiToken; this.eventIteratorQueue = null; - this.abortSignal = null; } /** @@ -95,100 +67,26 @@ export class BinderRepository { * @returns {AsyncIterable} An async iterator yielding responses from the API as they come in */ fetch() { - const headers = {}; - this.abortController = new AbortController(); - - if (this.apiToken && this.apiToken.length > 0) { - headers["Authorization"] = `Bearer ${this.apiToken}`; - } else { - const xsrf = _getXSRFToken(); - if (xsrf) { - headers["X-Xsrftoken"] = xsrf; - } - } - // setTimeout(() => this.close(), 1000); + this.eventSource = new EventSource(this.buildUrl); return new EventIterator((queue) => { this.eventIteratorQueue = queue; - fetchEventSource(this.buildUrl, { - headers, - // signal used for closing - signal: this.abortController.signal, - // openWhenHidden leaves connection open (matches default) - // otherwise fetch-event closes connections, - // which would be nice if our javascript handled restarting messages better - openWhenHidden: true, - onopen: (response) => { - if (response.ok) { - return; // everything's good - } else if ( - response.status >= 400 && - response.status < 500 && - response.status !== 429 - ) { - queue.push({ - phase: "failed", - message: `Failed to connect to event stream: ${response.status} - ${response.text}\n`, - }); - throw new EventStreamClose(); - } else { - queue.push({ - phase: "unknown", - message: `Error connecting to event stream, retrying: ${response.status} - ${response.text}\n`, - }); - throw new EventStreamRetry(); - } - }, - - onclose: () => { - if (!queue.isStopped) { - // close called before queue finished - queue.push({ - phase: "failed", - message: `Event stream closed unexpectedly\n`, - }); - queue.stop(); - // throw new EventStreamClose(); - } - }, - onerror: (error) => { - console.log("Event stream error", error); - if (error.name === "EventStreamRetry") { - // if we don't re-raise, connection will be retried; - queue.push({ - phase: "unknown", - message: `Error in event stream: ${error}\n`, - }); - return; - } - if ( - !(error.name === "EventStreamClose" || error.name === "AbortError") - ) { - // errors _other_ than EventStreamClose get displayed - queue.push({ - phase: "failed", - message: `Error in event stream: ${error}\n`, - }); - } - queue.stop(); - // need to rethrow to prevent reconnection - throw error; - }, - - onmessage: (event) => { - if (!event.data || event.data === "") { - // onmessage is called for the empty lines - return; - } - const data = JSON.parse(event.data); - // FIXME: fix case of phase/state upstream - if (data.phase) { - data.phase = data.phase.toLowerCase(); - } - queue.push(data); - if (data.phase === "failed") { - throw new EventStreamClose(); - } - }, + this.eventSource.onerror = () => { + queue.push({ + phase: "failed", + message: "Failed to connect to event stream\n", + }); + queue.stop(); + }; + + this.eventSource.addEventListener("message", (event) => { + // console.log("message received") + // console.log(event) + const data = JSON.parse(event.data); + // FIXME: fix case of phase/state upstream + if (data.phase) { + data.phase = data.phase.toLowerCase(); + } + queue.push(data); }); }); } @@ -197,15 +95,12 @@ export class BinderRepository { * Close the EventSource connection to the BinderHub API if it is open */ close() { - if (this.eventIteratorQueue) { + if (this.eventSource !== undefined) { + this.eventSource.close(); + } + if (this.eventIteratorQueue !== null) { // Stop any currently running fetch() iterations this.eventIteratorQueue.stop(); - this.eventIteratorQueue = null; - } - if (this.abortController) { - // close event source - this.abortController.abort(); - this.abortController = null; } } diff --git a/js/packages/binderhub-client/package.json b/js/packages/binderhub-client/package.json index db3a2ec7c..b2ab42c3c 100644 --- a/js/packages/binderhub-client/package.json +++ b/js/packages/binderhub-client/package.json @@ -14,7 +14,7 @@ }, "homepage": "https://github.com/jupyterhub/binderhub#readme", "dependencies": { - "@microsoft/fetch-event-source": "^2.0.1", + "event-source-polyfill": "^1.0.31", "event-iterator": "^2.0.0" } } diff --git a/js/packages/binderhub-client/tests/index.test.js b/js/packages/binderhub-client/tests/index.test.js index 04e35685b..0d35cdea0 100644 --- a/js/packages/binderhub-client/tests/index.test.js +++ b/js/packages/binderhub-client/tests/index.test.js @@ -1,6 +1,3 @@ -// fetch polyfill (only needed for node tests) -import { fetch, TextDecoder } from "@whatwg-node/fetch"; - import { BinderRepository, makeShareableBinderURL, @@ -9,32 +6,9 @@ import { import { parseEventSource, simpleEventSourceServer } from "./utils"; import { readFileSync } from "node:fs"; -async function wrapFetch(resource, options) { - /* like fetch, but ignore signal input - // abort signal shows up as uncaught in tests, despite working fine - */ - if (options) { - options.signal = null; - } - return fetch.apply(null, [resource, options]); -} - -beforeAll(() => { - // inject globals for fetch - global.TextDecoder = TextDecoder; - if (!global.window) { - global.window = {}; - } - if (!global.window.fetch) { - global.window.fetch = wrapFetch; - } -}); - test("Passed in URL object is not modified", () => { const buildEndpointUrl = new URL("https://test-binder.org/build"); - const br = new BinderRepository("gh/test/test", buildEndpointUrl, { - buildToken: "token", - }); + const br = new BinderRepository("gh/test/test", buildEndpointUrl, "token"); expect(br.buildEndpointUrl.toString()).not.toEqual( buildEndpointUrl.toString(), ); @@ -42,15 +16,13 @@ test("Passed in URL object is not modified", () => { test("Invalid URL errors out", () => { expect(() => { - new BinderRepository("gh/test/test", "/build", { buildToken: "token" }); + new BinderRepository("gh/test/test", "/build", "token"); }).toThrow(TypeError); }); test("Passing buildOnly flag works", () => { const buildEndpointUrl = new URL("https://test-binder.org/build"); - const br = new BinderRepository("gh/test/test", buildEndpointUrl, { - buildOnly: true, - }); + const br = new BinderRepository("gh/test/test", buildEndpointUrl, null, true); expect(br.buildUrl.toString()).toEqual( "https://test-binder.org/build/gh/test/test?build_only=true", ); @@ -74,9 +46,7 @@ test("Build URL correctly built from Build Endpoint", () => { test("Build URL correctly built from Build Endpoint when used with token", () => { const buildEndpointUrl = new URL("https://test-binder.org/build"); - const br = new BinderRepository("gh/test/test", buildEndpointUrl, { - buildToken: "token", - }); + const br = new BinderRepository("gh/test/test", buildEndpointUrl, "token"); expect(br.buildUrl.toString()).toEqual( "https://test-binder.org/build/gh/test/test?build_token=token", ); @@ -274,16 +244,12 @@ describe("Invalid eventsource response causes failure", () => { test("Invalid eventsource response should cause failure", async () => { const buildEndpointUrl = new URL(`${serverUrl}/build`); const br = new BinderRepository("gh/test/test", buildEndpointUrl); - let messages = []; for await (const item of br.fetch()) { - messages.push(item); - } - expect(messages).toStrictEqual([ - { + expect(item).toStrictEqual({ phase: "failed", - message: "Event stream closed unexpectedly\n", - }, - ]); + message: "Failed to connect to event stream\n", + }); + } }); }); diff --git a/package.json b/package.json index 916d542cf..e08b94bbc 100644 --- a/package.json +++ b/package.json @@ -4,6 +4,7 @@ "dependencies": { "bootstrap": "^3.4.1", "clipboard": "^2.0.11", + "event-source-polyfill": "^1.0.31", "jquery": "^3.6.4", "xterm": "^5.1.0", "xterm-addon-fit": "^0.7.0" @@ -14,7 +15,6 @@ "@babel/eslint-parser": "^7.22.15", "@babel/preset-env": "^7.21.4", "@types/jest": "^29.5.5", - "@whatwg-node/fetch": "^0.9.17", "babel-jest": "^29.7.0", "babel-loader": "^9.1.2", "css-loader": "^6.7.3",