diff --git a/server/src/main/java/keywhiz/service/permissions/AlwaysAllowDelegatingPermissionCheck.java b/server/src/main/java/keywhiz/service/permissions/AlwaysAllowDelegatingPermissionCheck.java index a7467160a..ef6553edf 100644 --- a/server/src/main/java/keywhiz/service/permissions/AlwaysAllowDelegatingPermissionCheck.java +++ b/server/src/main/java/keywhiz/service/permissions/AlwaysAllowDelegatingPermissionCheck.java @@ -7,6 +7,8 @@ public class AlwaysAllowDelegatingPermissionCheck implements PermissionCheck { private static final Logger logger = LoggerFactory.getLogger(AlwaysAllowDelegatingPermissionCheck.class); + private static final String successMetricName = MetricRegistry.name(AlwaysAllowDelegatingPermissionCheck.class, "success", "histogram"); + private static final String failureMetricName = MetricRegistry.name(AlwaysAllowDelegatingPermissionCheck.class, "failure", "histogram"); private PermissionCheck delegate; private MetricRegistry metricRegistry; @@ -17,16 +19,10 @@ public AlwaysAllowDelegatingPermissionCheck(MetricRegistry metricRegistry, Permi this.metricRegistry = metricRegistry; } - public boolean isAllowed(KeywhizPrincipal source, String action, Object target) { + public boolean isAllowed(Object 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); + emitHistogramMetrics(hasPermission); logger.info( String.format("isAllowed Actor: %s, Action: %s, Target: %s, Result: %s", source, action, target, @@ -36,19 +32,24 @@ public boolean isAllowed(KeywhizPrincipal source, String action, Object target) } @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"); + public void checkAllowedOrThrow(Object source, String action, Object target) { + Boolean isPermitted; try { delegate.checkAllowedOrThrow(source, action, target); - - metricRegistry.histogram(successMetricName).update(1); - metricRegistry.histogram(exceptionMetricName).update(0); + isPermitted = true; } 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); + isPermitted = false; } + + emitHistogramMetrics(isPermitted); + } + + private void emitHistogramMetrics(Boolean isPermitted) { + int hasPermissionSuccessMetricInt = isPermitted ? 1 : 0; + metricRegistry.histogram(successMetricName).update(hasPermissionSuccessMetricInt); + + int hasPermissionFailureMetricInt = isPermitted ? 0 : 1; + metricRegistry.histogram(failureMetricName).update(hasPermissionFailureMetricInt); } } diff --git a/server/src/main/java/keywhiz/service/permissions/AlwaysFailPermissionCheck.java b/server/src/main/java/keywhiz/service/permissions/AlwaysFailPermissionCheck.java index b3a5c7fa8..cd82857f5 100644 --- a/server/src/main/java/keywhiz/service/permissions/AlwaysFailPermissionCheck.java +++ b/server/src/main/java/keywhiz/service/permissions/AlwaysFailPermissionCheck.java @@ -6,7 +6,7 @@ public class AlwaysFailPermissionCheck implements PermissionCheck { @Inject public AlwaysFailPermissionCheck() {} - public boolean isAllowed(KeywhizPrincipal source, String action, Object target) { + public boolean isAllowed(Object source, String action, Object target) { return false; } } diff --git a/server/src/main/java/keywhiz/service/permissions/AutomationClientPermissionCheck.java b/server/src/main/java/keywhiz/service/permissions/AutomationClientPermissionCheck.java new file mode 100644 index 000000000..596e945aa --- /dev/null +++ b/server/src/main/java/keywhiz/service/permissions/AutomationClientPermissionCheck.java @@ -0,0 +1,50 @@ +package keywhiz.service.permissions; + +import com.codahale.metrics.MetricRegistry; +import com.google.inject.Inject; +import keywhiz.api.model.Client; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class AutomationClientPermissionCheck implements PermissionCheck{ + private static final Logger logger = LoggerFactory.getLogger( + AutomationClientPermissionCheck.class); + private static final String successMetricName = MetricRegistry.name(AutomationClientPermissionCheck.class, "success", "histogram"); + private static final String failureMetricName = MetricRegistry.name(AutomationClientPermissionCheck.class, "failure", "histogram"); + + private MetricRegistry metricRegistry; + + @Inject + public AutomationClientPermissionCheck(MetricRegistry metricRegistry) { + this.metricRegistry = metricRegistry; + } + + public boolean isAllowed(Object source, String action, Object target) { + boolean hasPermission = isAutomation(source); + + emitHistogramMetrics(hasPermission); + + logger.info( + String.format("isAllowed Actor: %s, Action: %s, Target: %s, Result: %s", source, action, target, + hasPermission)); + + return hasPermission; + } + + private boolean isAutomation(Object source) { + if (source instanceof Client) { + Client client = (Client) source; + return client.isAutomationAllowed(); + } + + return false; + } + + private void emitHistogramMetrics(Boolean isPermitted) { + int hasPermissionSuccessMetricInt = isPermitted ? 1 : 0; + metricRegistry.histogram(successMetricName).update(hasPermissionSuccessMetricInt); + + int hasPermissionFailureMetricInt = isPermitted ? 0 : 1; + metricRegistry.histogram(failureMetricName).update(hasPermissionFailureMetricInt); + } +} diff --git a/server/src/main/java/keywhiz/service/permissions/KeywhizPrincipal.java b/server/src/main/java/keywhiz/service/permissions/KeywhizPrincipal.java deleted file mode 100644 index 47552b97f..000000000 --- a/server/src/main/java/keywhiz/service/permissions/KeywhizPrincipal.java +++ /dev/null @@ -1,4 +0,0 @@ -package keywhiz.service.permissions; - -public interface KeywhizPrincipal { -} diff --git a/server/src/main/java/keywhiz/service/permissions/PermissionCheck.java b/server/src/main/java/keywhiz/service/permissions/PermissionCheck.java index 6081eb56d..fd659dcbd 100644 --- a/server/src/main/java/keywhiz/service/permissions/PermissionCheck.java +++ b/server/src/main/java/keywhiz/service/permissions/PermissionCheck.java @@ -1,9 +1,9 @@ package keywhiz.service.permissions; public interface PermissionCheck { - boolean isAllowed(KeywhizPrincipal source, String action, Object target); + boolean isAllowed(Object source, String action, Object target); - default void checkAllowedOrThrow(KeywhizPrincipal source, String action, Object target){ + default void checkAllowedOrThrow(Object 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/test/java/keywhiz/service/permission/AlwaysAllowDelegatingPermissionCheckTest.java b/server/src/test/java/keywhiz/service/permission/AlwaysAllowDelegatingPermissionCheckTest.java index 3795d2fe6..11b078b80 100644 --- a/server/src/test/java/keywhiz/service/permission/AlwaysAllowDelegatingPermissionCheckTest.java +++ b/server/src/test/java/keywhiz/service/permission/AlwaysAllowDelegatingPermissionCheckTest.java @@ -4,7 +4,6 @@ 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; @@ -28,13 +27,13 @@ public class AlwaysAllowDelegatingPermissionCheckTest { private MetricRegistry metricRegistry; private AlwaysAllowDelegatingPermissionCheck alwaysAllow; - private static KeywhizPrincipal principal; private static Objects target; + private static Objects principal; - 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"; + 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"; + private static final String CHECKALLOWEDORTHROW_SUCCESS_METRIC_NAME = "keywhiz.service.permissions.AlwaysAllowDelegatingPermissionCheck.success.histogram"; + private static final String CHECKALLOWEDORTHROW_EXCEPTION_METRIC_NAME = "keywhiz.service.permissions.AlwaysAllowDelegatingPermissionCheck.failure.histogram"; @Before public void setUp() { diff --git a/server/src/test/java/keywhiz/service/permission/AlwaysFailPermissionCheckTest.java b/server/src/test/java/keywhiz/service/permission/AlwaysFailPermissionCheckTest.java index 9e33b8462..ba31da51c 100644 --- a/server/src/test/java/keywhiz/service/permission/AlwaysFailPermissionCheckTest.java +++ b/server/src/test/java/keywhiz/service/permission/AlwaysFailPermissionCheckTest.java @@ -4,7 +4,6 @@ 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; @@ -14,8 +13,8 @@ public class AlwaysFailPermissionCheckTest { MetricRegistry metricRegistry; AlwaysFailPermissionCheck alwaysFail; - KeywhizPrincipal principal; Objects target; + Objects principal; @Before public void setUp() { diff --git a/server/src/test/java/keywhiz/service/permission/AutomationClientPermissionCheckTest.java b/server/src/test/java/keywhiz/service/permission/AutomationClientPermissionCheckTest.java new file mode 100644 index 000000000..2762a6712 --- /dev/null +++ b/server/src/test/java/keywhiz/service/permission/AutomationClientPermissionCheckTest.java @@ -0,0 +1,91 @@ +package keywhiz.service.permission; + +import com.codahale.metrics.MetricRegistry; +import java.util.Objects; +import keywhiz.auth.User; +import keywhiz.api.model.Client; +import keywhiz.service.permissions.Action; +import keywhiz.service.permissions.AutomationClientPermissionCheck; +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 AutomationClientPermissionCheckTest { + + private MetricRegistry metricRegistry; + private AutomationClientPermissionCheck automationCheck; + + private static Objects target; + + private static final String ISALLOWED_SUCCESS_METRIC_NAME = "keywhiz.service.permissions.AutomationClientPermissionCheck.success.histogram"; + private static final String ISALLOWED_FAILURE_METRIC_NAME = "keywhiz.service.permissions.AutomationClientPermissionCheck.failure.histogram"; + private static final String CHECKALLOWEDORTHROW_SUCCESS_METRIC_NAME = "keywhiz.service.permissions.AutomationClientPermissionCheck.success.histogram"; + private static final String CHECKALLOWEDORTHROW_EXCEPTION_METRIC_NAME = "keywhiz.service.permissions.AutomationClientPermissionCheck.failure.histogram"; + + private static final Client + automationClient = new Client(0, "automationClient", null, null, null, null, null, null, null, null, false, + true); + private static final Client + nonAutomationClient = new Client(0, "nonAutomationClient", null, null, null, null, null, null, null, null, false, + false); + private static final User user = User.named("user"); + + @Before + public void setUp() { + metricRegistry = new MetricRegistry(); + automationCheck = new AutomationClientPermissionCheck(metricRegistry); + } + + @Test public void testIsAllowedWithAutomationClient() { + boolean permitted = automationCheck.isAllowed(automationClient, Action.ADD, target); + + assertThat(permitted); + + checkCorrectMetrics(1); + } + + @Test public void testIsAllowedWithNonAutomationClient() { + boolean permitted = automationCheck.isAllowed(nonAutomationClient, Action.ADD, target); + + assertThat(!permitted); + + checkCorrectMetrics(0); + } + + @Test public void testIsAllowedWithUser() { + automationCheck.isAllowed(user, Action.ADD, target); + + checkCorrectMetrics(0); + } + + @Test public void testCheckAllowedOrThrowWithAutomationClient() { + automationCheck.checkAllowedOrThrow(automationClient, Action.ADD, target); + + checkCorrectMetrics(1); + + } + + @Test public void testCheckAllowedOrThrowWithNonAutomationClient() { + assertThatThrownBy(() -> automationCheck.checkAllowedOrThrow(nonAutomationClient, Action.ADD, + target)).isInstanceOf(RuntimeException.class); + + checkCorrectMetrics(0); + } + + @Test public void testCheckAllowedOrThrowWithUser() { + assertThatThrownBy(() -> automationCheck.checkAllowedOrThrow(user, Action.ADD, + target)).isInstanceOf(RuntimeException.class); + + checkCorrectMetrics(0); + } + + private void checkCorrectMetrics(int isPermitted) { + assertThat(metricRegistry.histogram(ISALLOWED_SUCCESS_METRIC_NAME).getCount()).isEqualTo(1); + assertThat(metricRegistry.histogram(ISALLOWED_SUCCESS_METRIC_NAME).getSnapshot().getMean()).isEqualTo(isPermitted); + + assertThat(metricRegistry.histogram(ISALLOWED_FAILURE_METRIC_NAME).getCount()).isEqualTo(1); + assertThat(metricRegistry.histogram(ISALLOWED_FAILURE_METRIC_NAME).getSnapshot().getMean()).isEqualTo(1-isPermitted); + } +}