PHOENIX-4528 PhoenixAccessController checks permissions only at table level when creating views(Karan Mehta)
Project: http://git-wip-us.apache.org/repos/asf/phoenix/repo Commit: http://git-wip-us.apache.org/repos/asf/phoenix/commit/61affd43 Tree: http://git-wip-us.apache.org/repos/asf/phoenix/tree/61affd43 Diff: http://git-wip-us.apache.org/repos/asf/phoenix/diff/61affd43 Branch: refs/heads/master Commit: 61affd431b8c4a1730804f0c0d5a0035b797e178 Parents: 16fa7f6 Author: Rajeshbabu Chintaguntla <rajeshb...@apache.org> Authored: Fri Jun 15 10:38:05 2018 -0700 Committer: Rajeshbabu Chintaguntla <rajeshb...@apache.org> Committed: Fri Jun 15 10:38:05 2018 -0700 ---------------------------------------------------------------------- .../phoenix/end2end/BasePermissionsIT.java | 4 + .../phoenix/end2end/ChangePermissionsIT.java | 26 +++++- .../coprocessor/PhoenixAccessController.java | 95 +++++++++++++------- 3 files changed, 92 insertions(+), 33 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/phoenix/blob/61affd43/phoenix-core/src/it/java/org/apache/phoenix/end2end/BasePermissionsIT.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/BasePermissionsIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/BasePermissionsIT.java index 9f91267..7698fca 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/BasePermissionsIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/BasePermissionsIT.java @@ -748,6 +748,10 @@ public class BasePermissionsIT extends BaseTest { } } + String surroundWithDoubleQuotes(String input) { + return "\"" + input + "\""; + } + void validateAccessDeniedException(AccessDeniedException ade) { String msg = ade.getMessage(); assertTrue("Exception contained unexpected message: '" + msg + "'", http://git-wip-us.apache.org/repos/asf/phoenix/blob/61affd43/phoenix-core/src/it/java/org/apache/phoenix/end2end/ChangePermissionsIT.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/ChangePermissionsIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/ChangePermissionsIT.java index 0d764d8..106438f 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/ChangePermissionsIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/ChangePermissionsIT.java @@ -23,6 +23,7 @@ import org.apache.hadoop.hbase.security.User; import org.apache.phoenix.jdbc.PhoenixDatabaseMetaData; import org.apache.phoenix.query.QueryConstants; import org.apache.phoenix.schema.TableNotFoundException; +import org.apache.phoenix.util.SchemaUtil; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -144,7 +145,7 @@ public class ChangePermissionsIT extends BasePermissionsIT { verifyAllowed(createSchema(SCHEMA_NAME), superUser1); verifyAllowed(grantPermissions("C", regularUser1, SCHEMA_NAME, true), superUser1); } else { - verifyAllowed(grantPermissions("C", regularUser1, "\"" + QueryConstants.HBASE_DEFAULT_SCHEMA_NAME + "\"", true), superUser1); + verifyAllowed(grantPermissions("C", regularUser1, surroundWithDoubleQuotes(QueryConstants.HBASE_DEFAULT_SCHEMA_NAME), true), superUser1); } // Create new table. Create indexes, views and view indexes on top of it. Verify the contents by querying it @@ -235,7 +236,7 @@ public class ChangePermissionsIT extends BasePermissionsIT { verifyAllowed(createSchema(SCHEMA_NAME), superUser1); verifyAllowed(grantPermissions("C", regularUser1, SCHEMA_NAME, true), superUser1); } else { - verifyAllowed(grantPermissions("C", regularUser1, "\"" + QueryConstants.HBASE_DEFAULT_SCHEMA_NAME + "\"", true), superUser1); + verifyAllowed(grantPermissions("C", regularUser1, surroundWithDoubleQuotes(QueryConstants.HBASE_DEFAULT_SCHEMA_NAME), true), superUser1); } // Create MultiTenant Table (View Index Table should be automatically created) @@ -266,4 +267,25 @@ public class ChangePermissionsIT extends BasePermissionsIT { verifyAllowed(readMultiTenantTableWithIndex(VIEW1_TABLE_NAME, "o1"), regularUser2); verifyAllowed(readMultiTenantTableWithoutIndex(VIEW2_TABLE_NAME, "o2"), regularUser2); } + + /** + * Grant RX permissions on the schema to regularUser1, + * Creating view on a table with that schema by regularUser1 should be allowed + */ + @Test + public void testCreateViewOnTableWithRXPermsOnSchema() throws Exception { + + startNewMiniCluster(); + grantSystemTableAccess(superUser1, regularUser1, regularUser2, regularUser3); + + if(isNamespaceMapped) { + verifyAllowed(createSchema(SCHEMA_NAME), superUser1); + verifyAllowed(createTable(FULL_TABLE_NAME), superUser1); + verifyAllowed(grantPermissions("RX", regularUser1, SCHEMA_NAME, true), superUser1); + } else { + verifyAllowed(createTable(FULL_TABLE_NAME), superUser1); + verifyAllowed(grantPermissions("RX", regularUser1, surroundWithDoubleQuotes(QueryConstants.HBASE_DEFAULT_SCHEMA_NAME), true), superUser1); + } + verifyAllowed(createView(VIEW1_TABLE_NAME, FULL_TABLE_NAME), regularUser1); + } } http://git-wip-us.apache.org/repos/asf/phoenix/blob/61affd43/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/PhoenixAccessController.java ---------------------------------------------------------------------- diff --git a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/PhoenixAccessController.java b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/PhoenixAccessController.java index b80dbe8..aa406fd 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/PhoenixAccessController.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/PhoenixAccessController.java @@ -35,6 +35,7 @@ import org.apache.hadoop.hbase.CoprocessorEnvironment; import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.NamespaceDescriptor; import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.ClusterConnection; import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; import org.apache.hadoop.hbase.client.Connection; import org.apache.hadoop.hbase.client.ConnectionFactory; @@ -46,7 +47,6 @@ import org.apache.hadoop.hbase.coprocessor.ObserverContext; import org.apache.hadoop.hbase.coprocessor.ObserverContextImpl; import org.apache.hadoop.hbase.coprocessor.RegionCoprocessor; import org.apache.hadoop.hbase.ipc.RpcServer; -import org.apache.hadoop.hbase.protobuf.ProtobufUtil; import org.apache.hadoop.hbase.protobuf.generated.AccessControlProtos; import org.apache.hadoop.hbase.protobuf.generated.AccessControlProtos.AccessControlService; import org.apache.hadoop.hbase.regionserver.RegionCoprocessorHost; @@ -59,7 +59,11 @@ import org.apache.hadoop.hbase.security.access.AuthResult; import org.apache.hadoop.hbase.security.access.Permission; import org.apache.hadoop.hbase.security.access.Permission.Action; import org.apache.hadoop.hbase.security.access.UserPermission; +import org.apache.hadoop.hbase.protobuf.ProtobufUtil; import org.apache.hadoop.hbase.util.Bytes; +import com.google.protobuf.ByteString; +import com.google.protobuf.RpcCallback; +import com.google.protobuf.RpcController; import org.apache.phoenix.coprocessor.PhoenixMetaDataCoprocessorHost.PhoenixMetaDataControllerEnvironment; import org.apache.phoenix.query.QueryServices; import org.apache.phoenix.query.QueryServicesOptions; @@ -68,7 +72,6 @@ import org.apache.phoenix.schema.PTable; import org.apache.phoenix.schema.PTableType; import org.apache.phoenix.util.MetaDataUtil; -import com.google.protobuf.RpcCallback; public class PhoenixAccessController extends BaseMetaDataEndpointObserver { @@ -173,7 +176,7 @@ public class PhoenixAccessController extends BaseMetaDataEndpointObserver { User user = getActiveUser(); List<UserPermission> permissionForUser = getPermissionForUser( - getUserPermissions(index.getNameAsString()), Bytes.toBytes(user.getShortName())); + getUserPermissions(index), Bytes.toBytes(user.getShortName())); Set<Action> requireAccess = new HashSet<>(); Set<Action> accessExists = new HashSet<>(); if (permissionForUser != null) { @@ -254,8 +257,8 @@ public class PhoenixAccessController extends BaseMetaDataEndpointObserver { @Override public Void run() throws IOException { try (Connection conn = ConnectionFactory.createConnection(env.getConfiguration())) { - List<UserPermission> userPermissions = getUserPermissions(fromTable.getNameAsString()); - List<UserPermission> permissionsOnTheTable = getUserPermissions(toTable.getNameAsString()); + List<UserPermission> userPermissions = getUserPermissions(fromTable); + List<UserPermission> permissionsOnTheTable = getUserPermissions(toTable); if (userPermissions != null) { for (UserPermission userPermission : userPermissions) { Set<Action> requireAccess = new HashSet<Action>(); @@ -403,34 +406,26 @@ public class PhoenixAccessController extends BaseMetaDataEndpointObserver { } } - private List<UserPermission> getUserPermissions(final String tableName) throws IOException { + /** + * Gets all the permissions for a given tableName for all the users + * Also, get the permissions at table's namespace level and merge all of them + * @throws IOException + */ + private List<UserPermission> getUserPermissions(final TableName tableName) throws IOException { return User.runAsLoginUser(new PrivilegedExceptionAction<List<UserPermission>>() { @Override public List<UserPermission> run() throws Exception { final List<UserPermission> userPermissions = new ArrayList<UserPermission>(); try (Connection connection = ConnectionFactory.createConnection(env.getConfiguration())) { + // Merge permissions from all accessController coprocessors loaded in memory for (MasterObserver service : accessControllers) { + // Use AccessControlClient API's if the accessController is an instance of org.apache.hadoop.hbase.security.access.AccessController if (service.getClass().getName().equals(org.apache.hadoop.hbase.security.access.AccessController.class.getName())) { - userPermissions.addAll(AccessControlClient.getUserPermissions(connection, tableName)); + userPermissions.addAll(AccessControlClient.getUserPermissions(connection, tableName.getNameAsString())); + userPermissions.addAll(AccessControlClient.getUserPermissions( + connection, AuthUtil.toGroupEntry(tableName.getNamespaceAsString()))); } else { - AccessControlProtos.GetUserPermissionsRequest.Builder builder = AccessControlProtos.GetUserPermissionsRequest - .newBuilder(); - builder.setTableName(ProtobufUtil.toProtoTableName(TableName.valueOf(tableName))); - builder.setType(AccessControlProtos.Permission.Type.Table); - AccessControlProtos.GetUserPermissionsRequest request = builder.build(); - - ((AccessControlService.Interface)service).getUserPermissions(null, request, - new RpcCallback<AccessControlProtos.GetUserPermissionsResponse>() { - @Override - public void run(AccessControlProtos.GetUserPermissionsResponse message) { - if (message != null) { - for (AccessControlProtos.UserPermission perm : message - .getUserPermissionList()) { - userPermissions.add(AccessControlUtil.toUserPermission(perm)); - } - } - } - }); + getUserPermsFromUserDefinedAccessController(userPermissions, connection, (AccessControlService.Interface) service); } } } catch (Throwable e) { @@ -443,12 +438,51 @@ public class PhoenixAccessController extends BaseMetaDataEndpointObserver { } return userPermissions; } + + private void getUserPermsFromUserDefinedAccessController(final List<UserPermission> userPermissions, Connection connection, AccessControlService.Interface service) { + + RpcController controller = (RpcController) ((ClusterConnection)connection) + .getRpcControllerFactory().newController(); + + AccessControlProtos.GetUserPermissionsRequest.Builder builderTablePerms = AccessControlProtos.GetUserPermissionsRequest + .newBuilder(); + builderTablePerms.setTableName(ProtobufUtil.toProtoTableName(tableName)); + builderTablePerms.setType(AccessControlProtos.Permission.Type.Table); + AccessControlProtos.GetUserPermissionsRequest requestTablePerms = builderTablePerms.build(); + + callGetUserPermissionsRequest(userPermissions, service, requestTablePerms, controller); + + AccessControlProtos.GetUserPermissionsRequest.Builder builderNamespacePerms = AccessControlProtos.GetUserPermissionsRequest + .newBuilder(); + builderNamespacePerms.setNamespaceName(ByteString.copyFrom(tableName.getNamespace())); + builderNamespacePerms.setType(AccessControlProtos.Permission.Type.Namespace); + AccessControlProtos.GetUserPermissionsRequest requestNamespacePerms = builderNamespacePerms.build(); + + callGetUserPermissionsRequest(userPermissions, service, requestNamespacePerms, controller); + + } + + private void callGetUserPermissionsRequest(final List<UserPermission> userPermissions, AccessControlService.Interface service + , AccessControlProtos.GetUserPermissionsRequest request, RpcController controller) { + ((AccessControlService.Interface)service).getUserPermissions(controller, request, + new RpcCallback<AccessControlProtos.GetUserPermissionsResponse>() { + @Override + public void run(AccessControlProtos.GetUserPermissionsResponse message) { + if (message != null) { + for (AccessControlProtos.UserPermission perm : message + .getUserPermissionList()) { + userPermissions.add(AccessControlUtil.toUserPermission(perm)); + } + } + } + }); + } }); } - + /** * Authorizes that the current user has all the given permissions for the - * given table + * given table and for the hbase namespace of the table * @param tableName Table requested * @throws IOException if obtaining the current user fails * @throws AccessDeniedException if user has no authorization @@ -458,7 +492,7 @@ public class PhoenixAccessController extends BaseMetaDataEndpointObserver { AuthResult result = null; List<Action> requiredAccess = new ArrayList<Action>(); for (Action permission : permissions) { - if (hasAccess(getUserPermissions(tableName.getNameAsString()), tableName, permission, user)) { + if (hasAccess(getUserPermissions(tableName), tableName, permission, user)) { result = AuthResult.allow(request, "Table permission granted", user, permission, tableName, null, null); } else { result = AuthResult.deny(request, "Insufficient permissions", user, permission, tableName, null, null); @@ -476,8 +510,7 @@ public class PhoenixAccessController extends BaseMetaDataEndpointObserver { /** * Checks if the user has access to the table for the specified action. - * - * @param perms All table permissions + * @param perms All table and table's namespace permissions * @param table tablename * @param action action for access is required * @return true if the user has access to the table for specified action, false otherwise @@ -503,7 +536,7 @@ public class PhoenixAccessController extends BaseMetaDataEndpointObserver { } } } else if (LOG.isDebugEnabled()) { - LOG.debug("No permissions found for table=" + table); + LOG.debug("No permissions found for table=" + table + " or namespace=" + table.getNamespaceAsString()); } return false; }
