Merge pull request #1068 from square/violet/AlwaysAllowPermissionChec…

…k_dev Added a permission check that always allows the action to take place. Will be adding real permission check on top of this skeleton.
square · Jun 6, 2022 · f65286a · f65286a
2 parents c03d8e6 + 6286f41
commit f65286a
Show file tree

Hide file tree

Showing 12 changed files with 275 additions and 3 deletions.
diff --git a/server/src/main/java/keywhiz/KeywhizService.java b/server/src/main/java/keywhiz/KeywhizService.java
@@ -39,6 +39,7 @@
 import keywhiz.inject.InjectorFactory;
 import keywhiz.service.filters.CookieRenewingFilter;
 import keywhiz.service.filters.SecurityHeadersFilter;
+import keywhiz.service.permissions.PermissionCheck;
 import keywhiz.service.providers.AuthResolver;
 import keywhiz.service.providers.AutomationClientAuthFactory;
 import keywhiz.service.providers.ClientAuthFactory;
@@ -140,6 +141,8 @@ private void doRun(KeywhizConfig config, Environment environment) {
         injector.getInstance(AutomationClientAuthFactory.class),
         injector.getInstance(UserAuthFactory.class)));
 
+    jersey.register(injector.getInstance(PermissionCheck.class));
+
     logger.debug("Registering resources");
     jersey.register(injector.getInstance(BackfillRowHmacResource.class));
     jersey.register(injector.getInstance(ClientResource.class));

diff --git a/server/src/main/java/keywhiz/ServiceModule.java b/server/src/main/java/keywhiz/ServiceModule.java
@@ -44,6 +44,7 @@
 import keywhiz.service.daos.SecretController;
 import keywhiz.service.daos.SecretDAO.SecretDAOFactory;
 import keywhiz.service.filters.SecurityHeadersFilter;
+import keywhiz.service.permissions.PermissionCheckModule;
 import keywhiz.service.resources.admin.SessionMeResource;
 import keywhiz.utility.DSLContexts;
 import org.jooq.DSLContext;
@@ -76,6 +77,7 @@ public ServiceModule(KeywhizConfig config, Environment environment) {
     install(new CryptoModule(config.getDerivationProviderClass(), config.getContentKeyStore()));
     install(new DaoModule());
     install(new StrictGuiceModule());
+    install(new PermissionCheckModule());
   }
 
   // AuditLog

diff --git a/server/src/main/java/keywhiz/inject/ContextModule.java b/server/src/main/java/keywhiz/inject/ContextModule.java
@@ -1,5 +1,6 @@
 package keywhiz.inject;
 
+import com.codahale.metrics.MetricRegistry;
 import com.google.inject.AbstractModule;
 import io.dropwizard.Configuration;
 import io.dropwizard.setup.Environment;
@@ -8,16 +9,19 @@
 public class ContextModule extends AbstractModule {
   private final KeywhizConfig config;
   private final Environment environment;
+  private final MetricRegistry metricRegistry;
 
   public ContextModule(KeywhizConfig config, Environment environment) {
     this.config = config;
     this.environment = environment;
+    this.metricRegistry = this.environment.metrics();
   }
 
   @Override protected void configure() {
     // TODO(justin): Consider https://github.com/HubSpot/dropwizard-guice.
     bind(Environment.class).toInstance(environment);
     bind(Configuration.class).toInstance(config);
     bind(KeywhizConfig.class).toInstance(config);
+    bind(MetricRegistry.class).toInstance(metricRegistry);
   }
 }
diff --git a/server/src/main/java/keywhiz/service/permissions/AlwaysAllowDelegatingPermissionCheck.java b/server/src/main/java/keywhiz/service/permissions/AlwaysAllowDelegatingPermissionCheck.java
@@ -0,0 +1,54 @@
+package keywhiz.service.permissions;
+
+import com.codahale.metrics.MetricRegistry;
+import com.google.inject.Inject;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class AlwaysAllowDelegatingPermissionCheck implements PermissionCheck {
+  private static final Logger logger = LoggerFactory.getLogger(AlwaysAllowDelegatingPermissionCheck.class);
+
+  private PermissionCheck delegate;
+  private MetricRegistry metricRegistry;
+
+  @Inject
+  public AlwaysAllowDelegatingPermissionCheck(MetricRegistry metricRegistry, PermissionCheck delegate) {
+    this.delegate = delegate;
+    this.metricRegistry = metricRegistry;
+  }
+
+  public boolean isAllowed(KeywhizPrincipal source, String action, Object target) {
+    boolean hasPermission = delegate.isAllowed(source, action, target);
+
+    int hasPermissionSuccessMetricInt = hasPermission ? 1 : 0;
+    String successMetricName = MetricRegistry.name(AlwaysAllowDelegatingPermissionCheck.class, "isAllowed", "success", "histogram");
+    metricRegistry.histogram(successMetricName).update(hasPermissionSuccessMetricInt);
+
+    int hasPermissionFailureMetricInt = hasPermission ? 0 : 1;
+    String failureMetricName = MetricRegistry.name(AlwaysAllowDelegatingPermissionCheck.class, "isAllowed", "failure", "histogram");
+    metricRegistry.histogram(failureMetricName).update(hasPermissionFailureMetricInt);
+
+    logger.info(
+        String.format("isAllowed Actor: %s, Action: %s, Target: %s, Result: %s", source, action, target,
+            hasPermission));
+
+    return true;
+  }
+
+  @Override
+  public void checkAllowedOrThrow(KeywhizPrincipal source, String action, Object target) {
+    String successMetricName = MetricRegistry.name(AlwaysAllowDelegatingPermissionCheck.class, "checkAllowedOrThrow", "success", "histogram");
+    String exceptionMetricName = MetricRegistry.name(AlwaysAllowDelegatingPermissionCheck.class, "checkAllowedOrThrow", "exception", "histogram");
+    try {
+      delegate.checkAllowedOrThrow(source, action, target);
+
+      metricRegistry.histogram(successMetricName).update(1);
+      metricRegistry.histogram(exceptionMetricName).update(0);
+    } catch (RuntimeException e) {
+      metricRegistry.histogram(successMetricName).update(0);
+      metricRegistry.histogram(exceptionMetricName).update(1);
+
+      logger.error(String.format("checkAllowedOrThrow Actor: %s, Action: %s, Target: %s throws exception", source, action, target),e);
+    }
+  }
+}
diff --git a/server/src/main/java/keywhiz/service/permissions/AlwaysFailPermissionCheck.java b/server/src/main/java/keywhiz/service/permissions/AlwaysFailPermissionCheck.java
@@ -0,0 +1,12 @@
+package keywhiz.service.permissions;
+
+import javax.inject.Inject;
+
+public class AlwaysFailPermissionCheck implements PermissionCheck {
+  @Inject
+  public AlwaysFailPermissionCheck() {}
+
+  public boolean isAllowed(KeywhizPrincipal source, String action, Object target) {
+    return false;
+  }
+}
diff --git a/server/src/main/java/keywhiz/service/permissions/PermissionCheck.java b/server/src/main/java/keywhiz/service/permissions/PermissionCheck.java
@@ -2,5 +2,11 @@
 
 public interface PermissionCheck {
   boolean isAllowed(KeywhizPrincipal source, String action, Object target);
-  void checkAllowedOrThrow(KeywhizPrincipal source, String action, Object target);
+
+  default void checkAllowedOrThrow(KeywhizPrincipal source, String action, Object target){
+    if (!isAllowed(source, action, target)) {
+      throw new RuntimeException(String.format("Actor: %s, Action: %s, Target: %s, Result: %s throws exception", source, action, target,
+          false));
+    }
+  }
 }
diff --git a/server/src/main/java/keywhiz/service/permissions/PermissionCheckModule.java b/server/src/main/java/keywhiz/service/permissions/PermissionCheckModule.java
@@ -0,0 +1,18 @@
+package keywhiz.service.permissions;
+
+import com.codahale.metrics.MetricRegistry;
+import com.google.inject.AbstractModule;
+import com.google.inject.Provides;
+
+public class PermissionCheckModule extends AbstractModule {
+
+  @Override
+  protected void configure() {}
+
+  @Provides
+  public PermissionCheck createPermissionCheck(MetricRegistry metricRegistry) {
+    PermissionCheck alwaysFail = new AlwaysFailPermissionCheck();
+    PermissionCheck alwaysAllow = new AlwaysAllowDelegatingPermissionCheck(metricRegistry, alwaysFail);
+    return alwaysAllow;
+  }
+}
diff --git a/server/src/test/java/keywhiz/inject/ContextModuleTest.java b/server/src/test/java/keywhiz/inject/ContextModuleTest.java
@@ -1,5 +1,6 @@
 package keywhiz.inject;
 
+import com.codahale.metrics.MetricRegistry;
 import com.google.inject.Guice;
 import io.dropwizard.Configuration;
 import io.dropwizard.setup.Environment;
@@ -17,6 +18,8 @@ class Holder {
       @Inject KeywhizConfig keywhizConfig;
       @Inject Configuration config;
       @Inject Environment environment;
+      @Inject MetricRegistry metricRegistry1;
+      @Inject MetricRegistry metricRegistry2;
     }
 
     Holder holder = new Holder();
@@ -29,5 +32,8 @@ class Holder {
     assertSame(context.getConfig(), holder.keywhizConfig);
     assertSame(context.getConfig(), holder.config);
     assertSame(context.getEnvironment(), holder.environment);
+    assertSame(context.getMetricRegistry(), holder.metricRegistry1);
+    assertSame(context.getMetricRegistry(), holder.metricRegistry2);
+    assertSame(holder.metricRegistry1, holder.metricRegistry2);
   }
 }
diff --git a/...er/src/test/java/keywhiz/service/permission/AlwaysAllowDelegatingPermissionCheckTest.java b/...er/src/test/java/keywhiz/service/permission/AlwaysAllowDelegatingPermissionCheckTest.java
@@ -0,0 +1,97 @@
+package keywhiz.service.permission;
+
+import com.codahale.metrics.MetricRegistry;
+import java.util.Objects;
+import keywhiz.service.permissions.Action;
+import keywhiz.service.permissions.AlwaysAllowDelegatingPermissionCheck;
+import keywhiz.service.permissions.KeywhizPrincipal;
+import keywhiz.service.permissions.PermissionCheck;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.when;
+
+public class AlwaysAllowDelegatingPermissionCheckTest {
+
+  @Rule public MockitoRule mockito = MockitoJUnit.rule();
+
+  @Mock PermissionCheck delegate;
+
+  private MetricRegistry metricRegistry;
+  private AlwaysAllowDelegatingPermissionCheck alwaysAllow;
+
+  private static KeywhizPrincipal principal;
+  private static Objects target;
+
+  private static final String ISALLOWED_SUCCESS_METRIC_NAME = "keywhiz.service.permissions.AlwaysAllowDelegatingPermissionCheck.isAllowed.success.histogram";
+  private static final String ISALLOWED_FAILURE_METRIC_NAME = "keywhiz.service.permissions.AlwaysAllowDelegatingPermissionCheck.isAllowed.failure.histogram";
+  private static final String CHECKALLOWEDORTHROW_SUCCESS_METRIC_NAME = "keywhiz.service.permissions.AlwaysAllowDelegatingPermissionCheck.checkAllowedOrThrow.success.histogram";
+  private static final String CHECKALLOWEDORTHROW_EXCEPTION_METRIC_NAME = "keywhiz.service.permissions.AlwaysAllowDelegatingPermissionCheck.checkAllowedOrThrow.exception.histogram";
+
+  @Before
+  public void setUp() {
+    metricRegistry = new MetricRegistry();
+    alwaysAllow = new AlwaysAllowDelegatingPermissionCheck(metricRegistry, delegate);
+  }
+
+  @Test public void isAllowedReturnsTrueWhenDelegateReturnsTrue() {
+    when(delegate.isAllowed(any(), any(), any())).thenReturn(true);
+
+    boolean permitted = alwaysAllow.isAllowed(principal, Action.ADD, target);
+
+    assertThat(permitted);
+
+    assertThat(metricRegistry.histogram(ISALLOWED_SUCCESS_METRIC_NAME).getCount()).isEqualTo(1);
+    assertThat(metricRegistry.histogram(ISALLOWED_SUCCESS_METRIC_NAME).getSnapshot().getMean()).isEqualTo(1);
+
+    assertThat(metricRegistry.histogram(ISALLOWED_FAILURE_METRIC_NAME).getCount()).isEqualTo(1);
+    assertThat(metricRegistry.histogram(ISALLOWED_FAILURE_METRIC_NAME).getSnapshot().getMean()).isEqualTo(0);
+  }
+
+  @Test public void isAllowedReturnsTrueWhenDelegateReturnsFalse() {
+    when(delegate.isAllowed(any(), any(), any())).thenReturn(false);
+
+    boolean permitted = alwaysAllow.isAllowed(principal, Action.ADD, target);
+
+    assertThat(permitted);
+
+    assertThat(metricRegistry.histogram(ISALLOWED_SUCCESS_METRIC_NAME).getCount()).isEqualTo(1);
+    assertThat(metricRegistry.histogram(ISALLOWED_SUCCESS_METRIC_NAME).getSnapshot().getMean()).isEqualTo(0);
+
+    assertThat(metricRegistry.histogram(ISALLOWED_FAILURE_METRIC_NAME).getCount()).isEqualTo(1);
+    assertThat(metricRegistry.histogram(ISALLOWED_FAILURE_METRIC_NAME).getSnapshot().getMean()).isEqualTo(1);
+  }
+
+  @Test public void CheckAllowedOrThrowReturnsVoidWhenDelegateReturnsVoid() {
+    doNothing().when(delegate).checkAllowedOrThrow(any(), any(), any());
+
+    alwaysAllow.checkAllowedOrThrow(principal, Action.ADD, target);
+
+    assertThat(metricRegistry.histogram(CHECKALLOWEDORTHROW_SUCCESS_METRIC_NAME).getCount()).isEqualTo(1);
+    assertThat(metricRegistry.histogram(CHECKALLOWEDORTHROW_SUCCESS_METRIC_NAME).getSnapshot().getMean()).isEqualTo(1);
+
+    assertThat(metricRegistry.histogram(CHECKALLOWEDORTHROW_EXCEPTION_METRIC_NAME).getCount()).isEqualTo(1);
+    assertThat(metricRegistry.histogram(CHECKALLOWEDORTHROW_EXCEPTION_METRIC_NAME).getSnapshot().getMean()).isEqualTo(0);
+  }
+
+  @Test public void CheckAllowedOrThrowReturnsVoidWhenDelegateThrowException() {
+    doThrow(RuntimeException.class).when(delegate).checkAllowedOrThrow(any(), any(), any());
+
+    alwaysAllow.checkAllowedOrThrow(principal, Action.ADD, target);
+
+    assertThat(metricRegistry.histogram(CHECKALLOWEDORTHROW_SUCCESS_METRIC_NAME).getCount()).isEqualTo(1);
+    assertThat(metricRegistry.histogram(CHECKALLOWEDORTHROW_SUCCESS_METRIC_NAME).getSnapshot().getMean()).isEqualTo(0);
+
+    assertThat(metricRegistry.histogram(CHECKALLOWEDORTHROW_EXCEPTION_METRIC_NAME).getCount()).isEqualTo(1);
+    assertThat(metricRegistry.histogram(CHECKALLOWEDORTHROW_EXCEPTION_METRIC_NAME).getSnapshot().getMean()).isEqualTo(1);
+  }
+}
+
diff --git a/server/src/test/java/keywhiz/service/permission/AlwaysFailPermissionCheckTest.java b/server/src/test/java/keywhiz/service/permission/AlwaysFailPermissionCheckTest.java
@@ -0,0 +1,35 @@
+package keywhiz.service.permission;
+
+import com.codahale.metrics.MetricRegistry;
+import java.util.Objects;
+import keywhiz.service.permissions.Action;
+import keywhiz.service.permissions.AlwaysFailPermissionCheck;
+import keywhiz.service.permissions.KeywhizPrincipal;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+public class AlwaysFailPermissionCheckTest {
+  MetricRegistry metricRegistry;
+  AlwaysFailPermissionCheck alwaysFail;
+  KeywhizPrincipal principal;
+  Objects target;
+
+  @Before
+  public void setUp() {
+    alwaysFail = new AlwaysFailPermissionCheck();
+  }
+
+  @Test
+  public void isAllowedReturnsFalse() {
+    boolean permitted = alwaysFail.isAllowed(principal, Action.ADD, target);
+    assertThat(permitted).isEqualTo(false);
+  }
+
+  @Test
+  public void checkAllowedOrThrowThrowsException() {
+    assertThatThrownBy(() -> {alwaysFail.checkAllowedOrThrow(principal, Action.ADD, target);}).isInstanceOf(RuntimeException.class);
+  }
+}
diff --git a/server/src/test/java/keywhiz/service/permission/PermissionCheckModuleTest.java b/server/src/test/java/keywhiz/service/permission/PermissionCheckModuleTest.java
@@ -0,0 +1,25 @@
+package keywhiz.service.permission;
+
+import javax.inject.Inject;
+import keywhiz.service.permissions.PermissionCheck;
+import org.junit.Test;
+
+import static keywhiz.test.KeywhizTests.createInjector;
+import static org.junit.Assert.assertNotNull;
+
+public class PermissionCheckModuleTest {
+  @Test
+  public void createsInjector() {
+    assertNotNull(createInjector());
+  }
+
+  @Test
+  public void injectPermissionCheckProvider() {
+    class Holder {
+      @Inject PermissionCheck permissionCheck;
+    }
+    Holder holder = new Holder();
+    createInjector().injectMembers(holder);
+    assertNotNull(holder.permissionCheck);
+  }
+}
diff --git a/server/src/test/java/keywhiz/test/ServiceContext.java b/server/src/test/java/keywhiz/test/ServiceContext.java
@@ -1,5 +1,6 @@
 package keywhiz.test;
 
+import com.codahale.metrics.MetricRegistry;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.base.Throwables;
 import com.google.common.io.Resources;
@@ -19,16 +20,19 @@ public class ServiceContext {
   private final Bootstrap<KeywhizConfig> bootstrap;
   private final KeywhizConfig config;
   private final Environment environment;
+  private final MetricRegistry metricRegistry;
 
   private ServiceContext(
       KeywhizService service,
       Bootstrap<KeywhizConfig> bootstrap,
       KeywhizConfig config,
-      Environment environment) {
+      Environment environment,
+      MetricRegistry metricRegistry) {
     this.service = service;
     this.bootstrap = bootstrap;
     this.config = config;
     this.environment = environment;
+    this.metricRegistry = metricRegistry;
   }
 
   public static ServiceContext create() {
@@ -48,7 +52,9 @@ public static ServiceContext create() {
         bootstrap.getMetricRegistry(),
         bootstrap.getClassLoader());
 
-    return new ServiceContext(service, bootstrap, config, environment);
+    MetricRegistry metricRegistry = environment.metrics();
+
+    return new ServiceContext(service, bootstrap, config, environment, metricRegistry);
   }
 
   public KeywhizService getService() { return service; }
@@ -68,4 +74,8 @@ private static KeywhizConfig loadTestConfig(ObjectMapper objectMapper, Validator
       throw Throwables.propagate(e);
     }
   }
+
+  public MetricRegistry getMetricRegistry() {
+    return metricRegistry;
+  }
 }