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: default self-signed JWTs #1054

Merged
merged 12 commits into from
Sep 22, 2020
Merged

feat: default self-signed JWTs #1054

merged 12 commits into from
Sep 22, 2020

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Sep 10, 2020

Start using self signed JWTs for client libraries when:

  • A service account is being used
  • A scope has not been set by the user
  • A target_audience has not been set by the user
  • An api_endpoint has not been set by the user

This will require upstream work in the client libraries to begin populating defaultScopes rather than scopes.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 10, 2020
@codecov
Copy link

codecov bot commented Sep 10, 2020

Codecov Report

Merging #1054 into master will increase coverage by 0.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1054      +/-   ##
==========================================
+ Coverage   91.52%   91.70%   +0.17%     
==========================================
  Files          21       21              
  Lines        4094     4145      +51     
  Branches      488      497       +9     
==========================================
+ Hits         3747     3801      +54     
+ Misses        347      344       -3     
Impacted Files Coverage Δ
src/auth/googleauth.ts 97.43% <100.00%> (+0.03%) ⬆️
src/auth/jwtaccess.ts 98.57% <100.00%> (+0.21%) ⬆️
src/auth/jwtclient.ts 97.04% <100.00%> (+0.95%) ⬆️

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 d835af7...bfa9e80. Read the comment docs.

@bshaffer
Copy link
Contributor

This looks great!

@bcoe bcoe marked this pull request as ready for review September 14, 2020 23:07
@bcoe bcoe requested a review from a team as a code owner September 14, 2020 23:07
Copy link
Contributor Author

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

I believe this is ready for review, when it comes time to integrate, the plan is that:

  • gax-nodejs will use the kDefaultScopes symbol to provide a default set of scopes. This allows us to differentiate between user-defined and gax-defined scopes, while discouraging users from setting the key themselves (as you need to go out of your way to do so.).
  • gax-nodejs will populate scopes rather than kDefaultScopes if an alternate servicePath has been set.

Note: there's an ongoing discussion about simpler audience strings (this feature will be implemented in a future PR.).

Update: I ended up simplifying the implementation and going back to a string, based on Alex's feedback.

Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

First pass. Looks great! Mostly just questions.

src/auth/jwtaccess.ts Outdated Show resolved Hide resolved
test/test.jwt.ts Show resolved Hide resolved
src/auth/jwtaccess.ts Outdated Show resolved Hide resolved
@bshaffer
Copy link
Contributor

bshaffer commented Sep 15, 2020

Should || this.defaultScopes be added at this line as well?

@bcoe
Copy link
Contributor Author

bcoe commented Sep 22, 2020

Should || this.defaultScopes be added at this line as well?

I had missed setting defaultScopes in a couple places -- I'm hoping I caught them all now.

@bcoe bcoe merged commit b4d139d into master Sep 22, 2020
@bcoe bcoe deleted the default_scopes branch September 22, 2020 17:29
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants