Verify SQL MERGE access control checks #13664

djsstarburst · 2022-08-13T15:42:01Z

Description

This commit adds a MERGE test in BaseConnectorTest that verifies
that in order execute a MERGE, you must have appropriate access
control rights. Specifically, if there is a source table, you must
have SELECT on the table columns referenced; if the MERGE does
inserts, you must have INSERT on the target table; if the MERGE
does deletes you must have DELETE on the target table; and if
the MERGE does updates, you must have UPDATE on the target
table.

Is this change a fix, improvement, new feature, refactoring, or other?

It is an added test.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

None of the above.

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

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

dain

Other than the formatting issue, looks good

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

electrum

These should move to TestAccessControl, as discussed offline. We don't need test data since we are only testing access control checks, not the merge functionality. I created #13665 which allows testing the positive case.

djsstarburst · 2022-08-13T20:31:11Z

@electrum suggested that I move this test to TestAccessControl. However, that test uses the Blackhole connector, which didn't support MERGE.

I added another commit to the start of my PR that moves the MERGE access control checks up to the top of StatementAnalysis.visitMerge. It a good change, but it's not sufficient to move the test to TestAccessControl. The problem is that we don't know which if any columns of the source and target tables will be SELECTed until StatementAnalysis.visitTable is called, and it make a call to get the merge rowId handle.

@electrum produced an ersatz implementation of MERGE for Blackhole, and maybe that makes sense as a followup. But at this point, I’m mildly in favor of merging my existing PR, now with the commit that does access checks earlier.

djsstarburst · 2022-08-13T20:33:08Z

I created #13665 which allows testing the positive case.

I don't see how that PR can work, because the body of storeMergedRows is empty.

electrum · 2022-08-13T23:52:09Z

It works because the Black Hole connector doesn't store or produce data (unless you ask it to produce nulls), so there is nothing to match or update. You can see that BlackHolePageSink ignores the provided page.

djsstarburst · 2022-08-14T00:50:50Z

I rebased on top of tip master, which has @electrum's ersatz implementation of MERGE for the Blackhole connector. That let me remove the test from BaseConnectorTest and move it to TestAccessControl, after I removed the final successful MERGE operation and verification, which Blackhole can't execute.

I think this now satisfies the concerns raised, and is ready to merge.

electrum · 2022-08-15T03:19:27Z

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

+                    .findFirst()
+                    .ifPresent(mergeCase -> accessControl.checkCanDeleteFromTable(session.toSecurityContext(), tableName));
+
+            ImmutableSet.Builder<String> allUpdateColumnNamesBuilder = ImmutableSet.builder();


Nit: we can use HashSet here with the variable named allUpdateColumnNames since we are just building this to pass to a function. Using an immutable builder is overkill and makes the code harder to read.

electrum · 2022-08-15T03:20:03Z

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

+
+        assertUpdate(format("CREATE TABLE %s (nation_name VARCHAR, region_name VARCHAR)", targetTable));
+
+        assertUpdate(format("INSERT INTO %s (nation_name, region_name) VALUES ('FRANCE', 'EUROPE'), ('ALGERIA', 'AFRICA'), ('GERMANY', 'EUROPE')", targetTable), 3);


No need to insert data, since we aren't testing the actual merge functionality, and the blackhole connector will discard the data anyway. (so it's confusing to the reader as to why we do it)

electrum · 2022-08-15T03:20:33Z

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

+        assertUpdate("DROP TABLE " + targetTable);
+    }
+
+    private void withPrivilegeDenied(String tableName, TestingPrivilegeType type, Runnable runnable)


We shouldn't need this method

electrum · 2022-08-15T03:21:26Z

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

+
+        // Show that without SELECT on the source table, the MERGE fails regardless of which case is included
+        for (String mergeCase : ImmutableList.of(deleteCase, updateCase, insertCase)) {
+            withPrivilegeDenied(sourceTable, SELECT_COLUMN, () ->


Use assertAccessDenied, like the other tests in this class.

electrum

Looks good overall. A few minor comments. Thanks for updating this.

electrum · 2022-08-15T03:23:19Z

Blackhole should be able to run the final MERGE using assertAccessAllowed. The existing merge test using assertAccessAllowed that this PR replaces runs.

djsstarburst · 2022-08-15T15:06:25Z

Blackhole should be able to run the final MERGE using assertAccessAllowed. The existing merge test using assertAccessAllowed that this PR replaces runs.

Added.

This commit adds a MERGE test in TestAccessControl that verifies that in order execute a MERGE, you must have appropriate access control rights. Specifically, if there is a source table, you must have SELECT on the table columns referenced; if the MERGE does inserts, you must have INSERT on the target table; if the MERGE does deletes you must have DELETE on the target table; and if the MERGE does updates, you must have UPDATE on the target table.

electrum · 2022-08-15T18:00:42Z

Thanks!

cla-bot bot added the cla-signed label Aug 13, 2022

djsstarburst requested review from electrum, findepi and homar August 13, 2022 15:42

djsstarburst force-pushed the david.stryker/merge-access-control branch from 0ecc938 to ff18825 Compare August 13, 2022 16:41

dain approved these changes Aug 13, 2022

View reviewed changes

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated Show resolved Hide resolved

djsstarburst force-pushed the david.stryker/merge-access-control branch from ff18825 to 4560ff9 Compare August 13, 2022 19:26

electrum requested changes Aug 13, 2022

View reviewed changes

djsstarburst force-pushed the david.stryker/merge-access-control branch from 4560ff9 to c342350 Compare August 13, 2022 20:19

Do access control checks at the start of MERGE analysis

160857b

djsstarburst force-pushed the david.stryker/merge-access-control branch from c342350 to fbec671 Compare August 14, 2022 00:47

djsstarburst mentioned this pull request Aug 14, 2022

Add access control tests in SQL MERGE #13620

Closed

electrum reviewed Aug 15, 2022

View reviewed changes

electrum approved these changes Aug 15, 2022

View reviewed changes

Add missing @JsonCreator/@JsonProperty in BlackHoleMergeTableHandle

a01d02b

djsstarburst force-pushed the david.stryker/merge-access-control branch from fbec671 to 2574eb0 Compare August 15, 2022 15:04

djsstarburst force-pushed the david.stryker/merge-access-control branch from 2574eb0 to 6ddca49 Compare August 15, 2022 15:47

electrum merged commit 121463c into trinodb:master Aug 15, 2022

github-actions bot added this to the 393 milestone Aug 15, 2022

colebow mentioned this pull request Aug 15, 2022

Add Trino 393 release notes #13519

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify SQL MERGE access control checks #13664

Verify SQL MERGE access control checks #13664

djsstarburst commented Aug 13, 2022

dain left a comment

electrum left a comment

djsstarburst commented Aug 13, 2022 •

edited

Loading

djsstarburst commented Aug 13, 2022

electrum commented Aug 13, 2022

djsstarburst commented Aug 14, 2022

electrum Aug 15, 2022

djsstarburst Aug 15, 2022

electrum Aug 15, 2022

djsstarburst Aug 15, 2022

electrum Aug 15, 2022

djsstarburst Aug 15, 2022

electrum Aug 15, 2022

djsstarburst Aug 15, 2022

electrum left a comment

electrum commented Aug 15, 2022 •

edited

Loading

djsstarburst commented Aug 15, 2022

electrum commented Aug 15, 2022


		assertUpdate(format("CREATE TABLE %s (nation_name VARCHAR, region_name VARCHAR)", targetTable));

		assertUpdate(format("INSERT INTO %s (nation_name, region_name) VALUES ('FRANCE', 'EUROPE'), ('ALGERIA', 'AFRICA'), ('GERMANY', 'EUROPE')", targetTable), 3);

Verify SQL MERGE access control checks #13664

Verify SQL MERGE access control checks #13664

Conversation

djsstarburst commented Aug 13, 2022

Description

Related issues, pull requests, and links

Documentation

Release notes

dain left a comment

Choose a reason for hiding this comment

electrum left a comment

Choose a reason for hiding this comment

djsstarburst commented Aug 13, 2022 • edited Loading

djsstarburst commented Aug 13, 2022

electrum commented Aug 13, 2022

djsstarburst commented Aug 14, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electrum left a comment

Choose a reason for hiding this comment

electrum commented Aug 15, 2022 • edited Loading

djsstarburst commented Aug 15, 2022

electrum commented Aug 15, 2022

djsstarburst commented Aug 13, 2022 •

edited

Loading

electrum commented Aug 15, 2022 •

edited

Loading