Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ErrorTransformer: prevent NPE #5633

Merged
merged 3 commits into from
Jun 21, 2024
Merged

ErrorTransformer: prevent NPE #5633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3af8830
ErrorTransformer: prevent NPE
nbauernfeind Jun 18, 2024
7d1b9d8
Ryan's feedback
nbauernfeind Jun 18, 2024
21e6602
consistent log format
nbauernfeind Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions server/src/main/java/io/deephaven/server/appmode/ApplicationTicketResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand All @@ -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) {
nbauernfeind marked this conversation as resolved.
Show resolved Hide resolved
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);
}
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand Down
36 changes: 24 additions & 12 deletions server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 =
nbauernfeind marked this conversation as resolved.
Show resolved Hide resolved
TicketRouter.getFlightInfo(transformed, descriptor, flightTicketForName(scopeName));

return SessionState.wrapAsExport(flightInfo);
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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");
}
}
21 changes: 21 additions & 0 deletions ...src/main/java/io/deephaven/server/hierarchicaltable/HierarchicalTableServiceGrpcImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
Expand Down Expand Up @@ -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;
});
Expand Down Expand Up @@ -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;
});
Expand Down Expand Up @@ -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;
});
Expand Down Expand Up @@ -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);
Expand Down
8 changes: 8 additions & 0 deletions ...r/src/main/java/io/deephaven/server/partitionedtable/PartitionedTableServiceGrpcImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
});
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/io/deephaven/server/session/ExportTicketResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
});
}

Expand Down
31 changes: 20 additions & 11 deletions server/src/main/java/io/deephaven/server/session/SharedTicketResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
});
}

Expand Down Expand Up @@ -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);
rcaudy marked this conversation as resolved.
Show resolved Hide resolved
if (result == null) {
throw newNotFoundSRE(logId, toHexString(sharedId));
}
return result;
});
}

Expand Down Expand Up @@ -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));
nbauernfeind marked this conversation as resolved.
Show resolved Hide resolved
}
}
3 changes: 2 additions & 1 deletion server/src/main/java/io/deephaven/server/session/TicketResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
Loading