-
Notifications
You must be signed in to change notification settings - Fork 1
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
token retrieval without init #102
Changes from 22 commits
c0ef4a1
e8639d4
0a33657
bd98e5f
3af320e
b352521
2367236
e41238c
0698d87
33d978c
1fad72d
3026d31
a46d20f
4a21945
a866a10
fdfbd2b
c080e41
ee870a0
4dd3792
fd5a423
bea4d2f
3888be8
08c4c70
25b73d0
a4ea126
5e60441
c607112
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,28 @@ function enrichIdentity(identity: LegacySDKCookie, now: number) { | |
}; | ||
} | ||
|
||
function getCookie(cookieName: string) { | ||
const docCookie = document.cookie; | ||
if (docCookie) { | ||
const payload = docCookie.split('; ').find((row) => row.startsWith(cookieName + '=')); | ||
if (payload) { | ||
return decodeURIComponent(payload.split('=')[1]); | ||
} | ||
} | ||
} | ||
|
||
export function loadIdentityFromCookieNoLegacy( | ||
cookieName: string | ||
): Identity | OptoutIdentity | null { | ||
const payload = getCookie(cookieName); | ||
if (payload) { | ||
const result = JSON.parse(payload) as unknown; | ||
if (isValidIdentity(result)) return result; | ||
if (isOptoutIdentity(result)) return result; | ||
} | ||
return null; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spoke with Lionell about making this a separate function from loadIdentityFromCookie in the CookieManager since we don't want them doing any setting (including migrating and setting the legacy cookie) without init, just getting |
||
export class CookieManager { | ||
private _opts: CookieOptions; | ||
private _cookieName: string; | ||
|
@@ -63,13 +85,7 @@ export class CookieManager { | |
';expires=Tue, 1 Jan 1980 23:59:59 GMT'; | ||
} | ||
private getCookie() { | ||
const docCookie = document.cookie; | ||
if (docCookie) { | ||
const payload = docCookie.split('; ').find((row) => row.startsWith(this._cookieName + '=')); | ||
if (payload) { | ||
return decodeURIComponent(payload.split('=')[1]); | ||
} | ||
} | ||
return getCookie(this._cookieName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this is conventional or not to have these two functions be the same name - one inside the class and one outside the class - happy to change the name if needed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't love it. It obviously works but it adds a little extra confusion. |
||
} | ||
|
||
private migrateLegacyCookie(identity: LegacySDKCookie, now: number): Identity { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,8 @@ import { StorageManager } from './storageManager'; | |
import { hashAndEncodeIdentifier } from './encoding/hash'; | ||
import { ProductDetails, ProductName } from './product'; | ||
import { storeConfig, updateConfig } from './configManager'; | ||
import { loadIdentityFromCookieNoLegacy } from './cookieManager'; | ||
import { loadIdentityFromLocalStorage } from './localStorageManager'; | ||
|
||
function hasExpired(expiry: number, now = Date.now()) { | ||
return expiry <= now; | ||
|
@@ -121,10 +123,12 @@ export abstract class SdkBase { | |
} | ||
|
||
public getIdentity(): Identity | null { | ||
return this._identity && !this.temporarilyUnavailable() && !isOptoutIdentity(this._identity) | ||
? this._identity | ||
const identity = this._identity ?? this.getIdentityNoInit(); | ||
return identity && !this.temporarilyUnavailable(identity) && !isOptoutIdentity(identity) | ||
? identity | ||
: null; | ||
} | ||
|
||
// When the SDK has been initialized, this function should return the token | ||
// from the most recent refresh request, if there is a request, wait for the | ||
// new token. Otherwise, returns a promise which will be resolved after init. | ||
|
@@ -260,13 +264,9 @@ export abstract class SdkBase { | |
return this._identity && !hasExpired(this._identity.refresh_expires); | ||
} | ||
|
||
private temporarilyUnavailable() { | ||
if (!this._identity && this._apiClient?.hasActiveRequests()) return true; | ||
if ( | ||
this._identity && | ||
hasExpired(this._identity.identity_expires) && | ||
!hasExpired(this._identity.refresh_expires) | ||
) | ||
private temporarilyUnavailable(identity: Identity | OptoutIdentity | null | undefined) { | ||
if (!identity && this._apiClient?.hasActiveRequests()) return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you just put a comment in here indicating that this means that the identity is expired but refreshable? Might help the next person. |
||
if (identity && hasExpired(identity.identity_expires) && !hasExpired(identity.refresh_expires)) | ||
return true; | ||
return false; | ||
} | ||
|
@@ -457,6 +457,13 @@ export abstract class SdkBase { | |
); | ||
} | ||
|
||
private getIdentityNoInit() { | ||
return ( | ||
loadIdentityFromCookieNoLegacy(this._product.cookieName) ?? | ||
loadIdentityFromLocalStorage(this._product.localStorageKey) | ||
); | ||
} | ||
|
||
protected async callCstgAndSetIdentity( | ||
request: { emailHash: string } | { phoneHash: string }, | ||
opts: ClientSideIdentityOptions | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,9 +25,10 @@ export class StorageManager { | |
} | ||
|
||
public loadIdentity(): Identity | OptoutIdentity | null { | ||
return this._opts.useCookie | ||
? this._cookieManager.loadIdentityFromCookie() | ||
: this._localStorageManager.loadIdentityFromLocalStorage(); | ||
return ( | ||
this._cookieManager.loadIdentityFromCookie() ?? | ||
this._localStorageManager.loadIdentityFromLocalStorage() | ||
); | ||
} | ||
|
||
public setIdentity(identity: Identity) { | ||
|
@@ -63,10 +64,7 @@ export class StorageManager { | |
} | ||
|
||
this._localStorageManager.setValue(value); | ||
if ( | ||
this._opts.useCookie === false && | ||
this._localStorageManager.loadIdentityFromLocalStorage() | ||
) { | ||
if (!this._opts.useCookie && this._localStorageManager.loadIdentityFromLocalStorage()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this._opts.useCookie will always be false here. It's checking for true and returning above on line 61. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can also be undefined and if it is undefined, we would still want the cookie to be removed I believe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure. but above it's checking if (this._opts.useCookie) so if (!this._opts.useCookie) will necessarily be true at this point in the code. The previous code was specifically looking for 'false.' If you just remove the check it should also execute when undefined. |
||
this._cookieManager.removeCookie(this._opts); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This state was only used in one test, and isn't really relevant anymore since an advertising token can be found without init