-
Notifications
You must be signed in to change notification settings - Fork 535
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
Add JWT access token support. #572
Conversation
|
||
// 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.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
3 things to consider:
Bahhh.... The credential is ItoeknAccess and IConfigurableHttpClientInitializer!
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.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Ok, so:
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.
This comment was marked as spam.
Sorry, something went wrong.
Change looks much much better now, after creating ICredential. Gus, most of the change looks good to me, worth to add tests (when possible). |
Okay, I've addressed all the comments. |
} | ||
Logger.Info("New access token was received successfully"); | ||
} | ||
return Token.AccessToken; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
I received an LGTM offline. Fine to merge? |
LGTM |
Add JWT access token support.
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.