Adding schema access rules to file based system access control #15886

agrawalreetika · 2021-03-30T04:37:57Z

Currently, file-based system access control allows defining catalog level rule. As a part of these changes, schema access rules can be included with file-based system access control for the below operations -

checkCanCreateSchema : If the user has access to the catalog and is also schema owner, then access is provided.
checkCanDropSchema : If the user has access to the catalog and is also schema owner, then access is provided.
checkCanRenameSchema : If the user has access to the catalog and is also schema owner, then access is provided.

== RELEASE NOTES ==

Security Changes

- Add ability to set schema access rules in file-based system access control

aweisberg · 2021-04-09T18:51:20Z

Can you link to the PR? It's a better source of context then the commit trinodb/trino#3766 Also the linked commit is not actually on Trino master.

aweisberg

Thanks!

Some simplification we can take from Trino and the tests seem to be a bit different and incomplete compared to what Trino is currently doing.

aweisberg · 2021-04-09T21:51:56Z

presto-main/src/main/java/com/facebook/presto/security/FileBasedSystemAccessControl.java

@@ -224,6 +227,10 @@ public void checkCanCreateSchema(Identity identity, AccessControlContext context
        if (!canAccessCatalog(identity, schema.getCatalogName(), ALL)) {
            denyCreateSchema(schema.toString());
        }
+


If you ignore the linked diff and look at just how Trino is operating today they dropped the canAccessCatalog check here and pushed it into isSchemaOwner() which is simpler.

trinodb/trino#5039

Sure sounds good. I will place a catalog access check with the isSchemaOwner() method itself.

aweisberg · 2021-04-09T21:52:37Z

presto-main/src/main/java/com/facebook/presto/security/FileBasedSystemAccessControl.java

@@ -232,6 +239,10 @@ public void checkCanDropSchema(Identity identity, AccessControlContext context,
        if (!canAccessCatalog(identity, schema.getCatalogName(), ALL)) {
            denyDropSchema(schema.toString());
        }
+


Same as above

aweisberg · 2021-04-09T21:57:59Z

presto-main/src/main/java/com/facebook/presto/security/FileBasedSystemAccessControl.java

@@ -240,6 +251,10 @@ public void checkCanRenameSchema(Identity identity, AccessControlContext context
        if (!canAccessCatalog(identity, schema.getCatalogName(), ALL)) {
            denyRenameSchema(schema.toString(), newSchemaName);
        }
+


aweisberg · 2021-04-09T21:59:19Z

presto-main/src/main/java/com/facebook/presto/security/FileBasedSystemAccessControl.java

@@ -385,4 +400,19 @@ public void checkCanRevokeTablePrivilege(Identity identity, AccessControlContext
            denyRevokeTablePrivilege(privilege.toString(), table.toString());
        }
    }
+
+    private boolean isSchemaOwner(Identity identity, String schemaName)


Where the simplified catalog check goes https://github.com/trinodb/trino/blob/6f5553f9f1f8638cd6e47278c35844dc0245fd3e/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedSystemAccessControl.java#L869

aweisberg · 2021-04-09T22:09:41Z

presto-main/src/test/java/com/facebook/presto/security/TestFileBasedSystemAccessControl.java

@@ -169,6 +169,73 @@ public void testSchemaOperations()
        }));
    }

+    @Test


We seem to deviate a lot on the tests? Are there "extra" tests that we don't need to cover? Can we do a better job of synchronizing?

@aweisberg I have tried covering all positive and negative tests for all the 3 schema operations. If you think something is missing here, I can add more tests to this.

I can see why we deviated in several ways like not supporting groups and thus not needing to test groups.

If you fix the assertThrows I think it will be fine.

@aweisberg I have made the required changes.

aweisberg · 2021-04-09T22:13:31Z

presto-main/src/test/java/com/facebook/presto/security/TestFileBasedSystemAccessControl.java

+                    accessControlManager.checkCanCreateSchema(transactionId, admin, context, new CatalogSchemaName("some-catalog", "some-schema"));
+                });
+
+        assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> {


Why does one assertThrows have a bunch of different things in it? It will stop executing after the first one fails?

Made the changes.

There are three cases now where there were five previously? It looks like it isn't validating whether the catalog can be accessed since coverage shows no cases where it is denied because the user can't access the catalog.

Yes look like those got removed while making other changes in test, added them back.

aweisberg

It's still not testing the catalog ownership check. You can see because if you step through in the debugger it will never stop at return false in the test cases for schema. It does happen to do it in the read only catalog case, but I don't want to depend on testing unrelated functionality to cover it.

Sorry I know I am being picky here.

Cherry pick of trinodb/trino#3766 Co-authored-by: haldes <[email protected]>

agrawalreetika · 2021-04-22T13:30:46Z

@aweisberg I had added tests for catalog ownership check via read-only catalog access as you mentioned. In all the other cases AccessManager would redirect checks for Accesscatalog to checkCanAccessCatalog. Please review and let me know if any other changes are required.

aweisberg · 2021-04-22T14:40:37Z

@arhimondr @rschlussel This is ready for review/merge.

agrawalreetika force-pushed the filebased-schemaRules branch from 7c6a42c to 160e98d Compare March 30, 2021 04:44

agrawalreetika self-assigned this Mar 30, 2021

aweisberg self-requested a review April 9, 2021 17:11

aweisberg suggested changes Apr 9, 2021

View reviewed changes

agrawalreetika force-pushed the filebased-schemaRules branch 3 times, most recently from b75a2c3 to f345b1c Compare April 14, 2021 03:21

aweisberg suggested changes Apr 15, 2021

View reviewed changes

Adding schema access rules to file based system access control

94af7fc

Cherry pick of trinodb/trino#3766 Co-authored-by: haldes <[email protected]>

agrawalreetika force-pushed the filebased-schemaRules branch from f345b1c to 94af7fc Compare April 15, 2021 18:45

aweisberg approved these changes Apr 22, 2021

View reviewed changes

rschlussel approved these changes Apr 28, 2021

View reviewed changes

rschlussel merged commit ea8e5f4 into prestodb:master Apr 28, 2021

agrawalreetika deleted the filebased-schemaRules branch April 28, 2021 15:52

agrawalreetika mentioned this pull request May 7, 2021

Granular File Based System Access Control #16052

Open

bhhari mentioned this pull request May 11, 2021

Add release notes for 0.253 #16072

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding schema access rules to file based system access control #15886

Adding schema access rules to file based system access control #15886

agrawalreetika commented Mar 30, 2021 •

edited by rschlussel

Loading

aweisberg commented Apr 9, 2021

aweisberg left a comment

aweisberg Apr 9, 2021

agrawalreetika Apr 12, 2021

aweisberg Apr 9, 2021

agrawalreetika Apr 12, 2021

aweisberg Apr 9, 2021

agrawalreetika Apr 12, 2021

aweisberg Apr 9, 2021

agrawalreetika Apr 12, 2021

aweisberg Apr 9, 2021

agrawalreetika Apr 12, 2021

aweisberg Apr 12, 2021

agrawalreetika Apr 12, 2021

aweisberg Apr 9, 2021

agrawalreetika Apr 12, 2021

aweisberg Apr 13, 2021

agrawalreetika Apr 14, 2021

aweisberg left a comment

agrawalreetika commented Apr 22, 2021

aweisberg commented Apr 22, 2021

@@ @@ -169,6 +169,73 @@ public void testSchemaOperations() @@
                       }));
                   }
+                  @Test

Adding schema access rules to file based system access control #15886

Adding schema access rules to file based system access control #15886

Conversation

agrawalreetika commented Mar 30, 2021 • edited by rschlussel Loading

aweisberg commented Apr 9, 2021

aweisberg left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweisberg left a comment

Choose a reason for hiding this comment

agrawalreetika commented Apr 22, 2021

aweisberg commented Apr 22, 2021

agrawalreetika commented Mar 30, 2021 •

edited by rschlussel

Loading