Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13285 )
Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext ...................................................................... Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@421 PS5, Line 421: AuthorizationContext authzCtx = null; : try { : authzCtx = authzChecker.preAuthorize(analysisResult_, catalog_); : authzChecker.authorize(authzCtx, analysisResult_, catalog_); : } catch (AuthorizationException e) { : authException = e; : } finally { : if (authzCtx != null) { : authzChecker.postAuthorize(authzCtx, analysisResult_, catalog_); : } : } : : // AuthorizationExceptions take precedence over AnalysisExceptions so as not : // to reveal the existence/absence of objects the user is not authorized to see. : if (authException != null) throw authException; > Can't this just be a try { } finally { } and you can remove the authException > local variable? I think it just makes it more readable since it's easy to tell the precedence with regards to AnalysisException vs AuthorizationException. > Also it seems like preAuthorize, authorize, postAuthorize are all called > sequentially. I'm not sure of the benefit of having them be separate methods > in the interface. The idea is to provide pre and post hooks for every authorization provider without authorization provider worries where and when the hooks are going to be applied. For example if we change the logic to try { pre authorize do A do B authorize do C } finally { do D post authorize } none of the authorization provider code needs to be modified. http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@29 PS5, Line 29: public interface AuthorizationChecker { > Is there a reason that we need preAuth, auth, postAuth as opposed to just h See previous comment. http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@38 PS5, Line 38: Executes some code before the authorization check. > nit: this documentation seems a bit casual. Done http://gerrit.cloudera.org:8080/#/c/13285/5/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@53 PS5, Line 53: * Executes some code after the authorization check. > nit: same as above Done -- To view, visit http://gerrit.cloudera.org:8080/13285 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3bc3a11220dae0f49ef3e73d9ff27a90e9d4a71c Gerrit-Change-Number: 13285 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Austin Nobis <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 10 May 2019 21:14:07 +0000 Gerrit-HasComments: Yes
