From 34ffbaaa382ce377eb1c75191760d182f74fa581 Mon Sep 17 00:00:00 2001 From: Andrii Rosa Date: Tue, 28 Nov 2017 14:50:58 +0100 Subject: [PATCH] Accept ROLE in GRANT/REVOKE Privileges statements Extracted-From: https://github.com/prestodb/presto/pull/10904 --- presto-docs/src/main/sphinx/sql/grant.rst | 6 +- presto-docs/src/main/sphinx/sql/revoke.rst | 6 +- .../prestosql/plugin/hive/HiveMetadata.java | 20 ++---- .../hive/security/LegacyAccessControl.java | 5 +- .../PartitionsAwareAccessControl.java | 4 +- .../security/SqlStandardAccessControl.java | 4 +- .../io/prestosql/execution/GrantTask.java | 5 +- .../io/prestosql/execution/RevokeTask.java | 5 +- .../java/io/prestosql/metadata/Metadata.java | 4 +- .../prestosql/metadata/MetadataManager.java | 4 +- .../io/prestosql/security/AccessControl.java | 4 +- .../security/AccessControlManager.java | 4 +- .../security/AllowAllAccessControl.java | 4 +- .../security/AllowAllSystemAccessControl.java | 5 +- .../security/DenyAllAccessControl.java | 4 +- .../FileBasedSystemAccessControl.java | 5 +- .../io/prestosql/testing/TestingMetadata.java | 5 +- .../metadata/AbstractMockMetadata.java | 4 +- .../security/TestAccessControlManager.java | 5 +- .../TestFileBasedSystemAccessControl.java | 6 +- .../antlr4/io/prestosql/sql/parser/SqlBase.g4 | 4 +- .../java/io/prestosql/sql/SqlFormatter.java | 4 +- .../io/prestosql/sql/parser/AstBuilder.java | 4 +- .../java/io/prestosql/sql/tree/Grant.java | 10 +-- .../java/io/prestosql/sql/tree/Revoke.java | 10 +-- .../prestosql/sql/parser/TestSqlParser.java | 67 +++++++++++++++---- .../sql/parser/TestStatementBuilder.java | 4 +- .../base/security/AllowAllAccessControl.java | 5 +- .../base/security/FileBasedAccessControl.java | 5 +- .../ForwardingConnectorAccessControl.java | 4 +- .../ForwardingSystemAccessControl.java | 5 +- .../base/security/ReadOnlyAccessControl.java | 5 +- .../prestosql/tests/hive/TestGrantRevoke.java | 53 +++++++++++++-- .../spi/connector/ConnectorAccessControl.java | 4 +- .../spi/connector/ConnectorMetadata.java | 4 +- .../ClassLoaderSafeConnectorMetadata.java | 4 +- .../spi/security/SystemAccessControl.java | 4 +- 37 files changed, 193 insertions(+), 112 deletions(-) diff --git a/presto-docs/src/main/sphinx/sql/grant.rst b/presto-docs/src/main/sphinx/sql/grant.rst index b2b91c6097fa..2da542939747 100644 --- a/presto-docs/src/main/sphinx/sql/grant.rst +++ b/presto-docs/src/main/sphinx/sql/grant.rst @@ -8,11 +8,9 @@ Synopsis .. code-block:: none GRANT ( privilege [, ...] | ( ALL PRIVILEGES ) ) - ON [ TABLE ] table_name TO ( grantee | PUBLIC ) + ON [ TABLE ] table_name TO ( user | USER user | ROLE role ) [ WITH GRANT OPTION ] -Usage of the term ``grantee`` denotes both users and roles. - Description ----------- @@ -39,7 +37,7 @@ Grant ``SELECT`` privilege on the table ``nation`` to user ``alice``, additional Grant ``SELECT`` privilege on the table ``orders`` to everyone:: - GRANT SELECT ON orders TO PUBLIC; + GRANT SELECT ON orders TO ROLE PUBLIC; Limitations ----------- diff --git a/presto-docs/src/main/sphinx/sql/revoke.rst b/presto-docs/src/main/sphinx/sql/revoke.rst index cdb9edd6fdee..571b724bfde2 100644 --- a/presto-docs/src/main/sphinx/sql/revoke.rst +++ b/presto-docs/src/main/sphinx/sql/revoke.rst @@ -9,9 +9,7 @@ Synopsis REVOKE [ GRANT OPTION FOR ] ( privilege [, ...] | ALL PRIVILEGES ) - ON [ TABLE ] table_name FROM ( grantee | PUBLIC ) - -Usage of the term ``grantee`` denotes both users and roles. + ON [ TABLE ] table_name FROM ( user | USER user | ROLE role ) Description ----------- @@ -35,7 +33,7 @@ Revoke ``INSERT`` and ``SELECT`` privileges on the table ``orders`` from user `` Revoke ``SELECT`` privilege on the table ``nation`` from everyone, additionally revoking the privilege to grant ``SELECT`` privilege:: - REVOKE GRANT OPTION FOR SELECT ON nation FROM PUBLIC; + REVOKE GRANT OPTION FOR SELECT ON nation FROM ROLE PUBLIC; Revoke all privileges on the table ``test`` from user ``alice``:: diff --git a/presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java b/presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java index 9a1e905041cc..bf645836d078 100644 --- a/presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java +++ b/presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java @@ -195,7 +195,6 @@ import static io.prestosql.spi.StandardErrorCode.NOT_SUPPORTED; import static io.prestosql.spi.StandardErrorCode.SCHEMA_NOT_EMPTY; import static io.prestosql.spi.predicate.TupleDomain.withColumnDomains; -import static io.prestosql.spi.security.PrincipalType.ROLE; import static io.prestosql.spi.security.PrincipalType.USER; import static java.lang.String.format; import static java.util.Collections.emptyList; @@ -1809,7 +1808,7 @@ public Set listEnabledRoles(ConnectorSession session) } @Override - public void grantTablePrivileges(ConnectorSession session, SchemaTableName schemaTableName, Set privileges, String grantee, boolean grantOption) + public void grantTablePrivileges(ConnectorSession session, SchemaTableName schemaTableName, Set privileges, PrestoPrincipal grantee, boolean grantOption) { String schemaName = schemaTableName.getSchemaName(); String tableName = schemaTableName.getTableName(); @@ -1818,11 +1817,11 @@ public void grantTablePrivileges(ConnectorSession session, SchemaTableName schem .map(privilege -> new HivePrivilegeInfo(toHivePrivilege(privilege), grantOption)) .collect(toSet()); - metastore.grantTablePrivileges(schemaName, tableName, getPrestoPrincipal(grantee), hivePrivilegeInfos); + metastore.grantTablePrivileges(schemaName, tableName, grantee, hivePrivilegeInfos); } @Override - public void revokeTablePrivileges(ConnectorSession session, SchemaTableName schemaTableName, Set privileges, String grantee, boolean grantOption) + public void revokeTablePrivileges(ConnectorSession session, SchemaTableName schemaTableName, Set privileges, PrestoPrincipal grantee, boolean grantOption) { String schemaName = schemaTableName.getSchemaName(); String tableName = schemaTableName.getTableName(); @@ -1831,18 +1830,7 @@ public void revokeTablePrivileges(ConnectorSession session, SchemaTableName sche .map(privilege -> new HivePrivilegeInfo(toHivePrivilege(privilege), grantOption)) .collect(toSet()); - metastore.revokeTablePrivileges(schemaName, tableName, getPrestoPrincipal(grantee), hivePrivilegeInfos); - } - - private PrestoPrincipal getPrestoPrincipal(String grantee) - { - // TODO this hack will be removed after grant for roles is introduced - if (grantee.equalsIgnoreCase("public")) { - return new PrestoPrincipal(ROLE, "public"); - } - else { - return new PrestoPrincipal(USER, grantee); - } + metastore.revokeTablePrivileges(schemaName, tableName, grantee, hivePrivilegeInfos); } @Override diff --git a/presto-hive/src/main/java/io/prestosql/plugin/hive/security/LegacyAccessControl.java b/presto-hive/src/main/java/io/prestosql/plugin/hive/security/LegacyAccessControl.java index dd1c77773ce4..91325de589c0 100644 --- a/presto-hive/src/main/java/io/prestosql/plugin/hive/security/LegacyAccessControl.java +++ b/presto-hive/src/main/java/io/prestosql/plugin/hive/security/LegacyAccessControl.java @@ -20,6 +20,7 @@ import io.prestosql.spi.connector.ConnectorTransactionHandle; import io.prestosql.spi.connector.SchemaTableName; import io.prestosql.spi.security.ConnectorIdentity; +import io.prestosql.spi.security.PrestoPrincipal; import io.prestosql.spi.security.Privilege; import javax.inject.Inject; @@ -188,12 +189,12 @@ public void checkCanSetCatalogSessionProperty(ConnectorTransactionHandle transac } @Override - public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String grantee, boolean withGrantOption) + public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal grantee, boolean withGrantOption) { } @Override - public void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String revokee, boolean grantOptionFor) + public void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal revokee, boolean grantOptionFor) { } } diff --git a/presto-hive/src/main/java/io/prestosql/plugin/hive/security/PartitionsAwareAccessControl.java b/presto-hive/src/main/java/io/prestosql/plugin/hive/security/PartitionsAwareAccessControl.java index e367fba11246..b447e9215a6b 100644 --- a/presto-hive/src/main/java/io/prestosql/plugin/hive/security/PartitionsAwareAccessControl.java +++ b/presto-hive/src/main/java/io/prestosql/plugin/hive/security/PartitionsAwareAccessControl.java @@ -177,13 +177,13 @@ public void checkCanSetCatalogSessionProperty(ConnectorTransactionHandle transac } @Override - public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String grantee, boolean withGrantOption) + public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal grantee, boolean withGrantOption) { delegate.checkCanGrantTablePrivilege(transactionHandle, identity, privilege, tableName, grantee, withGrantOption); } @Override - public void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String revokee, boolean grantOptionFor) + public void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal revokee, boolean grantOptionFor) { delegate.checkCanRevokeTablePrivilege(transactionHandle, identity, privilege, tableName, revokee, grantOptionFor); } diff --git a/presto-hive/src/main/java/io/prestosql/plugin/hive/security/SqlStandardAccessControl.java b/presto-hive/src/main/java/io/prestosql/plugin/hive/security/SqlStandardAccessControl.java index f5cef376acf8..b80369529534 100644 --- a/presto-hive/src/main/java/io/prestosql/plugin/hive/security/SqlStandardAccessControl.java +++ b/presto-hive/src/main/java/io/prestosql/plugin/hive/security/SqlStandardAccessControl.java @@ -247,7 +247,7 @@ public void checkCanSetCatalogSessionProperty(ConnectorTransactionHandle transac } @Override - public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String grantee, boolean withGrantOption) + public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal grantee, boolean withGrantOption) { if (checkTablePermission(transaction, identity, tableName, OWNERSHIP)) { return; @@ -260,7 +260,7 @@ public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transaction, } @Override - public void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String revokee, boolean grantOptionFor) + public void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal revokee, boolean grantOptionFor) { if (checkTablePermission(transaction, identity, tableName, OWNERSHIP)) { return; diff --git a/presto-main/src/main/java/io/prestosql/execution/GrantTask.java b/presto-main/src/main/java/io/prestosql/execution/GrantTask.java index ecef9efbc6d3..f9b7271a2301 100644 --- a/presto-main/src/main/java/io/prestosql/execution/GrantTask.java +++ b/presto-main/src/main/java/io/prestosql/execution/GrantTask.java @@ -32,6 +32,7 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.util.concurrent.Futures.immediateFuture; +import static io.prestosql.metadata.MetadataUtil.createPrincipal; import static io.prestosql.metadata.MetadataUtil.createQualifiedObjectName; import static io.prestosql.sql.analyzer.SemanticErrorCode.INVALID_PRIVILEGE; import static io.prestosql.sql.analyzer.SemanticErrorCode.MISSING_TABLE; @@ -68,10 +69,10 @@ public ListenableFuture execute(Grant statement, TransactionManager transacti // verify current identity has permissions to grant permissions for (Privilege privilege : privileges) { - accessControl.checkCanGrantTablePrivilege(session.getRequiredTransactionId(), session.getIdentity(), privilege, tableName, statement.getGrantee().getValue(), statement.isWithGrantOption()); + accessControl.checkCanGrantTablePrivilege(session.getRequiredTransactionId(), session.getIdentity(), privilege, tableName, createPrincipal(statement.getGrantee()), statement.isWithGrantOption()); } - metadata.grantTablePrivileges(session, tableName, privileges, statement.getGrantee().getValue(), statement.isWithGrantOption()); + metadata.grantTablePrivileges(session, tableName, privileges, createPrincipal(statement.getGrantee()), statement.isWithGrantOption()); return immediateFuture(null); } diff --git a/presto-main/src/main/java/io/prestosql/execution/RevokeTask.java b/presto-main/src/main/java/io/prestosql/execution/RevokeTask.java index ecbaff70bde9..6db26461cd1d 100644 --- a/presto-main/src/main/java/io/prestosql/execution/RevokeTask.java +++ b/presto-main/src/main/java/io/prestosql/execution/RevokeTask.java @@ -32,6 +32,7 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.util.concurrent.Futures.immediateFuture; +import static io.prestosql.metadata.MetadataUtil.createPrincipal; import static io.prestosql.metadata.MetadataUtil.createQualifiedObjectName; import static io.prestosql.sql.analyzer.SemanticErrorCode.INVALID_PRIVILEGE; import static io.prestosql.sql.analyzer.SemanticErrorCode.MISSING_TABLE; @@ -68,10 +69,10 @@ public ListenableFuture execute(Revoke statement, TransactionManager transact // verify current identity has permissions to revoke permissions for (Privilege privilege : privileges) { - accessControl.checkCanRevokeTablePrivilege(session.getRequiredTransactionId(), session.getIdentity(), privilege, tableName, statement.getGrantee().getValue(), statement.isGrantOptionFor()); + accessControl.checkCanRevokeTablePrivilege(session.getRequiredTransactionId(), session.getIdentity(), privilege, tableName, createPrincipal(statement.getGrantee()), statement.isGrantOptionFor()); } - metadata.revokeTablePrivileges(session, tableName, privileges, statement.getGrantee().getValue(), statement.isGrantOptionFor()); + metadata.revokeTablePrivileges(session, tableName, privileges, createPrincipal(statement.getGrantee()), statement.isGrantOptionFor()); return immediateFuture(null); } diff --git a/presto-main/src/main/java/io/prestosql/metadata/Metadata.java b/presto-main/src/main/java/io/prestosql/metadata/Metadata.java index df2a2c143bca..6a33117d070b 100644 --- a/presto-main/src/main/java/io/prestosql/metadata/Metadata.java +++ b/presto-main/src/main/java/io/prestosql/metadata/Metadata.java @@ -329,12 +329,12 @@ public interface Metadata /** * Grants the specified privilege to the specified user on the specified table */ - void grantTablePrivileges(Session session, QualifiedObjectName tableName, Set privileges, String grantee, boolean grantOption); + void grantTablePrivileges(Session session, QualifiedObjectName tableName, Set privileges, PrestoPrincipal grantee, boolean grantOption); /** * Revokes the specified privilege on the specified table from the specified user */ - void revokeTablePrivileges(Session session, QualifiedObjectName tableName, Set privileges, String grantee, boolean grantOption); + void revokeTablePrivileges(Session session, QualifiedObjectName tableName, Set privileges, PrestoPrincipal grantee, boolean grantOption); /** * Gets the privileges for the specified table available to the given grantee diff --git a/presto-main/src/main/java/io/prestosql/metadata/MetadataManager.java b/presto-main/src/main/java/io/prestosql/metadata/MetadataManager.java index 579f401a8cea..d86290a22f64 100644 --- a/presto-main/src/main/java/io/prestosql/metadata/MetadataManager.java +++ b/presto-main/src/main/java/io/prestosql/metadata/MetadataManager.java @@ -962,7 +962,7 @@ public Set listEnabledRoles(Session session, String catalog) } @Override - public void grantTablePrivileges(Session session, QualifiedObjectName tableName, Set privileges, String grantee, boolean grantOption) + public void grantTablePrivileges(Session session, QualifiedObjectName tableName, Set privileges, PrestoPrincipal grantee, boolean grantOption) { CatalogMetadata catalogMetadata = getCatalogMetadataForWrite(session, tableName.getCatalogName()); ConnectorId connectorId = catalogMetadata.getConnectorId(); @@ -972,7 +972,7 @@ public void grantTablePrivileges(Session session, QualifiedObjectName tableName, } @Override - public void revokeTablePrivileges(Session session, QualifiedObjectName tableName, Set privileges, String grantee, boolean grantOption) + public void revokeTablePrivileges(Session session, QualifiedObjectName tableName, Set privileges, PrestoPrincipal grantee, boolean grantOption) { CatalogMetadata catalogMetadata = getCatalogMetadataForWrite(session, tableName.getCatalogName()); ConnectorId connectorId = catalogMetadata.getConnectorId(); diff --git a/presto-main/src/main/java/io/prestosql/security/AccessControl.java b/presto-main/src/main/java/io/prestosql/security/AccessControl.java index 0ed43b32a3a3..6da0535501ea 100644 --- a/presto-main/src/main/java/io/prestosql/security/AccessControl.java +++ b/presto-main/src/main/java/io/prestosql/security/AccessControl.java @@ -179,14 +179,14 @@ public interface AccessControl * * @throws io.prestosql.spi.security.AccessDeniedException if not allowed */ - void checkCanGrantTablePrivilege(TransactionId transactionId, Identity identity, Privilege privilege, QualifiedObjectName tableName, String grantee, boolean withGrantOption); + void checkCanGrantTablePrivilege(TransactionId transactionId, Identity identity, Privilege privilege, QualifiedObjectName tableName, PrestoPrincipal grantee, boolean withGrantOption); /** * Check if identity is allowed to revoke a privilege from the revokee on the specified table. * * @throws io.prestosql.spi.security.AccessDeniedException if not allowed */ - void checkCanRevokeTablePrivilege(TransactionId transactionId, Identity identity, Privilege privilege, QualifiedObjectName tableName, String revokee, boolean grantOptionFor); + void checkCanRevokeTablePrivilege(TransactionId transactionId, Identity identity, Privilege privilege, QualifiedObjectName tableName, PrestoPrincipal revokee, boolean grantOptionFor); /** * Check if identity is allowed to set the specified system property. diff --git a/presto-main/src/main/java/io/prestosql/security/AccessControlManager.java b/presto-main/src/main/java/io/prestosql/security/AccessControlManager.java index dc1cf2ef9135..1682dbc024ec 100644 --- a/presto-main/src/main/java/io/prestosql/security/AccessControlManager.java +++ b/presto-main/src/main/java/io/prestosql/security/AccessControlManager.java @@ -466,7 +466,7 @@ public void checkCanCreateViewWithSelectFromColumns(TransactionId transactionId, } @Override - public void checkCanGrantTablePrivilege(TransactionId transactionId, Identity identity, Privilege privilege, QualifiedObjectName tableName, String grantee, boolean withGrantOption) + public void checkCanGrantTablePrivilege(TransactionId transactionId, Identity identity, Privilege privilege, QualifiedObjectName tableName, PrestoPrincipal grantee, boolean withGrantOption) { requireNonNull(identity, "identity is null"); requireNonNull(tableName, "tableName is null"); @@ -483,7 +483,7 @@ public void checkCanGrantTablePrivilege(TransactionId transactionId, Identity id } @Override - public void checkCanRevokeTablePrivilege(TransactionId transactionId, Identity identity, Privilege privilege, QualifiedObjectName tableName, String revokee, boolean grantOptionFor) + public void checkCanRevokeTablePrivilege(TransactionId transactionId, Identity identity, Privilege privilege, QualifiedObjectName tableName, PrestoPrincipal revokee, boolean grantOptionFor) { requireNonNull(identity, "identity is null"); requireNonNull(tableName, "tableName is null"); diff --git a/presto-main/src/main/java/io/prestosql/security/AllowAllAccessControl.java b/presto-main/src/main/java/io/prestosql/security/AllowAllAccessControl.java index 3f99eec3a35b..60055eca110d 100644 --- a/presto-main/src/main/java/io/prestosql/security/AllowAllAccessControl.java +++ b/presto-main/src/main/java/io/prestosql/security/AllowAllAccessControl.java @@ -137,12 +137,12 @@ public void checkCanCreateViewWithSelectFromColumns(TransactionId transactionId, } @Override - public void checkCanGrantTablePrivilege(TransactionId transactionId, Identity identity, Privilege privilege, QualifiedObjectName tableName, String grantee, boolean withGrantOption) + public void checkCanGrantTablePrivilege(TransactionId transactionId, Identity identity, Privilege privilege, QualifiedObjectName tableName, PrestoPrincipal grantee, boolean withGrantOption) { } @Override - public void checkCanRevokeTablePrivilege(TransactionId transactionId, Identity identity, Privilege privilege, QualifiedObjectName tableName, String revokee, boolean grantOptionFor) + public void checkCanRevokeTablePrivilege(TransactionId transactionId, Identity identity, Privilege privilege, QualifiedObjectName tableName, PrestoPrincipal revokee, boolean grantOptionFor) { } diff --git a/presto-main/src/main/java/io/prestosql/security/AllowAllSystemAccessControl.java b/presto-main/src/main/java/io/prestosql/security/AllowAllSystemAccessControl.java index e88e1727485a..677a42b55a12 100644 --- a/presto-main/src/main/java/io/prestosql/security/AllowAllSystemAccessControl.java +++ b/presto-main/src/main/java/io/prestosql/security/AllowAllSystemAccessControl.java @@ -17,6 +17,7 @@ import io.prestosql.spi.connector.CatalogSchemaTableName; import io.prestosql.spi.connector.SchemaTableName; import io.prestosql.spi.security.Identity; +import io.prestosql.spi.security.PrestoPrincipal; import io.prestosql.spi.security.Privilege; import io.prestosql.spi.security.SystemAccessControl; import io.prestosql.spi.security.SystemAccessControlFactory; @@ -178,12 +179,12 @@ public void checkCanSetCatalogSessionProperty(Identity identity, String catalogN } @Override - public void checkCanGrantTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, String grantee, boolean withGrantOption) + public void checkCanGrantTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, PrestoPrincipal grantee, boolean withGrantOption) { } @Override - public void checkCanRevokeTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, String revokee, boolean grantOptionFor) + public void checkCanRevokeTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, PrestoPrincipal revokee, boolean grantOptionFor) { } } diff --git a/presto-main/src/main/java/io/prestosql/security/DenyAllAccessControl.java b/presto-main/src/main/java/io/prestosql/security/DenyAllAccessControl.java index 914fb9ccc6ef..1627279cb386 100644 --- a/presto-main/src/main/java/io/prestosql/security/DenyAllAccessControl.java +++ b/presto-main/src/main/java/io/prestosql/security/DenyAllAccessControl.java @@ -185,13 +185,13 @@ public void checkCanCreateViewWithSelectFromColumns(TransactionId transactionId, } @Override - public void checkCanGrantTablePrivilege(TransactionId transactionId, Identity identity, Privilege privilege, QualifiedObjectName tableName, String grantee, boolean withGrantOption) + public void checkCanGrantTablePrivilege(TransactionId transactionId, Identity identity, Privilege privilege, QualifiedObjectName tableName, PrestoPrincipal grantee, boolean withGrantOption) { denyGrantTablePrivilege(privilege.name(), tableName.toString()); } @Override - public void checkCanRevokeTablePrivilege(TransactionId transactionId, Identity identity, Privilege privilege, QualifiedObjectName tableName, String revokee, boolean grantOptionFor) + public void checkCanRevokeTablePrivilege(TransactionId transactionId, Identity identity, Privilege privilege, QualifiedObjectName tableName, PrestoPrincipal revokee, boolean grantOptionFor) { denyRevokeTablePrivilege(privilege.name(), tableName.toString()); } diff --git a/presto-main/src/main/java/io/prestosql/security/FileBasedSystemAccessControl.java b/presto-main/src/main/java/io/prestosql/security/FileBasedSystemAccessControl.java index a6f6dde984ab..e97044da4ffd 100644 --- a/presto-main/src/main/java/io/prestosql/security/FileBasedSystemAccessControl.java +++ b/presto-main/src/main/java/io/prestosql/security/FileBasedSystemAccessControl.java @@ -23,6 +23,7 @@ import io.prestosql.spi.connector.CatalogSchemaTableName; import io.prestosql.spi.connector.SchemaTableName; import io.prestosql.spi.security.Identity; +import io.prestosql.spi.security.PrestoPrincipal; import io.prestosql.spi.security.Privilege; import io.prestosql.spi.security.SystemAccessControl; import io.prestosql.spi.security.SystemAccessControlFactory; @@ -303,12 +304,12 @@ public void checkCanSetCatalogSessionProperty(Identity identity, String catalogN } @Override - public void checkCanGrantTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, String grantee, boolean withGrantOption) + public void checkCanGrantTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, PrestoPrincipal grantee, boolean withGrantOption) { } @Override - public void checkCanRevokeTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, String revokee, boolean grantOptionFor) + public void checkCanRevokeTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, PrestoPrincipal revokee, boolean grantOptionFor) { } } diff --git a/presto-main/src/main/java/io/prestosql/testing/TestingMetadata.java b/presto-main/src/main/java/io/prestosql/testing/TestingMetadata.java index 624e91927212..bc9fa5e860ca 100644 --- a/presto-main/src/main/java/io/prestosql/testing/TestingMetadata.java +++ b/presto-main/src/main/java/io/prestosql/testing/TestingMetadata.java @@ -37,6 +37,7 @@ import io.prestosql.spi.connector.SchemaTableName; import io.prestosql.spi.connector.SchemaTablePrefix; import io.prestosql.spi.connector.ViewNotFoundException; +import io.prestosql.spi.security.PrestoPrincipal; import io.prestosql.spi.security.Privilege; import io.prestosql.spi.statistics.ComputedStatistics; import io.prestosql.spi.type.Type; @@ -272,10 +273,10 @@ public void renameColumn(ConnectorSession session, ConnectorTableHandle tableHan } @Override - public void grantTablePrivileges(ConnectorSession session, SchemaTableName tableName, Set privileges, String grantee, boolean grantOption) {} + public void grantTablePrivileges(ConnectorSession session, SchemaTableName tableName, Set privileges, PrestoPrincipal grantee, boolean grantOption) {} @Override - public void revokeTablePrivileges(ConnectorSession session, SchemaTableName tableName, Set privileges, String grantee, boolean grantOption) {} + public void revokeTablePrivileges(ConnectorSession session, SchemaTableName tableName, Set privileges, PrestoPrincipal grantee, boolean grantOption) {} public void clear() { diff --git a/presto-main/src/test/java/io/prestosql/metadata/AbstractMockMetadata.java b/presto-main/src/test/java/io/prestosql/metadata/AbstractMockMetadata.java index 07f4d8cb6f4b..df43faf5432f 100644 --- a/presto-main/src/test/java/io/prestosql/metadata/AbstractMockMetadata.java +++ b/presto-main/src/test/java/io/prestosql/metadata/AbstractMockMetadata.java @@ -396,13 +396,13 @@ public Set listEnabledRoles(Session session, String catalog) } @Override - public void grantTablePrivileges(Session session, QualifiedObjectName tableName, Set privileges, String grantee, boolean grantOption) + public void grantTablePrivileges(Session session, QualifiedObjectName tableName, Set privileges, PrestoPrincipal grantee, boolean grantOption) { throw new UnsupportedOperationException(); } @Override - public void revokeTablePrivileges(Session session, QualifiedObjectName tableName, Set privileges, String grantee, boolean grantOption) + public void revokeTablePrivileges(Session session, QualifiedObjectName tableName, Set privileges, PrestoPrincipal grantee, boolean grantOption) { throw new UnsupportedOperationException(); } diff --git a/presto-main/src/test/java/io/prestosql/security/TestAccessControlManager.java b/presto-main/src/test/java/io/prestosql/security/TestAccessControlManager.java index ec6e3580e260..d8e0236b7fd1 100644 --- a/presto-main/src/test/java/io/prestosql/security/TestAccessControlManager.java +++ b/presto-main/src/test/java/io/prestosql/security/TestAccessControlManager.java @@ -35,6 +35,7 @@ import io.prestosql.spi.security.BasicPrincipal; import io.prestosql.spi.security.ConnectorIdentity; import io.prestosql.spi.security.Identity; +import io.prestosql.spi.security.PrestoPrincipal; import io.prestosql.spi.security.Privilege; import io.prestosql.spi.security.SystemAccessControl; import io.prestosql.spi.security.SystemAccessControlFactory; @@ -385,13 +386,13 @@ public void checkCanSetCatalogSessionProperty(ConnectorTransactionHandle transac } @Override - public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String grantee, boolean withGrantOption) + public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal grantee, boolean withGrantOption) { throw new UnsupportedOperationException(); } @Override - public void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String revokee, boolean grantOptionFor) + public void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal revokee, boolean grantOptionFor) { throw new UnsupportedOperationException(); } diff --git a/presto-main/src/test/java/io/prestosql/security/TestFileBasedSystemAccessControl.java b/presto-main/src/test/java/io/prestosql/security/TestFileBasedSystemAccessControl.java index dd83e0e6a93e..0b9263b0cc7d 100644 --- a/presto-main/src/test/java/io/prestosql/security/TestFileBasedSystemAccessControl.java +++ b/presto-main/src/test/java/io/prestosql/security/TestFileBasedSystemAccessControl.java @@ -20,6 +20,7 @@ import io.prestosql.spi.connector.SchemaTableName; import io.prestosql.spi.security.AccessDeniedException; import io.prestosql.spi.security.Identity; +import io.prestosql.spi.security.PrestoPrincipal; import io.prestosql.spi.security.SystemAccessControl; import io.prestosql.transaction.TransactionManager; import org.testng.annotations.Test; @@ -33,6 +34,7 @@ import static com.google.common.io.Files.copy; import static io.prestosql.plugin.base.security.FileBasedAccessControlConfig.SECURITY_CONFIG_FILE; import static io.prestosql.plugin.base.security.FileBasedAccessControlConfig.SECURITY_REFRESH_PERIOD; +import static io.prestosql.spi.security.PrincipalType.USER; import static io.prestosql.spi.security.Privilege.SELECT; import static io.prestosql.spi.testing.InterfaceTestUtils.assertAllMethodsOverridden; import static io.prestosql.transaction.InMemoryTransactionManager.createTestTransactionManager; @@ -185,8 +187,8 @@ public void testViewOperations() accessControlManager.checkCanCreateViewWithSelectFromColumns(transactionId, alice, aliceTable, ImmutableSet.of()); accessControlManager.checkCanCreateViewWithSelectFromColumns(transactionId, alice, aliceView, ImmutableSet.of()); accessControlManager.checkCanSetCatalogSessionProperty(transactionId, alice, "alice-catalog", "property"); - accessControlManager.checkCanGrantTablePrivilege(transactionId, alice, SELECT, aliceTable, "grantee", true); - accessControlManager.checkCanRevokeTablePrivilege(transactionId, alice, SELECT, aliceTable, "revokee", true); + accessControlManager.checkCanGrantTablePrivilege(transactionId, alice, SELECT, aliceTable, new PrestoPrincipal(USER, "grantee"), true); + accessControlManager.checkCanRevokeTablePrivilege(transactionId, alice, SELECT, aliceTable, new PrestoPrincipal(USER, "revokee"), true); }); assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { accessControlManager.checkCanCreateView(transactionId, bob, aliceView); diff --git a/presto-parser/src/main/antlr4/io/prestosql/sql/parser/SqlBase.g4 b/presto-parser/src/main/antlr4/io/prestosql/sql/parser/SqlBase.g4 index 776d0152a37f..20e4a0fd1926 100644 --- a/presto-parser/src/main/antlr4/io/prestosql/sql/parser/SqlBase.g4 +++ b/presto-parser/src/main/antlr4/io/prestosql/sql/parser/SqlBase.g4 @@ -79,12 +79,12 @@ statement | SET ROLE (ALL | NONE | role=identifier) (IN catalog=identifier)? #setRole | GRANT (privilege (',' privilege)* | ALL PRIVILEGES) - ON TABLE? qualifiedName TO grantee=identifier + ON TABLE? qualifiedName TO grantee=principal (WITH GRANT OPTION)? #grant | REVOKE (GRANT OPTION FOR)? (privilege (',' privilege)* | ALL PRIVILEGES) - ON TABLE? qualifiedName FROM grantee=identifier #revoke + ON TABLE? qualifiedName FROM grantee=principal #revoke | SHOW GRANTS (ON TABLE? qualifiedName)? #showGrants | EXPLAIN ANALYZE? VERBOSE? diff --git a/presto-parser/src/main/java/io/prestosql/sql/SqlFormatter.java b/presto-parser/src/main/java/io/prestosql/sql/SqlFormatter.java index c31bb350cdb9..16c05388a9aa 100644 --- a/presto-parser/src/main/java/io/prestosql/sql/SqlFormatter.java +++ b/presto-parser/src/main/java/io/prestosql/sql/SqlFormatter.java @@ -1206,7 +1206,7 @@ public Void visitGrant(Grant node, Integer indent) } builder.append(node.getTableName()) .append(" TO ") - .append(node.getGrantee()); + .append(formatPrincipal(node.getGrantee())); if (node.isWithGrantOption()) { builder.append(" WITH GRANT OPTION"); } @@ -1237,7 +1237,7 @@ public Void visitRevoke(Revoke node, Integer indent) } builder.append(node.getTableName()) .append(" FROM ") - .append(node.getGrantee()); + .append(formatPrincipal(node.getGrantee())); return null; } diff --git a/presto-parser/src/main/java/io/prestosql/sql/parser/AstBuilder.java b/presto-parser/src/main/java/io/prestosql/sql/parser/AstBuilder.java index f576d5c08796..f5855201c113 100644 --- a/presto-parser/src/main/java/io/prestosql/sql/parser/AstBuilder.java +++ b/presto-parser/src/main/java/io/prestosql/sql/parser/AstBuilder.java @@ -896,7 +896,7 @@ public Node visitGrant(SqlBaseParser.GrantContext context) privileges, context.TABLE() != null, getQualifiedName(context.qualifiedName()), - (Identifier) visit(context.grantee), + getPrincipalSpecification(context.grantee), context.OPTION() != null); } @@ -918,7 +918,7 @@ public Node visitRevoke(SqlBaseParser.RevokeContext context) privileges, context.TABLE() != null, getQualifiedName(context.qualifiedName()), - (Identifier) visit(context.grantee)); + getPrincipalSpecification(context.grantee)); } @Override diff --git a/presto-parser/src/main/java/io/prestosql/sql/tree/Grant.java b/presto-parser/src/main/java/io/prestosql/sql/tree/Grant.java index c61603eb3762..93c2c3ea3d33 100644 --- a/presto-parser/src/main/java/io/prestosql/sql/tree/Grant.java +++ b/presto-parser/src/main/java/io/prestosql/sql/tree/Grant.java @@ -28,20 +28,20 @@ public class Grant private final Optional> privileges; // missing means ALL PRIVILEGES private final boolean table; private final QualifiedName tableName; - private final Identifier grantee; + private final PrincipalSpecification grantee; private final boolean withGrantOption; - public Grant(Optional> privileges, boolean table, QualifiedName tableName, Identifier grantee, boolean withGrantOption) + public Grant(Optional> privileges, boolean table, QualifiedName tableName, PrincipalSpecification grantee, boolean withGrantOption) { this(Optional.empty(), privileges, table, tableName, grantee, withGrantOption); } - public Grant(NodeLocation location, Optional> privileges, boolean table, QualifiedName tableName, Identifier grantee, boolean withGrantOption) + public Grant(NodeLocation location, Optional> privileges, boolean table, QualifiedName tableName, PrincipalSpecification grantee, boolean withGrantOption) { this(Optional.of(location), privileges, table, tableName, grantee, withGrantOption); } - private Grant(Optional location, Optional> privileges, boolean table, QualifiedName tableName, Identifier grantee, boolean withGrantOption) + private Grant(Optional location, Optional> privileges, boolean table, QualifiedName tableName, PrincipalSpecification grantee, boolean withGrantOption) { super(location); requireNonNull(privileges, "privileges is null"); @@ -67,7 +67,7 @@ public QualifiedName getTableName() return tableName; } - public Identifier getGrantee() + public PrincipalSpecification getGrantee() { return grantee; } diff --git a/presto-parser/src/main/java/io/prestosql/sql/tree/Revoke.java b/presto-parser/src/main/java/io/prestosql/sql/tree/Revoke.java index dab0ef14ad4e..a33ec508dfca 100644 --- a/presto-parser/src/main/java/io/prestosql/sql/tree/Revoke.java +++ b/presto-parser/src/main/java/io/prestosql/sql/tree/Revoke.java @@ -29,19 +29,19 @@ public class Revoke private final Optional> privileges; // missing means ALL PRIVILEGES private final boolean table; private final QualifiedName tableName; - private final Identifier grantee; + private final PrincipalSpecification grantee; - public Revoke(boolean grantOptionFor, Optional> privileges, boolean table, QualifiedName tableName, Identifier grantee) + public Revoke(boolean grantOptionFor, Optional> privileges, boolean table, QualifiedName tableName, PrincipalSpecification grantee) { this(Optional.empty(), grantOptionFor, privileges, table, tableName, grantee); } - public Revoke(NodeLocation location, boolean grantOptionFor, Optional> privileges, boolean table, QualifiedName tableName, Identifier grantee) + public Revoke(NodeLocation location, boolean grantOptionFor, Optional> privileges, boolean table, QualifiedName tableName, PrincipalSpecification grantee) { this(Optional.of(location), grantOptionFor, privileges, table, tableName, grantee); } - private Revoke(Optional location, boolean grantOptionFor, Optional> privileges, boolean table, QualifiedName tableName, Identifier grantee) + private Revoke(Optional location, boolean grantOptionFor, Optional> privileges, boolean table, QualifiedName tableName, PrincipalSpecification grantee) { super(location); this.grantOptionFor = grantOptionFor; @@ -72,7 +72,7 @@ public QualifiedName getTableName() return tableName; } - public Identifier getGrantee() + public PrincipalSpecification getGrantee() { return grantee; } diff --git a/presto-parser/src/test/java/io/prestosql/sql/parser/TestSqlParser.java b/presto-parser/src/test/java/io/prestosql/sql/parser/TestSqlParser.java index e60826c8468a..9f2432c558ee 100644 --- a/presto-parser/src/test/java/io/prestosql/sql/parser/TestSqlParser.java +++ b/presto-parser/src/test/java/io/prestosql/sql/parser/TestSqlParser.java @@ -1370,26 +1370,65 @@ public void testCreateView() public void testGrant() { assertStatement("GRANT INSERT, DELETE ON t TO u", - new Grant(Optional.of(ImmutableList.of("INSERT", "DELETE")), false, QualifiedName.of("t"), identifier("u"), false)); - assertStatement("GRANT SELECT ON t TO PUBLIC WITH GRANT OPTION", - new Grant(Optional.of(ImmutableList.of("SELECT")), false, QualifiedName.of("t"), identifier("PUBLIC"), true)); - assertStatement("GRANT ALL PRIVILEGES ON t TO u", - new Grant(Optional.empty(), false, QualifiedName.of("t"), identifier("u"), false)); - assertStatement("GRANT taco ON t TO PUBLIC WITH GRANT OPTION", - new Grant(Optional.of(ImmutableList.of("taco")), false, QualifiedName.of("t"), identifier("PUBLIC"), true)); + new Grant( + Optional.of(ImmutableList.of("INSERT", "DELETE")), + false, + QualifiedName.of("t"), + new PrincipalSpecification(PrincipalSpecification.Type.UNSPECIFIED, new Identifier("u")), + false)); + assertStatement("GRANT SELECT ON t TO ROLE PUBLIC WITH GRANT OPTION", + new Grant( + Optional.of(ImmutableList.of("SELECT")), + false, QualifiedName.of("t"), + new PrincipalSpecification(PrincipalSpecification.Type.ROLE, new Identifier("PUBLIC")), + true)); + assertStatement("GRANT ALL PRIVILEGES ON t TO USER u", + new Grant( + Optional.empty(), + false, + QualifiedName.of("t"), + new PrincipalSpecification(PrincipalSpecification.Type.USER, new Identifier("u")), + false)); + assertStatement("GRANT taco ON \"t\" TO ROLE \"public\" WITH GRANT OPTION", + new Grant( + Optional.of(ImmutableList.of("taco")), + false, + QualifiedName.of("t"), + new PrincipalSpecification(PrincipalSpecification.Type.ROLE, new Identifier("public")), + true)); } @Test public void testRevoke() { assertStatement("REVOKE INSERT, DELETE ON t FROM u", - new Revoke(false, Optional.of(ImmutableList.of("INSERT", "DELETE")), false, QualifiedName.of("t"), identifier("u"))); - assertStatement("REVOKE GRANT OPTION FOR SELECT ON t FROM PUBLIC", - new Revoke(true, Optional.of(ImmutableList.of("SELECT")), false, QualifiedName.of("t"), identifier("PUBLIC"))); - assertStatement("REVOKE ALL PRIVILEGES ON TABLE t FROM u", - new Revoke(false, Optional.empty(), true, QualifiedName.of("t"), identifier("u"))); - assertStatement("REVOKE taco ON TABLE t FROM u", - new Revoke(false, Optional.of(ImmutableList.of("taco")), true, QualifiedName.of("t"), identifier("u"))); + new Revoke( + false, + Optional.of(ImmutableList.of("INSERT", "DELETE")), + false, + QualifiedName.of("t"), + new PrincipalSpecification(PrincipalSpecification.Type.UNSPECIFIED, new Identifier("u")))); + assertStatement("REVOKE GRANT OPTION FOR SELECT ON t FROM ROLE PUBLIC", + new Revoke( + true, + Optional.of(ImmutableList.of("SELECT")), + false, + QualifiedName.of("t"), + new PrincipalSpecification(PrincipalSpecification.Type.ROLE, new Identifier("PUBLIC")))); + assertStatement("REVOKE ALL PRIVILEGES ON TABLE t FROM USER u", + new Revoke( + false, + Optional.empty(), + true, + QualifiedName.of("t"), + new PrincipalSpecification(PrincipalSpecification.Type.USER, new Identifier("u")))); + assertStatement("REVOKE taco ON TABLE \"t\" FROM \"u\"", + new Revoke( + false, + Optional.of(ImmutableList.of("taco")), + true, + QualifiedName.of("t"), + new PrincipalSpecification(PrincipalSpecification.Type.UNSPECIFIED, new Identifier("u")))); } @Test diff --git a/presto-parser/src/test/java/io/prestosql/sql/parser/TestStatementBuilder.java b/presto-parser/src/test/java/io/prestosql/sql/parser/TestStatementBuilder.java index cc7f24bb3747..5ce857e73a01 100644 --- a/presto-parser/src/test/java/io/prestosql/sql/parser/TestStatementBuilder.java +++ b/presto-parser/src/test/java/io/prestosql/sql/parser/TestStatementBuilder.java @@ -239,10 +239,10 @@ public void testStatementBuilder() printStatement("grant select on foo to alice with grant option"); printStatement("grant all privileges on foo to alice"); - printStatement("grant delete, select on foo to public"); + printStatement("grant delete, select on foo to role public"); printStatement("revoke grant option for select on foo from alice"); printStatement("revoke all privileges on foo from alice"); - printStatement("revoke insert, delete on foo from public"); //check support for public + printStatement("revoke insert, delete on foo from role public"); printStatement("show grants on table t"); printStatement("show grants on t"); printStatement("show grants"); diff --git a/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/AllowAllAccessControl.java b/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/AllowAllAccessControl.java index 8893e5a0caa6..6c5b16c0e095 100644 --- a/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/AllowAllAccessControl.java +++ b/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/AllowAllAccessControl.java @@ -17,6 +17,7 @@ import io.prestosql.spi.connector.ConnectorTransactionHandle; import io.prestosql.spi.connector.SchemaTableName; import io.prestosql.spi.security.ConnectorIdentity; +import io.prestosql.spi.security.PrestoPrincipal; import io.prestosql.spi.security.Privilege; import java.util.Set; @@ -127,12 +128,12 @@ public void checkCanSetCatalogSessionProperty(ConnectorTransactionHandle transac } @Override - public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String grantee, boolean withGrantOption) + public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal grantee, boolean withGrantOption) { } @Override - public void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String revokee, boolean grantOptionFor) + public void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal revokee, boolean grantOptionFor) { } } diff --git a/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/FileBasedAccessControl.java b/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/FileBasedAccessControl.java index 4ca2ba4489ff..952883c7f871 100644 --- a/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/FileBasedAccessControl.java +++ b/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/FileBasedAccessControl.java @@ -20,6 +20,7 @@ import io.prestosql.spi.connector.SchemaTableName; import io.prestosql.spi.security.AccessDeniedException; import io.prestosql.spi.security.ConnectorIdentity; +import io.prestosql.spi.security.PrestoPrincipal; import io.prestosql.spi.security.Privilege; import javax.inject.Inject; @@ -222,7 +223,7 @@ public void checkCanSetCatalogSessionProperty(ConnectorTransactionHandle transac } @Override - public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String grantee, boolean withGrantOption) + public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal grantee, boolean withGrantOption) { if (!checkTablePermission(identity, tableName, OWNERSHIP)) { denyGrantTablePrivilege(privilege.name(), tableName.toString()); @@ -230,7 +231,7 @@ public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transaction, } @Override - public void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String revokee, boolean grantOptionFor) + public void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal revokee, boolean grantOptionFor) { if (!checkTablePermission(identity, tableName, OWNERSHIP)) { denyRevokeTablePrivilege(privilege.name(), tableName.toString()); diff --git a/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/ForwardingConnectorAccessControl.java b/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/ForwardingConnectorAccessControl.java index 4fe285bcdf17..13a429c67412 100644 --- a/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/ForwardingConnectorAccessControl.java +++ b/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/ForwardingConnectorAccessControl.java @@ -165,13 +165,13 @@ public void checkCanSetCatalogSessionProperty(ConnectorTransactionHandle transac } @Override - public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String grantee, boolean withGrantOption) + public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal grantee, boolean withGrantOption) { delegate().checkCanGrantTablePrivilege(transactionHandle, identity, privilege, tableName, grantee, withGrantOption); } @Override - public void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String revokee, boolean grantOptionFor) + public void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal revokee, boolean grantOptionFor) { delegate().checkCanGrantTablePrivilege(transactionHandle, identity, privilege, tableName, revokee, grantOptionFor); } diff --git a/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/ForwardingSystemAccessControl.java b/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/ForwardingSystemAccessControl.java index 1c54c17977e1..4ee0c0c6c52b 100644 --- a/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/ForwardingSystemAccessControl.java +++ b/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/ForwardingSystemAccessControl.java @@ -17,6 +17,7 @@ import io.prestosql.spi.connector.CatalogSchemaTableName; import io.prestosql.spi.connector.SchemaTableName; import io.prestosql.spi.security.Identity; +import io.prestosql.spi.security.PrestoPrincipal; import io.prestosql.spi.security.Privilege; import io.prestosql.spi.security.SystemAccessControl; @@ -190,13 +191,13 @@ public void checkCanSetCatalogSessionProperty(Identity identity, String catalogN } @Override - public void checkCanGrantTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, String grantee, boolean withGrantOption) + public void checkCanGrantTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, PrestoPrincipal grantee, boolean withGrantOption) { delegate().checkCanGrantTablePrivilege(identity, privilege, table, grantee, withGrantOption); } @Override - public void checkCanRevokeTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, String revokee, boolean grantOptionFor) + public void checkCanRevokeTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, PrestoPrincipal revokee, boolean grantOptionFor) { delegate().checkCanRevokeTablePrivilege(identity, privilege, table, revokee, grantOptionFor); } diff --git a/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/ReadOnlyAccessControl.java b/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/ReadOnlyAccessControl.java index dde7b55b70f9..ec7586d2ba45 100644 --- a/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/ReadOnlyAccessControl.java +++ b/presto-plugin-toolkit/src/main/java/io/prestosql/plugin/base/security/ReadOnlyAccessControl.java @@ -17,6 +17,7 @@ import io.prestosql.spi.connector.ConnectorTransactionHandle; import io.prestosql.spi.connector.SchemaTableName; import io.prestosql.spi.security.ConnectorIdentity; +import io.prestosql.spi.security.PrestoPrincipal; import io.prestosql.spi.security.Privilege; import java.util.Set; @@ -138,13 +139,13 @@ public void checkCanSetCatalogSessionProperty(ConnectorTransactionHandle transac } @Override - public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String grantee, boolean withGrantOption) + public void checkCanGrantTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal grantee, boolean withGrantOption) { denyGrantTablePrivilege(privilege.name(), tableName.toString()); } @Override - public void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String revokee, boolean grantOptionFor) + public void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transaction, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal revokee, boolean grantOptionFor) { denyRevokeTablePrivilege(privilege.name(), tableName.toString()); } diff --git a/presto-product-tests/src/main/java/io/prestosql/tests/hive/TestGrantRevoke.java b/presto-product-tests/src/main/java/io/prestosql/tests/hive/TestGrantRevoke.java index b4f69f60ae20..964913b8ed85 100644 --- a/presto-product-tests/src/main/java/io/prestosql/tests/hive/TestGrantRevoke.java +++ b/presto-product-tests/src/main/java/io/prestosql/tests/hive/TestGrantRevoke.java @@ -30,6 +30,7 @@ import static io.prestosql.tests.TestGroups.PROFILE_SPECIFIC_TESTS; import static io.prestosql.tests.utils.QueryExecutors.connectToPresto; import static io.prestosql.tests.utils.QueryExecutors.onHive; +import static io.prestosql.tests.utils.QueryExecutors.onPresto; import static java.lang.String.format; public class TestGrantRevoke @@ -120,13 +121,13 @@ public void testShowGrants() @Test(groups = {AUTHORIZATION, PROFILE_SPECIFIC_TESTS}) public void testAll() { - aliceExecutor.executeQuery(format("GRANT ALL PRIVILEGES ON %s TO bob", tableName)); + aliceExecutor.executeQuery(format("GRANT ALL PRIVILEGES ON %s TO USER bob", tableName)); assertThat(bobExecutor.executeQuery(format("INSERT INTO %s VALUES (4, 13)", tableName))).hasRowsCount(1); assertThat(bobExecutor.executeQuery(format("SELECT * FROM %s", tableName))).hasRowsCount(1); bobExecutor.executeQuery(format("DELETE FROM %s", tableName)); assertThat(bobExecutor.executeQuery(format("SELECT * FROM %s", tableName))).hasNoRows(); - aliceExecutor.executeQuery(format("REVOKE ALL PRIVILEGES ON %s FROM bob", tableName)); + aliceExecutor.executeQuery(format("REVOKE ALL PRIVILEGES ON %s FROM USER bob", tableName)); assertAccessDeniedOnAllOperationsOnTable(bobExecutor, tableName); assertThat(bobExecutor.executeQuery(format("SHOW GRANTS ON %s", tableName))).hasNoRows(); @@ -135,9 +136,53 @@ public void testAll() @Test(groups = {AUTHORIZATION, PROFILE_SPECIFIC_TESTS}) public void testPublic() { - aliceExecutor.executeQuery(format("GRANT SELECT ON %s TO PUBLIC", tableName)); + aliceExecutor.executeQuery(format("GRANT SELECT ON %s TO ROLE PUBLIC", tableName)); assertThat(bobExecutor.executeQuery(format("SELECT * FROM %s", tableName))).hasNoRows(); - aliceExecutor.executeQuery(format("REVOKE SELECT ON %s FROM PUBLIC", tableName)); + aliceExecutor.executeQuery(format("REVOKE SELECT ON %s FROM ROLE PUBLIC", tableName)); + assertThat(() -> bobExecutor.executeQuery(format("SELECT * FROM %s", tableName))) + .failsWithMessage(format("Access Denied: Cannot select from table default.%s", tableName)); + assertThat(aliceExecutor.executeQuery(format("SELECT * FROM %s", tableName))).hasNoRows(); + } + + @Test(groups = {AUTHORIZATION, PROFILE_SPECIFIC_TESTS}) + public void testCustomRole() + { + onPresto().executeQuery("CREATE ROLE role1"); + onPresto().executeQuery("GRANT role1 TO USER bob"); + aliceExecutor.executeQuery(format("GRANT SELECT ON %s TO ROLE role1", tableName)); + assertThat(bobExecutor.executeQuery(format("SELECT * FROM %s", tableName))).hasNoRows(); + aliceExecutor.executeQuery(format("REVOKE SELECT ON %s FROM ROLE role1", tableName)); + assertThat(() -> bobExecutor.executeQuery(format("SELECT * FROM %s", tableName))) + .failsWithMessage(format("Access Denied: Cannot select from table default.%s", tableName)); + assertThat(aliceExecutor.executeQuery(format("SELECT * FROM %s", tableName))).hasNoRows(); + onPresto().executeQuery("DROP ROLE role1"); + } + + @Test(groups = {AUTHORIZATION, PROFILE_SPECIFIC_TESTS}) + public void testTransitiveRole() + { + onPresto().executeQuery("CREATE ROLE role1"); + onPresto().executeQuery("CREATE ROLE role2"); + onPresto().executeQuery("GRANT role1 TO USER bob"); + onPresto().executeQuery("GRANT role2 TO ROLE role1"); + aliceExecutor.executeQuery(format("GRANT SELECT ON %s TO ROLE role2", tableName)); + assertThat(bobExecutor.executeQuery(format("SELECT * FROM %s", tableName))).hasNoRows(); + aliceExecutor.executeQuery(format("REVOKE SELECT ON %s FROM ROLE role2", tableName)); + assertThat(() -> bobExecutor.executeQuery(format("SELECT * FROM %s", tableName))) + .failsWithMessage(format("Access Denied: Cannot select from table default.%s", tableName)); + assertThat(aliceExecutor.executeQuery(format("SELECT * FROM %s", tableName))).hasNoRows(); + onPresto().executeQuery("DROP ROLE role1"); + onPresto().executeQuery("DROP ROLE role2"); + } + + @Test(groups = {AUTHORIZATION, PROFILE_SPECIFIC_TESTS}) + public void testDropRoleWithPermissionsGranted() + { + onPresto().executeQuery("CREATE ROLE role1"); + onPresto().executeQuery("GRANT role1 TO USER bob"); + aliceExecutor.executeQuery(format("GRANT SELECT ON %s TO ROLE role1", tableName)); + assertThat(bobExecutor.executeQuery(format("SELECT * FROM %s", tableName))).hasNoRows(); + onPresto().executeQuery("DROP ROLE role1"); assertThat(() -> bobExecutor.executeQuery(format("SELECT * FROM %s", tableName))) .failsWithMessage(format("Access Denied: Cannot select from table default.%s", tableName)); assertThat(aliceExecutor.executeQuery(format("SELECT * FROM %s", tableName))).hasNoRows(); diff --git a/presto-spi/src/main/java/io/prestosql/spi/connector/ConnectorAccessControl.java b/presto-spi/src/main/java/io/prestosql/spi/connector/ConnectorAccessControl.java index 4b27bb46d6fa..b23e2d61e9f8 100644 --- a/presto-spi/src/main/java/io/prestosql/spi/connector/ConnectorAccessControl.java +++ b/presto-spi/src/main/java/io/prestosql/spi/connector/ConnectorAccessControl.java @@ -258,7 +258,7 @@ default void checkCanSetCatalogSessionProperty(ConnectorTransactionHandle transa * * @throws io.prestosql.spi.security.AccessDeniedException if not allowed */ - default void checkCanGrantTablePrivilege(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String grantee, boolean withGrantOption) + default void checkCanGrantTablePrivilege(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal grantee, boolean withGrantOption) { denyGrantTablePrivilege(privilege.toString(), tableName.toString()); } @@ -268,7 +268,7 @@ default void checkCanGrantTablePrivilege(ConnectorTransactionHandle transactionH * * @throws io.prestosql.spi.security.AccessDeniedException if not allowed */ - default void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, String revokee, boolean grantOptionFor) + default void checkCanRevokeTablePrivilege(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, Privilege privilege, SchemaTableName tableName, PrestoPrincipal revokee, boolean grantOptionFor) { denyRevokeTablePrivilege(privilege.toString(), tableName.toString()); } diff --git a/presto-spi/src/main/java/io/prestosql/spi/connector/ConnectorMetadata.java b/presto-spi/src/main/java/io/prestosql/spi/connector/ConnectorMetadata.java index b3e0d09123d1..27a0cd47e922 100644 --- a/presto-spi/src/main/java/io/prestosql/spi/connector/ConnectorMetadata.java +++ b/presto-spi/src/main/java/io/prestosql/spi/connector/ConnectorMetadata.java @@ -495,7 +495,7 @@ default Set listEnabledRoles(ConnectorSession session) /** * Grants the specified privilege to the specified user on the specified table */ - default void grantTablePrivileges(ConnectorSession session, SchemaTableName tableName, Set privileges, String grantee, boolean grantOption) + default void grantTablePrivileges(ConnectorSession session, SchemaTableName tableName, Set privileges, PrestoPrincipal grantee, boolean grantOption) { throw new PrestoException(NOT_SUPPORTED, "This connector does not support grants"); } @@ -503,7 +503,7 @@ default void grantTablePrivileges(ConnectorSession session, SchemaTableName tabl /** * Revokes the specified privilege on the specified table from the specified user */ - default void revokeTablePrivileges(ConnectorSession session, SchemaTableName tableName, Set privileges, String grantee, boolean grantOption) + default void revokeTablePrivileges(ConnectorSession session, SchemaTableName tableName, Set privileges, PrestoPrincipal grantee, boolean grantOption) { throw new PrestoException(NOT_SUPPORTED, "This connector does not support revokes"); } diff --git a/presto-spi/src/main/java/io/prestosql/spi/connector/classloader/ClassLoaderSafeConnectorMetadata.java b/presto-spi/src/main/java/io/prestosql/spi/connector/classloader/ClassLoaderSafeConnectorMetadata.java index 97cccaaa7d8b..4e4855468d1f 100644 --- a/presto-spi/src/main/java/io/prestosql/spi/connector/classloader/ClassLoaderSafeConnectorMetadata.java +++ b/presto-spi/src/main/java/io/prestosql/spi/connector/classloader/ClassLoaderSafeConnectorMetadata.java @@ -486,7 +486,7 @@ public Set listEnabledRoles(ConnectorSession session) } @Override - public void grantTablePrivileges(ConnectorSession session, SchemaTableName tableName, Set privileges, String grantee, boolean grantOption) + public void grantTablePrivileges(ConnectorSession session, SchemaTableName tableName, Set privileges, PrestoPrincipal grantee, boolean grantOption) { try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) { delegate.grantTablePrivileges(session, tableName, privileges, grantee, grantOption); @@ -494,7 +494,7 @@ public void grantTablePrivileges(ConnectorSession session, SchemaTableName table } @Override - public void revokeTablePrivileges(ConnectorSession session, SchemaTableName tableName, Set privileges, String grantee, boolean grantOption) + public void revokeTablePrivileges(ConnectorSession session, SchemaTableName tableName, Set privileges, PrestoPrincipal grantee, boolean grantOption) { try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) { delegate.revokeTablePrivileges(session, tableName, privileges, grantee, grantOption); diff --git a/presto-spi/src/main/java/io/prestosql/spi/security/SystemAccessControl.java b/presto-spi/src/main/java/io/prestosql/spi/security/SystemAccessControl.java index d07024bfdf04..10d23a24f2ed 100644 --- a/presto-spi/src/main/java/io/prestosql/spi/security/SystemAccessControl.java +++ b/presto-spi/src/main/java/io/prestosql/spi/security/SystemAccessControl.java @@ -287,7 +287,7 @@ default void checkCanSetCatalogSessionProperty(Identity identity, String catalog * * @throws AccessDeniedException if not allowed */ - default void checkCanGrantTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, String grantee, boolean withGrantOption) + default void checkCanGrantTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, PrestoPrincipal grantee, boolean withGrantOption) { denyGrantTablePrivilege(privilege.toString(), table.toString()); } @@ -297,7 +297,7 @@ default void checkCanGrantTablePrivilege(Identity identity, Privilege privilege, * * @throws AccessDeniedException if not allowed */ - default void checkCanRevokeTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, String revokee, boolean grantOptionFor) + default void checkCanRevokeTablePrivilege(Identity identity, Privilege privilege, CatalogSchemaTableName table, PrestoPrincipal revokee, boolean grantOptionFor) { denyRevokeTablePrivilege(privilege.toString(), table.toString()); }