Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

Ensure session expirations are Dates #132

Merged
merged 1 commit into from
Mar 16, 2021
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
- Throw a different error for a missing cookie upon OAuth return [#131](https://github.com/shopify/shopify-node-api/pull/131)
- Improved documentation for GraphQL and Rest Clients. [#123](https://github.com/Shopify/shopify-node-api/pull/123)
- Made Docs directory more browseable in GitHub. [#136](https://github.com/Shopify/shopify-node-api/pull/136)
- Make sure `CustomSessionStorage` converts the `expires` field from a string to `Date`. [#132](https://github.com/Shopify/shopify-node-api/pull/132)

### Fixed

Expand Down
9 changes: 9 additions & 0 deletions src/auth/session/storage/custom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,19 @@ export class CustomSessionStorage implements SessionStorage {
}
if (result) {
if (result instanceof Session) {
if (result.expires && typeof result.expires === 'string') {
result.expires = new Date(result.expires);
}

return result;
} else if (result instanceof Object && 'id' in result) {
let session = new Session(result.id as string);
session = {...session, ...result};

if (session.expires && typeof session.expires === 'string') {
session.expires = new Date(session.expires);
}
Comment on lines 43 to +48

Choose a reason for hiding this comment

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

Suggested change
let session = new Session(result.id as string);
session = {...session, ...result};
if (session.expires && typeof session.expires === 'string') {
session.expires = new Date(session.expires);
}
const session = Session.cloneSession(result, result.id);

and following part port to Session.cloneSession.

        if (session.expires && typeof session.expires === 'string') {
          session.expires = new Date(session.expires);
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't think that would work - we can't guarantee that result will be of type Session here, since it may have been loaded from something like a JSON.parse() call.

If we tried to call Session.clone with result it would break in Typescript, and we shouldn't expect the expires field to be a string there (it is typed as Date so it would have to be set when the session is created anyway).

Choose a reason for hiding this comment

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

@paulomarg I got it.
My suggestion is for the following reasons:

I wish I could improve the point that users have to convert the session.expires to Date type.
(This seems to be solved if session.isActive() is implemented)

session = {...session, ...result}; overwrites session.id with result.id.
If this assignment is OK, why does Session.cloneSession exist?


return session;
} else {
throw new ShopifyErrors.SessionStorageError(
Expand Down
282 changes: 195 additions & 87 deletions src/auth/session/test/custom.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,91 +4,199 @@ import {Session} from '../session';
import {CustomSessionStorage} from '../storage/custom';
import {SessionStorageError} from '../../../error';

test('can use custom session storage', async () => {
const sessionId = 'test_session';
let session: Session | undefined = new Session(sessionId);

let storeCalled = false;
let loadCalled = false;
let deleteCalled = false;
const storage = new CustomSessionStorage(
() => {
storeCalled = true;
return Promise.resolve(true);
},
() => {
loadCalled = true;
return Promise.resolve(session);
},
() => {
deleteCalled = true;
session = undefined;
return Promise.resolve(true);
},
);

await expect(storage.storeSession(session)).resolves.toBe(true);
expect(storeCalled).toBe(true);
storeCalled = false;

await expect(storage.loadSession(sessionId)).resolves.toEqual(session);
expect(loadCalled).toBe(true);
loadCalled = false;

await expect(storage.storeSession(session)).resolves.toBe(true);
expect(storeCalled).toBe(true);

await expect(storage.loadSession(sessionId)).resolves.toEqual(session);
expect(loadCalled).toBe(true);

await expect(storage.deleteSession(sessionId)).resolves.toBe(true);
expect(deleteCalled).toBe(true);
deleteCalled = false;

await expect(storage.loadSession(sessionId)).resolves.toBeUndefined();

// Deleting a non-existing session should work
await expect(storage.deleteSession(sessionId)).resolves.toBe(true);
expect(deleteCalled).toBe(true);
});

test('custom session storage failures and exceptions are raised', () => {
const sessionId = 'test_session';
const session = new Session(sessionId);

let storage = new CustomSessionStorage(
() => Promise.resolve(false),
() => Promise.resolve(undefined),
() => Promise.resolve(false),
);

expect(storage.storeSession(session)).resolves.toBe(false);
expect(storage.loadSession(sessionId)).resolves.toBeUndefined();
expect(storage.deleteSession(sessionId)).resolves.toBe(false);

storage = new CustomSessionStorage(
() => Promise.reject(new Error('Failed to store!')),
() => Promise.reject(new Error('Failed to load!')),
() => Promise.reject(new Error('Failed to delete!')),
);

const expectedStore = expect(storage.storeSession(session)).rejects;
expectedStore.toThrow(SessionStorageError);
expectedStore.toThrow(/Error: Failed to store!/);

const expectedLoad = expect(storage.loadSession(sessionId)).rejects;
expectedLoad.toThrow(SessionStorageError);
expectedLoad.toThrow(/Error: Failed to load!/);

const expectedDelete = expect(storage.deleteSession(sessionId)).rejects;
expectedDelete.toThrow(SessionStorageError);
expectedDelete.toThrow(/Error: Failed to delete!/);

storage = new CustomSessionStorage(
() => Promise.resolve(true),
() => Promise.resolve('this is not a Session' as any),
() => Promise.resolve(true),
);

expect(storage.loadSession(sessionId)).rejects.toThrow(SessionStorageError);
describe('custom session storage', () => {
test('can perform actions', async () => {
const sessionId = 'test_session';
let session: Session | undefined = new Session(sessionId);

let storeCalled = false;
let loadCalled = false;
let deleteCalled = false;
const storage = new CustomSessionStorage(
() => {
storeCalled = true;
return Promise.resolve(true);
},
() => {
loadCalled = true;
return Promise.resolve(session);
},
() => {
deleteCalled = true;
session = undefined;
return Promise.resolve(true);
},
);

await expect(storage.storeSession(session)).resolves.toBe(true);
expect(storeCalled).toBe(true);
storeCalled = false;

await expect(storage.loadSession(sessionId)).resolves.toEqual(session);
expect(loadCalled).toBe(true);
loadCalled = false;

await expect(storage.storeSession(session)).resolves.toBe(true);
expect(storeCalled).toBe(true);

await expect(storage.loadSession(sessionId)).resolves.toEqual(session);
expect(loadCalled).toBe(true);

await expect(storage.deleteSession(sessionId)).resolves.toBe(true);
expect(deleteCalled).toBe(true);
deleteCalled = false;

await expect(storage.loadSession(sessionId)).resolves.toBeUndefined();

// Deleting a non-existing session should work
await expect(storage.deleteSession(sessionId)).resolves.toBe(true);
expect(deleteCalled).toBe(true);
});

test('failures and exceptions are raised', () => {
const sessionId = 'test_session';
const session = new Session(sessionId);

let storage = new CustomSessionStorage(
() => Promise.resolve(false),
() => Promise.resolve(undefined),
() => Promise.resolve(false),
);

expect(storage.storeSession(session)).resolves.toBe(false);
expect(storage.loadSession(sessionId)).resolves.toBeUndefined();
expect(storage.deleteSession(sessionId)).resolves.toBe(false);

storage = new CustomSessionStorage(
() => Promise.reject(new Error('Failed to store!')),
() => Promise.reject(new Error('Failed to load!')),
() => Promise.reject(new Error('Failed to delete!')),
);

const expectedStore = expect(storage.storeSession(session)).rejects;
expectedStore.toThrow(SessionStorageError);
expectedStore.toThrow(/Error: Failed to store!/);

const expectedLoad = expect(storage.loadSession(sessionId)).rejects;
expectedLoad.toThrow(SessionStorageError);
expectedLoad.toThrow(/Error: Failed to load!/);

const expectedDelete = expect(storage.deleteSession(sessionId)).rejects;
expectedDelete.toThrow(SessionStorageError);
expectedDelete.toThrow(/Error: Failed to delete!/);

storage = new CustomSessionStorage(
() => Promise.resolve(true),
() => Promise.resolve('this is not a Session' as any),
() => Promise.resolve(true),
);

expect(storage.loadSession(sessionId)).rejects.toThrow(SessionStorageError);
});

it('converts the expiration date to a Date object', async () => {
const sessionId = 'test_session';
const expiration = new Date();
expiration.setDate(expiration.getDate() + 10);

let session: Session | undefined = new Session(sessionId);
session.expires = expiration;

const storage = new CustomSessionStorage(
() => {
(session as any).expires = (session as Session).expires?.toString();
return Promise.resolve(true);
},
() => {
return Promise.resolve(session);
},
() => {
session = undefined;
return Promise.resolve(true);
},
);

await storage.storeSession(session);
expect(typeof session.expires).toBe('string');

expect(await storage.loadSession(sessionId)).toEqual(session);
});

it('can load session from serialized object', async () => {
const sessionId = 'test_session';
const expiration = new Date();
expiration.setDate(expiration.getDate() + 10);

/* eslint-disable @typescript-eslint/naming-convention */
let session: Session | undefined = new Session(sessionId);
session.shop = 'test.myshopify.io';
session.state = '1234';
session.scope = 'read_products';
session.expires = expiration;
session.isOnline = true;
session.accessToken = '12356';
session.onlineAccessInfo = {
associated_user_scope: 'read_products',
expires_in: 12345,
associated_user: {
id: 54321,
account_owner: true,
collaborator: true,
email: 'not@email',
email_verified: true,
first_name: 'first',
last_name: 'last',
locale: 'en',
},
};
/* eslint-enable @typescript-eslint/naming-convention */

let serializedSession = '';
const storage = new CustomSessionStorage(
() => {
serializedSession = JSON.stringify(session);
return Promise.resolve(true);
},
() => {
return Promise.resolve(JSON.parse(serializedSession));
},
() => {
session = undefined;
return Promise.resolve(true);
},
);

expect(serializedSession).toHaveLength(0);
await storage.storeSession(session);
expect(serializedSession).not.toHaveLength(0);

expect(await storage.loadSession(sessionId)).toEqual(session);
});

it('allows empty fields in serialized object', async () => {
const sessionId = 'test_session';

let session: Session | undefined = new Session(sessionId);

let serializedSession = '';
const storage = new CustomSessionStorage(
() => {
serializedSession = JSON.stringify(session);
return Promise.resolve(true);
},
() => {
return Promise.resolve(JSON.parse(serializedSession));
},
() => {
session = undefined;
return Promise.resolve(true);
},
);

expect(serializedSession).toHaveLength(0);
await storage.storeSession(session);
expect(serializedSession).not.toHaveLength(0);

expect(await storage.loadSession(sessionId)).toEqual(session);
});
});