Address comments from CR/769886275

Author: jdcormie

binder/src/androidTest/java/io/grpc/binder/internal/BinderClientTransportTest.java

@@ -58,8 +58,10 @@
 import java.io.InputStream;
 import java.util.ArrayDeque;
 import java.util.Deque;
+import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.LinkedBlockingQueue;
 import java.util.concurrent.ScheduledExecutorService;
 import javax.annotation.Nullable;
 import org.junit.After;
@@ -185,7 +187,7 @@ public BinderTransport.BinderClientTransport build() {
 
   @After
   public void tearDown() throws Exception {
-    blockingSecurityPolicy.setAuthorizationResult(Status.ABORTED);
+    blockingSecurityPolicy.provideNextCheckAuthorizationResult(Status.ABORTED);
     transport.shutdownNow(Status.OK);
     HostServices.awaitServiceShutdown();
     shutdownAndTerminate(executorServicePool.getObject());

binder/src/main/java/io/grpc/binder/AndroidComponentAddress.java

@@ -250,7 +250,22 @@ public Builder setBindIntentFromComponent(ComponentName component) {
       return this;
     }
 
-    /** See {@link AndroidComponentAddress#getTargetUser()}. */
+    /**
+     * Specifies the Android user in which the built Address' bind Intent will be evaluated.
+     *
+     * <p>Connecting to a server in a different Android user is uncommon and requires the client app
+     * have runtime visibility of &#064;SystemApi's and hold certain &#064;SystemApi permissions.
+     * The device must also be running Android SDK version 30 or higher.
+     *
+     * <p>See https://developer.android.com/guide/app-compatibility/restrictions-non-sdk-interfaces
+     * for details on which apps can call the underlying &#064;SystemApi's needed to make this type
+     * of connection.
+     *
+     * <p>One of the "android.permission.INTERACT_ACROSS_XXX" permissions is required. The exact one
+     * depends on the calling user's relationship to the target user, whether client and server are
+     * in the same or different apps, and the version of Android in use. See {@link
+     * Context#bindServiceAsUser}, the essential underlying Android API, for details.
+     */
     @ExperimentalApi("https://github.com/grpc/grpc-java/issues/10173")
     public Builder setTargetUser(@Nullable UserHandle targetUser) {
       this.targetUser = targetUser;

binder/src/main/java/io/grpc/binder/ApiConstants.java

@@ -17,7 +17,6 @@
 package io.grpc.binder;
 
 import android.content.Intent;
-import android.content.pm.ServiceInfo;
 import android.os.UserHandle;
 import io.grpc.Attributes;
 import io.grpc.EquivalentAddressGroup;
@@ -38,11 +37,13 @@ private ApiConstants() {}
   /**
    * Specifies the Android user in which target URIs should be resolved.
    *
-   * <p>{@link UserHandle} can't reasonably be encoded in a target URI string. Instead, all
-   * {@link io.grpc.NameResolverProvider}s producing {@link AndroidComponentAddress}es should let
-   * clients address servers in another Android user using this argument.
+   * <p>{@link UserHandle} can't reasonably be encoded in a target URI string. Instead, all {@link
+   * io.grpc.NameResolverProvider}s producing {@link AndroidComponentAddress}es should let clients
+   * address servers in another Android user using this argument.
    *
-   * <p>See also {@link AndroidComponentAddress#getTargetUser()}.
+   * <p>Connecting to a server in a different Android user is uncommon and can only be done by a
+   * "system app" client with special permissions. See {@link
+   * AndroidComponentAddress.Builder#setTargetUser(UserHandle)} for details.
    */
   public static final NameResolver.Args.Key<UserHandle> TARGET_ANDROID_USER =
       NameResolver.Args.Key.create("target-android-user");
@@ -57,11 +58,4 @@ private ApiConstants() {}
   @EquivalentAddressGroup.Attr
   public static final Attributes.Key<Boolean> PRE_AUTH_SERVER_OVERRIDE =
       Attributes.Key.create("pre-auth-server-override");
-
-  /**
-   * The authentic ServiceInfo for an {@link io.grpc.EquivalentAddressGroup} of {@link
-   * AndroidComponentAddress}es, in case a {@link NameResolver} has already looked it up.
-   */
-  public static final Attributes.Key<ServiceInfo> TARGET_SERVICE_INFO =
-      Attributes.Key.create("target-service-info");
 }

binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java

@@ -158,7 +158,7 @@ public static BinderChannelBuilder forTarget(String target) {
   }
 
   private final ManagedChannelImplBuilder managedChannelImplBuilder;
-  final BinderClientTransportFactory.Builder transportFactoryBuilder;
+  private final BinderClientTransportFactory.Builder transportFactoryBuilder;
 
   private boolean strictLifecycleManagement;
 
@@ -242,9 +242,9 @@ public BinderChannelBuilder securityPolicy(SecurityPolicy securityPolicy) {
    * specify a {@link UserHandle}. If neither the Channel nor the {@link AndroidComponentAddress}
    * specifies a target user, the {@link UserHandle} of the current process will be used.
    *
-   * <p>Targeting a Service in a different Android user is uncommon and requires special permissions
-   * normally reserved for system apps. See {@link android.content.Context#bindServiceAsUser} for
-   * details.
+   * <p>Connecting to a server in a different Android user is uncommon and can only be done by a
+   * "system app" client with special permissions. See {@link
+   * AndroidComponentAddress.Builder#setTargetUser(UserHandle)} for details.
    *
    * @deprecated This method's name is misleading because it implies an impersonated client identity
    *     when it's actually specifying part of the server's location. It's also no longer necessary

binder/src/main/java/io/grpc/binder/internal/Bindable.java

@@ -50,7 +50,7 @@ interface Observer {
   /**
    * Fetches details about the remote Service from PackageManager without binding to it.
    *
-   * <p>Resolving an untrusted address before binding to it lets you screen out unauthorized servers
+   * <p>Resolving an untrusted address before binding to it lets you screen out problematic servers
    * before giving them a chance to run. However, note that the identity/existence of the resolved
    * Service can change between the time this method returns and the time you actually bind/connect
    * to it. For example, suppose the target package gets uninstalled right after this method

binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java

@@ -37,6 +37,7 @@
 import io.grpc.StatusException;
 import io.grpc.binder.BinderChannelCredentials;
 import java.lang.reflect.Method;
+import java.util.List;
 import java.util.concurrent.Executor;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -91,6 +92,8 @@ public String methodName() {
   private final Observer observer;
   private final Executor mainThreadExecutor;
 
+  private static volatile Method queryIntentServicesAsUserMethod;
+
   @GuardedBy("this")
   private State state;
 
@@ -253,19 +256,41 @@ void unbindInternal(Status reason) {
     }
   }
 
-  private static int getIdentifier(UserHandle userHandle) throws ReflectiveOperationException {
-    return (int) userHandle.getClass().getDeclaredMethod("getIdentifier").invoke(userHandle);
+  // Sadly the PackageManager#resolveServiceAsUser() API we need isn't part of the SDK or even a
+  // @SystemApi as of this writing. Modern Android prevents even system apps from calling it, by any
+  // means (https://developer.android.com/guide/app-compatibility/restrictions-non-sdk-interfaces).
+  // So instead we call queryIntentServicesAsUser(), which does more than we need but *is* a
+  // @SystemApi in all the SDK versions where we support cross-user Channels.
+  @Nullable
+  private static ResolveInfo resolveServiceAsUser(
+      PackageManager packageManager, Intent intent, int flags, UserHandle targetUserHandle) {
+    List<ResolveInfo> results =
+        queryIntentServicesAsUser(packageManager, intent, flags, targetUserHandle);
+    // The first query result is "what would be returned by resolveService", per the javadoc.
+    return (results != null && !results.isEmpty()) ? results.get(0) : null;
   }
 
-  // Sadly we must call this system API reflectively since it isn't part of the Android SDK.
-  private static ResolveInfo resolveServiceAsUser(
+  // The cross-user Channel feature requires the client to be a system app so we assume @SystemApi
+  // queryIntentServicesAsUser() is visible to us at runtime. It would be visible at build time too,
+  // if our host system app were written to call it directly. We only have to use reflection here
+  // because grpc-java is a library built outside the Android source tree where the compiler can't
+  // see the "non-SDK" @SystemApis that we need.
+  @Nullable
+  @SuppressWarnings("unchecked") // Safe by PackageManager#queryIntentServicesAsUser spec in AOSP.
+  private static List<ResolveInfo> queryIntentServicesAsUser(
       PackageManager packageManager, Intent intent, int flags, UserHandle targetUserHandle) {
     try {
-      return (ResolveInfo)
-          packageManager
-              .getClass()
-              .getMethod("resolveServiceAsUser", Intent.class, int.class, int.class)
-              .invoke(packageManager, intent, flags, getIdentifier(targetUserHandle));
+      if (queryIntentServicesAsUserMethod == null) {
+        synchronized (ServiceBinding.class) {
+          if (queryIntentServicesAsUserMethod == null) {
+            queryIntentServicesAsUserMethod =
+                PackageManager.class.getMethod(
+                    "queryIntentServicesAsUser", Intent.class, int.class, UserHandle.class);
+          }
+        }
+      }
+      return (List<ResolveInfo>)
+          queryIntentServicesAsUserMethod.invoke(packageManager, intent, flags, targetUserHandle);
     } catch (ReflectiveOperationException e) {
       throw new VerifyException(e);
     }

binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java

@@ -46,7 +46,6 @@
 import io.grpc.internal.ManagedClientTransport;
 import io.grpc.internal.ObjectPool;
 import io.grpc.internal.SharedResourcePool;
-import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.Executor;
 import java.util.concurrent.ScheduledExecutorService;
@@ -208,7 +207,7 @@ public void clientAuthorizesServerUidsInOrder() throws Exception {
     serverPkgInfo.applicationInfo.uid = 22222; // UID of the server *app*, which can be different.
     shadowOf(application.getPackageManager()).installPackage(serverPkgInfo);
     shadowOf(application.getPackageManager()).addOrUpdateService(serviceInfo);
-    server = newServer(Collections.emptyList());
+    server = newServer(ImmutableList.of());
     server.start(serverListener);
 
     SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();