Skip to content

Support AysncSecurityPolicy in SecurityPolicies.allOf. #12158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support AysncSecurityPolicy in SecurityPolicies.allOf. #12158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 45 additions & 2 deletions binder/src/main/java/io/grpc/binder/SecurityPolicies.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@

package io.grpc.binder;

import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.util.concurrent.Futures.immediateFuture;

import android.annotation.SuppressLint;
import android.app.admin.DevicePolicyManager;
import android.content.Context;
Expand All @@ -32,6 +35,9 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.hash.Hashing;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.MoreExecutors;
import com.google.errorprone.annotations.CheckReturnValue;
import io.grpc.ExperimentalApi;
import io.grpc.Status;
Expand Down Expand Up @@ -333,6 +339,9 @@ private static boolean checkPackageSignature(
* Creates a {@link SecurityPolicy} that allows access if and only if *all* of the specified
* {@code securityPolicies} allow access.
*
* <p>If any of the policies is an {@link AsyncSecurityPolicy}, then all policies may be evaluated
* concurrently to speed up the success scenario.
*
* @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.
Expand All @@ -341,10 +350,17 @@ public static SecurityPolicy allOf(SecurityPolicy... securityPolicies) {
Preconditions.checkNotNull(securityPolicies, "securityPolicies");
Preconditions.checkArgument(securityPolicies.length > 0, "securityPolicies must not be empty");

return allOfSecurityPolicy(securityPolicies);
boolean anyAsync =
Arrays
.stream(securityPolicies)
.anyMatch(policy -> policy instanceof AsyncSecurityPolicy);

return anyAsync
? allOfSecurityPolicyAsync(securityPolicies)
: allOfSecurityPolicySync(securityPolicies);
}

private static SecurityPolicy allOfSecurityPolicy(SecurityPolicy... securityPolicies) {
private static SecurityPolicy allOfSecurityPolicySync(SecurityPolicy... securityPolicies) {
return new SecurityPolicy() {
@Override
public Status checkAuthorization(int uid) {
Expand All @@ -360,6 +376,33 @@ public Status checkAuthorization(int uid) {
};
}

private static SecurityPolicy allOfSecurityPolicyAsync(SecurityPolicy... securityPolicies) {
return new AsyncSecurityPolicy() {
@Override
public ListenableFuture<Status> checkAuthorizationAsync(int uid) {
ImmutableList<ListenableFuture<Status>> allStatuses =
Arrays.stream(securityPolicies).map(policy -> {
AsyncSecurityPolicy asyncPolicy =
policy instanceof AsyncSecurityPolicy ? (AsyncSecurityPolicy) policy :
new AsyncSecurityPolicy() {
@Override
public ListenableFuture<Status> checkAuthorizationAsync(int uid) {
return immediateFuture(policy.checkAuthorization(uid));
Copy link
Member

@jdcormie jdcormie Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be a valid implementation of the checkAuthorizationAsync() API contract because it blocks the calling thread.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ALL of your input securityPolicies are AsyncSecurityPolicy, I think you can implement an async allOf(). But if even one of your inputs is not an AsyncSecurityPolicy, someone needs to provide an Executor to make this possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My goal was to not force the clients to pick another API if they have any AsyncSecurityPolicy, specially because the current method is still compatible (though inefficient) with AsyncSecurityPolicy, so I felt like having two methods could be confusing.

This doesn't seem to be a valid implementation of the checkAuthorizationAsync() API contract because it blocks the calling thread.

My intention was to treat all policies as AsyncSecurityPolicy by obtaining a List<ListenableFuture<Status>> and transforming them without blocking. If one of the policies is synchronous, we just produce an immediate future.

This could be harmful, yes, if that policy were slow, but I see that as API misuse: the integrator should have provided an AsyncSecurityPolicy instead and we wouldn't monopolize a thread waiting on it.

Please let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK what more to say except that implementations of checkAuthorizationAsync() aren't allowed to block. If they were, there wouldn't be much reason for AsyncSecurityPolicy to even exist: Plain old SecurityPolicy.checkAuthorization() has always had the property that it may or may not block. And we've always been able to turn checkAuthorization() into a ListenableFuture using Futures.submit(securityPolicy::checkAuthorization, blockingExecutor). What makes checkAuthorizationAsync() interesting is that it promises not to block so I can call it from any thread.

Hiding a blocking call to checkAuthorization() behind checkAuthorizationAsync() actively defeats one of your two most important callers here:

grpc-java/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java

Lines 778 to 782 in d2d8ed8

ListenableFuture<Status> authFuture =
(securityPolicy instanceof AsyncSecurityPolicy)
? ((AsyncSecurityPolicy) securityPolicy).checkAuthorizationAsync(remoteUid)
: Futures.submit(
() -> securityPolicy.checkAuthorization(remoteUid), offloadExecutor);

This could be harmful, yes, if that policy were slow, but I see that as API misuse: the integrator should have provided an AsyncSecurityPolicy instead and we wouldn't monopolize a thread waiting on it.

OK but your javadoc doesn't say any of that. Even if it did, we want APIs that are difficult to misuse and, ideally, impossible to misuse, by way of checks at compile time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I was approaching this with the intent that it would retroactively work for any usage of allOf without any required API changes (by requesting the addition of a Executor). I didn't feel like clients would correctly and proactively know they should use a separate method if they have 1 or more AsyncSecurityPolicy.

I see 3 options going forward:

  1. Take only async policies:

    public static AsyncSecurityPolicy allOf(AsyncSecurityPolicies...policies)

    An executor may be passed for use in Futures.transform or we omit it since the computation is likely lightweight enough for the direct executor.

    Pros: minimal room for API misuse
    Cons: puts the burden on the caller to convert any synchronous SecurityPolicy into AsyncSecurityPolicy. We could mitigate this with by providing helpers for converting SecurityPolicy or Function<Integer, Status> into AsyncSecurityPolicy.

  2. (current approach) Take a mix of policy types and assume correct usage

    We could improve Javadoc to discourage slow operations in the synchronous SecurityPolicy.

    Cons: room for API misuse already covered by you above

  3. Take an Executor to convert the SynchronousPolicies into async:

    public static AsyncSecurityPolicy allOf(ListeningExecutorService blockingExecutor, SyncSecurityPolicies...policies) {
  List<AsyncSecurityPolicy> asyncPolicies = policies.map(policy -> {
    if (policy instanceof AsyncSecurityPolicy) {
      return policy;
    }
    return asAsyncPolicy(policy, blockingExecutor);
  }).collect(toList());
  // ...
}

    Cons: assumes all blocking policies can be converted under the same executor; option 1 is a more generalized version of this that puts that responsibility on the caller.

Do you feel like any of these would be preferable?

}
};
return asyncPolicy.checkAuthorizationAsync(uid);
}).collect(toImmutableList());
ListenableFuture<List<Status>> futureStatuses = Futures.allAsList(allStatuses);

return Futures
.transform(
futureStatuses,statuses ->
statuses.stream().filter(status -> !status.isOk()).findFirst().orElse(Status.OK),
MoreExecutors.directExecutor());
}
};
}

/**
* Creates a {@link SecurityPolicy} that allows access if *any* of the specified {@code
* securityPolicies} allow access.
Expand Down
43 changes: 39 additions & 4 deletions binder/src/test/java/io/grpc/binder/SecurityPoliciesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import static android.content.pm.PackageInfo.REQUESTED_PERMISSION_GRANTED;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.util.concurrent.Futures.immediateFuture;
import static org.robolectric.Shadows.shadowOf;

import android.app.admin.DevicePolicyManager;
Expand All @@ -35,9 +36,11 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.hash.Hashing;
import com.google.common.util.concurrent.ListenableFuture;
import io.grpc.Status;
import java.util.HashMap;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Function;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -523,15 +526,38 @@ public void testAllOf_succeedsIfAllSecurityPoliciesAllowed() throws Exception {

@Test
public void testAllOf_failsIfOneSecurityPoliciesNotAllowed() throws Exception {
policy =
SecurityPolicies.allOf(
SecurityPolicies.internalOnly(),
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");
}

@Test
public void testAllOfAsync_succeedsIfAllSecurityPoliciesAllowed() {
policy =
SecurityPolicies.allOf(
SecurityPolicies.internalOnly(),
makeAsyncPolicy(uid -> immediateFuture(Status.OK)));

assertThat(policy.checkAuthorization(MY_UID).getCode()).isEqualTo(Status.OK.getCode());
}

@Test
public void testAllOfAsync_failsIfOneSecurityPoliciesNotAllowed() {
policy =
SecurityPolicies.allOf(
SecurityPolicies.internalOnly(),
SecurityPolicies.permissionDenied("Not allowed SecurityPolicy"));
makeAsyncPolicy(uid -> immediateFuture(Status.OK)),
makeAsyncPolicy(uid -> immediateFuture(Status.ABORTED)),
makeAsyncPolicy(uid -> immediateFuture(Status.INVALID_ARGUMENT)));

assertThat(policy.checkAuthorization(MY_UID).getCode())
.isEqualTo(Status.PERMISSION_DENIED.getCode());
assertThat(policy.checkAuthorization(MY_UID).getDescription())
.contains("Not allowed SecurityPolicy");
.isEqualTo(Status.Code.ABORTED);
}

@Test
Expand Down Expand Up @@ -703,4 +729,13 @@ public void testOneOfSignatureSha256Hash_failsIfPackageNameMatchAndOneOfSignatur
private static byte[] getSha256Hash(Signature signature) {
return Hashing.sha256().hashBytes(signature.toByteArray()).asBytes();
}

private static AsyncSecurityPolicy makeAsyncPolicy(Function<Integer, ListenableFuture<Status>> statusProvider) {
return new AsyncSecurityPolicy() {
@Override
public ListenableFuture<Status> checkAuthorizationAsync(int uid) {
return statusProvider.apply(uid);
}
};
}
}
Loading