You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Adam Holley (Code Review)" <ge...@cloudera.org> on 2018/07/11 05:03:11 UTC
[Impala-ASF-CR] IMPALA-7275: Create tbl auth error should not show table
Adam Holley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10919
Change subject: IMPALA-7275: Create tbl auth error should not show table
......................................................................
IMPALA-7275: Create tbl auth error should not show table
The authorization correctly checks database level authorization
but the exception includes the table name. This fix adds
a check for create and modifies the error string to remove
the table name.
Testsing: Ran all FE tests
Change-Id: I6711a744541abd5ff26574974ba8517b6e51c453
---
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
2 files changed, 63 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/10919/3
--
To view, visit http://gerrit.cloudera.org:8080/10919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6711a744541abd5ff26574974ba8517b6e51c453
Gerrit-Change-Number: 10919
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
[Impala-ASF-CR] IMPALA-7275: Create table authorization error should not show table name
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10919 )
Change subject: IMPALA-7275: Create table authorization error should not show table name
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6711a744541abd5ff26574974ba8517b6e51c453
Gerrit-Change-Number: 10919
Gerrit-PatchSet: 5
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Jul 2018 19:21:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7275: Create table authorization error should not show table name
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/10919 )
Change subject: IMPALA-7275: Create table authorization error should not show table name
......................................................................
IMPALA-7275: Create table authorization error should not show table name
The authorization correctly checks database level authorization but
the exception includes the table name. This fix adds a check for
create and modifies the error string to remove the table name. Having
an error with CREATE on a table is not correct since to create a table
a user needs CREATE on the parent database.
Example:
SQL: CREATE TABLE db.tbl(col1 INT)
Before:
User 'user1' does not have privileges to execute 'CREATE' on: db.tbl
After:
User 'user1' does not have privileges to execute 'CREATE' on: db
Testsing: Ran all FE tests
Change-Id: I6711a744541abd5ff26574974ba8517b6e51c453
---
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
2 files changed, 50 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/10919/5
--
To view, visit http://gerrit.cloudera.org:8080/10919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6711a744541abd5ff26574974ba8517b6e51c453
Gerrit-Change-Number: 10919
Gerrit-PatchSet: 5
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-7275: Create table authorization error should not show table name
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/10919 )
Change subject: IMPALA-7275: Create table authorization error should not show table name
......................................................................
IMPALA-7275: Create table authorization error should not show table name
The authorization correctly checks database level authorization but
the exception includes the table name. This fix adds a check for
create and modifies the error string to remove the table name. Having
an error with CREATE on a table is not correct since to create a table
a user needs CREATE on the parent database.
Example:
SQL: CREATE TABLE db.tbl(col1 INT)
Before:
User 'user1' does not have privileges to execute 'CREATE' on: db.tbl
After:
User 'user1' does not have privileges to execute 'CREATE' on: db
Testsing: Ran all FE tests
Change-Id: I6711a744541abd5ff26574974ba8517b6e51c453
---
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
2 files changed, 51 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/19/10919/4
--
To view, visit http://gerrit.cloudera.org:8080/10919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6711a744541abd5ff26574974ba8517b6e51c453
Gerrit-Change-Number: 10919
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-7275: Create table authorization error should not show table name
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10919 )
Change subject: IMPALA-7275: Create table authorization error should not show table name
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2795/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/10919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6711a744541abd5ff26574974ba8517b6e51c453
Gerrit-Change-Number: 10919
Gerrit-PatchSet: 5
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Jul 2018 19:22:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7275: Create table authorization error should not show table name
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10919 )
Change subject: IMPALA-7275: Create table authorization error should not show table name
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10919/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10919/3//COMMIT_MSG@7
PS3, Line 7: tbl auth
> nit: table authorization (we shouldn't be abbreviating it)
Done
http://gerrit.cloudera.org:8080/#/c/10919/3//COMMIT_MSG@7
PS3, Line 7: table
> nit: table name
Done
http://gerrit.cloudera.org:8080/#/c/10919/3//COMMIT_MSG@11
PS3, Line 11: remove
: the table name.
> Give an explanation why having a table name in the exception for CREATE TAB
Done
http://gerrit.cloudera.org:8080/#/c/10919/3/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java:
http://gerrit.cloudera.org:8080/#/c/10919/3/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@122
PS3, Line 122: List<DBModelAuthorizable> authorizeables = privilegeRequest.getAuthorizeable().
: getHiveAuthorizeableHierarchy();
: if (privilege == Privilege.CREATE && authorizeables.size() > 1 &&
: !(privilegeRequest.getAuthorizeable() instanceof AuthorizeableFn)) {
: // CREATE on an object requires CREATE on the parent and we shouldn't
: // expose the table name. Since we don't get back the modified
: // authorizable list, we need to check again and modify the error.
: authorizeables.remove(authorizeables.size() - 1);
: StringBuilder sb = new StringBuilder();
: Iterator<DBModelAuthorizable> authIt = authorizeables.iterator();
: while (authIt.hasNext()) {
: DBModelAuthorizable authorizeable = authIt.next();
: sb.append(authorizeable.getName());
: if (authIt.hasNext()) {
: sb.append(".");
: }
: }
: throw new AuthorizationException(String.format(
: "User '%s' does not have privileges to execute '%s' on: %s",
: user.getName(), privilege, sb.toString()));
:
> The logic here is way too complicated. You're mutating the list and also bu
Done
--
To view, visit http://gerrit.cloudera.org:8080/10919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6711a744541abd5ff26574974ba8517b6e51c453
Gerrit-Change-Number: 10919
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Jul 2018 16:49:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7275: Create tbl auth error should not show table
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10919 )
Change subject: IMPALA-7275: Create tbl auth error should not show table
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10919/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10919/3//COMMIT_MSG@7
PS3, Line 7: table
nit: table name
http://gerrit.cloudera.org:8080/#/c/10919/3//COMMIT_MSG@7
PS3, Line 7: tbl auth
nit: table authorization (we shouldn't be abbreviating it)
http://gerrit.cloudera.org:8080/#/c/10919/3//COMMIT_MSG@11
PS3, Line 11: remove
: the table name.
Give an explanation why having a table name in the exception for CREATE TABLE does not make any sense.
Also, provide an example like
SQL: CREATE TABLE db.tbl(i INT)
Before: User 'foo' does not have privileges to execute 'CREATE' on: db.tbl
After: User 'foo' does not have privileges to execute 'CREATE' on: db
http://gerrit.cloudera.org:8080/#/c/10919/3/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java:
http://gerrit.cloudera.org:8080/#/c/10919/3/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@122
PS3, Line 122: List<DBModelAuthorizable> authorizeables = privilegeRequest.getAuthorizeable().
: getHiveAuthorizeableHierarchy();
: if (privilege == Privilege.CREATE && authorizeables.size() > 1 &&
: !(privilegeRequest.getAuthorizeable() instanceof AuthorizeableFn)) {
: // CREATE on an object requires CREATE on the parent and we shouldn't
: // expose the table name. Since we don't get back the modified
: // authorizable list, we need to check again and modify the error.
: authorizeables.remove(authorizeables.size() - 1);
: StringBuilder sb = new StringBuilder();
: Iterator<DBModelAuthorizable> authIt = authorizeables.iterator();
: while (authIt.hasNext()) {
: DBModelAuthorizable authorizeable = authIt.next();
: sb.append(authorizeable.getName());
: if (authIt.hasNext()) {
: sb.append(".");
: }
: }
: throw new AuthorizationException(String.format(
: "User '%s' does not have privileges to execute '%s' on: %s",
: user.getName(), privilege, sb.toString()));
:
The logic here is way too complicated. You're mutating the list and also building the fully-qualified name manually. The whole logic can be simplified to this.
} else if (privilegeRequest.getPrivilege() == Privilege.CREATE &&
privilegeRequest.getAuthorizeable() instanceof AuthorizeableTable) {
AuthorizeableTable authorizeableTable =
(AuthorizeableTable) privilegeRequest.getAuthorizeable();
throw new AuthorizationException(String.format(
"User '%s' does not have privileges to execute '%s' on: %s",
user.getName(), privilege, authorizeableTable.getDbName()));
}
--
To view, visit http://gerrit.cloudera.org:8080/10919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6711a744541abd5ff26574974ba8517b6e51c453
Gerrit-Change-Number: 10919
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Jul 2018 06:19:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7275: Create table authorization error should not show table name
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10919 )
Change subject: IMPALA-7275: Create table authorization error should not show table name
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6711a744541abd5ff26574974ba8517b6e51c453
Gerrit-Change-Number: 10919
Gerrit-PatchSet: 5
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Jul 2018 22:40:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7275: Create table authorization error should not show table name
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10919 )
Change subject: IMPALA-7275: Create table authorization error should not show table name
......................................................................
IMPALA-7275: Create table authorization error should not show table name
The authorization correctly checks database level authorization but
the exception includes the table name. This fix adds a check for
create and modifies the error string to remove the table name. Having
an error with CREATE on a table is not correct since to create a table
a user needs CREATE on the parent database.
Example:
SQL: CREATE TABLE db.tbl(col1 INT)
Before:
User 'user1' does not have privileges to execute 'CREATE' on: db.tbl
After:
User 'user1' does not have privileges to execute 'CREATE' on: db
Testsing: Ran all FE tests
Change-Id: I6711a744541abd5ff26574974ba8517b6e51c453
Reviewed-on: http://gerrit.cloudera.org:8080/10919
Reviewed-by: Fredy Wijaya <fw...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
2 files changed, 50 insertions(+), 40 deletions(-)
Approvals:
Fredy Wijaya: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/10919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6711a744541abd5ff26574974ba8517b6e51c453
Gerrit-Change-Number: 10919
Gerrit-PatchSet: 6
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-7275: Create table authorization error should not show table name
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10919 )
Change subject: IMPALA-7275: Create table authorization error should not show table name
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10919/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java:
http://gerrit.cloudera.org:8080/#/c/10919/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@124
PS4, Line 124: Since we don't get back the modified
: // authorizable list, we need to check again and modify the error
> nit: I think this comment is no longer relevant.
Done
--
To view, visit http://gerrit.cloudera.org:8080/10919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6711a744541abd5ff26574974ba8517b6e51c453
Gerrit-Change-Number: 10919
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Jul 2018 19:20:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7275: Create table authorization error should not show table name
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10919 )
Change subject: IMPALA-7275: Create table authorization error should not show table name
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10919/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java:
http://gerrit.cloudera.org:8080/#/c/10919/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@124
PS4, Line 124: Since we don't get back the modified
: // authorizable list, we need to check again and modify the error
nit: I think this comment is no longer relevant.
--
To view, visit http://gerrit.cloudera.org:8080/10919
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6711a744541abd5ff26574974ba8517b6e51c453
Gerrit-Change-Number: 10919
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Jul 2018 17:07:11 +0000
Gerrit-HasComments: Yes