You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Quanlong Huang (Code Review)" <ge...@cloudera.org> on 2019/12/13 13:04:10 UTC
[Impala-ASF-CR] IMPALA-9231: support simplified privilege checks for SHOW DATABASES
Quanlong Huang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14904
Change subject: IMPALA-9231: support simplified privilege checks for SHOW DATABASES
......................................................................
IMPALA-9231: support simplified privilege checks for SHOW DATABASES
In IMPALA-9002 we introduce a flag simplify_check_on_show_tables which
simplifies privilege checks for SHOW TABLES. Only tables with privileges
implying SELECT privilege will be shown.
This patch provides the same mechanism for SHOW DATABASES. Also rename
the flag to simplify_check_on_show_dbs_and_tables to enable/disable them
together.
Tests:
- Add tests in test_authorization.py
Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
---
M be/src/common/global-flags.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/authorization/test_authorization.py
6 files changed, 63 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/14904/1
--
To view, visit http://gerrit.cloudera.org:8080/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
[Impala-ASF-CR] IMPALA-9231: support simplified privilege checks for SHOW DATABASES
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14904 )
Change subject: IMPALA-9231: support simplified privilege checks for SHOW DATABASES
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14904/1/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/14904/1/be/src/common/global-flags.cc@282
PS1, Line 282: false
I think its rare that users who have insert (or other privileges) do not have select privileges. So a boolean value would work for most of the users. However, in the rare case that an environment has some users who do not have select privileges, the show command will not list the dbs and tables. Can we change the type of this configuration to a stringset (set of allowed string values)? I propose we should allow SELECT, INSERT, ANY for now so that we cover all the usecases. Any thoughts?
--
To view, visit http://gerrit.cloudera.org:8080/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Dec 2019 18:05:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9231: support customized privilege checks for SHOW visibility
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14904 )
Change subject: IMPALA-9231: support customized privilege checks for SHOW visibility
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5361/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Dec 2019 18:04:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9231: support simplified privilege checks for SHOW DATABASES
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Vihang Karajgaonkar, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14904
to look at the new patch set (#2).
Change subject: IMPALA-9231: support simplified privilege checks for SHOW DATABASES
......................................................................
IMPALA-9231: support simplified privilege checks for SHOW DATABASES
In IMPALA-9002 we introduce a flag simplify_check_on_show_tables which
simplifies privilege checks for SHOW TABLES. Only tables with privileges
implying SELECT privilege will be shown.
This patch provides the same mechanism for SHOW DATABASES. Also augment
the flag to be a list of privilege names and rename it to
visibility_privilege_set. The default value is "any" which remains the
default behavior. If set to "select", only dbs/tables on which the user
has SELECT privilege will be shown. If set to "select,insert", only
dbs/tables on which the user has SELECT or INSERT privilege will be
shown.
Tests:
- Add tests in test_authorization.py
Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
---
M be/src/common/global-flags.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/authorization/test_authorization.py
9 files changed, 241 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/14904/2
--
To view, visit http://gerrit.cloudera.org:8080/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9231: support customized privilege checks for visibility
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Vihang Karajgaonkar, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14904
to look at the new patch set (#3).
Change subject: IMPALA-9231: support customized privilege checks for visibility
......................................................................
IMPALA-9231: support customized privilege checks for visibility
In IMPALA-9002 we introduce a flag simplify_check_on_show_tables which
simplifies privilege checks for SHOW TABLES. Only tables with privileges
implying SELECT privilege will be shown.
This patch provides the same mechanism for SHOW DATABASES. Also augment
the flag to be a list of privilege names and rename it to
visibility_privilege_set. The default value is "any" which remains the
default behavior. If set to "select", only dbs/tables on which the user
has SELECT privilege will be shown. If set to "select,insert", only
dbs/tables on which the user has SELECT or INSERT privilege will be
shown.
Tests:
- Add tests in test_authorization.py
Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
---
M be/src/common/global-flags.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/authorization/test_authorization.py
9 files changed, 241 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/14904/3
--
To view, visit http://gerrit.cloudera.org:8080/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9231: support simplified privilege checks for SHOW DATABASES
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14904 )
Change subject: IMPALA-9231: support simplified privilege checks for SHOW DATABASES
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/14904/1/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/14904/1/be/src/common/global-flags.cc@282
PS1, Line 282:
> I think its rare that users who have insert (or other privileges) do not ha
Good point. I change it to a set to accept customized privilege set to check.
http://gerrit.cloudera.org:8080/#/c/14904/1/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/14904/1/fe/src/main/java/org/apache/impala/service/Frontend.java@943
PS1, Line 943: // have permissions on.
: if (authzFactory_.getAuthorizationConfig().isEnabled()) {
: Iterator<? extends FeDb> iter = dbs.iterator();
:
> There could be a function for this like RequiredPrivilegeForShowDbsAndTable
Done
http://gerrit.cloudera.org:8080/#/c/14904/1/tests/authorization/test_authorization.py
File tests/authorization/test_authorization.py:
http://gerrit.cloudera.org:8080/#/c/14904/1/tests/authorization/test_authorization.py@547
PS1, Line 547: self.client.execute("drop database if exists %s cascade" % unique_db)
> nit: +2 indentation
Done
http://gerrit.cloudera.org:8080/#/c/14904/1/tests/authorization/test_authorization.py@557
PS1, Line 557: "--visibility_privilege_set=select" %
> nit: +2 indentation
Done
http://gerrit.cloudera.org:8080/#/c/14904/1/tests/authorization/test_authorization.py@597
PS1, Line 597: catalogd_args="--sentry_config={0}".format(SENTRY_CONFIG_FILE),
> nit: +2 indentation
Done
--
To view, visit http://gerrit.cloudera.org:8080/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Dec 2019 03:33:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9231: support simplified privilege checks for SHOW DATABASES
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14904 )
Change subject: IMPALA-9231: support simplified privilege checks for SHOW DATABASES
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5277/ : 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/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Dec 2019 13:34:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9231: support customized privilege checks for visibility
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14904 )
Change subject: IMPALA-9231: support customized privilege checks for visibility
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5302/ : 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/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Dec 2019 04:02:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9231: support customized privilege checks for SHOW visibility
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14904 )
Change subject: IMPALA-9231: support customized privilege checks for SHOW visibility
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Dec 2019 22:35:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9231: support customized privilege checks for visibility
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14904 )
Change subject: IMPALA-9231: support customized privilege checks for visibility
......................................................................
Patch Set 3:
(8 comments)
Thanks for making the suggested change. I have some minor suggestions below and its good to go from my side.
http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@282
PS3, Line 282: visibility_privilege_set
may be rename to min_privilege_set_for_show_stmts?
http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@283
PS3, Line 283: Any o
Any one of them..
http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@284
PS3, Line 284: "table. Default to \"any\" which means if the user has any privilege (ALL, SELECT, "
s/Default/Defaults
http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@288
PS3, Line 288: practise
s/practise/practice
http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@291
PS3, Line 291:
nit, additional space
http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@292
PS3, Line 292: Because a lot of privilege checks on invisible dbs/tables can "
: "be bypassed
I think this is redundant information and can be skipped.
http://gerrit.cloudera.org:8080/#/c/14904/3/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java
File fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java:
http://gerrit.cloudera.org:8080/#/c/14904/3/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java@93
PS3, Line 93: hasAccess
nit, not a blocker for this patch. I think sentry already does this for us.
http://gerrit.cloudera.org:8080/#/c/14904/3/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/14904/3/fe/src/main/java/org/apache/impala/service/Frontend.java@326
PS3, Line 326: LOG.error("Ignored illegal privilege name '{}'", pStr, e);
Do you think we should throw ImpalaException here instead of silently logging ignoring the user configuration?
--
To view, visit http://gerrit.cloudera.org:8080/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Dec 2019 22:55:45 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9231: support customized privilege checks for SHOW visibility
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14904 )
Change subject: IMPALA-9231: support customized privilege checks for SHOW visibility
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5318/ : 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/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Dec 2019 02:36:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9231: support simplified privilege checks for SHOW DATABASES
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14904 )
Change subject: IMPALA-9231: support simplified privilege checks for SHOW DATABASES
......................................................................
Patch Set 1: Code-Review+1
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14904/1/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/14904/1/fe/src/main/java/org/apache/impala/service/Frontend.java@943
PS1, Line 943: Privilege requiredPrivilege = Privilege.ANY;
: if (BackendConfig.INSTANCE.simplifyCheckOnShowDbsAndTables()) {
: requiredPrivilege = Privilege.SELECT;
: }
There could be a function for this like RequiredPrivilegeForShowDbsAndTables();
http://gerrit.cloudera.org:8080/#/c/14904/1/tests/authorization/test_authorization.py
File tests/authorization/test_authorization.py:
http://gerrit.cloudera.org:8080/#/c/14904/1/tests/authorization/test_authorization.py@547
PS1, Line 547: root_impalad_client, "show databases", user=another_user)
nit: +2 indentation
http://gerrit.cloudera.org:8080/#/c/14904/1/tests/authorization/test_authorization.py@557
PS1, Line 557: "drop database if exists db_%s_%s cascade" % (unique_name, priv))
nit: +2 indentation
http://gerrit.cloudera.org:8080/#/c/14904/1/tests/authorization/test_authorization.py@597
PS1, Line 597: "drop database if exists db_%s_%s cascade" % (unique_name, priv))
nit: +2 indentation
--
To view, visit http://gerrit.cloudera.org:8080/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Dec 2019 17:01:13 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9231: support customized privilege checks for SHOW visibility
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Vihang Karajgaonkar, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14904
to look at the new patch set (#4).
Change subject: IMPALA-9231: support customized privilege checks for SHOW visibility
......................................................................
IMPALA-9231: support customized privilege checks for SHOW visibility
In IMPALA-9002 we introduce a flag simplify_check_on_show_tables which
simplifies privilege checks for SHOW TABLES. Only tables with privileges
implying SELECT privilege will be shown.
This patch provides the same mechanism for SHOW DATABASES. Also augment
the flag to be a list of privilege names and rename it to
min_privilege_set_for_show_stmts. The default value is "any" which
remains the default behavior. If set to "select", only dbs/tables on
which the user has SELECT privilege will be shown. If set to
"select,insert", only dbs/tables on which the user has SELECT or INSERT
privilege will be shown.
Tests:
- Add tests in test_authorization.py
Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
---
M be/src/common/global-flags.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/authorization/test_authorization.py
9 files changed, 217 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/14904/4
--
To view, visit http://gerrit.cloudera.org:8080/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9231: support customized privilege checks for SHOW visibility
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14904 )
Change subject: IMPALA-9231: support customized privilege checks for SHOW visibility
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Dec 2019 18:04:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9231: support simplified privilege checks for SHOW DATABASES
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14904 )
Change subject: IMPALA-9231: support simplified privilege checks for SHOW DATABASES
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14904/2/tests/authorization/test_authorization.py
File tests/authorization/test_authorization.py:
http://gerrit.cloudera.org:8080/#/c/14904/2/tests/authorization/test_authorization.py@574
PS2, Line 574: .
flake8: E131 continuation line unaligned for hanging indent
http://gerrit.cloudera.org:8080/#/c/14904/2/tests/authorization/test_authorization.py@600
PS2, Line 600: .
flake8: E131 continuation line unaligned for hanging indent
--
To view, visit http://gerrit.cloudera.org:8080/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Dec 2019 03:33:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9231: support customized privilege checks for SHOW visibility
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14904 )
Change subject: IMPALA-9231: support customized privilege checks for SHOW visibility
......................................................................
Patch Set 4:
(10 comments)
Thanks for the review! Rename the flag and throw exception for illegal flag value.
http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@282
PS3, Line 282: min_privilege_set_for_sh
> may be rename to min_privilege_set_for_show_stmts?
Done. Feel hard to find a good name for this...
http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@283
PS3, Line 283: Any o
> Any one of them..
Done
http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@284
PS3, Line 284: "or table. Defaults to \"any\" which means if the user has any privilege (CREATE, "
> s/Default/Defaults
Done
http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@288
PS3, Line 288: be shown
> s/practise/practice
Done
http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@291
PS3, Line 291: ec
> nit, additional space
Done
http://gerrit.cloudera.org:8080/#/c/14904/3/be/src/common/global-flags.cc@292
PS3, Line 292: e of privileges. No significant performance gain when using "
: "Ranger");
> I think this is redundant information and can be skipped.
Done
http://gerrit.cloudera.org:8080/#/c/14904/3/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java
File fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java:
http://gerrit.cloudera.org:8080/#/c/14904/3/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java@93
PS3, Line 93: hasAccess
> nit, not a blocker for this patch. I think sentry already does this for us.
Sorry, I'm not quite clear... This is the base class for both Sentry and Ranger authorization. Do you mean we can override the 'hasAnyAccess' function in SentryAuthorizationChecker using Sentry's API directly?
http://gerrit.cloudera.org:8080/#/c/14904/3/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:
http://gerrit.cloudera.org:8080/#/c/14904/3/fe/src/main/java/org/apache/impala/service/Frontend.java@326
PS3, Line 326: LOG.error("Illegal privilege name '{}'", pStr, e);
> Do you think we should throw ImpalaException here instead of silently loggi
Yes, that's better for careless users that even don't read logs.
http://gerrit.cloudera.org:8080/#/c/14904/2/tests/authorization/test_authorization.py
File tests/authorization/test_authorization.py:
http://gerrit.cloudera.org:8080/#/c/14904/2/tests/authorization/test_authorization.py@574
PS2, Line 574:
> flake8: E131 continuation line unaligned for hanging indent
Done
http://gerrit.cloudera.org:8080/#/c/14904/2/tests/authorization/test_authorization.py@600
PS2, Line 600:
> flake8: E131 continuation line unaligned for hanging indent
Done
--
To view, visit http://gerrit.cloudera.org:8080/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Dec 2019 02:09:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9231: support customized privilege checks for SHOW visibility
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/14904 )
Change subject: IMPALA-9231: support customized privilege checks for SHOW visibility
......................................................................
IMPALA-9231: support customized privilege checks for SHOW visibility
In IMPALA-9002 we introduce a flag simplify_check_on_show_tables which
simplifies privilege checks for SHOW TABLES. Only tables with privileges
implying SELECT privilege will be shown.
This patch provides the same mechanism for SHOW DATABASES. Also augment
the flag to be a list of privilege names and rename it to
min_privilege_set_for_show_stmts. The default value is "any" which
remains the default behavior. If set to "select", only dbs/tables on
which the user has SELECT privilege will be shown. If set to
"select,insert", only dbs/tables on which the user has SELECT or INSERT
privilege will be shown.
Tests:
- Add tests in test_authorization.py
Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Reviewed-on: http://gerrit.cloudera.org:8080/14904
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/common/global-flags.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M tests/authorization/test_authorization.py
9 files changed, 217 insertions(+), 52 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 6
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9231: support customized privilege checks for SHOW visibility
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14904 )
Change subject: IMPALA-9231: support customized privilege checks for SHOW visibility
......................................................................
Patch Set 4: Code-Review+2
Thanks for making the suggested changes. Patch looks good to me.
--
To view, visit http://gerrit.cloudera.org:8080/14904
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I631fc5c386a52f0a1f62182473be15fcc3dd8609
Gerrit-Change-Number: 14904
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Dec 2019 18:03:52 +0000
Gerrit-HasComments: No