Examples: Add a JWT authentication example#5915
Conversation
|
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
8f238cd to
c8a7842
Compare
c8a7842 to
3b8516c
Compare
|
ping reviewers |
|
@sanjaypujare @ejona86 ping |
|
@dapengzhang0 @sanjaypujare @ejona86 all comments are resolved. Anything else? |
|
I haven't finished my review yet. Give me another day - sorry about the delay |
sanjaypujare
left a comment
There was a problem hiding this comment.
This LGTM. But chatting with @ejona86 it looks like he has some comments/questions so I would wait for his comments before merging this.
|
@ejona86 any plans to review / merge this? |
ejona86
left a comment
There was a problem hiding this comment.
Some fairly easy changes, necessary, but otherwise LGTM. I'm most concerned about the BearerToken comment, since I've seen far too much code pass around plain jwt/oauth tokens without a way to refresh, and I don't want to inadvertently encourage that behavior.
| HelloRequest request = HelloRequest.newBuilder().setName(name).build(); | ||
|
|
||
| String jwt = getJwt(clientId); // build JWT | ||
| CallCredentials credentials = new BearerToken(jwt); // Wrap JWT in CallCredentials |
There was a problem hiding this comment.
The call credential should be responsible for making sure the JWT is valid, so building the JWT should really be part of the CallCredential, so that it can be re-created automatically before it expires. I see there is no expiration involved here, so maybe a comment here is enough. But given how simple getJwt() is, it seems much better to move that into the CallCredential and rename it to be JwtCredential or some such (just passing the clientId). (Feel free to create the JWT in the constructor.)
I accept that something like BearerToken is appropriate at times, but it's not something that is best copied for the majority of cases.
| .withValue(Constant.CLIENT_ID_CONTEXT_KEY, claims.getBody().getSubject()); | ||
| return Contexts.interceptCall(ctx, serverCall, metadata, serverCallHandler); | ||
| } catch (Exception e) { | ||
| status = Status.UNAUTHENTICATED.withDescription(e.getMessage()).withCause(e); |
There was a problem hiding this comment.
This error handling worries me. I'd feel much better if this try did not include Contexts.interceptCall(), since that will be arbitrary code and there is no telling if the exception message strings may leak inappropriate information.
I'd also be more comfortable if catch just caught JwtException (or MalformedJwtException+SignatureException+ExpiredJwtException).
| } | ||
| } | ||
|
|
||
| serverCall.close(status, metadata); |
There was a problem hiding this comment.
s/metadata/new Metadata()/
Using metadata will echo back the request metadata, which isn't appropriate.
|
I updated this PR according to @ejona86's comment (last three commits). PTAL. |
LGTM i think it is ready to merge! |
|
finally it is merged, thanks @anarsultanov. This will help our users a lot! |
#5665