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/09/26 23:06:53 UTC

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Adam Holley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11531


Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................

IMPALA-7503: SHOW GRANT USER not showing all privileges.

This patch fixes the SHOW GRANT USER statement to show all privileges
granted to a user, either directly via object ownership, or granted
through a role via a group the user belongs to. The output for SHOW
GRANT USER will have two additional columns for privilege name and
privilege type so the user can know where the privilege comes from.

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
---
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
A fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
A fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
A testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
M tests/authorization/test_owner_privileges.py
A tests/authorization/test_show_grant_user.py
M tests/common/sentry_cache_test_suite.py
10 files changed, 1,188 insertions(+), 55 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11531/1
-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley <ah...@cloudera.com>

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.

Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 3:

(19 comments)

http://gerrit.cloudera.org:8080/#/c/11531/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11531/2//COMMIT_MSG@11
PS2, Line 11: The output for SHOW
            : GRANT USER will have two additional columns for privilege name and
            : privilege type so the user can know where the privilege comes from.
            : 
> Can we show a sample output here?
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@471
PS2, Line 471:    */
> I feel like it's better to have two separate methods, getRolePrivileges() a
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@472
PS2, Line 472: Name,
> showPrincipal is probably a better name for this.
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@474
PS2, Line 474: AnalysisException
> nit: move this to L473
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@544
PS2, Line 544: ic synchronized TResultSet getUserPrivileges(String principalName,
> This call can be quite expensive. We should only call it once L499 and L544
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@553
PS2, Line 553:         Type.STRING.toThrift()));
             :     addColumnOutputColumns(result.getSchema());
             :     resul
> We can avoid this copying if we create a method, e.g. getRolePrivileges tha
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
File fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java:

http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java@39
PS2, Line 39: 
> remove empty line
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
File fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java:

http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java@28
PS2, Line 28:       Model model) {
> fix indentation
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java@33
PS2, Line 33:       PolicyEngine policy, Model model) {
> fix indentation
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
File testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test:

http://gerrit.cloudera.org:8080/#/c/11531/2/testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test@1
PS2, Line 1: ====
> This test cases are a bit hard to read with the names that contain numbers,
The purpose of the tests is to verify different users to different groups to different role mappings.  If we use one group, there we'd only be testing one user.


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py
File tests/authorization/test_show_grant_user.py:

http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@1
PS2, Line 1: # Licensed to the Apache Software Foundation (ASF) under one
> nit: need consistency in this file whether to use ' or " for strings.
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@18
PS2, Line 18: # Client tests for SQL statement authorization
            : # These tests verify the functionality of SHOW GRANT USER. We
            : # create several users and groups to verify clear separation.
> this comment doesn't look correct.
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@75
PS2, Line 75:       # Ignore any errors here because these might fail for partial test runs.
            :       pass
> the finally isn't useful here.
I need an finally or except to ignore the statements above that might fail because of partial test runs.  Empty excepts fail flake8.


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@87
PS2, Line 87:     'org.apache.impala.service.CustomClusterResourceAuthorizationProvi
> nit: indentation
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@88
PS2, Line 88:     '--sentry_config={0}'.format(SENTRY_CONFIG_FILE),
            :       catalogd_args='--sentry_config={0}'.format(SENTRY_CONFIG_FILE) +
            :           ' --authorization_policy_provider_class=' +
> use python string format instead of +
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/common/sentry_cache_test_suite.py
File tests/common/sentry_cache_test_suite.py:

http://gerrit.cloudera.org:8080/#/c/11531/2/tests/common/sentry_cache_test_suite.py@43
PS2, Line 43: gnore_rol
> use snake_case, i.e. ignore_role
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/common/sentry_cache_test_suite.py@52
PS2, Line 52: offset
> call it index?
It's not the index though, it's an offset for the indexes.


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/common/sentry_cache_test_suite.py@52
PS2, Line 52:     offset = 2 if ignore_role else 0
            :     # results should have the following columns for offset 0
            :     # scope, dat
> can be simplified to index = 2 if ignore_role else 0
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/common/sentry_cache_test_suite.py@56
PS2, Line 56: principal name, principal type, scope, database, table, column, uri, priv
> Update the comment
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
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-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 17:20:31 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/839/ : 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/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
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-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 17:54:34 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 9:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/860/ : 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/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 9
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Sep 2018 07:46:02 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/826/ : 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/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Sep 2018 23:54:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 6:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/851/ : 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/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 6
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 21:54:25 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.

Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 2:

(19 comments)

http://gerrit.cloudera.org:8080/#/c/11531/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11531/2//COMMIT_MSG@11
PS2, Line 11: The output for SHOW
            : GRANT USER will have two additional columns for privilege name and
            : privilege type so the user can know where the privilege comes from.
            : 
Can we show a sample output here?


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@471
PS2, Line 471:   public synchronized TResultSet getPrincipalPrivileges(String principalName,
I feel like it's better to have two separate methods, getRolePrivileges() and getUserPrivileges(). It makes the code easier to read and we can factor out common code if needed.


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@472
PS2, Line 472: forUser
showPrincipal is probably a better name for this.


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@474
PS2, Line 474: AnalysisException
nit: move this to L473


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@544
PS2, Line 544: Set<String> groupNames = fe.getAuthzChecker().getUserGroups(user);
This call can be quite expensive. We should only call it once L499 and L544.


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@553
PS2, Line 553:         for (int i = 0; i < rolePrivs.rows.size(); ++i) {
             :           result.addToRows(rolePrivs.rows.get(i));
             :         }
We can avoid this copying if we create a method, e.g. getRolePrivileges that returns TResultRowBuilder.


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
File fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java:

http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java@39
PS2, Line 39: 
remove empty line


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
File fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java:

http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java@28
PS2, Line 28:                                                     
fix indentation


http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java@33
PS2, Line 33:                                                     
fix indentation


http://gerrit.cloudera.org:8080/#/c/11531/2/testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
File testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test:

http://gerrit.cloudera.org:8080/#/c/11531/2/testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test@1
PS2, Line 1: ====
This test cases are a bit hard to read with the names that contain numbers, i.e. group_1, group_2a, group_2b, etc. Is it not possible to just use a single group?


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py
File tests/authorization/test_show_grant_user.py:

http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@1
PS2, Line 1: # Licensed to the Apache Software Foundation (ASF) under one
nit: need consistency in this file whether to use ' or " for strings.


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@18
PS2, Line 18: # Client tests for SQL statement authorization
            : # In these tests we run all the tests twice. Once with just using cache, hence the
            : # long sentry poll, and once by ensuring refreshes happen from Sentry.
this comment doesn't look correct.


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@75
PS2, Line 75:     finally:
            :       pass
the finally isn't useful here.


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@87
PS2, Line 87: "org.apache.impala.service.CustomClusterResourceAuthorizationProvider"
nit: indentation


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@88
PS2, Line 88: catalogd_args="--sentry_config=" + SENTRY_CONFIG_FILE +
            :       " --authorization_policy_provider_class=" +
            :       "org.apache.impala.service.CustomClusterResourceAuthorizationProvider"
use python string format instead of +


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/common/sentry_cache_test_suite.py
File tests/common/sentry_cache_test_suite.py:

http://gerrit.cloudera.org:8080/#/c/11531/2/tests/common/sentry_cache_test_suite.py@43
PS2, Line 43: gnoreRole
use snake_case, i.e. ignore_role


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/common/sentry_cache_test_suite.py@52
PS2, Line 52: offset
call it index?


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/common/sentry_cache_test_suite.py@52
PS2, Line 52:     offset = 0
            :     if ignoreRole:
            :       offset = 2
can be simplified to index = 2 if ignore_role else 0


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/common/sentry_cache_test_suite.py@56
PS2, Line 56: scope, database, table, column, uri, privilege, grant_option, create_time
Update the comment



-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 2
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: Thu, 27 Sep 2018 03:36:23 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 10:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3244/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 10
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Sep 2018 13:23:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.

Adam Holley has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................

IMPALA-7503: SHOW GRANT USER not showing all privileges.

This patch fixes the SHOW GRANT USER statement to show all privileges
granted to a user, either directly via object ownership, or granted
through a role via a group the user belongs to. The output for SHOW
GRANT USER will have two additional columns for privilege name and
privilege type so the user can know where the privilege comes from.

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
---
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
A fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
A fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
A testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
A tests/authorization/test_show_grant_user.py
M tests/common/sentry_cache_test_suite.py
11 files changed, 1,245 insertions(+), 76 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11531/5
--
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
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>

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

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/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................

IMPALA-7503: SHOW GRANT USER not showing all privileges.

This patch fixes the SHOW GRANT USER statement to show all privileges
granted to a user, either directly via object ownership, or granted
through a role via a group the user belongs to. The output for SHOW
GRANT USER will have two additional columns for privilege name and
privilege type so the user can know where the privilege comes from.

Truncated sample showing two columns that are different from role:
+----------------+----------------+--------+----------+-...
| principal_type | principal_name | scope  | database | ...
+----------------+----------------+--------+----------+-...
| USER           | foo            | table  | foo_db   | ...
| ROLE           | foo_role       | server |          | ...
+----------------+----------------+--------+----------+-...

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Reviewed-on: http://gerrit.cloudera.org:8080/11531
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
A fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
A fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
M fe/src/test/resources/sentry-site_no_oo.xml.template
M fe/src/test/resources/sentry-site_oo.xml.template
M fe/src/test/resources/sentry-site_oo_nogrant.xml.template
M testdata/bin/run-sentry-service.sh
A testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
A tests/authorization/test_show_grant_user.py
M tests/common/sentry_cache_test_suite.py
15 files changed, 1,400 insertions(+), 177 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 11
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/829/ : 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/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 2
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: Thu, 27 Sep 2018 00:50:36 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 1:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/11531/1/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11531/1/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@479
PS1, Line 479:       result.getSchema().addToColumns(new TColumn("principal_type", Type.STRING.toThrift()));
line too long (93 > 90)


http://gerrit.cloudera.org:8080/#/c/11531/1/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@480
PS1, Line 480:       result.getSchema().addToColumns(new TColumn("principal_name", Type.STRING.toThrift()));
line too long (93 > 90)


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/authorization/test_show_grant_user.py
File tests/authorization/test_show_grant_user.py:

http://gerrit.cloudera.org:8080/#/c/11531/1/tests/authorization/test_show_grant_user.py@26
PS1, Line 26: from time import sleep
flake8: F401 'time.sleep' imported but unused


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/authorization/test_show_grant_user.py@30
PS1, Line 30: from tests.util.calculation_util import get_random_id
flake8: F401 'tests.util.calculation_util.get_random_id' imported but unused


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/authorization/test_show_grant_user.py@31
PS1, Line 31: from tests.verifiers.metric_verifier import MetricVerifier
flake8: F401 'tests.verifiers.metric_verifier.MetricVerifier' imported but unused


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/common/sentry_cache_test_suite.py
File tests/common/sentry_cache_test_suite.py:

http://gerrit.cloudera.org:8080/#/c/11531/1/tests/common/sentry_cache_test_suite.py@62
PS1, Line 62: +
flake8: E226 missing whitespace around arithmetic operator


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/common/sentry_cache_test_suite.py@64
PS1, Line 64: +
flake8: E226 missing whitespace around arithmetic operator


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/common/sentry_cache_test_suite.py@65
PS1, Line 65: +
flake8: E226 missing whitespace around arithmetic operator


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/common/sentry_cache_test_suite.py@66
PS1, Line 66: +
flake8: E226 missing whitespace around arithmetic operator



-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Sep 2018 23:07:33 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.

Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 8:

The reason the build failed is because of "test_user".  This user is not considered valid in the cluster tests because it does not belong to a group.  I updated the CustomClusterGroupMapping to add new users and groups for the owner_privilege tests. This removes previous concerns about running with user "root". No core code was changed for patch set 8.


-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 8
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Sep 2018 07:00:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 8:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/859/ : 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/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 8
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Sep 2018 07:31:03 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 10: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 10
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Sep 2018 17:05:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.

Adam Holley has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................

IMPALA-7503: SHOW GRANT USER not showing all privileges.

This patch fixes the SHOW GRANT USER statement to show all privileges
granted to a user, either directly via object ownership, or granted
through a role via a group the user belongs to. The output for SHOW
GRANT USER will have two additional columns for privilege name and
privilege type so the user can know where the privilege comes from.

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
---
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
A fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
A fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
A testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
M tests/authorization/test_owner_privileges.py
A tests/authorization/test_show_grant_user.py
M tests/common/sentry_cache_test_suite.py
10 files changed, 1,187 insertions(+), 55 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11531/2
-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 9: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 9
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Sep 2018 10:54:22 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.

Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 5: Code-Review+1

(2 comments)

Some nits, LGTM otherwise. I'll let Csaba take a look at it, too.

http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@478
PS5, Line 478: Principal
nit: this should be Role


http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@559
PS5, Line 559: Principal
nit: this should be User



-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 5
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 18:41:07 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.

Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11531/3/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:

http://gerrit.cloudera.org:8080/#/c/11531/3/fe/src/main/java/org/apache/impala/service/JniFrontend.java@571
PS3, Line 571:       throw new AnalysisException("Unexpected TPrincipalType: " +
> line too long (94 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/11531/3/tests/authorization/test_grant_revoke.py
File tests/authorization/test_grant_revoke.py:

http://gerrit.cloudera.org:8080/#/c/11531/3/tests/authorization/test_grant_revoke.py@293
PS3, Line 293: 
> flake8: E122 continuation line missing indentation or outdented
Done


http://gerrit.cloudera.org:8080/#/c/11531/3/tests/authorization/test_show_grant_user.py
File tests/authorization/test_show_grant_user.py:

http://gerrit.cloudera.org:8080/#/c/11531/3/tests/authorization/test_show_grant_user.py@87
PS3, Line 87: .
> flake8: E131 continuation line unaligned for hanging indent
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
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-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 17:26:51 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.

Adam Holley has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................

IMPALA-7503: SHOW GRANT USER not showing all privileges.

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11531/7
--
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 7
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 8:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/11531/8/tests/authorization/test_owner_privileges.py
File tests/authorization/test_owner_privileges.py:

http://gerrit.cloudera.org:8080/#/c/11531/8/tests/authorization/test_owner_privileges.py@118
PS8, Line 118:  
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/11531/8/tests/authorization/test_owner_privileges.py@118
PS8, Line 118:       impalad_args="--server_name=server1 --sentry_config={0} " 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/11531/8/tests/authorization/test_owner_privileges.py@234
PS8, Line 234:  
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/11531/8/tests/authorization/test_owner_privileges.py@234
PS8, Line 234:       impalad_args="--server_name=server1 --sentry_config={0} " 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/11531/8/tests/authorization/test_owner_privileges.py@307
PS8, Line 307:  
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/11531/8/tests/authorization/test_owner_privileges.py@307
PS8, Line 307:       impalad_args="--server_name=server1 --sentry_config={0} " 
line has trailing whitespace



-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 8
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Sep 2018 06:58:37 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.

Adam Holley has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................

IMPALA-7503: SHOW GRANT USER not showing all privileges.

This patch fixes the SHOW GRANT USER statement to show all privileges
granted to a user, either directly via object ownership, or granted
through a role via a group the user belongs to. The output for SHOW
GRANT USER will have two additional columns for privilege name and
privilege type so the user can know where the privilege comes from.

Truncated sample showing two columns that are different from role:
+----------------+----------------+--------+----------+-...
| principal_type | principal_name | scope  | database | ...
+----------------+----------------+--------+----------+-...
| USER           | foo            | table  | foo_db   | ...
| ROLE           | foo_role       | server |          | ...
+----------------+----------------+--------+----------+-...

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
---
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
A fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
A fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
M fe/src/test/resources/sentry-site_no_oo.xml.template
M fe/src/test/resources/sentry-site_oo.xml.template
M fe/src/test/resources/sentry-site_oo_nogrant.xml.template
M testdata/bin/run-sentry-service.sh
A testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
A tests/authorization/test_show_grant_user.py
M tests/common/sentry_cache_test_suite.py
15 files changed, 1,400 insertions(+), 177 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11531/9
-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 9
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 10: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 10
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Sep 2018 13:23:43 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.

Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 4:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
File fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java:

http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java@69
PS4, Line 69:       if (principalType_ == TPrincipalType.ROLE) {
            :         throw new AnalysisException(String.format("%s '%s' does not exist.",
            :             Principal.toString(principalType_), name_));
            :       } else if (principalType_ == TPrincipalType.USER){
            :         // Create a user object here because it's possible the user does not exist in
            :         // Sentry, but still exists according to the OS, or Hadoop, or other custom
            :         // group mapping provider.
            :         principal_ = Principal.newInstance(name_, principalType_, Sets.newHashSet());
            :       } else {
            :         throw new AnalysisException(String.format("Unexpected TPrincipalType: %s",
            :             Principal.toString(principalType_)));
            :       }
Usually we use switch statement for enums: https://stackoverflow.com/questions/4922932/java-performance-of-enums-vs-if-then-else


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@404
PS4, Line 404: 
nit: extra empty line


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@468
PS4, Line 468:    * Returns the privileges that have been granted to a principal as a tabular result set.
             :    * Allows for filtering based on a specific privilege spec or showing all privileges
             :    * granted to the principal. Used by the SHOW GRANT ROLE statement.
replace the word "principal" to "role"


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@473
PS4, Line 473: TPrincipalType type
Is the TPrincipalType still necessary since we know it should always be TPrincipalType.USER?


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@480
PS4, Line 480: getPrincipal(principalName, type)
Can we do getUser(principalName) instead?


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@485
PS4, Line 485: if (isPrivilegeFiltered(filter, privilege)) continue;
use && with L484 to avoid nested ifs.


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@521
PS4, Line 521: addCommonOutputResults
Maybe addShowPrincipalOutputResults is a better name? addCommonOutputResults is to generic and this class is not specific to "SHOW GRANT" commands.


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@542
PS4, Line 542: principal
replace principal with user.


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@545
PS4, Line 545: TPrincipalType type
Is the TPrincipalType necessary since we know it should always be TPrincipalType.USER?


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@557
PS4, Line 557: principal
user


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@585
PS4, Line 585: Principal rolePrincipal = getPrincipal(role.getName(), TPrincipalType.ROLE);
Can we do getRole(role.getName()) instead?


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@590
PS4, Line 590: if (isPrivilegeFiltered(filter, privilege)) continue;
use && with L589 to avoid nested ifs.


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:

http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/service/JniFrontend.java@564
PS4, Line 564:     if (params.getPrincipal_type() == TPrincipalType.USER) {
             :       result = frontend_.getCatalog().getAuthPolicy().getUserPrivileges(
             :           params.getName(), params.getPrivilege(), params.getPrincipal_type(), frontend_);
             :     } else  if (params.getPrincipal_type() == TPrincipalType.ROLE) {
             :       result = frontend_.getCatalog().getAuthPolicy().getRolePrivileges(
             :           params.getName(), params.getPrivilege(), params.getPrincipal_type());
             :     } else {
             :       throw new AnalysisException("Unexpected TPrincipalType: " +
             :           params.getPrincipal_type());
             :     }
Usually we use switch statement for enums: https://stackoverflow.com/questions/4922932/java-performance-of-enums-vs-if-then-else


http://gerrit.cloudera.org:8080/#/c/11531/4/tests/authorization/test_owner_privileges.py
File tests/authorization/test_owner_privileges.py:

http://gerrit.cloudera.org:8080/#/c/11531/4/tests/authorization/test_owner_privileges.py@89
PS4, Line 89: scope, database, table, column, uri, privilege, grant_option, create_time
Update comment to include missing "principal_type, principal_name"


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py
File tests/authorization/test_show_grant_user.py:

http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@75
PS2, Line 75:       # Ignore any errors here because these might fail for partial test runs.
            :       pass
> I need an finally or except to ignore the statements above that might fail 
I don't think that's what try/finally is for.


http://gerrit.cloudera.org:8080/#/c/11531/4/tests/authorization/test_show_grant_user.py
File tests/authorization/test_show_grant_user.py:

http://gerrit.cloudera.org:8080/#/c/11531/4/tests/authorization/test_show_grant_user.py@86
PS4, Line 86:       impalad_args='--server_name=server1 --authorization_policy_provider_class=' +
            :       'org.apache.impala.service.CustomClusterResourceAuthorizationProvider ' +
            :       '--sentry_config={0}'.format(SENTRY_CONFIG_FILE),
            :       catalogd_args='--sentry_config={0}'.format(SENTRY_CONFIG_FILE) +
            :       ' --authorization_policy_provider_class=' +
            :       'org.apache.impala.service.CustomClusterResourceAuthorizationProvider',
            :       sentry_config=SENTRY_CONFIG_FILE)
A mixed use of + and Python string format. Can we use Python string format instead?



-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
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-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 18:10:25 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/844/ : 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/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
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-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 18:15:32 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.

Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 2:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/11531/1/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11531/1/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@479
PS1, Line 479:       result.getSchema().addToColumns(new TColumn("principal_type",
> line too long (93 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/11531/1/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@480
PS1, Line 480:           Type.STRING.toThrift()));
> line too long (93 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/authorization/test_show_grant_user.py
File tests/authorization/test_show_grant_user.py:

http://gerrit.cloudera.org:8080/#/c/11531/1/tests/authorization/test_show_grant_user.py@26
PS1, Line 26: 
> flake8: F401 'time.sleep' imported but unused
Done


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/authorization/test_show_grant_user.py@30
PS1, Line 30: SENTRY_CONFIG_FILE = getenv('IMPALA_HOME') + \
> flake8: F401 'tests.util.calculation_util.get_random_id' imported but unuse
Done


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/authorization/test_show_grant_user.py@31
PS1, Line 31:     '/fe/src/test/resources/sentry-site_oo.xml'
> flake8: F401 'tests.verifiers.metric_verifier.MetricVerifier' imported but 
Done


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/common/sentry_cache_test_suite.py
File tests/common/sentry_cache_test_suite.py:

http://gerrit.cloudera.org:8080/#/c/11531/1/tests/common/sentry_cache_test_suite.py@62
PS1, Line 62:  
> flake8: E226 missing whitespace around arithmetic operator
Done


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/common/sentry_cache_test_suite.py@64
PS1, Line 64:  
> flake8: E226 missing whitespace around arithmetic operator
Done


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/common/sentry_cache_test_suite.py@65
PS1, Line 65:  
> flake8: E226 missing whitespace around arithmetic operator
Done


http://gerrit.cloudera.org:8080/#/c/11531/1/tests/common/sentry_cache_test_suite.py@66
PS1, Line 66:  
> flake8: E226 missing whitespace around arithmetic operator
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 2
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: Thu, 27 Sep 2018 00:01:02 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 9:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3241/ DRY_RUN=true


-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 9
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Sep 2018 07:02:27 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.

Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 9: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 9
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Sep 2018 13:21:59 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.

Adam Holley has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................

IMPALA-7503: SHOW GRANT USER not showing all privileges.

This patch fixes the SHOW GRANT USER statement to show all privileges
granted to a user, either directly via object ownership, or granted
through a role via a group the user belongs to. The output for SHOW
GRANT USER will have two additional columns for privilege name and
privilege type so the user can know where the privilege comes from.

Truncated sample showing two columns that are different from role:
+----------------+----------------+--------+----------+-...
| principal_type | principal_name | scope  | database | ...
+----------------+----------------+--------+----------+-...
| USER           | foo            | table  | foo_db   | ...
| ROLE           | foo_role       | server |          | ...
+----------------+----------------+--------+----------+-...

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
---
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
A fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
A fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
M fe/src/test/resources/sentry-site_no_oo.xml.template
M fe/src/test/resources/sentry-site_oo.xml.template
M fe/src/test/resources/sentry-site_oo_nogrant.xml.template
M testdata/bin/run-sentry-service.sh
A testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test
M tests/authorization/test_grant_revoke.py
M tests/authorization/test_owner_privileges.py
A tests/authorization/test_show_grant_user.py
M tests/common/sentry_cache_test_suite.py
15 files changed, 1,400 insertions(+), 177 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11531/8
-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 8
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.

Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 5:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
File fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java:

http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java@69
PS4, Line 69:       switch (principalType_) {
            :         case ROLE:
            :           throw new AnalysisException(String.format("%s '%s' does not exist.",
            :               Principal.toString(principalType_), name_));
            :         case USER:
            :           // Create a user object here because it's possible the user does not exist in
            :           // Sentry, but still exists according to the OS, or Hadoop, or other custom
            :           // group mapping provider.
            :           principal_ = Principal.newInstance(name_, principalType_, Sets.newHashSet());
            :           break;
            :         default:
            :        
> Usually we use switch statement for enums: https://stackoverflow.com/questi
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@404
PS4, Line 404:   }
> nit: extra empty line
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@468
PS4, Line 468:    * Allows for filtering based on a specific privilege spec or showing all privileges
             :    * granted to the role. Used by the SHOW GRANT ROLE statement.
             :    */
> replace the word "principal" to "role"
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@473
PS4, Line 473: ew TResultSet();
> Is the TPrincipalType still necessary since we know it should always be TPr
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@480
PS4, Line 480: ege p : role.getPrivileges()) {
> Can we do getUser(principalName) instead?
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@485
PS4, Line 485: 
> use && with L484 to avoid nested ifs.
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@521
PS4, Line 521: ullToEmpty(privilege.g
> Maybe addShowPrincipalOutputResults is a better name? addCommonOutputResult
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@542
PS4, Line 542: t = new T
> replace principal with user.
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@545
PS4, Line 545: dToColumns(new TCol
> Is the TPrincipalType necessary since we know it should always be TPrincipa
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@557
PS4, Line 557: cipalName
> user
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@585
PS4, Line 585:     TResultRowBuilder rowBuilder = new TResultRowBuilder();
> Can we do getRole(role.getName()) instead?
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@590
PS4, Line 590: 
> use && with L589 to avoid nested ifs.
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:

http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/service/JniFrontend.java@564
PS4, Line 564:     switch (params.getPrincipal_type()) {
             :       case USER:
             :         result = frontend_.getCatalog().getAuthPolicy().getUserPrivileges(
             :             params.getName(), params.getPrivilege(), frontend_);
             :         break;
             :       case ROLE:
             :         result = frontend_.getCatalog().getAuthPolicy().getRolePrivileges(
             :             params.getName(), params.getPrivilege());
             :         break;
             :      
> Usually we use switch statement for enums: https://stackoverflow.com/questi
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/tests/authorization/test_owner_privileges.py
File tests/authorization/test_owner_privileges.py:

http://gerrit.cloudera.org:8080/#/c/11531/4/tests/authorization/test_owner_privileges.py@89
PS4, Line 89: principal_name, principal_type, scope, database, table, column, uri, priv
> Update comment to include missing "principal_type, principal_name"
Done


http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py
File tests/authorization/test_show_grant_user.py:

http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@75
PS2, Line 75:   def restart_first_impalad(cls):
            :     impala
> I don't think that's what try/finally is for.
Done


http://gerrit.cloudera.org:8080/#/c/11531/4/tests/authorization/test_show_grant_user.py
File tests/authorization/test_show_grant_user.py:

http://gerrit.cloudera.org:8080/#/c/11531/4/tests/authorization/test_show_grant_user.py@86
PS4, Line 86:       'org.apache.impala.service.CustomClusterResourceAuthorizationProvider'
            :       .format(SENTRY_CONFIG_FILE),
            :       sentry_config=SENTRY_CONFIG_FILE)
            :   def test_show_grant_user(self, vector, unique_database):
            :     group_name = grp.getgrnam(getuser()).gr_name
            :     self.client.execute('create role sgu_test_primary')
            :     self.client.execute('grant all on s
> A mixed use of + and Python string format. Can we use Python string format 
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
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: Thu, 27 Sep 2018 18:35:15 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.

Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11531/6/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11531/6/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@582
PS6, Line 582:    * This method adds the rows to the output for the SHOW GRANT USER statement for user
> nit: adds
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 7
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 21:38:53 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.

Adam Holley has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................

IMPALA-7503: SHOW GRANT USER not showing all privileges.

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11531/4
--
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
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-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.

Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 6: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11531/6/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11531/6/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@582
PS6, Line 582:    * This method add the rows to the output for the SHOW GRANT USER statement for user
nit: adds


http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
File fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java:

http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java@32
PS5, Line 32: CustomClusterGroupMapper
> It's used in test_show_grant_user.py via the CustomClusterResourceAuthoriza
Thanks, I did not realize how that works.



-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 6
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 21:35:55 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.

Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 7: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 7
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 23:11:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.

Adam Holley has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................

IMPALA-7503: SHOW GRANT USER not showing all privileges.

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11531/3
--
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
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-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 7:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3240/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 7
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 23:12:02 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.

Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 5:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@563
PS5, Line 563:         if (filter != null) {
             :           if (isPrivilegeFiltered(filter, privilege)) continue;
             :         }
This seems to be the same logic as line 584, so it would better to make them look consistent.


http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@583
PS5, Line 583:           TPrivilege privilege = p.toThrift();
             :           if (filter != null && isPrivilegeFiltered(filter, privilege)) continue;
             :           TResultRowBuilder rowBuilder = new TResultRowBuilder();
             :           rowBuilder.add(Strings.nullToEmpty(TPrincipalType.ROLE.name().toUpperCase()));
             :           rowBuilder.add(Strings.nullToEmpty(rolePrincipal.getName()));
             :           result.addToRows(addShowPrincipalOutputResults(privilege, rowBuilder).get());
optional: The block in this loop and the one at line 561 seems very similar and could be extracted to a like createShowUserPrivilegesResultRow()


http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
File fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java:

http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java@32
PS5, Line 32: CustomClusterGroupMapper
I can't find where this class is used.


http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
File fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java:

http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java@25
PS5, Line 25: CustomClusterResourceAuthorizationProvider
I can't find where this class is used.


http://gerrit.cloudera.org:8080/#/c/11531/5/tests/authorization/test_owner_privileges.py
File tests/authorization/test_owner_privileges.py:

http://gerrit.cloudera.org:8080/#/c/11531/5/tests/authorization/test_owner_privileges.py@163
PS5, Line 163: ignore_role=True
optional: ignore_role seems redundant to me - it is always true for "show grant user", and always false for "show grant role".

I see several options, like deducing information by checking the query string, or creating separate validate_role_privileges/validate_user_privileges functions.


http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py
File tests/common/sentry_cache_test_suite.py:

http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py@47
PS5, Line 47: If we ignore_role
            :     output
This part of the sentence seem strange.


http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py@62
PS5, Line 62: offset
optional: offsets could be replace with 

if ignore_role:
  col = col[2:]


http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py@75
PS5, Line 75: the ignore role privileges.
I do not understand this part of the sentence - can you rephrase it?



-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 5
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 20:32:05 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 7: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/3240/


-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 7
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Sep 2018 02:49:22 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/848/ : 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/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 5
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 18:52:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.

Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 5:

Forgot to mention that I skipped the .test file during the review.


-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 5
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 20:32:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11531/3/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:

http://gerrit.cloudera.org:8080/#/c/11531/3/fe/src/main/java/org/apache/impala/service/JniFrontend.java@571
PS3, Line 571:       throw new AnalysisException("Unexpected TPrincipalType: " + params.getPrincipal_type());
line too long (94 > 90)


http://gerrit.cloudera.org:8080/#/c/11531/3/tests/authorization/test_grant_revoke.py
File tests/authorization/test_grant_revoke.py:

http://gerrit.cloudera.org:8080/#/c/11531/3/tests/authorization/test_grant_revoke.py@293
PS3, Line 293: r
flake8: E122 continuation line missing indentation or outdented


http://gerrit.cloudera.org:8080/#/c/11531/3/tests/authorization/test_show_grant_user.py
File tests/authorization/test_show_grant_user.py:

http://gerrit.cloudera.org:8080/#/c/11531/3/tests/authorization/test_show_grant_user.py@87
PS3, Line 87: '
flake8: E131 continuation line unaligned for hanging indent



-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
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-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 17:21:10 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.

Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 6:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:

http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@478
PS5, Line 478: Role role
> nit: this should be Role
Done


http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@559
PS5, Line 559: User user
> nit: this should be User
Done


http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@563
PS5, Line 563:     }
             : 
             :     // Ge
> This seems to be the same logic as line 584, so it would better to make the
Done


http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@583
PS5, Line 583:    * and associated roles.
             :    */
             :   private void createShowUserPrivilegesResultRows(TResultSet result,
             :       List<PrincipalPrivilege> privileges, TPrivilege filter, String name,
             :       TPrincipalType type) {
             :     for (PrincipalPrivilege p : privileges) {
> optional: The block in this loop and the one at line 561 seems very similar
Done


http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java
File fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java:

http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java@32
PS5, Line 32: CustomClusterGroupMapper
> I can't find where this class is used.
It's used in test_show_grant_user.py via the CustomClusterResourceAuthorizationProvider


http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java
File fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java:

http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java@25
PS5, Line 25: CustomClusterResourceAuthorizationProvider
> I can't find where this class is used.
It's used in test_show_grant_user.py.


http://gerrit.cloudera.org:8080/#/c/11531/5/tests/authorization/test_owner_privileges.py
File tests/authorization/test_owner_privileges.py:

http://gerrit.cloudera.org:8080/#/c/11531/5/tests/authorization/test_owner_privileges.py@163
PS5, Line 163: 
> optional: ignore_role seems redundant to me - it is always true for "show g
Done


http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py
File tests/common/sentry_cache_test_suite.py:

http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py@47
PS5, Line 47: If the query is
            :     for "s
> This part of the sentence seem strange.
Reworded.


http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py@62
PS5, Line 62: [2:]
> optional: offsets could be replace with 
Done


http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py@75
PS5, Line 75: ached. If delay_s is > 0 th
> I do not understand this part of the sentence - can you rephrase it?
Removed.



-- 
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 6
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 21:23:40 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................


Patch Set 7:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/853/ : 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/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 7
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Sep 2018 22:13:59 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7503: SHOW GRANT USER not showing all privileges.

Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.

Adam Holley has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/11531 )

Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges.
......................................................................

IMPALA-7503: SHOW GRANT USER not showing all privileges.

Testing:
- Create new custom cluster test with custom group mapping.
- Ran FE and custom cluster tests.

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11531/6
--
To view, visit http://gerrit.cloudera.org:8080/11531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1
Gerrit-Change-Number: 11531
Gerrit-PatchSet: 6
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>