diff --git a/server/src/main/java/keywhiz/service/permissions/AnyPermissionCheck.java b/server/src/main/java/keywhiz/service/permissions/AnyPermissionCheck.java index 6035be53f..75d18cb46 100644 --- a/server/src/main/java/keywhiz/service/permissions/AnyPermissionCheck.java +++ b/server/src/main/java/keywhiz/service/permissions/AnyPermissionCheck.java @@ -1,5 +1,6 @@ package keywhiz.service.permissions; +import com.google.common.collect.ImmutableList; import java.util.List; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -11,7 +12,7 @@ public class AnyPermissionCheck implements PermissionCheck { private List subordinateChecks; public AnyPermissionCheck(List subordinateChecks) { - this.subordinateChecks = subordinateChecks; + this.subordinateChecks = ImmutableList.copyOf(subordinateChecks); } public boolean isAllowed(Object source, String action, Object target) { diff --git a/server/src/test/java/keywhiz/service/permission/AnyPermissionCheckTest.java b/server/src/test/java/keywhiz/service/permission/AnyPermissionCheckTest.java index e7900c307..8659d64d2 100644 --- a/server/src/test/java/keywhiz/service/permission/AnyPermissionCheckTest.java +++ b/server/src/test/java/keywhiz/service/permission/AnyPermissionCheckTest.java @@ -26,24 +26,20 @@ public class AnyPermissionCheckTest { private static Object source; private static Object target; - @Test public void isAllowedReturnsTrueWithEmptyCheckList() { + @Test public void isAllowedReturnsFalseWithEmptyCheckList() { PermissionCheck anyPermissionCheck = new AnyPermissionCheck(Collections.emptyList()); assertFalse(anyPermissionCheck.isAllowed(source, Action.ADD, target)); } @Test public void isAllowedReturnsFalseWhenAllDelegatesReturnFalse() { - when(delegate1.isAllowed(any(), any(), any())).thenReturn(false); - when(delegate2.isAllowed(any(), any(), any())).thenReturn(false); - PermissionCheck anyPermissionCheck = new AnyPermissionCheck(List.of(delegate1, delegate2)); + PermissionCheck anyPermissionCheck = new AnyPermissionCheck(createDelegatesList(false, false)); assertFalse(anyPermissionCheck.isAllowed(source, Action.ADD, target)); } @Test public void isAllowedReturnsTrueWhenOneDelegateReturnsTrue() { - when(delegate1.isAllowed(any(), any(), any())).thenReturn(true); - when(delegate2.isAllowed(any(), any(), any())).thenReturn(false); - PermissionCheck anyPermissionCheck = new AnyPermissionCheck(List.of(delegate1, delegate2)); + PermissionCheck anyPermissionCheck = new AnyPermissionCheck(createDelegatesList(true, false)); assertTrue(anyPermissionCheck.isAllowed(source, Action.ADD, target)); } @@ -56,19 +52,21 @@ public class AnyPermissionCheckTest { } @Test public void checkAllowedOrThrowThrowsExceptionWhenAllDelegatesReturnFalse() { - when(delegate1.isAllowed(any(), any(), any())).thenReturn(false); - when(delegate2.isAllowed(any(), any(), any())).thenReturn(false); - PermissionCheck anyPermissionCheck = new AnyPermissionCheck(List.of(delegate1, delegate2)); + PermissionCheck anyPermissionCheck = new AnyPermissionCheck(createDelegatesList(false, false)); assertThatThrownBy(() -> anyPermissionCheck.checkAllowedOrThrow(source, Action.ADD, target)) .isInstanceOf(RuntimeException.class); } @Test public void checkAllowedOrThrowReturnsVoidWhenOneDelegateReturnsTrue() { - when(delegate1.isAllowed(any(), any(), any())).thenReturn(true); - when(delegate2.isAllowed(any(), any(), any())).thenReturn(false); - PermissionCheck anyPermissionCheck = new AnyPermissionCheck(List.of(delegate1, delegate2)); + PermissionCheck anyPermissionCheck = new AnyPermissionCheck(createDelegatesList(true, false)); - anyPermissionCheck.isAllowed(source, Action.ADD, target); + anyPermissionCheck.checkAllowedOrThrow(source, Action.ADD, target); + } + + private List createDelegatesList(boolean delegate1Allowed, boolean delegate2Allowed) { + when(delegate1.isAllowed(any(), any(), any())).thenReturn(delegate1Allowed); + when(delegate2.isAllowed(any(), any(), any())).thenReturn(delegate2Allowed); + return List.of(delegate1, delegate2); } }