Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary ......................................................................
Patch Set 1: (12 comments) 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? Removed. 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? Removed 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 Done Line 480: /// If the user asking for this profile is the same user that run the query > .. that runs the query ... Done 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. Both this and the GetRuntimeProfileStr now return an error code if user is not authorized to access the profile. 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 Done 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... Done Line 492: // These checks don't result in an Authorization exception but set the > AuthorizationException Done Line 494: // > extra line Done 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 e Done Line 2534: globalState_.maskedPrivilegeReqs.add(Pair.create(privReq, "")); > I'd prefer null instead of an empty string. Seems clearer what it means. Done Line 2538: if (Strings.isNullOrEmpty(authErrorMsg_)) { > Change this to authErrorMsg_ != null? Treating the empty string and null th Done -- 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-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-HasComments: Yes
