-
Notifications
You must be signed in to change notification settings - Fork 212
@ W-21066921 @W-21201637: Add HttpOnly session cookies for SLAS private client proxy #3680
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
Changes from 7 commits
c44ddf1
0f689e1
cbdfe90
fd41b17
b71897f
1184547
88427d9
7ab5c31
2980ddf
1e9bafe
9d46554
a22fa1c
a088f32
dbd0918
6090605
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 |
|---|---|---|
|
|
@@ -54,6 +54,8 @@ interface AuthConfig extends ApiClientConfigParams { | |
| refreshTokenRegisteredCookieTTL?: number | ||
| refreshTokenGuestCookieTTL?: number | ||
| hybridAuthEnabled?: boolean | ||
| /** When true, session tokens are set as HttpOnly cookies */ | ||
| useHttpOnlySessionCookies?: boolean | ||
| } | ||
|
|
||
| interface JWTHeaders { | ||
|
|
@@ -136,6 +138,7 @@ type AuthDataKeys = | |
| | 'uido' | ||
| | 'idp_refresh_token' | ||
| | 'dnt' | ||
| | 'access_token_expires_at' | ||
|
|
||
| type AuthDataMap = Record< | ||
| AuthDataKeys, | ||
|
|
@@ -250,6 +253,10 @@ const DATA_MAP: AuthDataMap = { | |
| uido: { | ||
| storageType: 'local', | ||
| key: 'uido' | ||
| }, | ||
| access_token_expires_at: { | ||
| storageType: 'local', | ||
| key: 'access_token_expires_at' | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -284,6 +291,7 @@ class Auth { | |
| | undefined | ||
|
|
||
| private hybridAuthEnabled: boolean | ||
| private useHttpOnlySessionCookies: boolean | ||
|
|
||
| constructor(config: AuthConfig) { | ||
| // Special proxy endpoint for injecting SLAS private client secret. | ||
|
|
@@ -380,6 +388,7 @@ class Auth { | |
| this.passwordlessLoginCallbackURI = config.passwordlessLoginCallbackURI || '' | ||
|
|
||
| this.hybridAuthEnabled = config.hybridAuthEnabled || false | ||
| this.useHttpOnlySessionCookies = config.useHttpOnlySessionCookies ?? false | ||
| } | ||
|
|
||
| get(name: AuthDataKeys) { | ||
|
|
@@ -517,6 +526,23 @@ class Auth { | |
| return validTimeSeconds <= tokenAgeSeconds | ||
| } | ||
|
|
||
| /** | ||
| * Returns whether the access token is expired. When useHttpOnlySessionCookies is true, | ||
| * uses access_token_expires_at from store; otherwise decodes the JWT from getAccessToken(). | ||
| */ | ||
| private isAccessTokenExpired(): boolean { | ||
| if (this.useHttpOnlySessionCookies) { | ||
| const expiresAt = this.get('access_token_expires_at') | ||
| if (expiresAt == null || expiresAt === '') return true | ||
| const expiresAtSec = Number(expiresAt) | ||
| if (Number.isNaN(expiresAtSec)) return true | ||
| const bufferSeconds = 60 | ||
|
Contributor
Author
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. The purpose of bufferSeconds is to avoid race conditions where a token is technically still valid when the check happens, but expires by the time the SCAPI request reaches the server. By refreshing 60 seconds early, you ensure the token is still valid when the downstream API processes the request. The original code has the same pattern |
||
| return Date.now() / 1000 >= expiresAtSec - bufferSeconds | ||
| } | ||
| const token = this.getAccessToken() | ||
| return !token || this.isTokenExpired(token) | ||
| } | ||
|
|
||
| /** | ||
| * Returns the SLAS access token or an empty string if the access token | ||
| * is not found in local store or if SFRA wants PWA to trigger refresh token login. | ||
|
|
@@ -680,33 +706,43 @@ class Auth { | |
| private handleTokenResponse(res: TokenResponse, isGuest: boolean) { | ||
| // Delete the SFRA auth token cookie if it exists | ||
| this.clearSFRAAuthToken() | ||
| this.set('access_token', res.access_token) | ||
|
|
||
| this.set('customer_id', res.customer_id) | ||
| this.set('enc_user_id', res.enc_user_id) | ||
| this.set('expires_in', `${res.expires_in}`) | ||
| this.set('id_token', res.id_token) | ||
| this.set('idp_access_token', res.idp_access_token) | ||
| this.set('token_type', res.token_type) | ||
| this.set('customer_type', isGuest ? 'guest' : 'registered') | ||
|
|
||
| const refreshTokenKey = isGuest ? 'refresh_token_guest' : 'refresh_token_registered' | ||
| const refreshTokenTTLValue = this.getRefreshTokenCookieTTLValue( | ||
| res.refresh_token_expires_in, | ||
| isGuest | ||
| ) | ||
| if (res.access_token) { | ||
| const {uido} = this.parseSlasJWT(res.access_token) | ||
| this.set('uido', uido) | ||
| } | ||
| const expiresDate = this.convertSecondsToDate(refreshTokenTTLValue) | ||
| this.set('refresh_token_expires_in', refreshTokenTTLValue.toString()) | ||
| this.set(refreshTokenKey, res.refresh_token, { | ||
| expires: expiresDate | ||
| }) | ||
| const expiresDate = this.convertSecondsToDate(refreshTokenTTLValue) | ||
| this.set('usid', res.usid ?? '', {expires: expiresDate}) | ||
|
|
||
| this.set('usid', res.usid, { | ||
| expires: expiresDate | ||
| }) | ||
| if (this.useHttpOnlySessionCookies) { | ||
| const uidoFromCookie = this.stores['cookie'].get('uido') | ||
| if (uidoFromCookie) this.set('uido', uidoFromCookie) | ||
| } else { | ||
| this.set('access_token', res.access_token) | ||
| this.set('idp_access_token', res.idp_access_token) | ||
| if (res.access_token) { | ||
| const {uido} = this.parseSlasJWT(res.access_token) | ||
| this.set('uido', uido) | ||
| } | ||
| const refreshTokenKey = isGuest ? 'refresh_token_guest' : 'refresh_token_registered' | ||
| this.set(refreshTokenKey, res.refresh_token, {expires: expiresDate}) | ||
| } | ||
| if ( | ||
| res && | ||
| typeof res === 'object' && | ||
| 'access_token_expires_at' in res && | ||
| res.access_token_expires_at != null | ||
| ) { | ||
| this.set('access_token_expires_at', String(res.access_token_expires_at)) | ||
| } | ||
| } | ||
|
|
||
| async refreshAccessToken() { | ||
|
|
@@ -747,7 +783,7 @@ class Auth { | |
|
|
||
| // refresh flow for TAOB | ||
| const accessToken = this.getAccessToken() | ||
| if (accessToken && this.isTokenExpired(accessToken)) { | ||
| if (this.isAccessTokenExpired()) { | ||
| try { | ||
| const {isGuest, usid, loginId, isAgent} = this.parseSlasJWT(accessToken) | ||
| if (isAgent) { | ||
|
|
@@ -888,8 +924,7 @@ class Auth { | |
| return await this.pendingToken | ||
| } | ||
|
|
||
| const accessToken = this.getAccessToken() | ||
| if (accessToken && !this.isTokenExpired(accessToken)) { | ||
| if (!this.isAccessTokenExpired()) { | ||
| return this.data | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,8 @@ export interface CommerceApiProviderProps extends ApiClientConfigParams { | |
| apiClients?: ApiClients | ||
| disableAuthInit?: boolean | ||
| hybridAuthEnabled?: boolean | ||
| /** When true, proxy returns tokens in HttpOnly cookies. */ | ||
| useHttpOnlySessionCookies?: boolean | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -145,12 +147,20 @@ const CommerceApiProvider = (props: CommerceApiProviderProps): ReactElement => { | |
| refreshTokenGuestCookieTTL, | ||
| apiClients, | ||
| disableAuthInit = false, | ||
| hybridAuthEnabled = false | ||
| hybridAuthEnabled = false, | ||
| useHttpOnlySessionCookies = false | ||
| } = props | ||
|
|
||
| // Set the logger based on provided configuration, or default to the console object if no logger is provided | ||
| const configLogger = logger || console | ||
|
|
||
| // When HttpOnly cookies are enabled, ensure fetch credentials allow cookies to be sent. | ||
| const effectiveFetchOptions = | ||
|
||
| useHttpOnlySessionCookies && | ||
| (!fetchOptions?.credentials || fetchOptions.credentials === 'omit') | ||
| ? {...fetchOptions, credentials: 'same-origin' as RequestCredentials} | ||
| : fetchOptions | ||
|
|
||
| const auth = useMemo(() => { | ||
| return new Auth({ | ||
| clientId, | ||
|
|
@@ -160,7 +170,7 @@ const CommerceApiProvider = (props: CommerceApiProviderProps): ReactElement => { | |
| proxy, | ||
| redirectURI, | ||
| headers, | ||
| fetchOptions, | ||
| fetchOptions: effectiveFetchOptions, | ||
| fetchedToken, | ||
| enablePWAKitPrivateClient, | ||
| privateClientProxyEndpoint, | ||
|
|
@@ -171,7 +181,8 @@ const CommerceApiProvider = (props: CommerceApiProviderProps): ReactElement => { | |
| passwordlessLoginCallbackURI, | ||
| refreshTokenRegisteredCookieTTL, | ||
| refreshTokenGuestCookieTTL, | ||
| hybridAuthEnabled | ||
| hybridAuthEnabled, | ||
| useHttpOnlySessionCookies | ||
| }) | ||
| }, [ | ||
| clientId, | ||
|
|
@@ -181,7 +192,7 @@ const CommerceApiProvider = (props: CommerceApiProviderProps): ReactElement => { | |
| proxy, | ||
| redirectURI, | ||
| headers, | ||
| fetchOptions, | ||
| effectiveFetchOptions, | ||
| fetchedToken, | ||
| enablePWAKitPrivateClient, | ||
| privateClientProxyEndpoint, | ||
|
|
@@ -193,7 +204,8 @@ const CommerceApiProvider = (props: CommerceApiProviderProps): ReactElement => { | |
| refreshTokenRegisteredCookieTTL, | ||
| refreshTokenGuestCookieTTL, | ||
| apiClients, | ||
| hybridAuthEnabled | ||
| hybridAuthEnabled, | ||
| useHttpOnlySessionCookies | ||
| ]) | ||
|
|
||
| const dwsid = auth.get(DWSID_COOKIE_NAME) | ||
|
|
@@ -212,7 +224,7 @@ const CommerceApiProvider = (props: CommerceApiProviderProps): ReactElement => { | |
| throwOnBadResponse: true, | ||
| fetchOptions: { | ||
| ...options.fetchOptions, | ||
| ...fetchOptions | ||
| ...effectiveFetchOptions | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -252,7 +264,7 @@ const CommerceApiProvider = (props: CommerceApiProviderProps): ReactElement => { | |
| currency | ||
| }, | ||
| throwOnBadResponse: true, | ||
| fetchOptions | ||
| fetchOptions: effectiveFetchOptions | ||
| } | ||
|
|
||
| return { | ||
|
|
@@ -279,7 +291,7 @@ const CommerceApiProvider = (props: CommerceApiProviderProps): ReactElement => { | |
| shortCode, | ||
| siteId, | ||
| proxy, | ||
| fetchOptions, | ||
| effectiveFetchOptions, | ||
| locale, | ||
| currency, | ||
| headers?.['correlation-id'], | ||
|
|
||
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 is just to avoid merge conflict when we merge in to develop branch