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

[v3] Add multi-site suffix to auth token keys #1208

Merged
merged 10 commits into from
May 18, 2023
Merged
Show file tree
Hide file tree
Changes from 7 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
8 changes: 4 additions & 4 deletions packages/commerce-sdk-react/src/auth/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ describe('Auth', () => {
expect(auth.get('refresh_token_guest')).toBe(refreshToken)
expect(auth.get('access_token')).toBe(accessToken)
// @ts-expect-error private property
expect([...auth.stores['cookie'].map.keys()]).toEqual([`siteId_cc-nx-g`])
expect([...auth.stores['cookie'].map.keys()]).toEqual([`cc-nx-g_siteId`])
// @ts-expect-error private property
expect([...auth.stores['local'].map.keys()]).toEqual([`siteId_access_token`])
expect([...auth.stores['local'].map.keys()]).toEqual([`access_token_siteId`])
})
test('set registered refresh token will clear guest refresh token, vise versa', () => {
const auth = new Auth(config)
Expand Down Expand Up @@ -300,7 +300,7 @@ describe('Auth', () => {
// @ts-expect-error private method
authA.set('refresh_token_guest', refreshTokenGuest)
// @ts-expect-error private property
expect([...authA.stores['memory'].map.keys()]).toEqual([`siteA_cc-nx-g`])
expect([...authA.stores['memory'].map.keys()]).toEqual([`cc-nx-g_siteA`])

// Create a second auth instance and ensure that its memory store has previous
// guest tokens set from the first store (this emulates a second lambda request.)
Expand All @@ -309,7 +309,7 @@ describe('Auth', () => {
authB.set('refresh_token_guest', refreshTokenGuest)

// @ts-expect-error private property
expect([...authB.stores['memory'].map.keys()]).toEqual([`siteA_cc-nx-g`, `siteB_cc-nx-g`])
expect([...authB.stores['memory'].map.keys()]).toEqual([`cc-nx-g_siteA`, `cc-nx-g_siteB`])

// Set mock value back to expected.
// @ts-expect-error read-only property
Expand Down
4 changes: 2 additions & 2 deletions packages/commerce-sdk-react/src/auth/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,9 +163,9 @@ class Auth {
fetchOptions: config.fetchOptions
})

const storageOptions = {keyPrefix: config.siteId}
const storageOptions = {keySuffix: config.siteId}
const serverStorageOptions = {
keyPrefix: config.siteId,
keySuffix: config.siteId,
sharedContext: true // This allows use to reused guest authentication tokens accross lambda runs.
}

Expand Down
5 changes: 2 additions & 3 deletions packages/commerce-sdk-react/src/auth/storage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,13 @@ const testCases = [
{
description: 'MemoryStorage works with options',
storageOptions: {
keyPrefix: 'prefix',
keyPrefixSeparator: '$'
keySuffix: 'suffix'
},
validate: (storage: BaseStorage) => {
storage.set(key, value)
expect(storage.get(key)).toBe(value)
// @ts-expect-error private property
expect([...storage.map.keys()]).toEqual([`prefix$${key}`])
expect([...storage.map.keys()]).toEqual([`${key}_suffix`])
storage.delete(key)
expect(storage.get(key)).toBe('')
}
Expand Down
58 changes: 28 additions & 30 deletions packages/commerce-sdk-react/src/auth/storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,23 @@ import Cookies from 'js-cookie'
export type StorageType = 'cookie' | 'local' | 'memory'

export interface BaseStorageOptions {
keyPrefix?: string
keyPrefixSeparator?: string
Copy link
Collaborator Author

@kevinxh kevinxh May 15, 2023

Choose a reason for hiding this comment

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

remove the option as changing the separator will break the compatibility with plugin_slas, we don't want to give user the option to do this.

keySuffix?: string
}

export interface MemoryStorageOptions extends BaseStorageOptions {
sharedContext?: boolean
}
export abstract class BaseStorage {
protected options: Required<BaseStorageOptions>
protected options: BaseStorageOptions = {}

constructor(options: BaseStorageOptions = {keyPrefixSeparator: '_'}) {
constructor(options?: BaseStorageOptions) {
this.options = {
keyPrefixSeparator: options.keyPrefix ? options.keyPrefixSeparator ?? '_' : '',
keyPrefix: options.keyPrefix ?? ''
keySuffix: options?.keySuffix ?? ''
}
}

protected getPrefixedKey(key: string): string {
return `${this.options.keyPrefix}${this.options.keyPrefixSeparator}${key}`
protected getSuffixedKey(key: string): string {
return this.options.keySuffix ? `${key}_${this.options.keySuffix}` : key
}
abstract set(key: string, value: string, options?: unknown): void
abstract get(key: string): string
Expand All @@ -49,16 +47,16 @@ export class CookieStorage extends BaseStorage {
}
}
set(key: string, value: string, options?: Cookies.CookieAttributes) {
const prefixedKey = this.getPrefixedKey(key)
Cookies.set(prefixedKey, value, {...options, secure: true})
const suffixedKey = this.getSuffixedKey(key)
Cookies.set(suffixedKey, value, {...options, secure: true})
}
get(key: string) {
const prefixedKey = this.getPrefixedKey(key)
return Cookies.get(prefixedKey) || ''
const suffixedKey = this.getSuffixedKey(key)
return Cookies.get(suffixedKey) || ''
}
delete(key: string) {
const prefixedKey = this.getPrefixedKey(key)
Cookies.remove(prefixedKey)
const suffixedKey = this.getSuffixedKey(key)
Cookies.remove(suffixedKey)
}
}

Expand All @@ -77,26 +75,26 @@ export class LocalStorage extends BaseStorage {
}
}
set(key: string, value: string) {
const prefixedKey = this.getPrefixedKey(key)
const oldValue = this.get(prefixedKey)
window.localStorage.setItem(prefixedKey, value)
const suffixedKey = this.getSuffixedKey(key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct me if I am wrong, it looks like we are replace prefix with suffix for all the keys in storage? I thought originally we only want to do it for site_id key, not every key.

Copy link
Collaborator Author

@kevinxh kevinxh May 16, 2023

Choose a reason for hiding this comment

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

there is only 1 use case for prefix/suffix and it is the siteId.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I get that for the side_id use case, but I was talking about if we add another key, with the changes here means it will apply for any key that need to have suffix? 🤔 , so basically we move from using prefix to suffix for any key that uses it?

Copy link
Collaborator Author

@kevinxh kevinxh May 17, 2023

Choose a reason for hiding this comment

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

that's right... unless there is a use case for prefix, this change moves from using prefix to suffix for all keys.

const oldValue = this.get(suffixedKey)
window.localStorage.setItem(suffixedKey, value)
const event = new StorageEvent('storage', {
key: prefixedKey,
key: suffixedKey,
oldValue: oldValue,
newValue: value
})
window.dispatchEvent(event)
}
get(key: string) {
const prefixedKey = this.getPrefixedKey(key)
return window.localStorage.getItem(prefixedKey) || ''
const suffixedKey = this.getSuffixedKey(key)
return window.localStorage.getItem(suffixedKey) || ''
}
delete(key: string) {
const prefixedKey = this.getPrefixedKey(key)
const oldValue = this.get(prefixedKey)
window.localStorage.removeItem(prefixedKey)
const suffixedKey = this.getSuffixedKey(key)
const oldValue = this.get(suffixedKey)
window.localStorage.removeItem(suffixedKey)
const event = new StorageEvent('storage', {
key: prefixedKey,
key: suffixedKey,
oldValue: oldValue,
newValue: null
})
Expand All @@ -114,15 +112,15 @@ export class MemoryStorage extends BaseStorage {
this.map = options?.sharedContext ? globalMap : new Map()
}
set(key: string, value: string) {
const prefixedKey = this.getPrefixedKey(key)
this.map.set(prefixedKey, value)
const suffixedKey = this.getSuffixedKey(key)
this.map.set(suffixedKey, value)
}
get(key: string) {
const prefixedKey = this.getPrefixedKey(key)
return this.map.get(prefixedKey) || ''
const suffixedKey = this.getSuffixedKey(key)
return this.map.get(suffixedKey) || ''
}
delete(key: string) {
const prefixedKey = this.getPrefixedKey(key)
this.map.delete(prefixedKey)
const suffixedKey = this.getSuffixedKey(key)
this.map.delete(suffixedKey)
}
}