From d7a0a49cbd60917c656a1b1ff2a48fd82b2b47a4 Mon Sep 17 00:00:00 2001 From: Paulo Margarido <64600052+paulomarg@users.noreply.github.com> Date: Thu, 23 Nov 2023 11:26:45 -0500 Subject: [PATCH] Improvements on OAuth refactoring --- .../server/authenticate/admin/authenticate.ts | 214 ++++-------------- .../ensure-app-is-embedded-if-required.ts | 18 ++ ...-session-token-search-param-if-required.ts | 25 ++ .../authenticate/admin/helpers/index.ts | 3 + .../admin/helpers/redirect-to-bounce-page.ts | 19 ++ .../admin/strategies/auth-code-flow.ts | 137 ++++++++--- .../authenticate/admin/strategies/types.ts | 25 +- .../helpers/reject-bot-request.ts | 2 +- .../public/checkout/authenticate.ts | 4 +- 9 files changed, 233 insertions(+), 214 deletions(-) create mode 100644 packages/shopify-app-remix/src/server/authenticate/admin/helpers/ensure-app-is-embedded-if-required.ts create mode 100644 packages/shopify-app-remix/src/server/authenticate/admin/helpers/ensure-session-token-search-param-if-required.ts create mode 100644 packages/shopify-app-remix/src/server/authenticate/admin/helpers/redirect-to-bounce-page.ts diff --git a/packages/shopify-app-remix/src/server/authenticate/admin/authenticate.ts b/packages/shopify-app-remix/src/server/authenticate/admin/authenticate.ts index 152524c58d..ee0ff587dc 100644 --- a/packages/shopify-app-remix/src/server/authenticate/admin/authenticate.ts +++ b/packages/shopify-app-remix/src/server/authenticate/admin/authenticate.ts @@ -1,20 +1,13 @@ -import {redirect} from '@remix-run/server-runtime'; -import { - JwtPayload, - Session, - Shopify, - ShopifyRestResources, -} from '@shopify/shopify-api'; +import {Session, Shopify, ShopifyRestResources} from '@shopify/shopify-api'; import type {BasicParams} from '../../types'; import type {AppConfig, AppConfigArg} from '../../config-types'; import { getSessionTokenHeader, - validateSessionToken, - rejectBotRequest as respondToBotRequest, - respondToOptionsRequest, ensureCORSHeadersFactory, getSessionTokenFromUrlParam, + respondToBotRequest, + respondToOptionsRequest, } from '../helpers'; import type {BillingContext} from './billing/types'; @@ -32,18 +25,10 @@ import type { import { createAdminApiContext, redirectFactory, - redirectToShopifyOrAppRoot, renderAppBridge, } from './helpers'; import {AuthorizationStrategy} from './strategies/types'; -const SESSION_TOKEN_PARAM = 'id_token'; - -interface ShopWithSessionContext { - sessionContext?: SessionContext; - shop: string; -} - interface AuthStrategyParams extends BasicParams { strategy: AuthorizationStrategy; } @@ -67,50 +52,52 @@ export class AuthStrategy< public async authenticateAdmin( request: Request, ): Promise> { - const {api, logger, config} = this; + const {config, logger} = this; + const params = {api: this.api, logger, config}; - let sessionContext: SessionContext; try { - respondToBotRequest({api, logger, config}, request); - respondToOptionsRequest({api, logger, config}, request); - await this.respondToBouncePageRequest(request); - await this.respondToExitIframeRequest(request); - await this.strategy.respondToOAuthRequests(request); - - logger.info('Authenticating admin request'); - - const sessionTokenHeader = getSessionTokenHeader(request); - - if (!sessionTokenHeader) { - await this.validateUrlParams(request); - await this.strategy.ensureInstalledOnShop(request); - await this.ensureAppIsEmbeddedIfRequired(request); - await this.ensureSessionTokenSearchParamIfRequired(request); - } - - const authenticatedSession = await this.loadActiveSession(request); + respondToBotRequest(params, request); + respondToOptionsRequest(params, request); + await this.respondToBouncePageRequest(params, request); + await this.respondToExitIframeRequest(params, request); + await this.strategy.authenticate(request); + + const headerSessionToken = getSessionTokenHeader(request); + const searchParamSessionToken = getSessionTokenFromUrlParam(request); + const sessionToken = (headerSessionToken || searchParamSessionToken)!; + + logger.debug('Request was authenticated', { + sessionToken: { + header: headerSessionToken, + search: searchParamSessionToken, + }, + }); - sessionContext = await this.strategy.acquireAccessToken({ - sessionContext: authenticatedSession.sessionContext, - shop: authenticatedSession.shop, + const sessionContext = await this.strategy.exchangeToken( request, + sessionToken, + ); + + logger.debug('Extracted session context from session token', { + shop: sessionContext.session.shop, + isOnline: sessionContext.session.isOnline, }); + + return this.createContext(request, sessionContext); } catch (errorOrResponse) { if (errorOrResponse instanceof Response) { - ensureCORSHeadersFactory( - {api, logger, config}, - request, - )(errorOrResponse); + ensureCORSHeadersFactory(params, request)(errorOrResponse); } throw errorOrResponse; } - - return this.createContext(request, sessionContext); } - private async respondToBouncePageRequest(request: Request) { - const {config, logger, api} = this; + private async respondToBouncePageRequest( + params: BasicParams, + request: Request, + ) { + const {config, logger, api} = params; const url = new URL(request.url); if (url.pathname === config.auth.patchSessionTokenPath) { @@ -119,8 +106,11 @@ export class AuthStrategy< } } - private async respondToExitIframeRequest(request: Request) { - const {config, logger, api} = this; + private async respondToExitIframeRequest( + params: BasicParams, + request: Request, + ) { + const {config, logger, api} = params; const url = new URL(request.url); if (url.pathname === config.auth.exitIframePath) { @@ -131,128 +121,6 @@ export class AuthStrategy< } } - private async validateUrlParams(request: Request) { - const {api, config, logger} = this; - - if (config.isEmbeddedApp) { - const url = new URL(request.url); - const shop = api.utils.sanitizeShop(url.searchParams.get('shop')!); - if (!shop) { - logger.debug('Missing or invalid shop, redirecting to login path', { - shop, - }); - throw redirect(config.auth.loginPath); - } - - const host = api.utils.sanitizeHost(url.searchParams.get('host')!); - if (!host) { - logger.debug('Invalid host, redirecting to login path', { - host: url.searchParams.get('host'), - }); - throw redirect(config.auth.loginPath); - } - } - } - - private async ensureAppIsEmbeddedIfRequired(request: Request) { - const {api, logger, config} = this; - const url = new URL(request.url); - - const shop = url.searchParams.get('shop')!; - - if (api.config.isEmbeddedApp && url.searchParams.get('embedded') !== '1') { - logger.debug('App is not embedded, redirecting to Shopify', {shop}); - await redirectToShopifyOrAppRoot(request, {api, logger, config}); - } - } - - private async ensureSessionTokenSearchParamIfRequired(request: Request) { - const {api, logger} = this; - const url = new URL(request.url); - - const shop = url.searchParams.get('shop')!; - const searchParamSessionToken = url.searchParams.get(SESSION_TOKEN_PARAM); - const isEmbedded = url.searchParams.get('embedded') === '1'; - - if (api.config.isEmbeddedApp && isEmbedded && !searchParamSessionToken) { - logger.debug( - 'Missing session token in search params, going to bounce page', - {shop}, - ); - this.redirectToBouncePage(url); - } - } - - private getShopFromSessionToken(payload: JwtPayload): string { - const dest = new URL(payload.dest); - return dest.hostname; - } - - private async loadActiveSession( - request: Request, - ): Promise { - const {config, logger, api} = this; - - let shop: string; - let sessionId: string | undefined; - let payload: JwtPayload | undefined; - - const sessionToken = (getSessionTokenHeader(request) || - getSessionTokenFromUrlParam(request))!; - - if (config.isEmbeddedApp) { - payload = await validateSessionToken({config, logger, api}, sessionToken); - shop = this.getShopFromSessionToken(payload); - - logger.debug('Session token is present, validating session', {shop}); - sessionId = config.useOnlineTokens - ? api.session.getJwtSessionId(shop, payload.sub) - : api.session.getOfflineId(shop); - } else { - const url = new URL(request.url); - shop = url.searchParams.get('shop')!; - - // eslint-disable-next-line no-warning-comments - // TODO move this check into loadSession once we add support for it in the library - // https://github.com/orgs/Shopify/projects/6899/views/1?pane=issue&itemId=28378114 - sessionId = await api.session.getCurrentId({ - isOnline: config.useOnlineTokens, - rawRequest: request, - }); - } - - if (!sessionId) { - logger.debug('Session id not found in cookies, redirecting to OAuth', { - shop, - }); - return {shop}; - } - - logger.debug('Loading session from storage', {sessionId}); - - const session = await config.sessionStorage.loadSession(sessionId); - - logger.debug('Found session, request is valid', {shop}); - - return {sessionContext: session && {session, token: payload}, shop}; - } - - private redirectToBouncePage(url: URL): never { - const {config} = this; - - // Make sure we always point to the configured app URL so it also works behind reverse proxies (that alter the Host - // header). - url.searchParams.set( - 'shopify-reload', - `${config.appUrl}${url.pathname}${url.search}`, - ); - - // eslint-disable-next-line no-warning-comments - // TODO Make sure this works on chrome without a tunnel (weird HTTPS redirect issue) - // https://github.com/orgs/Shopify/projects/6899/views/1?pane=issue&itemId=28376650 - throw redirect(`${config.auth.patchSessionTokenPath}${url.search}`); - } - private createContext( request: Request, sessionContext: SessionContext, diff --git a/packages/shopify-app-remix/src/server/authenticate/admin/helpers/ensure-app-is-embedded-if-required.ts b/packages/shopify-app-remix/src/server/authenticate/admin/helpers/ensure-app-is-embedded-if-required.ts new file mode 100644 index 0000000000..04fd21ae1a --- /dev/null +++ b/packages/shopify-app-remix/src/server/authenticate/admin/helpers/ensure-app-is-embedded-if-required.ts @@ -0,0 +1,18 @@ +import {BasicParams} from '../../../types'; + +import {redirectToShopifyOrAppRoot} from './redirect-to-shopify-or-app-root'; + +export const ensureAppIsEmbeddedIfRequired = async ( + params: BasicParams, + request: Request, +) => { + const {api, logger, config} = params; + const url = new URL(request.url); + + const shop = url.searchParams.get('shop')!; + + if (api.config.isEmbeddedApp && url.searchParams.get('embedded') !== '1') { + logger.debug('App is not embedded, redirecting to Shopify', {shop}); + await redirectToShopifyOrAppRoot(request, {api, logger, config}); + } +}; diff --git a/packages/shopify-app-remix/src/server/authenticate/admin/helpers/ensure-session-token-search-param-if-required.ts b/packages/shopify-app-remix/src/server/authenticate/admin/helpers/ensure-session-token-search-param-if-required.ts new file mode 100644 index 0000000000..0c2b06883f --- /dev/null +++ b/packages/shopify-app-remix/src/server/authenticate/admin/helpers/ensure-session-token-search-param-if-required.ts @@ -0,0 +1,25 @@ +import {BasicParams} from '../../../types'; + +import {redirectToBouncePage} from './redirect-to-bounce-page'; + +const SESSION_TOKEN_PARAM = 'id_token'; + +export const ensureSessionTokenSearchParamIfRequired = async ( + params: BasicParams, + request: Request, +) => { + const {api, logger} = params; + const url = new URL(request.url); + + const shop = url.searchParams.get('shop')!; + const searchParamSessionToken = url.searchParams.get(SESSION_TOKEN_PARAM); + const isEmbedded = url.searchParams.get('embedded') === '1'; + + if (api.config.isEmbeddedApp && isEmbedded && !searchParamSessionToken) { + logger.debug( + 'Missing session token in search params, going to bounce page', + {shop}, + ); + redirectToBouncePage(params, url); + } +}; diff --git a/packages/shopify-app-remix/src/server/authenticate/admin/helpers/index.ts b/packages/shopify-app-remix/src/server/authenticate/admin/helpers/index.ts index 2879308c44..417ccb446c 100644 --- a/packages/shopify-app-remix/src/server/authenticate/admin/helpers/index.ts +++ b/packages/shopify-app-remix/src/server/authenticate/admin/helpers/index.ts @@ -1,7 +1,10 @@ export * from './begin-auth'; export * from './create-admin-api-context'; +export * from './ensure-app-is-embedded-if-required'; +export * from './ensure-session-token-search-param-if-required'; export * from './handle-client-error'; export * from './redirect-to-auth-page'; +export * from './redirect-to-bounce-page'; export * from './redirect-to-shopify-or-app-root'; export * from './redirect-with-app-bridge-headers'; export * from './redirect-with-exitiframe'; diff --git a/packages/shopify-app-remix/src/server/authenticate/admin/helpers/redirect-to-bounce-page.ts b/packages/shopify-app-remix/src/server/authenticate/admin/helpers/redirect-to-bounce-page.ts new file mode 100644 index 0000000000..9797c34af2 --- /dev/null +++ b/packages/shopify-app-remix/src/server/authenticate/admin/helpers/redirect-to-bounce-page.ts @@ -0,0 +1,19 @@ +import {redirect} from '@remix-run/server-runtime'; + +import {BasicParams} from '../../../types'; + +export const redirectToBouncePage = (params: BasicParams, url: URL): never => { + const {config} = params; + + // Make sure we always point to the configured app URL so it also works behind reverse proxies (that alter the Host + // header). + url.searchParams.set( + 'shopify-reload', + `${config.appUrl}${url.pathname}${url.search}`, + ); + + // eslint-disable-next-line no-warning-comments + // TODO Make sure this works on chrome without a tunnel (weird HTTPS redirect issue) + // https://github.com/orgs/Shopify/projects/6899/views/1?pane=issue&itemId=28376650 + throw redirect(`${config.auth.patchSessionTokenPath}${url.search}`); +}; diff --git a/packages/shopify-app-remix/src/server/authenticate/admin/strategies/auth-code-flow.ts b/packages/shopify-app-remix/src/server/authenticate/admin/strategies/auth-code-flow.ts index 77041d2329..3f1bb0e101 100644 --- a/packages/shopify-app-remix/src/server/authenticate/admin/strategies/auth-code-flow.ts +++ b/packages/shopify-app-remix/src/server/authenticate/admin/strategies/auth-code-flow.ts @@ -7,11 +7,15 @@ import { Session, Shopify, ShopifyRestResources, + JwtPayload, } from '@shopify/shopify-api'; +import {redirect} from '@remix-run/server-runtime'; import type {BasicParams} from '../../../types'; import { beginAuth, + ensureAppIsEmbeddedIfRequired, + ensureSessionTokenSearchParamIfRequired, redirectToAuthPage, redirectToShopifyOrAppRoot, redirectWithExitIframe, @@ -19,6 +23,7 @@ import { } from '../helpers'; import {SessionContext} from '../types'; import {AppConfig} from '../../../config-types'; +import {getSessionTokenHeader, validateSessionToken} from '../../helpers'; import {AuthorizationStrategy} from './types'; @@ -36,7 +41,57 @@ export class AuthCodeFlowStrategy< this.logger = logger; } - async respondToOAuthRequests(request: Request) { + public async authenticate(request: Request): Promise { + const {logger} = this; + const params = {api: this.api, logger, config: this.config}; + + await this.respondToOAuthRequests(request); + + logger.info('Authenticating admin auth code request'); + + const sessionTokenHeader = getSessionTokenHeader(request); + + if (!sessionTokenHeader) { + await this.ensureInstalledOnShop(request); + await ensureAppIsEmbeddedIfRequired(params, request); + await ensureSessionTokenSearchParamIfRequired(params, request); + } + } + + public async exchangeToken( + request: Request, + sessionToken: string, + ): Promise { + const {api, config, logger} = this; + + const {shop, payload, sessionId} = await this.getSessionTokenContext( + request, + sessionToken, + ); + + if (!sessionId) { + logger.debug('Session id not found in cookies, redirecting to OAuth', { + shop, + }); + throw await beginAuth({api, config, logger}, request, false, shop); + } + + logger.debug('Loading session from storage', {sessionId}); + + const session = await config.sessionStorage.loadSession(sessionId); + + if (!session || !session.isActive(config.scopes)) { + const debugMessage = session + ? 'Found a session, but it has expired, redirecting to OAuth' + : 'No session found, redirecting to OAuth'; + logger.debug(debugMessage, {shop}); + await redirectToAuthPage({config, logger, api}, request, shop); + } + + return {session: session!, token: payload}; + } + + private async respondToOAuthRequests(request: Request) { const {api, config} = this; const url = new URL(request.url); const isAuthRequest = url.pathname === config.auth.path; @@ -54,34 +109,12 @@ export class AuthCodeFlowStrategy< } } - async acquireAccessToken({ - sessionContext, - shop, - request, - }: { - sessionContext?: SessionContext; - shop: string; - request: Request; - }): Promise { - const {config, logger, api} = this; - if ( - !sessionContext?.session || - !sessionContext?.session.isActive(config.scopes) - ) { - const debugMessage = sessionContext?.session - ? 'Found a session, but it has expired, redirecting to OAuth' - : 'No session found, redirecting to OAuth'; - logger.debug(debugMessage, {shop}); - await redirectToAuthPage({config, logger, api}, request, shop); - } + private async ensureInstalledOnShop(request: Request) { + const {api, config, logger} = this; - return sessionContext!; - } + this.validateUrlParams(request); - async ensureInstalledOnShop(request: Request) { - const {api, config, logger} = this; const url = new URL(request.url); - let shop = url.searchParams.get('shop'); // Ensure app is installed @@ -125,6 +158,58 @@ export class AuthCodeFlowStrategy< } } + private validateUrlParams(request: Request) { + const {api, config, logger} = this; + + if (config.isEmbeddedApp) { + const url = new URL(request.url); + const shop = api.utils.sanitizeShop(url.searchParams.get('shop')!); + if (!shop) { + logger.debug('Missing or invalid shop, redirecting to login path', { + shop, + }); + throw redirect(config.auth.loginPath); + } + + const host = api.utils.sanitizeHost(url.searchParams.get('host')!); + if (!host) { + logger.debug('Invalid host, redirecting to login path', { + host: url.searchParams.get('host'), + }); + throw redirect(config.auth.loginPath); + } + } + } + + private async getSessionTokenContext(request: Request, sessionToken: string) { + const {api, config, logger} = this; + + let shop: string; + let payload: JwtPayload | undefined; + let sessionId: string | undefined; + + if (config.isEmbeddedApp) { + payload = await validateSessionToken({config, logger, api}, sessionToken); + const dest = new URL(payload.dest); + shop = dest.hostname; + + logger.debug('Session token is present, validating session', {shop}); + sessionId = config.useOnlineTokens + ? api.session.getJwtSessionId(shop, payload.sub!) + : api.session.getOfflineId(shop); + } else { + const url = new URL(request.url); + shop = url.searchParams.get('shop')!; + + sessionId = await api.session.getCurrentId({ + isOnline: config.useOnlineTokens, + rawRequest: request, + }); + } + + return {shop, payload, sessionId}; + } + private async handleAuthBeginRequest( request: Request, shop: string, diff --git a/packages/shopify-app-remix/src/server/authenticate/admin/strategies/types.ts b/packages/shopify-app-remix/src/server/authenticate/admin/strategies/types.ts index 822bb58acc..2c2f03281b 100644 --- a/packages/shopify-app-remix/src/server/authenticate/admin/strategies/types.ts +++ b/packages/shopify-app-remix/src/server/authenticate/admin/strategies/types.ts @@ -1,16 +1,17 @@ +import {JwtPayload} from '@shopify/shopify-api'; + import {SessionContext} from '../types'; +export interface SessionTokenContext { + shop: string; + payload?: JwtPayload; + sessionId?: string; +} + export interface AuthorizationStrategy { - // authenticate: (request: Request) => Promise; - respondToOAuthRequests: (request: Request) => Promise; - acquireAccessToken: ({ - sessionContext, - shop, - request, - }: { - sessionContext?: SessionContext; - shop: string; - request: Request; - }) => Promise; - ensureInstalledOnShop: (request: Request) => Promise; + authenticate: (request: Request) => Promise; + exchangeToken: ( + request: Request, + sessionToken: string, + ) => Promise; } diff --git a/packages/shopify-app-remix/src/server/authenticate/helpers/reject-bot-request.ts b/packages/shopify-app-remix/src/server/authenticate/helpers/reject-bot-request.ts index fba41e3cb5..3df0ced224 100644 --- a/packages/shopify-app-remix/src/server/authenticate/helpers/reject-bot-request.ts +++ b/packages/shopify-app-remix/src/server/authenticate/helpers/reject-bot-request.ts @@ -2,7 +2,7 @@ import isbot from 'isbot'; import type {BasicParams} from '../../types'; -export function rejectBotRequest( +export function respondToBotRequest( {logger}: BasicParams, request: Request, ): void | never { diff --git a/packages/shopify-app-remix/src/server/authenticate/public/checkout/authenticate.ts b/packages/shopify-app-remix/src/server/authenticate/public/checkout/authenticate.ts index 3cb7a28884..c78e988eb1 100644 --- a/packages/shopify-app-remix/src/server/authenticate/public/checkout/authenticate.ts +++ b/packages/shopify-app-remix/src/server/authenticate/public/checkout/authenticate.ts @@ -2,7 +2,7 @@ import type {BasicParams} from '../../../types'; import { ensureCORSHeadersFactory, getSessionTokenHeader, - rejectBotRequest, + respondToBotRequest, respondToOptionsRequest, validateSessionToken, } from '../../helpers'; @@ -20,7 +20,7 @@ export function authenticateCheckoutFactory( const corsHeaders = options.corsHeaders ?? []; - rejectBotRequest(params, request); + respondToBotRequest(params, request); respondToOptionsRequest(params, request, corsHeaders); const sessionTokenHeader = getSessionTokenHeader(request);