Alex Behm has posted comments on this change. Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary ......................................................................
Patch Set 1: (12 comments) Still trying to understand all the different paths where the profile can be fetched. Here are a few comments to start with. http://gerrit.cloudera.org:8080/#/c/7064/1/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: Line 66: static const string LIMITED_PROFILE_INFO_STRINGS[] = {"Session ID", "Session Type", Will this show the timeline? http://gerrit.cloudera.org:8080/#/c/7064/1/be/src/service/client-request-state.h File be/src/service/client-request-state.h: Line 195: const std::string limited_profile_str() const; Why const std::string? http://gerrit.cloudera.org:8080/#/c/7064/1/be/src/service/impala-server.h File be/src/service/impala-server.h: Line 386: const std::string& GetEffectiveUser() { Consider changing this function to std::string GetEffectiveUser(SessionState); and move it into auth-util.h/cc That way it's obvious whether the two implementations do the same thing. Line 480: /// If the user asking for this profile is the same user that run the query .. that runs the query ... Line 489: /// query profile. Otherwise, this function returns an empty exec summary. Seems better to return an auth warning string instead of an empty string. http://gerrit.cloudera.org:8080/#/c/7064/1/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: Line 362: // profile. give an example why this may be the case http://gerrit.cloudera.org:8080/#/c/7064/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: Line 489: // Check any masked requests. If the masked requests have an associated error message, Check all masked requests. If a masked request has an associated... Line 492: // These checks don't result in an Authorization exception but set the AuthorizationException Line 494: // extra line http://gerrit.cloudera.org:8080/#/c/7064/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: Line 2533: if (!enablePrivChecks_) { This new logic and naming could use a little cleanup. How about we rename enablePrivChecks_ to maskPrivChecks_ and introduce a new Analyzer.setMaskPrivChecks(String msg) and Analyzer.unsetMaskPrivChecks() (or something similar). That way when masking is enabled we always add a priv req to the masked list together with the user-provided 'msg' which controls whether a failure to auth that masked priv request leads to an AuthException (the 'msg' could be null of course). Line 2534: globalState_.maskedPrivilegeReqs.add(Pair.create(privReq, "")); I'd prefer null instead of an empty string. Seems clearer what it means. Line 2538: if (Strings.isNullOrEmpty(authErrorMsg_)) { Change this to authErrorMsg_ != null? Treating the empty string and null the same way seems error prone. -- To view, visit http://gerrit.cloudera.org:8080/7064 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2255d587367c2d328590ae8534a5406c4b0c9b15 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-HasComments: Yes
