fix: always require permission for POST, PATCH, PUT, DELETE (#1152)

src/lib/routes/admin-api/context.ts

@@ -41,7 +41,7 @@ class ContextController extends Controller {
             this.deleteContextField,
             DELETE_CONTEXT_FIELD,
         );
-        this.post('/validate', this.validate);
+        this.post('/validate', this.validate, UPDATE_CONTEXT_FIELD);
     }
 
     async getContextFields(req: Request, res: Response): Promise<void> {

src/lib/routes/admin-api/feature.ts

@@ -44,7 +44,7 @@ class FeatureController extends Controller {
         this.get('/:featureName', this.getToggle);
         this.put('/:featureName', this.updateToggle, UPDATE_FEATURE);
         this.delete('/:featureName', this.archiveToggle, DELETE_FEATURE);
-        this.post('/validate', this.validate);
+        this.post('/validate', this.validate, UPDATE_FEATURE);
         this.post('/:featureName/toggle', this.toggle, UPDATE_FEATURE);
         this.post('/:featureName/toggle/on', this.toggleOn, UPDATE_FEATURE);
         this.post('/:featureName/toggle/off', this.toggleOff, UPDATE_FEATURE);

src/lib/routes/admin-api/project/features.ts

@@ -5,7 +5,11 @@ import { IUnleashConfig } from '../../../types/option';
 import { IUnleashServices } from '../../../types/services';
 import FeatureToggleService from '../../../services/feature-toggle-service';
 import { Logger } from '../../../logger';
-import { CREATE_FEATURE, UPDATE_FEATURE } from '../../../types/permissions';
+import {
+    CREATE_FEATURE,
+    DELETE_FEATURE,
+    UPDATE_FEATURE,
+} from '../../../types/permissions';
 import {
     FeatureToggleDTO,
     IConstraint,
@@ -84,9 +88,9 @@ export default class ProjectFeaturesController extends Controller {
         this.post(PATH_FEATURE_CLONE, this.cloneFeature, CREATE_FEATURE);
 
         this.get(PATH_FEATURE, this.getFeature);
-        this.put(PATH_FEATURE, this.updateFeature);
+        this.put(PATH_FEATURE, this.updateFeature, UPDATE_FEATURE);
-        this.patch(PATH_FEATURE, this.patchFeature);
+        this.patch(PATH_FEATURE, this.patchFeature, UPDATE_FEATURE);
-        this.delete(PATH_FEATURE, this.archiveFeature);
+        this.delete(PATH_FEATURE, this.archiveFeature, DELETE_FEATURE);
     }
 
     async getFeatures(

src/lib/routes/admin-api/tag-type.ts

@@ -25,7 +25,7 @@ class TagTypeController extends Controller {
         this.tagTypeService = tagTypeService;
         this.get('/', this.getTagTypes);
         this.post('/', this.createTagType, UPDATE_TAG_TYPE);
-        this.post('/validate', this.validate);
+        this.post('/validate', this.validate, UPDATE_TAG_TYPE);
         this.get('/:name', this.getTagType);
         this.put('/:name', this.updateTagType, UPDATE_TAG_TYPE);
         this.delete('/:name', this.deleteTagType, DELETE_TAG_TYPE);

src/lib/routes/admin-api/user-admin.ts

@@ -1,6 +1,6 @@
 import { Request, Response } from 'express';
 import Controller from '../controller';
-import { ADMIN } from '../../types/permissions';
+import { ADMIN, NONE } from '../../types/permissions';
 import UserService from '../../services/user-service';
 import { AccessService } from '../../services/access-service';
 import { Logger } from '../../logger';
@@ -60,12 +60,12 @@ export default class UserAdminController extends Controller {
         this.get('/', this.getUsers, ADMIN);
         this.get('/search', this.search);
         this.post('/', this.createUser, ADMIN);
-        this.post('/validate-password', this.validatePassword);
+        this.post('/validate-password', this.validatePassword, NONE);
         this.get('/:id', this.getUser, ADMIN);
         this.put('/:id', this.updateUser, ADMIN);
         this.post('/:id/change-password', this.changePassword, ADMIN);
         this.delete('/:id', this.deleteUser, ADMIN);
-        this.post('/reset-password', this.resetPassword);
+        this.post('/reset-password', this.resetPassword, ADMIN);
         this.get('/active-sessions', this.getActiveSessions, ADMIN);
     }
 

src/lib/routes/admin-api/user-feedback-controller.ts

@@ -6,6 +6,7 @@ import { IUnleashConfig } from '../../types/option';
 import { IUnleashServices } from '../../types/services';
 import UserFeedbackService from '../../services/user-feedback-service';
 import { IAuthRequest } from '../unleash-types';
+import { NONE } from '../../types/permissions';
 
 interface IFeedbackBody {
     neverShow?: boolean;
@@ -26,8 +27,8 @@ class UserFeedbackController extends Controller {
         this.logger = config.getLogger('feedback-controller.ts');
         this.userFeedbackService = userFeedbackService;
 
-        this.post('/', this.recordFeedback);
+        this.post('/', this.recordFeedback, NONE);
-        this.put('/:id', this.updateFeedbackSettings);
+        this.put('/:id', this.updateFeedbackSettings, NONE);
     }
 
     private async recordFeedback(

src/lib/routes/admin-api/user-splash-controller.ts

@@ -6,6 +6,7 @@ import { IUnleashConfig } from '../../types/option';
 import { IUnleashServices } from '../../types/services';
 import UserSplashService from '../../services/user-splash-service';
 import { IAuthRequest } from '../unleash-types';
+import { NONE } from '../../types/permissions';
 
 interface ISplashBody {
     seen: boolean;
@@ -25,7 +26,7 @@ class UserSplashController extends Controller {
         this.logger = config.getLogger('splash-controller.ts');
         this.userSplashService = userSplashService;
 
-        this.post('/:id', this.updateSplashSettings);
+        this.post('/:id', this.updateSplashSettings, NONE);
     }
 
     private async updateSplashSettings(

src/lib/routes/admin-api/user.ts

@@ -8,6 +8,7 @@ import UserService from '../../services/user-service';
 import SessionService from '../../services/session-service';
 import UserFeedbackService from '../../services/user-feedback-service';
 import UserSplashService from '../../services/user-splash-service';
+import { NONE } from '../../types/permissions';
 
 interface IChangeUserRequest {
     password: string;
@@ -50,7 +51,7 @@ class UserController extends Controller {
         this.userSplashService = userSplashService;
 
         this.get('/', this.getUser);
-        this.post('/change-password', this.updateUserPass);
+        this.post('/change-password', this.updateUserPass, NONE);
         this.get('/my-sessions', this.mySessions);
     }
 

src/lib/routes/auth/reset-password-controller.ts

@@ -4,6 +4,7 @@ import UserService from '../../services/user-service';
 import { Logger } from '../../logger';
 import { IUnleashConfig } from '../../types/option';
 import { IUnleashServices } from '../../types/services';
+import { NONE } from '../../types/permissions';
 
 interface IValidateQuery {
     token: string;
@@ -31,9 +32,9 @@ class ResetPasswordController extends Controller {
         );
         this.userService = userService;
         this.get('/validate', this.validateToken);
-        this.post('/password', this.changePassword);
+        this.post('/password', this.changePassword, NONE);
-        this.post('/validate-password', this.validatePassword);
+        this.post('/validate-password', this.validatePassword, NONE);
-        this.post('/password-email', this.sendResetPasswordEmail);
+        this.post('/password-email', this.sendResetPasswordEmail, NONE);
     }
 
     async sendResetPasswordEmail(req: Request, res: Response): Promise<void> {

src/lib/routes/client-api/metrics.ts

@@ -10,6 +10,7 @@ import { ALL } from '../../types/models/api-token';
 import ClientMetricsServiceV2 from '../../services/client-metrics/client-metrics-service-v2';
 import { User } from '../../server-impl';
 import { IClientApp } from '../../types/model';
+import { NONE } from '../../types/permissions';
 
 export default class ClientMetricsController extends Controller {
     logger: Logger;
@@ -35,7 +36,7 @@ export default class ClientMetricsController extends Controller {
         this.metrics = clientMetricsService;
         this.metricsV2 = clientMetricsServiceV2;
 
-        this.post('/', this.registerMetrics);
+        this.post('/', this.registerMetrics, NONE);
     }
 
     private resolveEnvironment(user: User, data: IClientApp) {

src/lib/routes/client-api/register.ts

@@ -8,6 +8,7 @@ import { IAuthRequest, User } from '../../server-impl';
 import { IClientApp } from '../../types/model';
 import ApiUser from '../../types/api-user';
 import { ALL } from '../../types/models/api-token';
+import { NONE } from '../../types/permissions';
 
 export default class RegisterController extends Controller {
     logger: Logger;
@@ -23,7 +24,9 @@ export default class RegisterController extends Controller {
         super(config);
         this.logger = config.getLogger('/api/client/register');
         this.metrics = clientMetricsService;
-        this.post('/', this.handleRegister);
+
+        // NONE permission is not optimal here in terms of readability.
+        this.post('/', this.handleRegister, NONE);
     }
 
     private resolveEnvironment(user: User, data: IClientApp) {

src/lib/routes/controller.ts

@@ -1,11 +1,10 @@
-import { IRouter, Request, Response } from 'express';
+import { IRouter, Router, Request, Response } from 'express';
 import { Logger } from 'lib/logger';
 import { IUnleashConfig } from '../types/option';
+import { NONE } from '../types/permissions';
 import { handleErrors } from './util';
-
+import NoAccessError from '../error/no-access-error';
-const { Router } = require('express');
+import requireContentType from '../middleware/content_type_checker';
-const NoAccessError = require('../error/no-access-error');
-const requireContentType = require('../middleware/content_type_checker');
 
 interface IRequestHandler<
     P = any,
@@ -20,7 +19,7 @@ interface IRequestHandler<
 }
 
 const checkPermission = (permission) => async (req, res, next) => {
-    if (!permission) {
+    if (!permission || permission === NONE) {
         return next();
     }
     if (req.checkRbac && (await req.checkRbac(permission))) {
@@ -73,7 +72,7 @@ export default class Controller {
     post(
         path: string,
         handler: IRequestHandler,
-        permission?: string,
+        permission: string,
         ...acceptedContentTypes: string[]
     ): void {
         this.app.post(
@@ -87,7 +86,7 @@ export default class Controller {
     put(
         path: string,
         handler: IRequestHandler,
-        permission?: string,
+        permission: string,
         ...acceptedContentTypes: string[]
     ): void {
         this.app.put(
@@ -101,7 +100,7 @@ export default class Controller {
     patch(
         path: string,
         handler: IRequestHandler,
-        permission?: string,
+        permission: string,
         ...acceptedContentTypes: string[]
     ): void {
         this.app.patch(
@@ -112,7 +111,7 @@ export default class Controller {
         );
     }
 
-    delete(path: string, handler: IRequestHandler, permission?: string): void {
+    delete(path: string, handler: IRequestHandler, permission: string): void {
         this.app.delete(
             path,
             checkPermission(permission),
@@ -124,7 +123,7 @@ export default class Controller {
         path: string,
         filehandler: IRequestHandler,
         handler: Function,
-        permission?: string,
+        permission: string,
     ): void {
         this.app.post(
             path,

src/lib/types/permissions.ts

@@ -1,5 +1,8 @@
+// Special
 export const ADMIN = 'ADMIN';
 export const CLIENT = 'CLIENT';
+export const NONE = 'NONE';
+
 export const CREATE_FEATURE = 'CREATE_FEATURE';
 export const UPDATE_FEATURE = 'UPDATE_FEATURE';
 export const DELETE_FEATURE = 'DELETE_FEATURE';

src/test/e2e/api/admin/project/features.auth.e2e.test.ts

@@ -0,0 +1,78 @@
+import dbInit, { ITestDb } from '../../../helpers/database-init';
+import { IUnleashTest, setupAppWithAuth } from '../../../helpers/test-helper';
+import getLogger from '../../../../fixtures/no-logger';
+import { DEFAULT_ENV } from '../../../../../lib/util/constants';
+import { RoleName } from '../../../../../lib/server-impl';
+
+let app: IUnleashTest;
+let db: ITestDb;
+
+beforeAll(async () => {
+    db = await dbInit('feature_strategy_auth_api_serial', getLogger);
+    app = await setupAppWithAuth(db.stores);
+});
+
+afterEach(async () => {
+    const all = await db.stores.projectStore.getEnvironmentsForProject(
+        'default',
+    );
+    await Promise.all(
+        all
+            .filter((env) => env !== DEFAULT_ENV)
+            .map(async (env) =>
+                db.stores.projectStore.deleteEnvironmentForProject(
+                    'default',
+                    env,
+                ),
+            ),
+    );
+});
+
+afterAll(async () => {
+    await app.destroy();
+    await db.destroy();
+});
+
+test('Should not be possible to update feature toggle without permission', async () => {
+    const email = 'user@mail.com';
+    const url = '/api/admin/projects/default/features';
+    const name = 'auth.toggle.update';
+
+    await db.stores.featureToggleStore.create('default', { name });
+
+    await app.services.userService.createUser({
+        email,
+        rootRole: RoleName.VIEWER,
+    });
+
+    await app.request.post('/auth/demo/login').send({
+        email,
+    });
+
+    await app.request
+        .put(`${url}/${name}`)
+        .send({ name, description: 'updated', type: 'kill-switch' })
+        .expect(403);
+});
+
+test('Should be possible to update feature toggle with permission', async () => {
+    const email = 'user2@mail.com';
+    const url = '/api/admin/projects/default/features';
+    const name = 'auth.toggle.update2';
+
+    await db.stores.featureToggleStore.create('default', { name });
+
+    await app.services.userService.createUser({
+        email,
+        rootRole: RoleName.EDITOR,
+    });
+
+    await app.request.post('/auth/demo/login').send({
+        email,
+    });
+
+    await app.request
+        .put(`${url}/${name}`)
+        .send({ name, description: 'updated', type: 'kill-switch' })
+        .expect(200);
+});