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

Examples: Add a JWT authentication example #5915

Merged
merged 18 commits into from Mar 18, 2020

Conversation

anarsultanov
Copy link
Contributor

@anarsultanov anarsultanov commented Jun 21, 2019

@thelinuxfoundation
Copy link

@thelinuxfoundation thelinuxfoundation commented Jun 21, 2019

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

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 Jun 22, 2019
@anarsultanov anarsultanov force-pushed the #5665-authentication-example branch from c8a7842 to 3b8516c Compare Jun 23, 2019
@dapengzhang0 dapengzhang0 requested a review from sanjaypujare Jun 26, 2019
@dapengzhang0 dapengzhang0 added the kokoro:run label Jul 2, 2019
@grpc-kokoro grpc-kokoro removed kokoro:run labels Jul 2, 2019
@dapengzhang0 dapengzhang0 requested a review from ejona86 Jul 2, 2019
@dapengzhang0
Copy link
Member

@dapengzhang0 dapengzhang0 commented Aug 7, 2019

ping reviewers

@dapengzhang0
Copy link
Member

@dapengzhang0 dapengzhang0 commented Aug 26, 2019

@sanjaypujare @ejona86 ping

@linux-foundation-easycla
Copy link

@linux-foundation-easycla linux-foundation-easycla bot commented Aug 29, 2019

CLA Check
The committers are authorized under a signed CLA.

@anarsultanov
Copy link
Contributor Author

@anarsultanov anarsultanov commented Sep 6, 2019

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

@dapengzhang0 dapengzhang0 added the kokoro:run label Sep 6, 2019
@grpc-kokoro grpc-kokoro removed the kokoro:run label Sep 6, 2019
@sanjaypujare
Copy link
Contributor

@sanjaypujare sanjaypujare commented Sep 6, 2019

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

@anarsultanov anarsultanov requested a review from sanjaypujare Sep 10, 2019
Copy link
Contributor

@sanjaypujare sanjaypujare left a 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.

@sanjaypujare
Copy link
Contributor

@sanjaypujare sanjaypujare commented Sep 16, 2019

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

@anarsultanov anarsultanov commented Dec 3, 2019

@ejona86 any plans to review / merge this?

Copy link
Member

@ejona86 ejona86 left a 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
Copy link
Member

@ejona86 ejona86 Mar 12, 2020

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

@voidzcy voidzcy Mar 17, 2020

Choose a reason for hiding this comment

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

Fixed.

.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

@ejona86 ejona86 Mar 12, 2020

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

@voidzcy voidzcy Mar 17, 2020

Choose a reason for hiding this comment

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

Fixed.

}
}

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

@ejona86 ejona86 Mar 12, 2020

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

@voidzcy voidzcy Mar 17, 2020

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

@creamsoup creamsoup left a comment

LGTM

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

@voidzcy voidzcy commented Mar 17, 2020

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

@creamsoup
Copy link
Contributor

@creamsoup creamsoup commented Mar 17, 2020

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 label Mar 18, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run label Mar 18, 2020
@creamsoup creamsoup merged commit b7859e7 into grpc:master Mar 18, 2020
13 checks passed
@creamsoup
Copy link
Contributor

@creamsoup creamsoup commented Mar 18, 2020

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

@anarsultanov anarsultanov deleted the #5665-authentication-example branch May 18, 2020
dfawley pushed a commit to dfawley/grpc-java that referenced this issue Jan 15, 2021
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants