Skip to content

Commit 20f3f42

Browse files
committed
Address more review comments.
Resolve with an explicit direct boot awareness flag # Conflicts: # binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
1 parent aafa682 commit 20f3f42

5 files changed

Lines changed: 20 additions & 30 deletions

File tree

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

Lines changed: 3 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -394,7 +394,6 @@ public void testBlackHoleSecurityPolicyAuthTimeout() throws Exception {
394394
assertThat(transportStatus.getCode()).isEqualTo(Code.DEADLINE_EXCEEDED);
395395
assertThat(transportStatus.getDescription()).contains("1234");
396396
transportListener.awaitTermination();
397-
398397
// If the transport gave up waiting on auth, it should cancel its request.
399398
assertThat(authRequest.isCancelled()).isTrue();
400399
}
@@ -409,14 +408,13 @@ public void testBlackHoleSecurityPolicyPreAuthTimeout() throws Exception {
409408
.setReadyTimeoutMillis(1_234)
410409
.build();
411410
transport.start(transportListener).run();
412-
// Take the pre-auth request but don't respond to it, in order to trigger the ready timeout.
411+
// Take the next authRequest but don't respond to it, in order to trigger the ready timeout.
413412
AuthRequest preAuthRequest = securityPolicy.takeNextAuthRequest(TIMEOUT_SECONDS, SECONDS);
414413

415414
Status transportStatus = transportListener.awaitShutdown();
416415
assertThat(transportStatus.getCode()).isEqualTo(Code.DEADLINE_EXCEEDED);
417416
assertThat(transportStatus.getDescription()).contains("1234");
418417
transportListener.awaitTermination();
419-
420418
// If the transport gave up waiting on auth, it should cancel its request.
421419
assertThat(preAuthRequest.isCancelled()).isTrue();
422420
}
@@ -492,11 +490,9 @@ public void testAsyncSecurityPolicyPreAuthSuccess() throws Exception {
492490
}
493491

494492
@Test
495-
public void testAsyncSecurityPolicyAuthCancelledUponExternalTermination() throws Exception {
493+
public void testAsyncSecurityPolicyCancelledUponExternalTermination() throws Exception {
496494
SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
497-
transport = new BinderClientTransportBuilder()
498-
.setPreAuthorizeServer(false)
499-
.setSecurityPolicy(securityPolicy).build();
495+
transport = new BinderClientTransportBuilder().setSecurityPolicy(securityPolicy).build();
500496
transport.start(transportListener).run();
501497
AuthRequest authRequest = securityPolicy.takeNextAuthRequest(TIMEOUT_SECONDS, SECONDS);
502498
transport.shutdownNow(Status.UNAVAILABLE); // 'authRequest' remains unanswered!
@@ -505,20 +501,6 @@ public void testAsyncSecurityPolicyAuthCancelledUponExternalTermination() throws
505501
assertThat(authRequest.isCancelled()).isTrue();
506502
}
507503

508-
@Test
509-
public void testAsyncSecurityPolicyPreAuthCancelledUponExternalTermination() throws Exception {
510-
SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
511-
transport = new BinderClientTransportBuilder()
512-
.setPreAuthorizeServer(true)
513-
.setSecurityPolicy(securityPolicy).build();
514-
transport.start(transportListener).run();
515-
AuthRequest preAuthRequest = securityPolicy.takeNextAuthRequest(TIMEOUT_SECONDS, SECONDS);
516-
transport.shutdownNow(Status.UNAVAILABLE); // 'preAuthRequest' remains unanswered!
517-
transportListener.awaitShutdown();
518-
transportListener.awaitTermination();
519-
assertThat(preAuthRequest.isCancelled()).isTrue();
520-
}
521-
522504
private static void startAndAwaitReady(
523505
BinderTransport.BinderClientTransport transport, TestTransportListener transportListener)
524506
throws Exception {

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,9 @@ interface Observer {
5353
* <p>Resolving an untrusted address before binding to it lets you screen out problematic servers
5454
* before giving them a chance to run. However, note that the identity/existence of the resolved
5555
* Service can change between the time this method returns and the time you actually bind/connect
56-
* to it. For example, suppose the target package gets uninstalled right after this method
57-
* returns. In {@link Observer#onBound}, you should verify that the server you resolved is the
58-
* same one you connected to.
56+
* to it. For example, suppose the target package gets uninstalled or upgraded right after this
57+
* method returns. In {@link Observer#onBound}, you should verify that the server you resolved is
58+
* the same one you connected to.
5959
*/
6060
@AnyThread
6161
ServiceInfo resolve() throws StatusException;

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@
5454
import io.grpc.Status;
5555
import io.grpc.StatusException;
5656
import io.grpc.binder.AndroidComponentAddress;
57-
import io.grpc.binder.ApiConstants;
5857
import io.grpc.binder.AsyncSecurityPolicy;
5958
import io.grpc.binder.InboundParcelablePolicy;
6059
import io.grpc.binder.SecurityPolicy;
@@ -81,7 +80,6 @@
8180
import java.util.concurrent.ScheduledExecutorService;
8281
import java.util.concurrent.ScheduledFuture;
8382
import java.util.concurrent.atomic.AtomicInteger;
84-
import java.util.concurrent.atomic.AtomicLong;
8583
import java.util.logging.Level;
8684
import java.util.logging.Logger;
8785
import javax.annotation.Nullable;
@@ -589,7 +587,7 @@ public static final class BinderClientTransport extends BinderTransport
589587
@GuardedBy("this")
590588
@Nullable private ListenableFuture<Status> authResultFuture; // null before we check auth.
591589
@GuardedBy("this")
592-
@Nullable private ListenableFuture<Status> preAuthResultFuture; // null before we check auth.
590+
@Nullable private ListenableFuture<Status> preAuthResultFuture; // null before we pre-auth.
593591

594592
/**
595593
* Constructs a new transport instance.

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import android.content.pm.PackageManager;
2727
import android.content.pm.ResolveInfo;
2828
import android.content.pm.ServiceInfo;
29+
import android.os.Build;
2930
import android.os.IBinder;
3031
import android.os.UserHandle;
3132
import androidx.annotation.AnyThread;
@@ -301,10 +302,19 @@ private static List<ResolveInfo> queryIntentServicesAsUser(
301302
public ServiceInfo resolve() throws StatusException {
302303
checkState(sourceContext != null);
303304
PackageManager packageManager = sourceContext.getPackageManager();
305+
int flags = 0;
306+
if (Build.VERSION.SDK_INT >= 29) {
307+
// Use the current locked/unlocked state of 'targetUserHandle' to filter <service> matches.
308+
// Callers want to resolve 'bindIntent' the same way a follow-up call to bind() will. And
309+
// if the target user is locked, bindService() ignores matches that can't presently be created
310+
// due to directBootAware=false. Of course this filter races against a concurrent unlock, but
311+
// that's no different than the other resolve/connect races our callers must already handle.
312+
flags |= PackageManager.MATCH_DIRECT_BOOT_AUTO;
313+
}
304314
ResolveInfo resolveInfo =
305315
targetUserHandle != null
306-
? resolveServiceAsUser(packageManager, bindIntent, 0, targetUserHandle)
307-
: packageManager.resolveService(bindIntent, 0);
316+
? resolveServiceAsUser(packageManager, bindIntent, flags, targetUserHandle)
317+
: packageManager.resolveService(bindIntent, flags);
308318
if (resolveInfo == null) {
309319
throw Status.UNIMPLEMENTED // Same status code as when bindService() returns false.
310320
.withDescription("resolveService(" + bindIntent + " / " + targetUserHandle + ") was null")

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,7 @@ public void testBindWithDeviceAdmin() throws Exception {
335335
allowBindDeviceAdminForUser(appContext, adminComponent, /* userId= */ 0);
336336
binding =
337337
newBuilder()
338-
.setTargetUserHandle(UserHandle.getUserHandleForUid(/* userId= */ 0))
338+
.setTargetUserHandle(UserHandle.getUserHandleForUid(/* uid= */ 0))
339339
.setTargetUserHandle(generateUserHandle(/* userId= */ 0))
340340
.setChannelCredentials(BinderChannelCredentials.forDevicePolicyAdmin(adminComponent))
341341
.build();

0 commit comments

Comments
 (0)