[WIP] Support role management #7346

arhimondr · 2017-02-10T19:52:43Z

amrutagokhale · 2017-02-14T17:08:00Z

Can you add documentation for the two commands?

maciejgrzybek

Squash "Add product tests for CREATE/DROP role" (1341f67a6bffa16236bb7dcf4a677ddb5632d925)
with "Implement CREATE/DROP role for Hive connector" (66eb6fb8018d1c186f803aa4b8bfb84690262414). They don't make sense separately.

maciejgrzybek · 2017-02-15T16:38:19Z

presto-parser/src/test/java/com/facebook/presto/sql/parser/TestSqlParser.java

+        assertStatement("CREATE ROLE role3 IN catalog1", new CreateRole("role3", Optional.empty(), Optional.of("catalog1")));
+        assertStatement("CREATE ROLE \"role\" WITH ADMIN \"admin\" IN \"catalog\"", new CreateRole("role", Optional.of("admin"), Optional.of("catalog")));
+        assertStatement("CREATE ROLE \"ro le\" WITH ADMIN \"ad min\" IN \"ca talog\"", new CreateRole("ro le", Optional.of("ad min"), Optional.of("ca talog")));
+    }


Please add tests for non-letter chars in role, admin and catalog (depending on if they're accepted or not, expect failure here or proper CreateRole object). E.g. role[] or (admin) etc.

maciejgrzybek · 2017-02-15T16:38:43Z

presto-parser/src/test/java/com/facebook/presto/sql/parser/TestSqlParser.java

+        assertStatement("DROP ROLE role1 IN catalog", new DropRole("role1", Optional.of("catalog")));
+        assertStatement("DROP ROLE \"role\" IN \"catalog\"", new DropRole("role", Optional.of("catalog")));
+        assertStatement("DROP ROLE \"ro le\" IN \"ca talog\"", new DropRole("ro le", Optional.of("ca talog")));
+    }


ditto non-letter chars.

maciejgrzybek · 2017-02-15T16:43:58Z

presto-main/src/main/java/com/facebook/presto/metadata/Metadata.java

@@ -253,6 +253,16 @@
    Optional<ResolvedIndex> resolveIndex(Session session, TableHandle tableHandle, Set<ColumnHandle> indexableColumns, Set<ColumnHandle> outputColumns, TupleDomain<ColumnHandle> tupleDomain);

    /**
+     * Create role


Explain any constraints on grantor maybe? Are there any? What if grantor is empty? This comment is useless currently.

maciejgrzybek · 2017-02-15T18:42:43Z

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java

+    {
+        Optional<CatalogMetadata> catalogMetadata = getOptionalCatalogMetadata(session, catalog);
+        if (!catalogMetadata.isPresent()) {
+            return ImmutableSet.of();


Why not throw here? If catalog does not exist but was used, it's an exceptional situation, isn't it?

I've taken the approach from listSchemaNames method

maciejgrzybek · 2017-02-15T18:46:24Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

+            throw new PrestoException(NOT_SUPPORTED, "WITH ADMIN statement is not supported");
+        }
+        // roles are case insensitive in Hive
+        String roleLowerCase = role.toLowerCase();


Specify locale, so we use the same as in other places, instead of JVMs defaults.

maciejgrzybek · 2017-02-15T19:09:22Z

presto-hive/src/main/java/com/facebook/presto/hive/metastore/CachingHiveMetastore.java

@@ -218,6 +219,17 @@ private CachingHiveMetastore(ExtendedHiveMetastore delegate, ExecutorService exe
                        return loadTablePrivileges(key.getUser(), key.getDatabase(), key.getTable());
                    }
                }, executor));
+
+        rolesCache = newCacheBuilder(expiresAfterWriteMillis, refreshMills, maximumSize)
+                .build(asyncReloading(new CacheLoader<String, Set<String>>()


Why not <Void, Set<String>>?

The only valid value for Void type is null. But null is not allowed to be a key in Guava cache.

You never refer to the key in cache in this case, do you?

maciejgrzybek · 2017-02-15T19:30:47Z

presto-hive/src/test/java/com/facebook/presto/hive/metastore/MockHiveMetastoreClient.java

+    public void createRole(String role, String grantor)
+            throws TException
+    {
+        // No-op


Why not mimic e.g. createTable's behavior - to throw the UnsupportedOperationException?

I use this mock in cache test. I don't want it to fail.

maciejgrzybek · 2017-02-15T19:34:18Z

presto-hive/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java

@@ -547,6 +550,34 @@ public synchronized void alterPartition(String databaseName, String tableName, P
    }

    @Override
+    public synchronized void createRole(String role, String grantor)


Why not create a synchronized object shared between roles methods instead of having synchronized method?
You don't interfere with e.g. getPartitionNames or alterPartition so you don't need synchronization with all synchronized methods in this class.

maciejgrzybek · 2017-02-15T19:35:03Z

presto-hive/src/test/java/com/facebook/presto/hive/metastore/TestingHiveMetastore.java

@@ -409,6 +413,30 @@ public synchronized void alterPartition(String databaseName, String tableName, P
    }

    @Override
+    public synchronized void createRole(String role, String grantor)


ditto synchronization

You didn't address this comment.

I addressed that comment for FileBasedMetastore, but i think it doesn't matter here, because it is inmemory.

I won't insist.

maciejgrzybek · 2017-02-15T19:47:53Z

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

+    @Test(groups = {HIVE_CONNECTOR, ROLES, AUTHORIZATION, PROFILE_SPECIFIC_TESTS})
+    public void testAccessControl()
+            throws Exception
+    {


Maybe mention here that alice is non-admin user?

maciejgrzybek

A few more comments

maciejgrzybek · 2017-02-16T14:18:31Z

presto-parser/src/main/java/com/facebook/presto/sql/tree/PrincipalSpecification.java

+    }
+
+    private final Type type;
+    private final String name;


Optional<String> instead? (for CURRENT_USER and CURRENET_ROLE)

I've started to implement this, but it just introduces additional code with no value. This field is completely private. We never expose that null.

So maybe annotate it with @Nullable at least?

maciejgrzybek · 2017-02-16T14:26:02Z

presto-main/src/main/java/com/facebook/presto/metadata/Metadata.java

@@ -264,6 +265,18 @@
    Optional<ResolvedIndex> resolveIndex(Session session, TableHandle tableHandle, Set<ColumnHandle> indexableColumns, Set<ColumnHandle> outputColumns, TupleDomain<ColumnHandle> tupleDomain);

    /**
+     * Creates the specified role in the specified connector.
+     *
+     * @param grantor represent the principal specified by WITH ADMIN statement


represents

maciejgrzybek · 2017-02-16T14:26:20Z

presto-main/src/main/java/com/facebook/presto/metadata/Metadata.java

+    void createRole(Session session, String role, Optional<PrestoPrincipal> grantor, String catalog);
+
+    /**
+     * Drops the specified role in the specified connector


s/connector/catalog/

maciejgrzybek · 2017-02-16T14:45:18Z

presto-main/src/main/java/com/facebook/presto/metadata/Metadata.java

@@ -277,6 +277,11 @@
    void dropRole(Session session, String role, String catalog);

    /**
+     * List available roles in specified connector


s/connector/catalog/
to be consistent with parameter name

maciejgrzybek · 2017-02-16T14:47:17Z

presto-hive/src/main/java/com/facebook/presto/hive/metastore/CachingHiveMetastore.java

@@ -218,6 +219,17 @@ private CachingHiveMetastore(ExtendedHiveMetastore delegate, ExecutorService exe
                        return loadTablePrivileges(key.getUser(), key.getDatabase(), key.getTable());
                    }
                }, executor));
+
+        rolesCache = newCacheBuilder(expiresAfterWriteMillis, refreshMills, maximumSize)
+                .build(asyncReloading(new CacheLoader<String, Set<String>>()


You never refer to the key in cache in this case, do you?

maciejgrzybek · 2017-02-16T14:49:35Z

presto-hive/src/test/java/com/facebook/presto/hive/metastore/TestingHiveMetastore.java

@@ -409,6 +413,30 @@ public synchronized void alterPartition(String databaseName, String tableName, P
    }

    @Override
+    public synchronized void createRole(String role, String grantor)


You didn't address this comment.

maciejgrzybek · 2017-02-16T14:52:39Z

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

+    @Override
+    public void checkCanCreateRole(TransactionId transactionId, Identity identity, String role, Optional<PrestoPrincipal> grantor, String catalogName)
+    {
+        requireNonNull(identity, "identity is null");


Do we need them here? You don't store those variables. NPE will be thrown immediately a couple lines below anyway.

It is done in all the methods in this class. I just wanted to keep it consistent.

arhimondr · 2017-03-08T20:23:59Z

Depends on #6629

cawallin · 2017-03-13T18:16:28Z

presto-hive/src/main/java/com/facebook/presto/hive/security/SqlStandardAccessControl.java

@@ -309,6 +335,10 @@ private boolean getGrantOptionForPrivilege(ConnectorTransactionHandle transactio

    private boolean checkTablePermission(ConnectorTransactionHandle transaction, Identity identity, SchemaTableName tableName, HivePrivilege... requiredPrivileges)
    {
+        if (tableName.equals(ROLES) && !isAdmin(transaction, identity)) {


Throwing an exception is an antipattern for information schema queries -- it would be better to filter out the roles table in the filterTables() method if the user isn't admin.

Yeah, makes total sense

The grantOption flag in PrivilegeInfo represents the WITH GRANT OPTION clause in GRANT. Also add UPDATE privilege to the SPI Privilege enum.

For SHOW ROLES, issue the query: select role_name as "Role" from catalog.information_schema.roles;

Instead of select * from information_schema.roles, SHOW CURRENT ROLES rewrites to select * from information_schema.enabled_roles. All users can see what roles they're currently using, so no need for access control checks.

Currently the only database permission we support is OWNERSHIP. Instead of creating that permission, and checking if it is granted it is more readable to just call `isDatabaseOwner` directly.

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.

arhimondr · 2017-03-27T18:41:14Z

Blocked on #6629

facebook-github-bot added the CLA Signed label Feb 10, 2017

maciejgrzybek reviewed Feb 15, 2017

View reviewed changes

arhimondr changed the title ~~Support role management~~ [WIP] Support role management Feb 15, 2017

arhimondr force-pushed the support_role_management branch 3 times, most recently from e1331c4 to 049a900 Compare February 16, 2017 03:10

maciejgrzybek suggested changes Feb 16, 2017

View reviewed changes

arhimondr force-pushed the support_role_management branch from 049a900 to e9ba73d Compare February 16, 2017 16:56

maciejgrzybek approved these changes Feb 16, 2017

View reviewed changes

arhimondr force-pushed the support_role_management branch 8 times, most recently from 1f0ebf9 to ff62efb Compare February 23, 2017 18:52

arhimondr force-pushed the support_role_management branch 6 times, most recently from 761f38f to c7cae56 Compare March 2, 2017 17:02

arhimondr force-pushed the support_role_management branch from c7cae56 to be4482b Compare March 8, 2017 20:02

cawallin reviewed Mar 13, 2017

View reviewed changes

arhimondr force-pushed the support_role_management branch from 1ed667d to 7e77429 Compare March 17, 2017 20:49

Amruta Gokhale added 2 commits March 20, 2017 10:45

Add syntax support for SHOW GRANTS

1839203

Add PrivilegeInfo class to SPI

90718e9

The grantOption flag in PrivilegeInfo represents the WITH GRANT OPTION clause in GRANT. Also add UPDATE privilege to the SPI Privilege enum.

cawallin and others added 27 commits March 20, 2017 15:49

Add SHOW ROLES to the parser

ecfbb4c

Rewrite SHOW ROLES as a select query

d2fda35

For SHOW ROLES, issue the query: select role_name as "Role" from catalog.information_schema.roles;

Add access control checks for SHOW ROLES

2acf38d

Add docs for SHOW ROLES

323b57b

Product tests for SHOW ROLES

66ff09e

Add SHOW CURRENT ROLES

acabc65

Instead of select * from information_schema.roles, SHOW CURRENT ROLES rewrites to select * from information_schema.enabled_roles. All users can see what roles they're currently using, so no need for access control checks.

Add SHOW ROLE GRANTS syntax

6a246e9

Add listRoleGrants to the SPI

06a9042

Implement listRoleGrants() in Hive

6969228

Implement SHOW ROLE GRANTS rewrite

02d9fbd

Add docs for SHOW ROLE GRANTS

7668f79

Access control for SHOW ROLE GRANTS and SHOW CURRENT ROLES

852d7cd

Remove redundant checkDatabasePermission methods

c4afa44

Currently the only database permission we support is OWNERSHIP. Instead of creating that permission, and checking if it is granted it is more readable to just call `isDatabaseOwner` directly.

Reorder methods in SqlStandardAccessControl

79946ef

Rename getGrantOptionForPrivilege to hasGrantOptionForPrivilege

d3a9ae0

Remove hive privilege null check

e16aaf1

Allow all for admin role

78bb8da

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

187af6e

Simplify checkTablePermission

567ffe1

Refactor canCreateView security checks

40467a8

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

Consider enabled roles for permissions

5e2cb1f

Refactor HivePrivilegeInfo

39a9298

Reorder methods in HivePrivilegeInfo

bb4a0e0

Move parsePrivilege to MetastoreUtil

b92e79c

Add grantor to HivePrivilegeInfo

7a9775f

Add grantor_type and grantee_type columns to table_privileges

9e0a23d

More product tests for SET ROLE

9b62dac

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

arhimondr force-pushed the support_role_management branch from 048b3a0 to 9b62dac Compare March 20, 2017 19:56

arhimondr closed this Mar 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Support role management #7346

[WIP] Support role management #7346

arhimondr commented Feb 10, 2017

amrutagokhale commented Feb 14, 2017

maciejgrzybek left a comment

maciejgrzybek Feb 15, 2017

maciejgrzybek Feb 15, 2017

maciejgrzybek Feb 15, 2017

maciejgrzybek Feb 15, 2017

arhimondr Feb 16, 2017

maciejgrzybek Feb 15, 2017

maciejgrzybek Feb 15, 2017

arhimondr Feb 16, 2017

maciejgrzybek Feb 16, 2017

maciejgrzybek Feb 15, 2017

arhimondr Feb 16, 2017

maciejgrzybek Feb 15, 2017

maciejgrzybek Feb 15, 2017

maciejgrzybek Feb 16, 2017

arhimondr Feb 16, 2017

maciejgrzybek Feb 16, 2017

maciejgrzybek Feb 15, 2017

maciejgrzybek left a comment

maciejgrzybek Feb 16, 2017

arhimondr Feb 16, 2017

maciejgrzybek Feb 16, 2017

maciejgrzybek Feb 16, 2017

maciejgrzybek Feb 16, 2017

maciejgrzybek Feb 16, 2017

maciejgrzybek Feb 16, 2017

maciejgrzybek Feb 16, 2017

maciejgrzybek Feb 16, 2017

arhimondr Feb 16, 2017

arhimondr commented Mar 8, 2017

cawallin Mar 13, 2017 •

edited

Loading

arhimondr Mar 13, 2017

arhimondr commented Mar 27, 2017 •

edited

Loading

[WIP] Support role management #7346

[WIP] Support role management #7346

Conversation

arhimondr commented Feb 10, 2017

amrutagokhale commented Feb 14, 2017

maciejgrzybek 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

Choose a reason for hiding this comment

maciejgrzybek 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

arhimondr commented Mar 8, 2017

cawallin Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arhimondr commented Mar 27, 2017 • edited Loading

cawallin Mar 13, 2017 •

edited

Loading

arhimondr commented Mar 27, 2017 •

edited

Loading