Skip to content

Commit

Permalink
Check access to masked columns in bulk when analyzing masks
Browse files Browse the repository at this point in the history
The `getColumnMasks` SPI call was added in order to reduce the number of
SPI calls in cases of tables with large number of columns, similar to
how `checkCanSelectFromColumns` does it. This is an important
performance optimization for access controls like OPA. But, once we
discover the column masks, we check whether the current session has
access to the masked columns - if not, we skip analyzing it further. The
problem was that each column mask was inspected in isolation, resulting
in multiple checks to `checkCanSelectFromColumns` with just a single
column. I believe this is contrary to the goals of the change which
introduced `getColumnMasks`.

This PR adds a fast path with a single check for all masked columns
instead of one check per each masked column. If that single check fails,
though, we enter the slow path, in which we analyze masks on all the
accessible columns to see if they reference any other columns. This is
relevant when the `hide-inaccessible-columns` option is set, in which
case we cannot assume that an inaccessible masked column will fail the
query.
  • Loading branch information
ksobolew committed Aug 19, 2024
1 parent 5286f64 commit e6d6654
Showing 1 changed file with 23 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2375,25 +2375,37 @@ private void checkStorageTableNotRedirected(QualifiedObjectName source)

private void analyzeFiltersAndMasks(Table table, QualifiedObjectName name, RelationType relationType, Scope accessControlScope)
{
ImmutableList.Builder<ColumnSchema> columnSchemaBuilder = ImmutableList.builder();
ImmutableMap.Builder<String, ColumnSchema> columnSchemaBuilder = ImmutableMap.builder();
for (int index = 0; index < relationType.getAllFieldCount(); index++) {
Field field = relationType.getFieldByIndex(index);
field.getName().ifPresent(fieldName -> columnSchemaBuilder.add(ColumnSchema.builder()
field.getName().ifPresent(fieldName -> columnSchemaBuilder.put(fieldName, ColumnSchema.builder()
.setName(fieldName)
.setType(field.getType())
.setHidden(field.isHidden())
.build()));
}
List<ColumnSchema> columnSchemas = columnSchemaBuilder.build();
Map<String, ColumnSchema> columnSchemas = columnSchemaBuilder.buildOrThrow();

Map<ColumnSchema, ViewExpression> masks = accessControl.getColumnMasks(session.toSecurityContext(), name, columnSchemas);
Map<ColumnSchema, ViewExpression> masks = accessControl.getColumnMasks(session.toSecurityContext(), name, ImmutableList.copyOf(columnSchemas.values()));

for (ColumnSchema columnSchema : columnSchemas) {
Optional.ofNullable(masks.get(columnSchema)).ifPresent(mask -> {
if (checkCanSelectFromColumn(name, columnSchema.getName())) {
// check for fast path: a single bulk check of all columns
if (checkCanSelectFromColumns(name, columnSchemas.keySet())) {
for (ColumnSchema columnSchema : columnSchemas.values()) {
Optional.ofNullable(masks.get(columnSchema)).ifPresent(mask -> {
analyzeColumnMask(session.getIdentity().getUser(), table, name, columnSchema, accessControlScope, mask);
}
});
});
}
}
else {
// slow path: if any column is inaccessible, we need to analyze all the accessible ones
// to see if they reference any other column(s)
for (ColumnSchema columnSchema : columnSchemas.values()) {
Optional.ofNullable(masks.get(columnSchema)).ifPresent(mask -> {
if (checkCanSelectFromColumns(name, ImmutableSet.of(columnSchema.getName()))) {
analyzeColumnMask(session.getIdentity().getUser(), table, name, columnSchema, accessControlScope, mask);
}
});
}
}

accessControl.getRowFilters(session.toSecurityContext(), name)
Expand All @@ -2412,10 +2424,10 @@ private void analyzeCheckConstraints(Table table, QualifiedObjectName name, Scop
}
}

private boolean checkCanSelectFromColumn(QualifiedObjectName name, String column)
private boolean checkCanSelectFromColumns(QualifiedObjectName name, Set<String> columns)
{
try {
accessControl.checkCanSelectFromColumns(session.toSecurityContext(), name, ImmutableSet.of(column));
accessControl.checkCanSelectFromColumns(session.toSecurityContext(), name, columns);
return true;
}
catch (AccessDeniedException e) {
Expand Down

0 comments on commit e6d6654

Please sign in to comment.