Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11762 )
Change subject: IMPALA-7742: Stores the Sentry user names in a case sensitive way ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/11762/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11762/1//COMMIT_MSG@11 PS1, Line 11: store > nit: store Done http://gerrit.cloudera.org:8080/#/c/11762/1/fe/src/main/java/org/apache/impala/catalog/User.java File fe/src/main/java/org/apache/impala/catalog/User.java: http://gerrit.cloudera.org:8080/#/c/11762/1/fe/src/main/java/org/apache/impala/catalog/User.java@41 PS1, Line 41: true > isn't this the one that needs to be switched to false? This actually only stores the privilege name, which should be case insensitive, especially looking at the existing code. The earlier comment was actually wrong. https://github.com/apache/impala/blob/93ee538c5403509804cb18411e4407352f5b242b/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java#L117-L118 https://github.com/apache/impala/blob/93ee538c5403509804cb18411e4407352f5b242b/fe/src/main/java/org/apache/impala/util/SentryProxy.java#L266 http://gerrit.cloudera.org:8080/#/c/11762/1/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/11762/1/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java@59 PS1, Line 59: FOOBAR > what is being tested with case sensitive group names? Because by putting "foobar" and "FOOBAR" to be in the same group will make any users regardless of the case granted to "foobar" (the group) authorized. http://gerrit.cloudera.org:8080/#/c/11762/1/tests/authorization/test_owner_privileges.py File tests/authorization/test_owner_privileges.py: http://gerrit.cloudera.org:8080/#/c/11762/1/tests/authorization/test_owner_privileges.py@414 PS1, Line 414: case > nit: case Done http://gerrit.cloudera.org:8080/#/c/11762/1/tests/authorization/test_owner_privileges.py@418 PS1, Line 418: FOOBAR_impalad_client = self.create_impala_client() > why are two clients needed? Yeah it's a bit weird but that's because a session is a connection to a particular user although we don't user a user name when creating a session(??). This is an example from an existing code: https://github.com/apache/impala/blob/93ee538c5403509804cb18411e4407352f5b242b/tests/common/impala_test_suite.py#L460-L462 -- To view, visit http://gerrit.cloudera.org:8080/11762 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04bec045e3f70fc4f41b16b9b5c55eeb60bd63b8 Gerrit-Change-Number: 11762 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 26 Oct 2018 19:14:08 +0000 Gerrit-HasComments: Yes
