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: implements the OAuth token exchange spec based on rfc8693 #1026

Merged
merged 5 commits into from
Aug 11, 2020
Merged

feat: implements the OAuth token exchange spec based on rfc8693 #1026

merged 5 commits into from
Aug 11, 2020

Conversation

bojeil-google
Copy link
Contributor

Implements an internal utility for exchanging OAuth tokens using the rfc/8693 spec.

@bojeil-google bojeil-google requested a review from a team as a code owner August 7, 2020 06:45
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 7, 2020
@codecov
Copy link

codecov bot commented Aug 7, 2020

Codecov Report

❗ No coverage uploaded for pull request base (byoid@3f1368d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             byoid    #1026   +/-   ##
========================================
  Coverage         ?   92.32%           
========================================
  Files            ?       23           
  Lines            ?     4550           
  Branches         ?      524           
========================================
  Hits             ?     4201           
  Misses           ?      349           
  Partials         ?        0           

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 3f1368d...2dd4ae5. Read the comment docs.

Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

LGTM with a few nits

* https://tools.ietf.org/html/rfc8693#section-2.1
*/
export interface StsCredentialsOptions {
grantType: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if the spec lays these out clearly, but I like to pull whatever text they have (if of appropriate value and length) to include in comments above the interface properties. Like:

/**
 * REQUIRED.  The value "urn:ietf:params:oauth:grant-type:token-exchange" indicates that a token exchange is being performed.
*/
grantType: string;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

actor_token_type?: string;
client_id?: string;
client_secret?: string;
[key: string]: string | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

That is less than ideal :/ It effectively means any string key is value here with a string value. Is this really an open property bag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Currently there is only one non-standard field here. I manually added it.

opts
);
// Successful response.
const stsSuccessfulResponse = response.data;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious - why return the response object along with the data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly following the existing pattern established here and elsewhere in that file.

} catch (error) {
// Translate error to OAuthError.
if (error.response) {
throw getErrorFromOAuthErrorResponse(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to double check - does this method create a new Error, or modify the message on the existing? I want to be about not creating a new error, because we want to preserve the original stack trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was creating a new error and not preserving the original error data. I have extended this to preserve the original error data (including stack) but to modify the error message.

@bcoe
Copy link
Contributor

bcoe commented Aug 11, 2020

@bojeil-google if you rebase with master I believe it will address the docs failures you're seeing.

@JustinBeckwith JustinBeckwith merged commit 7f8b124 into googleapis:byoid Aug 11, 2020
bcoe added a commit that referenced this pull request Feb 6, 2021
feat: implements the OAuth token exchange spec based on rfc8693 (#1026)
feat: defines ExternalAccountClient abstract class for external_account credentials (#1030)
feat: adds service account impersonation to `ExternalAccountClient` (#1041)
feat: defines `IdentityPoolClient` used for K8s and Azure workloads (#1042)
feat: implements AWS signature version 4 for signing requests (#1047)
feat: defines `ExternalAccountClient` used to instantiate external account clients (#1050)
feat!: integrates external_accounts with `GoogleAuth` and ADC (#1052)
feat: adds text/json credential_source support to IdentityPoolClients (#1059)
feat: get AWS region from environment variable (#1067)
Co-authored-by: Wilfred van der Deijl <wilfred@vanderdeijl.com>
Co-authored-by: Benjamin E. Coe <bencoe@google.com>
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.

6 participants