Skip to content

Commit

Permalink
IMPALA-6650: Add fine-grained DROP privilege
Browse files Browse the repository at this point in the history 
Add support for executing DROP statements by granting DROP privilege.

These are the new GRANT/REVOKE statements introduced at server,
database, and table scopes.

GRANT DROP on SERVER svr TO ROLE testrole;
GRANT DROP on DATABASE db TO ROLE testrole;
GRANT DROP on TABLE tbl TO ROLE testrole;

REVOKE DROP on SERVER svr FROM ROLE testrole;
REVOKE DROP on DATABASE db FROM ROLE testrole;
REVOKE DROP on TABLE tbl FROM ROLE testrole;

Testing:
- Added new front-end tests
- Ran front-end tests

Change-Id: Iea1dfb0b117cb1725e9656b9a79a9aebee82b5e4
Reviewed-on: http://gerrit.cloudera.org:8080/9911
Reviewed-by: Alex Behm <alex.behm@cloudera.com>
Tested-by: Impala Public Jenkins
  • Loading branch information
@fredyw @cloudera-hudson
fredyw authored and cloudera-hudson committed Apr 25, 2018
1 parent 99f001f commit a7cfdb7
Show file tree
Hide file tree
Showing 11 changed files with 157 additions and 30 deletions.
3 changes: 2 additions & 1 deletion common/thrift/CatalogObjects.thrift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,8 @@ enum TPrivilegeLevel {
SELECT,
REFRESH,
CREATE,
ALTER
ALTER,
DROP
}

// Represents a privilege in an authorization policy. Privileges contain the level
Expand Down
3 changes: 3 additions & 0 deletions fe/.gitignore
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ src/test/resources/authz-policy.ini
# Generated minicluster config
src/test/resources/minicluster-conf.xml

# Generated hive-log4j2.properties file
src/test/resources/hive-log4j2.properties

# Generated thrift files
generated-sources

Expand Down
2 changes: 2 additions & 0 deletions fe/src/main/cup/sql-parser.cup
View file
Original file line number Diff line number Diff line change
Expand Up @@ -966,6 +966,8 @@ privilege ::=
{: RESULT = TPrivilegeLevel.CREATE; :}
| KW_ALTER
{: RESULT = TPrivilegeLevel.ALTER; :}
| KW_DROP
{: RESULT = TPrivilegeLevel.DROP; :}
| KW_ALL
{: RESULT = TPrivilegeLevel.ALL; :}
;
Expand Down
7 changes: 2 additions & 5 deletions fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,10 @@ public void analyze(Analyzer analyzer) throws AnalysisException {
false);
}

// For now, if authorization is enabled, the user needs ALL on the server
// to drop functions.
// TODO: this is not the right granularity but acceptable for now.
analyzer.registerPrivReq(new PrivilegeRequest(
new AuthorizeableFn(desc_.dbName(), desc_.signatureString()), Privilege.ALL));
new AuthorizeableFn(desc_.dbName(), desc_.signatureString()), Privilege.DROP));

Db db = analyzer.getDb(desc_.dbName(), Privilege.DROP, false);
Db db = analyzer.getDb(desc_.dbName(), false);
if (db == null && !ifExists_) {
throw new AnalysisException(Analyzer.DB_DOES_NOT_EXIST_ERROR_MSG + desc_.dbName());
}
Expand Down
7 changes: 4 additions & 3 deletions fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,10 @@ public void analyze(Analyzer analyzer) throws AnalysisException {
if (privilegeLevel_ != TPrivilegeLevel.ALL &&
privilegeLevel_ != TPrivilegeLevel.REFRESH &&
privilegeLevel_ != TPrivilegeLevel.CREATE &&
privilegeLevel_ != TPrivilegeLevel.ALTER) {
throw new AnalysisException("Only 'ALL', 'REFRESH', 'CREATE', or 'ALTER' " +
"privilege may be applied at SERVER scope in privilege spec.");
privilegeLevel_ != TPrivilegeLevel.ALTER &&
privilegeLevel_ != TPrivilegeLevel.DROP) {
throw new AnalysisException("Only 'ALL', 'REFRESH', 'CREATE', 'ALTER', or " +
"'DROP' privilege may be applied at SERVER scope in privilege spec.");
}
break;
case DATABASE:
Expand Down
6 changes: 3 additions & 3 deletions fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,13 +125,13 @@ public void checkAccess(User user, PrivilegeRequest privilegeRequest)
Preconditions.checkNotNull(privilegeRequest);

if (!hasAccess(user, privilegeRequest)) {
Privilege privilege = privilegeRequest.getPrivilege();
if (privilegeRequest.getAuthorizeable() instanceof AuthorizeableFn) {
throw new AuthorizationException(String.format(
"User '%s' does not have privileges to CREATE/DROP functions in: %s",
user.getName(), privilegeRequest.getName()));
"User '%s' does not have privileges to %s functions in: %s",
user.getName(), privilege, privilegeRequest.getName()));
}

Privilege privilege = privilegeRequest.getPrivilege();
if (EnumSet.of(Privilege.ANY, Privilege.ALL, Privilege.VIEW_METADATA)
.contains(privilege)) {
throw new AuthorizationException(String.format(
Expand Down
3 changes: 2 additions & 1 deletion fe/src/main/java/org/apache/impala/authorization/Privilege.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
public enum Privilege {
ALL(SentryAction.ALL, false),
ALTER(SentryAction.ALTER, false),
DROP(SentryAction.ALL, false),
DROP(SentryAction.DROP, false),
CREATE(SentryAction.CREATE, false),
INSERT(SentryAction.INSERT, false),
SELECT(SentryAction.SELECT, false),
Expand Down Expand Up @@ -56,6 +56,7 @@ public enum SentryAction implements Action {
REFRESH("refresh"),
CREATE("create"),
ALTER("alter"),
DROP("drop"),
ALL("*");

private final String value;
Expand Down
18 changes: 14 additions & 4 deletions fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,8 @@ public void AnalyzeGrantRevokePriv() throws AnalysisException {
AnalyzesOk(String.format("%s INSERT ON DATABASE functional %s myrole",
formatArgs));
AnalysisError(String.format("%s INSERT ON SERVER %s myrole", formatArgs),
"Only 'ALL', 'REFRESH', 'CREATE', or 'ALTER' privilege may be applied at " +
"SERVER scope in privilege spec.");
"Only 'ALL', 'REFRESH', 'CREATE', 'ALTER', or 'DROP' privilege may be " +
"applied at SERVER scope in privilege spec.");
AnalysisError(String.format("%s INSERT ON URI 'hdfs:////abc//123' %s myrole",
formatArgs), "Only 'ALL' privilege may be applied at URI scope in privilege " +
"spec.");
Expand All @@ -181,8 +181,8 @@ public void AnalyzeGrantRevokePriv() throws AnalysisException {
AnalyzesOk(String.format("%s SELECT ON DATABASE functional %s myrole",
formatArgs));
AnalysisError(String.format("%s SELECT ON SERVER %s myrole", formatArgs),
"Only 'ALL', 'REFRESH', 'CREATE', or 'ALTER' privilege may be applied at " +
"SERVER scope in privilege spec.");
"Only 'ALL', 'REFRESH', 'CREATE', 'ALTER', or 'DROP' privilege may be " +
"applied at SERVER scope in privilege spec.");
AnalysisError(String.format("%s SELECT ON URI 'hdfs:////abc//123' %s myrole",
formatArgs), "Only 'ALL' privilege may be applied at URI scope in privilege " +
"spec.");
Expand Down Expand Up @@ -255,6 +255,16 @@ public void AnalyzeGrantRevokePriv() throws AnalysisException {
AnalysisError(String.format(
"%s ALTER ON URI 'hdfs:////abc/123' %s myrole", formatArgs),
"Only 'ALL' privilege may be applied at URI scope in privilege spec.");

// DROP privilege
AnalyzesOk(String.format("%s DROP ON SERVER %s myrole", formatArgs));
AnalyzesOk(String.format("%s DROP ON SERVER server1 %s myrole", formatArgs));
AnalyzesOk(String.format("%s DROP ON DATABASE functional %s myrole", formatArgs));
AnalyzesOk(String.format(
"%s DROP ON TABLE functional.alltypes %s myrole", formatArgs));
AnalysisError(String.format(
"%s DROP ON URI 'hdfs:////abc/123' %s myrole", formatArgs),
"Only 'ALL' privilege may be applied at URI scope in privilege spec.");
}

AnalysisContext authDisabledCtx = createAuthDisabledAnalysisCtx();
Expand Down
124 changes: 112 additions & 12 deletions fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.apache.impala.catalog.Db;
import org.apache.impala.catalog.ImpaladCatalog;
import org.apache.impala.catalog.ScalarFunction;
import org.apache.impala.catalog.ScalarType;
import org.apache.impala.catalog.Type;
import org.apache.impala.common.AnalysisException;
import org.apache.impala.common.FrontendTestBase;
Expand Down Expand Up @@ -80,11 +81,11 @@ public class AuthorizationTest extends FrontendTestBase {
// ALL permission on 'tpch' database and 'newdb' database
// ALL permission on 'functional_seq_snap' database
// SELECT permissions on all tables in 'tpcds' database
// SELECT, REFRESH permissions on 'functional.alltypesagg' (no INSERT permissions)
// SELECT, REFRESH, DROP permissions on 'functional.alltypesagg'
// ALTER permissions on 'functional.alltypeserror'
// SELECT permissions on 'functional.complex_view' (no INSERT permissions)
// SELECT, REFRESH permissions on 'functional.view_view' (no INSERT permissions)
// ALTER permission on 'functional.alltypes_view'
// SELECT permissions on 'functional.complex_view'
// SELECT, REFRESH permissions on 'functional.view_view'
// ALTER, DROP permissions on 'functional.alltypes_view'
// SELECT permissions on columns ('id', 'int_col', and 'year') on
// 'functional.alltypessmall' (no SELECT permissions on 'functional.alltypessmall')
// SELECT permissions on columns ('id', 'int_struct_col', 'struct_array_col',
Expand All @@ -98,7 +99,7 @@ public class AuthorizationTest extends FrontendTestBase {
// No permissions on database 'functional_rc'
// Only column level permissions in 'functional_avro':
// SELECT permissions on columns ('id') on 'functional_avro.alltypessmall'
// REFRESH, INSERT, CREATE, ALTER permissions on 'functional_text_lzo' database
// REFRESH, INSERT, CREATE, ALTER, DROP permissions on 'functional_text_lzo' database
public final static String AUTHZ_POLICY_FILE = "/test-warehouse/authz-policy.ini";
public final static User USER = new User(System.getProperty("user.name"));

Expand Down Expand Up @@ -260,6 +261,18 @@ private static void setupSentryService(AuthorizationConfig authzConfig)
privilege.setTable_name("alltypesagg");
sentryService.grantRolePrivilege(USER, roleName, privilege);

// drop_functional_alltypesagg
roleName = "drop_functional_alltypesagg";
sentryService.createRole(USER, roleName, true);
sentryService.grantRoleToGroup(USER, roleName, USER.getName());

privilege = new TPrivilege("", TPrivilegeLevel.DROP,
TPrivilegeScope.TABLE, false);
privilege.setServer_name("server1");
privilege.setDb_name("functional");
privilege.setTable_name("alltypesagg");
sentryService.grantRolePrivilege(USER, roleName, privilege);

// refresh_functional_view_view
roleName = "refresh_functional_view_view";
sentryService.createRole(USER, roleName, true);
Expand All @@ -272,6 +285,18 @@ private static void setupSentryService(AuthorizationConfig authzConfig)
privilege.setTable_name("view_view");
sentryService.grantRolePrivilege(USER, roleName, privilege);

// drop_functional_alltypes_view
roleName = "drop_functional_alltypes_view";
sentryService.createRole(USER, roleName, true);
sentryService.grantRoleToGroup(USER, roleName, USER.getName());

privilege = new TPrivilege("", TPrivilegeLevel.DROP,
TPrivilegeScope.TABLE, false);
privilege.setServer_name("server1");
privilege.setDb_name("functional");
privilege.setTable_name("alltypes_view");
sentryService.grantRolePrivilege(USER, roleName, privilege);

// insert_functional_text_lzo
roleName = "insert_functional_text_lzo";
sentryService.createRole(USER, roleName, true);
Expand Down Expand Up @@ -308,6 +333,18 @@ private static void setupSentryService(AuthorizationConfig authzConfig)
privilege.setTable_name(AuthorizeableTable.ANY_TABLE_NAME);
sentryService.grantRolePrivilege(USER, roleName, privilege);

// drop_functional_text_lzo
roleName = "drop_functional_text_lzo";
sentryService.createRole(USER, roleName, true);
sentryService.grantRoleToGroup(USER, roleName, USER.getName());

privilege = new TPrivilege("", TPrivilegeLevel.DROP, TPrivilegeScope.DATABASE,
false);
privilege.setServer_name("server1");
privilege.setDb_name("functional_text_lzo");
privilege.setTable_name(AuthorizeableTable.ANY_TABLE_NAME);
sentryService.grantRolePrivilege(USER, roleName, privilege);

// alter_functional_alltypeserror
roleName = "alter_functional_alltypeserror";
sentryService.createRole(USER, roleName, true);
Expand Down Expand Up @@ -1248,6 +1285,15 @@ public void TestDropDatabase() throws ImpalaException {
AuthzOk("drop database if exists newdb");
AuthzOk("drop database if exists newdb cascade");
AuthzOk("drop database if exists newdb restrict");

// User has DROP privilege on functional_text_lzo database.
AuthzOk("drop database functional_text_lzo");
AuthzOk("drop database functional_text_lzo cascade");
AuthzOk("drop database functional_text_lzo restrict");
AuthzOk("drop database if exists functional_text_lzo");
AuthzOk("drop database if exists functional_text_lzo cascade");
AuthzOk("drop database if exists functional_text_lzo restrict");

// User has permission, database does not exists, IF EXISTS not specified.
try {
AuthzOk("drop database newdb");
Expand Down Expand Up @@ -1298,6 +1344,9 @@ public void TestDropTable() throws ImpalaException {
AuthzOk("drop table tpch.lineitem");
AuthzOk("drop table if exists tpch.lineitem");

// User has DROP privilege on functional.alltypesagg table.
AuthzOk("drop table functional.alltypesagg");

// Drop table (user does not have permission).
AuthzError("drop table functional.alltypes",
"User '%s' does not have privileges to execute 'DROP' on: functional.alltypes");
Expand Down Expand Up @@ -1334,10 +1383,13 @@ public void TestDropView() throws ImpalaException {
AuthzOk("drop view functional_seq_snap.alltypes_view");
AuthzOk("drop view if exists functional_seq_snap.alltypes_view");

// Drop view (user does not have permission).
AuthzError("drop view functional.alltypes_view",
// User has DROP privilege on functional.alltypes_view view.
AuthzOk("drop view functional.alltypes_view");

// User does not have DROP privilege on functional.alltypes_view_sub view.
AuthzError("drop view functional.alltypes_view_sub",
"User '%s' does not have privileges to execute 'DROP' on: functional.alltypes");
AuthzError("drop view if exists functional.alltypes_view",
AuthzError("drop view if exists functional.alltypes_view_sub",
"User '%s' does not have privileges to execute 'DROP' on: functional.alltypes");

// Drop view with unqualified table name.
Expand Down Expand Up @@ -2207,7 +2259,7 @@ public void TestFunction() throws Exception {

AuthzError(ctx, "create function f() returns int location " +
"'/test-warehouse/libTestUdfs.so' symbol='NoArgs'",
"User '%s' does not have privileges to CREATE/DROP functions in: default.f()");
"User '%s' does not have privileges to CREATE functions in: default.f()");

// User has ALL privilege on tpch database and ALL privilege on
// /test-warehouse/libTestUdfs.so URI.
Expand All @@ -2216,13 +2268,25 @@ public void TestFunction() throws Exception {

AuthzError(ctx, "create function notdb.f() returns int location " +
"'/test-warehouse/libTestUdfs.so' symbol='NoArgs'",
"User '%s' does not have privileges to CREATE/DROP functions in: notdb.f()");
"User '%s' does not have privileges to CREATE functions in: notdb.f()");

// User has DROP privilege on functional_text_lzo database.
try {
ctx_.catalog.addFunction(ScalarFunction.createForTesting("functional_text_lzo",
"f", new ArrayList<Type>(), Type.INT, "/dummy", "dummy.class", null,
null, TFunctionBinaryType.NATIVE));
AuthzOk("drop function functional_text_lzo.f()");
} finally {
ctx_.catalog.removeFunction(ScalarFunction.createForTesting("functional_text_lzo",
"f", new ArrayList<Type>(), Type.INT, "/dummy", "dummy.class", null,
null, TFunctionBinaryType.NATIVE));
}

AuthzError(ctx, "drop function if exists f()",
"User '%s' does not have privileges to CREATE/DROP functions in: default.f()");
"User '%s' does not have privileges to DROP functions in: default.f()");

AuthzError(ctx, "drop function notdb.f()",
"User '%s' does not have privileges to CREATE/DROP functions in: notdb.f()");
"User '%s' does not have privileges to DROP functions in: notdb.f()");

// User does not have ALL privilege on SERVER and tries to create a function with
// the same name as the built-in function.
Expand Down Expand Up @@ -2519,6 +2583,42 @@ public void TestServerLevelAlter() throws ImpalaException {
}
}

@Test
public void TestServerLevelDrop() throws ImpalaException {
// TODO: Add test support for dynamically changing privileges for
// file-based policy.
if (ctx_.authzConfig.isFileBasedPolicy()) return;

SentryPolicyService sentryService =
new SentryPolicyService(ctx_.authzConfig.getSentryConfig());

// User has DROP privilege on server.
String roleName = "drop_role";
try {
sentryService.createRole(USER, roleName, true);
TPrivilege privilege = new TPrivilege("", TPrivilegeLevel.DROP,
TPrivilegeScope.SERVER, false);
privilege.setServer_name("server1");
sentryService.grantRolePrivilege(USER, roleName, privilege);
sentryService.grantRoleToGroup(USER, roleName, USER.getName());
ctx_.catalog.reset();

AuthzOk("drop database functional");
AuthzOk("drop table functional.alltypes");
AuthzOk("drop view functional.alltypes_view_sub");
ctx_.catalog.addFunction(ScalarFunction.createForTesting("functional",
"f", new ArrayList<Type>(), Type.INT, "/dummy", "dummy.class", null,
null, TFunctionBinaryType.NATIVE));
AuthzOk("drop function functional.f()");
} finally {
sentryService.dropRole(USER, roleName, true);
ctx_.catalog.reset();
ctx_.catalog.addFunction(ScalarFunction.createForTesting("functional",
"f", new ArrayList<Type>(), Type.INT, "/dummy", "dummy.class", null,
null, TFunctionBinaryType.NATIVE));
}
}

private void TestWithIncorrectConfig(AuthorizationConfig authzConfig, User user)
throws ImpalaException {
Frontend fe = new Frontend(authzConfig, ctx_.catalog);
Expand Down
6 changes: 6 additions & 0 deletions fe/src/test/java/org/apache/impala/analysis/ParserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3592,6 +3592,12 @@ public void TestGrantRevokePrivilege() {
ParsesOk(String.format("%s ALTER ON DATABASE foo %s myRole", formatStr));
ParsesOk(String.format("%s ALTER ON TABLE foo %s myRole", formatStr));

// DROP privilege.
ParsesOk(String.format("%s DROP ON SERVER %s myRole", formatStr));
ParsesOk(String.format("%s DROP ON SERVER foo %s myRole", formatStr));
ParsesOk(String.format("%s DROP ON DATABASE foo %s myRole", formatStr));
ParsesOk(String.format("%s DROP ON TABLE foo %s myRole", formatStr));

// Server scope does not accept a name.
ParsesOk(String.format("%s ALL ON SERVER %s myRole", formatStr));
ParsesOk(String.format("%s INSERT ON SERVER %s myRole", formatStr));
Expand Down
8 changes: 7 additions & 1 deletion fe/src/test/resources/authz-policy.ini.template
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ ${USER} = all_tpch, all_newdb, all_functional_seq_snap, select_tpcds,\
refresh_functional_view_view, insert_functional_text_lzo,\
create_functional_text_lzo, alter_functional_text_lzo,\
alter_functional_alltypeserror, alter_functional_alltypes_view,\
libtestudfs_uri
libtestudfs_uri, drop_functional_text_lzo, drop_functional_alltypesagg,\
drop_functional_alltypes_view
auth_to_local_group = test_role
server_admin = all_server

Expand All @@ -55,15 +56,20 @@ insert_parquet = server=server1->db=functional_parquet->table=*->action=insert
refresh_functional_text_lzo = server=server1->db=functional_text_lzo->action=refresh
refresh_functional_alltypesagg =\
server=server1->db=functional->table=alltypesagg->action=refresh
drop_functional_alltypesagg =\
server=server1->db=functional->table=alltypesagg->action=drop
alter_functional_alltypeserror =\
server=server1->db=functional->table=alltypeserror->action=alter
refresh_functional_view_view =\
server=server1->db=functional->table=view_view->action=refresh
drop_functional_alltypes_view =\
server=server1->db=functional->table=alltypes_view->action=drop
alter_functional_alltypes_view =\
server=server1->db=functional->table=alltypes_view->action=alter
insert_functional_text_lzo = server=server1->db=functional_text_lzo->action=insert
create_functional_text_lzo = server=server1->db=functional_text_lzo->action=create
alter_functionl_text_lzo = server=server1->db=functional_text_lzo->action=alter
drop_functional_text_lzo = server=server1->db=functional_text_lzo->action=drop
select_column_level_functional =\
server=server1->db=functional->table=alltypessmall->column=id->action=select,\
server=server1->db=functional->table=alltypessmall->column=int_col->action=select,\
Expand Down

0 comments on commit a7cfdb7

Please sign in to comment.