binder: describe security policy evaluation failures uniformly as 'Authorization future failed'

Author: AgraVator

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

@@ -72,11 +72,10 @@ public Status checkAuthorizationForService(int uid, String serviceName) {
   @CheckReturnValue
   ListenableFuture<Status> checkAuthorizationForServiceAsync(int uid, String serviceName) {
     SecurityPolicy securityPolicy = perServicePolicies.getOrDefault(serviceName, defaultPolicy);
-    if (securityPolicy instanceof AsyncSecurityPolicy) {
-      return ((AsyncSecurityPolicy) securityPolicy).checkAuthorizationAsync(uid);
-    }
-
     try {
+      if (securityPolicy instanceof AsyncSecurityPolicy) {
+        return ((AsyncSecurityPolicy) securityPolicy).checkAuthorizationAsync(uid);
+      }
       Status status = securityPolicy.checkAuthorization(uid);
       return Futures.immediateFuture(status);
     } catch (Exception e) {

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

@@ -110,11 +110,7 @@ public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(
         authStatus = Futures.getDone(authStatusFuture);
       } catch (ExecutionException | CancellationException e) {
         // Failed futures are treated as an internal error rather than a security rejection.
-        authStatus = Status.INTERNAL.withCause(e);
-        @Nullable String message = e.getMessage();
-        if (message != null) {
-          authStatus = authStatus.withDescription(message);
-        }
+        authStatus = Status.INTERNAL.withCause(e).withDescription("Authorization future failed");
       }
 
       if (authStatus.isOk()) {

binder/src/test/java/io/grpc/binder/RobolectricBinderSecurityTest.java

@@ -161,10 +161,29 @@ public void testAsyncServerSecurityPolicy_failed_returnsFailureStatus() throws E
 
   @Test
   public void testAsyncServerSecurityPolicy_failedFuture_failsWithCodeInternal() throws Exception {
-    ListenableFuture<Status> status = makeCall();
+    ListenableFuture<Status> status1 = makeCall();
     statusesToSet.take().setException(new IllegalStateException("oops"));
 
-    assertThat(status.get().getCode()).isEqualTo(Status.Code.INTERNAL);
+    Status result1 = status1.get();
+    assertThat(result1.getCode()).isEqualTo(Status.Code.INTERNAL);
+    assertThat(result1.getDescription()).isEqualTo("Authorization future failed");
+
+    // Subsequent call on the same channel should also get uniform opaque failure description
+    ListenableFuture<Status> status2 = makeCall();
+    Status result2 = status2.get();
+    assertThat(result2.getCode()).isEqualTo(Status.Code.INTERNAL);
+    assertThat(result2.getDescription()).isEqualTo("Authorization future failed");
+  }
+
+  @Test
+  public void testAsyncServerSecurityPolicy_cancelledFuture_failsWithUniformOpaqueDescription()
+      throws Exception {
+    ListenableFuture<Status> status1 = makeCall();
+    statusesToSet.take().cancel(true);
+
+    Status result1 = status1.get();
+    assertThat(result1.getCode()).isEqualTo(Status.Code.INTERNAL);
+    assertThat(result1.getDescription()).isEqualTo("Authorization future failed");
   }
 
   @Test

binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java

@@ -333,6 +333,44 @@ public void testPerServiceNoDefaultAsync() throws Exception {
         .isEqualTo(Status.PERMISSION_DENIED.getCode());
   }
 
+  @Test
+  public void testAsyncSecurityPolicy_throwsExceptionSynchronously_returnsFailedFuture() {
+    policy =
+        ServerSecurityPolicy.newBuilder()
+            .servicePolicy(
+                SERVICE1,
+                asyncPolicy(
+                    uid -> {
+                      throw new IllegalStateException("Sync policy error");
+                    }))
+            .build();
+
+    ListenableFuture<Status> future = policy.checkAuthorizationForServiceAsync(MY_UID, SERVICE1);
+    assertThat(future.isDone()).isTrue();
+    ExecutionException thrown = assertThrows(ExecutionException.class, future::get);
+    assertThat(thrown.getCause()).isInstanceOf(IllegalStateException.class);
+    assertThat(thrown.getCause().getMessage()).isEqualTo("Sync policy error");
+  }
+
+  @Test
+  public void testSecurityPolicy_throwsExceptionSynchronously_returnsFailedFuture() {
+    policy =
+        ServerSecurityPolicy.newBuilder()
+            .servicePolicy(
+                SERVICE1,
+                policy(
+                    uid -> {
+                      throw new IllegalArgumentException("Sync standard policy error");
+                    }))
+            .build();
+
+    ListenableFuture<Status> future = policy.checkAuthorizationForServiceAsync(MY_UID, SERVICE1);
+    assertThat(future.isDone()).isTrue();
+    ExecutionException thrown = assertThrows(ExecutionException.class, future::get);
+    assertThat(thrown.getCause()).isInstanceOf(IllegalArgumentException.class);
+    assertThat(thrown.getCause().getMessage()).isEqualTo("Sync standard policy error");
+  }
+
   /**
    * Shortcut for invoking {@link ServerSecurityPolicy#checkAuthorizationForServiceAsync} without
    * dealing with concurrency details. Returns a {link @Status.Code} for convenience.