From d2ae4dce522a03b761185afdd1c1a1c1086160a0 Mon Sep 17 00:00:00 2001 From: Grant Oakley Date: Thu, 5 May 2022 11:24:57 -0700 Subject: [PATCH 1/3] binder: Add SecurityPolicies#anyOf(), similar to allOf(). Adds some details to the allOf() javadoc to match. --- .../java/io/grpc/binder/SecurityPolicies.java | 63 +++++++++++++++++++ .../io/grpc/binder/SecurityPoliciesTest.java | 53 +++++++++++++++- 2 files changed, 115 insertions(+), 1 deletion(-) diff --git a/binder/src/main/java/io/grpc/binder/SecurityPolicies.java b/binder/src/main/java/io/grpc/binder/SecurityPolicies.java index 879cc69167c..10a86a2aff1 100644 --- a/binder/src/main/java/io/grpc/binder/SecurityPolicies.java +++ b/binder/src/main/java/io/grpc/binder/SecurityPolicies.java @@ -29,7 +29,10 @@ import com.google.errorprone.annotations.CheckReturnValue; import io.grpc.ExperimentalApi; import io.grpc.Status; +import java.util.ArrayList; import java.util.Collection; +import java.util.Iterator; +import java.util.List; /** Static factory methods for creating standard security policies. */ @CheckReturnValue @@ -191,6 +194,12 @@ private static boolean checkPackageSignature( * Creates a {@link SecurityPolicy} that allows access if and only if *all* of the specified * {@code securityPolicies} allow access. * + *

Policies will be checked in the order that they are passed. If a policy denies access, + * subsequent policies will not be checked. + * + *

If a policy denies access, the {@link io.grpc.Status} returned by {@code checkAuthorization} + * will be that of the failed policy. + * * @param securityPolicies the security policies that all must allow access. * @throws NullPointerException if any of the inputs are {@code null}. * @throws IllegalArgumentException if {@code securityPolicies} is empty. @@ -218,6 +227,60 @@ public Status checkAuthorization(int uid) { }; } + /** + * Creates a {@link SecurityPolicy} that allows access if *any* of the specified {@code + * securityPolicies} allow access. + * + *

Policies will be checked in the order that they are passed. If a policy allows access, + * subsequent policies will not be checked. + * + *

If all policies deny access, the {@link io.grpc.Status} returned by {@code + * checkAuthorization} will included the concatenated descriptions of the failed policies and + * attach any additional causes as suppressed throwables. The status code will be that of the + * first failed policy. + * + * @param securityPolicies the security policies that will be checked. + * @throws NullPointerException if any of the inputs are {@code null}. + * @throws IllegalArgumentException if {@code securityPolicies} is empty. + */ + public static SecurityPolicy anyOf(SecurityPolicy... securityPolicies) { + Preconditions.checkNotNull(securityPolicies, "securityPolicies"); + Preconditions.checkArgument(securityPolicies.length > 0, "securityPolicies must not be empty"); + + return anyOfSecurityPolicy(securityPolicies); + } + + private static SecurityPolicy anyOfSecurityPolicy(SecurityPolicy... securityPolicies) { + return new SecurityPolicy() { + @Override + public Status checkAuthorization(int uid) { + List failed = new ArrayList<>(); + for (SecurityPolicy policy : securityPolicies) { + Status checkAuth = policy.checkAuthorization(uid); + if (checkAuth.isOk()) { + return checkAuth; + } + failed.add(checkAuth); + } + + Iterator iter = failed.iterator(); + Status toReturn = iter.next(); + while (iter.hasNext()) { + Status append = iter.next(); + toReturn = toReturn.augmentDescription(append.getDescription()); + if (append.getCause() != null) { + if (toReturn.getCause() != null) { + toReturn.getCause().addSuppressed(append.getCause()); + } else { + toReturn = toReturn.withCause(append.getCause()); + } + } + } + return toReturn; + } + }; + } + /** * Creates a {@link SecurityPolicy} which checks if the caller has all of the given permissions * from {@code permissions}. diff --git a/binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java b/binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java index bff414db60f..369366dfd73 100644 --- a/binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java +++ b/binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java @@ -34,6 +34,7 @@ import com.google.common.collect.ImmutableSet; import io.grpc.Status; import java.util.HashMap; +import java.util.concurrent.atomic.AtomicBoolean; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -382,14 +383,64 @@ public void testAllOf_succeedsIfAllSecurityPoliciesAllowed() throws Exception { @Test public void testAllOf_failsIfOneSecurityPoliciesNotAllowed() throws Exception { + final AtomicBoolean policyCalled = new AtomicBoolean(false); policy = SecurityPolicies.allOf( SecurityPolicies.internalOnly(), - SecurityPolicies.permissionDenied("Not allowed SecurityPolicy")); + SecurityPolicies.permissionDenied("Not allowed SecurityPolicy"), + new SecurityPolicy() { + @Override + public Status checkAuthorization(int uid) { + policyCalled.set(true); + throw new AssertionError(); + } + }); assertThat(policy.checkAuthorization(MY_UID).getCode()) .isEqualTo(Status.PERMISSION_DENIED.getCode()); assertThat(policy.checkAuthorization(MY_UID).getDescription()) .contains("Not allowed SecurityPolicy"); + assertThat(policyCalled.get()).isFalse(); + } + + @Test + public void testAnyOf_succeedsIfAnySecurityPoliciesAllowed() throws Exception { + final AtomicBoolean policyCalled = new AtomicBoolean(false); + policy = + SecurityPolicies.anyOf( + SecurityPolicies.internalOnly(), + new SecurityPolicy() { + @Override + public Status checkAuthorization(int uid) { + policyCalled.set(true); + throw new AssertionError(); + } + }); + + assertThat(policy.checkAuthorization(MY_UID).getCode()).isEqualTo(Status.OK.getCode()); + assertThat(policyCalled.get()).isFalse(); + } + + @Test + public void testAnyOf_failsIfNoSecurityPolicyIsAllowed() throws Exception { + policy = + SecurityPolicies.anyOf( + new SecurityPolicy() { + @Override + public Status checkAuthorization(int uid) { + return Status.PERMISSION_DENIED.withDescription("Not allowed: first"); + } + }, + new SecurityPolicy() { + @Override + public Status checkAuthorization(int uid) { + return Status.UNAUTHENTICATED.withDescription("Not allowed: second"); + } + }); + + assertThat(policy.checkAuthorization(MY_UID).getCode()) + .isEqualTo(Status.PERMISSION_DENIED.getCode()); + assertThat(policy.checkAuthorization(MY_UID).getDescription()).contains("Not allowed: first"); + assertThat(policy.checkAuthorization(MY_UID).getDescription()).contains("Not allowed: second"); } } From dd7597e2ff6f884113c81b94118a129bec59c3e3 Mon Sep 17 00:00:00 2001 From: Grant Oakley Date: Fri, 6 May 2022 15:52:55 -0700 Subject: [PATCH 2/3] Resolving PR comments. --- .../java/io/grpc/binder/SecurityPolicies.java | 6 ---- .../io/grpc/binder/SecurityPoliciesTest.java | 36 ++++++++----------- 2 files changed, 14 insertions(+), 28 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/SecurityPolicies.java b/binder/src/main/java/io/grpc/binder/SecurityPolicies.java index 10a86a2aff1..0fd5430ba61 100644 --- a/binder/src/main/java/io/grpc/binder/SecurityPolicies.java +++ b/binder/src/main/java/io/grpc/binder/SecurityPolicies.java @@ -194,12 +194,6 @@ private static boolean checkPackageSignature( * Creates a {@link SecurityPolicy} that allows access if and only if *all* of the specified * {@code securityPolicies} allow access. * - *

Policies will be checked in the order that they are passed. If a policy denies access, - * subsequent policies will not be checked. - * - *

If a policy denies access, the {@link io.grpc.Status} returned by {@code checkAuthorization} - * will be that of the failed policy. - * * @param securityPolicies the security policies that all must allow access. * @throws NullPointerException if any of the inputs are {@code null}. * @throws IllegalArgumentException if {@code securityPolicies} is empty. diff --git a/binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java b/binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java index 369366dfd73..784f0c9239c 100644 --- a/binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java +++ b/binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java @@ -383,42 +383,24 @@ public void testAllOf_succeedsIfAllSecurityPoliciesAllowed() throws Exception { @Test public void testAllOf_failsIfOneSecurityPoliciesNotAllowed() throws Exception { - final AtomicBoolean policyCalled = new AtomicBoolean(false); policy = SecurityPolicies.allOf( SecurityPolicies.internalOnly(), - SecurityPolicies.permissionDenied("Not allowed SecurityPolicy"), - new SecurityPolicy() { - @Override - public Status checkAuthorization(int uid) { - policyCalled.set(true); - throw new AssertionError(); - } - }); + SecurityPolicies.permissionDenied("Not allowed SecurityPolicy")); assertThat(policy.checkAuthorization(MY_UID).getCode()) .isEqualTo(Status.PERMISSION_DENIED.getCode()); assertThat(policy.checkAuthorization(MY_UID).getDescription()) .contains("Not allowed SecurityPolicy"); - assertThat(policyCalled.get()).isFalse(); } @Test public void testAnyOf_succeedsIfAnySecurityPoliciesAllowed() throws Exception { - final AtomicBoolean policyCalled = new AtomicBoolean(false); - policy = - SecurityPolicies.anyOf( - SecurityPolicies.internalOnly(), - new SecurityPolicy() { - @Override - public Status checkAuthorization(int uid) { - policyCalled.set(true); - throw new AssertionError(); - } - }); + RecordingPolicy recordingPolicy = new RecordingPolicy(); + policy = SecurityPolicies.anyOf(SecurityPolicies.internalOnly(), recordingPolicy); assertThat(policy.checkAuthorization(MY_UID).getCode()).isEqualTo(Status.OK.getCode()); - assertThat(policyCalled.get()).isFalse(); + assertThat(recordingPolicy.numCalls.get()).isEqualTo(0); } @Test @@ -443,4 +425,14 @@ public Status checkAuthorization(int uid) { assertThat(policy.checkAuthorization(MY_UID).getDescription()).contains("Not allowed: first"); assertThat(policy.checkAuthorization(MY_UID).getDescription()).contains("Not allowed: second"); } + + private static final class RecordingPolicy extends SecurityPolicy { + private final AtomicInteger numCalls = new AtomicInteger(0); + + @Override + public Status checkAuthorization(int uid) { + numCalls.incrementAndGet(); + return Status.OK; + } + } } From b0642624f29140ce897ddd8e18edab7ff9162027 Mon Sep 17 00:00:00 2001 From: Grant Oakley Date: Sun, 8 May 2022 08:50:17 -0700 Subject: [PATCH 3/3] Fix import. Was AtomicBoolean, should be AtomicInteger. --- binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java b/binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java index 784f0c9239c..a17162325e6 100644 --- a/binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java +++ b/binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java @@ -34,7 +34,7 @@ import com.google.common.collect.ImmutableSet; import io.grpc.Status; import java.util.HashMap; -import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith;