fix(core): Roles query pagination (#3826)

packages/core/e2e/role.e2e-spec.ts

@@ -11,7 +11,7 @@ import path from 'path';
 import { afterAll, beforeAll, 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 { ROLE_FRAGMENT } from './graphql/fragments';
 import * as Codegen from './graphql/generated-e2e-admin-types';
@@ -448,8 +448,8 @@ describe('Role resolver', () => {
 
     // https://github.com/vendure-ecommerce/vendure/issues/1874
     describe('role escalation', () => {
-        let defaultChannel: Codegen.GetChannelsQuery['channels'][number];
-        let secondChannel: Codegen.GetChannelsQuery['channels'][number];
+        let defaultChannel: Codegen.GetChannelsQuery['channels']['items'][number];
+        let secondChannel: Codegen.GetChannelsQuery['channels']['items'][number];
         let limitedAdmin: Codegen.CreateAdministratorMutation['createAdministrator'];
         let orderReaderRole: Codegen.CreateRoleMutation['createRole'];
         let adminCreatorRole: Codegen.CreateRoleMutation['createRole'];
@@ -681,6 +681,82 @@ describe('Role resolver', () => {
             }, 'Active user does not have sufficient permissions'),
         );
     });
+
+    describe('roles query', () => {
+        let limitedChannelAdmin: Codegen.CreateAdministratorMutation['createAdministrator'];
+
+        beforeAll(async () => {
+            adminClient.setChannelToken(E2E_DEFAULT_CHANNEL_TOKEN);
+            await adminClient.asSuperAdmin();
+
+            // Create roles that will be hidden from limited admin
+            await adminClient.query<Codegen.CreateRoleMutation, Codegen.CreateRoleMutationVariables>(
+                CREATE_ROLE,
+                {
+                    input: {
+                        code: 'hidden-role',
+                        description: 'Hidden role',
+                        // Some permission the limited admin user doesn't have, so the role is hidden
+                        permissions: [Permission.ReadOrder],
+                    },
+                },
+            );
+
+            // Create a role to assign to the limited admin user
+            const visibleRole = await adminClient.query<
+                Codegen.CreateRoleMutation,
+                Codegen.CreateRoleMutationVariables
+            >(CREATE_ROLE, {
+                input: {
+                    code: 'visible-role',
+                    description: 'Visible role',
+                    permissions: [Permission.ReadAdministrator],
+                },
+            });
+
+            const { createAdministrator } = await adminClient.query<
+                Codegen.CreateAdministratorMutation,
+                Codegen.CreateAdministratorMutationVariables
+            >(CREATE_ADMINISTRATOR, {
+                input: {
+                    firstName: 'Limited',
+                    lastName: 'Admin',
+                    emailAddress: 'limited@test.com',
+                    roleIds: [visibleRole.createRole.id],
+                    password: 'test',
+                },
+            });
+            limitedChannelAdmin = createAdministrator;
+        });
+
+        it('should return only visible roles with correct pagination', async () => {
+            // Login as limited admin
+            await adminClient.asUserWithCredentials(limitedChannelAdmin.emailAddress, 'test');
+
+            // Query first page with pagination, sorted by createdAt ASC
+            const result = await adminClient.query<Codegen.GetRolesQuery, Codegen.GetRolesQueryVariables>(
+                GET_ROLES,
+                {
+                    options: {
+                        take: 2,
+                    },
+                },
+            );
+
+            // Should have at least visible role and test role created earlier
+            expect(result.roles.items).toHaveLength(2);
+            expect(result.roles.totalItems).toBe(2);
+
+            // The returned role should be one that the limited admin can see
+            const roleCodes = result.roles.items.map(r => r.code);
+            expect(roleCodes).toContain('visible-role');
+            expect(roleCodes).not.toContain('hidden-role');
+        });
+
+        afterAll(async () => {
+            await adminClient.asSuperAdmin();
+        });
+    });
 });
 
 export const GET_ROLES = gql`

packages/core/src/service/services/role.service.ts

@@ -14,9 +14,11 @@ import {
 } from '@vendure/common/lib/shared-constants';
 import { ID, PaginatedList } from '@vendure/common/lib/shared-types';
 import { unique } from '@vendure/common/lib/unique';
+import { In } from 'typeorm';
 
 import { RequestContext } from '../../api/common/request-context';
 import { RelationPaths } from '../../api/decorators/relations.decorator';
+import { CacheService } from '../../cache';
 import { RequestContextCacheService } from '../../cache/request-context-cache.service';
 import { getAllPermissionsMetadata } from '../../common/constants';
 import {
@@ -53,6 +55,14 @@ import { ChannelService } from './channel.service';
 @Injectable()
 @Instrument()
 export class RoleService {
+    private rolesCacheKey = 'RoleService.allRoles';
+    private rolesCache = this.cacheService.createCache({
+        getKey: () => this.rolesCacheKey,
+        options: {
+            ttl: 1000 * 60 * 60, // 1 hour
+        },
+    });
+
     constructor(
         private connection: TransactionalConnection,
         private channelService: ChannelService,
@@ -60,7 +70,13 @@ export class RoleService {
         private configService: ConfigService,
         private eventBus: EventBus,
         private requestContextCache: RequestContextCacheService,
-    ) {}
+        private cacheService: CacheService,
+    ) {
+        // When a Role is created, updated or deleted, we need to invalidate the roles cache
+        this.eventBus.ofType(RoleEvent).subscribe(event => {
+            void this.rolesCache.delete(this.rolesCacheKey);
+        });
+    }
 
     async initRoles() {
         await this.ensureSuperAdminRoleExists();
@@ -68,27 +84,33 @@ export class RoleService {
         await this.ensureRolesHaveValidPermissions();
     }
 
-    findAll(
+    async findAll(
         ctx: RequestContext,
         options?: ListQueryOptions<Role>,
         relations?: RelationPaths<Role>,
     ): Promise<PaginatedList<Role>> {
-        return this.listQueryBuilder
-            .build(Role, options, { relations: unique([...(relations ?? []), 'channels']), ctx })
-            .getManyAndCount()
-            .then(async ([items, totalItems]) => {
-                const visibleRoles: Role[] = [];
-                for (const item of items) {
-                    const canRead = await this.activeUserCanReadRole(ctx, item);
-                    if (canRead) {
-                        visibleRoles.push(item);
-                    }
-                }
-                return {
-                    items: visibleRoles,
-                    totalItems,
-                };
-            });
+        // Compute the set of Role IDs the active user can read (channel + permission check) up front to ensure sort/skip/take operate only over visible Roles.
+        const allRoles = await this.getAllRolesWithChannels(ctx);
+
+        const visibleRoleIds: ID[] = [];
+        for (const role of allRoles) {
+            if (await this.activeUserCanReadRole(ctx, role)) {
+                visibleRoleIds.push(role.id);
+            }
+        }
+
+        if (visibleRoleIds.length === 0) {
+            return { items: [], totalItems: 0 };
+        }
+
+        const [items, totalItems] = await this.listQueryBuilder
+            .build(Role, options, {
+                relations: unique([...(relations ?? []), 'channels']),
+                ctx,
+            })
+            .andWhere({ id: In(visibleRoleIds) })
+            .getManyAndCount();
+        return { items, totalItems };
     }
 
     findOne(ctx: RequestContext, roleId: ID, relations?: RelationPaths<Role>): Promise<Role | undefined> {
@@ -188,6 +210,15 @@ export class RoleService {
         return true;
     }
 
+    private async getAllRolesWithChannels(ctx: RequestContext): Promise<Role[]> {
+        const allRolesJson = await this.rolesCache.get(this.rolesCacheKey, async () => {
+            const roles = await this.connection.getRepository(ctx, Role).find({ relations: ['channels'] });
+            return JSON.stringify(roles);
+        });
+
+        return JSON.parse(allRolesJson);
+    }
+
     /**
      * @description
      * Returns true if the User has all the specified permissions on that Channel