From f235249f154e476c0197bd200a100c5d23db033e Mon Sep 17 00:00:00 2001 From: Michael Bromley Date: Fri, 20 Sep 2024 16:11:38 +0200 Subject: [PATCH] fix(core): Fix RequestContext race condition causing null activeOrder Fixes #2097. This commit alters the way we store the RequestContext object on the `req` object so that we can better target individual handlers, preventing parallel execution of queries from interfering with one another. --- packages/core/e2e/auth.e2e-spec.ts | 34 +++++++--- .../test-plugins/issue-2097-plugin.ts | 34 ++++++++++ .../core/src/api/common/request-context.ts | 63 +++++++++++++++++++ .../decorators/request-context.decorator.ts | 16 +---- .../core/src/api/middleware/auth-guard.ts | 11 ++-- .../api/middleware/transaction-interceptor.ts | 31 +++------ .../request-context.service.ts | 5 +- 7 files changed, 144 insertions(+), 50 deletions(-) create mode 100644 packages/core/e2e/fixtures/test-plugins/issue-2097-plugin.ts diff --git a/packages/core/e2e/auth.e2e-spec.ts b/packages/core/e2e/auth.e2e-spec.ts index 90afad88b7..1d3483bac8 100644 --- a/packages/core/e2e/auth.e2e-spec.ts +++ b/packages/core/e2e/auth.e2e-spec.ts @@ -7,11 +7,12 @@ import path from 'path'; import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; import { initialData } from '../../../e2e-common/e2e-initial-data'; -import { testConfig, TEST_SETUP_TIMEOUT_MS } from '../../../e2e-common/test-config'; +import { TEST_SETUP_TIMEOUT_MS, testConfig } from '../../../e2e-common/test-config'; +import { Issue2097Plugin } from './fixtures/test-plugins/issue-2097-plugin'; import { ProtectedFieldsPlugin, transactions } from './fixtures/test-plugins/with-protected-field-resolver'; -import { ErrorCode, Permission } from './graphql/generated-e2e-admin-types'; import * as Codegen from './graphql/generated-e2e-admin-types'; +import { ErrorCode, Permission } from './graphql/generated-e2e-admin-types'; import * as CodegenShop from './graphql/generated-e2e-shop-types'; import { ATTEMPT_LOGIN, @@ -32,7 +33,7 @@ import { assertThrowsWithMessage } from './utils/assert-throws-with-message'; describe('Authorization & permissions', () => { const { server, adminClient, shopClient } = createTestEnvironment({ ...testConfig(), - plugins: [ProtectedFieldsPlugin], + plugins: [ProtectedFieldsPlugin, Issue2097Plugin], }); beforeAll(async () => { @@ -74,9 +75,8 @@ describe('Authorization & permissions', () => { let customerEmailAddress: string; beforeAll(async () => { await adminClient.asSuperAdmin(); - const { customers } = await adminClient.query( - GET_CUSTOMER_LIST, - ); + const { customers } = + await adminClient.query(GET_CUSTOMER_LIST); customerEmailAddress = customers.items[0].emailAddress; }); @@ -388,6 +388,24 @@ describe('Authorization & permissions', () => { expect(getErrorCode(e)).toBe('FORBIDDEN'); } }); + + // https://github.com/vendure-ecommerce/vendure/issues/2097 + it('does not overwrite ctx.authorizedAsOwnerOnly with multiple parallel top-level queries', async () => { + // We run this multiple times since the error is based on a race condition that does not + // show up consistently. + for (let i = 0; i < 10; i++) { + const result = await shopClient.query( + gql(` + query Issue2097 { + ownerProtectedThing + publicThing + } + `), + ); + expect(result.ownerProtectedThing).toBe(true); + expect(result.publicThing).toBe(true); + } + }); }); async function assertRequestAllowed(operation: DocumentNode, variables?: V) { @@ -437,7 +455,7 @@ describe('Authorization & permissions', () => { const identifier = `${code}@${Math.random().toString(16).substr(2, 8)}`; const password = 'test'; - const adminResult = await adminClient.query< + await adminClient.query< Codegen.CreateAdministratorMutation, Codegen.CreateAdministratorMutationVariables >(CREATE_ADMINISTRATOR, { @@ -449,8 +467,6 @@ describe('Authorization & permissions', () => { roleIds: [role.id], }, }); - const admin = adminResult.createAdministrator; - return { identifier, password, diff --git a/packages/core/e2e/fixtures/test-plugins/issue-2097-plugin.ts b/packages/core/e2e/fixtures/test-plugins/issue-2097-plugin.ts new file mode 100644 index 0000000000..a81a4022a8 --- /dev/null +++ b/packages/core/e2e/fixtures/test-plugins/issue-2097-plugin.ts @@ -0,0 +1,34 @@ +import { Query, Resolver } from '@nestjs/graphql'; +import { Allow, Ctx, Permission, RequestContext, VendurePlugin } from '@vendure/core'; +import gql from 'graphql-tag'; + +@Resolver() +class TestResolver { + @Query() + async publicThing(@Ctx() ctx: RequestContext) { + return true; + } + + @Query() + @Allow(Permission.Owner) + async ownerProtectedThing(@Ctx() ctx: RequestContext) { + if (ctx.authorizedAsOwnerOnly) { + return true; + } else { + return false; + } + } +} + +@VendurePlugin({ + shopApiExtensions: { + schema: gql` + extend type Query { + publicThing: Boolean! + ownerProtectedThing: Boolean! + } + `, + resolvers: [TestResolver], + }, +}) +export class Issue2097Plugin {} diff --git a/packages/core/src/api/common/request-context.ts b/packages/core/src/api/common/request-context.ts index e94b90fafb..fc2c5e5432 100644 --- a/packages/core/src/api/common/request-context.ts +++ b/packages/core/src/api/common/request-context.ts @@ -1,9 +1,11 @@ +import { ExecutionContext } from '@nestjs/common'; import { CurrencyCode, LanguageCode, Permission } from '@vendure/common/lib/generated-types'; import { ID, JsonCompatible } from '@vendure/common/lib/shared-types'; import { isObject } from '@vendure/common/lib/shared-utils'; import { Request } from 'express'; import { TFunction } from 'i18next'; +import { REQUEST_CONTEXT_KEY, REQUEST_CONTEXT_MAP_KEY } from '../../common/constants'; import { idsAreEqual } from '../../common/utils'; import { CachedSession } from '../../config/session-cache/session-cache-strategy'; import { Channel } from '../../entity/channel/channel.entity'; @@ -20,6 +22,67 @@ export type SerializedRequestContext = { _authorizedAsOwnerOnly: boolean; }; +/** + * @description + * This function is used to set the {@link RequestContext} on the `req` object. This is the underlying + * mechanism by which we are able to access the `RequestContext` from different places. + * + * For example, here is a diagram to show how, in an incoming API request, we are able to store + * and retrieve the `RequestContext` in a resolver: + * ``` + * - query { product } + * | + * - AuthGuard.canActivate() + * | | creates a `RequestContext`, stores it on `req` + * | + * - product() resolver + * | @Ctx() decorator fetching `RequestContext` from `req` + * ``` + * + * We named it this way to discourage usage outside the framework internals. + */ +export function internal_setRequestContext( + req: Request, + ctx: RequestContext, + executionContext?: ExecutionContext, +) { + // If we have access to the `ExecutionContext`, it means we are able to bind + // the `ctx` object to the specific "handler", i.e. the resolver function (for GraphQL) + // or controller (for REST). + if (executionContext && typeof executionContext.getHandler === 'function') { + // eslint-disable-next-line @typescript-eslint/ban-types + const map: Map = (req as any)[REQUEST_CONTEXT_MAP_KEY] || new Map(); + map.set(executionContext.getHandler(), ctx); + + (req as any)[REQUEST_CONTEXT_MAP_KEY] = map; + } + // We also bind to a shared key so that we can access the `ctx` object + // later even if we don't have a reference to the `ExecutionContext` + (req as any)[REQUEST_CONTEXT_KEY] = ctx; +} + +/** + * @description + * Gets the {@link RequestContext} from the `req` object. See {@link internal_setRequestContext} + * for more details on this mechanism. + */ +export function internal_getRequestContext( + req: Request, + executionContext?: ExecutionContext, +): RequestContext { + if (executionContext && typeof executionContext.getHandler === 'function') { + // eslint-disable-next-line @typescript-eslint/ban-types + const map: Map | undefined = (req as any)[REQUEST_CONTEXT_MAP_KEY]; + const ctx = map?.get(executionContext.getHandler()); + // If we have a ctx associated with the current handler (resolver function), we + // return it. Otherwise, we fall back to the shared key which will be there. + if (ctx) { + return ctx; + } + } + return (req as any)[REQUEST_CONTEXT_KEY]; +} + /** * @description * The RequestContext holds information relevant to the current request, which may be diff --git a/packages/core/src/api/decorators/request-context.decorator.ts b/packages/core/src/api/decorators/request-context.decorator.ts index 0e7b3e0af3..0e2abc0c87 100644 --- a/packages/core/src/api/decorators/request-context.decorator.ts +++ b/packages/core/src/api/decorators/request-context.decorator.ts @@ -1,6 +1,6 @@ import { ContextType, createParamDecorator, ExecutionContext } from '@nestjs/common'; -import { REQUEST_CONTEXT_KEY, REQUEST_CONTEXT_MAP_KEY } from '../../common/constants'; +import { internal_getRequestContext } from '../common/request-context'; /** * @description @@ -19,21 +19,11 @@ import { REQUEST_CONTEXT_KEY, REQUEST_CONTEXT_MAP_KEY } from '../../common/const * @docsPage Ctx Decorator */ export const Ctx = createParamDecorator((data, ctx: ExecutionContext) => { - const getContext = (req: any) => { - // eslint-disable-next-line @typescript-eslint/ban-types - const map: Map | undefined = req[REQUEST_CONTEXT_MAP_KEY]; - - // If a map contains associated transactional context with this handler - // we have to use it. It means that this handler was wrapped with @Transaction decorator. - // Otherwise use default context. - return map?.get(ctx.getHandler()) || req[REQUEST_CONTEXT_KEY]; - }; - if (ctx.getType() === 'graphql') { // GraphQL request - return getContext(ctx.getArgByIndex(2).req); + return internal_getRequestContext(ctx.getArgByIndex(2).req, ctx); } else { // REST request - return getContext(ctx.switchToHttp().getRequest()); + return internal_getRequestContext(ctx.switchToHttp().getRequest(), ctx); } }); diff --git a/packages/core/src/api/middleware/auth-guard.ts b/packages/core/src/api/middleware/auth-guard.ts index 79dd1ba237..5bdc4f9e9a 100644 --- a/packages/core/src/api/middleware/auth-guard.ts +++ b/packages/core/src/api/middleware/auth-guard.ts @@ -4,7 +4,6 @@ import { Permission } from '@vendure/common/lib/generated-types'; import { Request, Response } from 'express'; import { GraphQLResolveInfo } from 'graphql'; -import { REQUEST_CONTEXT_KEY } from '../../common/constants'; import { ForbiddenError } from '../../common/error/errors'; import { ConfigService } from '../../config/config.service'; import { LogLevel } from '../../config/logger/vendure-logger'; @@ -16,7 +15,11 @@ import { CustomerService } from '../../service/services/customer.service'; import { SessionService } from '../../service/services/session.service'; import { extractSessionToken } from '../common/extract-session-token'; import { parseContext } from '../common/parse-context'; -import { RequestContext } from '../common/request-context'; +import { + internal_getRequestContext, + internal_setRequestContext, + RequestContext, +} from '../common/request-context'; import { setSessionToken } from '../common/set-session-token'; import { PERMISSIONS_METADATA_KEY } from '../decorators/allow.decorator'; @@ -54,7 +57,7 @@ export class AuthGuard implements CanActivate { const hasOwnerPermission = !!permissions && permissions.includes(Permission.Owner); let requestContext: RequestContext; if (isFieldResolver) { - requestContext = (req as any)[REQUEST_CONTEXT_KEY]; + requestContext = internal_getRequestContext(req); } else { const session = await this.getSession(req, res, hasOwnerPermission); requestContext = await this.requestContextService.fromRequest(req, info, permissions, session); @@ -68,7 +71,7 @@ export class AuthGuard implements CanActivate { session, ); } - (req as any)[REQUEST_CONTEXT_KEY] = requestContext; + internal_setRequestContext(req, requestContext, context); } if (authDisabled || !permissions || isPublic) { diff --git a/packages/core/src/api/middleware/transaction-interceptor.ts b/packages/core/src/api/middleware/transaction-interceptor.ts index 838d690a4a..a8b139dcb1 100644 --- a/packages/core/src/api/middleware/transaction-interceptor.ts +++ b/packages/core/src/api/middleware/transaction-interceptor.ts @@ -2,16 +2,15 @@ import { CallHandler, ExecutionContext, Injectable, NestInterceptor } from '@nes import { Reflector } from '@nestjs/core'; import { Observable, of } from 'rxjs'; -import { RequestContext } from '..'; -import { REQUEST_CONTEXT_KEY, REQUEST_CONTEXT_MAP_KEY } from '../../common/constants'; +import { internal_getRequestContext, internal_setRequestContext, RequestContext } from '..'; import { TransactionWrapper } from '../../connection/transaction-wrapper'; import { TransactionalConnection } from '../../connection/transactional-connection'; import { parseContext } from '../common/parse-context'; import { - TransactionMode, + TRANSACTION_ISOLATION_LEVEL_METADATA_KEY, TRANSACTION_MODE_METADATA_KEY, TransactionIsolationLevel, - TRANSACTION_ISOLATION_LEVEL_METADATA_KEY, + TransactionMode, } from '../decorators/transaction.decorator'; /** @@ -28,8 +27,8 @@ export class TransactionInterceptor implements NestInterceptor { ) {} intercept(context: ExecutionContext, next: CallHandler): Observable { - const { isGraphQL, req } = parseContext(context); - const ctx: RequestContext | undefined = (req as any)[REQUEST_CONTEXT_KEY]; + const { req } = parseContext(context); + const ctx: RequestContext | undefined = internal_getRequestContext(req, context); if (ctx) { const transactionMode = this.reflector.get( TRANSACTION_MODE_METADATA_KEY, @@ -44,7 +43,9 @@ export class TransactionInterceptor implements NestInterceptor { this.transactionWrapper.executeInTransaction( ctx, _ctx => { - this.registerTransactionalContext(_ctx, context.getHandler(), req); + // Registers transactional request context associated + // with execution handler function + internal_setRequestContext(req, _ctx, context); return next.handle(); }, @@ -57,20 +58,4 @@ export class TransactionInterceptor implements NestInterceptor { return next.handle(); } } - - /** - * Registers transactional request context associated with execution handler function - * - * @param ctx transactional request context - * @param handler handler function from ExecutionContext - * @param req Request object - */ - // eslint-disable-next-line @typescript-eslint/ban-types - registerTransactionalContext(ctx: RequestContext, handler: Function, req: any) { - // eslint-disable-next-line @typescript-eslint/ban-types - const map: Map = req[REQUEST_CONTEXT_MAP_KEY] || new Map(); - map.set(handler, ctx); - - req[REQUEST_CONTEXT_MAP_KEY] = map; - } } diff --git a/packages/core/src/service/helpers/request-context/request-context.service.ts b/packages/core/src/service/helpers/request-context/request-context.service.ts index d306c81348..f87c5216a2 100644 --- a/packages/core/src/service/helpers/request-context/request-context.service.ts +++ b/packages/core/src/service/helpers/request-context/request-context.service.ts @@ -25,7 +25,10 @@ import { getUserChannelsPermissions } from '../utils/get-user-channels-permissio @Injectable() export class RequestContextService { /** @internal */ - constructor(private channelService: ChannelService, private configService: ConfigService) {} + constructor( + private channelService: ChannelService, + private configService: ConfigService, + ) {} /** * @description