Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201706022213.v52MDTU9008932@ip-10-146-233-104.ec2.internal>
Date: Fri, 2 Jun 2017 22:13:29 +0000
From: "Alex Behm (Code Review)" <gerrit@cloudera.org>
To: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Reply-To: alex.behm@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4965=3A_Authorize_access_to_runtime_profile_and_exec_summary=0A?=
In-Reply-To: <gerrit.1496430187000.I2255d587367c2d328590ae8534a5406c4b0c9b15@gerrit.cloudera.org>
References: <gerrit.1496430187000.I2255d587367c2d328590ae8534a5406c4b0c9b15@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Fri, 02 Jun 2017 22:13:35 -0000

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 <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-HasComments: Yes