[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. IMPALA-8851: Do not throw authorization exception in drop if exists queries Note that this is the continuation of work in https://github.com/vihangk1/impala/commits/IMPALA-8851 This patch's goal is to change Impala's behavior in the following case: - the query is a DROP TABLE/VIEW/DATABASE/FUNCTIONS IF EXISTS statement - the given object does not exist - the user has some kind of privilege on the object, which imples the privilege to know whether object exists, but does not have DROP privilege on the object Until now this lead to an authorization exception, while it will be allowed with this change. An example where this is useful is a user who has CREATE privilege on a database, and creates table t_owned, and gets ownership of the table. In this case DROP TABLE IF EXISTS was non idempotent: DROP TABLE IF EXISTS t_owned; -> success DROP TABLE IF EXISTS t_owned; -> authorization error, as the privileges for the table were deleted when the table was successfully dropped After this change the second statement will be also successful. The authorization logic has to avoid leaking information that the user has no right to know. For this reason DROP IF EXISTS has to return the same error message regardless whether the object exists or not if the user has no right to know it's existence. This is achieved with the following pattern: - in the IF EXISTS case first an ANY privilege is registered, then the existence of the object is checked and if it doesn't exist, the analysis returns successfully - if the object exists, the DROP privilege is registered (if there is no IF EXISTS in the query, this always happens) - as the authorization logic checks privileges in the order of registration, first the ANY will be checked, and DROP will be only checked if the user has ANY privileges Testing: - Added a new test case in the sentry tests which confirms that the authorization exception is not thrown when a drop if exists query is issued on a object which does not exist. - Changed several tests affected by the new behavior. - Ran core tests. Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Reviewed-on: http://gerrit.cloudera.org:8080/14121 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java M fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestSentryGroupMapper.java M testdata/workloads/functional-query/queries/QueryTest/create-database.test M tests/authorization/test_owner_privileges.py M tests/common/sentry_cache_test_suite.py 11 files changed, 280 insertions(+), 38 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 12 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 11: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 11 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 29 Aug 2019 12:34:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 11: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 11 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 29 Aug 2019 08:28:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 11: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4873/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 11 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 29 Aug 2019 08:28:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 10: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4869/ -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 10 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 29 Aug 2019 00:58:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 10: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 10 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 28 Aug 2019 22:59:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 10: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4869/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 10 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 28 Aug 2019 20:54:51 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 10: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4420/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 10 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 28 Aug 2019 20:24:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 9: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4419/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 9 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 28 Aug 2019 20:18:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 10: (2 comments) PS 9 added new tests + some fixes for the issues caused by those tests. PS 10 is just removing some new lines. Carry +1 from Vihang http://gerrit.cloudera.org:8080/#/c/14121/10/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/14121/10/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2668 PS10, Line 2668: return getCatalog().getDb(dbName) != null; stmtTableCache.dbs turned out to be empty unless tables were loaded. http://gerrit.cloudera.org:8080/#/c/14121/10/fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java File fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/10/fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java@72 PS10, Line 72: // available in the toThrift() method. : serverName_ = analyzer.getServerName(); serverName_ has to be set before returning from the function. The new tests caught a null pointer exception in catalogd when this didn't happen. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 10 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 28 Aug 2019 19:48:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Hello Vihang Karajgaonkar, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14121 to look at the new patch set (#10). Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. IMPALA-8851: Do not throw authorization exception in drop if exists queries Note that this is the continuation of work in https://github.com/vihangk1/impala/commits/IMPALA-8851 This patch's goal is to change Impala's behavior in the following case: - the query is a DROP TABLE/VIEW/DATABASE/FUNCTIONS IF EXISTS statement - the given object does not exist - the user has some kind of privilege on the object, which imples the privilege to know whether object exists, but does not have DROP privilege on the object Until now this lead to an authorization exception, while it will be allowed with this change. An example where this is useful is a user who has CREATE privilege on a database, and creates table t_owned, and gets ownership of the table. In this case DROP TABLE IF EXISTS was non idempotent: DROP TABLE IF EXISTS t_owned; -> success DROP TABLE IF EXISTS t_owned; -> authorization error, as the privileges for the table were deleted when the table was successfully dropped After this change the second statement will be also successful. The authorization logic has to avoid leaking information that the user has no right to know. For this reason DROP IF EXISTS has to return the same error message regardless whether the object exists or not if the user has no right to know it's existence. This is achieved with the following pattern: - in the IF EXISTS case first an ANY privilege is registered, then the existence of the object is checked and if it doesn't exist, the analysis returns successfully - if the object exists, the DROP privilege is registered (if there is no IF EXISTS in the query, this always happens) - as the authorization logic checks privileges in the order of registration, first the ANY will be checked, and DROP will be only checked if the user has ANY privileges Testing: - Added a new test case in the sentry tests which confirms that the authorization exception is not thrown when a drop if exists query is issued on a object which does not exist. - Changed several tests affected by the new behavior. - Ran core tests. Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java M fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestSentryGroupMapper.java M testdata/workloads/functional-query/queries/QueryTest/create-database.test M tests/authorization/test_owner_privileges.py M tests/common/sentry_cache_test_suite.py 11 files changed, 280 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/14121/10 -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 10 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Hello Vihang Karajgaonkar, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14121 to look at the new patch set (#9). Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. IMPALA-8851: Do not throw authorization exception in drop if exists queries Note that this is the continuation of work in https://github.com/vihangk1/impala/commits/IMPALA-8851 This patch's goal is to change Impala's behavior in the following case: - the query is a DROP TABLE/VIEW/DATABASE/FUNCTIONS IF EXISTS statement - the given object does not exist - the user has some kind of privilege on the object, which imples the privilege to know whether object exists, but does not have DROP privilege on the object Until now this lead to an authorization exception, while it will be allowed with this change. An example where this is useful is a user who has CREATE privilege on a database, and creates table t_owned, and gets ownership of the table. In this case DROP TABLE IF EXISTS was non idempotent: DROP TABLE IF EXISTS t_owned; -> success DROP TABLE IF EXISTS t_owned; -> authorization error, as the privileges for the table were deleted when the table was successfully dropped After this change the second statement will be also successful. The authorization logic has to avoid leaking information that the user has no right to know. For this reason DROP IF EXISTS has to return the same error message regardless whether the object exists or not if the user has no right to know it's existence. This is achieved with the following pattern: - in the IF EXISTS case first an ANY privilege is registered, then the existence of the object is checked and if it doesn't exist, the analysis returns successfully - if the object exists, the DROP privilege is registered (if there is no IF EXISTS in the query, this always happens) - as the authorization logic checks privileges in the order of registration, first the ANY will be checked, and DROP will be only checked if the user has ANY privileges Testing: - Added a new test case in the sentry tests which confirms that the authorization exception is not thrown when a drop if exists query is issued on a object which does not exist. - Changed several tests affected by the new behavior. - Ran core tests. Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java M fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestSentryGroupMapper.java M testdata/workloads/functional-query/queries/QueryTest/create-database.test M tests/authorization/test_owner_privileges.py M tests/common/sentry_cache_test_suite.py 11 files changed, 282 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/14121/9 -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 9 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 8: > > Patch Set 8: Code-Review+1 > > would be good if we have Tim's +1 as well since this relates to > authorization of drop command. It looks good to me from my side. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 8 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 28 Aug 2019 18:19:22 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 8: > Patch Set 8: Code-Review+1 would be good if we have Tim's +1 as well since this relates to authorization of drop command. It looks to me from my side. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 8 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 28 Aug 2019 18:18:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java File fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java@117 PS6, Line 117: tableName_ > Yeah maybe we should require that callers pass in dbName_ and tableName_, s I confirmed that the current solution works and the gets the correct database from the session. Thanks for checking that the case of the table name is not a problem. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 6 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 28 Aug 2019 18:17:25 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 8: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 8 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 28 Aug 2019 18:18:00 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java File fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java@117 PS6, Line 117: tableName_ > Sorry for bugging you on this. Adding the check in the tableExists won't be Yeah maybe we should require that callers pass in dbName_ and tableName_, since it looks like all of these analyze() functions are computing the dbName_ already. With the case question, the TableName class equals() and hashCode() methods convert to lower case (since comparisons should be case insensitive). So it's OK so long as you are using the TableName object to look up the hash tables (which it looks like you are). -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 6 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 28 Aug 2019 00:51:28 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4401/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 8 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 27 Aug 2019 23:24:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Hello Vihang Karajgaonkar, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14121 to look at the new patch set (#8). Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. IMPALA-8851: Do not throw authorization exception in drop if exists queries Note that this is the continuation of work in https://github.com/vihangk1/impala/commits/IMPALA-8851 This patch's goal is to change Impala's behavior in the following case: - the query is a DROP TABLE/VIEW/DATABASE/FUNCTIONS IF EXISTS statement - the given object does not exist - the user has some kind of privilege on the object, which imples the privilege to know whether object exists, but does not have DROP privilege on the object Until now this lead to an authorization exception, while it will be allowed with this change. An example where this is useful is a user who has CREATE privilege on a database, and creates table t_owned, and gets ownership of the table. In this case DROP TABLE IF EXISTS was non idempotent: DROP TABLE IF EXISTS t_owned; -> success DROP TABLE IF EXISTS t_owned; -> authorization error, as the privileges for the table were deleted when the table was successfully dropped After this change the second statement will be also successful. The authorization logic has to avoid leaking information that the user has no right to know. For this reason DROP IF EXISTS has to return the same error message regardless whether the object exists or not if the user has no right to know it's existence. This is achieved with the following pattern: - in the IF EXISTS case first an ANY privilege is registered, then the existence of the object is checked and if it doesn't exist, the analysis returns successfully - if the object exists, the DROP privilege is registered (if there is no IF EXISTS in the query, this always happens) - as the authorization logic checks privileges in the order of registration, first the ANY will be checked, and DROP will be only checked if the user has ANY privileges Testing: - Added a new test case in the sentry tests which confirms that the authorization exception is not thrown when a drop if exists query is issued on a object which does not exist. - Changed several tests affected by the new behavior. - Ran core tests. Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java M fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java M testdata/workloads/functional-query/queries/QueryTest/create-database.test M tests/authorization/test_owner_privileges.py M tests/common/sentry_cache_test_suite.py 10 files changed, 228 insertions(+), 35 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/14121/8 -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 8 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 7: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/4396/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 7 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 27 Aug 2019 22:37:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/14121/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/14121/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2659 PS7, Line 2659: getFqTableName I would have preferred a precondition check here instead of assuming what the caller wants since based on the result of this call, authorization decisions are being made. http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java File fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java@117 PS6, Line 117: tableName_ > I created a fix inside tableExists(). Sorry for bugging you on this. Adding the check in the tableExists won't be right if dbName_ here is not default. For instance if dbName_ = foo and tblName_.tblName = testTbl then this call will look up default.testTbl Also, does the case matter. I think it is important to make sure that this call is doing the right thing otherwise a non-privileged user can drop the table. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 7 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 27 Aug 2019 22:33:22 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/14121/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14121/6//COMMIT_MSG@15 PS6, Line 15: privilige > nit, spell-check Done http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2659 PS6, Line 2659: tblName > I was trying out your patch locally and I found a bug in this code which ca Thanks for spotting this! I used getFqTableName() to fix this, similarly to what getTable does. http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java File fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java@117 PS6, Line 117: tableName_ > I noticed that the tableName_ here is not fully qualified for some reason. I created a fix inside tableExists(). The reason for using unqualified name is probably to be able to reconstruct the original query in toSql(), but this seems quite dangerous to me. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 6 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 27 Aug 2019 22:13:15 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Hello Vihang Karajgaonkar, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14121 to look at the new patch set (#7). Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. IMPALA-8851: Do not throw authorization exception in drop if exists queries Note that this is the continuation of work in https://github.com/vihangk1/impala/commits/IMPALA-8851 This patch's goal is to change Impala's behavior in the following case: - the query is a DROP TABLE/VIEW/DATABASE/FUNCTIONS IF EXISTS statement - the given object does not exist - the user has some kind of privilege on the object, which imples the privilege to know whether object exists, but does not have DROP privilege on the object Until now this lead to an authorization exception, while it will be allowed with this change. An example where this is useful is a user who has CREATE privilege on a database, and creates table t_owned, and gets ownership of the table. In this case DROP TABLE IF EXISTS was non idempotent: DROP TABLE IF EXISTS t_owned; -> success DROP TABLE IF EXISTS t_owned; -> authorization error, as the privileges for the table were deleted when the table was successfully dropped After this change the second statement will be also successful. The authorization logic has to avoid leaking information that the user has no right to know. For this reason DROP IF EXISTS has to return the same error message regardless whether the object exists or not if the user has no right to know it's existence. This is achieved with the following pattern: - in the IF EXISTS case first an ANY privilege is registered, then the existence of the object is checked and if it doesn't exist, the analysis returns successfully - if the object exists, the DROP privilege is registered (if there is no IF EXISTS in the query, this always happens) - as the authorization logic checks privileges in the order of registration, first the ANY will be checked, and DROP will be only checked if the user has ANY privileges Testing: - Added a new test case in the sentry tests which confirms that the authorization exception is not thrown when a drop if exists query is issued on a object which does not exist. - Changed several tests affected by the new behavior. - Ran core tests. Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java M fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java M testdata/workloads/functional-query/queries/QueryTest/create-database.test M tests/authorization/test_owner_privileges.py M tests/common/sentry_cache_test_suite.py 10 files changed, 228 insertions(+), 35 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/14121/7 -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 7 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2659 PS6, Line 2659: tblName I was trying out your patch locally and I found a bug in this code which can lead to a privilege escalation problem. If the tblName is not fully qualified in this method this will always return false. Can you add a Precondition check to make sure that tblname is fully qualified here? Not sure why it was not caught in the tests. http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java File fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java@115 PS6, Line 115: dbName_, getTbl() > I think this logic is faulty and will fail in the following scenario. Synced up with Csaba offline. If the user2 has privileges on t2 within the db the show tables command skips listing of t1. Hence a ANY privilege on db is not a suitable representative to know whether user has permissions to see the existance of t1. In such a case, the current approach makes sense to me. Thanks for clarification. http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java@117 PS6, Line 117: tableName_ I noticed that the tableName_ here is not fully qualified for some reason. We should pass TableName(dbName, getTbl()) here. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 6 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 27 Aug 2019 21:10:43 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 5: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4856/ -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 5 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 27 Aug 2019 19:27:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/14121/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14121/6//COMMIT_MSG@15 PS6, Line 15: privilige nit, spell-check http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java File fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/6/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java@115 PS6, Line 115: dbName_, getTbl() I think this logic is faulty and will fail in the following scenario. Lets say user1 has create privileges on database functional and they create tables t1 and t2 Lets assume user2 has select privileges on tables functional.t1 and functional.t2 Now lets say user1 drops functional.t1 which should succeed. user1 should reissue drop if exists functional.t1 should not error out either. However, for user2 a drop if exists functional.t1 will fail with a authorization exception since they don't have the privilege on t1 anymore. However, user2 still has a privilege to know the existance of such a table using show tables in functional; which works and only lists t2. Same applies to db and functions as well. I think this privilege should be registered at the parent level of the object. So, for drop table we should register ANY privilege on db and so on. Let me know if this is not a correct understanding of the problem. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 6 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 27 Aug 2019 19:10:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4391/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 6 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 27 Aug 2019 17:19:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/14121/5/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java File fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/5/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java@123 PS5, Line 123: // DROP VIEW IF EXISTS 'table' succeeds, similarly to Hive, but unlike postgres. > Ok, that makes a lot of sense. Agree we should preserve the current behavio I added comments + also checked what Hive does. It simply succeeds, while Impala at least tells in the result that there was an issue. There is a test for drop table on view: https://github.com/apache/impala/blob/master/testdata/workloads/functional-query/queries/QueryTest/views-ddl.test#L196 -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 6 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 27 Aug 2019 16:40:58 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Hello Vihang Karajgaonkar, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14121 to look at the new patch set (#6). Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. IMPALA-8851: Do not throw authorization exception in drop if exists queries Note that this is the continuation of work in https://github.com/vihangk1/impala/commits/IMPALA-8851 This patch's goal is to change Impala's behavior in the following case: - the query is a DROP TABLE/VIEW/DATABASE/FUNCTIONS IF EXISTS statement - the given object does not exist - the user has some kind of privilige on the object, which imples the privilege to know whether object exists, but does not have DROP privilege on the object Until now this lead to an authorization exception, while it will be allowed with this change. An example where this is useful is a user who has CREATE privilege on a database, and creates table t_owned, and gets ownership of the table. In this case DROP TABLE IF EXISTS was non idempotent: DROP TABLE IF EXISTS t_owned; -> success DROP TABLE IF EXISTS t_owned; -> authorization error, as the privileges for the table were deleted when the table was successfully dropped After this change the second statement will be also successful. The authorization logic has to avoid leaking information that the user has no right to know. For this reason DROP IF EXISTS has to return the same error message regardless whether the object exists or not if the user has no right to know it's existence. This is achieved with the following pattern: - in the IF EXISTS case first an ANY privilege is registered, then the existence of the object is checked and if it doesn't exist, the analysis returns successfully - if the object exists, the DROP privilege is registered (if there is no IF EXISTS in the query, this always happens) - as the authorization logic checks privileges in the order of registration, first the ANY will be checked, and DROP will be only checked if the user has ANY privileges Testing: - Added a new test case in the sentry tests which confirms that the authorization exception is not thrown when a drop if exists query is issued on a object which does not exist. - Changed several tests affected by the new behavior. - Ran core tests. Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java M fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java M testdata/workloads/functional-query/queries/QueryTest/create-database.test M tests/authorization/test_owner_privileges.py M tests/common/sentry_cache_test_suite.py 10 files changed, 225 insertions(+), 35 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/14121/6 -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 6 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/14121/5/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java File fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/5/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java@123 PS5, Line 123: if (ifExists_) return; > I did it this way to avoid changing existing behavior. There was a test I b Ok, that makes a lot of sense. Agree we should preserve the current behaviour then. Would be good to note in the code that this is intended to preserve existing behaviour. Do we have tests that exercise the other case - i.e. drop table on a view? -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 5 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 27 Aug 2019 16:02:23 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/14121/5/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java File fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/5/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java@123 PS5, Line 123: if (ifExists_) return; > This is counterintuitive - I would expect this to fail if the object exists I did it this way to avoid changing existing behavior. There was a test I broken at my first attempt that threw exception: https://github.com/apache/impala/blob/master/testdata/workloads/functional-query/queries/QueryTest/views-ddl.test#L188 Note the catch clause that was removed at the end of this function - it used to catch AnalysisException and ignore it in the IF EXISTS case. I prefer the new version as it cannot hide exceptions by accident. My approach in this change was that error messages can change, but I shouldn't disallow something that was allowed until now, as that could break existing workflows. ( I can even imagine the current logic to be used to DROP something without knowing whether it is a view or a table - doing both DROP VIEW IF EXISTS and DROP TABLE IF EXISTS would do the job. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 5 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 27 Aug 2019 15:55:53 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/14121/5/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java File fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/5/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java@123 PS5, Line 123: if (ifExists_) return; This is counterintuitive - I would expect this to fail if the object exists and it doesn't drop it. Not sure that this is the right behaviour. If it is the intended behaviour it needs some explanation. Postgres raises errors in this case: postgres=# create table foo(i int); CREATE TABLE postgres=# drop view if exists foo; ERROR: "foo" is not a view HINT: Use DROP TABLE to remove a table. postgres=# create view bar as select * from foo; CREATE VIEW postgres=# drop table if exists bar; ERROR: "bar" is not a table HINT: Use DROP VIEW to remove a view. postgres=# drop table if exists baz; NOTICE: table "baz" does not exist, skipping DROP TABLE -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 5 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 27 Aug 2019 15:39:56 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 5: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4856/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 5 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 27 Aug 2019 15:21:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/4385/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 4 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 27 Aug 2019 14:37:03 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14121 ) Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2654 PS3, Line 2654: /** > Missing java doc for the two new methods Done http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java File fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java@77 PS3, Line 77: ()); > What happens when we register a privilege for a db which doesn't exist? The It is possible that the user does not have the permission to list databases on the server, but has the permission to use a specific database. http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java File fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java@86 PS3, Line 86: // Start with > Is this registering the priv twice if the function exists and if exists cla The registerFnPriv at line 116 is only called if ifExists_ is true to "raise" the requested privilege from ANY do DROP. Added some comments about this. http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java@94 PS3, Line 94: throw new AnalysisException(Analyzer.DB_DOES_NOT_EXIST_ERROR_MSG + desc_.dbName()); > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java@97 PS3, Line 97: (ifExists_) return; : throw new AnalysisException( > Can we make this section cleaner by creating a method which checks for exis I was thinking about something similar, but it seemed tricky to preserve the correct error messages: - If the user has no right to know the functions's existence, then "function does not exist" messages have to be hidden by registering the privilege first (the calling logic will give precedence to authorization errors). - Meanwhile if there is no authorization problem, but the function or db does not exist, then a correct informative error should be returned. I wanted to avoid changing these things too much to avoid breaking even more tests. http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java@119 PS3, Line 119: > this method can be private. Done http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java File fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java: http://gerrit.cloudera.org:8080/#/c/14121/3/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java@116 PS3, Line 116: build()); > What is the behavior when the table doesn't exist and a privilege request i As far as I understand Sentry it simply means that you don't have privileges for the given object (but you can still have privileges for the hierarchies above it like DB and server). Sentry doesn't care about the actual objects, only about privileges. http://gerrit.cloudera.org:8080/#/c/14121/3/tests/authorization/test_owner_privileges.py File tests/authorization/test_owner_privileges.py: http://gerrit.cloudera.org:8080/#/c/14121/3/tests/authorization/test_owner_privileges.py@149 PS3, Line 149: unique_ > is this argument needed? Done http://gerrit.cloudera.org:8080/#/c/14121/3/tests/authorization/test_owner_privileges.py@182 PS3, Line 182: _execute_drop_if_exists > Do you think we need a additional test which exercises the drop if exists f I see several holes in the testing of privileges but I would prefer to move this to a different Jira. -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 5 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 27 Aug 2019 14:26:55 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8851: Do not throw authorization exception in drop if exists queries
Hello Vihang Karajgaonkar, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14121 to look at the new patch set (#5). Change subject: IMPALA-8851: Do not throw authorization exception in drop if exists queries .. IMPALA-8851: Do not throw authorization exception in drop if exists queries Note that this is the continuation of work in https://github.com/vihangk1/impala/commits/IMPALA-8851 This patch's goal is to change Impala's behavior in the following case: - the query is a DROP TABLE/VIEW/DATABASE/FUNCTIONS IF EXISTS statement - the given object does not exist - the user has some kind of privilige on the object, which imples the privilege to know whether object exists, but does not have DROP privilege on the object Until now this lead to an authorization exception, while it will be allowed with this change. An example where this is useful is a user who has CREATE privilege on a database, and creates table t_owned, and gets ownership of the table. In this case DROP TABLE IF EXISTS was non idempotent: DROP TABLE IF EXISTS t_owned; -> success DROP TABLE IF EXISTS t_owned; -> authorization error, as the privileges for the table were deleted when the table was successfully dropped After this change the second statement will be also successful. The authorization logic has to avoid leaking information that the user has no right to know. For this reason DROP IF EXISTS has to return the same error message regardless whether the object exists or not if the user has no right to know it's existence. This is achieved with the following pattern: - in the IF EXISTS case first an ANY privilege is registered, then the existence of the object is checked and if it doesn't exist, the analysis returns successfully - if the object exists, the DROP privilege is registered (if there is no IF EXISTS in the query, this always happens) - as the authorization logic checks privileges in the order of registration, first the ANY will be checked, and DROP will be only checked if the user has ANY privileges Testing: - Added a new test case in the sentry tests which confirms that the authorization exception is not thrown when a drop if exists query is issued on a object which does not exist. - Changed several tests affected by the new behavior. - Ran core tests. Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java M fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java M testdata/workloads/functional-query/queries/QueryTest/create-database.test M tests/authorization/test_owner_privileges.py M tests/common/sentry_cache_test_suite.py 10 files changed, 223 insertions(+), 35 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/14121/5 -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 5 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8851 : Do not throw authorization exception in drop if exists queries
Hello Vihang Karajgaonkar, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/14121 to look at the new patch set (#4). Change subject: IMPALA-8851 : Do not throw authorization exception in drop if exists queries .. IMPALA-8851 : Do not throw authorization exception in drop if exists queries Note that this is the continuation of work in https://github.com/vihangk1/impala/commits/IMPALA-8851 This patch's goal is to change Impala's behavior in the following case: - the query is a DROP TABLE/VIEW/DATABASE/FUNCTIONS IF EXISTS statement - the given object does not exist - the user has some kind of privilige on the object, which imples the privilege to know whether object exists, but does not have DROP privilege on the object Until now this lead to an authorization exception, while it will be allowed with this change. An example where this is useful is a user who has CREATE privilege on a database, and creates table t_owned, and gets ownership of the table. In this case DROP TABLE IF EXISTS was non idempotent: DROP TABLE IF EXISTS t_owned; -> success DROP TABLE IF EXISTS t_owned; -> authorization error, as the privileges for the table were deleted when the table was successfully dropped After this change the second statement will be also successful. The authorization logic has to avoid leaking information that the user has no right to know. For this reason DROP IF EXISTS has to return the same error message regardless whether the object exists or not if the user has no right to know it's existence. This is achieved with the following pattern: - in the IF EXISTS case first an ANY privilege is registered, then the existence of the object is checked and if it doesn't exist, the analysis returns successfully - if the object exists, the DROP privilege is registered (if there is no IF EXISTS in the query, this always happens) - as the authorization logic checks privileges in the order of registration, first the ANY will be checked, and DROP will be only checked if the user has ANY privileges Testing: - Added a new test case in the sentry tests which confirms that the authorization exception is not thrown when a drop if exists query is issued on a object which does not exist. - Changed several tests affected by the new behavior. - Ran core tests. Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/DropDbStmt.java M fe/src/main/java/org/apache/impala/analysis/DropFunctionStmt.java M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java M fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java M testdata/workloads/functional-query/queries/QueryTest/create-database.test M tests/authorization/test_owner_privileges.py M tests/common/sentry_cache_test_suite.py 10 files changed, 223 insertions(+), 35 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/14121/4 -- To view, visit http://gerrit.cloudera.org:8080/14121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iba068935e5da92d71e16e2321afdb8e7b781086a Gerrit-Change-Number: 14121 Gerrit-PatchSet: 4 Gerrit-Owner: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar
