Improve access control check for MV #18371

homar · 2023-07-21T11:23:41Z

Description

Add additional checks for access control for queries on materialized views that don't touch underlying data.

Additional context and related issues

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

kokosing · 2023-07-21T11:25:08Z

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java

@@ -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);


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.

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

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

consider it done

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

ksobolew

I would consider the Iceberg tests redundant, too. For someone looking from the outside, it would seem strange why Iceberg and not all connectors?

homar · 2023-07-21T12:25:28Z

I would consider the Iceberg tests redundant, too. For someone looking from the outside, it would seem strange why Iceberg and not all connectors?

ok i will remove them

findepi · 2023-07-21T13:54:54Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

@@ -158,15 +158,15 @@ protected SystemAccessControl delegate()
                })
                .withGetViews((connectorSession, prefix) -> {
                    ConnectorViewDefinition definitionRunAsDefiner = new ConnectorViewDefinition(
-                            "select 1",
+                            "select 1 as test",


Why? separate commit?

It wont work without these changes so i think it can be in the same commit

this looks MV unrelated, so it looks unrelated to commit title, right?

Please tell me if current version is better

kokosing · 2023-07-21T20:40:32Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

@@ -158,15 +158,15 @@ protected SystemAccessControl delegate()
                })
                .withGetViews((connectorSession, prefix) -> {
                    ConnectorViewDefinition definitionRunAsDefiner = new ConnectorViewDefinition(
-                            "select 1",


Use capital letters for keywords

kokosing · 2023-07-21T20:40:55Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

@@ -158,15 +158,15 @@ protected SystemAccessControl delegate()
                })


Create views correctly

let's explain why it was incorrect

Not having a named column resulted in SemanticException claiming that View is in invalid state: "a column of type INTEGER projected from query view at position 1 has no name"

homar · 2023-07-21T21:25:42Z

@kokosing commit message updated, upper case used

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

kokosing · 2023-07-31T13:46:51Z

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

@@ -253,6 +253,18 @@ 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");


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

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

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.

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)

findepi · 2023-08-01T09:48:59Z

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java

@@ -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);


Improve access control check for views

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

cla-bot bot added the cla-signed label Jul 21, 2023

kokosing reviewed Jul 21, 2023

View reviewed changes

homar force-pushed the homar/add_missing_access_control_check branch from 42ee909 to 8a4d41e Compare July 21, 2023 11:53

homar requested review from kokosing and ksobolew July 21, 2023 11:53

homar force-pushed the homar/add_missing_access_control_check branch from 8a4d41e to bfda1d1 Compare July 21, 2023 12:04

ksobolew approved these changes Jul 21, 2023

View reviewed changes

homar force-pushed the homar/add_missing_access_control_check branch from bfda1d1 to aa115bb Compare July 21, 2023 12:32

homar requested a review from ksobolew July 21, 2023 12:32

ksobolew approved these changes Jul 21, 2023

View reviewed changes

findepi approved these changes Jul 21, 2023

View reviewed changes

homar force-pushed the homar/add_missing_access_control_check branch from aa115bb to ac39e50 Compare July 21, 2023 15:43

kokosing approved these changes Jul 21, 2023

View reviewed changes

Create views correctly in TestAccessControl

15a2af7

Not having a named column resulted in SemanticException claiming that View is in invalid state: "a column of type INTEGER projected from query view at position 1 has no name"

homar force-pushed the homar/add_missing_access_control_check branch from ac39e50 to 311c39f Compare July 21, 2023 21:24

homar requested a review from kokosing July 21, 2023 21:25

hashhar approved these changes Jul 26, 2023

View reviewed changes

findepi approved these changes Jul 31, 2023

View reviewed changes

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java Show resolved Hide resolved

kokosing approved these changes Jul 31, 2023

View reviewed changes

homar force-pushed the homar/add_missing_access_control_check branch from 311c39f to 74d8934 Compare August 1, 2023 09:00

homar requested review from findepi and kokosing August 1, 2023 09:00

homar force-pushed the homar/add_missing_access_control_check branch from 74d8934 to e7945f7 Compare August 1, 2023 09:41

findepi approved these changes Aug 1, 2023

View reviewed changes

homar added 2 commits August 1, 2023 11:51

Check materialized view access when no columns selected

775cb05

Add more checks to TestAccessControl

c103210

homar force-pushed the homar/add_missing_access_control_check branch from e7945f7 to c103210 Compare August 1, 2023 09:51

findepi merged commit fa4a067 into trinodb:master Aug 1, 2023

github-actions bot added this to the 423 milestone Aug 1, 2023

findepi mentioned this pull request Aug 1, 2023

Fix code style error #18484

Merged

colebow mentioned this pull request Aug 2, 2023

Add Trino 423 release notes #18496

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve access control check for MV #18371

Improve access control check for MV #18371

homar commented Jul 21, 2023 •

edited

Loading

kokosing Jul 21, 2023

homar Jul 21, 2023

ksobolew Jul 21, 2023

homar Jul 21, 2023

martint Jul 31, 2023 •

edited

Loading

ksobolew left a comment

homar commented Jul 21, 2023

findepi Jul 21, 2023

homar Jul 21, 2023

findepi Jul 21, 2023

homar Jul 21, 2023

kokosing Jul 21, 2023

kokosing Jul 21, 2023

homar commented Jul 21, 2023

kokosing Jul 31, 2023

findepi Jul 31, 2023 •

edited

Loading

kokosing Jul 31, 2023

findepi Jul 31, 2023

findepi Aug 1, 2023

		@@ -158,15 +158,15 @@ protected SystemAccessControl delegate()
		})

Improve access control check for MV #18371

Improve access control check for MV #18371

Conversation

homar commented Jul 21, 2023 • edited Loading

Description

Additional context and related issues

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martint Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

ksobolew left a comment

Choose a reason for hiding this comment

homar commented Jul 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homar commented Jul 21, 2023

Choose a reason for hiding this comment

findepi Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homar commented Jul 21, 2023 •

edited

Loading

martint Jul 31, 2023 •

edited

Loading

findepi Jul 31, 2023 •

edited

Loading