binder: Introduce server authorization strategy v2 #12398
Conversation
jdcormie
changed the title
jdcormie
force-pushed
the
isolated-process-3
branch
4 times, most recently
from
3ffbf7c to
9c52e3d
Compare
…12445) Avoids waiting for the handshake timeout on the server side in this case. I also add test coverage for the `!setOutgoingBinder()` case to make sure it works in the new location. My ulterior motive for this change is simplifying the client handshake code in preparation for #12398 -- An (impossible) !isShutdown() clause goes away for easy to understand reasons and I'll no longer have to pass the server's binder as an arg from async function to function in two separate handshake impls. Fixes #12438
jdcormie
force-pushed
the
isolated-process-3
branch
2 times, most recently
from
3469743 to
b6ae99c
Compare
Moves the existing handshake logic behind an interface using the strategy pattern. No functional change.
jdcormie
force-pushed
the
isolated-process-3
branch
from
b6ae99c to
ee2c7dc
Compare
|
@mateusazis Could you please take a look? You have already reviewed the first commit. I recommend just looking at the second one "Introduce server authorization strategy v2". I plan not to squash these two before merging. Thanks! |
binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java
Outdated
Show resolved
Hide resolved
binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java
Outdated
Show resolved
Hide resolved
…rpc#12445) Avoids waiting for the handshake timeout on the server side in this case. I also add test coverage for the `!setOutgoingBinder()` case to make sure it works in the new location. My ulterior motive for this change is simplifying the client handshake code in preparation for grpc#12398 -- An (impossible) !isShutdown() clause goes away for easy to understand reasons and I'll no longer have to pass the server's binder as an arg from async function to function in two separate handshake impls. Fixes grpc#12438
|
@ejona86: mateusazis has reviewed the code and ISE-mobile has reviewed the design. Would you please consider this for approval? |
| * | ||
| * @return this | ||
| */ | ||
| public BinderChannelBuilder useLegacyAuthStrategy() { |
There was a problem hiding this comment.
I was going to suggest @ExperimentalApi (especially if you plan to delete it after migration). But then I realized BinderChannelBuilder is experimental itself. Looks like we only stabilized BinderServerBuilder. Is the experimental channel builder still appropriate? Do we want this to be experimental anyway?
(Feel free to use the same #12397 issue for this method; it'll at least be a hint to what's going on.)
There was a problem hiding this comment.
OK I marked that method experimental too. The new method can be experimental even if the legacy strategy behind it is the same as it ever was.
I think BinderChannelBuilder should graduate out of experimental too. I can drive the stablization exercise if there is one. Looks like BinderServerBuilder's annotation was lifted in #9669. Do you have notes from the corresponding API review meeting? Perhaps BinderChannelBuilder was reviewed in that meeting too but just happened to not have any issues/comments.
There was a problem hiding this comment.
The notes from the API review meeting were #9669 (review) . It seems it wasn't a goal to stabilize the client-side. In our 2022-09-01 meeting I see some limited discussion about the client-side, as it was noted it doesn't support ChannelCredentials and there was a suggestion of using it to pass in android.context.Context or using a global. But that's it.
|
(I'm sorry about the Mac CI. It has gotten really bad recently. I am suspecting an infrastructure issue, as it just hangs in the middle of the build and increasing the timeout didn't help.) |
Adds support for android:isolatedProcess Services (fixing grpc#12293) and moves all peer authorization checks to the handshake, allowing subsequent transactions to go unchecked.
jdcormie
force-pushed
the
isolated-process-3
branch
from
888ee6e to
f0e9460
Compare
3 participants
Adds support for
android:isolatedProcessServices (fixing #12293) and moves all security checks to the handshake, making subsequent transactions more efficient.