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

Improve access control check for MV #18371

Merged
merged 3 commits into from
Aug 1, 2023
Merged

Improve access control check for MV #18371

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
15a2af7
Create views correctly in TestAccessControl
homar Jul 21, 2023
775cb05
Check materialized view access when no columns selected
homar Jul 21, 2023
c103210
Add more checks to TestAccessControl
homar Aug 1, 2023
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
1 change: 1 addition & 0 deletions core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2193,6 +2193,7 @@ protected Scope visitTable(Table table, Optional<Scope> scope)
Optional<MaterializedViewDefinition> optionalMaterializedView = metadata.getMaterializedView(session, name);
if (optionalMaterializedView.isPresent()) {
MaterializedViewDefinition materializedViewDefinition = optionalMaterializedView.get();
analysis.addEmptyColumnReferencesForTable(accessControl, session.getIdentity(), name);
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add a test to TestAccessControl? This is a generic change for engine so we should have generic test coverage. I would even consider removing iceberg tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add a test there but I'd prefer to leave an Iceberg test as it exactly covers what was the original problem

Copy link
Contributor

Choose a reason for hiding this comment

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

And perhaps also tests for regular views (if there aren't already), to make sure that they don't have the same issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

consider it done

Copy link
Member

@martint martint Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What’s the original problem? Please add motivation and details in the PR description.

Copy link
Member

Choose a reason for hiding this comment

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

Improve access control check for views

change commit message to "Check materialized view access when no columns selected"

if (isMaterializedViewSufficientlyFresh(session, name, materializedViewDefinition)) {
// If materialized view is sufficiently fresh with respect to its grace period, answer the query using the storage table
QualifiedName storageName = getMaterializedViewStorageTableName(materializedViewDefinition)
Expand Down
28 changes: 24 additions & 4 deletions testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,15 +158,15 @@ protected SystemAccessControl delegate()
})
Copy link
Member

Choose a reason for hiding this comment

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

Create views correctly

let's explain why it was incorrect

.withGetViews((connectorSession, prefix) -> {
ConnectorViewDefinition definitionRunAsDefiner = new ConnectorViewDefinition(
"select 1",
Copy link
Member

Choose a reason for hiding this comment

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

Use capital letters for keywords

"SELECT 1 AS test",
Optional.of("mock"),
Optional.of("default"),
ImmutableList.of(new ConnectorViewDefinition.ViewColumn("test", BIGINT.getTypeId(), Optional.empty())),
Optional.of("comment"),
Optional.of("admin"),
false);
ConnectorViewDefinition definitionRunAsInvoker = new ConnectorViewDefinition(
"select 1",
"SELECT 1 AS test",
Optional.of("mock"),
Optional.of("default"),
ImmutableList.of(new ConnectorViewDefinition.ViewColumn("test", BIGINT.getTypeId(), Optional.empty())),
Expand All @@ -182,7 +182,7 @@ protected SystemAccessControl delegate()
public Map<SchemaTableName, ConnectorMaterializedViewDefinition> apply(ConnectorSession session, SchemaTablePrefix schemaTablePrefix)
{
ConnectorMaterializedViewDefinition materializedViewDefinition = new ConnectorMaterializedViewDefinition(
"select 1",
"SELECT 1 AS test",
Optional.empty(),
Optional.empty(),
Optional.empty(),
Expand Down Expand Up @@ -253,7 +253,27 @@ public void testAccessControl()
assertAccessAllowed("SELECT name AS my_alias FROM nation", privilege("my_alias", SELECT_COLUMN));
assertAccessAllowed("SELECT my_alias from (SELECT name AS my_alias FROM nation)", privilege("my_alias", SELECT_COLUMN));
assertAccessDenied("SELECT name AS my_alias FROM nation", "Cannot select from columns \\[name] in table .*.nation.*", privilege("nation.name", SELECT_COLUMN));

assertAccessAllowed("SELECT 1 FROM mock.default.test_materialized_view");
Copy link
Member

Choose a reason for hiding this comment

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

add a test also for SELECT count(*) FROM mock.default.test_materialized_view

Copy link
Member

@findepi findepi Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

does this add anything on top of SELECT 1 FROM ..?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so, but that assumption is based on knowledge how semantic analyzer works. It is a another way of reading no column data, so maybe it could trigger different execution path too.

Copy link
Member

Choose a reason for hiding this comment

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

fine, but add a code comment explaining why we deem it worthwhile to fire up these two test queries, otherwise someone can just remove them saying they are redundant (because, in fact, they are)

assertAccessDenied("SELECT 1 FROM mock.default.test_materialized_view", "Cannot select from columns.*", privilege("test_materialized_view", SELECT_COLUMN));
assertAccessAllowed("SELECT * FROM mock.default.test_materialized_view");
assertAccessDenied("SELECT * FROM mock.default.test_materialized_view", "Cannot select from columns.*", privilege("test_materialized_view", SELECT_COLUMN));
assertAccessAllowed("SELECT 1 FROM mock.default.test_view_definer");
assertAccessDenied("SELECT 1 FROM mock.default.test_view_definer", "Cannot select from columns.*", privilege("test_view_definer", SELECT_COLUMN));
assertAccessAllowed("SELECT * FROM mock.default.test_view_definer");
assertAccessDenied("SELECT * FROM mock.default.test_view_definer", "Cannot select from columns.*", privilege("test_view_definer", SELECT_COLUMN));
assertAccessAllowed("SELECT 1 FROM mock.default.test_view_invoker");
assertAccessDenied("SELECT 1 FROM mock.default.test_view_invoker", "Cannot select from columns.*", privilege("test_view_invoker", SELECT_COLUMN));
assertAccessAllowed("SELECT * FROM mock.default.test_view_invoker");
assertAccessDenied("SELECT * FROM mock.default.test_view_invoker", "Cannot select from columns.*", privilege("test_view_invoker", SELECT_COLUMN));
findepi marked this conversation as resolved.
Show resolved Hide resolved
// with current implementation this next block of checks is redundant to `SELECT 1 FROM ..`, but it is not obvious unless details of how
// semantics analyzer works are known
assertAccessAllowed("SELECT count(*) FROM mock.default.test_materialized_view");
assertAccessDenied("SELECT count(*) FROM mock.default.test_materialized_view", "Cannot select from columns.*", privilege("test_materialized_view", SELECT_COLUMN));
assertAccessAllowed("SELECT count(*) FROM mock.default.test_view_invoker");
assertAccessDenied("SELECT count(*) FROM mock.default.test_view_invoker", "Cannot select from columns.*", privilege("test_view_invoker", SELECT_COLUMN));
assertAccessAllowed("SELECT count(*) FROM mock.default.test_view_definer");
assertAccessDenied("SELECT count(*) FROM mock.default.test_view_definer", "Cannot select from columns.*", privilege("test_view_definer", SELECT_COLUMN));

assertAccessDenied(
"SELECT orders.custkey, lineitem.quantity FROM orders JOIN lineitem USING (orderkey)",
"Cannot select from columns \\[orderkey, custkey] in table .*",
Expand Down