Skip to content

Spark client can create/alter/drop a view #1391

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.

Sign up for GitHub

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

Merged
vihangk1 merged 5 commits into from
Sep 2, 2020
Merged

Spark client can create/alter/drop a view #1391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4010958
Spark client can create/alter/drop a view
saihemanth-cloudera Aug 11, 2020
bcc17a0
Size of input hive privilege objects changed from 1 to 2 in checkSing…
saihemanth-cloudera Aug 12, 2020
76490a4
Non deferred views don't have to go through ranger authorization
saihemanth-cloudera Aug 14, 2020
eb79970
Added javadoc for deferred view. Constant value for string 'Authorized'
saihemanth-cloudera Aug 31, 2020
1b971f7
Rerun CI checks
saihemanth-cloudera Aug 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 49 additions & 2 deletions ...rc/java/org/apache/hadoop/hive/ql/security/authorization/command/CommandAuthorizerV2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@
import java.util.Set;
import java.util.Map.Entry;

import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hive.conf.HiveConf;
import org.apache.hadoop.hive.metastore.HiveMetaStore;
import org.apache.hadoop.hive.metastore.TableType;
import org.apache.hadoop.hive.metastore.api.Database;
import org.apache.hadoop.hive.ql.exec.FunctionInfo;
import org.apache.hadoop.hive.ql.exec.FunctionUtils;
Expand Down Expand Up @@ -104,8 +108,25 @@ private static List<HivePrivilegeObject> getHivePrivObjects(List<? extends Entit
continue;
}
if (privObject instanceof ReadEntity && !((ReadEntity)privObject).isDirect()) {
// This ReadEntity represents one of the underlying tables/views of a view, so skip it.
continue;
// This ReadEntity represents one of the underlying tables/views of a view, skip it if
// it's not inside a deferred authorized view.
ReadEntity reTable = (ReadEntity)privObject;
Boolean isDeferred = false;
if( reTable.getParents() != null && reTable.getParents().size() > 0){
for( ReadEntity re: reTable.getParents()){
if (re.getTyp() == Type.TABLE && re.getTable() != null ) {
Table t = re.getTable();
if(!isDeferredAuthView(t)){
continue;
}else{
isDeferred = true;
}
}
}
}
if(!isDeferred){
continue;
}
}
if (privObject instanceof WriteEntity && ((WriteEntity)privObject).isTempURI()) {
// do not authorize temporary uris
Expand All @@ -121,6 +142,32 @@ private static List<HivePrivilegeObject> getHivePrivObjects(List<? extends Entit
return hivePrivobjs;
}

/**
* A deferred authorization view is view created by non-super user like spark-user. This view contains a parameter "Authorized"
* set to false, so ranger will not authorize it during view creation. When a select statement is issued, then the ranger authorizes
* the under lying tables.
* @param Table t
* @return boolean value
*/
private static boolean isDeferredAuthView(Table t){
String tableType = t.getTTable().getTableType();
String authorizedKeyword = "Authorized";
Copy link
Copy Markdown
Contributor

@vihangk1 vihangk1 Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant was having a constant defined in HiveMetastoreAuthorizer.java like below.
public static final String DEFERRED_AUTHORIZED_KEY = "Authorized";

Once you do that you can use the key DEFERRED_AUTHORIZED_KEY everywhere whereever you are directly looking for "Authorized" key. The advantage of doing this way is that code is less error-prone and maintainable.

  1. Future code modifications do introduce uppercase or lower case issues. (e.g. using "authorized" v/s "Authorized")
  2. Its easy to look for all the places where deferred authorized key is being used in the IDE by looking for the usages of the constant. Currently, we will have to do a git grep "Authorized" which is inconvenient.

boolean isView = false;
if (TableType.MATERIALIZED_VIEW.name().equals(tableType) || TableType.VIRTUAL_VIEW.name().equals(tableType)) {
isView = true;
}
if(isView){
Map<String, String> params = t.getParameters();
if (params != null && params.containsKey(authorizedKeyword)) {
String authorizedValue = params.get(authorizedKeyword);
if ("false".equalsIgnoreCase(authorizedValue)) {
return true;
}
}
}
return false;
}

private static void addHivePrivObject(Entity privObject, Map<String, List<String>> tableName2Cols,
List<HivePrivilegeObject> hivePrivObjs) {
HivePrivilegeObjectType privObjType = AuthorizationUtils.getHivePrivilegeObjectType(privObject.getType());
Expand Down
13 changes: 10 additions & 3 deletions ...pache/hadoop/hive/ql/security/authorization/plugin/metastore/HiveMetaStoreAuthorizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;

/**
* HiveMetaStoreAuthorizer : Do authorization checks on MetaStore Events in MetaStorePreEventListener
Expand Down Expand Up @@ -380,19 +381,25 @@ HiveMetaStoreAuthzInfo buildAuthzContext(PreEventContext preEventContext) throws
case CREATE_TABLE:
authzEvent = new CreateTableEvent(preEventContext);
if (isViewOperation(preEventContext) && (!isSuperUser(getCurrentUser(authzEvent)))) {
throw new MetaException(getErrorMessage("CREATE_VIEW", getCurrentUser(authzEvent)));
//we allow view to be created, but mark it as having not been authorized
PreCreateTableEvent pcte = (PreCreateTableEvent)preEventContext;
Map<String, String> params = pcte.getTable().getParameters();
params.put("Authorized", "false");
}
break;
case ALTER_TABLE:
authzEvent = new AlterTableEvent(preEventContext);
if (isViewOperation(preEventContext) && (!isSuperUser(getCurrentUser(authzEvent)))) {
throw new MetaException(getErrorMessage("ALTER_VIEW", getCurrentUser(authzEvent)));
//we allow view to be altered, but mark it as having not been authorized
PreAlterTableEvent pcte = (PreAlterTableEvent)preEventContext;
Map<String, String> params = pcte.getNewTable().getParameters();
params.put("Authorized", "false");
}
break;
case DROP_TABLE:
authzEvent = new DropTableEvent(preEventContext);
if (isViewOperation(preEventContext) && (!isSuperUser(getCurrentUser(authzEvent)))) {
throw new MetaException(getErrorMessage("DROP_VIEW", getCurrentUser(authzEvent)));
//TODO: do we need to check Authorized flag?
}
break;
case ADD_PARTITION:
Expand Down
35 changes: 32 additions & 3 deletions ...e/hadoop/hive/ql/security/authorization/plugin/metastore/TestHiveMetaStoreAuthorizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,12 @@
import org.junit.runners.MethodSorters;
import org.junit.Before;
import org.junit.Test;
import java.util.Map;

import java.io.File;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

/*
Test whether HiveAuthorizer for MetaStore operation is trigger and HiveMetaStoreAuthzInfo is created by HiveMetaStoreAuthorizer
Expand Down Expand Up @@ -138,10 +140,37 @@ public void testC_CreateView_anyUser() throws Exception {
.setOwner(authorizedUser)
.build(conf);
hmsHandler.create_table(viewObj);
Map<String, String> params = viewObj.getParameters();
assertTrue(params.containsKey("Authorized"));
assertTrue("false".equalsIgnoreCase(params.get("Authorized")));
} catch (Exception e) {
String err = e.getMessage();
String expected = "Operation type CREATE_VIEW not allowed for user:" + authorizedUser;
assertEquals(expected, err);
// no Exceptions for user same as normal user is now allowed CREATE_VIEW operation
Copy link
Copy Markdown
Contributor

@vihangk1 vihangk1 Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The catch all exception here is not good, since the test will pass in case there is a MetaException thrown on line 142. The test added will pass without code modifications in the HiveMetastoreAuthorizer as well and hence I feel is not really a good regression test.

Also, looks like there are other tests in this class which do a catch all exception blocks which can give false positive (eg. testD_CreateView_SuperUser). Would be good to fix them up as well.

}
}

@Test
public void testC2_AlterView_anyUser() throws Exception{
UserGroupInformation.setLoginUser(UserGroupInformation.createRemoteUser(authorizedUser));
try {
Table viewObj = new TableBuilder()
.setTableName(viewName)
.setType(TableType.VIRTUAL_VIEW.name())
.addCol("name", ColumnType.STRING_TYPE_NAME)
.setOwner(authorizedUser)
.build(conf);
hmsHandler.create_table(viewObj);
viewObj = new TableBuilder()
.setTableName(viewName)
.setType(TableType.VIRTUAL_VIEW.name())
.addCol("dep", ColumnType.STRING_TYPE_NAME)
.setOwner(authorizedUser)
.build(conf);
hmsHandler.alter_table("default", viewName, viewObj);
Map<String, String> params = viewObj.getParameters();
assertTrue(params.containsKey("Authorized"));
assertTrue("false".equalsIgnoreCase(params.get("Authorized")));
} catch (Exception e) {
Copy link
Copy Markdown
Contributor

@vihangk1 vihangk1 Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above related to exception handling.

// no Exceptions for user same as normal user is now allowed Alter_VIEW operation
}
}

Expand Down