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

Add JWT access token support. #572

Merged
merged 16 commits into from
Aug 11, 2015

Conversation

jtattermusch
Copy link
Contributor

This adds support for JWT access tokens to a special flavor of ServiceAccountCredential (named JwtServiceAccountCredential). This allows users to use JWT access tokens to authenticate with a service either directly (by using JwtServiceAccountCredential) or through Application Default Credentials stack.

the JwtServiceAccountCredential works in two modes:
-- if you don't add any scopes to it, it uses JWT access tokens to authenticate the requests.
-- once scopes are added, the behavior falls back to exactly what ServiceAccountCredential does.

If you are using GoogleCredentials, and IsCreateScopedRequired is true (no scopes present), you'll get the JWT access token behavior. After calling CreateScoped(), you'll get the old ServiceAccountCredential behavior. This is consistent to what implementations in other languages are doing (google nodeJS auth library for example).

A few remarks on the changes I've made:
-- a bit of refactoring in ServiceCredential, ServiceAccountCredential and UserCredential to expose ITokenAccess interface directly (which addresses one of the TODOs we've had)
-- the ITokenAccess has changed and now contains method GetAccessTokenForRequestAsync() which accepts auth URI parameter (required for JWT access token support).
-- stripped lots of code from GoogleCredential, thanks to the interface changes I've made.
-- semantics of ServiceAccountCredential is fully preserved so users shouldn't notice any change.


// Without adding scopes, the credential should be generating JWT scopes.
string accessToken = await (credential as ITokenAccess).GetAccessTokenForRequestAsync(DummyAuthUri);
var parts = accessToken.Split(new[] {'.'}, 3);

This comment was marked as spam.

This comment was marked as spam.

@peleyal
Copy link
Collaborator

peleyal commented Aug 7, 2015

3 things to consider:

  1. An interface ICredential
    return new GoogleCredential(credential, credential, credential);
    protected GoogleCredential(object credential, ITokenAccess tokenAccess, IConfigurableHttpClientInitializer clientInitializer)

Bahhh....

The credential is ItoeknAccess and IConfigurableHttpClientInitializer!
We should introduce a new interface that wraps the 3 together (we might want to include UnsuccessfulResponseHandler as well)

  1. JwtServiceAccount should be merged to ServiceAccount
    Not sure why we need Jwt specific class.
    Why do we need to declare a new JwtAccessToken class? We should use the TokenResponse.
  2. Some classes should be moved to Google.Apis.Auth (not dotnet4)
    https://github.com/google/google-api-dotnet-client/blob/592fef8270cc83f0ef9477069d8da9ba21f40cf5/Src/GoogleApis.Auth.DotNet4/OAuth2/ITokenAccess.cs
    and
    https://github.com/google/google-api-dotnet-client/blob/592fef8270cc83f0ef9477069d8da9ba21f40cf5/Src/GoogleApis.Auth.DotNet4/OAuth2/JsonCredentialParameters.cs
    They are not for a specific platform. They should be in the PCL project.

Let's chat ASAP, before I'm leaving.

}
}

var accessToken = await GetAccessTokenForRequestAsync(request.RequestUri.ToString(), cancellationToken);

This comment was marked as spam.

This comment was marked as spam.

@jtattermusch
Copy link
Contributor Author

Ok, so:

  1. I introduced the ICredential interface.
  2. Moved the classes you wanted (see my comment for JsonCredentialParameters)
  3. Got rid of JwtServiceAccountCredential.

I believe this addresses all the comments you've had.

private readonly ICredential credential;

/// <summary>Creates a new <c>GoogleCredential</c>.</summary>
internal GoogleCredential(ICredential credential)

This comment was marked as spam.

@peleyal
Copy link
Collaborator

peleyal commented Aug 8, 2015

Change looks much much better now, after creating ICredential.
I would be happy also to see how can we delete ServiceAccountGoogleCredential if it's possible.

Gus, most of the change looks good to me, worth to add tests (when possible).
Your opinion is valuable, I won't be able to have another review, so do your best :) 👍

@jtattermusch
Copy link
Contributor Author

Okay, I've addressed all the comments.
I wasn't able to remove the ServiceAccountGoogleClass in a way I would be happy with, so I just added explanatory comment why we need it (see one of the previous comments for further explanation).

@jtattermusch jtattermusch assigned gguuss and unassigned peleyal Aug 8, 2015
}
Logger.Info("New access token was received successfully");
}
return Token.AccessToken;

This comment was marked as spam.

This comment was marked as spam.

@jtattermusch
Copy link
Contributor Author

I received an LGTM offline. Fine to merge?

@gguuss
Copy link

gguuss commented Aug 11, 2015

LGTM

jtattermusch added a commit that referenced this pull request Aug 11, 2015
@jtattermusch jtattermusch merged commit 9405c89 into googleapis:master Aug 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants