Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Examples: Add a JWT authentication example #5915

Merged
merged 18 commits into from Mar 18, 2020
Merged

Examples: Add a JWT authentication example #5915

merged 18 commits into from Mar 18, 2020

Conversation

anarsultanov
Copy link
Contributor

@anarsultanov anarsultanov commented Jun 21, 2019 •
edited

@thelinuxfoundation
Copy link

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,
CLA GitHub bot

dapengzhang0
dapengzhang0 reviewed Jun 21, 2019
View reviewed changes
examples/build.gradle Outdated Show resolved Hide resolved
@anarsultanov anarsultanov force-pushed the #5665-authentication-example branch 2 times, most recently from 8f238cd to c8a7842 Compare June 22, 2019 15:26
dapengzhang0
dapengzhang0 reviewed Jun 26, 2019
View reviewed changes
examples/README.md Outdated Show resolved Hide resolved
examples/example-jwt-auth/README.md Outdated Show resolved Hide resolved
examples/example-jwt-auth/build.gradle Outdated Show resolved Hide resolved
examples/example-jwt-auth/src/test/java/io/grpc/examples/authentication/AuthClientTest.java Outdated Show resolved Hide resolved
...les/example-jwt-auth/src/main/java/io/grpc/examples/authentication/JwtServerInterceptor.java Outdated Show resolved Hide resolved
examples/example-jwt-auth/src/main/java/io/grpc/examples/authentication/Constant.java Outdated Show resolved Hide resolved
examples/example-jwt-auth/src/main/java/io/grpc/examples/authentication/AuthServer.java Outdated Show resolved Hide resolved
examples/example-jwt-auth/src/main/java/io/grpc/examples/authentication/AuthClient.java Outdated Show resolved Hide resolved
examples/example-jwt-auth/src/main/java/io/grpc/examples/authentication/BearerToken.java Outdated Show resolved Hide resolved
examples/example-jwt-auth/src/main/java/io/grpc/examples/authentication/AuthServer.java Outdated Show resolved Hide resolved
@dapengzhang0 dapengzhang0 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Jul 2, 2019
@grpc-kokoro grpc-kokoro removed kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run labels Jul 2, 2019
@dapengzhang0 dapengzhang0 requested a review from ejona86 July 2, 2019 20:25
@dapengzhang0
Copy link
Member

ping reviewers

@dapengzhang0
Copy link
Member

@sanjaypujare @ejona86 ping

@linux-foundation-easycla
Copy link

CLA Check
The committers are authorized under a signed CLA.

@anarsultanov
Copy link
Contributor Author

@dapengzhang0 @sanjaypujare @ejona86 all comments are resolved. Anything else?

@dapengzhang0 dapengzhang0 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 6, 2019
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 6, 2019
@sanjaypujare
Copy link
Contributor

I haven't finished my review yet. Give me another day - sorry about the delay

sanjaypujare
sanjaypujare approved these changes Sep 11, 2019
View reviewed changes
Copy link
Contributor

@sanjaypujare sanjaypujare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. But chatting with @ejona86 it looks like he has some comments/questions so I would wait for his comments before merging this.

@sanjaypujare
Copy link
Contributor

This LGTM. But chatting with @ejona86 it looks like he has some comments/questions so I would wait for his comments before merging this.

Ping @ejona86

@anarsultanov
Copy link
Contributor Author

@ejona86 any plans to review / merge this?

@augi augi mentioned this pull request Mar 12, 2020
Closed
@voidzcy voidzcy requested a review from creamsoup March 12, 2020 17:24
ejona86
ejona86 approved these changes Mar 12, 2020
View reviewed changes
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

examples/example-jwt-auth/src/main/java/io/grpc/examples/jwtauth/AuthClient.java Outdated
HelloRequest request = HelloRequest.newBuilder().setName(name).build();

String jwt = getJwt(clientId); // build JWT
CallCredentials credentials = new BearerToken(jwt); // Wrap JWT in CallCredentials
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

examples/example-jwt-auth/src/main/java/io/grpc/examples/jwtauth/JwtServerInterceptor.java Outdated
.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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

examples/example-jwt-auth/src/main/java/io/grpc/examples/jwtauth/JwtServerInterceptor.java Outdated
}
}

serverCall.close(status, metadata);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/metadata/new Metadata()/

Using metadata will echo back the request metadata, which isn't appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

creamsoup
creamsoup approved these changes Mar 17, 2020
View reviewed changes
Copy link
Contributor

@creamsoup creamsoup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

examples/example-jwt-auth/build.gradle Show resolved Hide resolved
@voidzcy
Copy link
Contributor

voidzcy commented Mar 17, 2020 •
edited

I updated this PR according to @ejona86's comment (last three commits). PTAL.

@creamsoup
Copy link
Contributor

I updated this PR according to @ejona86's comment (last three commits). PTAL.

LGTM i think it is ready to merge!

@creamsoup creamsoup added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 18, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 18, 2020
@creamsoup creamsoup merged commit b7859e7 into grpc:master Mar 18, 2020
@creamsoup
Copy link
Contributor

finally it is merged, thanks @anarsultanov. This will help our users a lot!

@anarsultanov anarsultanov deleted the #5665-authentication-example branch May 18, 2020 06:47
dfawley pushed a commit to dfawley/grpc-java that referenced this pull request Jan 15, 2021
@anarsultanov @dfawley
examples: Add a JWT authentication example (grpc#5915)
5c3d2be
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@voidzcy voidzcy voidzcy left review comments

@dapengzhang0 dapengzhang0 dapengzhang0 left review comments

@creamsoup creamsoup creamsoup approved these changes

@ejona86 ejona86 ejona86 approved these changes

@sanjaypujare sanjaypujare sanjaypujare approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@anarsultanov @thelinuxfoundation @dapengzhang0 @sanjaypujare @voidzcy @creamsoup @ejona86 @grpc-kokoro