-
Notifications
You must be signed in to change notification settings - Fork 4.8k
HIVE-29628: Incorrect objectName in PARTITION HivePrivilegeObject for… #6508
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -149,6 +149,13 @@ | |
| } | ||
| } | ||
|
|
||
| if ((privObject.getTyp() == Type.PARTITION || privObject.getTyp() == Type.DUMMYPARTITION) | ||
| && privObject instanceof ReadEntity | ||
|
Check warning on line 153 in ql/src/java/org/apache/hadoop/hive/ql/security/authorization/command/CommandAuthorizerV2.java
|
||
| && isPartitionAccessedViaRegularView((ReadEntity) privObject, privObjects)) { | ||
| // skip Partition Entity auth for regular view | ||
| continue; | ||
| } | ||
|
|
||
| addHivePrivObject(privObject, tableName2Cols, hivePrivobjs, hiveOpType); | ||
| } | ||
| return hivePrivobjs; | ||
|
|
@@ -180,6 +187,79 @@ | |
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Returns true when a PARTITION entity should not produce its own privilege object | ||
| * because access is already covered by a view's TABLE_OR_VIEW object. | ||
| */ | ||
| private static boolean isPartitionAccessedViaRegularView(ReadEntity partitionEntity, | ||
|
Check failure on line 194 in ql/src/java/org/apache/hadoop/hive/ql/security/authorization/command/CommandAuthorizerV2.java
|
||
| List<? extends Entity> allEntities) { | ||
| if (hasDeferredViewParent(partitionEntity)) { | ||
| return false; | ||
| } | ||
| if (hasRegularViewParent(partitionEntity)) { | ||
| return true; | ||
| } | ||
| Table partTable = partitionEntity.getTable(); | ||
| if (partTable == null) { | ||
| return false; | ||
| } | ||
| for (Entity entity : allEntities) { | ||
|
Check warning on line 206 in ql/src/java/org/apache/hadoop/hive/ql/security/authorization/command/CommandAuthorizerV2.java
|
||
| if (!(entity instanceof ReadEntity) || entity.getTyp() != Type.TABLE) { | ||
| continue; | ||
| } | ||
| ReadEntity tableEntity = (ReadEntity) entity; | ||
| if (tableEntity.isDirect() || tableEntity.getTable() == null) { | ||
| continue; | ||
| } | ||
| Table table = tableEntity.getTable(); | ||
| if (!partTable.getDbName().equals(table.getDbName()) | ||
| || !partTable.getTableName().equals(table.getTableName())) { | ||
| continue; | ||
| } | ||
| if (hasDeferredViewParent(tableEntity)) { | ||
| return false; | ||
| } | ||
| if (hasRegularViewParent(tableEntity)) { | ||
| return true; | ||
| } | ||
| } | ||
|
Comment on lines
+206
to
+225
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't quite understand this logic. Why do we need to check all the entites for a given partition object. This potentially lead to O(N^2) for huge partitioned table creating a bottleneck during compile phase (because authorization happens here)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also this whole method needs a refactor to simplify it as there are too many |
||
| return false; | ||
| } | ||
|
|
||
| private static boolean hasDeferredViewParent(ReadEntity entity) { | ||
| Set<ReadEntity> parents = entity.getParents(); | ||
| if (parents == null || parents.isEmpty()) { | ||
| return false; | ||
| } | ||
| for (ReadEntity parent : parents) { | ||
| if (parent.getTyp() == Type.TABLE && parent.getTable() != null | ||
| && isDeferredAuthView(parent.getTable())) { | ||
| return true; | ||
| } | ||
| } | ||
|
Comment on lines
+234
to
+239
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can also lead to O(N^2). same with hasRegularViewParent() also. |
||
| return false; | ||
| } | ||
|
|
||
| private static boolean hasRegularViewParent(ReadEntity entity) { | ||
| Set<ReadEntity> parents = entity.getParents(); | ||
| if (parents == null || parents.isEmpty()) { | ||
| return false; | ||
| } | ||
| for (ReadEntity parent : parents) { | ||
| if (parent.getTyp() == Type.TABLE && parent.getTable() != null | ||
| && isView(parent.getTable()) && !isDeferredAuthView(parent.getTable())) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| private static boolean isView(Table t) { | ||
| String tableType = t.getTTable().getTableType(); | ||
| return TableType.MATERIALIZED_VIEW.name().equals(tableType) | ||
| || TableType.VIRTUAL_VIEW.name().equals(tableType); | ||
| } | ||
|
|
||
| private static void addHivePrivObject(Entity privObject, Map<String, List<String>> tableName2Cols, | ||
| List<HivePrivilegeObject> hivePrivObjs, HiveOperationType hiveOpType) throws HiveException { | ||
| HivePrivilegeObjectType privObjType = AuthorizationUtils.getHivePrivilegeObjectType(privObject.getType()); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,189 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.hadoop.hive.ql.security.authorization.plugin; | ||
|
|
||
| import org.apache.hadoop.hive.conf.HiveConf; | ||
| import org.apache.hadoop.hive.conf.HiveConf.ConfVars; | ||
| import org.apache.hadoop.hive.conf.HiveConfForTest; | ||
| import org.apache.hadoop.hive.metastore.utils.TestTxnDbUtil; | ||
| import org.apache.hadoop.hive.ql.Driver; | ||
| import org.apache.hadoop.hive.ql.lockmgr.DbTxnManager; | ||
| import org.apache.hadoop.hive.ql.security.HiveAuthenticationProvider; | ||
| import org.apache.hadoop.hive.ql.session.SessionState; | ||
| import org.apache.hadoop.security.UserGroupInformation; | ||
| import org.junit.AfterClass; | ||
| import org.junit.Assert; | ||
| import org.junit.Before; | ||
| import org.junit.BeforeClass; | ||
| import org.junit.Test; | ||
| import org.mockito.ArgumentCaptor; | ||
| import org.mockito.Mockito; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| import static org.mockito.ArgumentMatchers.any; | ||
| import static org.mockito.Mockito.atLeastOnce; | ||
| import static org.mockito.Mockito.reset; | ||
| import static org.mockito.Mockito.verify; | ||
|
|
||
| /** | ||
| * Tests the {@link HivePrivilegeObject} inputs passed to {@link HiveAuthorizer#checkPrivileges} | ||
| * for view queries over partitioned base tables (HIVE-29628). | ||
| */ | ||
| public class TestViewPartitionPrivilegeObjects { | ||
|
|
||
| static final String DATA_DB = "datadb"; | ||
| static final String VIEW_DB = "viewdb"; | ||
| static final String BASE_TABLE = "t1"; | ||
| static final String VIEW_NAME = "v1"; | ||
|
|
||
| protected static HiveConf conf; | ||
| protected static Driver driver; | ||
| static HiveAuthorizer mockedAuthorizer; | ||
|
|
||
| static class MockedHiveAuthorizerFactory implements HiveAuthorizerFactory { | ||
| @Override | ||
| public HiveAuthorizer createHiveAuthorizer(HiveMetastoreClientFactory metastoreClientFactory, | ||
| HiveConf conf, HiveAuthenticationProvider authenticator, HiveAuthzSessionContext ctx) { | ||
| TestViewPartitionPrivilegeObjects.mockedAuthorizer = Mockito.mock(HiveAuthorizer.class); | ||
| return TestViewPartitionPrivilegeObjects.mockedAuthorizer; | ||
| } | ||
| } | ||
|
|
||
| @BeforeClass | ||
| public static void beforeClass() throws Exception { | ||
| UserGroupInformation.setLoginUser(UserGroupInformation.createRemoteUser("hive")); | ||
| conf = new HiveConfForTest(TestViewPartitionPrivilegeObjects.class); | ||
| conf.setVar(ConfVars.HIVE_AUTHORIZATION_MANAGER, MockedHiveAuthorizerFactory.class.getName()); | ||
| conf.setBoolVar(ConfVars.HIVE_AUTHORIZATION_ENABLED, true); | ||
| conf.setBoolVar(ConfVars.HIVE_SERVER2_ENABLE_DOAS, false); | ||
| conf.setBoolVar(ConfVars.HIVE_SUPPORT_CONCURRENCY, true); | ||
| conf.setVar(ConfVars.HIVE_TXN_MANAGER, DbTxnManager.class.getName()); | ||
| conf.setVar(ConfVars.HIVE_MAPRED_MODE, "nonstrict"); | ||
|
Check warning on line 78 in ql/src/test/org/apache/hadoop/hive/ql/security/authorization/plugin/TestViewPartitionPrivilegeObjects.java
|
||
| conf.setVar(ConfVars.DYNAMIC_PARTITIONING_MODE, "nonstrict"); | ||
| conf.setVar(ConfVars.HIVE_FETCH_TASK_CONVERSION, "none"); | ||
| conf.setVar(ConfVars.HIVE_EXECUTION_ENGINE, "mr"); | ||
|
|
||
| TestTxnDbUtil.prepDb(conf); | ||
| SessionState.start(conf); | ||
| driver = new Driver(conf); | ||
|
|
||
| runCmd("CREATE DATABASE IF NOT EXISTS " + DATA_DB); | ||
| runCmd("CREATE TABLE IF NOT EXISTS " + DATA_DB + "." + BASE_TABLE | ||
| + " (i INT) PARTITIONED BY (dept STRING)"); | ||
| runCmd("ALTER TABLE " + DATA_DB + "." + BASE_TABLE + " ADD IF NOT EXISTS PARTITION (dept='a')"); | ||
| runCmd("CREATE DATABASE IF NOT EXISTS " + VIEW_DB); | ||
| runCmd("CREATE VIEW IF NOT EXISTS " + VIEW_DB + "." + VIEW_NAME | ||
| + " AS SELECT * FROM " + DATA_DB + "." + BASE_TABLE); | ||
| } | ||
|
|
||
| @Before | ||
| public void resetMock() { | ||
| if (mockedAuthorizer != null) { | ||
| reset(mockedAuthorizer); | ||
| } | ||
| } | ||
|
|
||
| @AfterClass | ||
| public static void afterClass() throws Exception { | ||
| runCmd("DROP VIEW IF EXISTS " + VIEW_DB + "." + VIEW_NAME); | ||
| runCmd("DROP TABLE IF EXISTS " + DATA_DB + "." + BASE_TABLE); | ||
| runCmd("DROP DATABASE IF EXISTS " + VIEW_DB); | ||
| runCmd("DROP DATABASE IF EXISTS " + DATA_DB); | ||
| driver.close(); | ||
| } | ||
|
|
||
| /** | ||
| * Mirrors {@code authorization_view_without_base_select_priv.q} with | ||
| * {@code hive.fetch.task.conversion=none}: a view-only user must not produce a | ||
| * PARTITION privilege object on the underlying base table. | ||
| */ | ||
| @Test | ||
| public void testViewSelectNoBaseTablePartitionPrivObj() throws Exception { | ||
| conf.setVar(ConfVars.HIVE_FETCH_TASK_CONVERSION, "none"); | ||
| SessionState.get().setConf(conf); | ||
|
|
||
| HiveAuthenticationProvider user1Auth = Mockito.mock(HiveAuthenticationProvider.class); | ||
| Mockito.when(user1Auth.getUserName()).thenReturn("user1"); | ||
| SessionState.get().setAuthenticator(user1Auth); | ||
|
|
||
| driver.compile("SELECT * FROM " + VIEW_DB + "." + VIEW_NAME, true); | ||
|
|
||
| List<HivePrivilegeObject> inputs = getInputPrivObjects(); | ||
|
|
||
| Assert.assertTrue("Expected a TABLE_OR_VIEW object for the view", | ||
| inputs.stream().anyMatch(h -> | ||
| h.getType() == HivePrivilegeObject.HivePrivilegeObjectType.TABLE_OR_VIEW | ||
| && VIEW_NAME.equalsIgnoreCase(h.getObjectName()) | ||
| && VIEW_DB.equalsIgnoreCase(h.getDbname()))); | ||
|
|
||
| Assert.assertFalse("View query must not send a PARTITION object on the base table", | ||
| inputs.stream().anyMatch(h -> | ||
| h.getType() == HivePrivilegeObject.HivePrivilegeObjectType.PARTITION | ||
| && BASE_TABLE.equalsIgnoreCase(h.getObjectName()) | ||
| && DATA_DB.equalsIgnoreCase(h.getDbname()))); | ||
|
|
||
| Assert.assertFalse("View query must not send a base-table TABLE_OR_VIEW object", | ||
| inputs.stream().anyMatch(h -> | ||
| h.getType() == HivePrivilegeObject.HivePrivilegeObjectType.TABLE_OR_VIEW | ||
| && BASE_TABLE.equalsIgnoreCase(h.getObjectName()) | ||
| && DATA_DB.equalsIgnoreCase(h.getDbname()))); | ||
| } | ||
|
|
||
| /** | ||
| * Direct reads on a partitioned table must still emit a PARTITION privilege object | ||
| * so table/partition policies (e.g. Ranger) can be enforced. | ||
| */ | ||
| @Test | ||
| public void testDirectTableSelectHasPartitionPrivObj() throws Exception { | ||
| conf.setVar(ConfVars.HIVE_FETCH_TASK_CONVERSION, "none"); | ||
| SessionState.get().setConf(conf); | ||
|
|
||
| driver.compile("SELECT * FROM " + DATA_DB + "." + BASE_TABLE, true); | ||
|
|
||
| List<HivePrivilegeObject> inputs = getInputPrivObjects(); | ||
|
|
||
| Assert.assertTrue("Expected a PARTITION privilege object for direct table access", | ||
| inputs.stream().anyMatch(h -> | ||
| h.getType() == HivePrivilegeObject.HivePrivilegeObjectType.PARTITION | ||
| && BASE_TABLE.equalsIgnoreCase(h.getObjectName()) | ||
| && DATA_DB.equalsIgnoreCase(h.getDbname()))); | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need tests for the sibling logic (Skip logic #2 from the PR description). |
||
| @SuppressWarnings("unchecked") | ||
| private List<HivePrivilegeObject> getInputPrivObjects() | ||
| throws HiveAuthzPluginException, HiveAccessControlException { | ||
| Class<List<HivePrivilegeObject>> cls = (Class) List.class; | ||
| ArgumentCaptor<List<HivePrivilegeObject>> inputsCapturer = ArgumentCaptor.forClass(cls); | ||
| ArgumentCaptor<List<HivePrivilegeObject>> outputsCapturer = ArgumentCaptor.forClass(cls); | ||
|
|
||
| verify(mockedAuthorizer, atLeastOnce()).checkPrivileges( | ||
| any(HiveOperationType.class), | ||
| inputsCapturer.capture(), | ||
| outputsCapturer.capture(), | ||
| any(HiveAuthzContext.class)); | ||
|
|
||
| List<List<HivePrivilegeObject>> all = inputsCapturer.getAllValues(); | ||
| return all.get(all.size() - 1); | ||
| } | ||
|
|
||
| private static void runCmd(String cmd) throws Exception { | ||
| driver.run(cmd); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method is specific enough that the if condition can be completely checked in the method and we can just simplify this to
so that it's easier to read. What do you think?