Support AysncSecurityPolicy in SecurityPolicies.allOf.

[mateusazis]

Addresses part of #11547.

[jdcormie]

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

[jdcormie]

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.

[mateusazis]

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.

[jdcormie]

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.