Skip to content

Ensure first table matched rule in access control #12204

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

Merged
Praveen2112 merged 1 commit into from
May 9, 2022
Merged

Ensure first table matched rule in access control #12204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 6 additions & 4 deletions ...no-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedAccessControl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,9 +602,10 @@ public List<ViewExpression> getRowFilters(ConnectorSecurityContext context, Sche
return tableRules.stream()
.filter(rule -> rule.matches(identity.getUser(), identity.getEnabledSystemRoles(), identity.getGroups(), tableName))
.map(rule -> rule.getFilter(identity.getUser(), catalogName, tableName.getSchemaName()))
.flatMap(Optional::stream)
// we return the first one we find
.limit(1)
.findFirst()
.stream()
.flatMap(Optional::stream)
.collect(toImmutableList());
}

Expand All @@ -619,9 +620,10 @@ public List<ViewExpression> getColumnMasks(ConnectorSecurityContext context, Sch
return tableRules.stream()
.filter(rule -> rule.matches(identity.getUser(), identity.getEnabledSystemRoles(), identity.getGroups(), tableName))
.map(rule -> rule.getColumnMask(identity.getUser(), catalogName, tableName.getSchemaName(), columnName))
.flatMap(Optional::stream)
// we return the first one we find
.limit(1)
.findFirst()
.stream()
.flatMap(Optional::stream)
.collect(toImmutableList());
}

Expand Down
10 changes: 6 additions & 4 deletions ...gin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedSystemAccessControl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,9 +961,10 @@ public List<ViewExpression> getRowFilters(SystemSecurityContext context, Catalog
return tableRules.stream()
.filter(rule -> rule.matches(identity.getUser(), identity.getEnabledRoles(), identity.getGroups(), table))
.map(rule -> rule.getFilter(identity.getUser(), table.getCatalogName(), tableName.getSchemaName()))
.flatMap(Optional::stream)
// we return the first one we find
.limit(1)
.findFirst()
.stream()
.flatMap(Optional::stream)
.collect(toImmutableList());
}

Expand All @@ -979,9 +980,10 @@ public List<ViewExpression> getColumnMasks(SystemSecurityContext context, Catalo
return tableRules.stream()
.filter(rule -> rule.matches(identity.getUser(), identity.getEnabledRoles(), identity.getGroups(), table))
.map(rule -> rule.getColumnMask(identity.getUser(), table.getCatalogName(), table.getSchemaTableName().getSchemaName(), columnName))
.flatMap(Optional::stream)
// we return the first one we find
.limit(1)
.findFirst()
.stream()
.flatMap(Optional::stream)
.collect(toImmutableList());
}

Expand Down
73 changes: 73 additions & 0 deletions ...lugin-toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedAccessControl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
package io.trino.plugin.base.security;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import io.trino.plugin.base.CatalogName;
Expand All @@ -26,6 +27,7 @@
import io.trino.spi.security.PrincipalType;
import io.trino.spi.security.Privilege;
import io.trino.spi.security.TrinoPrincipal;
import io.trino.spi.security.ViewExpression;
import io.trino.spi.type.Type;
import org.testng.Assert.ThrowingRunnable;
import org.testng.annotations.DataProvider;
Expand All @@ -34,6 +36,7 @@
import java.io.File;
import java.net.URISyntaxException;
import java.util.EnumSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
Expand All @@ -42,6 +45,7 @@
import static com.google.common.io.Resources.getResource;
import static io.trino.spi.security.Privilege.UPDATE;
import static io.trino.spi.testing.InterfaceTestUtils.assertAllMethodsOverridden;
import static io.trino.spi.type.VarcharType.VARCHAR;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.testng.Assert.assertEquals;

Expand Down Expand Up @@ -349,6 +353,75 @@ public void testTableRules()
assertDenied(() -> accessControl.checkCanSetViewAuthorization(ALICE, bobTable, new TrinoPrincipal(PrincipalType.USER, "some_user")));
}

@Test
public void testTableRulesForMixedGroupUsers()
{
SchemaTableName myTable = new SchemaTableName("my_schema", "my_table");

ConnectorAccessControl accessControl = createAccessControl("table-mixed-groups.json");

ConnectorSecurityContext userGroup1Group2 = user("user_1_2", ImmutableSet.of("group1", "group2"));
ConnectorSecurityContext userGroup2 = user("user_2", ImmutableSet.of("group2"));

accessControl.checkCanCreateTable(userGroup1Group2, myTable, Map.of());
accessControl.checkCanInsertIntoTable(userGroup1Group2, myTable);
accessControl.checkCanDeleteFromTable(userGroup1Group2, myTable);
accessControl.checkCanDropTable(userGroup1Group2, myTable);
accessControl.checkCanSelectFromColumns(userGroup1Group2, myTable, ImmutableSet.of());
assertEquals(
accessControl.getColumnMasks(userGroup1Group2, myTable, "col_a", VARCHAR),
ImmutableList.of());
assertEquals(
accessControl.getRowFilters(userGroup1Group2, myTable),
ImmutableList.of());

assertDenied(() -> accessControl.checkCanCreateTable(userGroup2, myTable, Map.of()));
assertDenied(() -> accessControl.checkCanInsertIntoTable(userGroup2, myTable));
assertDenied(() -> accessControl.checkCanDeleteFromTable(userGroup2, myTable));
assertDenied(() -> accessControl.checkCanDropTable(userGroup2, myTable));
accessControl.checkCanSelectFromColumns(userGroup2, myTable, ImmutableSet.of());
assertViewExpressionEquals(
accessControl.getColumnMasks(userGroup2, myTable, "col_a", VARCHAR),
new ViewExpression(userGroup2.getIdentity().getUser(), Optional.of("test_catalog"), Optional.of("my_schema"), "'mask_a'"));
assertEquals(
accessControl.getRowFilters(userGroup2, myTable),
ImmutableList.of());

ConnectorSecurityContext userGroup1Group3 = user("user_1_3", ImmutableSet.of("group1", "group3"));
ConnectorSecurityContext userGroup3 = user("user_3", ImmutableSet.of("group3"));

accessControl.checkCanCreateTable(userGroup1Group3, myTable, Map.of());
accessControl.checkCanInsertIntoTable(userGroup1Group3, myTable);
accessControl.checkCanDeleteFromTable(userGroup1Group3, myTable);
accessControl.checkCanDropTable(userGroup1Group3, myTable);
accessControl.checkCanSelectFromColumns(userGroup1Group3, myTable, ImmutableSet.of());
assertEquals(
accessControl.getColumnMasks(userGroup1Group3, myTable, "col_a", VARCHAR),
ImmutableList.of());

assertDenied(() -> accessControl.checkCanCreateTable(userGroup3, myTable, Map.of()));
assertDenied(() -> accessControl.checkCanInsertIntoTable(userGroup3, myTable));
assertDenied(() -> accessControl.checkCanDeleteFromTable(userGroup3, myTable));
assertDenied(() -> accessControl.checkCanDropTable(userGroup3, myTable));
accessControl.checkCanSelectFromColumns(userGroup3, myTable, ImmutableSet.of());
assertViewExpressionEquals(
accessControl.getColumnMasks(userGroup3, myTable, "col_a", VARCHAR),
new ViewExpression(userGroup3.getIdentity().getUser(), Optional.of("test_catalog"), Optional.of("my_schema"), "'mask_a'"));
assertViewExpressionEquals(
accessControl.getRowFilters(userGroup3, myTable),
new ViewExpression(userGroup3.getIdentity().getUser(), Optional.of("test_catalog"), Optional.of("my_schema"), "country='US'"));
}

private static void assertViewExpressionEquals(List<ViewExpression> result, ViewExpression expected)
{
assertEquals(result.size(), 1);
ViewExpression actual = result.get(0);
assertEquals(actual.getIdentity(), expected.getIdentity(), "Identity");
assertEquals(actual.getCatalog(), expected.getCatalog(), "Catalog");
assertEquals(actual.getSchema(), expected.getSchema(), "Schema");
assertEquals(actual.getExpression(), expected.getExpression(), "Expression");
}

@Test
public void testTableFilter()
{
Expand Down
44 changes: 44 additions & 0 deletions ...toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedSystemAccessControl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -687,6 +687,50 @@ public void testTableRulesForCheckCanRenameColumn()
assertAccessDenied(() -> accessControl.checkCanRenameColumn(BOB, new CatalogSchemaTableName("some-catalog", "bobschema", "bobtable")), RENAME_COLUMNS_ACCESS_DENIED_MESSAGE);
}

@Test
public void testTableRulesForMixedGroupUsers()
{
SystemAccessControl accessControl = newFileBasedSystemAccessControl("file-based-system-access-table-mixed-groups.json");

SystemSecurityContext userGroup1Group2 = new SystemSecurityContext(Identity.forUser("user_1_2")
.withGroups(ImmutableSet.of("group1", "group2")).build(), Optional.empty());
SystemSecurityContext userGroup2 = new SystemSecurityContext(Identity.forUser("user_2")
.withGroups(ImmutableSet.of("group2")).build(), Optional.empty());

assertEquals(
accessControl.getColumnMasks(
userGroup1Group2,
new CatalogSchemaTableName("some-catalog", "my_schema", "my_table"),
"col_a",
VARCHAR),
ImmutableList.of());

assertViewExpressionEquals(
accessControl.getColumnMasks(
userGroup2,
new CatalogSchemaTableName("some-catalog", "my_schema", "my_table"),
"col_a",
VARCHAR),
new ViewExpression(userGroup2.getIdentity().getUser(), Optional.of("some-catalog"), Optional.of("my_schema"), "'mask_a'"));

SystemSecurityContext userGroup1Group3 = new SystemSecurityContext(Identity.forUser("user_1_3")
.withGroups(ImmutableSet.of("group1", "group3")).build(), Optional.empty());
SystemSecurityContext userGroup3 = new SystemSecurityContext(Identity.forUser("user_3")
.withGroups(ImmutableSet.of("group3")).build(), Optional.empty());

assertEquals(
accessControl.getRowFilters(
userGroup1Group3,
Copy link
Copy Markdown
Member

@martint martint May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting edge case. The user belongs to group1 and group3, but the table only has a filter for group3. I'm not quite sure what the correct semantics should be here.

For instance, the first entry may be about allowing users in group1 (e.g., employees in some department) to access a table, but the second entry might be about limiting what users in group3 can see (e.g., employees in some specific geography). In that case, it should use all filters that are present.

Let me think more about it.

Copy link
Copy Markdown
Member Author

@guyco33 guyco33 May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that in all cases we should pick the first matching rule. This way we can control the way we want to limit access.
For example, in cases that we want to implement a more restrictive approach, we will place the table rules with filters and masks before the rules without them.

Copy link
Copy Markdown
Contributor

@ksobolew ksobolew May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally would lean towards returning all matching rules. But I'm not very familiar with file-based AC and its use cases so I'm not sure what's actually expected.

I think that in all cases we should pick the first matching rule. This way we can control the way we want to limit access.

But doesn't it mean that if there is a second matching rule, it will never actually be returned and is entirely redundant?

Copy link
Copy Markdown
Member Author

@guyco33 guyco33 May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Safe logic for returning all matching rules must take into account the rules without filters and maskings.
Rules order is important, so possible solution might be retrieving all matched filters / maskings rules until a rule without filter / masks is matched.
Anyway it's a subject for different PR

new CatalogSchemaTableName("some-catalog", "my_schema", "my_table")),
ImmutableList.of());

assertViewExpressionEquals(
accessControl.getRowFilters(
userGroup3,
new CatalogSchemaTableName("some-catalog", "my_schema", "my_table")),
new ViewExpression(userGroup3.getIdentity().getUser(), Optional.of("some-catalog"), Optional.of("my_schema"), "country='US'"));
}

@Test
public void testCheckCanSetTableAuthorizationForAdmin()
{
Expand Down
46 changes: 46 additions & 0 deletions lib/trino-plugin-toolkit/src/test/resources/file-based-system-access-table-mixed-groups.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
{
"tables": [
{
"catalog": "some-catalog",
"schema": "my_schema",
"table": "my_table",
"privileges": ["SELECT", "GRANT_SELECT", "OWNERSHIP", "INSERT", "DELETE", "UPDATE"],
"group": "group1"
},
{
"catalog": "some-catalog",
"schema": "my_schema",
"table": "my_table",
"columns" : [
{
"name": "col_a",
"mask": "'mask_a'"
},
{
"name": "col_b",
"mask": "'mask_b'"
}
],
"privileges": ["SELECT", "GRANT_SELECT"],
"group": "group2"
},
{
"catalog": "some-catalog",
"schema": "my_schema",
"table": "my_table",
"columns" : [
{
"name": "col_a",
"mask": "'mask_a'"
},
{
"name": "col_b",
"mask": "'mask_b'"
}
],
"privileges": ["SELECT", "GRANT_SELECT"],
"filter": "country='US'",
"group": "group3"
}
]
}
43 changes: 43 additions & 0 deletions lib/trino-plugin-toolkit/src/test/resources/table-mixed-groups.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
{
"tables": [
{
"schema": "my_schema",
"table": "my_table",
"privileges": ["SELECT", "GRANT_SELECT", "OWNERSHIP", "INSERT", "DELETE", "UPDATE"],
"group": "group1"
},
{
"schema": "my_schema",
"table": "my_table",
"columns" : [
{
"name": "col_a",
"mask": "'mask_a'"
},
{
"name": "col_b",
"mask": "'mask_b'"
}
],
"privileges": ["SELECT", "GRANT_SELECT"],
"group": "group2"
},
{
"schema": "my_schema",
"table": "my_table",
"columns" : [
{
"name": "col_a",
"mask": "'mask_a'"
},
{
"name": "col_b",
"mask": "'mask_b'"
}
],
"privileges": ["SELECT", "GRANT_SELECT"],
"filter": "country='US'",
"group": "group3"
}
]
}