Austin Nobis has posted comments on this change. ( http://gerrit.cloudera.org:8080/13285 )
Change subject: IMPALA-8528: Refactor authorization check in AnalysisContext ...................................................................... Patch Set 5: (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? 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. 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 having an auth method and leaving those details up the class that implements this interface? 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. maybe: "Function to be executed before an authorization check occurs." 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 -- 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: 5 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 19:47:50 +0000 Gerrit-HasComments: Yes
