From 1eeadc61148df503bcd9fc944f8032ecae7438be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Mon, 24 Apr 2023 09:45:31 +0000 Subject: [PATCH] refactor(core): Setup decorator based RBAC (no-changelog) (#5787) --- .../UserManagement/UserManagementHelper.ts | 24 ------- .../cli/src/controllers/auth.controller.ts | 6 +- .../cli/src/controllers/ldap.controller.ts | 3 +- packages/cli/src/controllers/me.controller.ts | 3 +- .../src/controllers/nodeTypes.controller.ts | 3 +- .../cli/src/controllers/nodes.controller.ts | 12 +--- .../cli/src/controllers/owner.controller.ts | 3 +- .../controllers/passwordReset.controller.ts | 3 - .../cli/src/controllers/tags.controller.ts | 13 ++-- .../src/controllers/translation.controller.ts | 3 +- .../cli/src/controllers/users.controller.ts | 5 +- packages/cli/src/decorators/Authorized.ts | 16 +++++ packages/cli/src/decorators/constants.ts | 1 + packages/cli/src/decorators/index.ts | 1 + .../cli/src/decorators/registerController.ts | 43 ++++++++++-- packages/cli/src/decorators/types.ts | 4 ++ .../cli/src/eventbus/eventBus.controller.ts | 30 ++++----- packages/cli/src/middlewares/auth.ts | 65 ++++--------------- packages/cli/src/middlewares/isOwner.ts | 12 ---- .../saml/middleware/samlEnabledMiddleware.ts | 17 +---- .../src/sso/saml/routes/saml.controller.ee.ts | 15 +++-- .../cli/test/integration/shared/constants.ts | 2 +- .../cli/test/integration/users.api.test.ts | 14 +++- 23 files changed, 133 insertions(+), 165 deletions(-) create mode 100644 packages/cli/src/decorators/Authorized.ts delete mode 100644 packages/cli/src/middlewares/isOwner.ts diff --git a/packages/cli/src/UserManagement/UserManagementHelper.ts b/packages/cli/src/UserManagement/UserManagementHelper.ts index 3e705efbd6..88927f4c73 100644 --- a/packages/cli/src/UserManagement/UserManagementHelper.ts +++ b/packages/cli/src/UserManagement/UserManagementHelper.ts @@ -196,30 +196,6 @@ export async function getUserById(userId: string): Promise { return user; } -/** - * Check if a URL contains an auth-excluded endpoint. - */ -export function isAuthExcluded(url: string, ignoredEndpoints: Readonly): boolean { - return !!ignoredEndpoints - .filter(Boolean) // skip empty paths - .find((ignoredEndpoint) => url.startsWith(`/${ignoredEndpoint}`)); -} - -/** - * Check if the endpoint is `POST /users/:id`. - */ -export function isPostUsersId(req: express.Request, restEndpoint: string): boolean { - return ( - req.method === 'POST' && - new RegExp(`/${restEndpoint}/users/[\\w\\d-]*`).test(req.url) && - !req.url.includes('reinvite') - ); -} - -export function isAuthenticatedRequest(request: express.Request): request is AuthenticatedRequest { - return request.user !== undefined; -} - // ---------------------------------- // hashing // ---------------------------------- diff --git a/packages/cli/src/controllers/auth.controller.ts b/packages/cli/src/controllers/auth.controller.ts index 068522ce87..7aeb7e1cfe 100644 --- a/packages/cli/src/controllers/auth.controller.ts +++ b/packages/cli/src/controllers/auth.controller.ts @@ -1,5 +1,5 @@ import validator from 'validator'; -import { Get, Post, RestController } from '@/decorators'; +import { Authorized, Get, Post, RestController } from '@/decorators'; import { AuthError, BadRequestError, InternalServerError } from '@/ResponseHelper'; import { sanitizeUser, withFeatureFlags } from '@/UserManagement/UserManagementHelper'; import { issueCookie, resolveJwt } from '@/auth/jwt'; @@ -58,7 +58,6 @@ export class AuthController { /** * Log in a user. - * Authless endpoint. */ @Post('/login') async login(req: LoginRequest, res: Response): Promise { @@ -135,7 +134,6 @@ export class AuthController { /** * Validate invite token to enable invitee to set up their account. - * Authless endpoint. */ @Get('/resolve-signup-token') async resolveSignupToken(req: UserRequest.ResolveSignUp) { @@ -196,8 +194,8 @@ export class AuthController { /** * Log out a user. - * Authless endpoint. */ + @Authorized() @Post('/logout') logout(req: Request, res: Response) { res.clearCookie(AUTH_COOKIE_NAME); diff --git a/packages/cli/src/controllers/ldap.controller.ts b/packages/cli/src/controllers/ldap.controller.ts index 619a70db28..b354ef6600 100644 --- a/packages/cli/src/controllers/ldap.controller.ts +++ b/packages/cli/src/controllers/ldap.controller.ts @@ -1,5 +1,5 @@ import pick from 'lodash.pick'; -import { Get, Post, Put, RestController } from '@/decorators'; +import { Authorized, Get, Post, Put, RestController } from '@/decorators'; import { getLdapConfig, getLdapSynchronizations, updateLdapConfig } from '@/Ldap/helpers'; import { LdapService } from '@/Ldap/LdapService.ee'; import { LdapSync } from '@/Ldap/LdapSync.ee'; @@ -8,6 +8,7 @@ import { BadRequestError } from '@/ResponseHelper'; import { NON_SENSIBLE_LDAP_CONFIG_PROPERTIES } from '@/Ldap/constants'; import { InternalHooks } from '@/InternalHooks'; +@Authorized(['global', 'owner']) @RestController('/ldap') export class LdapController { constructor( diff --git a/packages/cli/src/controllers/me.controller.ts b/packages/cli/src/controllers/me.controller.ts index 3cf1ba409b..0ff4cd78f0 100644 --- a/packages/cli/src/controllers/me.controller.ts +++ b/packages/cli/src/controllers/me.controller.ts @@ -1,6 +1,6 @@ import validator from 'validator'; import { plainToInstance } from 'class-transformer'; -import { Delete, Get, Patch, Post, RestController } from '@/decorators'; +import { Authorized, Delete, Get, Patch, Post, RestController } from '@/decorators'; import { compareHash, hashPassword, @@ -30,6 +30,7 @@ import { randomBytes } from 'crypto'; import { isSamlLicensedAndEnabled } from '../sso/saml/samlHelpers'; import { UserService } from '@/user/user.service'; +@Authorized() @RestController('/me') export class MeController { private readonly logger: ILogger; diff --git a/packages/cli/src/controllers/nodeTypes.controller.ts b/packages/cli/src/controllers/nodeTypes.controller.ts index 5f8af5a909..63002c7c12 100644 --- a/packages/cli/src/controllers/nodeTypes.controller.ts +++ b/packages/cli/src/controllers/nodeTypes.controller.ts @@ -2,11 +2,12 @@ import { readFile } from 'fs/promises'; import get from 'lodash.get'; import { Request } from 'express'; import type { INodeTypeDescription, INodeTypeNameVersion } from 'n8n-workflow'; -import { Post, RestController } from '@/decorators'; +import { Authorized, Post, RestController } from '@/decorators'; import { getNodeTranslationPath } from '@/TranslationHelpers'; import type { Config } from '@/config'; import type { NodeTypes } from '@/NodeTypes'; +@Authorized() @RestController('/node-types') export class NodeTypesController { private readonly config: Config; diff --git a/packages/cli/src/controllers/nodes.controller.ts b/packages/cli/src/controllers/nodes.controller.ts index e82bd3e589..9d872c8693 100644 --- a/packages/cli/src/controllers/nodes.controller.ts +++ b/packages/cli/src/controllers/nodes.controller.ts @@ -4,7 +4,7 @@ import { STARTER_TEMPLATE_NAME, UNKNOWN_FAILURE_REASON, } from '@/constants'; -import { Delete, Get, Middleware, Patch, Post, RestController } from '@/decorators'; +import { Authorized, Delete, Get, Middleware, Patch, Post, RestController } from '@/decorators'; import { NodeRequest } from '@/requests'; import { BadRequestError, InternalServerError } from '@/ResponseHelper'; import { @@ -30,10 +30,10 @@ import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials'; import { InternalHooks } from '@/InternalHooks'; import { Push } from '@/push'; import { Config } from '@/config'; -import { isAuthenticatedRequest } from '@/UserManagement/UserManagementHelper'; const { PACKAGE_NOT_INSTALLED, PACKAGE_NAME_NOT_PROVIDED } = RESPONSE_ERROR_MESSAGES; +@Authorized(['global', 'owner']) @RestController('/nodes') export class NodesController { constructor( @@ -43,14 +43,6 @@ export class NodesController { private internalHooks: InternalHooks, ) {} - // TODO: move this into a new decorator `@Authorized` - @Middleware() - checkIfOwner(req: Request, res: Response, next: NextFunction) { - if (!isAuthenticatedRequest(req) || req.user.globalRole.name !== 'owner') - res.status(403).json({ status: 'error', message: 'Unauthorized' }); - else next(); - } - // TODO: move this into a new decorator `@IfConfig('executions.mode', 'queue')` @Middleware() checkIfCommunityNodesEnabled(req: Request, res: Response, next: NextFunction) { diff --git a/packages/cli/src/controllers/owner.controller.ts b/packages/cli/src/controllers/owner.controller.ts index d83a1fecee..ae8745a9af 100644 --- a/packages/cli/src/controllers/owner.controller.ts +++ b/packages/cli/src/controllers/owner.controller.ts @@ -1,6 +1,6 @@ import validator from 'validator'; import { validateEntity } from '@/GenericHelpers'; -import { Get, Post, RestController } from '@/decorators'; +import { Authorized, Get, Post, RestController } from '@/decorators'; import { BadRequestError } from '@/ResponseHelper'; import { hashPassword, @@ -20,6 +20,7 @@ import type { WorkflowRepository, } from '@db/repositories'; +@Authorized(['global', 'owner']) @RestController('/owner') export class OwnerController { private readonly config: Config; diff --git a/packages/cli/src/controllers/passwordReset.controller.ts b/packages/cli/src/controllers/passwordReset.controller.ts index d78424f6d7..64e4658206 100644 --- a/packages/cli/src/controllers/passwordReset.controller.ts +++ b/packages/cli/src/controllers/passwordReset.controller.ts @@ -65,7 +65,6 @@ export class PasswordResetController { /** * Send a password reset email. - * Authless endpoint. */ @Post('/forgot-password') async forgotPassword(req: PasswordResetRequest.Email) { @@ -171,7 +170,6 @@ export class PasswordResetController { /** * Verify password reset token and user ID. - * Authless endpoint. */ @Get('/resolve-password-token') async resolvePasswordToken(req: PasswordResetRequest.Credentials) { @@ -213,7 +211,6 @@ export class PasswordResetController { /** * Verify password reset token and user ID and update password. - * Authless endpoint. */ @Post('/change-password') async changePassword(req: PasswordResetRequest.NewPassword, res: Response) { diff --git a/packages/cli/src/controllers/tags.controller.ts b/packages/cli/src/controllers/tags.controller.ts index d8ccb4a28c..7b68d95aaf 100644 --- a/packages/cli/src/controllers/tags.controller.ts +++ b/packages/cli/src/controllers/tags.controller.ts @@ -1,13 +1,14 @@ import { Request, Response, NextFunction } from 'express'; import type { Config } from '@/config'; -import { Delete, Get, Middleware, Patch, Post, RestController } from '@/decorators'; +import { Authorized, Delete, Get, Middleware, Patch, Post, RestController } from '@/decorators'; import type { IDatabaseCollections, IExternalHooksClass, ITagWithCountDb } from '@/Interfaces'; import { TagEntity } from '@db/entities/TagEntity'; import type { TagRepository } from '@db/repositories'; import { validateEntity } from '@/GenericHelpers'; -import { BadRequestError, UnauthorizedError } from '@/ResponseHelper'; +import { BadRequestError } from '@/ResponseHelper'; import { TagsRequest } from '@/requests'; +@Authorized() @RestController('/tags') export class TagsController { private config: Config; @@ -91,15 +92,9 @@ export class TagsController { return tag; } + @Authorized(['global', 'owner']) @Delete('/:id(\\d+)') async deleteTag(req: TagsRequest.Delete) { - const isInstanceOwnerSetUp = this.config.getEnv('userManagement.isInstanceOwnerSetUp'); - if (isInstanceOwnerSetUp && req.user.globalRole.name !== 'owner') { - throw new UnauthorizedError( - 'You are not allowed to perform this action', - 'Only owners can remove tags', - ); - } const { id } = req.params; await this.externalHooks.run('tag.beforeDelete', [id]); diff --git a/packages/cli/src/controllers/translation.controller.ts b/packages/cli/src/controllers/translation.controller.ts index 8240a376a7..6d36d650d2 100644 --- a/packages/cli/src/controllers/translation.controller.ts +++ b/packages/cli/src/controllers/translation.controller.ts @@ -2,7 +2,7 @@ import type { Request } from 'express'; import { ICredentialTypes } from 'n8n-workflow'; import { join } from 'path'; import { access } from 'fs/promises'; -import { Get, RestController } from '@/decorators'; +import { Authorized, Get, RestController } from '@/decorators'; import { BadRequestError, InternalServerError } from '@/ResponseHelper'; import { Config } from '@/config'; import { NODES_BASE_DIR } from '@/constants'; @@ -14,6 +14,7 @@ export declare namespace TranslationRequest { export type Credential = Request<{}, {}, {}, { credentialType: string }>; } +@Authorized() @RestController('/') export class TranslationController { constructor(private config: Config, private credentialTypes: ICredentialTypes) {} diff --git a/packages/cli/src/controllers/users.controller.ts b/packages/cli/src/controllers/users.controller.ts index fc9faf06fd..c2ae07d344 100644 --- a/packages/cli/src/controllers/users.controller.ts +++ b/packages/cli/src/controllers/users.controller.ts @@ -5,7 +5,7 @@ import { ErrorReporterProxy as ErrorReporter } from 'n8n-workflow'; import { User } from '@db/entities/User'; import { SharedCredentials } from '@db/entities/SharedCredentials'; import { SharedWorkflow } from '@db/entities/SharedWorkflow'; -import { Delete, Get, Post, RestController } from '@/decorators'; +import { Authorized, NoAuthRequired, Delete, Get, Post, RestController } from '@/decorators'; import { addInviteLinkToUser, generateUserInviteUrl, @@ -41,6 +41,7 @@ import type { UserRepository, } from '@db/repositories'; +@Authorized(['global', 'owner']) @RestController('/users') export class UsersController { private config: Config; @@ -282,6 +283,7 @@ export class UsersController { /** * Fill out user shell with first name, last name, and password. */ + @NoAuthRequired() @Post('/:id') async updateUser(req: UserRequest.Update, res: Response) { const { id: inviteeId } = req.params; @@ -343,6 +345,7 @@ export class UsersController { return withFeatureFlags(this.postHog, sanitizeUser(updatedUser)); } + @Authorized('any') @Get('/') async listUsers(req: UserRequest.List) { const users = await this.userRepository.find({ relations: ['globalRole', 'authIdentities'] }); diff --git a/packages/cli/src/decorators/Authorized.ts b/packages/cli/src/decorators/Authorized.ts new file mode 100644 index 0000000000..880938ae5c --- /dev/null +++ b/packages/cli/src/decorators/Authorized.ts @@ -0,0 +1,16 @@ +/* eslint-disable @typescript-eslint/ban-types */ +/* eslint-disable @typescript-eslint/naming-convention */ +import { CONTROLLER_AUTH_ROLES } from './constants'; +import type { AuthRoleMetadata } from './types'; + +export function Authorized(authRole: AuthRoleMetadata[string] = 'any'): Function { + return function (target: Function | Object, handlerName?: string) { + const controllerClass = handlerName ? target.constructor : target; + const authRoles = (Reflect.getMetadata(CONTROLLER_AUTH_ROLES, controllerClass) ?? + {}) as AuthRoleMetadata; + authRoles[handlerName ?? '*'] = authRole; + Reflect.defineMetadata(CONTROLLER_AUTH_ROLES, authRoles, controllerClass); + }; +} + +export const NoAuthRequired = () => Authorized('none'); diff --git a/packages/cli/src/decorators/constants.ts b/packages/cli/src/decorators/constants.ts index 6bff0e4a6c..5a5efc938d 100644 --- a/packages/cli/src/decorators/constants.ts +++ b/packages/cli/src/decorators/constants.ts @@ -1,3 +1,4 @@ export const CONTROLLER_ROUTES = 'CONTROLLER_ROUTES'; export const CONTROLLER_BASE_PATH = 'CONTROLLER_BASE_PATH'; export const CONTROLLER_MIDDLEWARES = 'CONTROLLER_MIDDLEWARES'; +export const CONTROLLER_AUTH_ROLES = 'CONTROLLER_AUTH_ROLES'; diff --git a/packages/cli/src/decorators/index.ts b/packages/cli/src/decorators/index.ts index 71b82b5b69..0e683d410c 100644 --- a/packages/cli/src/decorators/index.ts +++ b/packages/cli/src/decorators/index.ts @@ -1,3 +1,4 @@ +export { Authorized, NoAuthRequired } from './Authorized'; export { RestController } from './RestController'; export { Get, Post, Put, Patch, Delete } from './Route'; export { Middleware } from './Middleware'; diff --git a/packages/cli/src/decorators/registerController.ts b/packages/cli/src/decorators/registerController.ts index e20293ae21..fe792040b6 100644 --- a/packages/cli/src/decorators/registerController.ts +++ b/packages/cli/src/decorators/registerController.ts @@ -1,10 +1,36 @@ /* eslint-disable @typescript-eslint/naming-convention */ import { Router } from 'express'; -import type { Config } from '@/config'; -import { CONTROLLER_BASE_PATH, CONTROLLER_MIDDLEWARES, CONTROLLER_ROUTES } from './constants'; -import { send } from '@/ResponseHelper'; // TODO: move `ResponseHelper.send` to this file import type { Application, Request, Response, RequestHandler } from 'express'; -import type { Controller, MiddlewareMetadata, RouteMetadata } from './types'; +import type { Config } from '@/config'; +import type { AuthenticatedRequest } from '@/requests'; +import { send } from '@/ResponseHelper'; // TODO: move `ResponseHelper.send` to this file +import { + CONTROLLER_AUTH_ROLES, + CONTROLLER_BASE_PATH, + CONTROLLER_MIDDLEWARES, + CONTROLLER_ROUTES, +} from './constants'; +import type { + AuthRole, + AuthRoleMetadata, + Controller, + MiddlewareMetadata, + RouteMetadata, +} from './types'; + +export const createAuthMiddleware = + (authRole: AuthRole): RequestHandler => + ({ user }: AuthenticatedRequest, res, next) => { + if (authRole === 'none') return next(); + + if (!user) return res.status(401).json({ status: 'error', message: 'Unauthorized' }); + + const { globalRole } = user; + if (authRole === 'any' || (globalRole.scope === authRole[0] && globalRole.name === authRole[1])) + return next(); + + res.status(403).json({ status: 'error', message: 'Unauthorized' }); + }; export const registerController = (app: Application, config: Config, controller: object) => { const controllerClass = controller.constructor; @@ -14,11 +40,16 @@ export const registerController = (app: Application, config: Config, controller: if (!controllerBasePath) throw new Error(`${controllerClass.name} is missing the RestController decorator`); + const authRoles = Reflect.getMetadata(CONTROLLER_AUTH_ROLES, controllerClass) as + | AuthRoleMetadata + | undefined; const routes = Reflect.getMetadata(CONTROLLER_ROUTES, controllerClass) as RouteMetadata[]; if (routes.length > 0) { const router = Router({ mergeParams: true }); const restBasePath = config.getEnv('endpoints.rest'); - const prefix = `/${[restBasePath, controllerBasePath].join('/')}`.replace(/\/+/g, '/'); + const prefix = `/${[restBasePath, controllerBasePath].join('/')}` + .replace(/\/+/g, '/') + .replace(/\/$/, ''); const controllerMiddlewares = ( (Reflect.getMetadata(CONTROLLER_MIDDLEWARES, controllerClass) ?? []) as MiddlewareMetadata[] @@ -28,8 +59,10 @@ export const registerController = (app: Application, config: Config, controller: ); routes.forEach(({ method, path, middlewares: routeMiddlewares, handlerName }) => { + const authRole = authRoles && (authRoles[handlerName] ?? authRoles['*']); router[method]( path, + ...(authRole ? [createAuthMiddleware(authRole)] : []), ...controllerMiddlewares, ...routeMiddlewares, send(async (req: Request, res: Response) => diff --git a/packages/cli/src/decorators/types.ts b/packages/cli/src/decorators/types.ts index 250abf0d27..d118e6e6c9 100644 --- a/packages/cli/src/decorators/types.ts +++ b/packages/cli/src/decorators/types.ts @@ -1,7 +1,11 @@ import type { Request, Response, RequestHandler } from 'express'; +import type { RoleNames, RoleScopes } from '@db/entities/Role'; export type Method = 'get' | 'post' | 'put' | 'patch' | 'delete'; +export type AuthRole = [RoleScopes, RoleNames] | 'any' | 'none'; +export type AuthRoleMetadata = Record; + export interface MiddlewareMetadata { handlerName: string; } diff --git a/packages/cli/src/eventbus/eventBus.controller.ts b/packages/cli/src/eventbus/eventBus.controller.ts index bc18472d4c..7c9ce21ccc 100644 --- a/packages/cli/src/eventbus/eventBus.controller.ts +++ b/packages/cli/src/eventbus/eventBus.controller.ts @@ -28,15 +28,13 @@ import type { IRunExecutionData, } from 'n8n-workflow'; import { MessageEventBusDestinationTypeNames, EventMessageTypeNames } from 'n8n-workflow'; -import type { User } from '@db/entities/User'; -import * as ResponseHelper from '@/ResponseHelper'; import type { EventMessageNodeOptions } from './EventMessageClasses/EventMessageNode'; import { EventMessageNode } from './EventMessageClasses/EventMessageNode'; import { recoverExecutionDataFromEventLogMessages } from './MessageEventBus/recoverEvents'; -import { RestController, Get, Post, Delete } from '@/decorators'; +import { RestController, Get, Post, Delete, Authorized } from '@/decorators'; import type { MessageEventBusDestination } from './MessageEventBusDestination/MessageEventBusDestination.ee'; -import { isOwnerMiddleware } from '../middlewares/isOwner'; import type { DeleteResult } from 'typeorm'; +import { AuthenticatedRequest } from '@/requests'; // ---------------------------------------- // TypeGuards @@ -74,12 +72,14 @@ const isMessageEventBusDestinationOptions = ( // Controller // ---------------------------------------- +@Authorized() @RestController('/eventbus') export class EventBusController { // ---------------------------------------- // Events // ---------------------------------------- - @Get('/event', { middlewares: [isOwnerMiddleware] }) + @Authorized(['global', 'owner']) + @Get('/event') async getEvents( req: express.Request, ): Promise> { @@ -132,7 +132,8 @@ export class EventBusController { return; } - @Post('/event', { middlewares: [isOwnerMiddleware] }) + @Authorized(['global', 'owner']) + @Post('/event') async postEvent(req: express.Request): Promise { let msg: EventMessageTypes | undefined; if (isEventMessageOptions(req.body)) { @@ -172,12 +173,9 @@ export class EventBusController { } } - @Post('/destination', { middlewares: [isOwnerMiddleware] }) - async postDestination(req: express.Request): Promise { - if (!req.user || (req.user as User).globalRole.name !== 'owner') { - throw new ResponseHelper.UnauthorizedError('Invalid request'); - } - + @Authorized(['global', 'owner']) + @Post('/destination') + async postDestination(req: AuthenticatedRequest): Promise { let result: MessageEventBusDestination | undefined; if (isMessageEventBusDestinationOptions(req.body)) { switch (req.body.__type) { @@ -228,11 +226,9 @@ export class EventBusController { return false; } - @Delete('/destination', { middlewares: [isOwnerMiddleware] }) - async deleteDestination(req: express.Request): Promise { - if (!req.user || (req.user as User).globalRole.name !== 'owner') { - throw new ResponseHelper.UnauthorizedError('Invalid request'); - } + @Authorized(['global', 'owner']) + @Delete('/destination') + async deleteDestination(req: AuthenticatedRequest): Promise { if (isWithIdString(req.query)) { return eventBus.removeDestination(req.query.id); } else { diff --git a/packages/cli/src/middlewares/auth.ts b/packages/cli/src/middlewares/auth.ts index bd0f580c8a..231b48bedd 100644 --- a/packages/cli/src/middlewares/auth.ts +++ b/packages/cli/src/middlewares/auth.ts @@ -10,13 +10,7 @@ import type { AuthenticatedRequest } from '@/requests'; import config from '@/config'; import { AUTH_COOKIE_NAME, EDITOR_UI_DIST_DIR } from '@/constants'; import { issueCookie, resolveJwtContent } from '@/auth/jwt'; -import { - isAuthenticatedRequest, - isAuthExcluded, - isPostUsersId, - isUserManagementEnabled, -} from '@/UserManagement/UserManagementHelper'; -import { SamlUrls } from '@/sso/saml/constants'; +import { isUserManagementEnabled } from '@/UserManagement/UserManagementHelper'; import type { UserRepository } from '@db/repositories'; const jwtFromRequest = (req: Request) => { @@ -66,6 +60,17 @@ const staticAssets = globSync(['**/*.html', '**/*.svg', '**/*.png', '**/*.ico'], cwd: EDITOR_UI_DIST_DIR, }); +// TODO: delete this +const isPostUsersId = (req: Request, restEndpoint: string): boolean => + req.method === 'POST' && + new RegExp(`/${restEndpoint}/users/[\\w\\d-]*`).test(req.url) && + !req.url.includes('reinvite'); + +const isAuthExcluded = (url: string, ignoredEndpoints: Readonly): boolean => + !!ignoredEndpoints + .filter(Boolean) // skip empty paths + .find((ignoredEndpoint) => url.startsWith(`/${ignoredEndpoint}`)); + /** * This sets up the auth middlewares in the correct order */ @@ -85,20 +90,16 @@ export const setupAuthMiddlewares = ( // skip authentication for preflight requests req.method === 'OPTIONS' || staticAssets.includes(req.url.slice(1)) || + isAuthExcluded(req.url, ignoredEndpoints) || req.url.startsWith(`/${restEndpoint}/settings`) || req.url.startsWith(`/${restEndpoint}/login`) || - req.url.startsWith(`/${restEndpoint}/logout`) || req.url.startsWith(`/${restEndpoint}/resolve-signup-token`) || isPostUsersId(req, restEndpoint) || req.url.startsWith(`/${restEndpoint}/forgot-password`) || req.url.startsWith(`/${restEndpoint}/resolve-password-token`) || req.url.startsWith(`/${restEndpoint}/change-password`) || req.url.startsWith(`/${restEndpoint}/oauth2-credential/callback`) || - req.url.startsWith(`/${restEndpoint}/oauth1-credential/callback`) || - req.url.startsWith(`/${restEndpoint}/sso/saml${SamlUrls.metadata}`) || - req.url.startsWith(`/${restEndpoint}/sso/saml${SamlUrls.initSSO}`) || - req.url.startsWith(`/${restEndpoint}/sso/saml${SamlUrls.acs}`) || - isAuthExcluded(req.url, ignoredEndpoints) + req.url.startsWith(`/${restEndpoint}/oauth1-credential/callback`) ) { return next(); } @@ -115,43 +116,5 @@ export const setupAuthMiddlewares = ( return passportMiddleware(req, res, next); }); - app.use((req: Request | AuthenticatedRequest, res: Response, next: NextFunction) => { - // req.user is empty for public routes, so just proceed - // owner can do anything, so proceed as well - if (!req.user || (isAuthenticatedRequest(req) && req.user.globalRole.name === 'owner')) { - next(); - return; - } - // Not owner and user exists. We now protect restricted urls. - const postRestrictedUrls = [ - `/${restEndpoint}/users`, - `/${restEndpoint}/owner`, - `/${restEndpoint}/ldap/sync`, - `/${restEndpoint}/ldap/test-connection`, - ]; - const getRestrictedUrls = [`/${restEndpoint}/ldap/sync`, `/${restEndpoint}/ldap/config`]; - const putRestrictedUrls = [`/${restEndpoint}/ldap/config`]; - const trimmedUrl = req.url.endsWith('/') ? req.url.slice(0, -1) : req.url; - if ( - (req.method === 'POST' && postRestrictedUrls.includes(trimmedUrl)) || - (req.method === 'GET' && getRestrictedUrls.includes(trimmedUrl)) || - (req.method === 'PUT' && putRestrictedUrls.includes(trimmedUrl)) || - (req.method === 'DELETE' && - new RegExp(`/${restEndpoint}/users/[^/]+`, 'gm').test(trimmedUrl)) || - (req.method === 'POST' && - new RegExp(`/${restEndpoint}/users/[^/]+/reinvite`, 'gm').test(trimmedUrl)) || - new RegExp(`/${restEndpoint}/owner/[^/]+`, 'gm').test(trimmedUrl) - ) { - Logger.verbose('User attempted to access endpoint without authorization', { - endpoint: `${req.method} ${trimmedUrl}`, - userId: isAuthenticatedRequest(req) ? req.user.id : 'unknown', - }); - res.status(403).json({ status: 'error', message: 'Unauthorized' }); - return; - } - - next(); - }); - app.use(refreshExpiringCookie); }; diff --git a/packages/cli/src/middlewares/isOwner.ts b/packages/cli/src/middlewares/isOwner.ts deleted file mode 100644 index d3e3c70a0f..0000000000 --- a/packages/cli/src/middlewares/isOwner.ts +++ /dev/null @@ -1,12 +0,0 @@ -import type { RequestHandler } from 'express'; -import { LoggerProxy } from 'n8n-workflow'; -import type { AuthenticatedRequest } from '@/requests'; - -export const isOwnerMiddleware: RequestHandler = (req: AuthenticatedRequest, res, next) => { - if (req.user.globalRole.name === 'owner') { - next(); - } else { - LoggerProxy.debug('Request failed because user is not owner'); - res.status(401).send('Unauthorized'); - } -}; diff --git a/packages/cli/src/sso/saml/middleware/samlEnabledMiddleware.ts b/packages/cli/src/sso/saml/middleware/samlEnabledMiddleware.ts index 6e4600b895..69015838d7 100644 --- a/packages/cli/src/sso/saml/middleware/samlEnabledMiddleware.ts +++ b/packages/cli/src/sso/saml/middleware/samlEnabledMiddleware.ts @@ -1,24 +1,11 @@ import type { RequestHandler } from 'express'; -import type { AuthenticatedRequest } from '@/requests'; import { isSamlLicensed, isSamlLicensedAndEnabled } from '../samlHelpers'; -export const samlLicensedOwnerMiddleware: RequestHandler = ( - req: AuthenticatedRequest, - res, - next, -) => { - if (isSamlLicensed() && req.user?.globalRole.name === 'owner') { - next(); - } else { - res.status(401).json({ status: 'error', message: 'Unauthorized' }); - } -}; - export const samlLicensedAndEnabledMiddleware: RequestHandler = (req, res, next) => { if (isSamlLicensedAndEnabled()) { next(); } else { - res.status(401).json({ status: 'error', message: 'Unauthorized' }); + res.status(403).json({ status: 'error', message: 'Unauthorized' }); } }; @@ -26,6 +13,6 @@ export const samlLicensedMiddleware: RequestHandler = (req, res, next) => { if (isSamlLicensed()) { next(); } else { - res.status(401).json({ status: 'error', message: 'Unauthorized' }); + res.status(403).json({ status: 'error', message: 'Unauthorized' }); } }; diff --git a/packages/cli/src/sso/saml/routes/saml.controller.ee.ts b/packages/cli/src/sso/saml/routes/saml.controller.ee.ts index 86e7b75cfa..232020b594 100644 --- a/packages/cli/src/sso/saml/routes/saml.controller.ee.ts +++ b/packages/cli/src/sso/saml/routes/saml.controller.ee.ts @@ -1,10 +1,9 @@ import express from 'express'; -import { Get, Post, RestController } from '@/decorators'; +import { Authorized, Get, Post, RestController } from '@/decorators'; import { SamlUrls } from '../constants'; import { samlLicensedAndEnabledMiddleware, samlLicensedMiddleware, - samlLicensedOwnerMiddleware, } from '../middleware/samlEnabledMiddleware'; import { SamlService } from '../saml.service.ee'; import { SamlConfiguration } from '../types/requests'; @@ -39,7 +38,8 @@ export class SamlController { * GET /sso/saml/config * Return SAML config */ - @Get(SamlUrls.config, { middlewares: [samlLicensedOwnerMiddleware] }) + @Authorized(['global', 'owner']) + @Get(SamlUrls.config, { middlewares: [samlLicensedMiddleware] }) async configGet() { const prefs = this.samlService.samlPreferences; return { @@ -53,7 +53,8 @@ export class SamlController { * POST /sso/saml/config * Set SAML config */ - @Post(SamlUrls.config, { middlewares: [samlLicensedOwnerMiddleware] }) + @Authorized(['global', 'owner']) + @Post(SamlUrls.config, { middlewares: [samlLicensedMiddleware] }) async configPost(req: SamlConfiguration.Update) { const validationResult = await validate(req.body); if (validationResult.length === 0) { @@ -71,7 +72,8 @@ export class SamlController { * POST /sso/saml/config/toggle * Set SAML config */ - @Post(SamlUrls.configToggleEnabled, { middlewares: [samlLicensedOwnerMiddleware] }) + @Authorized(['global', 'owner']) + @Post(SamlUrls.configToggleEnabled, { middlewares: [samlLicensedMiddleware] }) async toggleEnabledPost(req: SamlConfiguration.Toggle, res: express.Response) { if (req.body.loginEnabled === undefined) { throw new BadRequestError('Body should contain a boolean "loginEnabled" property'); @@ -155,7 +157,8 @@ export class SamlController { * Test SAML config * This endpoint is available if SAML is licensed and the requestor is an instance owner */ - @Get(SamlUrls.configTest, { middlewares: [samlLicensedOwnerMiddleware] }) + @Authorized(['global', 'owner']) + @Get(SamlUrls.configTest, { middlewares: [samlLicensedMiddleware] }) async configTestGet(req: AuthenticatedRequest, res: express.Response) { return this.handleInitSSO(res, getServiceProviderConfigTestReturnUrl()); } diff --git a/packages/cli/test/integration/shared/constants.ts b/packages/cli/test/integration/shared/constants.ts index 3608225eaa..37798aa4d0 100644 --- a/packages/cli/test/integration/shared/constants.ts +++ b/packages/cli/test/integration/shared/constants.ts @@ -42,7 +42,7 @@ export const ROUTES_REQUIRING_AUTHORIZATION: Readonly = [ 'POST /users', 'DELETE /users/123', 'POST /users/123/reinvite', - 'POST /owner/pre-setup', + 'GET /owner/pre-setup', 'POST /owner/setup', 'POST /owner/skip-setup', ]; diff --git a/packages/cli/test/integration/users.api.test.ts b/packages/cli/test/integration/users.api.test.ts index 759d12b8cb..966bb6d169 100644 --- a/packages/cli/test/integration/users.api.test.ts +++ b/packages/cli/test/integration/users.api.test.ts @@ -31,6 +31,7 @@ let credentialOwnerRole: Role; let owner: User; let authlessAgent: SuperAgentTest; let authOwnerAgent: SuperAgentTest; +let authAgentFor: (user: User) => SuperAgentTest; beforeAll(async () => { const app = await utils.initTestServer({ endpointGroups: ['users'] }); @@ -49,7 +50,8 @@ beforeAll(async () => { owner = await testDb.createUser({ globalRole: globalOwnerRole }); authlessAgent = utils.createAgent(app); - authOwnerAgent = utils.createAuthAgent(app)(owner); + authAgentFor = utils.createAuthAgent(app); + authOwnerAgent = authAgentFor(owner); }); beforeEach(async () => { @@ -69,7 +71,7 @@ afterAll(async () => { }); describe('GET /users', () => { - test('should return all users', async () => { + test('should return all users (for owner)', async () => { await testDb.createUser({ globalRole: globalMemberRole }); const response = await authOwnerAgent.get('/users'); @@ -103,6 +105,14 @@ describe('GET /users', () => { expect(apiKey).not.toBeDefined(); }); }); + + test('should return all users (for member)', async () => { + const member = await testDb.createUser({ globalRole: globalMemberRole }); + const response = await authAgentFor(member).get('/users'); + + expect(response.statusCode).toBe(200); + expect(response.body.data.length).toBe(2); + }); }); describe('DELETE /users/:id', () => {