mirror of
https://github.com/Unleash/unleash.git
synced 2024-12-22 19:07:54 +01:00
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.
This commit is contained in:
parent
faf2153423
commit
55da9b8133
@ -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;
|
||||
|
@ -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(),
|
||||
|
@ -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) {
|
||||
|
@ -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
|
||||
*/
|
||||
|
Loading…
Reference in New Issue
Block a user