Skip to content

Commit

Permalink
Clean up final usage of the old permission model & remove obsolete cl…
Browse files Browse the repository at this point in the history 
…asses that were used internally during the migration
  • Loading branch information
@tivv
tivv committed Jun 14, 2021
1 parent f4e95c5 commit 133c91f
Show file tree
Hide file tree
Showing 21 changed files with 56 additions and 1,003 deletions.
21 changes: 7 additions & 14 deletions cdap-app-fabric/src/main/java/io/cdap/cdap/app/guice/AuthorizationModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,11 @@
import io.cdap.cdap.proto.id.NamespaceId;
import io.cdap.cdap.security.authorization.AccessControllerInstantiator;
import io.cdap.cdap.security.authorization.AuthorizationContextFactory;
import io.cdap.cdap.security.authorization.AuthorizerInstantiator;
import io.cdap.cdap.security.authorization.DefaultAuthorizationContext;
import io.cdap.cdap.security.authorization.DelegatingPermissionManager;
import io.cdap.cdap.security.authorization.DelegatingPrivilegeManager;
import io.cdap.cdap.security.spi.authorization.AccessController;
import io.cdap.cdap.security.spi.authorization.AuthorizationContext;
import io.cdap.cdap.security.spi.authorization.Authorizer;
import io.cdap.cdap.security.spi.authorization.PermissionManager;
import io.cdap.cdap.security.spi.authorization.PrivilegesManager;
import org.apache.tephra.TransactionContext;
import org.apache.tephra.TransactionSystemClient;

Expand All @@ -59,16 +56,16 @@
*
* 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 AuthorizerInstantiator}.
* 2. The {@link AuthorizerInstantiator} needs a {@link DefaultAuthorizationContext}.
* 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 MultiThreadDatasetCache} is
* created for datasets in the {@link NamespaceId#SYSTEM}, since the datasets that {@link Authorizer} extensions need
* are created in the system namespace.
* created for datasets in the {@link NamespaceId#SYSTEM}, since the datasets that {@link AccessController} extensions
* need are created in the system namespace.
* 2. Binding the {@link DatasetContext} to the {@link MultiThreadDatasetCache} created above.
* 3. Using the {@link MultiThreadDatasetCache} to create a {@link TransactionContext} for providing the
* {@link Transactional}.
Expand All @@ -77,8 +74,8 @@
* 5. Using the bound {@link DatasetContext}, {@link Admin} and {@link Transactional} to provide the injections for
* {@link DefaultAuthorizationContext}, which is provided using a {@link Guice} {@link FactoryModuleBuilder} to
* construct a {@link AuthorizationContextFactory}.
* 6. Only exposing a {@link Singleton} binding to {@link AuthorizerInstantiator} from this module. The
* {@link AuthorizerInstantiator} can just {@link Inject} the {@link AuthorizationContextFactory} and call
* 6. Only exposing a {@link Singleton} binding to {@link AccessControllerInstantiator} from this module. The
* {@link AccessControllerInstantiator} can just {@link Inject} the {@link AuthorizationContextFactory} and call
* {@link AuthorizationContextFactory#create(Properties)} using an {@link Assisted} {@link Properties} object.
*/
public class AuthorizationModule extends PrivateModule {
Expand All @@ -95,13 +92,9 @@ protected void configure() {
.build(AuthorizationContextFactory.class)
);

bind(AuthorizerInstantiator.class).in(Scopes.SINGLETON);
expose(AuthorizerInstantiator.class);
bind(AccessControllerInstantiator.class).in(Scopes.SINGLETON);
expose(AccessControllerInstantiator.class);

bind(PrivilegesManager.class).to(DelegatingPrivilegeManager.class);
expose(PrivilegesManager.class);
bind(PermissionManager.class).to(DelegatingPermissionManager.class);
expose(PermissionManager.class);
}
Expand Down
12 changes: 6 additions & 6 deletions cdap-app-fabric/src/main/java/io/cdap/cdap/app/preview/PreviewRunnerModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
import io.cdap.cdap.security.impersonation.OwnerStore;
import io.cdap.cdap.security.impersonation.UGIProvider;
import io.cdap.cdap.security.spi.authorization.AccessEnforcer;
import io.cdap.cdap.security.spi.authorization.PrivilegesManager;
import io.cdap.cdap.security.spi.authorization.PermissionManager;
import io.cdap.cdap.store.DefaultOwnerStore;

/**
Expand All @@ -86,7 +86,7 @@ public class PreviewRunnerModule extends PrivateModule {
private final ArtifactStore artifactStore;
private final AccessControllerInstantiator accessControllerInstantiator;
private final AccessEnforcer accessEnforcer;
private final PrivilegesManager privilegesManager;
private final PermissionManager permissionManager;
private final PreferencesService preferencesService;
private final ProgramRuntimeProviderLoader programRuntimeProviderLoader;
private final ArtifactRepositoryReaderProvider artifactRepositoryReaderProvider;
Expand All @@ -98,7 +98,7 @@ public class PreviewRunnerModule extends PrivateModule {
PreviewRunnerModule(ArtifactRepositoryReaderProvider readerProvider, ArtifactStore artifactStore,
AccessControllerInstantiator accessControllerInstantiator,
AccessEnforcer accessEnforcer,
PrivilegesManager privilegesManager, PreferencesService preferencesService,
PermissionManager permissionManager, PreferencesService preferencesService,
ProgramRuntimeProviderLoader programRuntimeProviderLoader,
PluginFinderProvider pluginFinderProvider,
PreferencesFetcherProvider preferencesFetcherProvider,
Expand All @@ -107,7 +107,7 @@ public class PreviewRunnerModule extends PrivateModule {
this.artifactStore = artifactStore;
this.accessControllerInstantiator = accessControllerInstantiator;
this.accessEnforcer = accessEnforcer;
this.privilegesManager = privilegesManager;
this.permissionManager = permissionManager;
this.preferencesService = preferencesService;
this.programRuntimeProviderLoader = programRuntimeProviderLoader;
this.pluginFinderProvider = pluginFinderProvider;
Expand Down Expand Up @@ -141,8 +141,8 @@ protected void configure() {
expose(AccessEnforcer.class);
bind(AccessControllerInstantiator.class).toInstance(accessControllerInstantiator);
expose(AccessControllerInstantiator.class);
bind(PrivilegesManager.class).toInstance(privilegesManager);
expose(PrivilegesManager.class);
bind(PermissionManager.class).toInstance(permissionManager);
expose(PermissionManager.class);
bind(PreferencesService.class).toInstance(preferencesService);
// bind explore client to mock.
bind(ExploreClient.class).to(MockExploreClient.class);
Expand Down
63 changes: 21 additions & 42 deletions ...-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/meta/RemotePrivilegesHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,16 @@
import io.cdap.cdap.common.internal.remote.MethodArgument;
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.AuthorizationPrivilege;
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.StandardPermission;
import io.cdap.cdap.proto.security.VisibilityRequest;
import io.cdap.cdap.security.spi.authorization.AccessEnforcer;
import io.cdap.cdap.security.spi.authorization.AuthorizationEnforcer;
import io.cdap.cdap.security.spi.authorization.PrivilegesManager;
import io.cdap.cdap.security.spi.authorization.PermissionManager;
import io.cdap.http.HttpResponder;
import io.netty.handler.codec.http.FullHttpRequest;
import io.netty.handler.codec.http.HttpResponseStatus;
Expand All @@ -43,8 +42,6 @@
import java.nio.charset.StandardCharsets;
import java.util.Iterator;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import javax.inject.Inject;
import javax.ws.rs.POST;
import javax.ws.rs.Path;
Expand All @@ -56,21 +53,18 @@
@Path(AbstractRemoteSystemOpsHandler.VERSION + "/execute")
public class RemotePrivilegesHandler extends AbstractRemoteSystemOpsHandler {
private static final Logger LOG = LoggerFactory.getLogger(RemotePrivilegesHandler.class);
private static final Type SET_OF_ACTIONS = new TypeLiteral<Set<Action>>() { }.getType();
private static final Type SET_OF_PERMISSIONS = new TypeLiteral<Set<? extends Permission>>() { }.getType();
private static final Gson GSON = new GsonBuilder()
.registerTypeAdapter(EntityId.class, new EntityIdTypeAdapter())
.registerTypeAdapterFactory(new PermissionAdapterFactory())
.create();

private final PrivilegesManager privilegesManager;
private final AuthorizationEnforcer authorizationEnforcer;
private final PermissionManager permissionManager;
private final AccessEnforcer accessEnforcer;

@Inject
RemotePrivilegesHandler(PrivilegesManager privilegesManager, AuthorizationEnforcer authorizationEnforcer,
AccessEnforcer accessEnforcer) {
this.privilegesManager = privilegesManager;
this.authorizationEnforcer = authorizationEnforcer;
RemotePrivilegesHandler(PermissionManager permissionManager, AccessEnforcer accessEnforcer) {
this.permissionManager = permissionManager;
this.accessEnforcer = accessEnforcer;
}

Expand All @@ -81,11 +75,6 @@ public void enforce(FullHttpRequest request, HttpResponder responder) throws Exc
AuthorizationPrivilege.class);
LOG.debug("Enforcing for {}", authorizationPrivilege);
Set<Permission> permissions = authorizationPrivilege.getPermissions();
if (authorizationPrivilege.getActions() != null) {
permissions = Stream.of(permissions, authorizationPrivilege.getActions())
.flatMap(set -> set.stream())
.collect(Collectors.toSet());
}
if (authorizationPrivilege.getChildEntityType() != null) {
//It's expected that we'll always have one, but let's handle generic case
for (Permission permission: permissions) {
Expand All @@ -99,16 +88,6 @@ public void enforce(FullHttpRequest request, HttpResponder responder) throws Exc
responder.sendStatus(HttpResponseStatus.OK);
}

@POST
@Path("/isSingleVisible")
public void isSingleVisible(FullHttpRequest request, HttpResponder responder) throws Exception {
AuthorizationPrivilege authorizationPrivilege = GSON.fromJson(request.content().toString(StandardCharsets.UTF_8),
AuthorizationPrivilege.class);
LOG.debug("Enforcing visibility for {}", authorizationPrivilege);
authorizationEnforcer.isVisible(authorizationPrivilege.getEntity(), authorizationPrivilege.getPrincipal());
responder.sendStatus(HttpResponseStatus.OK);
}

@POST
@Path("/isVisible")
public void isVisible(FullHttpRequest request, HttpResponder responder) throws Exception {
Expand All @@ -117,7 +96,7 @@ public void isVisible(FullHttpRequest request, HttpResponder responder) throws E
Principal principal = visibilityRequest.getPrincipal();
Set<EntityId> entityIds = visibilityRequest.getEntityIds();
LOG.trace("Checking visibility for principal {} on entities {}", principal, entityIds);
Set<? extends EntityId> visiableEntities = authorizationEnforcer.isVisible(entityIds, principal);
Set<? extends EntityId> visiableEntities = accessEnforcer.isVisible(entityIds, principal);
LOG.debug("Returning entities visible for principal {} as {}", principal, visiableEntities);
responder.sendJson(HttpResponseStatus.OK, GSON.toJson(visiableEntities));
}
Expand All @@ -127,10 +106,10 @@ public void isVisible(FullHttpRequest request, HttpResponder responder) throws E
public void listPrivileges(FullHttpRequest request, HttpResponder responder) throws Exception {
Iterator<MethodArgument> arguments = parseArguments(request);
Principal principal = deserializeNext(arguments);
LOG.trace("Listing privileges for principal {}", principal);
Set<Privilege> privileges = privilegesManager.listPrivileges(principal);
LOG.debug("Returning privileges for principal {} as {}", principal, privileges);
responder.sendJson(HttpResponseStatus.OK, GSON.toJson(privileges));
LOG.trace("Listing grantedPermissions for principal {}", principal);
Set<GrantedPermission> grantedPermissions = permissionManager.listGrants(principal);
LOG.debug("Returning grantedPermissions for principal {} as {}", principal, grantedPermissions);
responder.sendJson(HttpResponseStatus.OK, GSON.toJson(grantedPermissions));
}

@POST
Expand All @@ -139,10 +118,10 @@ public void grant(FullHttpRequest request, HttpResponder responder) throws Excep
Iterator<MethodArgument> arguments = parseArguments(request);
EntityId entityId = deserializeNext(arguments);
Principal principal = deserializeNext(arguments);
Set<Action> actions = deserializeNext(arguments, SET_OF_ACTIONS);
LOG.trace("Granting {} on {} to {}", actions, entityId, principal);
privilegesManager.grant(Authorizable.fromEntityId(entityId), principal, actions);
LOG.info("Granted {} on {} to {} successfully", actions, entityId, principal);
Set<? extends Permission> permissions = deserializeNext(arguments, SET_OF_PERMISSIONS);
LOG.trace("Granting {} on {} to {}", permissions, entityId, principal);
permissionManager.grant(Authorizable.fromEntityId(entityId), principal, permissions);
LOG.info("Granted {} on {} to {} successfully", permissions, entityId, principal);
responder.sendStatus(HttpResponseStatus.OK);
}

Expand All @@ -152,10 +131,10 @@ public void revoke(FullHttpRequest request, HttpResponder responder) throws Exce
Iterator<MethodArgument> arguments = parseArguments(request);
EntityId entityId = deserializeNext(arguments);
Principal principal = deserializeNext(arguments);
Set<Action> actions = deserializeNext(arguments, SET_OF_ACTIONS);
LOG.trace("Revoking {} on {} from {}", actions, entityId, principal);
privilegesManager.revoke(Authorizable.fromEntityId(entityId), principal, actions);
LOG.info("Revoked {} on {} from {} successfully", actions, entityId, principal);
Set<? extends Permission> permissions = deserializeNext(arguments, SET_OF_PERMISSIONS);
LOG.trace("Revoking {} on {} from {}", permissions, entityId, principal);
permissionManager.revoke(Authorizable.fromEntityId(entityId), principal, permissions);
LOG.info("Revoked {} on {} from {} successfully", permissions, entityId, principal);
responder.sendStatus(HttpResponseStatus.OK);
}

Expand All @@ -165,7 +144,7 @@ public void revokeAll(FullHttpRequest request, HttpResponder responder) throws E
Iterator<MethodArgument> arguments = parseArguments(request);
EntityId entityId = deserializeNext(arguments);
LOG.trace("Revoking all actions on {}", entityId);
privilegesManager.revoke(Authorizable.fromEntityId(entityId));
permissionManager.revoke(Authorizable.fromEntityId(entityId));
LOG.info("Revoked all actions on {} successfully", entityId);
responder.sendStatus(HttpResponseStatus.OK);
}
Expand Down
4 changes: 2 additions & 2 deletions ...ore/remote/RemotePrivilegesCacheTest.java → ...re/remote/RemotePermissionsCacheTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@
/**
* Test {@link RemoteAccessEnforcer} with cache enabled.
*/
public class RemotePrivilegesCacheTest extends RemotePrivilegesTestBase {
public class RemotePermissionsCacheTest extends RemotePermissionsTestBase {

@BeforeClass
public static void beforeClass() throws IOException, InterruptedException {
cConf.setInt(Constants.Security.Authorization.CACHE_MAX_ENTRIES, 10000);
RemotePrivilegesTestBase.setup();
RemotePermissionsTestBase.setup();
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions ...e/remote/RemotePrivilegesNoCacheTest.java → .../remote/RemotePermissionsNoCacheTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,12 @@
/**
* Test {@link RemoteAccessEnforcer} with cache disabled.
*/
public class RemotePrivilegesNoCacheTest extends RemotePrivilegesTestBase {
public class RemotePermissionsNoCacheTest extends RemotePermissionsTestBase {

@BeforeClass
public static void beforeClass() throws IOException, InterruptedException {
cConf.setInt(Constants.Security.Authorization.CACHE_MAX_ENTRIES, 0);
RemotePrivilegesTestBase.setup();
RemotePermissionsTestBase.setup();
}

@Override
Expand Down
21 changes: 10 additions & 11 deletions ...tore/remote/RemotePrivilegesTestBase.java → ...ore/remote/RemotePermissionsTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
import io.cdap.cdap.security.authorization.RemoteAccessEnforcer;
import io.cdap.cdap.security.spi.authorization.AccessEnforcer;
import io.cdap.cdap.security.spi.authorization.PermissionManager;
import io.cdap.cdap.security.spi.authorization.PrivilegesManager;
import io.cdap.cdap.security.spi.authorization.UnauthorizedException;
import org.apache.twill.discovery.DiscoveryServiceClient;
import org.junit.After;
Expand All @@ -59,11 +58,11 @@
import java.util.concurrent.TimeUnit;

/**
* Tests for remote implementations of {@link AccessEnforcer} and {@link PrivilegesManager}.
* Tests for remote implementations of {@link AccessEnforcer} and {@link PermissionManager}.
* These are in app-fabric, because we need to start app-fabric in these tests.
*/
@SuppressWarnings("WeakerAccess")
public abstract class RemotePrivilegesTestBase {
public abstract class RemotePermissionsTestBase {
@ClassRule
public static final TemporaryFolder TEMPORARY_FOLDER = new TemporaryFolder();

Expand Down Expand Up @@ -107,9 +106,9 @@ public void after() throws Exception {
}

@Test
public void testPrivilegesManager() throws Exception {
// In this test, grants and revokes happen via PrivilegesManager, privilege listing and enforcement happens via
// Authorizer. Also, since grants and revokes go directly to master and don't need a proxy, the
public void testPermissionManager() throws Exception {
// In this test, grants and revokes happen via PermissionManager, privilege listing and enforcement happens via
// AccessEnforcer. Also, since grants and revokes go directly to master and don't need a proxy, the
// RemoteSystemOperationsService does not need to be started in this release.
permissionManager.grant(Authorizable.fromEntityId(NS), ALICE, EnumSet.allOf(StandardPermission.class));
permissionManager.grant(Authorizable.fromEntityId(APP), ALICE, Collections.singleton(StandardPermission.UPDATE));
Expand All @@ -127,9 +126,9 @@ public void testPrivilegesManager() throws Exception {
permissionManager.revoke(Authorizable.fromEntityId(NS), ALICE, EnumSet.allOf(StandardPermission.class));
permissionManager.revoke(Authorizable.fromEntityId(NS, EntityType.PROFILE), ALICE,
Collections.singleton(StandardPermission.LIST));
Set<GrantedPermission> privileges = permissionManager.listGrants(ALICE);
Assert.assertTrue(String.format("Expected all of alice's privileges to be revoked, but found %s", privileges),
privileges.isEmpty());
Set<GrantedPermission> permissions = permissionManager.listGrants(ALICE);
Assert.assertTrue(String.format("Expected all of alice's permissions to be revoked, but found %s", permissions),
permissions.isEmpty());
}

@Test
Expand Down Expand Up @@ -168,13 +167,13 @@ public void testVisibility() throws Exception {
DatasetId ds1 = NS.dataset("ds1");
DatasetId ds2 = NS.dataset("ds2");

// Grant privileges on non-numbered entities to ALICE
// Grant permissions on non-numbered entities to ALICE
permissionManager.grant(Authorizable.fromEntityId(PROGRAM), ALICE,
Collections.singleton(ApplicationPermission.EXECUTE));
permissionManager.grant(Authorizable.fromEntityId(ds), ALICE,
EnumSet.of(StandardPermission.GET, StandardPermission.UPDATE));

// Grant privileges on entities ending with 2 to BOB
// Grant permissions on entities ending with 2 to BOB
permissionManager.grant(Authorizable.fromEntityId(program2), BOB,
Collections.singleton(StandardPermission.UPDATE));
permissionManager.grant(Authorizable.fromEntityId(ds2), BOB,
Expand Down
Loading

0 comments on commit 133c91f

Please sign in to comment.