Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

Commit

Permalink
Introducing top-level permission check method for target type
Browse files Browse the repository at this point in the history
  • Loading branch information
@mmontgomery-square
mmontgomery-square committed Aug 4, 2022
1 parent cafad62 commit ce13137
Show file tree
Hide file tree
Showing 11 changed files with 101 additions and 31 deletions.
30 changes: 29 additions & 1 deletion server/src/main/java/keywhiz/service/permissions/AlwaysAllowDelegatingPermissionCheck.java
View file
Expand Up @@ -20,6 +20,7 @@ public AlwaysAllowDelegatingPermissionCheck(MetricRegistry metricRegistry, Permi
this.metricRegistry = metricRegistry;
}

@Override
public boolean isAllowed(Object source, String action, Object target) {
boolean hasPermission = delegate.isAllowed(source, action, target);

Expand All @@ -32,14 +33,41 @@ public boolean isAllowed(Object source, String action, Object target) {
return true;
}

@Override
public boolean isAllowedForTargetType(Object source, String action, Class<?> targetType) {
boolean hasPermission = delegate.isAllowedForTargetType(source, action, targetType);

emitHistogramMetrics(hasPermission);

logger.info(
String.format("isAllowedByType Actor: %s, Action: %s, Target type: %s, Result: %s", source, action, targetType,
hasPermission));

return true;
}

@Override
public void checkAllowedOrThrow(Object source, String action, Object target) {
Boolean isPermitted;
try {
delegate.checkAllowedOrThrow(source, action, target);
isPermitted = true;
} catch (RuntimeException e) {
logger.error(String.format("checkAllowedOrThrow Actor: %s, Action: %s, Target: %s throws exception", source, action, target),e);
logger.error(String.format("checkAllowedOrThrow Actor: %s, Action: %s, Target: %s throws exception", source, action, target), e);
isPermitted = false;
}

emitHistogramMetrics(isPermitted);
}

@Override
public void checkAllowedForTargetTypeOrThrow(Object source, String action, Class<?> targetType) {
Boolean isPermitted;
try {
delegate.checkAllowedForTargetTypeOrThrow(source, action, targetType);
isPermitted = true;
} catch (RuntimeException e) {
logger.error(String.format("checkAllowedByTypeOrThrow Actor: %s, Action: %s, Target type: %s throws exception", source, action, targetType), e);
isPermitted = false;
}

Expand Down
2 changes: 2 additions & 0 deletions server/src/main/java/keywhiz/service/permissions/AlwaysFailPermissionCheck.java
View file
Expand Up @@ -9,4 +9,6 @@ public AlwaysFailPermissionCheck() {}
public boolean isAllowed(Object source, String action, Object target) {
return false;
}

public boolean isAllowedForTargetType(Object source, String action, Class<?> targetType) { return false; }
}
20 changes: 19 additions & 1 deletion server/src/main/java/keywhiz/service/permissions/AnyPermissionCheck.java
View file
Expand Up @@ -3,7 +3,6 @@
import com.codahale.metrics.MetricRegistry;
import com.google.common.collect.ImmutableList;
import java.util.List;
import javax.inject.Inject;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -41,6 +40,25 @@ public boolean isAllowed(Object source, String action, Object target) {
return hasPermission;
}

public boolean isAllowedForTargetType(Object source, String action, Class<?> targetType) {
boolean hasPermission = false;

for (PermissionCheck subordinateCheck : subordinateChecks) {
if (subordinateCheck.isAllowedForTargetType(source, action, targetType)) {
hasPermission = true;
break;
}
}

logger.info(
String.format("isAllowedByType Actor: %s, Action: %s, Target type: %s, Result: %s", source, action, targetType,
hasPermission));

emitHistogramMetrics(hasPermission);

return hasPermission;
}

private void emitHistogramMetrics(Boolean isPermitted) {
int hasPermissionSuccessMetricInt = isPermitted ? 1 : 0;
metricRegistry.histogram(SUCCESS_METRIC_NAME).update(hasPermissionSuccessMetricInt);
Expand Down
16 changes: 14 additions & 2 deletions server/src/main/java/keywhiz/service/permissions/AutomationClientPermissionCheck.java
View file
Expand Up @@ -6,7 +6,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class AutomationClientPermissionCheck implements PermissionCheck{
public class AutomationClientPermissionCheck implements PermissionCheck {
private static final Logger logger = LoggerFactory.getLogger(AutomationClientPermissionCheck.class);

private static final String SUCCESS_METRIC_NAME = MetricRegistry.name(AutomationClientPermissionCheck.class, "success", "histogram");
Expand All @@ -31,7 +31,19 @@ public boolean isAllowed(Object source, String action, Object target) {
return hasPermission;
}

private boolean isAutomation(Object source) {
public boolean isAllowedForTargetType(Object source, String action, Class<?> targetType) {
boolean hasPermission = isAutomation(source);

emitHistogramMetrics(hasPermission);

logger.info(
String.format("isAllowedByType Actor: %s, Action: %s, Target type: %s, Result: %s", source, action, targetType,
hasPermission));

return hasPermission;
}

private static boolean isAutomation(Object source) {
if (source instanceof Client) {
Client client = (Client) source;
return client.isAutomationAllowed();
Expand Down
14 changes: 13 additions & 1 deletion server/src/main/java/keywhiz/service/permissions/OwnershipPermissionCheck.java
View file
Expand Up @@ -59,7 +59,19 @@ public boolean isAllowed(Object source, String action, Object target) {
return hasPermission;
}

private boolean isClient(Object source) {
public boolean isAllowedForTargetType(Object source, String action, Class<?> targetType) {
boolean hasPermission = false;

emitHistogramMetrics(hasPermission);

logger.info(
String.format("isAllowedByType Actor: %s, Action: %s, Target type: %s, Result: %s", source, action, targetType,
hasPermission));

return hasPermission;
}

private static boolean isClient(Object source) {
return source instanceof Client;
}

Expand Down
11 changes: 6 additions & 5 deletions server/src/main/java/keywhiz/service/permissions/PermissionCheck.java
View file
Expand Up @@ -3,9 +3,7 @@
public interface PermissionCheck {
boolean isAllowed(Object source, String action, Object target);

default boolean isAllowed(Object source, String action) {
return isAllowed(source, action, null);
}
boolean isAllowedForTargetType(Object source, String action, Class<?> targetType);

default void checkAllowedOrThrow(Object source, String action, Object target){
if (!isAllowed(source, action, target)) {
Expand All @@ -14,7 +12,10 @@ default void checkAllowedOrThrow(Object source, String action, Object target){
}
}

default void checkAllowedOrThrow(Object source, String action) {
checkAllowedOrThrow(source, action, null);
default void checkAllowedForTargetTypeOrThrow(Object source, String action, Class<?> targetType) {
if (!isAllowedForTargetType(source, action, targetType)) {
throw new RuntimeException(String.format("Actor: %s, Action: %s, Target type: %s, Result: %s throws exception", source, action, targetType,
false));
}
}
}
4 changes: 2 additions & 2 deletions server/src/main/java/keywhiz/service/resources/automation/v2/ClientResource.java
View file
Expand Up @@ -98,7 +98,7 @@ public class ClientResource {
@Consumes(APPLICATION_JSON)
public Response createClient(@Auth AutomationClient automationClient,
@Valid CreateClientRequestV2 request) {
permissionCheck.checkAllowedOrThrow(automationClient, Action.CREATE);
permissionCheck.checkAllowedForTargetTypeOrThrow(automationClient, Action.CREATE, Client.class);

return tagErrors(() -> doCreateClient(automationClient, request));
}
Expand Down Expand Up @@ -146,7 +146,7 @@ private Response doCreateClient(AutomationClient automationClient,
@GET
@Produces(APPLICATION_JSON)
public Iterable<String> clientListing(@Auth AutomationClient automationClient) {
permissionCheck.checkAllowedOrThrow(automationClient, Action.READ);
permissionCheck.checkAllowedForTargetTypeOrThrow(automationClient, Action.READ, Client.class);

return clientDAOReadOnly.getClients().stream()
.map(Client::getName)
Expand Down
6 changes: 2 additions & 4 deletions server/src/main/java/keywhiz/service/resources/automation/v2/GroupResource.java
View file
Expand Up @@ -45,12 +45,10 @@
import org.slf4j.LoggerFactory;

import static java.lang.String.format;
import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toSet;
import static javax.ws.rs.core.MediaType.APPLICATION_JSON;
import static keywhiz.Tracing.setTag;
import static keywhiz.Tracing.tagErrors;
import static keywhiz.api.model.SanitizedSecretWithGroups.fromSecretSeriesAndContentAndGroups;

/**
* parentEndpointName automation/v2-group-management
Expand Down Expand Up @@ -88,7 +86,7 @@ public class GroupResource {
@Consumes(APPLICATION_JSON)
public Response createGroup(@Auth AutomationClient automationClient,
@Valid CreateGroupRequestV2 request) {
permissionCheck.checkAllowedOrThrow(automationClient, Action.CREATE);
permissionCheck.checkAllowedForTargetTypeOrThrow(automationClient, Action.CREATE, Group.class);

return tagErrors(() -> doCreateGroup(automationClient, request));
}
Expand Down Expand Up @@ -127,7 +125,7 @@ private Response doCreateGroup(AutomationClient automationClient,
@GET
@Produces(APPLICATION_JSON)
public Iterable<String> groupListing(@Auth AutomationClient automationClient) {
permissionCheck.checkAllowedOrThrow(automationClient, Action.READ);
permissionCheck.checkAllowedForTargetTypeOrThrow(automationClient, Action.READ, Group.class);

return groupDAOReadOnly.getGroups().stream()
.map(Group::getName)
Expand Down
16 changes: 8 additions & 8 deletions server/src/main/java/keywhiz/service/resources/automation/v2/SecretResource.java
View file
Expand Up @@ -126,7 +126,7 @@ public class SecretResource {
@Consumes(APPLICATION_JSON)
public Response createSecret(@Auth AutomationClient automationClient,
@Valid CreateSecretRequestV2 request) {
permissionCheck.checkAllowedOrThrow(automationClient, Action.CREATE);
permissionCheck.checkAllowedForTargetTypeOrThrow(automationClient, Action.CREATE, Secret.class);

// allows new version, return version in resulting path
String name = request.name();
Expand Down Expand Up @@ -192,7 +192,7 @@ public Response createOrUpdateSecret(@Auth AutomationClient automationClient,
if (maybeSecretSeriesAndContent.isPresent()) {
permissionCheck.checkAllowedOrThrow(automationClient, Action.UPDATE, maybeSecretSeriesAndContent.get());
} else {
permissionCheck.checkAllowedOrThrow(automationClient, Action.CREATE);
permissionCheck.checkAllowedForTargetTypeOrThrow(automationClient, Action.CREATE, Secret.class);
secretOwner = getSecretOwnerForSecretCreation(secretOwner, automationClient);
}

Expand Down Expand Up @@ -292,7 +292,7 @@ public Response partialUpdateSecret(@Auth AutomationClient automationClient,
public Iterable<String> secretListing(@Auth AutomationClient automationClient,
@QueryParam("idx") Integer idx, @QueryParam("num") Integer num,
@DefaultValue("true") @QueryParam("newestFirst") boolean newestFirst) {
permissionCheck.checkAllowedOrThrow(automationClient, Action.READ);
permissionCheck.checkAllowedForTargetTypeOrThrow(automationClient, Action.READ, Secret.class);

if (idx != null && num != null) {
if (idx < 0 || num < 0) {
Expand Down Expand Up @@ -326,7 +326,7 @@ public Iterable<String> secretListing(@Auth AutomationClient automationClient,
public Iterable<SanitizedSecret> secretListingV2(@Auth AutomationClient automationClient,
@QueryParam("idx") Integer idx, @QueryParam("num") Integer num,
@DefaultValue("true") @QueryParam("newestFirst") boolean newestFirst) {
permissionCheck.checkAllowedOrThrow(automationClient, Action.READ);
permissionCheck.checkAllowedForTargetTypeOrThrow(automationClient, Action.READ, SanitizedSecret.class);

if (idx != null && num != null) {
if (idx < 0 || num < 0) {
Expand All @@ -350,7 +350,7 @@ public Iterable<SanitizedSecret> secretListingV2(@Auth AutomationClient automati
@GET
@Produces(APPLICATION_JSON)
public Iterable<String> secretListingExpiring(@Auth AutomationClient automationClient, @PathParam("time") Long time) {
permissionCheck.checkAllowedOrThrow(automationClient, Action.READ);
permissionCheck.checkAllowedForTargetTypeOrThrow(automationClient, Action.READ, Secret.class);

List<SanitizedSecret> secrets = secretControllerReadOnly.getSanitizedSecrets(time, null);
return secrets.stream()
Expand All @@ -370,7 +370,7 @@ public Iterable<String> secretListingExpiring(@Auth AutomationClient automationC
@GET
@Produces(APPLICATION_JSON)
public Iterable<SanitizedSecret> secretListingExpiringV2(@Auth AutomationClient automationClient, @PathParam("time") Long time) {
permissionCheck.checkAllowedOrThrow(automationClient, Action.READ);
permissionCheck.checkAllowedForTargetTypeOrThrow(automationClient, Action.READ, SanitizedSecret.class);

List<SanitizedSecret> secrets = secretControllerReadOnly.getSanitizedSecrets(time, null);
return secrets;
Expand Down Expand Up @@ -398,7 +398,7 @@ public Iterable<SanitizedSecret> secretListingExpiringV2(@Auth AutomationClient
@Produces(APPLICATION_JSON)
public Iterable<SanitizedSecretWithGroups> secretListingExpiringV3(@Auth AutomationClient automationClient,
@PathParam("time") Long maxTime) {
permissionCheck.checkAllowedOrThrow(automationClient, Action.READ);
permissionCheck.checkAllowedForTargetTypeOrThrow(automationClient, Action.READ, SanitizedSecretWithGroups.class);

return secretControllerReadOnly.getSanitizedSecretsWithGroups(maxTime);
}
Expand Down Expand Up @@ -433,7 +433,7 @@ public SanitizedSecretWithGroupsListAndCursor secretListingExpiringV4(
@QueryParam("maxTime") Long maxTime,
@QueryParam("limit") Integer limit,
@QueryParam("cursor") String cursor) {
permissionCheck.checkAllowedOrThrow(automationClient, Action.READ);
permissionCheck.checkAllowedForTargetTypeOrThrow(automationClient, Action.READ, SanitizedSecretWithGroupsListAndCursor.class);

SecretRetrievalCursor cursorDecoded = null;
if (cursor != null) {
Expand Down
9 changes: 4 additions & 5 deletions ...r/src/test/java/keywhiz/service/permissions/AlwaysAllowDelegatingPermissionCheckTest.java
View file
@@ -1,7 +1,6 @@
package keywhiz.service.permissions;

import com.codahale.metrics.MetricRegistry;
import java.util.Objects;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
Expand All @@ -24,8 +23,8 @@ public class AlwaysAllowDelegatingPermissionCheckTest {
private MetricRegistry metricRegistry;
private AlwaysAllowDelegatingPermissionCheck alwaysAllow;

private static Objects target;
private static Objects principal;
private static Object target = new Object();
private static Object principal = new Object();

private static final String ISALLOWED_SUCCESS_METRIC_NAME = "keywhiz.service.permissions.AlwaysAllowDelegatingPermissionCheck.success.histogram";
private static final String ISALLOWED_FAILURE_METRIC_NAME = "keywhiz.service.permissions.AlwaysAllowDelegatingPermissionCheck.failure.histogram";
Expand Down Expand Up @@ -66,7 +65,7 @@ public void setUp() {
assertThat(metricRegistry.histogram(ISALLOWED_FAILURE_METRIC_NAME).getSnapshot().getMean()).isEqualTo(1);
}

@Test public void CheckAllowedOrThrowReturnsVoidWhenDelegateReturnsVoid() {
@Test public void checkAllowedOrThrowReturnsVoidWhenDelegateReturnsVoid() {
doNothing().when(delegate).checkAllowedOrThrow(any(), any(), any());

alwaysAllow.checkAllowedOrThrow(principal, Action.ADD, target);
Expand All @@ -78,7 +77,7 @@ public void setUp() {
assertThat(metricRegistry.histogram(CHECKALLOWEDORTHROW_EXCEPTION_METRIC_NAME).getSnapshot().getMean()).isEqualTo(0);
}

@Test public void CheckAllowedOrThrowReturnsVoidWhenDelegateThrowException() {
@Test public void checkAllowedOrThrowReturnsVoidWhenDelegateThrowException() {
doThrow(RuntimeException.class).when(delegate).checkAllowedOrThrow(any(), any(), any());

alwaysAllow.checkAllowedOrThrow(principal, Action.ADD, target);
Expand Down
4 changes: 2 additions & 2 deletions server/src/test/java/keywhiz/service/permissions/AnyPermissionCheckTest.java
View file
Expand Up @@ -22,8 +22,8 @@ public class AnyPermissionCheckTest {
@Mock private PermissionCheck delegate1;
@Mock private PermissionCheck delegate2;

private static Object source;
private static Object target;
private static Object source = new Object();
private static Object target = new Object();

private MetricRegistry metricRegistry;

Expand Down

0 comments on commit ce13137

Please sign in to comment.