Alex Behm has posted comments on this change. Change subject: IMPALA-4965: Authorize access to runtime profile and exec summary ......................................................................
Patch Set 3: (10 comments) Some ideas to discuss / think through. http://gerrit.cloudera.org:8080/#/c/7064/3/be/src/service/client-request-state.h File be/src/service/client-request-state.h: Line 151: const bool user_has_profile_access() const { single line if it fits Line 222: /// error. If base64_encoded is true, outputs the base64-encoded profile output, This API is confusing to me. Why does base64_encoded influence the auth behavior? Also an empty user will always be authorized right? http://gerrit.cloudera.org:8080/#/c/7064/3/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 606: return request_state->GetRuntimeProfile(user, base64_encoded, output); We not have many different GetRuntimeProfile*() and GetExecSummary() functions. I'm wondering if we can do the auth check more directly here, for example, with a function Status CheckProfileAccess(user); The function could be implemented in both ClientRequestState and the QueryLogRecord. Apart from that, most of the existing code would not have to change. Another alternative along these lines is to have two CheckProfileAccess() functions in auth.h. One version would take a ClientRequestState and a user, the other would take a QueryLogRecord and the user. Happy to discuss the idea before you try it. http://gerrit.cloudera.org:8080/#/c/7064/3/be/src/service/impala-server.h File be/src/service/impala-server.h: Line 560: std::string limited_profile_str; Remove? http://gerrit.cloudera.org:8080/#/c/7064/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: Line 2535: public void registerPrivReq(PrivilegeRequest privReq) { As discussed, I think this logic can be further simplified and made more explicit. http://gerrit.cloudera.org:8080/#/c/7064/1/tests/authorization/test_authorization.py File tests/authorization/test_authorization.py: Line 92: def test_access_runtime_profile(self): Nice test! Line 135: execute_statement_req.statement = """create view if not exists tpch.customer_view as Can we put this into a different db? Might be good to make sure this view is dropped even if there is a failure. Line 137: execute_statement_resp = self.hs2_client.ExecuteStatement(execute_statement_req) also test that EXPLAIN works Line 145: # Repeat as a deleted user delegated Line 281: """Runs statement 'stmt' and retrieves the runtime profile and exec summary. If remove 'statement' -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
