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

feat: adds JwtCredentials with custom claims #290

Merged
merged 24 commits into from
Aug 5, 2019
Merged

feat: adds JwtCredentials with custom claims #290

merged 24 commits into from
Aug 5, 2019

Conversation

chingor13
Copy link
Contributor

@chingor13 chingor13 commented Jul 9, 2019

Adds the ability to create custom JWT Credentials (JwtCredentials) with custom claims from a ServiceAccountJwtAccessCredentials instance.

  • ServiceAccountJwtAccessCredentials will continue to provide on-demand credentials where the audience is based on the requested URI
  • Adds JwtCredentials class which can be directly built with a PrivateKey, privateKeyId string, and custom JwtCredentials.Claims. You can also build new JwtCredentials via JwtCredentials#jwtWithClaims(JwtCredentials.Claims) which only overrides the new specified claims fields.
  • Add ServiceAccountJwtAccessCredentials#jwtWithClaims(JwtCredentials.Claims) which will provide a JwtCredentials instance with custom claims
  • Changed the credentials cache to be keyed of the full Claims object rather than just the URI.

Fixes: #30

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 9, 2019
@chingor13 chingor13 requested a review from kolea2 July 9, 2019 17:59
@codecov
Copy link

codecov bot commented Jul 9, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@1a16f38). Click here to learn what that means.
The diff coverage is 82.78%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #290   +/-   ##
=========================================
  Coverage          ?   79.07%           
  Complexity        ?      357           
=========================================
  Files             ?       24           
  Lines             ?     1606           
  Branches          ?      168           
=========================================
  Hits              ?     1270           
  Misses            ?      251           
  Partials          ?       85
Impacted Files Coverage Δ Complexity Δ
.../google/auth/oauth2/ServiceAccountCredentials.java 81.21% <0%> (ø) 44 <0> (?)
...tp/java/com/google/auth/oauth2/JwtCredentials.java 84.21% <84.21%> (ø) 13 <13> (?)
...h2_http/java/com/google/auth/oauth2/JwtClaims.java 88.88% <88.88%> (ø) 8 <8> (?)
...uth/oauth2/ServiceAccountJwtAccessCredentials.java 75.52% <96.66%> (ø) 36 <2> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a16f38...7db400e. Read the comment docs.

@chingor13 chingor13 marked this pull request as ready for review July 11, 2019 18:44
@chingor13 chingor13 requested a review from a team as a code owner July 11, 2019 18:44
@chingor13 chingor13 changed the title WIP: Implement JwtCredentials with custom claims Implement JwtCredentials with custom claims Jul 11, 2019
@chingor13 chingor13 requested a review from theacodes July 12, 2019 16:35
Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

Choose a reason for hiding this comment

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

Why wouldn't you push the cache down as well?

@chingor13
Copy link
Contributor Author

Why wouldn't you push the cache down as well?

The cache in ServiceAccountJwtAccessCredentials is used because the credentials are reactionary to the request URI which is used as the audience (and thus handles many audiences). You hang onto the single instance of the ServiceAccountJwtAccessCredentials for all calls you make.

When you request a JwtCredentials with custom claims, you can hang onto that new Credentials instance which can refresh itself when it expires.

@igorbernstein2
Copy link
Contributor

The cache has a secondary benefit of async refresh as well

@igorbernstein2
Copy link
Contributor

Actually nevermind, I never added that part. Sorry for the noise

@chingor13 chingor13 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 15, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 15, 2019
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

This justifies a minor version bump to 0.17

oauth2_http/java/com/google/auth/oauth2/JwtProvider.java Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@chingor13 chingor13 added the semver: minor A new feature was added. No breaking changes. label Jul 16, 2019
@chingor13 chingor13 changed the title Implement JwtCredentials with custom claims feat: adds JwtCredentials with custom claims Jul 26, 2019
@chingor13
Copy link
Contributor Author

@kolea2, @igorbernstein2, @elharo any other concerns here?

@chingor13 chingor13 merged commit 3f37172 into googleapis:master Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. semver: minor A new feature was added. No breaking changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JWT token + getRequestMetadata(entry point)
7 participants