[CDAP-20852] changing cdap-security-spi to support return response

cdap-app-fabric/src/main/java/io/cdap/cdap/app/guice/AuthorizationModule.java

@@ -42,6 +42,8 @@
 import io.cdap.cdap.security.authorization.AuthorizationContextFactory;
 import io.cdap.cdap.security.authorization.DefaultAuthorizationContext;
 import io.cdap.cdap.security.authorization.DelegatingPermissionManager;
+import io.cdap.cdap.security.authorization.DelegatingRoleController;
+import io.cdap.cdap.security.authorization.RoleController;
 import io.cdap.cdap.security.spi.authorization.AccessController;
 import io.cdap.cdap.security.spi.authorization.AuthorizationContext;
 import io.cdap.cdap.security.spi.authorization.PermissionManager;
@@ -53,14 +55,13 @@
  * {@link PrivateModule} for authorization classes. This module is necessary and must be in
  * app-fabric because classes like {@link DatasetFramework}, {@link DynamicDatasetCache}, {@link
  * DefaultAdmin} are not available in cdap-security.
- *
  * This module is part of the injector created in StandaloneMain and MasterServiceMain, which makes
  * it available to services. The requirements for this module are: 1. This module is used for
  * creating and exposing {@link AccessControllerInstantiator}. 2. The {@link
  * AccessControllerInstantiator} needs a {@link DefaultAuthorizationContext}. 3. The {@link
  * DefaultAuthorizationContext} needs a {@link DatasetContext}, a {@link Admin} and a {@link
  * Transactional}.
- *
+ * -
  * These requirements are fulfilled by: 1. Constructing a {@link Singleton} {@link
  * MultiThreadDatasetCache} by injecting a {@link DatasetFramework}, a {@link
  * TransactionSystemClient} and a {@link MetricsCollectionService}. This {@link
@@ -99,6 +100,9 @@ protected void configure() {
 
     bind(PermissionManager.class).to(DelegatingPermissionManager.class);
     expose(PermissionManager.class);
+
+    bind(RoleController.class).to(DelegatingRoleController.class);
+    expose(RoleController.class);
   }
 
   @Singleton

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/AuthorizationHandler.java

@@ -33,18 +33,19 @@
 import io.cdap.cdap.proto.codec.EntityIdTypeAdapter;
 import io.cdap.cdap.proto.id.EntityId;
 import io.cdap.cdap.proto.security.Action;
+import io.cdap.cdap.proto.security.Authorizable;
 import io.cdap.cdap.proto.security.AuthorizationRequest;
 import io.cdap.cdap.proto.security.GrantRequest;
+import io.cdap.cdap.proto.security.GrantedPermission;
 import io.cdap.cdap.proto.security.Permission;
 import io.cdap.cdap.proto.security.PermissionAdapterFactory;
 import io.cdap.cdap.proto.security.Principal;
 import io.cdap.cdap.proto.security.Privilege;
 import io.cdap.cdap.proto.security.RevokeRequest;
 import io.cdap.cdap.proto.security.Role;
-import io.cdap.cdap.security.authorization.AccessControllerInstantiator;
+import io.cdap.cdap.security.authorization.RoleController;
 import io.cdap.cdap.security.spi.authentication.AuthenticationContext;
 import io.cdap.cdap.security.spi.authentication.SecurityRequestContext;
-import io.cdap.cdap.security.spi.authorization.AccessController;
 import io.cdap.cdap.security.spi.authorization.PermissionManager;
 import io.cdap.http.HttpResponder;
 import io.netty.handler.codec.http.FullHttpRequest;
@@ -67,7 +68,7 @@
 import org.slf4j.LoggerFactory;
 
 /**
- * Exposes {@link AccessController} operations via HTTP.
+ * Exposes {@link RoleController} operations via HTTP.
  */
 @Path(Constants.Gateway.API_VERSION_3 + "/security/authorization")
 public class AuthorizationHandler extends AbstractAppFabricHttpHandler {
@@ -83,20 +84,29 @@ public class AuthorizationHandler extends AbstractAppFabricHttpHandler {
   private final boolean authenticationEnabled;
   private final boolean authorizationEnabled;
   private final PermissionManager permissionManager;
-  private final AccessController accessController;
+  private final RoleController roleController;
   private final AuthenticationContext authenticationContext;
 
   @Inject
   AuthorizationHandler(PermissionManager permissionManager,
-      AccessControllerInstantiator accessControllerInstantiator,
-      CConfiguration cConf, AuthenticationContext authenticationContext) {
+      CConfiguration cConf, AuthenticationContext authenticationContext, RoleController roleController) {
     this.permissionManager = permissionManager;
-    this.accessController = accessControllerInstantiator.get();
+    this.roleController = roleController;
     this.authenticationContext = authenticationContext;
     this.authenticationEnabled = cConf.getBoolean(Constants.Security.ENABLED);
     this.authorizationEnabled = cConf.getBoolean(Constants.Security.Authorization.ENABLED);
   }
 
+  /**
+   * Grants the given permissions of the {@link Authorizable} for the given {@link Principal} in the request body.
+   *
+   * @param httpRequest
+   * @param httpResponder
+   * @throws BadRequestException
+   * @throws FeatureDisabledException
+   * @throws UnknownHostException
+   * @throws AccessException
+   */
   @Path("/privileges/grant")
   @POST
   @AuditPolicy(AuditDetail.REQUEST_BODY)
@@ -117,6 +127,17 @@ public void grant(FullHttpRequest httpRequest, HttpResponder httpResponder)
     createLogEntry(httpRequest, HttpResponseStatus.OK);
   }
 
+  /**
+   * Removes the permissions and privileges for the given {@link Authorizable} , optionally with specific
+   * {@link Principal} and {@link Permission}.
+   *
+   * @param httpRequest
+   * @param httpResponder
+   * @throws FeatureDisabledException
+   * @throws BadRequestException
+   * @throws UnknownHostException
+   * @throws AccessException
+   */
   @Path("/privileges/revoke")
   @POST
   @AuditPolicy(AuditDetail.REQUEST_BODY)
@@ -142,6 +163,15 @@ public void revoke(FullHttpRequest httpRequest, HttpResponder httpResponder)
     createLogEntry(httpRequest, HttpResponseStatus.OK);
   }
 
+  /**
+   * Lists all the {@link GrantedPermission} for the specified principalType and principalName.
+   *
+   * @param httpRequest
+   * @param httpResponder
+   * @param principalType
+   * @param principalName
+   * @throws Exception
+   */
   @Path("{principal-type}/{principal-name}/privileges")
   @GET
   public void listPrivileges(HttpRequest httpRequest, HttpResponder httpResponder,
@@ -151,7 +181,7 @@ public void listPrivileges(HttpRequest httpRequest, HttpResponder httpResponder,
     Principal principal = new Principal(principalName,
         Principal.PrincipalType.valueOf(principalType.toUpperCase()));
     httpResponder.sendJson(HttpResponseStatus.OK,
-        GSON.toJson(accessController.listGrants(principal), PRIVILEGE_SET_TYPE));
+        GSON.toJson(permissionManager.listGrants(principal), PRIVILEGE_SET_TYPE));
     createLogEntry(httpRequest, HttpResponseStatus.OK);
   }
 
@@ -160,34 +190,66 @@ public void listPrivileges(HttpRequest httpRequest, HttpResponder httpResponder,
    * Role Management : For Role Based Access Control
    ********************************************************************************************************************/
 
+  /**
+   * Creates a role.
+   *
+   * @param httpRequest
+   * @param httpResponder
+   * @param roleName
+   * @throws Exception
+   */
   @Path("/roles/{role-name}")
   @PUT
   public void createRole(HttpRequest httpRequest, HttpResponder httpResponder,
       @PathParam("role-name") String roleName) throws Exception {
     ensureSecurityEnabled();
-    accessController.createRole(new Role(roleName));
+    roleController.createRole(new Role(roleName));
     httpResponder.sendStatus(HttpResponseStatus.OK);
     createLogEntry(httpRequest, HttpResponseStatus.OK);
   }
 
+  /**
+   * Deletes a role.
+   *
+   * @param httpRequest
+   * @param httpResponder
+   * @param roleName
+   * @throws Exception
+   */
   @Path("/roles/{role-name}")
   @DELETE
   public void dropRole(HttpRequest httpRequest, HttpResponder httpResponder,
       @PathParam("role-name") String roleName) throws Exception {
     ensureSecurityEnabled();
-    accessController.dropRole(new Role(roleName));
+    roleController.dropRole(new Role(roleName));
     httpResponder.sendStatus(HttpResponseStatus.OK);
     createLogEntry(httpRequest, HttpResponseStatus.OK);
   }
 
+  /**
+   * Returns all available {@link Role roles}.
+   *
+   * @param httpRequest
+   * @param httpResponder
+   * @throws Exception
+   */
   @Path("/roles")
   @GET
   public void listAllRoles(HttpRequest httpRequest, HttpResponder httpResponder) throws Exception {
     ensureSecurityEnabled();
-    httpResponder.sendJson(HttpResponseStatus.OK, GSON.toJson(accessController.listAllRoles()));
+    httpResponder.sendJson(HttpResponseStatus.OK, GSON.toJson(roleController.listAllRoles()));
     createLogEntry(httpRequest, HttpResponseStatus.OK);
   }
 
+  /**
+   * Returns a set of all {@link Role roles} for the specified principalType and principalName.
+   *
+   * @param httpRequest
+   * @param httpResponder
+   * @param principalType
+   * @param principalName
+   * @throws Exception
+   */
   @Path("{principal-type}/{principal-name}/roles")
   @GET
   public void listRoles(HttpRequest httpRequest, HttpResponder httpResponder,
@@ -197,10 +259,20 @@ public void listRoles(HttpRequest httpRequest, HttpResponder httpResponder,
     Principal principal = new Principal(principalName,
         Principal.PrincipalType.valueOf(principalType.toUpperCase()));
     httpResponder.sendJson(HttpResponseStatus.OK,
-        GSON.toJson(accessController.listRoles(principal)));
+        GSON.toJson(roleController.listRoles(principal)));
     createLogEntry(httpRequest, HttpResponseStatus.OK);
   }
 
+  /**
+   * Adds a role to the specified principalType and principalName.
+   *
+   * @param httpRequest
+   * @param httpResponder
+   * @param principalType
+   * @param principalName
+   * @param roleName
+   * @throws Exception
+   */
   @Path("/{principal-type}/{principal-name}/roles/{role-name}")
   @PUT
   public void addRoleToPrincipal(HttpRequest httpRequest, HttpResponder httpResponder,
@@ -210,11 +282,21 @@ public void addRoleToPrincipal(HttpRequest httpRequest, HttpResponder httpRespon
     ensureSecurityEnabled();
     Principal principal = new Principal(principalName,
         Principal.PrincipalType.valueOf(principalType.toUpperCase()));
-    accessController.addRoleToPrincipal(new Role(roleName), principal);
+    roleController.addRoleToPrincipal(new Role(roleName), principal);
     httpResponder.sendStatus(HttpResponseStatus.OK);
     createLogEntry(httpRequest, HttpResponseStatus.OK);
   }
 
+  /**
+   * Deletes a role from the specified principalType and principalName.
+   *
+   * @param httpRequest
+   * @param httpResponder
+   * @param principalType
+   * @param principalName
+   * @param roleName
+   * @throws Exception
+   */
   @Path("/{principal-type}/{principal-name}/roles/{role-name}")
   @DELETE
   public void removeRoleFromPrincipal(HttpRequest httpRequest, HttpResponder httpResponder,
@@ -224,7 +306,7 @@ public void removeRoleFromPrincipal(HttpRequest httpRequest, HttpResponder httpR
     ensureSecurityEnabled();
     Principal principal = new Principal(principalName,
         Principal.PrincipalType.valueOf(principalType.toUpperCase()));
-    accessController.removeRoleFromPrincipal(new Role(roleName), principal);
+    roleController.removeRoleFromPrincipal(new Role(roleName), principal);
     httpResponder.sendStatus(HttpResponseStatus.OK);
     createLogEntry(httpRequest, HttpResponseStatus.OK);
   }
@@ -244,7 +326,7 @@ private void ensureSecurityEnabled() throws FeatureDisabledException {
   private void createLogEntry(HttpRequest httpRequest, HttpResponseStatus responseStatus)
       throws UnknownHostException {
     InetAddress clientAddr = InetAddress.getByName(
-        Objects.firstNonNull(SecurityRequestContext.getUserIP(), "0.0.0.0"));
+        Objects.firstNonNull(SecurityRequestContext.getUserIp(), "0.0.0.0"));
     AuditLogEntry logEntry = new AuditLogEntry(httpRequest, clientAddr.getHostAddress());
     logEntry.setUserName(authenticationContext.getPrincipal().getName());
     logEntry.setResponse(responseStatus.code(), 0L);

cdap-app-fabric/src/test/java/io/cdap/cdap/gateway/handlers/AuthorizationHandlerTest.java

@@ -39,11 +39,11 @@
 import io.cdap.cdap.proto.security.Role;
 import io.cdap.cdap.proto.security.StandardPermission;
 import io.cdap.cdap.security.auth.context.MasterAuthenticationContext;
-import io.cdap.cdap.security.authorization.AccessControllerInstantiator;
 import io.cdap.cdap.security.authorization.AuthorizationContextFactory;
-import io.cdap.cdap.security.authorization.InMemoryAccessController;
+//import io.cdap.cdap.security.authorization.InMemoryAccessController;
+import io.cdap.cdap.security.authorization.InMemoryPermissionManager;
+import io.cdap.cdap.security.authorization.InMemoryRoleController;
 import io.cdap.cdap.security.authorization.NoOpAuthorizationContextFactory;
-import io.cdap.cdap.security.spi.authorization.AccessController;
 import io.cdap.cdap.security.spi.authorization.AlreadyExistsException;
 import io.cdap.http.ChannelPipelineModifier;
 import io.cdap.http.NettyHttpService;
@@ -82,15 +82,11 @@ public void setUp() throws Exception {
     conf.setBoolean(Constants.Security.Authorization.ENABLED, true);
     conf.setBoolean(Constants.Security.ENABLED, true);
     properties.setProperty("superusers", admin.getName());
-    final InMemoryAccessController auth = new InMemoryAccessController();
-    auth.initialize(FACTORY.create(properties));
+    final InMemoryPermissionManager auth = new InMemoryPermissionManager();
+    final InMemoryRoleController inMemoryRoleController = new InMemoryRoleController();
+    //    auth.initialize(FACTORY.create(properties)); //Will be used on migration to SPI implementation
     service = new CommonNettyHttpServiceBuilder(conf, getClass().getSimpleName(), new NoOpMetricsCollectionService())
-      .setHttpHandlers(new AuthorizationHandler(auth, new AccessControllerInstantiator(conf, FACTORY) {
-        @Override
-        public AccessController get() {
-          return auth;
-        }
-      }, conf, new MasterAuthenticationContext()))
+      .setHttpHandlers(new AuthorizationHandler(auth, conf, new MasterAuthenticationContext(), inMemoryRoleController))
       .setChannelPipelineModifier(new ChannelPipelineModifier() {
         @Override
         public void modify(ChannelPipeline pipeline) {
@@ -135,16 +131,12 @@ public void testAuthorizationDisabled() throws Exception {
 
   private void testDisabled(CConfiguration cConf, FeatureDisabledException.Feature feature,
                             String configSetting) throws Exception {
-    final InMemoryAccessController accessController = new InMemoryAccessController();
+    final InMemoryPermissionManager accessController = new InMemoryPermissionManager();
+    final InMemoryRoleController inMemoryRoleController = new InMemoryRoleController();
     NettyHttpService service = new CommonNettyHttpServiceBuilder(cConf, getClass().getSimpleName(),
                                                                  new NoOpMetricsCollectionService())
       .setHttpHandlers(new AuthorizationHandler(
-        accessController, new AccessControllerInstantiator(cConf, FACTORY) {
-        @Override
-        public AccessController get() {
-          return accessController;
-        }
-      }, cConf, new MasterAuthenticationContext()))
+        accessController, cConf, new MasterAuthenticationContext(), inMemoryRoleController))
       .build();
     service.start();
     try {
@@ -277,7 +269,7 @@ public void testRevokeEntity() throws Exception {
   }
 
   @Test
-  public void testRBAC() throws Exception {
+  public void testRbac() throws Exception {
     Role admins = new Role("admins");
     Role engineers = new Role("engineers");
     // create a role