fix(core): Fix permission escalation when creating Roles/Admins

Fixes #1874

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

@@ -5,7 +5,7 @@ import gql from 'graphql-tag';
 import path from 'path';
 
 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 { ADMINISTRATOR_FRAGMENT } from './graphql/fragments';
 import {
@@ -19,7 +19,7 @@ import {
     UpdateActiveAdministrator,
     UpdateAdministrator,
 } from './graphql/generated-e2e-admin-types';
-import { CREATE_ADMINISTRATOR } from './graphql/shared-definitions';
+import { CREATE_ADMINISTRATOR, UPDATE_ADMINISTRATOR } from './graphql/shared-definitions';
 import { assertThrowsWithMessage } from './utils/assert-throws-with-message';
 
 describe('Administrator resolver', () => {
@@ -283,15 +283,6 @@ export const UPDATE_ACTIVE_ADMINISTRATOR = gql`
     ${ADMINISTRATOR_FRAGMENT}
 `;
 
-export const UPDATE_ADMINISTRATOR = gql`
-    mutation UpdateAdministrator($input: UpdateAdministratorInput!) {
-        updateAdministrator(input: $input) {
-            ...Administrator
-        }
-    }
-    ${ADMINISTRATOR_FRAGMENT}
-`;
-
 export const DELETE_ADMINISTRATOR = gql`
     mutation DeleteAdministrator($id: ID!) {
         deleteAdministrator(id: $id) {

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

@@ -10,7 +10,7 @@ import gql from 'graphql-tag';
 import path from 'path';
 
 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 {
     AssignProductsToChannel,
@@ -36,6 +36,7 @@ import {
     CREATE_ADMINISTRATOR,
     CREATE_CHANNEL,
     CREATE_ROLE,
+    GET_CHANNELS,
     GET_CUSTOMER_LIST,
     GET_PRODUCT_WITH_VARIANTS,
     ME,
@@ -250,6 +251,7 @@ describe('Channels', () => {
     });
 
     it('createRole with no channelId implicitly uses active channel', async () => {
+        await adminClient.asSuperAdmin();
         const { createRole } = await adminClient.query<CreateRole.Mutation, CreateRole.Variables>(
             CREATE_ROLE,
             {
@@ -360,16 +362,6 @@ describe('Channels', () => {
     });
 });
 
-const GET_CHANNELS = gql`
-    query GetChannels {
-        channels {
-            id
-            code
-            token
-        }
-    }
-`;
-
 const DELETE_CHANNEL = gql`
     mutation DeleteChannel($id: ID!) {
         deleteChannel(id: $id) {

packages/core/e2e/graphql/shared-definitions.ts

@@ -995,3 +995,22 @@ export const DELETE_PROMOTION = gql`
         }
     }
 `;
+
+export const GET_CHANNELS = gql`
+    query GetChannels {
+        channels {
+            id
+            code
+            token
+        }
+    }
+`;
+
+export const UPDATE_ADMINISTRATOR = gql`
+    mutation UpdateAdministrator($input: UpdateAdministratorInput!) {
+        updateAdministrator(input: $input) {
+            ...Administrator
+        }
+    }
+    ${ADMINISTRATOR_FRAGMENT}
+`;

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

@@ -1,32 +1,49 @@
+/* tslint:disable:no-non-null-assertion */
 import { omit } from '@vendure/common/lib/omit';
 import {
     CUSTOMER_ROLE_CODE,
     DEFAULT_CHANNEL_CODE,
     SUPER_ADMIN_ROLE_CODE,
 } from '@vendure/common/lib/shared-constants';
-import { createTestEnvironment } from '@vendure/testing';
+import { createTestEnvironment, E2E_DEFAULT_CHANNEL_TOKEN } from '@vendure/testing';
 import gql from 'graphql-tag';
 import path from 'path';
 
 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 {
     ChannelFragment,
+    CreateAdministratorMutation,
+    CreateAdministratorMutationVariables,
     CreateChannel,
     CreateRole,
+    CreateRoleMutation,
+    CreateRoleMutationVariables,
     CurrencyCode,
     DeleteRole,
     DeletionResult,
+    GetChannelsQuery,
     GetRole,
     GetRoles,
     LanguageCode,
     Permission,
     Role,
+    UpdateAdministratorMutation,
+    UpdateAdministratorMutationVariables,
     UpdateRole,
+    UpdateRoleMutation,
+    UpdateRoleMutationVariables,
 } from './graphql/generated-e2e-admin-types';
-import { CREATE_CHANNEL, CREATE_ROLE, UPDATE_ROLE } from './graphql/shared-definitions';
+import {
+    CREATE_ADMINISTRATOR,
+    CREATE_CHANNEL,
+    CREATE_ROLE,
+    GET_CHANNELS,
+    UPDATE_ADMINISTRATOR,
+    UPDATE_ROLE,
+} from './graphql/shared-definitions';
 import { assertThrowsWithMessage } from './utils/assert-throws-with-message';
 import { sortById } from './utils/test-order-utils';
 
@@ -401,6 +418,212 @@ describe('Role resolver', () => {
             ]);
         });
     });
+
+    // https://github.com/vendure-ecommerce/vendure/issues/1874
+    describe('role escalation', () => {
+        let defaultChannel: GetChannelsQuery['channels'][number];
+        let secondChannel: GetChannelsQuery['channels'][number];
+        let limitedAdmin: CreateAdministratorMutation['createAdministrator'];
+        let orderReaderRole: CreateRoleMutation['createRole'];
+        let adminCreatorRole: CreateRoleMutation['createRole'];
+        let adminCreatorAdministrator: CreateAdministratorMutation['createAdministrator'];
+
+        beforeAll(async () => {
+            const { channels } = await adminClient.query<GetChannelsQuery>(GET_CHANNELS);
+            defaultChannel = channels.find(c => c.token === E2E_DEFAULT_CHANNEL_TOKEN)!;
+            secondChannel = channels.find(c => c.token !== E2E_DEFAULT_CHANNEL_TOKEN)!;
+            await adminClient.setChannelToken(E2E_DEFAULT_CHANNEL_TOKEN);
+            await adminClient.asSuperAdmin();
+            const { createRole } = await adminClient.query<CreateRoleMutation, CreateRoleMutationVariables>(
+                CREATE_ROLE,
+                {
+                    input: {
+                        code: 'second-channel-admin-manager',
+                        description: '',
+                        channelIds: [secondChannel.id],
+                        permissions: [
+                            Permission.CreateAdministrator,
+                            Permission.ReadAdministrator,
+                            Permission.UpdateAdministrator,
+                            Permission.DeleteAdministrator,
+                        ],
+                    },
+                },
+            );
+
+            const { createAdministrator } = await adminClient.query<
+                CreateAdministratorMutation,
+                CreateAdministratorMutationVariables
+            >(CREATE_ADMINISTRATOR, {
+                input: {
+                    firstName: 'channel2',
+                    lastName: 'admin manager',
+                    emailAddress: 'channel2@test.com',
+                    roleIds: [createRole.id],
+                    password: 'test',
+                },
+            });
+            limitedAdmin = createAdministrator;
+
+            const { createRole: createRole2 } = await adminClient.query<
+                CreateRoleMutation,
+                CreateRoleMutationVariables
+            >(CREATE_ROLE, {
+                input: {
+                    code: 'second-channel-order-manager',
+                    description: '',
+                    channelIds: [secondChannel.id],
+                    permissions: [Permission.ReadOrder],
+                },
+            });
+
+            orderReaderRole = createRole2;
+
+            adminClient.setChannelToken(secondChannel.token);
+            await adminClient.asUserWithCredentials(limitedAdmin.emailAddress, 'test');
+        });
+
+        it(
+            'limited admin cannot create Role with SuperAdmin permission',
+            assertThrowsWithMessage(async () => {
+                await adminClient.query<CreateRoleMutation, CreateRoleMutationVariables>(CREATE_ROLE, {
+                    input: {
+                        code: 'evil-superadmin',
+                        description: '',
+                        channelIds: [secondChannel.id],
+                        permissions: [Permission.SuperAdmin],
+                    },
+                });
+            }, 'The permission "SuperAdmin" may not be assigned'),
+        );
+
+        it(
+            'limited admin cannot create Administrator with SuperAdmin role',
+            assertThrowsWithMessage(async () => {
+                const superAdminRole = defaultRoles.find(r => r.code === SUPER_ADMIN_ROLE_CODE)!;
+                await adminClient.query<CreateAdministratorMutation, CreateAdministratorMutationVariables>(
+                    CREATE_ADMINISTRATOR,
+                    {
+                        input: {
+                            firstName: 'Dr',
+                            lastName: 'Evil',
+                            emailAddress: 'drevil@test.com',
+                            roleIds: [superAdminRole.id],
+                            password: 'test',
+                        },
+                    },
+                );
+            }, 'Active user does not have sufficient permissions'),
+        );
+
+        it(
+            'limited admin cannot create Role with permissions it itself does not have',
+            assertThrowsWithMessage(async () => {
+                await adminClient.query<CreateRoleMutation, CreateRoleMutationVariables>(CREATE_ROLE, {
+                    input: {
+                        code: 'evil-order-manager',
+                        description: '',
+                        channelIds: [secondChannel.id],
+                        permissions: [Permission.ReadOrder],
+                    },
+                });
+            }, 'Active user does not have sufficient permissions'),
+        );
+
+        it(
+            'limited admin cannot create Role on channel it does not have permissions on',
+            assertThrowsWithMessage(async () => {
+                await adminClient.query<CreateRoleMutation, CreateRoleMutationVariables>(CREATE_ROLE, {
+                    input: {
+                        code: 'evil-order-manager',
+                        description: '',
+                        channelIds: [defaultChannel.id],
+                        permissions: [Permission.CreateAdministrator],
+                    },
+                });
+            }, 'You are not currently authorized to perform this action'),
+        );
+
+        it(
+            'limited admin cannot create Administrator with a Role with greater permissions than they themselves have',
+            assertThrowsWithMessage(async () => {
+                await adminClient.query<CreateAdministratorMutation, CreateAdministratorMutationVariables>(
+                    CREATE_ADMINISTRATOR,
+                    {
+                        input: {
+                            firstName: 'Dr',
+                            lastName: 'Evil',
+                            emailAddress: 'drevil@test.com',
+                            roleIds: [orderReaderRole.id],
+                            password: 'test',
+                        },
+                    },
+                );
+            }, 'Active user does not have sufficient permissions'),
+        );
+
+        it('limited admin can create Role with permissions it itself has', async () => {
+            const { createRole } = await adminClient.query<CreateRoleMutation, CreateRoleMutationVariables>(
+                CREATE_ROLE,
+                {
+                    input: {
+                        code: 'good-admin-creator',
+                        description: '',
+                        channelIds: [secondChannel.id],
+                        permissions: [Permission.CreateAdministrator],
+                    },
+                },
+            );
+
+            expect(createRole.code).toBe('good-admin-creator');
+            adminCreatorRole = createRole;
+        });
+
+        it('limited admin can create Administrator with permissions it itself has', async () => {
+            const { createAdministrator } = await adminClient.query<
+                CreateAdministratorMutation,
+                CreateAdministratorMutationVariables
+            >(CREATE_ADMINISTRATOR, {
+                input: {
+                    firstName: 'Admin',
+                    lastName: 'Creator',
+                    emailAddress: 'admincreator@test.com',
+                    roleIds: [adminCreatorRole.id],
+                    password: 'test',
+                },
+            });
+
+            expect(createAdministrator.emailAddress).toBe('admincreator@test.com');
+            adminCreatorAdministrator = createAdministrator;
+        });
+
+        it(
+            'limited admin cannot update Role with permissions it itself lacks',
+            assertThrowsWithMessage(async () => {
+                await adminClient.query<UpdateRoleMutation, UpdateRoleMutationVariables>(UPDATE_ROLE, {
+                    input: {
+                        id: adminCreatorRole.id,
+                        permissions: [Permission.ReadOrder],
+                    },
+                });
+            }, 'Active user does not have sufficient permissions'),
+        );
+
+        it(
+            'limited admin cannot update Administrator with Role containing permissions it itself lacks',
+            assertThrowsWithMessage(async () => {
+                await adminClient.query<UpdateAdministratorMutation, UpdateAdministratorMutationVariables>(
+                    UPDATE_ADMINISTRATOR,
+                    {
+                        input: {
+                            id: adminCreatorAdministrator.id,
+                            roleIds: [adminCreatorRole.id, orderReaderRole.id],
+                        },
+                    },
+                );
+            }, 'Active user does not have sufficient permissions'),
+        );
+    });
 });
 
 export const GET_ROLES = gql`

packages/core/src/i18n/messages/en.json

@@ -1,5 +1,6 @@
 {
   "error": {
+    "active-user-does-not-have-sufficient-permissions": "Active user does not have sufficient permissions",
     "cannot-delete-role": "The role '{ roleCode }' cannot be deleted",
     "cannot-delete-sole-superadmin": "The sole SuperAdmin cannot be deleted",
     "cannot-locate-customer-for-user": "Cannot locate a Customer for the user",

packages/core/src/service/helpers/utils/get-user-channels-permissions.ts

@@ -2,6 +2,7 @@ import { Permission } from '@vendure/common/lib/generated-types';
 import { ID } from '@vendure/common/lib/shared-types';
 import { unique } from '@vendure/common/lib/unique';
 
+import { Role } from '../../../entity/index';
 import { User } from '../../../entity/user/user.entity';
 
 export interface UserChannelPermissions {
@@ -15,9 +16,17 @@ export interface UserChannelPermissions {
  * Returns an array of Channels and permissions on those Channels for the given User.
  */
 export function getUserChannelsPermissions(user: User): UserChannelPermissions[] {
+    return getChannelPermissions(user.roles);
+}
+
+/**
+ * @description
+ * Returns an array of Channels and permissions on those Channels for the given Roles.
+ */
+export function getChannelPermissions(roles: Role[]): UserChannelPermissions[] {
     const channelsMap: { [code: string]: UserChannelPermissions } = {};
 
-    for (const role of user.roles) {
+    for (const role of roles) {
         for (const channel of role.channels) {
             if (!channelsMap[channel.code]) {
                 channelsMap[channel.code] = {

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

@@ -8,13 +8,14 @@ import { ID, PaginatedList } from '@vendure/common/lib/shared-types';
 
 import { RequestContext } from '../../api/common/request-context';
 import { RelationPaths } from '../../api/index';
-import { EntityNotFoundError, InternalServerError } from '../../common/error/errors';
+import { EntityNotFoundError, InternalServerError, UserInputError } from '../../common/error/errors';
 import { idsAreEqual } from '../../common/index';
 import { ListQueryOptions } from '../../common/types/common-types';
 import { ConfigService } from '../../config';
 import { TransactionalConnection } from '../../connection/transactional-connection';
 import { Administrator } from '../../entity/administrator/administrator.entity';
 import { NativeAuthenticationMethod } from '../../entity/authentication-method/native-authentication-method.entity';
+import { Role } from '../../entity/index';
 import { User } from '../../entity/user/user.entity';
 import { EventBus } from '../../event-bus';
 import { AdministratorEvent } from '../../event-bus/events/administrator-event';
@@ -22,6 +23,7 @@ import { RoleChangeEvent } from '../../event-bus/events/role-change-event';
 import { CustomFieldRelationService } from '../helpers/custom-field-relation/custom-field-relation.service';
 import { ListQueryBuilder } from '../helpers/list-query-builder/list-query-builder';
 import { PasswordCipher } from '../helpers/password-cipher/password-cipher';
+import { getChannelPermissions } from '../helpers/utils/get-user-channels-permissions';
 import { patchEntity } from '../helpers/utils/patch-entity';
 
 import { RoleService } from './role.service';
@@ -113,6 +115,7 @@ export class AdministratorService {
      * Create a new Administrator.
      */
     async create(ctx: RequestContext, input: CreateAdministratorInput): Promise<Administrator> {
+        await this.checkActiveUserCanGrantRoles(ctx, input.roleIds);
         const administrator = new Administrator(input);
         administrator.user = await this.userService.createAdminUser(ctx, input.emailAddress, input.password);
         let createdAdministrator = await this.connection
@@ -140,6 +143,9 @@ export class AdministratorService {
         if (!administrator) {
             throw new EntityNotFoundError('Administrator', input.id);
         }
+        if (input.roleIds) {
+            await this.checkActiveUserCanGrantRoles(ctx, input.roleIds);
+        }
         let updatedAdministrator = patchEntity(administrator, input);
         await this.connection.getRepository(ctx, Administrator).save(administrator, { reload: false });
 
@@ -189,6 +195,28 @@ export class AdministratorService {
         return updatedAdministrator;
     }
 
+    /**
+     * @description
+     * Checks that the active user is allowed to grant the specified Roles when creating or
+     * updating an Administrator.
+     */
+    private async checkActiveUserCanGrantRoles(ctx: RequestContext, roleIds: ID[]) {
+        const roles = await this.connection
+            .getRepository(ctx, Role)
+            .findByIds(roleIds, { relations: ['channels'] });
+        const permissionsRequired = getChannelPermissions(roles);
+        for (const channelPermissions of permissionsRequired) {
+            const activeUserHasRequiredPermissions = await this.roleService.userHasAllPermissionsOnChannel(
+                ctx,
+                channelPermissions.id,
+                channelPermissions.permissions,
+            );
+            if (!activeUserHasRequiredPermissions) {
+                throw new UserInputError('error.active-user-does-not-have-sufficient-permissions');
+            }
+        }
+    }
+
     /**
      * @description
      * Assigns a Role to the Administrator's User entity.
@@ -274,19 +302,20 @@ export class AdministratorService {
                 roleIds: [superAdminRole.id],
             });
         } else {
-            const superAdministrator = await this.connection.rawConnection.getRepository(Administrator).findOne({
-                where: {
-                    user: superAdminUser,
-                },
-            });
+            const superAdministrator = await this.connection.rawConnection
+                .getRepository(Administrator)
+                .findOne({
+                    where: {
+                        user: superAdminUser,
+                    },
+                });
             if (!superAdministrator) {
                 const administrator = new Administrator({
                     emailAddress: superadminCredentials.identifier,
                     firstName: 'Super',
                     lastName: 'Admin',
                 });
-                const createdAdministrator = await this.connection
-                    .rawConnection
+                const createdAdministrator = await this.connection.rawConnection
                     .getRepository(Administrator)
                     .save(administrator);
                 createdAdministrator.user = superAdminUser;

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

@@ -34,7 +34,10 @@ import { User } from '../../entity/user/user.entity';
 import { EventBus } from '../../event-bus';
 import { RoleEvent } from '../../event-bus/events/role-event';
 import { ListQueryBuilder } from '../helpers/list-query-builder/list-query-builder';
-import { getUserChannelsPermissions } from '../helpers/utils/get-user-channels-permissions';
+import {
+    getChannelPermissions,
+    getUserChannelsPermissions,
+} from '../helpers/utils/get-user-channels-permissions';
 import { patchEntity } from '../helpers/utils/patch-entity';
 
 import { ChannelService } from './channel.service';
@@ -133,15 +136,46 @@ export class RoleService {
 
     /**
      * @description
-     * Returns true if the User has any of the specified permission on that Channel
+     * Returns true if the User has any of the specified permissions on that Channel
      */
     async userHasAnyPermissionsOnChannel(
         ctx: RequestContext,
         channelId: ID,
         permissions: Permission[],
     ): Promise<boolean> {
+        const permissionsOnChannel = await this.getActiveUserPermissionsOnChannel(ctx, channelId);
+        for (const permission of permissions) {
+            if (permissionsOnChannel.includes(permission)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /**
+     * @description
+     * Returns true if the User has all the specified permissions on that Channel
+     */
+    async userHasAllPermissionsOnChannel(
+        ctx: RequestContext,
+        channelId: ID,
+        permissions: Permission[],
+    ): Promise<boolean> {
+        const permissionsOnChannel = await this.getActiveUserPermissionsOnChannel(ctx, channelId);
+        for (const permission of permissions) {
+            if (!permissionsOnChannel.includes(permission)) {
+                return false;
+            }
+        }
+        return true;
+    }
+
+    private async getActiveUserPermissionsOnChannel(
+        ctx: RequestContext,
+        channelId: ID,
+    ): Promise<Permission[]> {
         if (ctx.activeUserId == null) {
-            return false;
+            return [];
         }
         const user = await this.connection.getEntityOrThrow(ctx, User, ctx.activeUserId, {
             relations: ['roles', 'roles.channels'],
@@ -149,14 +183,9 @@ export class RoleService {
         const userChannels = getUserChannelsPermissions(user);
         const channel = userChannels.find(c => idsAreEqual(c.id, channelId));
         if (!channel) {
-            return false;
-        }
-        for (const permission of permissions) {
-            if (channel.permissions.includes(permission)) {
-                return true;
-            }
+            return [];
         }
-        return false;
+        return channel.permissions;
     }
 
     async create(ctx: RequestContext, input: CreateRoleInput): Promise<Role> {
@@ -168,6 +197,7 @@ export class RoleService {
         } else {
             targetChannels = [ctx.channel];
         }
+        await this.checkActiveUserHasSufficientPermissions(ctx, targetChannels, input.permissions);
         const role = await this.createRoleForChannels(ctx, input, targetChannels);
         this.eventBus.publish(new RoleEvent(ctx, role, 'created', input));
         return role;
@@ -182,6 +212,16 @@ export class RoleService {
         if (role.code === SUPER_ADMIN_ROLE_CODE || role.code === CUSTOMER_ROLE_CODE) {
             throw new InternalServerError(`error.cannot-modify-role`, { roleCode: role.code });
         }
+        const targetChannels = input.channelIds
+            ? await this.getPermittedChannels(ctx, input.channelIds)
+            : undefined;
+        if (input.permissions) {
+            await this.checkActiveUserHasSufficientPermissions(
+                ctx,
+                targetChannels ?? role.channels,
+                input.permissions,
+            );
+        }
         const updatedRole = patchEntity(role, {
             code: input.code,
             description: input.description,
@@ -189,8 +229,8 @@ export class RoleService {
                 ? unique([Permission.Authenticated, ...input.permissions])
                 : undefined,
         });
-        if (input.channelIds && ctx.activeUserId) {
-            updatedRole.channels = await this.getPermittedChannels(ctx, input.channelIds);
+        if (targetChannels) {
+            updatedRole.channels = targetChannels;
         }
         await this.connection.getRepository(ctx, Role).save(updatedRole, { reload: false });
         this.eventBus.publish(new RoleEvent(ctx, role, 'updated', input));
@@ -246,6 +286,35 @@ export class RoleService {
         }
     }
 
+    /**
+     * @description
+     * Checks that the active User has sufficient Permissions on the target Channels to create
+     * a Role with the given Permissions. The rule is that an Administrator may only grant
+     * Permissions that they themselves already possess.
+     */
+    private async checkActiveUserHasSufficientPermissions(
+        ctx: RequestContext,
+        targetChannels: Channel[],
+        permissions: Permission[],
+    ) {
+        const permissionsRequired = getChannelPermissions([
+            new Role({
+                permissions: unique([Permission.Authenticated, ...permissions]),
+                channels: targetChannels,
+            }),
+        ]);
+        for (const channelPermissions of permissionsRequired) {
+            const activeUserHasRequiredPermissions = await this.userHasAllPermissionsOnChannel(
+                ctx,
+                channelPermissions.id,
+                channelPermissions.permissions,
+            );
+            if (!activeUserHasRequiredPermissions) {
+                throw new UserInputError('error.active-user-does-not-have-sufficient-permissions');
+            }
+        }
+    }
+
     private getRoleByCode(ctx: RequestContext | undefined, code: string) {
         const repository = ctx
             ? this.connection.getRepository(ctx, Role)