Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14356 )
Change subject: IMPALA-8587: show grant does not produce correct privileges ...................................................................... Patch Set 4: (5 comments) About the high level design: I prefer https://gerrit.cloudera.org/#/c/13673/ , as it gives back the exact privileges that the user/group has. I can imagine the scenario when you want to revoke someone's privilege to access a given object, so you call SHOW GRANT, and then revoke the privileges you see there. This will be more tricky if you cannot distinguish between server/db/table/column level privileges in SHOW GRANT's output. Your change is simpler, but the whole class is just as complex in my opinion. So I would prefer to take over Austin's change and some comments to make it clearer. It would be also good to know how Hive handles inherited privileges + how this works with Sentry in Impala. http://gerrit.cloudera.org:8080/#/c/14356/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14356/4//COMMIT_MSG@9 PS4, Line 9: Currently the show grant command cannot produce correct privileges when the nit: please wrap commit message at 72 chars http://gerrit.cloudera.org:8080/#/c/14356/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/14356/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@180 PS4, Line 180: boolean nameIsPresent; : if (rangerPolicyResource.getValues().contains(ANY) && resourceName != null) { : nameIsPresent = true; : } else { : nameIsPresent = rangerPolicyResource.getValues().contains(resourceName); : } : : return nameIsPresent ? Optional.of(resourceName) : Optional.of(ANY); I would prefer to handle to ANY case elsewhere. The change is very small, but I still needed a lot of time to understand who does it actually changes the results. http://gerrit.cloudera.org:8080/#/c/14356/4/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/14356/4/tests/authorization/test_ranger.py@369 PS4, Line 369: unique_table): This test verifies a non-perfect solution, as it would be better to return the exact grant the gives the privileges on a given object. This could be mentioned in a comment. http://gerrit.cloudera.org:8080/#/c/14356/4/tests/authorization/test_ranger.py@372 PS4, Line 372: all It would be nice to add tests for mixed cases, e.g. when the user has only SELECT privilege on the server, but has CREATE on the database. Would we see both privileges listen in SHOW GRANT ON <db>? http://gerrit.cloudera.org:8080/#/c/14356/4/tests/authorization/test_ranger.py@379 PS4, Line 379: # Grant database privileges and verify It could be mentioned that the new privilege is masked completely by the db level privilege. -- To view, visit http://gerrit.cloudera.org:8080/14356 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8511656fe386a37a66d20e07ce1b875190bc4b65 Gerrit-Change-Number: 14356 Gerrit-PatchSet: 4 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Austin Nobis <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Mon, 27 Jan 2020 15:07:02 +0000 Gerrit-HasComments: Yes
