Roles management #12383

arhimondr · 2019-02-25T16:52:04Z

Cherry picked from

wenleix · 2019-02-25T18:22:12Z

Why trinodb/trino#90 adds ~8000 lines while this PR only adds ~7800 lines ? :P

wenleix

"Introduce CREATE ROLE and DROP ROLE statements"

Looks good.

There are some minor difference with trinodb/trino@d177e3b :

PUBLIC is no longer added as reserved word.
DateFormat.tokens is not shown in PrestoSQL
TestSqlParserErrorHandling.java has slightly different change.

wenleix

The following commits looks good by comparing with trinodb/trino#90

"Move PrincipalType to presto-spi "
"Expose Create/Drop/List roles methods in SPI"
"Introduce .information_schema.roles table"
"Assign admin role to subset of users in FileHiveMetastore"

wenleix

"Implement Create/Drop/List roles in Hive connector"

Looks good % minor difference with trinodb/trino@22de2d8:

PrestoSQL's version also have some changes to RecordingHiveMetastore.java
PrestoSQL's version also have some changes to 'TestRecordingHiveMetastore.javaandUnimplementedHiveMetastore.java`

wenleix

"Introduce GRANT/REVOKE roles statements" - Looks good.
"Add Grant/Revoke/List roles authorization to the SPI" - Looks good % PrestoSQL's version has some change to ForwardingConnectorAccessControl.java: trinodb/trino@03101af

wenleix

"Introduce APPLICABLE_ROLES view" : Looks good
"Implement Grant/Revoke/ListApplicableRoles in Hive": Looks good % minor difference comparing with trinodb/trino@006216d:
- PrestoSQL's version has change to RecordingHiveMetastore.java, TestRecordingHiveMetastore.java and UnimplementedHiveMetastore.java
- Changes to TestRoles.java are slightly different.

wenleix

Refactor GRANT/REVOKE in Hive:

Looks good except familiar minor differences in trinodb/trino@35b85b4

RecordingHiveMetastore.java, TestRecordingHiveMetastore.java, UnimplementedHiveMetastore.java

wenleix

"Introduce access control for GRANT/REVOKE ROLE": Looks good. What the RoleGrant in trinodb/trino@487fe85 ?
"Prepare metastore interface to accept ROLE for GRANT/REVOKE" : Looks good except minor diff:
- I didn't see the removal to HivePrincipal.java in trinodb/trino@f892c99
- the same RecordingHiveMetastore.java change ;)

arhimondr · 2019-02-26T15:51:31Z

* `TestSqlParserErrorHandling.java` has slightly different change.

For some reason a test was duplicated in prestosql

arhimondr · 2019-02-26T16:45:37Z

@wenleix

Why prestosql/presto#90 adds ~8000 lines while this PR only adds ~7800 lines ? :P

I picked that from the #11645, as literally cherry picking of 65 commits from the prestosql is too much of work. It is curious where the 150 additions were lost. We have 100 more removals. Maybe some difference because of rebasing / repackaging and slightly different base?

wenleix

The first 6 commits (up to "Implement Create/Drop/List roles in Hive connector") looks good.

For "Implement Create/Drop/List roles in Hive connector": It looks like changes in TestRecordingHiveMetastore.java is missing (compared with trinodb/trino@22de2d8)

wenleix · 2019-02-26T20:38:29Z

Looks like file ClientSelectedRole.java is missing from Implement SET ROLE (compared with trinodb/trino@e02bdab)

presto-jdbc/src/main/java/com/facebook/presto/jdbc/PrestoConnection.java

presto-jdbc/src/main/java/com/facebook/presto/jdbc/PrestoStatement.java

wenleix

Commits "Introduce GRANT/REVOKE roles statements" to "Introduce ConnectorIdentity " looks good.

Commit "Implement SET ROLE" seems to have some minor difference.

arhimondr · 2019-02-26T22:00:54Z

@wenleix

For "Implement Create/Drop/List roles in Hive connector": It looks like changes in TestRecordingHiveMetastore.java is missing (compared with prestosql/presto@22de2d8)

This change is missing in that commit in prestosql. Probably while rebasing the prestosql version they didn't compile every commit.

arhimondr · 2019-02-26T22:05:41Z

@wenleix

Looks like file ClientSelectedRole.java is missing from Implement SET ROLE (compared with prestosql/presto@e02bdab)

In prestosql they removed SPI from the presto-client dependencies. That's why they had to copy the SelectedRole class to the presto-client package.

wenleix · 2019-02-26T22:31:25Z

presto-main/src/main/java/com/facebook/presto/Session.java

@@ -327,11 +327,9 @@ public Session beginTransactionId(TransactionId transactionId, TransactionManage
        for (Entry<String, SelectedRole> entry : identity.getRoles().entrySet()) {
            String catalogName = entry.getKey();
            SelectedRole role = entry.getValue();
-


This commit seems doing nothing . But let's keep consistent with PrestoSQL ;)

What commit is that?

Found it: Store catalog selected roles in Identity. Let me drop it.

I see. Sorry for forgetting to use "Add Review" mode...

wenleix

"Introduce ENABLED_ROLES view": Some minor comments.

wenleix · 2019-02-26T23:05:58Z

presto-main/src/main/java/com/facebook/presto/connector/ConnectorManager.java

@@ -196,7 +196,7 @@ private synchronized void addCatalogConnector(String catalogName, ConnectorId co

        MaterializedConnector informationSchemaConnector = new MaterializedConnector(
                createInformationSchemaConnectorId(connectorId),
-                new InformationSchemaConnector(catalogName, nodeManager, metadataManager, accessControlManager));
+                new InformationSchemaConnector(catalogName, nodeManager, metadataManager, accessControlManager, transactionManager));


This change and InformationSchemaConnector.java doesn't appear in trinodb/trino@7baabd1 :). Maybe again get dropped on PrestoSQL side.

Yeah, good catch

wenleix · 2019-02-26T23:07:51Z

...ava/com/facebook/presto/connector/informationSchema/InformationSchemaPageSourceProvider.java


-    public InformationSchemaPageSourceProvider(Metadata metadata, AccessControl accessControl)
+    public InformationSchemaPageSourceProvider(Metadata metadata, AccessControl accessControl, TransactionManager transactionManager)


This change does't appear in trinodb/trino@7baabd1 . -- also doesn't seem useful?

wenleix · 2019-02-26T23:08:26Z

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

@@ -198,7 +198,7 @@ private static ConnectorId registerBogusConnector(CatalogManager catalogManager,
                connectorId,
                connector,
                createInformationSchemaConnectorId(connectorId),
-                new InformationSchemaConnector(catalogName, nodeManager, metadata, accessControl),
+                new InformationSchemaConnector(catalogName, nodeManager, metadata, accessControl, transactionManager),


Looks like trinodb/trino@7baabd1 removed all the transactionManager related stuff.

wenleix

"Add access control checks for SHOW ROLES" : minor diff with trinodb/trino@e2a2ff0

Also looks like changes to ReadOnlyAccessControl.java doesn't show up in this PR.

wenleix · 2019-02-26T23:13:04Z

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

+    }
+
+    @Override
+    public Set<String> filterRoles(Identity identity, String catalogName, Set<String> roles)


This method was dropped in trinodb/trino@e2a2ff0

wenleix · 2019-02-26T23:14:20Z

presto-spi/src/main/java/com/facebook/presto/spi/security/SystemAccessControl.java

+    /**
+     * Filter the list of roles to those visible to the identity in the given catalog.
+     */
+    default Set<String> filterRoles(Identity identity, String catalogName, Set<String> roles)


dropped in trinodb/trino@e2a2ff0

wenleix

"Access control for SHOW ROLE GRANTS and SHOW CURRENT ROLES". Minor diff against trinodb/trino@4f4b268#diff-cc0689ca452d7c2209c27e411d80c45aR177

wenleix · 2019-02-26T23:21:17Z

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

@@ -187,15 +187,4 @@ public void checkCanGrantTablePrivilege(Identity identity, Privilege privilege,
    public void checkCanRevokeTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, PrestoPrincipal revokee, boolean grantOptionFor)
    {
    }
-


Doesn't see this change in trinodb/trino@4f4b268 . Maybe they already implement in different commit?

Recovered in Add access control checks for SHOW ROLES

wenleix · 2019-02-26T23:21:35Z

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

@@ -110,15 +110,4 @@ public void checkCanShowSchemas(Identity identity, String catalogName)
    public void checkCanShowTablesMetadata(Identity identity, CatalogSchemaName schema)
    {
    }
-


same for this one.

Recovered in Add access control checks for SHOW ROLES

wenleix · 2019-02-26T23:22:32Z

...src/main/java/com/facebook/presto/plugin/base/security/ForwardingConnectorAccessControl.java

@@ -205,4 +205,22 @@ public void checkCanSetRole(ConnectorTransactionHandle transactionHandle, Connec
    {
        delegate().checkCanSetRole(transactionHandle, identity, role, catalogName);
    }
+
+    @Override
+    public void checkCanShowRoles(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, String catalogName)


trinodb/trino@4f4b268#diff-cc0689ca452d7c2209c27e411d80c45aR177 also adds checkCanSetRole

Actually that is mistake. This method has to be moved to the Add access control checks for SHOW ROLES

wenleix · 2019-02-26T23:22:54Z

presto-spi/src/main/java/com/facebook/presto/spi/security/SystemAccessControl.java

@@ -302,22 +301,4 @@ default void checkCanRevokeTablePrivilege(Identity identity, Privilege privilege
    {
        denyRevokeTablePrivilege(privilege.toString(), table.toString());
    }
-
-    /**


same for this one.

Restored in Add access control checks for SHOW ROLES

wenleix · 2019-02-26T23:24:38Z

Finish "human diff" the following commits:

Introduce GRANT/REVOKE roles statements
Add Grant/Revoke/List roles authorization to the SPI
Introduce APPLICABLE_ROLES view
Implement Grant/Revoke/ListApplicableRoles in Hive
Refactor GRANT/REVOKE in Hive
Introduce access control for GRANT/REVOKE ROLE
Prepare metastore interface to accept ROLE for GRANT/REVOKE
Introduce SET ROLE statement
Introduce ConnectorIdentity
Implement SET ROLE
Make HiveQueryRunner default user have admin role
Set admin role before setting system properties in storage format test
Store catalog selected roles in Identity
Implement SET ROLE in Hive Connector
Accept ROLE in GRANT/REVOKE Privileges statements
Add SHOW ROLES to the parser
Add access control checks for SHOW ROLES
Add docs for SHOW ROLES
Product tests for SHOW ROLES
Add SHOW CURRENT ROLES
Add SHOW ROLE GRANTS syntax
Add listRoleGrants to the SPI
Implement listRoleGrants() in Hive
Implement SHOW ROLE GRANTS rewrite
Add docs for SHOW ROLE GRANTS
Access control for SHOW ROLE GRANTS and SHOW CURRENT ROLES

wenleix

"Add grantor_type and grantee_type columns to table_privileges"

minor diff to trinodb/trino@6735d23

wenleix · 2019-02-26T23:34:23Z

presto-product-tests/src/main/java/com/facebook/presto/tests/hive/TestGrantRevoke.java

        }
        catch (Exception e) {
            Logger.get(getClass()).warn(e, "failed to drop table/view");
        }
    }

+    private void cleanupRoles()


trinodb/trino@6735d23 adds annotation @Test(groups = {AUTHORIZATION, PROFILE_SPECIFIC_TESTS})

wenleix

"More product tests for SET ROLE":

one line different with trinodb/trino@aec0671

wenleix · 2019-02-26T23:36:31Z

presto-product-tests/src/main/java/com/facebook/presto/tests/hive/TestRoles.java

@@ -563,6 +564,119 @@ public void testShowRoleGrants()
                        row("role1"));
    }

+    @Test(groups = {ROLES, AUTHORIZATION, PROFILE_SPECIFIC_TESTS})


trinodb/trino@aec0671 also renamed testAccessControl to testCreateDropRoleAccessControl . Although seems unrelated.

Yeah. Let's keep the name.

wenleix · 2019-02-26T23:39:10Z

Finish "human diff" the following commits against trinodb/trino#90

Remove redundant checkDatabasePermission methods
Reorder methods in SqlStandardAccessControl
Rename getGrantOptionForPrivilege to hasGrantOptionForPrivilege
Remove hive privilege null check
Allow all for admin role
Introduce isTableOwner method for readability
Simplify checkTablePermission
Refactor canCreateView security checks
Consider enabled roles for permissions
Refactor HivePrivilegeInfo
Move parsePrivilege to MetastoreUtil
Add grantor to HivePrivilegeInfo
Add grantor_type and grantee_type columns to table_privileges
More product tests for SET ROLE
Document role management

Admin user has all the available permissions for all the entities implicitly. So it may be considered as a database and table "owner" for all tables and databases. Also it has all the SELECT, INSERT, DELETE permissions implicitly.

hasGrantOptionForPrivilege cannot be used in security checks for createView because it doesn't consider the session role.

Verify that role set with `SET ROLE` is considering during the access check.

Previously when SqlStandardAccessControl was checking if given role is enabled, it listed all role grants and check if that role is is among all listed role grants. Now it list all role grants until it finds that role.

That way roles are enumerated lazily.

This way table privileges are enumerated lazily.

Currently Presto shows that the owner of a table has ALL privileges, even after some privileges are revoked. This commit fixes this issue by listing only privileges actually present in the metastore.

Presto currently lists only privilges of the tables owned by the current user, even after the admin role is set. This commit fixes this and lists all privileges for admins.

When tables of the same name exist across different schemas, Presto lists privileges of the table from all schemas instead of the single schema mentioned in the SHOW GRANTS query. This commit fixes the issue.

Extracted-From: prestodb#10904

`equals()` did not compare some fields (like `roles`). Also, it compared `principal` even though `Identity#equals` does not do that. To avoid any ambiguity, `equals()` gets disabled, along with `hashCode`.

facebook-github-bot added the CLA Signed label Feb 25, 2019

arhimondr mentioned this pull request Feb 25, 2019

Support role management (v5) #11645

Closed

arhimondr force-pushed the roles branch from 130416a to 00b06de Compare February 25, 2019 17:42

wenleix reviewed Feb 25, 2019

View reviewed changes

arhimondr force-pushed the roles branch from 00b06de to 0fbfa58 Compare February 26, 2019 15:55

wenleix reviewed Feb 26, 2019

View reviewed changes

presto-jdbc/src/main/java/com/facebook/presto/jdbc/PrestoConnection.java Show resolved Hide resolved

wenleix reviewed Feb 26, 2019

View reviewed changes

presto-jdbc/src/main/java/com/facebook/presto/jdbc/PrestoStatement.java Show resolved Hide resolved

wenleix reviewed Feb 26, 2019

View reviewed changes

arhimondr force-pushed the roles branch from 0fbfa58 to 11ef7bf Compare February 26, 2019 22:23

wenleix reviewed Feb 26, 2019

View reviewed changes

Andrii Rosa and others added 27 commits February 27, 2019 12:03

Rename getGrantOptionForPrivilege to hasGrantOptionForPrivilege

75d535f

Remove hive privilege null check

9bf4589

Allow all for admin role

4875984

Admin user has all the available permissions for all the entities implicitly. So it may be considered as a database and table "owner" for all tables and databases. Also it has all the SELECT, INSERT, DELETE permissions implicitly.

Introduce isTableOwner method for readability

11c0491

Simplify checkTablePermission

c127b45

Refactor canCreateView security checks

43affe2

hasGrantOptionForPrivilege cannot be used in security checks for createView because it doesn't consider the session role.

Consider enabled roles for permissions

112e5b8

Refactor HivePrivilegeInfo

640716a

Move parsePrivilege to MetastoreUtil

256beec

Add grantor to HivePrivilegeInfo

b2d02dd

Add grantor_type and grantee_type columns to table_privileges

8253e3b

More product tests for SET ROLE

470ad52

Verify that role set with `SET ROLE` is considering during the access check.

Document role management

8917b09

Catalog access control for roles

91bd916

Remove unused metastore statistics beans

267985d

Extract role name constants

fcee097

Enumerate roles until enabled role is found

cee23c4

Previously when SqlStandardAccessControl was checking if given role is enabled, it listed all role grants and check if that role is is among all listed role grants. Now it list all role grants until it finds that role.

Make ThriftMetastoreUtil.list*Roles methods to return Stream

0219fc7

That way roles are enumerated lazily.

Make list*TablePrivileges to return Stream

3b78502

This way table privileges are enumerated lazily.

Fix listing privileges of tables owned by an user

f1c3f3b

Currently Presto shows that the owner of a table has ALL privileges, even after some privileges are revoked. This commit fixes this issue by listing only privileges actually present in the metastore.

Fix listing privileges when admin role is set

922fd56

Presto currently lists only privilges of the tables owned by the current user, even after the admin role is set. This commit fixes this and lists all privileges for admins.

Fix rewrite SHOW GRANTS as a SELECT query

9de63e5

When tables of the same name exist across different schemas, Presto lists privileges of the table from all schemas instead of the single schema mentioned in the SHOW GRANTS query. This commit fixes the issue.

Remove 'IN catalog' syntax from role management commands

fe7a7a6

Refactor RecordingHiveMetastore to make it consistent between releases

ea06d6e

Extracted-From: prestodb#10904

Disable ConnectorIdentity#equals and #hashCode

d800339

`equals()` did not compare some fields (like `roles`). Also, it compared `principal` even though `Identity#equals` does not do that. To avoid any ambiguity, `equals()` gets disabled, along with `hashCode`.

Improve ConnectorIdentity#toString

39ac7b0

Correct roles documentation for catalog support

209f00e

arhimondr force-pushed the roles branch from 578413e to 209f00e Compare February 27, 2019 17:37

arhimondr merged commit bfeb574 into prestodb:master Feb 27, 2019

arhimondr deleted the roles branch February 27, 2019 18:32


		public InformationSchemaPageSourceProvider(Metadata metadata, AccessControl accessControl)
		public InformationSchemaPageSourceProvider(Metadata metadata, AccessControl accessControl, TransactionManager transactionManager)

Roles management #12383

Roles management #12383

Conversation

arhimondr commented Feb 25, 2019

wenleix commented Feb 25, 2019

wenleix left a comment

Choose a reason for hiding this comment

wenleix left a comment • edited Loading

Choose a reason for hiding this comment

wenleix left a comment

Choose a reason for hiding this comment

wenleix left a comment

Choose a reason for hiding this comment

wenleix left a comment

Choose a reason for hiding this comment

wenleix left a comment • edited Loading

Choose a reason for hiding this comment

wenleix left a comment • edited Loading

Choose a reason for hiding this comment

arhimondr commented Feb 26, 2019

arhimondr commented Feb 26, 2019 • edited Loading

wenleix left a comment

Choose a reason for hiding this comment

wenleix commented Feb 26, 2019

wenleix left a comment

Choose a reason for hiding this comment

arhimondr commented Feb 26, 2019

arhimondr commented Feb 26, 2019

wenleix Feb 26, 2019 • 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

wenleix 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

wenleix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenleix 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

wenleix commented Feb 26, 2019

wenleix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenleix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenleix commented Feb 26, 2019

wenleix left a comment •

edited

Loading

wenleix left a comment •

edited

Loading

wenleix left a comment •

edited

Loading

arhimondr commented Feb 26, 2019 •

edited

Loading

wenleix Feb 26, 2019 •

edited

Loading