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

Fixed issue with cache not retaining refresh token #333

Merged
merged 9 commits into from
Jan 16, 2020
Merged

Fixed issue with cache not retaining refresh token #333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a33e750
Fixed issue with cache not retaining refresh token
Jan 15, 2020
f40313e
Fix integration tests
Jan 15, 2020
cdde94e
Removed unused core-js import
Jan 15, 2020
9a113dd
Extracted 1 day in seconds value to a constant
Jan 15, 2020
b95dc7b
Applied comment to be consistant with related test
Jan 15, 2020
f042137
Applied brace styling for consistancy
Jan 15, 2020
fbff51c
Reworked expiry tests to mock date instead of using negative exp
Jan 15, 2020
7adaa6e
Added some comments to the cache tests to explain the test scenario
Jan 16, 2020
2f1a443
Cleaned up JS return statement styling
Jan 16, 2020
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
255 changes: 188 additions & 67 deletions __tests__/cache.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { InMemoryCache, LocalStorageCache } from '../src/cache';

const nowSeconds = () => Math.floor(Date.now() / 1000);
const dayInSeconds = 86400;

describe('InMemoryCache', () => {
let cache: InMemoryCache;
Expand Down Expand Up @@ -54,17 +55,56 @@ describe('InMemoryCache', () => {
user: { name: 'Test' }
}
});

// Test that the cache state is normal up until just before the expiry time..
jest.advanceTimersByTime(799);
expect(Object.keys(cache.cache).length).toBe(1);

// Advance the time to match the expiry time..
jest.advanceTimersByTime(1);

// and test that the cache has been emptied.
expect(Object.keys(cache.cache).length).toBe(0);
});

it('strips everything except the refresh token when expiry has been reached', () => {
cache.save({
client_id: 'test-client',
audience: 'the_audience',
scope: 'the_scope',
id_token: 'idtoken',
access_token: 'accesstoken',
refresh_token: 'refreshtoken',
expires_in: 1,
decodedToken: {
claims: {
__raw: 'idtoken',
name: 'Test',
exp: new Date().getTime() / 1000 + 2
},
user: { name: 'Test' }
}
});

// Test that the cache state is normal up until just before the expiry time..
jest.advanceTimersByTime(799);
expect(Object.keys(cache.cache).length).toBe(1);

// Advance the time to just past the expiry..
jest.advanceTimersByTime(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this call needed again if you already did this 2 lines above?

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 a preexisting test from before this work went in, but essentially it tests that the cache is in one state right up to the expiry time, then as soon as the time advances onto the expiry time the cache should be in another state (i.e. it's been removed by a setTimeout).

Here, 800ms is used because Luis had a calculation in that defines the expiry time as Math.min(expiresIn, expTime) * 1000 * 0.8, and the expiresIn for this test is defined as 1000ms.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think something of the above, simplified of course, can be put as a comment to illustrate this testing intention? So future me follows it

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!


// And test that the cache has been emptied, except for the refresh token
expect(cache.cache).toStrictEqual({
'@@auth0spajs@@::test-client::the_audience::the_scope': {
refresh_token: 'refreshtoken'
}
});
});

it('expires after `user.exp` when `user.exp` < `expires_in`', () => {
cache.save({
client_id: 'test-client',
audience: 'the_audiene',
audience: 'the_audience',
scope: 'the_scope',
id_token: 'idtoken',
access_token: 'accesstoken',
Expand All @@ -78,9 +118,15 @@ describe('InMemoryCache', () => {
user: { name: 'Test' }
}
});

// Test that the cache state is normal up until just before the expiry time..
jest.advanceTimersByTime(799);
expect(Object.keys(cache.cache).length).toBe(1);

// Advance the time to just past the expiry..
jest.advanceTimersByTime(1);

// And test that the cache has been emptied
expect(Object.keys(cache.cache).length).toBe(0);
});
});
Expand All @@ -93,6 +139,7 @@ describe('LocalStorageCache', () => {
beforeEach(() => {
cache = new LocalStorageCache();

jest.clearAllMocks();
jest.useFakeTimers();
localStorage.clear();
(<any>localStorage.removeItem).mockClear();
Expand All @@ -109,11 +156,11 @@ describe('LocalStorageCache', () => {
scope: '__TEST_SCOPE__',
id_token: '__ID_TOKEN__',
access_token: '__ACCESS_TOKEN__',
expires_in: 86400,
expires_in: dayInSeconds,
decodedToken: {
claims: {
__raw: 'idtoken',
exp: nowSeconds() + 86500,
exp: nowSeconds() + dayInSeconds + 100,
name: 'Test'
},
user: { name: 'Test' }
Expand All @@ -127,59 +174,67 @@ describe('LocalStorageCache', () => {
global.Date.now = realDateNow;
});

it('can set a value into the cache when expires_in < exp', () => {
cache.save(defaultEntry);

expect(localStorage.setItem).toHaveBeenCalledWith(
'@@auth0spajs@@::__TEST_CLIENT_ID__::__TEST_AUDIENCE__::__TEST_SCOPE__',
JSON.stringify({
body: defaultEntry,
expiresAt: nowSeconds() + 86400 - 60
})
);
});

it('can set a value into the cache when exp < expires_in', () => {
const entry = Object.assign({}, defaultEntry, {
expires_in: 86500,
decodedToken: {
claims: {
exp: nowSeconds() + 100
}
}
describe('cache.get', () => {
it('can retrieve an item from the cache', () => {
localStorage.setItem(
'@@auth0spajs@@::__TEST_CLIENT_ID__::__TEST_AUDIENCE__::__TEST_SCOPE__',
JSON.stringify({
body: defaultEntry,
expiresAt: nowSeconds() + dayInSeconds
})
);

expect(
cache.get({
client_id: '__TEST_CLIENT_ID__',
audience: '__TEST_AUDIENCE__',
scope: '__TEST_SCOPE__'
})
).toStrictEqual(defaultEntry);
});

cache.save(entry);

expect(localStorage.setItem).toHaveBeenCalledWith(
'@@auth0spajs@@::__TEST_CLIENT_ID__::__TEST_AUDIENCE__::__TEST_SCOPE__',
JSON.stringify({
body: entry,
expiresAt: nowSeconds() + 40
})
);
});

it('can retrieve an item from the cache', () => {
localStorage.setItem(
'@@auth0spajs@@::__TEST_CLIENT_ID__::__TEST_AUDIENCE__::__TEST_SCOPE__',
JSON.stringify({
body: defaultEntry,
expiresAt: nowSeconds() + 86400
})
);

expect(
cache.get({
client_id: '__TEST_CLIENT_ID__',
audience: '__TEST_AUDIENCE__',
scope: '__TEST_SCOPE__'
})
).toStrictEqual(defaultEntry);
});
it('returns undefined when there is no data', () => {
expect(cache.get({ scope: '', audience: '' })).toBeUndefined();
});

it('returns undefined when there is no data', () => {
expect(cache.get({ scope: '', audience: '' })).toBeUndefined();
it('strips the data, leaving the refresh token, when the expiry has been reached', () => {
localStorage.setItem(
'@@auth0spajs@@::__TEST_CLIENT_ID__::__TEST_AUDIENCE__::__TEST_SCOPE__',
JSON.stringify({
body: {
client_id: '__TEST_CLIENT_ID__',
audience: '__TEST_AUDIENCE__',
scope: '__TEST_SCOPE__',
id_token: '__ID_TOKEN__',
access_token: '__ACCESS_TOKEN__',
refresh_token: '__REFRESH_TOKEN__',
expires_in: 10,
decodedToken: {
claims: {
__raw: 'idtoken',
exp: nowSeconds() + 15,
name: 'Test'
},
user: { name: 'Test' }
}
},
expiresAt: nowSeconds() + 10
})
);

const now = nowSeconds();
global.Date.now = jest.fn(() => (now + 30) * 1000);

expect(
cache.get({
client_id: '__TEST_CLIENT_ID__',
audience: '__TEST_AUDIENCE__',
scope: '__TEST_SCOPE__'
})
).toStrictEqual({
refresh_token: '__REFRESH_TOKEN__'
});
});
});

it('expires after cache `expiresAt` when expiresAt < current time', () => {
Expand All @@ -192,20 +247,23 @@ describe('LocalStorageCache', () => {
scope: '__TEST_SCOPE__',
id_token: '__ID_TOKEN__',
access_token: '__ACCESS_TOKEN__',
expires_in: -10,
expires_in: 10,
decodedToken: {
claims: {
__raw: 'idtoken',
exp: nowSeconds() - 5,
exp: nowSeconds() + 15,
name: 'Test'
},
user: { name: 'Test' }
}
},
expiresAt: nowSeconds() - 10
expiresAt: nowSeconds() + 10
})
);

const now = nowSeconds();
global.Date.now = jest.fn(() => (now + 30) * 1000);

expect(
cache.get({
client_id: '__TEST_CLIENT_ID__',
Expand All @@ -219,22 +277,85 @@ describe('LocalStorageCache', () => {
);
});

it('deletes the cache item once the timeout has been reached', () => {
const entry = Object.assign({}, defaultEntry, {
expires_in: 120,
decodedToken: {
claims: {
exp: nowSeconds() + 240
describe('cache.save', () => {
it('can set a value into the cache when expires_in < exp', () => {
cache.save(defaultEntry);

expect(localStorage.setItem).toHaveBeenCalledWith(
'@@auth0spajs@@::__TEST_CLIENT_ID__::__TEST_AUDIENCE__::__TEST_SCOPE__',
JSON.stringify({
body: defaultEntry,
expiresAt: nowSeconds() + dayInSeconds - 60
})
);
});

it('can set a value into the cache when exp < expires_in', () => {
const entry = Object.assign({}, defaultEntry, {
expires_in: dayInSeconds + 100,
decodedToken: {
claims: {
exp: nowSeconds() + 100
}
}
}
});

cache.save(entry);

expect(localStorage.setItem).toHaveBeenCalledWith(
'@@auth0spajs@@::__TEST_CLIENT_ID__::__TEST_AUDIENCE__::__TEST_SCOPE__',
JSON.stringify({
body: entry,
expiresAt: nowSeconds() + 40
})
);
});

cache.save(entry);
it('deletes the cache item once the timeout has been reached', () => {
const entry = Object.assign({}, defaultEntry, {
expires_in: 120,
decodedToken: {
claims: {
exp: nowSeconds() + 240
}
}
});

// 96000, because the timeout time will be calculated at expires_in * 1000 * 0.8
jest.advanceTimersByTime(96000);
cache.save(entry);

expect(localStorage.removeItem).toHaveBeenCalled();
// 96000, because the timeout time will be calculated at expires_in * 1000 * 0.8
jest.advanceTimersByTime(96000);

expect(localStorage.removeItem).toHaveBeenCalled();
});

it('strips the cache data, leaving the refresh token, once the timeout has been reached', () => {
const exp = nowSeconds() + 240;
const expiresIn = nowSeconds() + 120;

const entry = Object.assign({}, defaultEntry, {
expires_in: 120,
refresh_token: 'refresh-token',
decodedToken: {
claims: {
exp
}
}
});

cache.save(entry);

// 96000, because the timeout time will be calculated at expires_in * 1000 * 0.8
jest.advanceTimersByTime(96000);

const payload = JSON.parse(
localStorage.getItem(
'@@auth0spajs@@::__TEST_CLIENT_ID__::__TEST_AUDIENCE__::__TEST_SCOPE__'
)
);

expect(payload.body).toStrictEqual({ refresh_token: 'refresh-token' });
});
});

it('removes the correct items when the cache is cleared', () => {
Expand Down
14 changes: 13 additions & 1 deletion __tests__/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1267,7 +1267,19 @@ describe('Auth0', () => {

await auth0.getTokenSilently();

//we only evaluate that the code didn't bail out because of the cache
// we only evaluate that the code didn't bail out because of the cache
expect(utils.encode).toHaveBeenCalledWith(TEST_RANDOM_STRING);
});

it('continues method execution when there is a value from the cache but no access token', async () => {
const { auth0, utils, cache } = await setup();

cache.get.mockReturnValue({});

await auth0.getTokenSilently();

// we only evaluate that the code didn't bail out because the cache didn't return
// an access token
expect(utils.encode).toHaveBeenCalledWith(TEST_RANDOM_STRING);
lbalmaceda marked this conversation as resolved.
Show resolved Hide resolved
});
});
Expand Down
5 changes: 3 additions & 2 deletions cypress/integration/getTokenSilently.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ describe('getTokenSilently', function() {
cy.toggleSwitch('local-storage');

cy.login().then(() => {
cy.reload();
cy.reload().wait(5000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there a new 5 second delay here?

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 intermittently fails because the UI isn’t ready yet, I’m hoping the delay will fix that. Seems to in the tests I’ve run locally, will see how it pans out over time.


cy.get('[data-cy=get-token]')
.click()
Expand All @@ -82,7 +82,8 @@ describe('getTokenSilently', function() {
cy.toggleSwitch('use-cache');

cy.login().then(() => {
cy.toggleSwitch('refresh-tokens').wait(250);
cy.toggleSwitch('refresh-tokens').wait(1000);
lbalmaceda marked this conversation as resolved.
Show resolved Hide resolved
win.localStorage.clear();

cy.get('[data-cy=get-token]')
.click()
Expand Down
Loading