refactor: split NoAccessError into ForbiddenError + PermissionError (#4190)

In some of the places we used `NoAccessError` for permissions, other
places we used it for a more generic 403 error with a different
message. This refactoring splits the error type into two distinct
types instead to make the error messages more consistent.

src/lib/error/forbidden-error.ts

@@ -0,0 +1,6 @@
+import { UnleashError } from './unleash-error';
+
+class ForbiddenError extends UnleashError {}
+
+export default ForbiddenError;
+module.exports = ForbiddenError;

src/lib/error/index.ts

@@ -7,13 +7,14 @@ import InvalidOperationError from './invalid-operation-error';
 import InvalidTokenError from './invalid-token-error';
 import MinimumOneEnvironmentError from './minimum-one-environment-error';
 import NameExistsError from './name-exists-error';
-import NoAccessError from './no-access-error';
+import PermissionError from './permission-error';
 import { OperationDeniedError } from './operation-denied-error';
 import UserTokenError from './used-token-error';
 import RoleInUseError from './role-in-use-error';
 import ProjectWithoutOwnerError from './project-without-owner-error';
 import PasswordUndefinedError from './password-undefined';
 import PasswordMismatchError from './password-mismatch';
+import ForbiddenError from './forbidden-error';
 
 export {
     BadDataError,
@@ -26,7 +27,8 @@ export {
     InvalidTokenError,
     MinimumOneEnvironmentError,
     NameExistsError,
-    NoAccessError,
+    PermissionError,
+    ForbiddenError,
     OperationDeniedError,
     UserTokenError,
     RoleInUseError,

src/lib/error/permission-error.ts

@@ -2,7 +2,7 @@ import { ApiErrorSchema, UnleashError } from './unleash-error';
 
 type Permission = string | string[];
 
-class NoAccessError extends UnleashError {
+class PermissionError extends UnleashError {
     permissions: Permission;
 
     constructor(permission: Permission = [], environment?: string) {
@@ -12,11 +12,13 @@ class NoAccessError extends UnleashError {
 
         const permissionsMessage =
             permissions.length === 1
-                ? `the ${permissions[0]} permission`
-                : `any of the following permissions: ${permissions.join(', ')}`;
+                ? `the "${permissions[0]}" permission`
+                : `all of the following permissions: ${permissions
+                      .map((perm) => `"${perm}"`)
+                      .join(', ')}`;
 
         const message =
-            `You don't have the required permissions to perform this operation. You need ${permissionsMessage}" to perform this action` +
+            `You don't have the required permissions to perform this operation. To perform this action, you need ${permissionsMessage}` +
             (environment ? ` in the "${environment}" environment.` : `.`);
 
         super(message);
@@ -32,5 +34,5 @@ class NoAccessError extends UnleashError {
     }
 }
 
-export default NoAccessError;
-module.exports = NoAccessError;
+export default PermissionError;
+module.exports = PermissionError;

src/lib/error/unleash-error.test.ts

@@ -6,7 +6,7 @@ import BadDataError, {
     fromOpenApiValidationError,
     fromOpenApiValidationErrors,
 } from './bad-data-error';
-import NoAccessError from './no-access-error';
+import PermissionError from './permission-error';
 import OwaspValidationError from './owasp-validation-error';
 import IncompatibleProjectError from './incompatible-project-error';
 import PasswordUndefinedError from './password-undefined';
@@ -462,7 +462,7 @@ describe('Error serialization special cases', () => {
 
     it('NoAccessError: adds `permissions`', () => {
         const permission = 'x';
-        const error = new NoAccessError(permission);
+        const error = new PermissionError(permission);
         const json = error.toJSON();
 
         expect(json.permissions).toStrictEqual([permission]);
@@ -470,7 +470,7 @@ describe('Error serialization special cases', () => {
 
     it('NoAccessError: supports multiple permissions', () => {
         const permission = ['x', 'y', 'z'];
-        const error = new NoAccessError(permission);
+        const error = new PermissionError(permission);
         const json = error.toJSON();
 
         expect(json.permissions).toStrictEqual(permission);

src/lib/error/unleash-error.ts

@@ -24,9 +24,10 @@ export const UnleashApiErrorTypes = [
     'ValidationError',
     'AuthenticationRequired',
     'UnauthorizedError',
-    'NoAccessError',
+    'PermissionError',
     'InvalidTokenError',
     'OwaspValidationError',
+    'ForbiddenError',
 
     // server errors; not the end user's fault
     'InternalError',
@@ -86,6 +87,10 @@ const statusCode = (errorName: string): number => {
             return 403;
         case 'AuthenticationRequired':
             return 401;
+        case 'ForbiddenError':
+            return 403;
+        case 'PermissionError':
+            return 403;
         case 'BadRequestError': //thrown by express; do not remove
             return 400;
         default:

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

@@ -17,7 +17,7 @@ import { FeatureVariantsSchema } from '../../../openapi/spec/feature-variants-sc
 import { createRequestSchema } from '../../../openapi/util/create-request-schema';
 import { createResponseSchema } from '../../../openapi/util/create-response-schema';
 import { AccessService } from '../../../services';
-import { BadDataError, NoAccessError } from '../../../../lib/error';
+import { BadDataError, PermissionError } from '../../../../lib/error';
 import { User } from 'lib/server-impl';
 import { PushVariantsSchema } from 'lib/openapi/spec/push-variants-schema';
 
@@ -309,7 +309,7 @@ export default class VariantsController extends Controller {
                     environment,
                 ))
             ) {
-                throw new NoAccessError(
+                throw new PermissionError(
                     UPDATE_FEATURE_ENVIRONMENT_VARIANTS,
                     environment,
                 );

src/lib/routes/controller.ts

@@ -3,8 +3,8 @@ 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';
 import requireContentType from '../middleware/content_type_checker';
+import { PermissionError } from '../error';
 
 interface IRequestHandler<
     P = any,
@@ -52,7 +52,7 @@ const checkPermission =
         if (req.checkRbac && (await req.checkRbac(permissions))) {
             return next();
         }
-        return res.status(403).json(new NoAccessError(permissions)).end();
+        return res.status(403).json(new PermissionError(permissions)).end();
     };
 
 /**

src/lib/services/feature-toggle-service.ts

@@ -44,7 +44,12 @@ import { Logger } from '../logger';
 import BadDataError from '../error/bad-data-error';
 import NameExistsError from '../error/name-exists-error';
 import InvalidOperationError from '../error/invalid-operation-error';
-import { FOREIGN_KEY_VIOLATION, OperationDeniedError } from '../error';
+import {
+    FOREIGN_KEY_VIOLATION,
+    OperationDeniedError,
+    PermissionError,
+    ForbiddenError,
+} from '../error';
 import {
     constraintSchema,
     featureMetadataSchema,
@@ -79,7 +84,6 @@ import {
 } from '../features/playground/feature-evaluator/helpers';
 import { AccessService } from './access-service';
 import { User } from '../server-impl';
-import NoAccessError from '../error/no-access-error';
 import { IFeatureProjectUserParams } from '../routes/admin-api/project/project-features';
 import { unique } from '../util/unique';
 import { ISegmentService } from 'lib/segments/segment-service-interface';
@@ -976,7 +980,7 @@ class FeatureToggleService {
                 projectId,
             );
         if (changeRequestEnabled) {
-            throw new NoAccessError(
+            throw new ForbiddenError(
                 `Cloning not allowed. Project ${projectId} has change requests enabled.`,
             );
         }
@@ -1517,7 +1521,7 @@ class FeatureToggleService {
                 newProject,
             );
         if (changeRequestEnabled) {
-            throw new NoAccessError(
+            throw new ForbiddenError(
                 `Changing project not allowed. Project ${newProject} has change requests enabled.`,
             );
         }
@@ -1918,7 +1922,7 @@ class FeatureToggleService {
                 user,
             );
         if (!canBypass) {
-            throw new NoAccessError(SKIP_CHANGE_REQUEST);
+            throw new PermissionError(SKIP_CHANGE_REQUEST);
         }
     }
 
@@ -1950,7 +1954,7 @@ class FeatureToggleService {
                         environment,
                     ));
                 if (!canAddStrategies) {
-                    throw new NoAccessError(CREATE_FEATURE_STRATEGY);
+                    throw new PermissionError(CREATE_FEATURE_STRATEGY);
                 }
             }
         }

src/lib/services/project-service.ts

@@ -47,7 +47,6 @@ import {
     IRoleDescriptor,
 } from '../types/stores/access-store';
 import FeatureToggleService from './feature-toggle-service';
-import NoAccessError from '../error/no-access-error';
 import IncompatibleProjectError from '../error/incompatible-project-error';
 import { IFeatureTagStore } from 'lib/types/stores/feature-tag-store';
 import ProjectWithoutOwnerError from '../error/project-without-owner-error';
@@ -58,6 +57,7 @@ import { FavoritesService } from './favorites-service';
 import { calculateAverageTimeToProd } from '../features/feature-toggle/time-to-production/time-to-production';
 import { IProjectStatsStore } from 'lib/types/stores/project-stats-store-type';
 import { uniqueByKey } from '../util/unique';
+import { PermissionError } from '../error';
 
 const getCreatedBy = (user: IUser) => user.email || user.username || 'unknown';
 
@@ -259,7 +259,7 @@ export default class ProjectService {
         const feature = await this.featureToggleStore.get(featureName);
 
         if (feature.project !== currentProjectId) {
-            throw new NoAccessError(MOVE_FEATURE_TOGGLE);
+            throw new PermissionError(MOVE_FEATURE_TOGGLE);
         }
         const project = await this.getProject(newProjectId);
 
@@ -274,7 +274,7 @@ export default class ProjectService {
         );
 
         if (!authorized) {
-            throw new NoAccessError(MOVE_FEATURE_TOGGLE);
+            throw new PermissionError(MOVE_FEATURE_TOGGLE);
         }
 
         const isCompatibleWithTargetProject =

src/test/e2e/services/access-service.e2e.test.ts

@@ -776,7 +776,7 @@ test('Should be denied move feature toggle to project where the user does not ha
             projectOrigin.id,
         );
     } catch (e) {
-        expect(e.name).toContain('NoAccess');
+        expect(e.name).toContain('Permission');
         expect(e.message.includes('permission')).toBeTruthy();
         expect(
             e.message.includes(permissions.MOVE_FEATURE_TOGGLE),

src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts

@@ -11,7 +11,7 @@ import { FeatureStrategySchema } from '../../../lib/openapi';
 import User from '../../../lib/types/user';
 import { IConstraint, IVariant, SKIP_CHANGE_REQUEST } from '../../../lib/types';
 import EnvironmentService from '../../../lib/services/environment-service';
-import { NoAccessError } from '../../../lib/error';
+import { ForbiddenError, PermissionError } from '../../../lib/error';
 import { ISegmentService } from '../../../lib/segments/segment-service-interface';
 import { ChangeRequestAccessReadModel } from '../../../lib/features/change-request-access-service/sql-change-request-access-read-model';
 
@@ -350,7 +350,7 @@ test('cloning a feature toggle not allowed for change requests enabled', async (
             'test-user',
         ),
     ).rejects.toEqual(
-        new NoAccessError(
+        new ForbiddenError(
             `Cloning not allowed. Project default has change requests enabled.`,
         ),
     );
@@ -364,7 +364,7 @@ test('changing to a project with change requests enabled should not be allowed',
     await expect(
         service.changeProject('newToggleName', 'default', 'user'),
     ).rejects.toEqual(
-        new NoAccessError(
+        new ForbiddenError(
             `Changing project not allowed. Project default has change requests enabled.`,
         ),
     );
@@ -484,7 +484,7 @@ test('If change requests are enabled, cannot change variants without going via C
             },
             [],
         ),
-    ).rejects.toThrowError(new NoAccessError(SKIP_CHANGE_REQUEST));
+    ).rejects.toThrowError(new PermissionError(SKIP_CHANGE_REQUEST));
 });
 
 test('If CRs are protected for any environment in the project stops bulk update of variants', async () => {
@@ -590,7 +590,7 @@ test('If CRs are protected for any environment in the project stops bulk update
                 isAPI: true,
             },
         ),
-    ).rejects.toThrowError(new NoAccessError(SKIP_CHANGE_REQUEST));
+    ).rejects.toThrowError(new PermissionError(SKIP_CHANGE_REQUEST));
 });
 
 test('getPlaygroundFeatures should return ids and titles (if they exist) on client strategies', async () => {