Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/23569 )
Change subject: IMPALA-14507: Register column-level privilege requests for INSERT ...................................................................... Patch Set 14: (4 comments) I have replied to reviewers' comments and will prepare a follow-up patch set accordingly. http://gerrit.cloudera.org:8080/#/c/23569/14/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/23569/14/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java@156 PS14, Line 156: // The table-level SELECT, INSERT, or CREATE must be the first table-level : // request, and it must precede all column-level privilege requests. : // We allow the CREATE privilege because in CreateTableAsSelectStmt#analyze(), : // we call InsertStmt#analyze(), which in turn registers column-level INSERT : // privilege requests. : // We allow the ALL privilege because for the UPSERT operation against Kudu : // tables, we set the required privilege to ALL since we don't have an UPSERT : // privilege yet. Refer to InsertStmt#analyzeTargetTable(). : Preconditions.checkState((requests.isEmpty() || : !(privReq.getAuthorizable().getType() == Authorizable.Type.COLUMN)) || : (requests.get(0).getAuthorizable().getType() == Authorizable.Type.TABLE && : ALLOWED_HIER_AUTHZ_TABLE_PRIVILEGES.contains( : requests.get(0).getPrivilege()))); > Is this really what we want to check here? Thanks for pointing this out Csaba! I don't think this is really what we want to check here. Based on the code comment before this patch, I think we wanted to check the following. However, it looks like we actually allow the cases when there is only one column-level privilege request, or when there is one column-level privilege request followed by a table-level privilege request as you pointed out. // The table-level SELECT must be the first table-level request, and it // must precede all column-level privilege requests. I am wondering maybe we could change the precondition to the following. !requests.isEmpty() || (privReq.getAuthorizable().getType() == Authorizable.Type.TABLE && ALLOWED_HIER_AUTHZ_TABLE_PRIVILEGES.contains(privReq.getPrivilege())) That is, if 'requests' is empty (meaning that the current 'privReq' is the first privilege request associated with 'tableName'), we should guarantee that a) the type of Authorizable of 'privReq' is TABLE, and b) 'privReq' should be one in {SELECT, INSERT, CREATE, ALL}. I hope I did not miss something important above. I will try the revised condition above in the next patch and will trigger a pre-commit run to make sure this does not cause problems. I will also revise the code comment accordingly. On a related note, it looks like this complicated condition was originally added by https://gerrit.cloudera.org/c/732/8/fe/src/main/java/com/cloudera/impala/analysis/AnalysisContext.java#396 for CDH-23206: Impala support for column-level authorization (part 2), which is not even associated with an Apache Impala JIRA number. On another related note, the code comment above was actually suggested by Alex Behm at https://gerrit.cloudera.org/c/732/4/fe/src/main/java/com/cloudera/impala/analysis/AnalysisContext.java#394. http://gerrit.cloudera.org:8080/#/c/23569/14/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java@191 PS14, Line 191: Type.COLUMN > nit: Authorizable.Type.COLUMN would be more consistent I will address this comment in the next patch. http://gerrit.cloudera.org:8080/#/c/23569/14/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java File fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java: http://gerrit.cloudera.org:8080/#/c/23569/14/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java@268 PS14, Line 268: "insert into functional.alltypes (id, string_col) partition " + : "(year, month) values (1, \"abc\", 2026, 12) > nit: we can declare 'insertQuery' outside and use it here as well. Thanks Quanlong! I will address this in the next patch. http://gerrit.cloudera.org:8080/#/c/23569/14/fe/src/test/java/org/apache/impala/authorization/ranger/RangerAuditLogTest.java@511 PS14, Line 511: // It's possible that we still have a policy with exactly the same resource : // specified by ('databaseName', 'tableName', 'columnName') e Thanks Csaba! I just realize that the code comment below is not true when 'columnName' is not "*", the wildcard character denoting all the columns of a table. // It's possible that we still have a policy with exactly the same resource // specified by ('databaseName', 'tableName', 'columnName') even though // there is no user or group associated with the policy. Specifically, when we are using CDP Ranger 2.4.0.7.3.1.500-182, we are not able to create a policy on a column that is not associated with any principals, e.g., groups, or users (either via the GRANT statements or the REST API calls on the command line). So it looks like the scenario is impossible as you pointed out. > If this is not a normal scenario, then I think that we should return an error > in this case. On the other hand, before this test case is run, if there is already a policy on the same column, i.e., functional.alltypes.id, associated with principals other than 'user_.getShortName()', then createRangerPolicy() would fail. I guess we could consider this case as an abnormal case and just return the error (v.s. deleting the matching policy with respect to ('databaseName', 'tableName', 'columnName') to make the test case proceed). I will revise the patch accordingly in the next patch. -- To view, visit http://gerrit.cloudera.org:8080/23569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2ef61801d3b394c56702b193c250492a62b111df Gerrit-Change-Number: 23569 Gerrit-PatchSet: 14 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Tue, 06 Jan 2026 22:57:43 +0000 Gerrit-HasComments: Yes
