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

@@ -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<Codegen.GetCustomerListQuery>(
-                    GET_CUSTOMER_LIST,
-                );
+                const { customers } =
+                    await adminClient.query<Codegen.GetCustomerListQuery>(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<V>(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,

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 {}

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<Function, RequestContext> = (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<Function, RequestContext> | 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

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<Function, any> | 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<ContextType | 'graphql'>() === '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);
     }
 });

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) {

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<any> {
-        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<TransactionMode>(
                 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<Function, RequestContext> = req[REQUEST_CONTEXT_MAP_KEY] || new Map();
-        map.set(handler, ctx);
-
-        req[REQUEST_CONTEXT_MAP_KEY] = map;
-    }
 }

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