HBASE-20185 Fix ACL check for MasterRpcServices#execProcedure

hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java

@@ -57,6 +57,7 @@ import org.apache.hadoop.hbase.io.hfile.HFile;
 import org.apache.hadoop.hbase.ipc.CoprocessorRpcUtils;
 import org.apache.hadoop.hbase.ipc.PriorityFunction;
 import org.apache.hadoop.hbase.ipc.QosPriority;
+import org.apache.hadoop.hbase.ipc.RpcServer;
 import org.apache.hadoop.hbase.ipc.RpcServer.BlockingServiceAndInterface;
 import org.apache.hadoop.hbase.ipc.RpcServerFactory;
 import org.apache.hadoop.hbase.ipc.RpcServerInterface;
@@ -831,8 +832,8 @@ public class MasterRpcServices extends RSRpcServices
   @Override
   public ExecProcedureResponse execProcedure(RpcController controller,
       ExecProcedureRequest request) throws ServiceException {
-    rpcPreCheck("execProcedure");
     try {
+      master.checkInitialized();
       ProcedureDescription desc = request.getProcedure();
       MasterProcedureManager mpm = master.getMasterProcedureManagerHost().getProcedureManager(
         desc.getSignature());
@@ -841,6 +842,7 @@ public class MasterRpcServices extends RSRpcServices
           + desc.getSignature()));
       }
       LOG.info(master.getClientIdAuditPrefix() + " procedure request for: " + desc.getSignature());
+      mpm.checkPermissions(desc, accessChecker, RpcServer.getRequestUser().orElse(null));
       mpm.execProcedure(desc);
       // send back the max amount of time the client should wait for the procedure
       // to complete

hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java

@@ -63,6 +63,7 @@ import org.apache.hadoop.hbase.procedure.ZKProcedureCoordinator;
 import org.apache.hadoop.hbase.procedure2.ProcedureExecutor;
 import org.apache.hadoop.hbase.security.AccessDeniedException;
 import org.apache.hadoop.hbase.security.User;
+import org.apache.hadoop.hbase.security.access.AccessChecker;
 import org.apache.hadoop.hbase.snapshot.ClientSnapshotDescriptionUtils;
 import org.apache.hadoop.hbase.snapshot.HBaseSnapshotException;
 import org.apache.hadoop.hbase.snapshot.RestoreSnapshotException;
@@ -1149,6 +1150,13 @@ public class SnapshotManager extends MasterProcedureManager implements Stoppable
     takeSnapshot(toSnapshotDescription(desc));
   }
 
+  @Override
+  public void checkPermissions(ProcedureDescription desc, AccessChecker accessChecker, User user)
+      throws IOException {
+    // Done by AccessController as part of preSnapshot coprocessor hook (legacy code path).
+    // In future, when we AC is removed for good, that check should be moved here.
+  }
+
   @Override
   public boolean isProcedureDone(ProcedureDescription desc) throws IOException {
     return isSnapshotDone(toSnapshotDescription(desc));

hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/MasterProcedureManager.java

@@ -19,6 +19,8 @@ package org.apache.hadoop.hbase.procedure;
 
 import java.io.IOException;
 
+import org.apache.hadoop.hbase.security.User;
+import org.apache.hadoop.hbase.security.access.AccessChecker;
 import org.apache.yetus.audience.InterfaceAudience;
 import org.apache.yetus.audience.InterfaceStability;
 import org.apache.hadoop.hbase.Stoppable;
@@ -34,28 +36,26 @@ import org.apache.zookeeper.KeeperException;
 *
 * To implement a custom globally barriered procedure, user needs to extend two classes:
 * {@link MasterProcedureManager} and {@link RegionServerProcedureManager}. Implementation of
-* {@link MasterProcedureManager} is loaded into {@link org.apache.hadoop.hbase.master.HMaster} 
+* {@link MasterProcedureManager} is loaded into {@link org.apache.hadoop.hbase.master.HMaster}
 * process via configuration parameter 'hbase.procedure.master.classes', while implementation of
-* {@link RegionServerProcedureManager} is loaded into 
+* {@link RegionServerProcedureManager} is loaded into
 * {@link org.apache.hadoop.hbase.regionserver.HRegionServer} process via
 * configuration parameter 'hbase.procedure.regionserver.classes'.
 *
-* An example of globally barriered procedure implementation is 
+* An example of globally barriered procedure implementation is
 * {@link org.apache.hadoop.hbase.master.snapshot.SnapshotManager} and
 * {@link org.apache.hadoop.hbase.regionserver.snapshot.RegionServerSnapshotManager}.
 *
 * A globally barriered procedure is identified by its signature (usually it is the name of the
 * procedure znode). During the initialization phase, the initialize methods are called by both
-* {@link org.apache.hadoop.hbase.master.HMaster} 
+* {@link org.apache.hadoop.hbase.master.HMaster}
-* and {@link org.apache.hadoop.hbase.regionserver.HRegionServer} which create the procedure znode 
+* and {@link org.apache.hadoop.hbase.regionserver.HRegionServer} which create the procedure znode
-* and register the listeners. A procedure can be triggered by its signature and an instant name 
+* and register the listeners. A procedure can be triggered by its signature and an instant name
-* (encapsulated in a {@link ProcedureDescription} object). When the servers are shutdown, 
+* (encapsulated in a {@link ProcedureDescription} object). When the servers are shutdown,
 * the stop methods on both classes are called to clean up the data associated with the procedure.
 */
 @InterfaceAudience.Private
-@InterfaceStability.Evolving
+public abstract class MasterProcedureManager extends ProcedureManager implements Stoppable {
-public abstract class MasterProcedureManager extends ProcedureManager implements
-    Stoppable {
   /**
    * Initialize a globally barriered procedure for master.
    *
@@ -73,9 +73,7 @@ public abstract class MasterProcedureManager extends ProcedureManager implements
    * @param desc Procedure description
    * @throws IOException
    */
-  public void execProcedure(ProcedureDescription desc) throws IOException {
+  public void execProcedure(ProcedureDescription desc) throws IOException {}
-
-  }
 
   /**
    * Execute a distributed procedure on cluster with return data.
@@ -89,6 +87,13 @@ public abstract class MasterProcedureManager extends ProcedureManager implements
     return null;
   }
 
+  /**
+   * Check for required permissions before executing the procedure.
+   * @throws IOException if permissions requirements are not met.
+   */
+  public abstract void checkPermissions(ProcedureDescription desc, AccessChecker accessChecker,
+      User user) throws IOException;
+
   /**
    * Check if the procedure is finished successfully
    *

hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/flush/MasterFlushTableProcedureManager.java

@@ -41,6 +41,9 @@ import org.apache.hadoop.hbase.procedure.Procedure;
 import org.apache.hadoop.hbase.procedure.ProcedureCoordinator;
 import org.apache.hadoop.hbase.procedure.ProcedureCoordinatorRpcs;
 import org.apache.hadoop.hbase.procedure.ZKProcedureCoordinator;
+import org.apache.hadoop.hbase.security.User;
+import org.apache.hadoop.hbase.security.access.AccessChecker;
+import org.apache.hadoop.hbase.security.access.Permission;
 import org.apache.hadoop.hbase.util.Pair;
 import org.apache.hadoop.hbase.zookeeper.MetaTableLocator;
 import org.apache.yetus.audience.InterfaceAudience;
@@ -181,6 +184,13 @@ public class MasterFlushTableProcedureManager extends MasterProcedureManager {
     monitor.rethrowException();
   }
 
+  @Override
+  public void checkPermissions(ProcedureDescription desc, AccessChecker accessChecker, User user)
+      throws IOException {
+    // Done by AccessController as part of preTableFlush coprocessor hook (legacy code path).
+    // In future, when we AC is removed for good, that check should be moved here.
+  }
+
   @Override
   public synchronized boolean isProcedureDone(ProcedureDescription desc) throws IOException {
     // Procedure instance name is the table name.

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java

@@ -331,7 +331,7 @@ public class RSRpcServices implements HBaseRPCErrorHandler,
   // Initialized in start() since AccessChecker needs ZKWatcher which is created by HRegionServer
   // after RSRpcServices constructor and before start() is called.
   // Initialized only if authorization is enabled, else remains null.
-  private AccessChecker accessChecker;
+  protected AccessChecker accessChecker;
 
   /**
    * Services launched in RSRpcServices. By default they are on but you can use the below

hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java

@@ -1136,6 +1136,7 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor,
   public void preSnapshot(final ObserverContext<MasterCoprocessorEnvironment> ctx,
       final SnapshotDescription snapshot, final TableDescriptor hTableDescriptor)
       throws IOException {
+    // Move this ACL check to SnapshotManager#checkPermissions as part of AC deprecation.
     requirePermission(ctx, "snapshot " + snapshot.getName(),
         hTableDescriptor.getTableName(), null, null, Permission.Action.ADMIN);
   }
@@ -1266,6 +1267,8 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor,
   @Override
   public void preTableFlush(final ObserverContext<MasterCoprocessorEnvironment> ctx,
       final TableName tableName) throws IOException {
+    // Move this ACL check to MasterFlushTableProcedureManager#checkPermissions as part of AC
+    // deprecation.
     requirePermission(ctx, "flushTable", tableName,
         null, null, Action.ADMIN, Action.CREATE);
   }

hbase-server/src/test/java/org/apache/hadoop/hbase/procedure/SimpleMasterProcedureManager.java

@@ -29,6 +29,8 @@ import org.apache.hadoop.hbase.errorhandling.ForeignExceptionDispatcher;
 import org.apache.hadoop.hbase.master.MasterServices;
 import org.apache.hadoop.hbase.master.MetricsMaster;
 import org.apache.hadoop.hbase.shaded.protobuf.generated.HBaseProtos.ProcedureDescription;
+import org.apache.hadoop.hbase.security.User;
+import org.apache.hadoop.hbase.security.access.AccessChecker;
 import org.apache.zookeeper.KeeperException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -113,6 +115,10 @@ public class SimpleMasterProcedureManager extends MasterProcedureManager {
     return returnData.values().iterator().next();
   }
 
+  @Override
+  public void checkPermissions(ProcedureDescription desc, AccessChecker accessChecker, User user)
+      throws IOException {}
+
   @Override
   public boolean isProcedureDone(ProcedureDescription desc) throws IOException {
     return done;