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

Pace token refresh requests to avoid rate-limiting #79

Merged
merged 1 commit into from
Feb 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 6 additions & 12 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,30 +25,24 @@ If you want to contribute to the repository, follow these steps:
1. Fork the repo.
2. Install dependencies: `npm install`
3. Build the code: `npm run build`
4. Verify the build before beginning your changes: `npm run test-unit`
4. Verify the build before beginning your changes: `npm test`
2. Develop and test your code changes.
3. Travis-CI will run the tests for all services once your changes are merged.
4. Add a test for your changes. Only refactoring and documentation changes require no new tests.
5. Make the test pass.
5. Make sure all the tests pass.
2. Check code style with `npm run lint` and fix any issues.
6. Commit your changes. Remember the follow the correct commit message guidelines.
7. Push to your fork and submit a pull request.
3. Travis-CI will run the tests once your changes are merged.
8. Be sure to sign the CLA.

## Tests

Out of the box, `npm test` runs linting, unit tests, and integration tests (which require credentials).
To run only the unit tests (sufficient for most cases), use `npm run test-unit`.

To run the integration tests, you need to provide credentials to the integration test framework.
The integration test framework will skip integration tests for any service that does not have credentials,

To provide credentials for the integration tests, copy `test/resources/auth.example.js` to `test/resources/auth.js`
and fill in credentials for the service(s) you wish to test.
`npm test` runs the full suite of unit tests.
mkistler marked this conversation as resolved.
Show resolved Hide resolved

To run only specific tests, invoke jest with the `--testNamePattern` or `-t` flag:

```
npm run jest -- /test/integration -t "resources key"
npm run jest -- /test/unit -t "resources key"
```

This will only run tests that match the test name pattern you provide.
Expand Down
70 changes: 44 additions & 26 deletions auth/token-managers/jwt-token-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export class JwtTokenManager {
protected requestWrapperInstance: RequestWrapper;
private tokenInfo: any;
private expireTime: number;
private refreshTime: number;

/**
* Create a new [[JwtTokenManager]] instance.
Expand Down Expand Up @@ -97,6 +98,12 @@ export class JwtTokenManager {
return this.tokenInfo[this.tokenName];
});
} else {
// If refresh needed, kick one off
if (this.tokenNeedsRefresh()) {
this.requestToken().then(tokenResponse => {
this.saveTokenInfo(tokenResponse.result);
});
}
// 2. use valid, managed token
return Promise.resolve(this.tokenInfo[this.tokenName]);
}
Expand Down Expand Up @@ -142,8 +149,7 @@ export class JwtTokenManager {
}

/**
* Check if currently stored token is "expired"
* i.e. past the window to request a new token
* Check if currently stored token is expired
*
* @private
* @returns {boolean}
Expand All @@ -156,7 +162,28 @@ export class JwtTokenManager {
}

const currentTime = getCurrentTime();
return expireTime < currentTime;
return expireTime <= currentTime;
}

/**
* Check if currently stored token should be refreshed
* i.e. past the window to request a new token
*
* @private
* @returns {boolean}
*/
private tokenNeedsRefresh(): boolean {
const { refreshTime } = this;
const currentTime = getCurrentTime();

if (refreshTime && refreshTime > currentTime) {
return false;
}

// Update refreshTime to 60 seconds from now to avoid redundant refreshes
this.refreshTime = currentTime + 60;

return true;
}

/**
Expand All @@ -175,36 +202,27 @@ export class JwtTokenManager {
throw new Error(err);
}

this.expireTime = this.calculateTimeForNewToken(accessToken);
this.tokenInfo = extend({}, tokenResponse);
}

/**
* Decode the access token and calculate the time to request a new token.
*
* A time buffer prevents the edge case of the token expiring before the request could be made.
* The buffer will be a fraction of the total time to live - we are using 80%
*
* @param accessToken - JSON Web Token received from the service
* @private
* @returns {void}
*/
private calculateTimeForNewToken(accessToken): number {
// the time of expiration is found by decoding the JWT access token
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to integrate calculateTimeForNewToken into saveTokenInfo because we now need to set both the expireTime and refreshTime from the values in the decoded token.

// exp is the time of expire and iat is the time of token retrieval
let timeForNewToken;
const decodedResponse = jwt.decode(accessToken);
if (decodedResponse) {
const { exp, iat } = decodedResponse;
const fractionOfTtl = 0.8;
const timeToLive = exp - iat;
timeForNewToken = exp - (timeToLive * (1.0 - fractionOfTtl));
} else {
if (!decodedResponse) {
const err = 'Access token recieved is not a valid JWT'
logger.error(err);
throw new Error(err);
}

return timeForNewToken;
const { exp, iat } = decodedResponse;
// There are no required claims in JWT
if (!exp || !iat) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just extra bullet-proofing for the case where the JWT does not have an exp or iat claim.

this.expireTime = 0;
this.refreshTime = 0;
} else {
const fractionOfTtl = 0.8;
const timeToLive = exp - iat;
this.expireTime = exp;
this.refreshTime = exp - (timeToLive * (1.0 - fractionOfTtl));
}

this.tokenInfo = extend({}, tokenResponse);
}
}
58 changes: 20 additions & 38 deletions test/unit/iam-token-manager.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ RequestWrapper.mockImplementation(() => {
});

const ACCESS_TOKEN = '9012';
const CURRENT_ACCESS_TOKEN = '1234';
const IAM_RESPONSE = {
result: {
access_token: ACCESS_TOKEN,
Expand Down Expand Up @@ -62,14 +63,11 @@ describe('iam_token_manager_v1', function() {
const requestMock = jest.spyOn(instance, 'requestToken');

const currentTokenInfo = {
access_token: '1234',
refresh_token: '5678',
token_type: 'Bearer',
expires_in: 3600,
expiration: Math.floor(Date.now() / 1000),
access_token: CURRENT_ACCESS_TOKEN,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the iam-token-manager was changed to simply treat the access token as a JWT, none of the other fields in the IAM response are used, so we can eliminate them.

};

instance.tokenInfo = currentTokenInfo;
instance.expireTime = Math.floor(Date.now() / 1000) - 1;

mockSendRequest.mockImplementation(parameters => Promise.resolve(IAM_RESPONSE));

Expand All @@ -79,64 +77,48 @@ describe('iam_token_manager_v1', function() {
done();
});

it('should use a valid access token if one is stored', async done => {
it('should refresh an access token whose refreshTime has passed', async done => {
const instance = new IamTokenManager({ apikey: 'abcd-1234' });
const requestMock = jest.spyOn(instance, 'requestToken');

const currentTokenInfo = {
access_token: ACCESS_TOKEN,
refresh_token: '5678',
token_type: 'Bearer',
expires_in: 3600,
expiration: Math.floor(Date.now() / 1000) + 3000,
access_token: CURRENT_ACCESS_TOKEN,
};

instance.tokenInfo = currentTokenInfo;
instance.timeToLive = currentTokenInfo.expires_in;
instance.expireTime = currentTokenInfo.expiration;

const token = await instance.getToken();
expect(token).toBe(ACCESS_TOKEN);
expect(requestMock).not.toHaveBeenCalled();
done();
});
instance.expireTime = Math.floor(Date.now() / 1000) + 60;
instance.refreshTime = Math.floor(Date.now() / 1000) - 1;

it('should refresh an access token without expires_in field', async done => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was actually no longer valid, since the expires_in field is not used. It only happened to pass because the test did not set the expireTime in the instance.

const instance = new IamTokenManager({ apikey: 'abcd-1234' });
const requestMock = jest.spyOn(instance, 'requestToken');

const currentTokenInfo = {
access_token: '1234',
refresh_token: '5678',
token_type: 'Bearer',
expiration: Math.floor(Date.now() / 1000),
};

instance.tokenInfo = currentTokenInfo;
const requestTokenSpy = jest
.spyOn(instance, 'requestToken')
.mockImplementation(() => Promise.resolve({ result: { access_token: ACCESS_TOKEN } }));

mockSendRequest.mockImplementation(parameters => Promise.resolve(IAM_RESPONSE));

const token = await instance.getToken();
expect(token).toBe(ACCESS_TOKEN);
expect(token).toBe(CURRENT_ACCESS_TOKEN);
expect(requestMock).toHaveBeenCalled();
expect(requestTokenSpy).toHaveBeenCalled();

requestTokenSpy.mockRestore();
done();
});

it('should request a new token when refresh token does not have expiration field', async done => {
it('should use a valid access token if one is stored', async done => {
const instance = new IamTokenManager({ apikey: 'abcd-1234' });
const requestMock = jest.spyOn(instance, 'requestToken');

const currentTokenInfo = {
access_token: '1234',
refresh_token: '5678',
token_type: 'Bearer',
access_token: ACCESS_TOKEN,
};

instance.tokenInfo = currentTokenInfo;

mockSendRequest.mockImplementation(parameters => Promise.resolve(IAM_RESPONSE));
instance.expireTime = Math.floor(Date.now() / 1000) + 60 * 60;
instance.refreshTime = Math.floor(Date.now() / 1000) + 48 * 60;

const token = await instance.getToken();
expect(token).toBe(ACCESS_TOKEN);
expect(requestMock).not.toHaveBeenCalled();
done();
});

Expand Down
64 changes: 52 additions & 12 deletions test/unit/jwt-token-manager.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ function getCurrentTime() {
}

const ACCESS_TOKEN = 'abc123';
const CURRENT_ACCESS_TOKEN = 'abc123';

describe('JWT Token Manager', () => {
it('should initialize base variables', () => {
Expand All @@ -33,7 +34,7 @@ describe('JWT Token Manager', () => {

const decodeSpy = jest
.spyOn(jwt, 'decode')
.mockImplementation(token => ({ iat: 0, exp: 100 }));
.mockImplementation(token => ({ iat: 10, exp: 100 }));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't use iat = 0 because that triggers the new check in the jwt-token-manager.


const requestTokenSpy = jest
.spyOn(instance, 'requestToken')
Expand All @@ -51,14 +52,14 @@ describe('JWT Token Manager', () => {
done();
});

it('should request a token if token is stored but expired', async done => {
it('should request a token if token is stored but needs refresh', async done => {
const instance = new JwtTokenManager();
instance.tokenInfo.access_token = '987zxc';
instance.tokenInfo.access_token = CURRENT_ACCESS_TOKEN;

const saveTokenInfoSpy = jest.spyOn(instance, 'saveTokenInfo');
const decodeSpy = jest
.spyOn(jwt, 'decode')
.mockImplementation(token => ({ iat: 0, exp: 100 }));
.mockImplementation(token => ({ iat: 10, exp: 100 }));

const requestTokenSpy = jest
.spyOn(instance, 'requestToken')
Expand All @@ -68,7 +69,7 @@ describe('JWT Token Manager', () => {
expect(requestTokenSpy).toHaveBeenCalled();
expect(saveTokenInfoSpy).toHaveBeenCalled();
expect(decodeSpy).toHaveBeenCalled();
expect(token).toBe(ACCESS_TOKEN);
expect(token).toBe(CURRENT_ACCESS_TOKEN);

saveTokenInfoSpy.mockRestore();
decodeSpy.mockRestore();
Expand Down Expand Up @@ -132,6 +133,7 @@ describe('JWT Token Manager', () => {
const instance = new JwtTokenManager();
instance.tokenInfo.access_token = ACCESS_TOKEN;
instance.expireTime = getCurrentTime() + 1000;
instance.refreshTime = getCurrentTime() + 800;
const token = await instance.getToken();
expect(token).toBe(ACCESS_TOKEN);
done();
Expand Down Expand Up @@ -175,19 +177,40 @@ describe('JWT Token Manager', () => {
});
});

describe('tokenNeedsRefresh', () => {
it('should return true if current time is past refresh time', () => {
const instance = new JwtTokenManager();
instance.refreshTime = getCurrentTime() - 1000;

expect(instance.tokenNeedsRefresh()).toBe(true);
});

it('should return false if current time has not reached refresh time', () => {
const instance = new JwtTokenManager();
instance.refreshTime = getCurrentTime() + 1000;

expect(instance.tokenNeedsRefresh()).toBe(false);
});

it('should return true if refresh time has not been set', () => {
const instance = new JwtTokenManager();
expect(instance.tokenNeedsRefresh()).toBe(true);
});
});

describe('saveTokenInfo', () => {
it('should save information to object state', () => {
const instance = new JwtTokenManager();

const expireTime = 100;
instance.calculateTimeForNewToken = jest.fn(token => expireTime);
const decodeSpy = jest
.spyOn(jwt, 'decode')
.mockImplementation(token => ({ iat: 10, exp: 100 }));

const tokenResponse = { access_token: ACCESS_TOKEN };

instance.saveTokenInfo(tokenResponse);
expect(instance.expireTime).toBe(expireTime);
expect(instance.expireTime).toBe(100);
expect(instance.tokenInfo).toEqual(tokenResponse);
expect(instance.calculateTimeForNewToken).toHaveBeenCalledWith(ACCESS_TOKEN);
decodeSpy.mockRestore();
});

it('should throw an error when access token is undefined', () => {
Expand All @@ -203,15 +226,32 @@ describe('JWT Token Manager', () => {
const instance = new JwtTokenManager();
const decodeSpy = jest
.spyOn(jwt, 'decode')
.mockImplementation(token => ({ iat: 0, exp: 100 }));
.mockImplementation(token => ({ iat: 100, exp: 200 }));

const tokenResponse = { access_token: ACCESS_TOKEN };

expect(instance.calculateTimeForNewToken(ACCESS_TOKEN)).toBe(80);
instance.saveTokenInfo(tokenResponse);
expect(instance.refreshTime).toBe(180);
decodeSpy.mockRestore();
});

it('should throw an error if token is not a valid jwt', () => {
const instance = new JwtTokenManager();
expect(() => instance.calculateTimeForNewToken()).toThrow();
});

it('should gracefully handle a jwt without exp or iat claims', () => {
const instance = new JwtTokenManager();
const decodeSpy = jest
.spyOn(jwt, 'decode')
.mockImplementation(token => ({ foo: 0, bar: 100 }));

const tokenResponse = { access_token: ACCESS_TOKEN };

instance.saveTokenInfo(tokenResponse);
expect(instance.expireTime).toBe(0);
expect(instance.tokenInfo).toEqual(tokenResponse);
decodeSpy.mockRestore();
});
});
});