From 55da9b81336b7943c9468f78f6173aaf32126a75 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Tue, 12 Mar 2024 10:39:37 +0100 Subject: [PATCH] fix: admin token requests does not automatically have id (#6501) To check that users do indeed have permissions to update the roles from project-service, we've been depending on req.user.id. We had one error on Friday March 8th, where we managed to send undefined/null to a method that requires a number. This PR assumes that if we have an API token, and we have admin permissions and userId is not set we're a legacy admin token. It uses the util method for extractUserId(req: IAuthRequest | IApiRequest), so if we've passed through the apiTokenMiddleware first, we'll have userId -42, if we haven't, we'll get -1337. --- src/lib/features/project/project-service.ts | 12 +++- src/lib/middleware/rbac-middleware.test.ts | 61 ++++++++++++++++++++- src/lib/middleware/rbac-middleware.ts | 12 +++- src/lib/services/access-service.ts | 4 +- 4 files changed, 81 insertions(+), 8 deletions(-) diff --git a/src/lib/features/project/project-service.ts b/src/lib/features/project/project-service.ts index 31db616405..d627ba8b11 100644 --- a/src/lib/features/project/project-service.ts +++ b/src/lib/features/project/project-service.ts @@ -8,6 +8,7 @@ import { nameType } from '../../routes/util'; import { projectSchema } from '../../services/project-schema'; import NotFoundError from '../../error/notfound-error'; import { + ADMIN_TOKEN_USER, CreateProject, DEFAULT_PROJECT, FeatureToggle, @@ -44,6 +45,7 @@ import { ProjectUserUpdateRoleEvent, RoleName, SYSTEM_USER, + SYSTEM_USER_ID, } from '../../types'; import { IProjectAccessModel, @@ -701,8 +703,12 @@ export default class ProjectService { ); } - private isAdmin(roles: IRoleWithProject[]): boolean { - return roles.some((r) => r.name === RoleName.ADMIN); + private isAdmin(userId: number, roles: IRoleWithProject[]): boolean { + return ( + userId === SYSTEM_USER_ID || + userId === ADMIN_TOKEN_USER.id || + roles.some((r) => r.name === RoleName.ADMIN) + ); } private isProjectOwner( @@ -723,7 +729,7 @@ export default class ProjectService { projectId, ); if ( - this.isAdmin(userRoles) || + this.isAdmin(userAddingAccess, userRoles) || this.isProjectOwner(userRoles, projectId) ) { return true; diff --git a/src/lib/middleware/rbac-middleware.test.ts b/src/lib/middleware/rbac-middleware.test.ts index 05a9f1501d..0e9b84572d 100644 --- a/src/lib/middleware/rbac-middleware.test.ts +++ b/src/lib/middleware/rbac-middleware.test.ts @@ -7,7 +7,7 @@ import ApiUser from '../types/api-user'; import { IFeatureToggleStore } from '../features/feature-toggle/types/feature-toggle-store-type'; import FakeFeatureToggleStore from '../features/feature-toggle/fakes/fake-feature-toggle-store'; import { ApiTokenType } from '../types/models/api-token'; -import { ISegmentStore } from '../types'; +import { ISegmentStore, SYSTEM_USER_ID } from '../types'; import FakeSegmentStore from '../../test/fixtures/fake-segment-store'; let config: IUnleashConfig; @@ -73,6 +73,65 @@ test('should give api-user ADMIN permission', async () => { expect(hasAccess).toBe(true); }); +describe('ADMIN tokens should have user id -1337 when only passed through rbac-middleware', () => { + /// Will be -42 (ADMIN_USER.id) when we have the api-token-middleware run first + test('Should give ADMIN api-user userid -1337 (SYSTEM_USER_ID)', async () => { + const accessService = { + hasPermission: jest.fn(), + }; + + const func = rbacMiddleware( + config, + { featureToggleStore, segmentStore }, + accessService, + ); + + const cb = jest.fn(); + const req: any = { + user: new ApiUser({ + tokenName: 'api', + permissions: [perms.ADMIN], + project: '*', + environment: '*', + type: ApiTokenType.ADMIN, + secret: 'a', + }), + }; + func(req, undefined, cb); + const hasAccess = await req.checkRbac(perms.ADMIN); + expect(req.user.id).toBe(SYSTEM_USER_ID); + expect(hasAccess).toBe(true); + }); + /// Will be -42 (ADMIN_USER.id) when we have the api-token-middleware run first + test('Also when checking against permission NONE, userid should still be -1337', async () => { + const accessService = { + hasPermission: jest.fn(), + }; + + const func = rbacMiddleware( + config, + { featureToggleStore, segmentStore }, + accessService, + ); + + const cb = jest.fn(); + const req: any = { + user: new ApiUser({ + tokenName: 'api', + permissions: [perms.ADMIN], + project: '*', + environment: '*', + type: ApiTokenType.ADMIN, + secret: 'a', + }), + }; + func(req, undefined, cb); + const hasAccess = await req.checkRbac(perms.NONE); + expect(req.user.id).toBe(SYSTEM_USER_ID); + expect(hasAccess).toBe(true); + }); +}); + test('should not give api-user ADMIN permission', async () => { const accessService = { hasPermission: jest.fn(), diff --git a/src/lib/middleware/rbac-middleware.ts b/src/lib/middleware/rbac-middleware.ts index 541c0d797e..d59c2cc274 100644 --- a/src/lib/middleware/rbac-middleware.ts +++ b/src/lib/middleware/rbac-middleware.ts @@ -1,7 +1,7 @@ import { + ADMIN, CREATE_FEATURE, DELETE_FEATURE, - ADMIN, UPDATE_FEATURE, UPDATE_PROJECT_SEGMENT, } from '../types/permissions'; @@ -9,6 +9,7 @@ import { IUnleashConfig } from '../types/option'; import { IUnleashStores } from '../types/stores'; import User from '../types/user'; import { Request } from 'express'; +import { extractUserId } from '../util'; interface PermissionChecker { hasPermission( @@ -56,7 +57,14 @@ const rbacMiddleware = ( } if (user.isAPI) { - return user.permissions.includes(ADMIN); + if (user.permissions.includes(ADMIN)) { + if (!req.user.id) { + req.user.id = extractUserId(req); + } + return true; + } else { + return false; + } } if (!user.id) { diff --git a/src/lib/services/access-service.ts b/src/lib/services/access-service.ts index 8f33a85e3e..822201e665 100644 --- a/src/lib/services/access-service.ts +++ b/src/lib/services/access-service.ts @@ -31,8 +31,8 @@ import { roleSchema } from '../schema/role-schema'; import { ALL_ENVS, ALL_PROJECTS, - CUSTOM_ROOT_ROLE_TYPE, CUSTOM_PROJECT_ROLE_TYPE, + CUSTOM_ROOT_ROLE_TYPE, ROOT_ROLE_TYPES, } from '../util/constants'; import { DEFAULT_PROJECT } from '../types/project'; @@ -219,7 +219,7 @@ export class AccessService { /** * Returns all roles the user has in the project. * Including roles via groups. - * In addition it includes root roles + * In addition, it includes root roles * @param userId user to find roles for * @param project project to find roles for */