ErrorTransformer: prevent NPE (#5633)

deephaven · Jun 21, 2024 · a9ebacf · a9ebacf
1 parent 57941cd
commit a9ebacf
Show file tree

Hide file tree

Showing 7 changed files with 96 additions and 31 deletions.
diff --git a/server/src/main/java/io/deephaven/server/appmode/ApplicationTicketResolver.java b/server/src/main/java/io/deephaven/server/appmode/ApplicationTicketResolver.java
@@ -17,7 +17,9 @@
 import io.deephaven.server.session.SessionState;
 import io.deephaven.server.session.TicketResolverBase;
 import io.deephaven.server.session.TicketRouter;
+import io.grpc.StatusRuntimeException;
 import org.apache.arrow.flight.impl.Flight;
+import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
 import javax.inject.Inject;
@@ -79,10 +81,12 @@ private <T> SessionState.ExportObject<T> resolve(final AppFieldId id, final Stri
         }
         final Field<Object> field = id.app.getField(id.fieldName);
         if (field == null) {
-            throw Exceptions.statusRuntimeException(Code.NOT_FOUND,
-                    "Could not resolve '" + logId + "': field '" + getLogNameFor(id) + "' not found");
+            throw newNotFoundSRE(logId, id);
         }
         Object value = authorization.transform(field.value());
+        if (value == null) {
+            throw newNotFoundSRE(logId, id);
+        }
         // noinspection unchecked
         return SessionState.wrapAsExport((T) value);
     }
@@ -100,16 +104,18 @@ public SessionState.ExportObject<Flight.FlightInfo> flightInfoFor(
         synchronized (id.app) {
             Field<?> field = id.app.getField(id.fieldName);
             if (field == null) {
-                throw Exceptions.statusRuntimeException(Code.NOT_FOUND,
-                        "Could not resolve '" + logId + "': field '" + getLogNameFor(id) + "' not found");
+                throw newNotFoundSRE(logId, id);
             }
             Object value = field.value();
             if (value instanceof Table) {
+                // may return null if the table is not authorized
                 value = authorization.transform(value);
+            }
+
+            if (value instanceof Table) {
                 info = TicketRouter.getFlightInfo((Table) value, descriptor, flightTicketForName(id.app, id.fieldName));
             } else {
-                throw Exceptions.statusRuntimeException(Code.NOT_FOUND,
-                        "Could not resolve '" + logId + "': field '" + getLogNameFor(id) + "' is not a flight");
+                throw newNotFoundSRE(logId, id);
             }
         }
 
@@ -151,7 +157,10 @@ public void forAllFlightInfo(@Nullable SessionState session, Consumer<Flight.Fli
             app.listFields().forEach(field -> {
                 Object value = field.value();
                 if (value instanceof Table) {
+                    // may return null if the table is not authorized
                     value = authorization.transform(value);
+                }
+                if (value instanceof Table) {
                     final Flight.FlightInfo info = TicketRouter.getFlightInfo((Table) value,
                             descriptorForName(app, field.name()), flightTicketForName(app, field.name()));
                     visitor.accept(info);
@@ -202,6 +211,11 @@ public static Flight.FlightDescriptor descriptorForName(final ApplicationState a
                 .build();
     }
 
+    private @NotNull StatusRuntimeException newNotFoundSRE(String logId, AppFieldId id) {
+        return Exceptions.statusRuntimeException(Code.NOT_FOUND,
+                "Could not resolve '" + logId + "': field '" + getLogNameFor(id) + "' not found");
+    }
+
     private AppFieldId appFieldIdFor(final ByteBuffer ticket, final String logId) {
         if (ticket == null) {
             throw Exceptions.statusRuntimeException(Code.FAILED_PRECONDITION,

diff --git a/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java b/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java
@@ -18,7 +18,9 @@
 import io.deephaven.server.session.SessionState;
 import io.deephaven.server.session.TicketResolverBase;
 import io.deephaven.server.session.TicketRouter;
+import io.grpc.StatusRuntimeException;
 import org.apache.arrow.flight.impl.Flight;
+import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
 import javax.inject.Inject;
@@ -51,19 +53,20 @@ public SessionState.ExportObject<Flight.FlightInfo> flightInfoFor(
         // there is no mechanism to wait for a scope variable to resolve; require that the scope variable exists now
         final String scopeName = nameForDescriptor(descriptor, logId);
 
-        QueryScope queryScope = ExecutionContext.getContext().getQueryScope();
-        Object scopeVar = queryScope.unwrapObject(queryScope.readParamValue(scopeName, null));
+        final QueryScope queryScope = ExecutionContext.getContext().getQueryScope();
+        final Object scopeVar = queryScope.unwrapObject(queryScope.readParamValue(scopeName, null));
         if (scopeVar == null) {
-            throw Exceptions.statusRuntimeException(Code.NOT_FOUND,
-                    "Could not resolve '" + logId + ": no table exists with name '" + scopeName + "'");
+            throw newNotFoundSRE(logId, scopeName);
         }
         if (!(scopeVar instanceof Table)) {
-            throw Exceptions.statusRuntimeException(Code.NOT_FOUND,
-                    "Could not resolve '" + logId + "': no table exists with name '" + scopeName + "'");
+            throw newNotFoundSRE(logId, scopeName);
         }
 
-        Table transformed = authorization.transform((Table) scopeVar);
-        Flight.FlightInfo flightInfo =
+        final Table transformed = authorization.transform((Table) scopeVar);
+        if (transformed == null) {
+            throw newNotFoundSRE(logId, scopeName);
+        }
+        final Flight.FlightInfo flightInfo =
                 TicketRouter.getFlightInfo(transformed, descriptor, flightTicketForName(scopeName));
 
         return SessionState.wrapAsExport(flightInfo);
@@ -72,8 +75,13 @@ public SessionState.ExportObject<Flight.FlightInfo> flightInfoFor(
     @Override
     public void forAllFlightInfo(@Nullable final SessionState session, final Consumer<Flight.FlightInfo> visitor) {
         final QueryScope queryScope = ExecutionContext.getContext().getQueryScope();
-        queryScope.toMap(queryScope::unwrapObject, (n, t) -> t instanceof Table).forEach((name, table) -> visitor
-                .accept(TicketRouter.getFlightInfo((Table) table, descriptorForName(name), flightTicketForName(name))));
+        queryScope.toMap(queryScope::unwrapObject, (n, t) -> t instanceof Table).forEach((name, table) -> {
+            final Table transformedTable = authorization.transform((Table) table);
+            if (transformedTable != null) {
+                visitor.accept(TicketRouter.getFlightInfo(
+                        transformedTable, descriptorForName(name), flightTicketForName(name)));
+            }
+        });
     }
 
     @Override
@@ -101,8 +109,7 @@ private <T> SessionState.ExportObject<T> resolve(final String scopeName, final S
         export = authorization.transform(export);
 
         if (export == null) {
-            return SessionState.wrapAsFailedExport(Exceptions.statusRuntimeException(Code.FAILED_PRECONDITION,
-                    "Could not resolve '" + logId + "': no variable exists with name '" + scopeName + "'"));
+            return SessionState.wrapAsFailedExport(newNotFoundSRE(logId, scopeName));
         }
 
         return SessionState.wrapAsExport(export);
@@ -268,4 +275,9 @@ public static Flight.FlightDescriptor ticketToDescriptor(final Flight.Ticket tic
     public static Flight.Ticket descriptorToTicket(final Flight.FlightDescriptor descriptor, final String logId) {
         return flightTicketForName(nameForDescriptor(descriptor, logId));
     }
+
+    private static @NotNull StatusRuntimeException newNotFoundSRE(String logId, String scopeName) {
+        return Exceptions.statusRuntimeException(Code.NOT_FOUND,
+                "Could not resolve '" + logId + ": variable '" + scopeName + "' not found");
+    }
 }
diff --git a/...src/main/java/io/deephaven/server/hierarchicaltable/HierarchicalTableServiceGrpcImpl.java b/...src/main/java/io/deephaven/server/hierarchicaltable/HierarchicalTableServiceGrpcImpl.java
@@ -105,6 +105,10 @@ public void rollup(
                                 aggregations, includeConstituents, groupByColumns);
 
                         final RollupTable transformedResult = authTransformation.transform(result);
+                        if (transformedResult == null) {
+                            throw Exceptions.statusRuntimeException(
+                                    Code.FAILED_PRECONDITION, "Not authorized to rollup hierarchical table");
+                        }
                         safelyComplete(responseObserver, RollupResponse.getDefaultInstance());
                         return transformedResult;
                     });
@@ -161,6 +165,10 @@ public void tree(
                                 identifierColumn.name(), parentIdentifierColumn.name());
 
                         final TreeTable transformedResult = authTransformation.transform(result);
+                        if (transformedResult == null) {
+                            throw Exceptions.statusRuntimeException(
+                                    Code.FAILED_PRECONDITION, "Not authorized to tree hierarchical table");
+                        }
                         safelyComplete(responseObserver, TreeResponse.getDefaultInstance());
                         return transformedResult;
                     });
@@ -262,6 +270,10 @@ public void apply(
                         }
 
                         final HierarchicalTable<?> transformedResult = authTransformation.transform(result);
+                        if (transformedResult == null) {
+                            throw Exceptions.statusRuntimeException(
+                                    Code.FAILED_PRECONDITION, "Not authorized to apply to hierarchical table");
+                        }
                         safelyComplete(responseObserver, HierarchicalTableApplyResponse.getDefaultInstance());
                         return transformedResult;
                     });
@@ -423,6 +435,10 @@ public void view(
                         }
 
                         final HierarchicalTableView transformedResult = authTransformation.transform(result);
+                        if (transformedResult == null) {
+                            throw Exceptions.statusRuntimeException(
+                                    Code.FAILED_PRECONDITION, "Not authorized to view hierarchical table");
+                        }
                         safelyComplete(responseObserver, HierarchicalTableViewResponse.getDefaultInstance());
                         return transformedResult;
                     });
@@ -478,6 +494,11 @@ public void exportSource(
                         authWiring.checkPermissionExportSource(session.getAuthContext(), request, List.of(result));
 
                         final Table transformedResult = authTransformation.transform(result);
+                        if (transformedResult == null) {
+                            throw Exceptions.statusRuntimeException(
+                                    Code.FAILED_PRECONDITION,
+                                    "Not authorized to export source from hierarchical table");
+                        }
                         final ExportedTableCreationResponse response =
                                 ExportUtil.buildTableCreationResponse(request.getResultTableId(), transformedResult);
                         safelyComplete(responseObserver, response);

diff --git a/...r/src/main/java/io/deephaven/server/partitionedtable/PartitionedTableServiceGrpcImpl.java b/...r/src/main/java/io/deephaven/server/partitionedtable/PartitionedTableServiceGrpcImpl.java
@@ -112,6 +112,10 @@ public void merge(
                             merged = partitionedTable.get().merge();
                         }
                         merged = authorizationTransformation.transform(merged);
+                        if (merged == null) {
+                            throw Exceptions.statusRuntimeException(
+                                    Code.FAILED_PRECONDITION, "Not authorized to merge table.");
+                        }
                         final ExportedTableCreationResponse response =
                                 buildTableCreationResponse(request.getResultId(), merged);
                         safelyComplete(responseObserver, response);
@@ -187,6 +191,10 @@ public void getTable(
                         table = authorizationTransformation.transform(table);
                         final ExportedTableCreationResponse response =
                                 buildTableCreationResponse(request.getResultId(), table);
+                        if (table == null) {
+                            throw Exceptions.statusRuntimeException(
+                                    Code.FAILED_PRECONDITION, "Not authorized to get table.");
+                        }
                         safelyComplete(responseObserver, response);
                         return table;
                     });

diff --git a/server/src/main/java/io/deephaven/server/session/ExportTicketResolver.java b/server/src/main/java/io/deephaven/server/session/ExportTicketResolver.java
@@ -55,7 +55,7 @@ public SessionState.ExportObject<Flight.FlightInfo> flightInfoFor(
                     }
 
                     throw Exceptions.statusRuntimeException(Code.NOT_FOUND,
-                            "Could not resolve '" + logId + "': flight '" + descriptor + "' is not a table");
+                            "Could not resolve '" + logId + "': flight '" + descriptor + "' not found");
                 });
     }
 

diff --git a/server/src/main/java/io/deephaven/server/session/SharedTicketResolver.java b/server/src/main/java/io/deephaven/server/session/SharedTicketResolver.java
@@ -15,6 +15,7 @@
 import io.deephaven.proto.util.Exceptions;
 import io.deephaven.proto.util.SharedTicketHelper;
 import io.deephaven.server.auth.AuthorizationProvider;
+import io.grpc.StatusRuntimeException;
 import org.apache.arrow.flight.impl.Flight;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -63,22 +64,22 @@ public SessionState.ExportObject<Flight.FlightInfo> flightInfoFor(
 
         SessionState.ExportObject<?> export = sharedVariables.get(sharedId);
         if (export == null) {
-            throw Exceptions.statusRuntimeException(Code.NOT_FOUND, String.format(
-                    "Could not resolve '%s': no shared ticket exists with id 0x%s", logId, toHexString(sharedId)));
+            throw newNotFoundSRE(logId, toHexString(sharedId));
         }
 
         return session.<Flight.FlightInfo>nonExport()
                 .require(export)
                 .submit(() -> {
-                    final Object result = export.get();
+                    Object result = export.get();
                     if (result instanceof Table) {
-                        final Table table = (Table) authorization.transform(result);
-                        return TicketRouter.getFlightInfo(table, descriptor,
+                        result = authorization.transform(result);
+                    }
+                    if (result instanceof Table) {
+                        return TicketRouter.getFlightInfo((Table) result, descriptor,
                                 FlightExportTicketHelper.descriptorToFlightTicket(descriptor, logId));
                     }
 
-                    throw Exceptions.statusRuntimeException(Code.NOT_FOUND, String.format(
-                            "Could not resolve '%s': flight '%s' is not a table", logId, descriptor));
+                    throw newNotFoundSRE(logId, toHexString(sharedId));
                 });
     }
 
@@ -109,16 +110,19 @@ private <T> SessionState.ExportObject<T> resolve(
         // noinspection unchecked
         final SessionState.ExportObject<T> sharedVar = (SessionState.ExportObject<T>) sharedVariables.get(sharedId);
         if (sharedVar == null) {
-            return SessionState.wrapAsFailedExport(Exceptions.statusRuntimeException(Code.NOT_FOUND, String.format(
-                    "Could not resolve '%s': no shared ticket exists with id '%s'", logId, toHexString(sharedId))));
+            return SessionState.wrapAsFailedExport(newNotFoundSRE(logId, toHexString(sharedId)));
         }
 
         // we need to wrap this in a new export object to hand off to the new session and defer checking permissions
         return session.<T>nonExport()
                 .require(sharedVar)
                 .submit(() -> {
-                    final T result = sharedVar.get();
-                    return authorization.transform(result);
+                    T result = sharedVar.get();
+                    result = authorization.transform(result);
+                    if (result == null) {
+                        throw newNotFoundSRE(logId, toHexString(sharedId));
+                    }
+                    return result;
                 });
     }
 
@@ -284,4 +288,9 @@ public static Flight.FlightDescriptor ticketToDescriptor(final Flight.Ticket tic
     public static Flight.Ticket descriptorToTicket(final Flight.FlightDescriptor descriptor, final String logId) {
         return flightTicketForId(idForDescriptor(descriptor, logId).toByteArray());
     }
+
+    private static @NotNull StatusRuntimeException newNotFoundSRE(String logId, String sharedId) {
+        return Exceptions.statusRuntimeException(Code.NOT_FOUND, String.format(
+                "Could not resolve '%s': ticket '%s' not found", logId, sharedId));
+    }
 }
diff --git a/server/src/main/java/io/deephaven/server/session/TicketResolver.java b/server/src/main/java/io/deephaven/server/session/TicketResolver.java
@@ -24,7 +24,8 @@ interface Authorization {
          *          transformations to requested resources.
          *
          * @param source the object to transform (such as by applying ACLs)
-         * @return an object that has been sanitized to be used by the current user
+         * @return an object that has been sanitized to be used by the current user; may return null if user does not
+         *         have access to the resource
          */
         <T> T transform(T source);