Conversation
…nd not bound to only JWT
|
Shouldn't we set an auth handler when we do register a handler instead of being global to the server ? |
…not via the constructor of the gRPC server
Right. That would be better. I changed the API. |
|
@vietj |
|
can we also expect Oauth2 support and client support ? |
|
|
||
| User user(); | ||
|
|
||
| GrpcServerRequest<Req, Resp> setUser(User user); |
There was a problem hiding this comment.
I removed the setter from the interface but the getter is required. Otherwise a user of the API is unable to access the authenticated user. At this point there is no "Context" object for gRPC so I put the user in the request.
I added token credential support to the client. As for Oauth2 support I don't feel comfortable to add it at this stage. |
|
can you describe the issue for oauth2 ? it seems pretty ok to me, but pehraps I'm missing something |
|
@vietj I took a look at the existing webclient implementation https://github.com/vert-x3/vertx-web/blob/master/vertx-web-client/src/main/java/io/vertx/ext/web/client/impl/OAuth2AwareInterceptor.java |
vietj
left a comment
vietj
left a comment
There was a problem hiding this comment.
thanks for the contribution and sorry for the late review.
| * @return a reference to this, so the API can be used fluently | ||
| */ | ||
| @GenIgnore(GenIgnore.PERMITTED_TYPE) | ||
| GrpcClient withCredentials(Credentials credentials); |
There was a problem hiding this comment.
This should be named credentials, we prefer to use withXXX in builders
| GrpcServerRequestImpl<Req, Resp> grpcRequest = new GrpcServerRequestImpl<>(httpRequest, method.messageDecoder, method.messageEncoder, methodCall); | ||
| grpcRequest.init(); | ||
| GrpcAuthenticationHandler authHandler = method.authHandler; | ||
| if (authHandler != null) { |
There was a problem hiding this comment.
here we need to pause the grpc request and resume it after to avoid loosing messages
There was a problem hiding this comment.
Is this required because in ìnit additional handlers are being added?
There was a problem hiding this comment.
in general any server request we receive should be paused then resumed if an asynchronous operation is achieved between its reception and usage
There was a problem hiding this comment.
I think you can write a test for this, by sending messages in the client and have an authentication handler that delays the actual auth handler by a delay and check we got all messages on the server
| } | ||
| } | ||
|
|
||
| private Future<String> parseAuthorization(HttpServerRequest req, |
There was a problem hiding this comment.
we should use a result here and avoid relying on exception to communicate an authentication failure (and thus avoid GrpcException), the result should clearly be an enum like result and the gRPC code calling for authentication should set the corresponding status on the HTTP/2 response
There was a problem hiding this comment.
I agree. Failures should only be used for unexpected states. I started rewriting this to use an enum as suggested:
return Future.succeededFuture(GrpcAuthenticationResult.UNAUTHENTICATED);I however don't see how I can return the token string in the same method via:
return Future.succeededFuture(authorization.substring(idx + 1));Do have an example or a pointer for me on how to solve this?
There was a problem hiding this comment.
I will need to have a look deeper to make an elaborated suggestion here
|
|
||
| @Override | ||
| public GrpcClient credentials(Credentials credentials) { | ||
| if (credentials == null) { |
There was a problem hiding this comment.
I think null could be accepted to null out credentials
2 participants
Motivation:
This PR adds JWT support to the GrpcServer.
TODO: