impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4965: Authorize access to runtime profile and exec summary
Date Fri, 02 Jun 2017 22:13:29 GMT
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

Mime
View raw message