Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13744 )
Change subject: IMPALA-8716: Log a group of privileges into a single audit event. ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/13744/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/13744/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@318 PS2, Line 318: fferAu > nit: authorized ? (same below) Done http://gerrit.cloudera.org:8080/#/c/13744/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@319 PS2, Line 319: null : new RangerBufferAuditHandler(originalAuditHandler); : for (Privilege impliedPrivilege: privilege.getImpliedPrivileges()) { : if (authorizeResource(authzCtx, resource, user, impliedPrivilege, : tmpAuditHandler)) { : authorized = true; > I think this could use a comment. Also, create a copy c'tor? (same below) Good idea about copy constructor. Done. http://gerrit.cloudera.org:8080/#/c/13744/2/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@385 PS2, Line 385: RangerBufferAuditHandler auditHandler) throws InternalException { > just checking, is the accessResult automatically inferred from newAuditEven Yeah since we're assigning newAuditEvent to an existing instance and mutating it, so it will have the accessResult. http://gerrit.cloudera.org:8080/#/c/13744/2/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java File fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java: http://gerrit.cloudera.org:8080/#/c/13744/2/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java@141 PS2, Line 141: 1 > Mind adding a couple more tests? Those test cases have already been been covered: accessResult = 0, isAny = true in L193-L199 accessResult = 1, isAny = false in L63-L136 --> isAny = false is basically all privileges in https://github.com/apache/impala/blob/c353cf7a648651244ac39677d0cb028e704281d0/fe/src/main/java/org/apache/impala/authorization/Privilege.java#L28-L35 (anyOf_ = false) -- To view, visit http://gerrit.cloudera.org:8080/13744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib53102bc1ceaf9d62544090dc00f3231fae0efca Gerrit-Change-Number: 13744 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Mon, 01 Jul 2019 19:23:20 +0000 Gerrit-HasComments: Yes
