fix(core): Correctly implement SettingsStore validation & argument order (#3808)

docs/docs/guides/developer-guide/settings-store/index.mdx

@@ -12,8 +12,8 @@ The Settings Store is a flexible system for storing configuration data with supp
 and validation. It allows plugins and the core system to store and retrieve arbitrary JSON data with
 fine-grained control over access and isolation.
 
-It provides a robust, secure, and flexible system for managing configuration data in your Vendure 
-application. Use it to store user preferences, plugin settings, feature flags, and any other 
+It provides a robust, secure, and flexible system for managing configuration data in your Vendure
+application. Use it to store user preferences, plugin settings, feature flags, and any other
 settings data your application needs.
 
 
@@ -221,7 +221,7 @@ mutation SetSettingsStoreValues($inputs: [SettingsStoreInput!]!) {
 ```
 
 :::note
-By default, the Settings Store is not exposed in the Shop API. 
+By default, the Settings Store is not exposed in the Shop API.
 However, you can expose this functionality via a custom mutations & queries
 that internally use the `SettingsStoreService` (see next section).
 :::
@@ -310,12 +310,12 @@ For programmatic access within plugins or services, use the [SettingsStoreServic
             constructor(private settingsStoreService: SettingsStoreService) {}
 
             async getUserTheme(ctx: RequestContext): Promise<string> {
-                const theme = await this.settingsStoreService.get<string>('dashboard.theme', ctx);
+                const theme = await this.settingsStoreService.get<string>(ctx, 'dashboard.theme');
                 return theme || 'light'; // Default fallback
             }
 
             async setUserTheme(ctx: RequestContext, theme: string): Promise<boolean> {
-                const result = await this.settingsStoreService.set('dashboard.theme', theme, ctx);
+                const result = await this.settingsStoreService.set(ctx, 'dashboard.theme', theme);
                 return result.result;
             }
         }
@@ -339,11 +339,11 @@ For programmatic access within plugins or services, use the [SettingsStoreServic
             constructor(private settingsStoreService: SettingsStoreService) {}
 
             async getDashboardSettings(ctx: RequestContext): Promise<DashboardSettings> {
-                const settings = await this.settingsStoreService.getMany([
+                const settings = await this.settingsStoreService.getMany(ctx, [
                     'dashboard.theme',
                     'dashboard.language',
                     'dashboard.notifications'
-                ], ctx);
+                ]);
 
                 return {
                     theme: settings['dashboard.theme'] || 'light',
@@ -364,7 +364,7 @@ For programmatic access within plugins or services, use the [SettingsStoreServic
                     updates['dashboard.notifications'] = settings.notifications;
                 }
 
-                const results = await this.settingsStoreService.setMany(updates, ctx);
+                const results = await this.settingsStoreService.setMany(ctx, updates);
 
                 return {
                     success: results.every(r => r.result),
@@ -382,15 +382,21 @@ For programmatic access within plugins or services, use the [SettingsStoreServic
 
 | Method                    | Description                                         |
 | ------------------------- | --------------------------------------------------- |
-| `get<T>(key, ctx)`        | Get a single value with optional type parameter     |
-| `getMany(keys, ctx)`      | Get multiple values efficiently in a single query   |
-| `set<T>(key, value, ctx)` | Set a value with structured result feedback         |
-| `setMany(values, ctx)`    | Set multiple values with individual result feedback |
+| `get<T>(ctx, key)`        | Get a single value with optional type parameter     |
+| `getMany(ctx, keys)`      | Get multiple values efficiently in a single query   |
+| `set<T>(ctx, key, value)` | Set a value with structured result feedback         |
+| `setMany(ctx, values)`    | Set multiple values with individual result feedback |
 | `getFieldDefinition(key)` | Get the field configuration for a key               |
 
+:::note
+Prior to v3.4.2, `ctx` was the _last_ argument to the above methods. However, since
+this is contrary to all other method usage which has `ctx` as the _first_ argument, it was
+changed while deprecating (but still supporting) the former signature.
+:::
+
 ## Orphaned Entries Cleanup
 
-When field definitions are removed from your configuration, the corresponding 
+When field definitions are removed from your configuration, the corresponding
 database entries become "orphaned". The Settings Store includes an automatic cleanup system to handle this.
 
 ### Manual Cleanup

packages/core/src/api/resolvers/admin/settings-store.resolver.ts

@@ -10,6 +10,11 @@ export class SettingsStoreInput {
     value: any;
 }
 
+const ErrorMessage = {
+    permissions: 'Insufficient permissions to set settings store value',
+    readonly: 'Cannot modify readonly settings store field via API',
+};
+
 /**
  * @description
  * Resolvers for settings store operations in the Admin API.
@@ -20,7 +25,10 @@ export class SettingsStoreAdminResolver {
 
     @Query()
     async getSettingsStoreValue(@Ctx() ctx: RequestContext, @Args('key') key: string): Promise<any> {
-        return this.settingsStoreService.get(key, ctx);
+        if (!this.settingsStoreService.hasPermission(ctx, key)) {
+            return undefined;
+        }
+        return this.settingsStoreService.get(ctx, key);
     }
 
     @Query()
@@ -28,7 +36,13 @@ export class SettingsStoreAdminResolver {
         @Ctx() ctx: RequestContext,
         @Args('keys') keys: string[],
     ): Promise<Record<string, any>> {
-        return this.settingsStoreService.getMany(keys, ctx);
+        const permittedKeys = [];
+        for (const key of keys) {
+            if (this.settingsStoreService.hasPermission(ctx, key)) {
+                permittedKeys.push(key);
+            }
+        }
+        return this.settingsStoreService.getMany(ctx, permittedKeys);
     }
 
     @Mutation()
@@ -36,7 +50,21 @@ export class SettingsStoreAdminResolver {
         @Ctx() ctx: RequestContext,
         @Args('input') input: SettingsStoreInput,
     ): Promise<SetSettingsStoreValueResult> {
-        return this.settingsStoreService.set(input.key, input.value, ctx);
+        if (!this.settingsStoreService.hasPermission(ctx, input.key)) {
+            return {
+                key: input.key,
+                result: false,
+                error: ErrorMessage.permissions,
+            };
+        }
+        if (this.settingsStoreService.isReadonly(input.key)) {
+            return {
+                key: input.key,
+                result: false,
+                error: ErrorMessage.readonly,
+            };
+        }
+        return this.settingsStoreService.set(ctx, input.key, input.value);
     }
 
     @Mutation()
@@ -44,14 +72,27 @@ export class SettingsStoreAdminResolver {
         @Ctx() ctx: RequestContext,
         @Args('inputs') inputs: SettingsStoreInput[],
     ): Promise<SetSettingsStoreValueResult[]> {
-        const values = inputs.reduce(
-            (acc, input) => {
-                acc[input.key] = input.value;
-                return acc;
-            },
-            {} as Record<string, any>,
-        );
-
-        return this.settingsStoreService.setMany(values, ctx);
+        const results: SetSettingsStoreValueResult[] = [];
+        for (const input of inputs) {
+            const hasPermission = this.settingsStoreService.hasPermission(ctx, input.key);
+            const isWritable = !this.settingsStoreService.isReadonly(input.key);
+            if (!hasPermission) {
+                results.push({
+                    key: input.key,
+                    result: false,
+                    error: ErrorMessage.permissions,
+                });
+            } else if (!isWritable) {
+                results.push({
+                    key: input.key,
+                    result: false,
+                    error: ErrorMessage.readonly,
+                });
+            } else {
+                const result = await this.settingsStoreService.set(ctx, input.key, input.value);
+                results.push(result);
+            }
+        }
+        return results;
     }
 }

packages/core/src/service/helpers/settings-store/settings-store.service.ts

@@ -99,13 +99,17 @@ export class SettingsStoreService implements OnModuleInit {
      * @param ctx - Request context for scoping and permissions
      * @returns The stored value or undefined if not found or access denied
      */
-    async get<T = JsonCompatible<any>>(key: string, ctx: RequestContext): Promise<T | undefined> {
+    async get<T = JsonCompatible<any>>(ctx: RequestContext, key: string): Promise<T | undefined>;
+    /**
+     * @deprecated Use the `ctx` arg in the first position
+     */
+    async get<T = JsonCompatible<any>>(key: string, ctx: RequestContext): Promise<T | undefined>;
+    async get<T = JsonCompatible<any>>(
+        keyOrCtx: string | RequestContext,
+        ctxOrKey: RequestContext | string,
+    ): Promise<T | undefined> {
+        const { ctx, other: key } = this.determineCtx(keyOrCtx, ctxOrKey);
         const fieldConfig = this.getFieldConfig(key);
-
-        if (!this.hasPermission(ctx, fieldConfig)) {
-            return undefined;
-        }
-
         const scope = this.generateScope(key, undefined, ctx, fieldConfig);
 
         const entry = await this.connection.getRepository(ctx, SettingsStoreEntry).findOne({
@@ -124,7 +128,16 @@ export class SettingsStoreService implements OnModuleInit {
      * @param ctx - Request context for scoping and permissions
      * @returns Object mapping keys to their values
      */
-    async getMany(keys: string[], ctx: RequestContext): Promise<Record<string, JsonCompatible<any>>> {
+    async getMany(ctx: RequestContext, keys: string[]): Promise<Record<string, JsonCompatible<any>>>;
+    /**
+     * @deprecated Use `ctx` as the first argument
+     */
+    async getMany(keys: string[], ctx: RequestContext): Promise<Record<string, JsonCompatible<any>>>;
+    async getMany(
+        keysOrCtx: string[] | RequestContext,
+        ctxOrKeys: RequestContext | string[],
+    ): Promise<Record<string, JsonCompatible<any>>> {
+        const { ctx, other: keys } = this.determineCtx(keysOrCtx, ctxOrKeys);
         const result: Record<string, any> = {};
 
         // Build array of key/scopeKey pairs for authorized keys
@@ -132,11 +145,8 @@ export class SettingsStoreService implements OnModuleInit {
 
         for (const key of keys) {
             const fieldConfig = this.getFieldConfig(key);
-
-            if (this.hasPermission(ctx, fieldConfig)) {
-                const scope = this.generateScope(key, undefined, ctx, fieldConfig);
-                queries.push({ key, scope });
-            }
+            const scope = this.generateScope(key, undefined, ctx, fieldConfig);
+            queries.push({ key, scope });
         }
 
         if (queries.length === 0) {
@@ -181,30 +191,31 @@ export class SettingsStoreService implements OnModuleInit {
      * @param ctx - Request context for scoping and permissions
      * @returns SetSettingsStoreValueResult with operation status and error details
      */
+    async set<T extends JsonCompatible<any> = JsonCompatible<any>>(
+        ctx: RequestContext,
+        key: string,
+        value: T,
+    ): Promise<SetSettingsStoreValueResult>;
+    /**
+     * @deprecated Use `ctx` as the first argument
+     */
     async set<T extends JsonCompatible<any> = JsonCompatible<any>>(
         key: string,
         value: T,
         ctx: RequestContext,
+    ): Promise<SetSettingsStoreValueResult>;
+    async set<T extends JsonCompatible<any> = JsonCompatible<any>>(
+        keyOrCtx: string | RequestContext,
+        keyOrValue: string | T,
+        ctxOrValue: RequestContext | T,
     ): Promise<SetSettingsStoreValueResult> {
+        // Sort out the overloaded signatures
+        const ctx = keyOrCtx instanceof RequestContext ? keyOrCtx : (ctxOrValue as RequestContext);
+        const key = keyOrCtx instanceof RequestContext ? (keyOrValue as string) : keyOrCtx;
+        const value = ctxOrValue instanceof RequestContext ? (keyOrValue as T) : ctxOrValue;
+
         try {
             const fieldConfig = this.getFieldConfig(key);
-
-            if (!this.hasPermission(ctx, fieldConfig)) {
-                return {
-                    key,
-                    result: false,
-                    error: 'Insufficient permissions to set settings store value',
-                };
-            }
-
-            if (fieldConfig.readonly) {
-                return {
-                    key,
-                    result: false,
-                    error: 'Cannot modify readonly settings store field via API',
-                };
-            }
-
             // Validate the value
             await this.validateValue(key, value, ctx);
 
@@ -245,19 +256,27 @@ export class SettingsStoreService implements OnModuleInit {
      * Set multiple values with structured result feedback for each operation.
      * This method will not throw errors but will return
      * detailed results for each key-value pair.
-     *
-     * @param values - Object mapping keys to their values
-     * @param ctx - Request context for scoping and permissions
-     * @returns Array of SetSettingsStoreValueResult with operation status for each key
+     */
+    async setMany(
+        ctx: RequestContext,
+        values: Record<string, JsonCompatible<any>>,
+    ): Promise<SetSettingsStoreValueResult[]>;
+    /**
+     * @deprecated Use `ctx` as the first argument
      */
     async setMany(
         values: Record<string, JsonCompatible<any>>,
         ctx: RequestContext,
+    ): Promise<SetSettingsStoreValueResult[]>;
+    async setMany(
+        valuesOrCtx: Record<string, JsonCompatible<any>> | RequestContext,
+        ctxOrValues: RequestContext | Record<string, JsonCompatible<any>>,
     ): Promise<SetSettingsStoreValueResult[]> {
+        const { ctx, other: values } = this.determineCtx(valuesOrCtx, ctxOrValues);
         const results: SetSettingsStoreValueResult[] = [];
 
         for (const [key, value] of Object.entries(values)) {
-            const result = await this.set(key, value, ctx);
+            const result = await this.set(ctx, key, value);
             results.push(result);
         }
 
@@ -438,18 +457,56 @@ export class SettingsStoreService implements OnModuleInit {
     /**
      * @description
      * Check if the current user has permission to access a field.
+     * This is not called internally in the get and set methods, so should
+     * be used by any methods which are exposing these methods via the GraphQL
+     * APIs.
+     */
+    hasPermission(ctx: RequestContext, key: string): boolean {
+        try {
+            const fieldConfig = this.getFieldConfig(key);
+            // Admin API: check required permissions
+            const requiredPermissions = fieldConfig.requiresPermission;
+            if (requiredPermissions) {
+                const permissions = Array.isArray(requiredPermissions)
+                    ? requiredPermissions
+                    : [requiredPermissions];
+                return ctx.userHasPermissions(permissions as any);
+            }
+
+            // Default: require authentication
+            return ctx.userHasPermissions([Permission.Authenticated]);
+        } catch (error) {
+            return true;
+        }
+    }
+
+    /**
+     * @description
+     * Returns true if the settings field has the `readonly: true` configuration.
      */
-    private hasPermission(ctx: RequestContext, fieldConfig: SettingsStoreFieldConfig): boolean {
-        // Admin API: check required permissions
-        const requiredPermissions = fieldConfig.requiresPermission;
-        if (requiredPermissions) {
-            const permissions = Array.isArray(requiredPermissions)
-                ? requiredPermissions
-                : [requiredPermissions];
-            return ctx.userHasPermissions(permissions as any);
+    isReadonly(key: string): boolean {
+        try {
+            const fieldConfig = this.getFieldConfig(key);
+            return fieldConfig.readonly === true;
+        } catch (error) {
+            return false;
         }
+    }
 
-        // Default: require authentication
-        return ctx.userHasPermissions([Permission.Authenticated]);
+    /**
+     * This unfortunate workaround is here because in the first version of the SettingsStore we have the
+     * ctx arg last, which goes against all patterns in the rest of the code base. In v3.4.2 we overload
+     * the methods to allow the correct ordering, and deprecate the original order.
+     */
+    private determineCtx<K>(
+        a: K | RequestContext,
+        b: K | RequestContext,
+    ): {
+        other: K;
+        ctx: RequestContext;
+    } {
+        const ctx = a instanceof RequestContext ? a : (b as RequestContext);
+        const other = a instanceof RequestContext ? (b as K) : a;
+        return { other, ctx };
     }
 }