Skip to content
This repository was archived by the owner on Sep 26, 2023. It is now read-only.

Add opt-in support for JWT tokens #504

Merged
merged 3 commits into from
Apr 11, 2018

Conversation

igorbernstein2
Copy link
Contributor

@igorbernstein2 igorbernstein2 commented Mar 21, 2018

Currently JWT tokens are fairly hard to enable because GoogleCredentialsProvider will always set scopes on ServiceAccountCredentials. The only workaround is manually set
FixCredentials.of(ServiceAccountJwtAccessCredentials). This PR enables automatic JWT tokens support for services that opt into it.

The main limitation of JWT tokens is that they don't support scopes. So they can't be used when a user wants to explicitly limit a service account's permissions within a service. (ie. using https://www.googleapis.com/auth/bigtable.data.readonly instead of https://www.googleapis.com/auth/bigtable.data). This should be a minority of users...most will just use the DEFAULT_SERVICE_SCOPES in their settings.

To opt-in for JWT, a client will need to register scopes that are equivalent to having full access to the target service (ie. https://www.googleapis.com/auth/bigtable.data and https://www.googleapis.com/auth/cloud-platform).

The intended usage for this is that EnhancedBigtableStubSettings.Builder will:

  • create a new instance of GoogleCredentialsProvider
  • set BigtableStubSettings#getDefaultServiceScopes() on the new provider
  • set https://www.googleapis.com/auth/bigtable.data, https://www.googleapis.com/auth/cloud-bigtable.data and https://www.googleapis.com/auth/cloud-platform as JwtEnabledScopes
  • set the new provider as the CredentialsProvider in base settings

To enable testing, I had to add powermock to mock GoogleCredentials.getApplicationDefault() inside GoogleCredentialsProvider

If this works out well, I would recommend to deprecate and remove the ability for users to configure custom scopes and try to use JWT tokens as much as possible.

@codecov-io
Copy link

codecov-io commented Mar 21, 2018

Codecov Report

Merging #504 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #504      +/-   ##
============================================
- Coverage     72.53%   72.46%   -0.07%     
- Complexity      859      863       +4     
============================================
  Files           170      170              
  Lines          3462     3929     +467     
  Branches        277      305      +28     
============================================
+ Hits           2511     2847     +336     
- Misses          808      938     +130     
- Partials        143      144       +1
Impacted Files Coverage Δ Complexity Δ
...google/api/gax/core/GoogleCredentialsProvider.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...main/java/com/google/api/gax/rpc/StubSettings.java 76.78% <0%> (-16.48%) 10% <0%> (ø)
...oogle/api/gax/rpc/ServerStreamingCallSettings.java 67.27% <0%> (-14.95%) 6% <0%> (ø)
...ain/java/com/google/api/gax/core/Distribution.java 60% <0%> (-8.19%) 6% <0%> (ø)
...va/com/google/api/gax/batching/FlowController.java 78.12% <0%> (-3.85%) 17% <0%> (ø)
...rc/main/java/com/google/api/gax/rpc/Callables.java 66.66% <0%> (-2.3%) 7% <0%> (ø)
...m/google/api/gax/httpjson/HttpJsonCallContext.java 27.27% <0%> (-2.24%) 8% <0%> (ø)
.../com/google/api/gax/grpc/GrpcResponseMetadata.java 73.33% <0%> (-1.67%) 7% <0%> (ø)
.../java/com/google/api/gax/grpc/GrpcCallContext.java 82.05% <0%> (-1.63%) 38% <0%> (ø)
...ava/com/google/longrunning/OperationsSettings.java 29.03% <0%> (-0.97%) 2% <0%> (ø)
... and 64 more

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 1b6278b...4844558. Read the comment docs.

@igorbernstein2 igorbernstein2 changed the title Add opt-in support for JWT tokens DON'T MERGE (but please review): Add opt-in support for JWT tokens Mar 21, 2018
@igorbernstein2 igorbernstein2 changed the title DON'T MERGE (but please review): Add opt-in support for JWT tokens Add opt-in support for JWT tokens Apr 9, 2018
Currently JWT tokens are fairly hard to enable because GoogleCredentialsProvider will always set scopes on ServiceAccountCredentials. The only workaround is manually set
FixCredentials.of(ServiceAccountJwtAccessCredentials). This PR enables automatic JWT tokens support for services that opt into it.

The main limitation of JWT tokens is that they don't support scopes. So they can't be used when a user wants to explicitly limit a service account's permissions __within__ a service. (ie. using https://www.googleapis.com/auth/bigtable.data.readonly instead of https://www.googleapis.com/auth/bigtable.data). This should be a minority of users...most will just use the DEFAULT_SERVICE_SCOPES in their settings.

To opt-in for JWT a client will need to register scopes that are equivalent to having full access to the target service (ie. https://www.googleapis.com/auth/bigtable.data and https://www.googleapis.com/auth/cloud-platform).

To enable testing, I had to add powermock to mock GoogleCredentials.getApplicationDefault() inside GoogleCredentialsProvider
@igorbernstein2
Copy link
Contributor Author

igorbernstein2 commented Apr 9, 2018

Now that JWT caching is available in google-auth-library, this is ready for review. @garrettjonesgoogle PTAL

@pongad
Copy link
Contributor

pongad commented Apr 11, 2018

The class should be immutable, but it isn't. I created #526 to track this. If @igorbernstein2 wants to address it while working on this, please go ahead, but if not I'll fix it after this lands.

LGTM

package com.google.api.gax.core;

import static com.google.common.truth.Truth.*;
import static org.junit.Assert.*;

This comment was marked as spam.

This comment was marked as spam.

Copy link
Member

@garrettjonesgoogle garrettjonesgoogle left a comment

Choose a reason for hiding this comment

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

LGTM

@igorbernstein2
Copy link
Contributor Author

@pongad I'm not sure I understand what you mean..It's an AutoValue class.

Thanks for reviewing! I removed the star imports. Please merge when ready

@pongad
Copy link
Contributor

pongad commented Apr 11, 2018

@igorbernstein2 Sorry I didn't explain well. Currently you can set the fields with an ArrayList and mutate the list. If we want guaranteed immutability, we need to use ImmutableList.

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.

4 participants