From 7b13defc15212450a0ae05ec938b067989e95b46 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Wed, 21 Aug 2024 20:36:15 -0500 Subject: [PATCH] fix: JS API should use GetConfigurationConstants for auth calls (#5959) In addition to more closely following what other Deephaven clients do, this prevents an issue where Open/Next pairs of gRPC calls (to emulate a bidirectional call) can race each other to redeem a one-time use auth token. Fixes #5955 --- .../deephaven/web/client/api/CoreClient.java | 13 -- .../web/client/api/WorkerConnection.java | 80 +++++----- .../stream/HandshakeStreamFactory.java | 150 ------------------ .../web/client/api/grpc/UnaryWithHeaders.java | 44 +++++ 4 files changed, 87 insertions(+), 200 deletions(-) delete mode 100644 web/client-api/src/main/java/io/deephaven/web/client/api/barrage/stream/HandshakeStreamFactory.java create mode 100644 web/client-api/src/main/java/io/deephaven/web/client/api/grpc/UnaryWithHeaders.java diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/CoreClient.java b/web/client-api/src/main/java/io/deephaven/web/client/api/CoreClient.java index 8687dedcd18..6c6c2d2d556 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/CoreClient.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/CoreClient.java @@ -131,19 +131,6 @@ public Promise login(@TsTypeRef(LoginCredentials.class) JsPropertyMap login = loginPromise.asPromise(); - // fetch configs and check session timeout - login.then(ignore -> getServerConfigValues()).then(configs -> { - for (String[] config : configs) { - if (config[0].equals("http.session.durationMs")) { - workerConnection.setSessionTimeoutMs(Double.parseDouble(config[1])); - } - } - return null; - }).catch_(ignore -> { - // Ignore this failure and suppress browser logging, we have a safe fallback - return Promise.resolve((Object) null); - }); - if (alreadyRunning) { ideConnection.connection.get().forceReconnect(); } diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java index 86549556f5a..ab88008a3de 100644 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java @@ -23,13 +23,12 @@ import io.deephaven.javascript.proto.dhinternal.arrow.flight.flatbuf.schema_generated.org.apache.arrow.flatbuf.Schema; import io.deephaven.javascript.proto.dhinternal.arrow.flight.protocol.browserflight_pb_service.BrowserFlightServiceClient; import io.deephaven.javascript.proto.dhinternal.arrow.flight.protocol.flight_pb.FlightData; -import io.deephaven.javascript.proto.dhinternal.arrow.flight.protocol.flight_pb.HandshakeRequest; -import io.deephaven.javascript.proto.dhinternal.arrow.flight.protocol.flight_pb.HandshakeResponse; import io.deephaven.javascript.proto.dhinternal.arrow.flight.protocol.flight_pb_service.FlightServiceClient; import io.deephaven.javascript.proto.dhinternal.browserheaders.BrowserHeaders; import io.deephaven.javascript.proto.dhinternal.flatbuffers.Builder; import io.deephaven.javascript.proto.dhinternal.flatbuffers.Long; import io.deephaven.javascript.proto.dhinternal.grpcweb.grpc.Code; +import io.deephaven.javascript.proto.dhinternal.grpcweb.grpc.UnaryOutput; import io.deephaven.javascript.proto.dhinternal.io.deephaven.barrage.flatbuf.barrage_generated.io.deephaven.barrage.flatbuf.BarrageMessageType; import io.deephaven.javascript.proto.dhinternal.io.deephaven.barrage.flatbuf.barrage_generated.io.deephaven.barrage.flatbuf.BarrageMessageWrapper; import io.deephaven.javascript.proto.dhinternal.io.deephaven.barrage.flatbuf.barrage_generated.io.deephaven.barrage.flatbuf.BarrageSubscriptionOptions; @@ -40,6 +39,9 @@ import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.application_pb.FieldsChangeUpdate; import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.application_pb.ListFieldsRequest; import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.application_pb_service.ApplicationServiceClient; +import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.config_pb.ConfigurationConstantsRequest; +import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.config_pb.ConfigurationConstantsResponse; +import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.config_pb_service.ConfigService; import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.config_pb_service.ConfigServiceClient; import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.console_pb.LogSubscriptionData; import io.deephaven.javascript.proto.dhinternal.io.deephaven.proto.console_pb.LogSubscriptionRequest; @@ -72,13 +74,13 @@ import io.deephaven.web.client.api.barrage.def.ColumnDefinition; import io.deephaven.web.client.api.barrage.def.InitialTableDefinition; import io.deephaven.web.client.api.barrage.stream.BiDiStream; -import io.deephaven.web.client.api.barrage.stream.HandshakeStreamFactory; import io.deephaven.web.client.api.barrage.stream.ResponseStreamWrapper; import io.deephaven.web.client.api.batch.RequestBatcher; import io.deephaven.web.client.api.batch.TableConfig; import io.deephaven.web.client.api.console.JsVariableChanges; import io.deephaven.web.client.api.console.JsVariableDefinition; import io.deephaven.web.client.api.console.JsVariableType; +import io.deephaven.web.client.api.grpc.UnaryWithHeaders; import io.deephaven.web.client.api.i18n.JsTimeZone; import io.deephaven.web.client.api.impl.TicketAndPromise; import io.deephaven.web.client.api.lifecycle.HasLifecycle; @@ -479,54 +481,58 @@ private Promise authUpdate() { DomGlobal.clearTimeout(scheduledAuthUpdate); scheduledAuthUpdate = null; } - return new Promise<>((resolve, reject) -> { - // the streamfactory will automatically reference our existing metadata, but we can listen to update it - BiDiStream handshake = HandshakeStreamFactory.create(this); - handshake.onHeaders(headers -> { - // unchecked cast is required here due to "aliasing" in ts/webpack resulting in BrowserHeaders != - // Metadata - JsArray authorization = Js.uncheckedCast(headers).get(FLIGHT_AUTH_HEADER_NAME); - if (authorization.length > 0) { - JsArray existing = metadata().get(FLIGHT_AUTH_HEADER_NAME); - if (!existing.getAt(0).equals(authorization.getAt(0))) { - // use this new token - metadata().set(FLIGHT_AUTH_HEADER_NAME, authorization); - CustomEventInit init = CustomEventInit.create(); - init.setDetail(new JsRefreshToken(authorization.getAt(0), sessionTimeoutMs)); - info.fireEvent(EVENT_REFRESH_TOKEN_UPDATED, init); + return UnaryWithHeaders.call( + this, ConfigService.GetConfigurationConstants, new ConfigurationConstantsRequest()) + .then(result -> { + BrowserHeaders headers = result.getHeaders(); + // unchecked cast is required here due to "aliasing" in ts/webpack resulting in BrowserHeaders != + // Metadata + JsArray authorization = + Js.uncheckedCast(headers).get(FLIGHT_AUTH_HEADER_NAME); + if (authorization.length > 0) { + JsArray existing = metadata().get(FLIGHT_AUTH_HEADER_NAME); + if (!existing.getAt(0).equals(authorization.getAt(0))) { + // use this new token + metadata().set(FLIGHT_AUTH_HEADER_NAME, authorization); + CustomEventInit init = CustomEventInit.create(); + init.setDetail(new JsRefreshToken(authorization.getAt(0), sessionTimeoutMs)); + info.fireEvent(EVENT_REFRESH_TOKEN_UPDATED, init); + } } - } - }); - handshake.onStatus(status -> { - if (status.isOk()) { + + // Read the timeout from the server, we'll refresh at less than that + result.getMessage().getConfigValuesMap().forEach((item, key) -> { + if (key.equals("http.session.durationMs")) { + sessionTimeoutMs = Double.parseDouble(item.getStringValue()); + } + }); + // schedule an update based on our currently configured delay scheduledAuthUpdate = DomGlobal.setTimeout(ignore -> { authUpdate(); }, sessionTimeoutMs / 2); - resolve.onInvoke((Void) null); - } else { - if (status.getCode() == Code.Unauthenticated) { + return Promise.resolve((Void) null); + }).catch_(err -> { + UnaryOutput result = (UnaryOutput) err; + if (result.getStatus() == Code.Unauthenticated) { // explicitly clear out any metadata for authentication, and signal that auth failed metadata.delete(FLIGHT_AUTH_HEADER_NAME); // Fire an event for the UI to attempt to re-auth info.fireEvent(CoreClient.EVENT_RECONNECT_AUTH_FAILED); - return; + + // We return here rather than continue and call checkStatus() + return Promise.reject("Authentication failed, please reconnect"); } - // TODO deephaven-core#2564 fire an event for the UI to re-auth - checkStatus(status); - if (status.getDetails() == null || status.getDetails().isEmpty()) { - reject.onInvoke("Error occurred while authenticating, gRPC status " + status.getCode()); + checkStatus(ResponseStreamWrapper.Status.of(result.getStatus(), result.getMessage().toString(), + result.getTrailers())); + if (result.getMessage() == null || result.getMessage().toString().isEmpty()) { + return Promise.reject(result.getMessage()); } else { - reject.onInvoke(status.getDetails()); + return Promise.reject("Error occurred while authenticating, gRPC status " + result.getStatus()); } - } - }); - - handshake.send(new HandshakeRequest()); - handshake.end(); - }); + }); } private void subscribeToTerminationNotification() { diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/barrage/stream/HandshakeStreamFactory.java b/web/client-api/src/main/java/io/deephaven/web/client/api/barrage/stream/HandshakeStreamFactory.java deleted file mode 100644 index bae2d3c41fd..00000000000 --- a/web/client-api/src/main/java/io/deephaven/web/client/api/barrage/stream/HandshakeStreamFactory.java +++ /dev/null @@ -1,150 +0,0 @@ -// -// Copyright (c) 2016-2024 Deephaven Data Labs and Patent Pending -// -package io.deephaven.web.client.api.barrage.stream; - -import elemental2.core.Function; -import elemental2.core.JsArray; -import io.deephaven.javascript.proto.dhinternal.arrow.flight.protocol.browserflight_pb_service.BrowserFlightService; -import io.deephaven.javascript.proto.dhinternal.arrow.flight.protocol.browserflight_pb_service.ResponseStream; -import io.deephaven.javascript.proto.dhinternal.arrow.flight.protocol.flight_pb.HandshakeRequest; -import io.deephaven.javascript.proto.dhinternal.arrow.flight.protocol.flight_pb.HandshakeResponse; -import io.deephaven.javascript.proto.dhinternal.arrow.flight.protocol.flight_pb_service.BidirectionalStream; -import io.deephaven.javascript.proto.dhinternal.arrow.flight.protocol.flight_pb_service.FlightService; -import io.deephaven.javascript.proto.dhinternal.grpcweb.Grpc; -import io.deephaven.javascript.proto.dhinternal.grpcweb.client.ClientRpcOptions; -import io.deephaven.javascript.proto.dhinternal.grpcweb.grpc.Client; -import io.deephaven.javascript.proto.dhinternal.grpcweb.invoke.InvokeRpcOptions; -import io.deephaven.javascript.proto.dhinternal.grpcweb.invoke.Request; -import io.deephaven.web.client.api.WorkerConnection; -import jsinterop.base.Js; - -import java.util.HashMap; -import java.util.Map; - -/** - * Improbable-eng's grpc-web implementation doesn't pass headers to api callers, only trailers (which sometimes includes - * headers) are included when calls fail, but never when successful. The current Flight auth v2 setup requires reading - * headers from responses, but strictly speaking doesn't require reading them from all calls - we can make extra - * FlightService/Handshake calls as long as we can read the response headers with them. - *

- *

- * This class includes a custom implementation of the Handshake method that is able to notify callers about headers that - * are received. - */ -public class HandshakeStreamFactory { - - private static final String DATA_EVENT_LISTENER_NAME = "data"; - private static final String END_EVENT_LISTENER_NAME = "end"; - private static final String STATUS_EVENT_LISTENER_NAME = "status"; - private static final String HEADERS_EVENT_LISTENER_NAME = "headers"; - - public static BiDiStream create(WorkerConnection connection) { - return connection.streamFactory().create( - metadata -> { - Map> listeners = listenerMap(); - ClientRpcOptions options = ClientRpcOptions.create(); - options.setHost(connection.flightServiceClient().serviceHost); - options.setTransport(null);// ts doesn't expose these two, stick with defaults for now - options.setDebug(false); - Client client = Grpc.client(FlightService.Handshake, - (io.deephaven.javascript.proto.dhinternal.grpcweb.grpc.ClientRpcOptions) options); - client.onEnd((status, statusMessage, trailers) -> { - listeners.get(STATUS_EVENT_LISTENER_NAME).forEach((item, index) -> item.call(null, - ResponseStreamWrapper.Status.of(status, statusMessage, metadata))); - listeners.get(END_EVENT_LISTENER_NAME).forEach((item, index) -> item.call(null, - ResponseStreamWrapper.Status.of(status, statusMessage, metadata))); - listeners.clear(); - }); - client.onMessage(message -> { - listeners.get(DATA_EVENT_LISTENER_NAME).forEach((item, index) -> item.call(null, message)); - }); - client.onHeaders(headers -> { - listeners.get(HEADERS_EVENT_LISTENER_NAME) - .forEach((item, index) -> item.call(null, headers)); - }); - client.start(metadata); - - return new BidirectionalStream() { - @Override - public void cancel() { - listeners.clear(); - client.close(); - } - - @Override - public void end() { - client.finishSend(); - } - - @Override - public BidirectionalStream on(String type, - Function handler) { - listeners.get(type).push(handler); - return this; - } - - @Override - public BidirectionalStream write( - HandshakeRequest message) { - client.send(message); - return this; - } - }; - }, - (first, metadata) -> { - Map> listeners = listenerMap(); - io.deephaven.javascript.proto.dhinternal.grpcweb.grpc.InvokeRpcOptions props = - Js.cast(InvokeRpcOptions.create()); - props.setRequest(first); - props.setHost(connection.browserFlightServiceClient().serviceHost); - props.setMetadata(metadata); - props.setTransport(null);// ts doesnt expose these two, stick with defaults for now - props.setDebug(false); - props.setOnMessage(responseMessage -> { - listeners.get(DATA_EVENT_LISTENER_NAME) - .forEach((item, index) -> item.call(null, responseMessage)); - }); - props.setOnEnd((status, statusMessage, trailers) -> { - listeners.get(STATUS_EVENT_LISTENER_NAME).forEach( - (item, index) -> item.call(null, - ResponseStreamWrapper.Status.of(status, statusMessage, metadata))); - listeners.get(END_EVENT_LISTENER_NAME).forEach( - (item, index) -> item.call(null, - ResponseStreamWrapper.Status.of(status, statusMessage, metadata))); - listeners.clear(); - }); - props.setOnHeaders(headers -> { - listeners.get(HEADERS_EVENT_LISTENER_NAME) - .forEach((item, index) -> item.call(null, headers)); - }); - Request client = Grpc.invoke.onInvoke(BrowserFlightService.OpenHandshake, props); - - return new ResponseStream() { - @Override - public void cancel() { - listeners.clear(); - client.getClose().onInvoke(); - } - - @Override - public ResponseStream on(String type, Function handler) { - listeners.get(type).push(handler); - return this; - } - }; - }, - (next, headers, callback) -> connection.browserFlightServiceClient().nextHandshake(next, headers, - callback::apply), - new HandshakeRequest()); - } - - private static Map> listenerMap() { - Map> listeners = new HashMap<>(); - listeners.put(DATA_EVENT_LISTENER_NAME, new JsArray<>()); - listeners.put(END_EVENT_LISTENER_NAME, new JsArray<>()); - listeners.put(STATUS_EVENT_LISTENER_NAME, new JsArray<>()); - listeners.put(HEADERS_EVENT_LISTENER_NAME, new JsArray<>()); - return listeners; - } -} diff --git a/web/client-api/src/main/java/io/deephaven/web/client/api/grpc/UnaryWithHeaders.java b/web/client-api/src/main/java/io/deephaven/web/client/api/grpc/UnaryWithHeaders.java new file mode 100644 index 00000000000..2b82a40d2b8 --- /dev/null +++ b/web/client-api/src/main/java/io/deephaven/web/client/api/grpc/UnaryWithHeaders.java @@ -0,0 +1,44 @@ +// +// Copyright (c) 2016-2024 Deephaven Data Labs and Patent Pending +// +package io.deephaven.web.client.api.grpc; + +import elemental2.promise.IThenable; +import elemental2.promise.Promise; +import io.deephaven.javascript.proto.dhinternal.grpcweb.Grpc; +import io.deephaven.javascript.proto.dhinternal.grpcweb.grpc.Code; +import io.deephaven.javascript.proto.dhinternal.grpcweb.unary.UnaryOutput; +import io.deephaven.javascript.proto.dhinternal.grpcweb.unary.UnaryRpcOptions; +import io.deephaven.web.client.api.WorkerConnection; + +public class UnaryWithHeaders { + + /** + * Improbable-eng's grpc-web implementation doesn't pass headers to api callers - this changes the contract a bit so + * that we can get a typed UnaryOutput with the headers/trailers intact. + * + * @param connection provides access to the metadata and the server url + * @param methodDescriptor the service method to invoke + * @return a promise that will resolve to the response plus headers/trailers, or reject with the headers/trailers + * @param type of the message object + */ + public static Promise> call(WorkerConnection connection, Object methodDescriptor, + Req request) { + return new Promise<>((resolve, reject) -> { + UnaryRpcOptions props = UnaryRpcOptions.create(); + props.setHost(connection.configServiceClient().serviceHost); + props.setMetadata(connection.metadata()); + props.setTransport(null);// ts doesn't expose these two, stick with defaults for now + props.setDebug(false); + props.setOnEnd(response -> { + if (response.getStatus() != Code.OK) { + reject.onInvoke(response); + } else { + resolve.onInvoke((IThenable>) response); + } + }); + props.setRequest(request); + Grpc.unary.onInvoke(methodDescriptor, props); + }); + } +}