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.

Sign up for GitHub

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

Jump to bottom

feat: Add ability to refresh token automatically before expiration. #208

Closed
wants to merge 20 commits into from
Closed

feat: Add ability to refresh token automatically before expiration. #208

wants to merge 20 commits into from

Conversation

nolanmar511
Copy link
Contributor

@nolanmar511 nolanmar511 commented Dec 15, 2017 •
edited
Loading

This pull request makes it possible to refresh the token when it is not expired but will expire in some period of time.
This can be used to address the issues described in #133, by allowing users to specify that tokens should be refreshed if they will expire soon. This is also necessary to fix googleapis/cloud-profiler-nodejs#94

ofrobots and others added 12 commits November 20, 2017 15:01
@ofrobots
Reland "chore: Switch from gulp to gts, TypeScript 2.6" (googleapis#186)
bd474fd 
This reverts commit 86c4190.
@JustinBeckwith
chore: upgrade to typed versions of jws and gtoken (googleapis#183)
7a0c748
@JustinBeckwith
chore: switch to axios (googleapis#182)
98b2b1a
@JustinBeckwith
chore: remove callbacks for sync APIs (googleapis#191)
e41e9d0
@JustinBeckwith
test: add asserts on authorize (googleapis#195)
8389e77
@JustinBeckwith
chore: use gcloud config-helper to get projectId (googleapis#194)
7466794
@JustinBeckwith
test: add typescript test (googleapis#192)
f5bd408
@JustinBeckwith
fix: add setCredentials method back to AuthClient (googleapis#198)
e2a3370
@JustinBeckwith
chore: export class types and remove properties from GoogleAuth (goog…
2bf0c28 
…leapis#202)

BREAKING CHANGE: The IAMAuth, Compute, JWT, JWTAccess, OAuth2, and UserRefreshClient types are no longer available as public properties on the GoogleAuth object.  They are now exported directly from the top level module.
@JustinBeckwith
fix: use urlencoded content-type for POST (googleapis#203)
c799c2d
@JustinBeckwith
1.0.0-alpha.1
38ac616
@JustinBeckwith
fix: retry checking isGCE (googleapis#193)
a5b107d
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 15, 2017
ofrobots
ofrobots reviewed Dec 18, 2017
View reviewed changes
ts/lib/auth/computeclient.ts Outdated
constructor() {
super();
constructor(refreshTokenEarlyMillis?: number) {
super(undefined, undefined, undefined, undefined, refreshTokenEarlyMillis);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@nolanmar511 nolanmar511 force-pushed the refresh-early branch 2 times, most recently from bea663e to ea2eda7 Compare December 22, 2017 15:53
@ofrobots ofrobots mentioned this pull request Dec 22, 2017
Merged
@nolanmar511
Copy link
Contributor Author

If I run the the travis tests on my fork of the master branch, without modifying anything, the travis tests fail in the same way.
The PRs which currently have passing tests seem to have different scripts in their package.json files.

How should I address the failing tests?

JustinBeckwith
JustinBeckwith reviewed Dec 22, 2017
View reviewed changes
ts/lib/auth/computeclient.ts Outdated
@@ -39,9 +39,11 @@ export class Compute extends OAuth2Client {
*
* Retrieve access token from the metadata server.
* See: https://developers.google.com/compute/docs/authentication
*
* @param {number=} refreshTokeEarlyMillis The token should be refreshed if it will expire within this many milliseconds.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

JustinBeckwith
JustinBeckwith suggested changes Dec 22, 2017
View reviewed changes
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.

Biggest feedback - this should be rebased and submitted against the next branch, and we should make the constructor changes Ali suggested. I'm happy to submit the constructor change as part of a separate PR if that's helpful.

ts/lib/auth/computeclient.ts Outdated
constructor() {
super();
constructor(refreshTokenEarlyMillis?: number) {
super(undefined, undefined, undefined, undefined, refreshTokenEarlyMillis);

This comment was marked as spam.

Sign in to view

ts/lib/auth/googleauth.ts Outdated
@@ -105,6 +105,11 @@ export class GoogleAuth {
*/
public static DefaultTransporter = DefaultTransporter;

/**
* @param {number=} refreshTokeEarlyMillis The token should be refreshed if it will expire within this many milliseconds.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ts/lib/auth/googleauth.ts Outdated
@@ -400,9 +406,12 @@ export class GoogleAuth {
// Set the JSON contents
this.jsonContent = json;
if (json.type === 'authorized_user') {
client = new UserRefreshClient();
client = new UserRefreshClient(
undefined, undefined, undefined, this.refreshTokenEarlyMillis);

This comment was marked as spam.

Sign in to view

ts/lib/auth/googleauth.ts Outdated
/**
* @param {number=} refreshTokeEarlyMillis The token should be refreshed if it will expire within this many milliseconds.
*/
constructor(private refreshTokenEarlyMillis?: number) {}

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ts/lib/auth/jwtclient.ts Outdated
@@ -44,12 +44,13 @@ export class JWT extends OAuth2Client {
* @param {string=} key value of key
* @param {(string|array)=} scopes list of requested scopes or a single scope.
* @param {string=} subject impersonated account's email address.
* @param {number=} refreshTokeEarlyMillis The token should be refreshed if it will expire within this many milliseconds.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ts/lib/auth/oauth2client.ts Outdated
@@ -52,17 +53,19 @@ export class OAuth2Client extends AuthClient {
* @param {string=} clientSecret The authentication client secret.
* @param {string=} redirectUri The URI to redirect to after completing the auth request.
* @param {Object=} opt_opts optional options for overriding the given parameters.
* @param {number=} refreshTokeEarlyMillis The token should be refreshed if it will expire within this many milliseconds.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ts/lib/auth/refreshclient.ts Outdated
@@ -29,10 +29,14 @@ export class UserRefreshClient extends OAuth2Client {
* @param {string} clientId The authentication client ID.
* @param {string} clientSecret The authentication client secret.
* @param {string} refreshToken The authentication refresh token.
* @param {number=} refreshTokeEarlyMillis The token should be refreshed if it will expire within this many milliseconds.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ts/test/test.oauth2.ts Outdated
expiry_date: (new Date()).getTime() + 3000
};

oauth2client.request({uri: 'http://example.com'}, () => {

This comment was marked as spam.

Sign in to view

@JustinBeckwith
Copy link
Contributor

Instead of trying to get the tests to pass on the master branch, please take a look at rebasing against the next branch.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot 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 Jan 3, 2018
@nolanmar511 nolanmar511 mentioned this pull request Jan 3, 2018
Closed
@nolanmar511 nolanmar511 closed this Jan 3, 2018
@nolanmar511
Copy link
Contributor Author

Opened PR #227, which has these changes on top of the next branch.

@nolanmar511 nolanmar511 mentioned this pull request Jan 10, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ofrobots ofrobots ofrobots left review comments

@JustinBeckwith JustinBeckwith JustinBeckwith requested changes

Assignees
No one assigned
Labels
cla: no This human has *not* signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"Unauthorized" error when credentials expire
5 participants
@nolanmar511 @JustinBeckwith @googlebot @ofrobots @dropdevcoding