feat: defines ExternalAccountClient abstract class for external_account credentials #1030

bojeil-google · 2020-08-11T22:14:59Z

Base external account client is used to instantiate AuthClients for exchanging external account credentials for GCP access token and authorizing requests to GCP APIs.

The base class implements common logic for exchanging various type of external credentials via GCP STS token exchange endpoint for GCP access tokens. The logic of determining and retrieving the external credential based on the environment and credential_source in the credentials JSON file will be left for the subclasses.

…rary-nodejs

…nt credentials

codecov · 2020-08-11T22:15:05Z

Codecov Report

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

@@           Coverage Diff            @@
##             byoid    #1030   +/-   ##
========================================
  Coverage         ?   92.85%           
========================================
  Files            ?       24           
  Lines            ?     4884           
  Branches         ?      564           
========================================
  Hits             ?     4535           
  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 7f8b124...436067f. Read the comment docs.

* chore: updated samples/package.json [ci skip] * chore: updated CHANGELOG.md [ci skip] * chore: updated package.json Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/5f7f9c6d-c75a-4c60-8bb8-0026a14cead7/targets - [ ] To automatically regenerate this PR, check this box. Source-Link: googleapis/synthtool@94421c4

…brary-nodejs into external-account-client

google-cla · 2020-08-12T08:34:26Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-08-12T08:38:38Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2020-08-12T18:00:41Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/b742586e-df31-4aac-8092-78288e9ea8e7/targets - [ ] To automatically regenerate this PR, check this box. Source-Link: googleapis/synthtool@bd0deaa

bcoe

Left a few nits, this is looking solid to me.

bcoe · 2020-08-12T18:35:13Z

src/auth/externalclient.ts

+import {ClientAuthentication} from './oauth2common';
+
+/**
+ * The required token exchange grant_type: rfc8693#section-2.1


I appreciate the links to RFCs, this will be useful for future generations.

Yeah, definitely. Let me know anytime something is not clear. I can provide relevant links and context so the code is self-explanatory.

bcoe · 2020-08-12T18:36:17Z

src/auth/externalclient.ts

+/**
+ * Offset to take into account network delays and server clock skews.
+ */
+export const EXPIRATION_TIME_OFFSET = 5 * 60 * 1000;


Where does the 5 minute value come from? wondering if this is similar to where other clients landed.

It is defined here:

google-auth-library-nodejs/src/auth/oauth2client.ts

Line 456 in eb54ee9

opts.eagerRefreshThresholdMillis || 5 * 60 * 1000;

and used here

google-auth-library-nodejs/src/auth/oauth2client.ts

Line 1323 in eb54ee9

? expiryDate <= new Date().getTime() + this.eagerRefreshThresholdMillis

It defaults to 5mins but they allow it to be overridden.

I added a second optional object argument to constructor that takes in a similar RefreshOptions. So the default behavior here is now the same as oauth2client. That object can be extended with additional customization parameters in the future, if needed.

I updated the tests to cover this (custom threshold and option to force refresh or disable refresh on 401/403).

src/auth/externalclient.ts

bcoe · 2020-08-12T18:40:05Z

src/auth/externalclient.ts

+   * @param options The external account options object typically loaded
+   *   from the external account JSON credential file.
+   */
+  constructor(private readonly options: ExternalAccountClientOptions) {


do you know if the readonly modifier applies to accessing properties on the object? I fixed a variety of bugs in this library related to people modifying an object, not realizing it's passed by reference.

If you mean:

client.options.audience = 'overrwrite';

Unfortunately, this will not throw.
I guess readonly acts more like a const for a private or public property.

client.options = 'this will throw';

Though, even then, there are workarounds for that:

Object.defineProperty(client, 'options', { value: 'overrwrite of entire options', });

I find it good practice to use it as it ensures I don't overwrite private and public properties internally that should not be overwritten.

Are you worried that developers could accidentally update the options content and break the behavior here? If so, I have updated the logic so that we just keep copies by value of the options content. So changes to that object will not break the behavior.

bcoe · 2020-08-12T18:41:21Z

src/auth/externalclient.ts

+        // - The response was a 401 or a 403
+        // - The request didn't send a readableStream
+        const isReadableStream = res.config.data instanceof stream.Readable;
+        const isAuthErr = statusCode === 401 || statusCode === 403;


Do we use this retry logic in a few places, it seems like something we should abstract into a helper.

Seems to be mainly in oauth2client which most other clients extend:

google-auth-library-nodejs/src/auth/oauth2client.ts

Line 950 in a7e5701

const isAuthErr = statusCode === 401 || statusCode === 403;

There may be some subtle differences but I can define requestAsync in the base class AuthClient which both classes extend. I can look into it in a separate dedicated PR as this PR is getting a bit too large.

I'd even be happy with a tracking issue, just seems like an area for some code cleanup.

…brary-nodejs into external-account-client

google-cla · 2020-08-14T18:10:59Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

…tions`.

google-cla · 2020-08-14T19:56:58Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

bcoe

LGTM.

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 <[email protected]> Co-authored-by: Benjamin E. Coe <[email protected]>

grayside and others added 4 commits August 7, 2020 16:32

refactor(samples): idtoken-cloudrun => idtoken-serverless (#1025)

11b0cb7

docs: README updated with new sample paths

4df48df

Merge branch 'byoid' of https://github.com/googleapis/google-auth-lib…

9f60e47

…rary-nodejs

feat: defines ExternalAccountClient abstract class for external_accou…

1a30988

…nt credentials

bojeil-google requested a review from a team as a code owner August 11, 2020 22:14

blunderbuss-gcf bot assigned bcoe Aug 11, 2020

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 11, 2020

chore: release 6.0.7 (#1023)

6dfee1d

* chore: updated samples/package.json [ci skip] * chore: updated CHANGELOG.md [ci skip] * chore: updated package.json Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

sofisl added automerge Merge the pull request once unit tests and other checks pass. and removed automerge Merge the pull request once unit tests and other checks pass. labels Aug 12, 2020

yoshi-automation and others added 3 commits August 12, 2020 05:22

Adds additional tests for tokens event emission on new access tokens.

6a5458b

Merge branch 'master' of https://github.com/googleapis/google-auth-li…

184b6a7

…brary-nodejs into external-account-client

google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Aug 12, 2020

bcoe added the cla: yes This human has signed the Contributor License Agreement. label Aug 12, 2020

google-cla bot removed the cla: no This human has *not* signed the Contributor License Agreement. label Aug 12, 2020

bcoe suggested changes Aug 12, 2020

View reviewed changes

renovate-bot and others added 2 commits August 12, 2020 14:04

fix(deps): roll back dependency google-auth-library to ^6.0.6 (#1033)

eb54ee9

Merge branch 'master' of https://github.com/googleapis/google-auth-li…

7b05460

…brary-nodejs into external-account-client

google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Aug 13, 2020

bojeil-google added 2 commits August 12, 2020 21:56

Addresses review comments.

9e06fa5

Javadoc fix and remove unncessary ! on options.

a1eb968

bcoe added the cla: yes This human has signed the Contributor License Agreement. label Aug 14, 2020

bcoe removed the cla: no This human has *not* signed the Contributor License Agreement. label Aug 14, 2020

bcoe approved these changes Aug 14, 2020

View reviewed changes

Removes unnecessary credential_source from `ExternalAccountClientOp…

436067f

…tions`.

google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Aug 14, 2020

bojeil-google requested a review from bcoe August 14, 2020 18:54

bcoe added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Aug 14, 2020

bcoe approved these changes Aug 14, 2020

View reviewed changes

bcoe merged commit 1174e4a into googleapis:byoid Aug 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: defines ExternalAccountClient abstract class for external_account credentials #1030

feat: defines ExternalAccountClient abstract class for external_account credentials #1030

bojeil-google commented Aug 11, 2020

codecov bot commented Aug 11, 2020 •

edited

Loading

google-cla bot commented Aug 12, 2020

googlebot commented Aug 12, 2020

google-cla bot commented Aug 12, 2020

bcoe left a comment

bcoe Aug 12, 2020

bojeil-google Aug 13, 2020

bcoe Aug 12, 2020

bojeil-google Aug 13, 2020

bcoe Aug 12, 2020

bojeil-google Aug 13, 2020

bcoe Aug 12, 2020

bojeil-google Aug 13, 2020

bcoe Aug 14, 2020

google-cla bot commented Aug 14, 2020

google-cla bot commented Aug 14, 2020

bcoe left a comment

feat: defines ExternalAccountClient abstract class for external_account credentials #1030

feat: defines ExternalAccountClient abstract class for external_account credentials #1030

Conversation

bojeil-google commented Aug 11, 2020

codecov bot commented Aug 11, 2020 • edited Loading

Codecov Report

google-cla bot commented Aug 12, 2020

googlebot commented Aug 12, 2020

google-cla bot commented Aug 12, 2020

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-cla bot commented Aug 14, 2020

google-cla bot commented Aug 14, 2020

bcoe left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 11, 2020 •

edited

Loading