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 9: (7 comments) I have addressed Quanlong's comments on the patch set 8. Let me know if there are additional suggestions. Thanks! http://gerrit.cloudera.org:8080/#/c/23569/8/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java File fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java: http://gerrit.cloudera.org:8080/#/c/23569/8/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java@273 PS8, Line 273: * 4. Privilege level is neither SELECT nor INSERT. > nit: need to update this as well Done http://gerrit.cloudera.org:8080/#/c/23569/8/fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java File fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java: http://gerrit.cloudera.org:8080/#/c/23569/8/fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java@31 PS8, Line 31: public class AuthorizableColumn extends AuthorizableTable { > Most of the code in this class is identical to AuthorizableTable. Can we le Thanks Quanlong! Indeed, the code in AuthorizableColumn and AuthorizableTable is almost the same. I will make AuthorizableColumn inherit AuthorizableTable and just override getType(), getColumnName(), and getName(). getName() has to be overridden because it should return the fully-qualified column name of a column. http://gerrit.cloudera.org:8080/#/c/23569/8/fe/src/main/java/org/apache/impala/authorization/AuthorizableColumn.java@79 PS8, Line 79: : > nit: can be in one line Done http://gerrit.cloudera.org:8080/#/c/23569/8/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/8/fe/src/main/java/org/apache/impala/authorization/BaseAuthorizationChecker.java@53 PS8, Line 53: ALLOWED_HIER_AUTHZ_TABLE > I was confused why REFRESH, ALTER, DROP are not here. Then I realized this Thanks Quanlong! In the next patch, I will change this to ALLOWED_HIER_AUTHZ_TABLE_PRIVILEGES. http://gerrit.cloudera.org:8080/#/c/23569/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/23569/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@731 PS7, Line 731: // rowFilterResult.isRowFilterEnabled() will return false and thus the requesting Yes. A test case added by https://gerrit.cloudera.org/c/17230/ (IMPALA-10554: Block updates when row-filter/column-mask is enabled for the user) in AuthorizationStmtTest#testUpdateOnMaskedTables() covers this. In this test case, the requesting user is granted the ALL privilege on the SERVER but still should not be able to insert values into the table 'functional.alltypes' since there is a row filtering policy defined on the table 'functional.alltypes' against the requesting user. insert into functional.alltypes partition(year, month) select * from functional.alltypestiny Note that given such an INSERT statement, after this patch, Impala's authorization checker would check the following 2 items during authorization (v.s. only the first one). The requesting user would be allowed to execute the INSERT statement as long as the user could insert values into each target column involved in the query, even though the user could not insert values into the entire table. - Whether the requesting user is allowed to insert values into the entire table. - Whether the requesting user is allowed to insert values into each target column. Without us temporarily changing the type of resource from column to table, rowFilterResult.isRowFilterEnabled() at https://gerrit.cloudera.org/c/23569/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java#738 would evaluate to false when authorizable.getType() is Type.COLUMN, and thus the privilege request for inserting values into the corresponding column would not be denied. I will add more code comment in the next patch because the current code comment is not clear enough. > Should we set the owner here? I also tried to see if it really matters. I found that the result of rowFilterResult.isRowFilterEnabled() would be the same whether we provide the owner of the table. But I think it does not hurt to set the owner of the table here. http://gerrit.cloudera.org:8080/#/c/23569/7/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/23569/7/tests/authorization/test_ranger.py@1489 PS7, Line 1489: admin_client.execute("drop database if exists {0} cascade" > Just realized even inserting to columns not used in the masking policies is Ack http://gerrit.cloudera.org:8080/#/c/23569/8/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/23569/8/tests/authorization/test_ranger.py@1522 PS8, Line 1522: tbl, columns[0] > nit: might be more readable to use a variable like 'tbl', e.g. change L1519 Thanks for the suggestion Quanlong! I will revise this test and _test_deny_insert_into_column_partitioned_table() accordingly. -- 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: 9 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, 23 Dec 2025 22:47:31 +0000 Gerrit-HasComments: Yes
