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 label Jul 9, 2019
@chingor13 chingor13 requested a review from kolea2 Jul 9, 2019
@codecov
Copy link

@codecov 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 Jul 11, 2019
@chingor13 chingor13 requested a review from as a code owner Jul 11, 2019
@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 Jul 12, 2019
Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

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

@chingor13
Copy link
Contributor Author

@chingor13 chingor13 commented Jul 12, 2019

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

@igorbernstein2 igorbernstein2 commented Jul 12, 2019

The cache has a secondary benefit of async refresh as well

@igorbernstein2
Copy link
Contributor

@igorbernstein2 igorbernstein2 commented Jul 12, 2019

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

@chingor13 chingor13 added the kokoro:force-run label Jul 15, 2019
@kokoro-team kokoro-team removed the kokoro:force-run label Jul 15, 2019
Copy link
Contributor

@elharo elharo left a comment

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

@chingor13 chingor13 commented Jul 26, 2019

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

kolea2
kolea2 approved these changes Aug 5, 2019
@chingor13 chingor13 merged commit 3f37172 into googleapis:master Aug 5, 2019
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes semver: minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants