From d515f5843ebd0c5d190bd1f61bc3e9bbb34b61c1 Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Wed, 11 Nov 2026 11:21:29 -0500 Subject: [PATCH 1/3] Enforce deletion permissions and state rules across single and batch APIs (#3326) Single and batch deletion of experiments, feature flags and segments now share one backend policy instead of relying on the UI: - Role: PermissionService checks the shared ROLE_PERMISSIONS table (moved from the frontend into upgrade_types; the frontend now reads it too). Disallowed roles get 403 before any target is locked. - State/usage: DeletionPolicyService evaluates fields read under the target's deletion lock, in the deletion transaction. Experiments must be Draft, Inactive, Completed/Cancelled or Archived; Enabled flags, Used segments and global-exclude segments are refused. - Single routes return 409 (DELETION_NOT_ALLOWED) on refusal. Batch items are reported with a new 'refused' outcome and policy reason codes (state_not_deletable, segment_in_use, global_exclude_protected), and the batch continues. Refused items write no audit log and trigger no cache invalidation or recompute. - DELETE /segments/:segmentId now receives @CurrentUser(). - DeletionRepository.isSegmentInUse() evaluates the Used status for one segment; an integration test pins it to getSegmentStatus(). - Frontend accepts 'refused' and reports skipped items. - Batch deletion integration tests inverted to assert role and state refusals, including targets that become ineligible before the lock. Load tests (upgrade-automated-tests) still need an authorized account and deletable test entities before cleanup. Co-Authored-By: Claude Opus 5.5 --- packages/backend/CLAUDE.md | 9 + .../api/controllers/ExperimentController.ts | 18 +- .../api/controllers/FeatureFlagController.ts | 20 +- .../src/api/controllers/SegmentController.ts | 27 +- .../api/middlewares/ErrorHandlerMiddleware.ts | 4 + .../api/repositories/DeletionRepository.ts | 67 +++- .../src/api/services/BatchDeleteService.ts | 13 +- .../src/api/services/DeletionPolicyService.ts | 98 +++++ .../src/api/services/PermissionService.ts | 15 + .../src/api/services/SegmentService.ts | 9 +- .../test/integration/BatchDelete/index.ts | 367 +++++++++++------- .../controllers/ExperimentController.test.ts | 5 + ...ExperimentControllerAdaptiveConfig.test.ts | 1 + .../controllers/FeatureFlagController.test.ts | 5 + .../controllers/SegmentController.test.ts | 5 + .../unit/services/BatchDeleteService.test.ts | 81 +++- .../services/DeletionPolicyService.test.ts | 122 ++++++ .../upgrade/src/app/core/auth/auth.service.ts | 55 +-- .../src/app/core/auth/store/auth.models.ts | 19 +- .../batch-actions/batch-actions.helpers.ts | 21 +- .../batch-actions.integration.spec.ts | 21 + .../projects/upgrade/src/assets/i18n/en.json | 2 + packages/types/src/BatchActions/index.ts | 20 +- packages/types/src/Experiment/enums.ts | 1 + packages/types/src/Permissions/index.ts | 76 ++++ packages/types/src/index.ts | 9 + 26 files changed, 846 insertions(+), 244 deletions(-) create mode 100644 packages/backend/src/api/services/DeletionPolicyService.ts create mode 100644 packages/backend/src/api/services/PermissionService.ts create mode 100644 packages/backend/test/unit/services/DeletionPolicyService.test.ts create mode 100644 packages/types/src/Permissions/index.ts diff --git a/packages/backend/CLAUDE.md b/packages/backend/CLAUDE.md index 305ee2c201..ef986b4d88 100644 --- a/packages/backend/CLAUDE.md +++ b/packages/backend/CLAUDE.md @@ -63,6 +63,15 @@ npm run migration:run - `@Authorized()` decorator (from routing-controllers) enforces auth on controllers - `@CurrentUser()` injects the authenticated `User` object into handler params +## Deletion Policy + +`DeletionPolicyService` is the one role/state/usage policy for the user-facing deletion APIs (`DELETE /experiments/:id`, `/flags/:id`, `/segments/:segmentId` and the three `POST /…/batch-delete` routes). Deletable experiment states (`DELETABLE_EXPERIMENT_STATES`) live in `upgrade_types` so the frontend checks the same list. + +- The role is checked per request (`assertRoleCanDelete` → `PermissionService.assertPermitted(user, resource, 'delete')` → 403) before any target is locked. Role permissions for every resource and action live in the shared `ROLE_PERMISSIONS` table in `upgrade_types`, which the frontend also uses; enforce other actions through `PermissionService` rather than per-action role lists. State and usage rules stay per action (as `DeletionPolicyService` does for deletion). +- State and usage are checked by `getRefusalReason()` on fields read by `DeletionRepository.findForDeletion()` under the target's `pessimistic_write` lock, in the deletion transaction, never from an earlier unlocked read. Segment usage (`isSegmentInUse`) is evaluated for that one segment and mirrors the Used status from `SegmentService.getSegmentStatus()`. +- Single routes pass `singleDeletionTransaction()` to the entity service and return 409 (`SERVER_ERROR.DELETION_NOT_ALLOWED`) on refusal. Batch items are reported `refused` with a policy `DeletionReasonCode`, and the batch continues. +- Internal deletions (create rollback, private-list removal via `deleteList`) call the entity services without the policy executor, deliberately. + ## Environment Copy `.env.example` to `.env` for local dev. Never commit `.env` files. Key sections: `app`, `db`, `google`, `swagger`, `aws`. See `src/env.ts` for the full typed schema. diff --git a/packages/backend/src/api/controllers/ExperimentController.ts b/packages/backend/src/api/controllers/ExperimentController.ts index bd6446f554..6a3f66e93c 100644 --- a/packages/backend/src/api/controllers/ExperimentController.ts +++ b/packages/backend/src/api/controllers/ExperimentController.ts @@ -1,6 +1,7 @@ import { BatchDeleteResult } from 'upgrade_types'; import { Inject } from 'typedi'; import { BatchDeleteService } from '../services/BatchDeleteService'; +import { DeletionPolicyService } from '../services/DeletionPolicyService'; import { BatchEntityIdsValidator } from './validators/BatchEntityIdsValidator'; import { Body, @@ -669,7 +670,8 @@ export class ExperimentController { public cacheService: CacheService, public thompsonSamplingCrudService: ThompsonSamplingExperimentCrudService, public adaptiveExperimentConfigDispatcher: AdaptiveExperimentConfigDispatcherService, - @Inject(() => BatchDeleteService) private batchDeleteService: BatchDeleteService + @Inject(() => BatchDeleteService) private batchDeleteService: BatchDeleteService, + @Inject(() => DeletionPolicyService) private deletionPolicyService: DeletionPolicyService ) {} /** @@ -677,7 +679,7 @@ export class ExperimentController { * /experiments/batch-delete: * post: * summary: Delete the selected experiments - * description: Deletes selected experiments sequentially regardless of state, skipping missing items. Execution failures stop the remaining items. Returns one result per ID. + * description: Deletes selected experiments sequentially. Requires the Admin or Creator role. Missing items are skipped; experiments that are not Draft, Inactive, Completed or Archived are refused and skipped. Execution failures stop the remaining items. Returns one result per ID. * tags: * - Experiments * parameters: @@ -695,6 +697,8 @@ export class ExperimentController { * description: Expected a nonempty array of unique UUIDs. * '401': * description: AuthorizationRequiredError + * '403': + * description: The current user's role cannot delete experiments. Nothing is deleted. */ @Post('/batch-delete') public batchDelete( @@ -1182,8 +1186,12 @@ export class ExperimentController { * description: ExperimentId should be a valid UUID. * '401': * description: AuthorizationRequiredError + * '403': + * description: The current user's role cannot delete experiments * '404': * description: Not found error + * '409': + * description: The experiment is Running, Paused, Preview or Scheduled and was not deleted * '500': * description: id should be of type UUID */ @@ -1196,7 +1204,11 @@ export class ExperimentController { ): Promise { request.logger.child({ user: currentUser }); - const experiment = await this.experimentService.delete(id, currentUser, { logger: request.logger }); + this.deletionPolicyService.assertRoleCanDelete('experiments', currentUser); + const experiment = await this.experimentService.delete(id, currentUser, { + logger: request.logger, + executeTransaction: this.deletionPolicyService.singleDeletionTransaction('experiments', id), + }); if (!experiment) { throw new NotFoundException('Experiment not found.'); diff --git a/packages/backend/src/api/controllers/FeatureFlagController.ts b/packages/backend/src/api/controllers/FeatureFlagController.ts index 10cd20fd1a..a3bfa1d9f7 100644 --- a/packages/backend/src/api/controllers/FeatureFlagController.ts +++ b/packages/backend/src/api/controllers/FeatureFlagController.ts @@ -1,6 +1,7 @@ import { BatchDeleteResult } from 'upgrade_types'; import { Inject } from 'typedi'; import { BatchDeleteService } from '../services/BatchDeleteService'; +import { DeletionPolicyService } from '../services/DeletionPolicyService'; import { BatchEntityIdsValidator } from './validators/BatchEntityIdsValidator'; import { JsonController, @@ -161,7 +162,8 @@ export class FeatureFlagsController { constructor( public featureFlagService: FeatureFlagService, public experimentUserService: ExperimentUserService, - @Inject(() => BatchDeleteService) private batchDeleteService: BatchDeleteService + @Inject(() => BatchDeleteService) private batchDeleteService: BatchDeleteService, + @Inject(() => DeletionPolicyService) private deletionPolicyService: DeletionPolicyService ) {} /** @@ -169,7 +171,7 @@ export class FeatureFlagsController { * /flags/batch-delete: * post: * summary: Delete the selected flags - * description: Deletes selected feature flags sequentially regardless of status, skipping missing items. Execution failures stop the remaining items. Returns one result per ID. + * description: Deletes selected feature flags sequentially. Requires the Admin or Creator role. Missing items are skipped; Enabled flags are refused and skipped. Execution failures stop the remaining items. Returns one result per ID. * tags: * - Feature Flags * parameters: @@ -187,6 +189,8 @@ export class FeatureFlagsController { * description: Expected a nonempty array of unique UUIDs. * '401': * description: AuthorizationRequiredError + * '403': + * description: The current user's role cannot delete feature flags. Nothing is deleted. */ @Post('/batch-delete') public batchDelete( @@ -464,8 +468,12 @@ export class FeatureFlagsController { * responses: * '200': * description: Delete Feature flag By Id + * '403': + * description: The current user's role cannot delete feature flags * '404': * description: Feature flag not found + * '409': + * description: The feature flag is Enabled and was not deleted */ @Delete('/:id') @@ -474,7 +482,13 @@ export class FeatureFlagsController { @CurrentUser() currentUser: UserDTO, @Req() request: AppRequest ): Promise { - const featureFlag = await this.featureFlagService.delete(id, currentUser, request.logger); + this.deletionPolicyService.assertRoleCanDelete('flags', currentUser); + const featureFlag = await this.featureFlagService.delete( + id, + currentUser, + request.logger, + this.deletionPolicyService.singleDeletionTransaction('flags', id) + ); if (!featureFlag) { throw new NotFoundException('Feature flag not found.'); diff --git a/packages/backend/src/api/controllers/SegmentController.ts b/packages/backend/src/api/controllers/SegmentController.ts index c4949ff15f..0babea13c3 100644 --- a/packages/backend/src/api/controllers/SegmentController.ts +++ b/packages/backend/src/api/controllers/SegmentController.ts @@ -2,6 +2,7 @@ import { UserDTO } from '../DTO/UserDTO'; import { BatchDeleteResult } from 'upgrade_types'; import { Inject } from 'typedi'; import { BatchDeleteService } from '../services/BatchDeleteService'; +import { DeletionPolicyService } from '../services/DeletionPolicyService'; import { BatchEntityIdsValidator } from './validators/BatchEntityIdsValidator'; import { JsonController, @@ -64,10 +65,10 @@ interface SegmentPaginationInfo extends PaginationResponse { * format: uuid * outcome: * type: string - * enum: [deleted, not_found, failed, unknown, not_attempted] + * enum: [deleted, not_found, refused, failed, unknown, not_attempted] * reasonCode: * type: string - * enum: [not_found, delete_failed, lock_timeout, outcome_unknown, post_delete_failed] + * enum: [not_found, delete_failed, lock_timeout, outcome_unknown, post_delete_failed, state_not_deletable, segment_in_use, global_exclude_protected] * BatchDeleteResult: * type: object * required: [results] @@ -273,7 +274,8 @@ interface SegmentPaginationInfo extends PaginationResponse { export class SegmentController { constructor( public segmentService: SegmentService, - @Inject(() => BatchDeleteService) private batchDeleteService: BatchDeleteService + @Inject(() => BatchDeleteService) private batchDeleteService: BatchDeleteService, + @Inject(() => DeletionPolicyService) private deletionPolicyService: DeletionPolicyService ) {} /** @@ -281,7 +283,7 @@ export class SegmentController { * /segments/batch-delete: * post: * summary: Delete the selected segments - * description: Deletes selected segments sequentially using the existing deletion workflow, skipping missing items. Execution failures stop the remaining items. Returns one result per ID. + * description: Deletes selected segments sequentially using the existing deletion workflow. Requires the Admin, Creator or User Manager role. Missing items are skipped; Used and global-exclude segments are refused and skipped. Execution failures stop the remaining items. Returns one result per ID. * tags: * - Segment * parameters: @@ -299,6 +301,8 @@ export class SegmentController { * description: Expected a nonempty array of unique UUIDs. * '401': * description: AuthorizationRequiredError + * '403': + * description: The current user's role cannot delete segments. Nothing is deleted. */ @Post('/batch-delete') public batchDelete( @@ -664,15 +668,26 @@ export class SegmentController { * $ref: '#/definitions/segmentResponse' * '401': * description: Authorization Required Error + * '403': + * description: The current user's role cannot delete segments + * '409': + * description: The segment is Used or is a global-exclude segment, and was not deleted * '500': * description: Internal Server Error, SegmentId is not valid */ @Delete('/:segmentId') - public deleteSegment( + public async deleteSegment( @Params({ validate: true }) { segmentId }: SegmentIdValidator, + @CurrentUser() currentUser: UserDTO, @Req() request: AppRequest ): Promise { - return this.segmentService.deleteSegment(segmentId, request.logger); + this.deletionPolicyService.assertRoleCanDelete('segments', currentUser); + return this.segmentService.deleteSegment( + segmentId, + request.logger, + this.deletionPolicyService.singleDeletionTransaction('segments', segmentId), + false + ); } /** diff --git a/packages/backend/src/api/middlewares/ErrorHandlerMiddleware.ts b/packages/backend/src/api/middlewares/ErrorHandlerMiddleware.ts index c1668fc078..38f9ab67a2 100644 --- a/packages/backend/src/api/middlewares/ErrorHandlerMiddleware.ts +++ b/packages/backend/src/api/middlewares/ErrorHandlerMiddleware.ts @@ -79,6 +79,10 @@ export class ErrorHandlerMiddleware implements ExpressErrorMiddlewareInterface { type = SERVER_ERROR.EMAIL_SEND_ERROR; message = errorMessage; break; + case SERVER_ERROR.DELETION_NOT_ALLOWED: + type = SERVER_ERROR.DELETION_NOT_ALLOWED; + message = errorMessage; + break; default: switch (error.httpCode) { case 400: diff --git a/packages/backend/src/api/repositories/DeletionRepository.ts b/packages/backend/src/api/repositories/DeletionRepository.ts index 6f6ca8d5a0..92c31d822f 100644 --- a/packages/backend/src/api/repositories/DeletionRepository.ts +++ b/packages/backend/src/api/repositories/DeletionRepository.ts @@ -1,11 +1,31 @@ import { EntityManager, Repository } from 'typeorm'; -import { BatchDeleteEntity } from 'upgrade_types'; +import { + BatchDeleteEntity, + EXPERIMENT_STATE, + FEATURE_FLAG_STATUS, + SEGMENT_TYPE, + STANDARD_LIST_TYPE, +} from 'upgrade_types'; import { EntityRepository } from '../../typeorm-typedi-extensions'; import { Experiment } from '../models/Experiment'; import { FeatureFlag } from '../models/FeatureFlag'; import { Segment } from '../models/Segment'; import repositoryError from './utils/repositoryError'; +/** The locked fields the deletion policy evaluates. Only the fields for the target's entity type are set. */ +export interface DeletionTarget { + id: string; + state?: EXPERIMENT_STATE; + status?: FEATURE_FLAG_STATUS; + type?: SEGMENT_TYPE; +} + +const policyFields = { + experiments: { id: true, state: true }, + flags: { id: true, status: true }, + segments: { id: true, type: true }, +}; + /** All reads and locks use the caller's deletion transaction, never the repository's default manager. */ @EntityRepository() export class DeletionRepository extends Repository { @@ -13,13 +33,52 @@ export class DeletionRepository extends Repository { entity: BatchDeleteEntity, id: string, manager: EntityManager - ): Promise<{ id: string } | null> { + ): Promise { const target = { experiments: Experiment, flags: FeatureFlag, segments: Segment }[entity]; return manager - .getRepository<{ id: string }>(target) - .findOne({ where: { id }, select: { id: true }, lock: { mode: 'pessimistic_write' } }) + .getRepository(target) + .findOne({ where: { id }, select: policyFields[entity], lock: { mode: 'pessimistic_write' } }) .catch((errorMsg: any) => { throw repositoryError('DeletionRepository', 'findForDeletion', { entity, id }, errorMsg); }); } + + /** + * Whether a segment has the Used status that `SegmentService.getSegmentStatus()` reports, evaluated for one + * segment instead of every segment. A segment is Used when it, or any segment that contains it, is a list of a + * non-archived experiment or of any feature flag, or when it is a member of a segment-type list that belongs to + * another segment. + */ + public async isSegmentInUse(segmentId: string, manager: EntityManager): Promise { + const rows: { used: boolean }[] = await manager + .query( + `WITH RECURSIVE containing ("id") AS ( + SELECT $1::uuid + UNION + SELECT sfs."parentSegmentId" FROM "segment_for_segment" sfs JOIN containing c ON sfs."childSegmentId" = c."id" + ) + SELECT ( + EXISTS ( + SELECT 1 FROM "experiment_segment_inclusion" esi JOIN "experiment" e ON e."id" = esi."experimentId" + WHERE esi."segmentId" IN (SELECT "id" FROM containing) AND e."state" <> $2 + ) + OR EXISTS ( + SELECT 1 FROM "experiment_segment_exclusion" ese JOIN "experiment" e ON e."id" = ese."experimentId" + WHERE ese."segmentId" IN (SELECT "id" FROM containing) AND e."state" <> $2 + ) + OR EXISTS (SELECT 1 FROM "feature_flag_segment_inclusion" WHERE "segmentId" IN (SELECT "id" FROM containing)) + OR EXISTS (SELECT 1 FROM "feature_flag_segment_exclusion" WHERE "segmentId" IN (SELECT "id" FROM containing)) + OR EXISTS ( + SELECT 1 FROM "segment_for_segment" m JOIN "segment" l ON l."id" = m."parentSegmentId" + WHERE m."childSegmentId" = $1 AND l."listType" = $3 + AND EXISTS (SELECT 1 FROM "segment_for_segment" o WHERE o."childSegmentId" = l."id") + ) + ) AS "used"`, + [segmentId, EXPERIMENT_STATE.ARCHIVED, STANDARD_LIST_TYPE.SEGMENT] + ) + .catch((errorMsg: any) => { + throw repositoryError('DeletionRepository', 'isSegmentInUse', { segmentId }, errorMsg); + }); + return rows[0]?.used === true; + } } diff --git a/packages/backend/src/api/services/BatchDeleteService.ts b/packages/backend/src/api/services/BatchDeleteService.ts index c029e410e0..55c906d6a1 100644 --- a/packages/backend/src/api/services/BatchDeleteService.ts +++ b/packages/backend/src/api/services/BatchDeleteService.ts @@ -8,6 +8,7 @@ import { UserDTO } from '../DTO/UserDTO'; import { ExperimentService } from './ExperimentService'; import { FeatureFlagService } from './FeatureFlagService'; import { SegmentService } from './SegmentService'; +import { DeletionPolicyService } from './DeletionPolicyService'; import { DeletionRepository } from '../repositories/DeletionRepository'; class BatchDeleteSkippedError extends Error { @@ -23,7 +24,8 @@ export class BatchDeleteService { @Inject(() => ExperimentService) private experiments: ExperimentService, @Inject(() => FeatureFlagService) private flags: FeatureFlagService, @Inject(() => SegmentService) private segments: SegmentService, - @InjectRepository() private deletionRepository: DeletionRepository + @InjectRepository() private deletionRepository: DeletionRepository, + @Inject(() => DeletionPolicyService) private deletionPolicy: DeletionPolicyService ) {} public async delete( @@ -32,12 +34,14 @@ export class BatchDeleteService { user: UserDTO, logger: UpgradeLogger ): Promise { + // The role applies to the whole request: refuse it before locking or reading any target. + this.deletionPolicy.assertRoleCanDelete(entity, user); const results: BatchDeleteItemResult[] = []; for (const id of ids) { const result = await this.deleteOne(entity, id, user, logger); results.push(result); - // An already absent target does not prevent independent items from being deleted. - if (result.outcome === 'not_found') continue; + // An already absent or policy-refused target does not prevent independent items from being deleted. + if (result.outcome === 'not_found' || result.outcome === 'refused') continue; // A committed item with a post-delete failure must not be retried, but still stops this batch. if (result.outcome !== 'deleted' || result.reasonCode) { for (let index = results.length; index < ids.length; index++) { @@ -73,6 +77,9 @@ export class BatchDeleteService { if (!target) { throw new BatchDeleteSkippedError({ id, outcome: 'not_found', reasonCode: DeletionReasonCode.NOT_FOUND }); } + // Evaluate the policy on the state read under this lock, before any mutation or side effect. + const reasonCode = await this.deletionPolicy.getRefusalReason(entity, target, runner.manager); + if (reasonCode) throw new BatchDeleteSkippedError({ id, outcome: 'refused', reasonCode }); mutationStarted = true; response = await work(runner.manager); // Legacy experiment/flag repositories return arrays despite their declared return types. diff --git a/packages/backend/src/api/services/DeletionPolicyService.ts b/packages/backend/src/api/services/DeletionPolicyService.ts new file mode 100644 index 0000000000..3f937d9f34 --- /dev/null +++ b/packages/backend/src/api/services/DeletionPolicyService.ts @@ -0,0 +1,98 @@ +import { HttpError } from 'routing-controllers'; +import { Inject, Service } from 'typedi'; +import { DataSource, EntityManager } from 'typeorm'; +import { + BatchDeleteEntity, + DELETABLE_EXPERIMENT_STATES, + DeletionReasonCode, + FEATURE_FLAG_STATUS, + PermissionResource, + SEGMENT_TYPE, + SERVER_ERROR, +} from 'upgrade_types'; +import { InjectDataSource, InjectRepository } from '../../typeorm-typedi-extensions'; +import { DeletionTransaction } from '../../types/DeletionTransaction'; +import { UserDTO } from '../DTO/UserDTO'; +import { DeletionRepository, DeletionTarget } from '../repositories/DeletionRepository'; +import { PermissionService } from './PermissionService'; + +/** The deletion APIs call feature flags `flags`; the shared permission table calls them `featureFlags`. */ +const PERMISSION_RESOURCE: Record = { + experiments: 'experiments', + flags: 'featureFlags', + segments: 'segments', +}; + +export type DeletionRefusalCode = + | DeletionReasonCode.STATE_NOT_DELETABLE + | DeletionReasonCode.SEGMENT_IN_USE + | DeletionReasonCode.GLOBAL_EXCLUDE_PROTECTED; + +/** A single-item deletion refused by policy. The target is unchanged. */ +export class DeletionRefusedError extends HttpError { + public readonly type = SERVER_ERROR.DELETION_NOT_ALLOWED; + + constructor(public readonly reasonCode: DeletionRefusalCode) { + super(409, `Deletion not allowed: ${reasonCode}`); + Object.setPrototypeOf(this, DeletionRefusedError.prototype); + } +} + +/** + * The one deletion policy for the single and batch deletion APIs. The UI hides or disables Delete using the same + * rules, but the current caller role and the target's state read under its deletion lock are the authority. + * + * Internal deletions (rollback of a failed create, removing a private list) call the entity services directly and + * are not subject to this policy. + */ +@Service() +export class DeletionPolicyService { + constructor( + @InjectDataSource() private dataSource: DataSource, + @InjectRepository() private deletionRepository: DeletionRepository, + @Inject(() => PermissionService) private permissionService: PermissionService + ) {} + + /** Throws 403 when the caller's role cannot delete this entity type. Call before locking or reading any target. */ + public assertRoleCanDelete(entity: BatchDeleteEntity, user: UserDTO | undefined): void { + this.permissionService.assertPermitted(user, PERMISSION_RESOURCE[entity], 'delete'); + } + + /** + * Evaluates the state and usage rules for a target already locked by `DeletionRepository.findForDeletion()`, + * using the same transaction manager. Returns the refusal reason, or undefined when deletion is allowed. + */ + public async getRefusalReason( + entity: BatchDeleteEntity, + target: DeletionTarget, + manager: EntityManager + ): Promise { + if (entity === 'experiments') { + return DELETABLE_EXPERIMENT_STATES.includes(target.state) ? undefined : DeletionReasonCode.STATE_NOT_DELETABLE; + } + if (entity === 'flags') { + return target.status === FEATURE_FLAG_STATUS.ENABLED ? DeletionReasonCode.STATE_NOT_DELETABLE : undefined; + } + if (target.type === SEGMENT_TYPE.GLOBAL_EXCLUDE) return DeletionReasonCode.GLOBAL_EXCLUDE_PROTECTED; + return (await this.deletionRepository.isSegmentInUse(target.id, manager)) + ? DeletionReasonCode.SEGMENT_IN_USE + : undefined; + } + + /** + * A deletion transaction for the single-item DELETE routes: locks the target, refuses it with + * `DeletionRefusedError` before any mutation, then runs the entity service's deletion in the same transaction. + * A missing target is left for the entity service to report, so each route keeps its existing not-found response. + */ + public singleDeletionTransaction(entity: BatchDeleteEntity, id: string): DeletionTransaction { + return (work) => + this.dataSource.transaction(async (manager) => { + const target = await this.deletionRepository.findForDeletion(entity, id, manager); + if (target) { + const reasonCode = await this.getRefusalReason(entity, target, manager); + if (reasonCode) throw new DeletionRefusedError(reasonCode); + } + return work(manager); + }); + } +} diff --git a/packages/backend/src/api/services/PermissionService.ts b/packages/backend/src/api/services/PermissionService.ts new file mode 100644 index 0000000000..0264a2da34 --- /dev/null +++ b/packages/backend/src/api/services/PermissionService.ts @@ -0,0 +1,15 @@ +import { ForbiddenError } from 'routing-controllers'; +import { Service } from 'typedi'; +import { isPermitted, PermissionAction, PermissionResource } from 'upgrade_types'; +import { UserDTO } from '../DTO/UserDTO'; + +/** Enforces the shared `ROLE_PERMISSIONS` table the frontend uses to show or hide actions. */ +@Service() +export class PermissionService { + /** Throws 403 when the caller's role may not perform the action on the resource. */ + public assertPermitted(user: UserDTO | undefined, resource: PermissionResource, action: PermissionAction): void { + if (!isPermitted(user?.role, resource, action)) { + throw new ForbiddenError(`Your role does not allow the ${action} action on ${resource}.`); + } + } +} diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index 715e15f24d..3f828bf60e 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -536,16 +536,21 @@ export class SegmentService { return this.addSegmentDataWithPipeline(segment, logger, transactionalEntityManager, skipScheduleRecompute); } + /** + * `awaitPostCommit` (the batch path) collects owners inside the supplied transaction after the target lock and + * awaits their recomputation after commit; otherwise recomputation runs in the background. + */ public async deleteSegment( id: string, logger: UpgradeLogger, - executeTransaction?: DeletionTransaction + executeTransaction?: DeletionTransaction, + awaitPostCommit = executeTransaction !== undefined ): Promise { logger.info({ message: `Delete segment by id. segmentId: ${id}` }); const transaction: DeletionTransaction = executeTransaction || ((work) => this.dataSource.transaction(work)); - if (executeTransaction) { + if (awaitPostCommit) { let affectedFlagIds: string[]; let affectedExperimentIds: string[]; const deletedSegment = await transaction(async (transactionalEntityManager) => { diff --git a/packages/backend/test/integration/BatchDelete/index.ts b/packages/backend/test/integration/BatchDelete/index.ts index 24b2d1680d..1d84ac55c3 100644 --- a/packages/backend/test/integration/BatchDelete/index.ts +++ b/packages/backend/test/integration/BatchDelete/index.ts @@ -15,7 +15,9 @@ import { FEATURE_FLAG_STATUS, LOG_TYPE, POST_EXPERIMENT_RULE, + SEGMENT_STATUS, SEGMENT_TYPE, + SERVER_ERROR, STANDARD_LIST_TYPE, SYSTEM_USER_EMAIL, UserRole, @@ -32,7 +34,6 @@ import { ExperimentCondition } from '../../../src/api/models/ExperimentCondition import { ExperimentPrecomputedSegment } from '../../../src/api/models/ExperimentPrecomputedSegment'; import { ExperimentSegmentInclusion } from '../../../src/api/models/ExperimentSegmentInclusion'; import { FeatureFlag } from '../../../src/api/models/FeatureFlag'; -import { FeatureFlagPrecomputedSegment } from '../../../src/api/models/FeatureFlagPrecomputedSegment'; import { FeatureFlagSegmentInclusion } from '../../../src/api/models/FeatureFlagSegmentInclusion'; import { IndividualForSegment } from '../../../src/api/models/IndividualForSegment'; import { Segment } from '../../../src/api/models/Segment'; @@ -42,8 +43,7 @@ import { User } from '../../../src/api/models/User'; import { DeletionRepository } from '../../../src/api/repositories/DeletionRepository'; import { CacheService } from '../../../src/api/services/CacheService'; import { ExperimentPrecomputedSegmentService } from '../../../src/api/services/ExperimentPrecomputedSegmentService'; -import { FeatureFlagPrecomputedSegmentService } from '../../../src/api/services/FeatureFlagPrecomputedSegmentService'; -import { SegmentService } from '../../../src/api/services/SegmentService'; +import { SegmentService, SegmentWithStatus } from '../../../src/api/services/SegmentService'; import { BatchDeleteService } from '../../../src/api/services/BatchDeleteService'; import { currentUserChecker } from '../../../src/auth/currentUserChecker'; import { env } from '../../../src/env'; @@ -53,6 +53,16 @@ import { iocLoader } from '../../../src/loaders/iocLoader'; const entities: BatchDeleteEntity[] = ['experiments', 'flags', 'segments']; const model = { experiments: Experiment, flags: FeatureFlag, segments: Segment }; const route = (entity: BatchDeleteEntity) => `/api/${entity}/batch-delete`; +const deletionAudit = { experiments: LOG_TYPE.EXPERIMENT_DELETED, flags: LOG_TYPE.FEATURE_FLAG_DELETED }; +const storedStates = [ + ...new Set(Object.values(EXPERIMENT_STATE).map((state) => EXPERIMENT_STATE_INTERNAL_NAME_OVERRIDES[state] || state)), +]; +const deletableStates = [ + EXPERIMENT_STATE.DRAFT, + EXPERIMENT_STATE.INACTIVE, + EXPERIMENT_STATE.CANCELLED, + EXPERIMENT_STATE.ARCHIVED, +]; export function registerBatchDeleteTests(connections: () => [DataSource, DataSource]) { describe('Batch deletion', () => { @@ -173,30 +183,48 @@ export function registerBatchDeleteTests(connections: () => [DataSource, DataSou .expect(401); expect(spy).not.toHaveBeenCalled(); + // With authentication off and no system user there is no caller role, so nothing may be deleted. env.google.authTokenRequired = false; await db.getRepository(User).delete({ email: SYSTEM_USER_EMAIL }); const [single, batch] = await create(entity, 2); - await request(app).delete(`/api/${entity}/${single.id}`).expect(200); - const { body } = await request(app) + await request(app).delete(`/api/${entity}/${single.id}`).expect(403); + await request(app) .post(route(entity)) .send({ ids: [batch.id] }) - .expect(200); - expect(body.results).toEqual([{ id: batch.id, outcome: 'deleted' }]); - expect(await db.getRepository(model[entity]).countBy({ id: In([single.id, batch.id]) })).toBe(0); + .expect(403); + expect(await db.getRepository(model[entity]).countBy({ id: In([single.id, batch.id]) })).toBe(2); }); - test.each(Object.values(UserRole))('matches single-delete behavior for the %s role', async (role) => { + const permitted: Record = { + [UserRole.ADMIN]: ['experiments', 'flags', 'segments'], + [UserRole.CREATOR]: ['experiments', 'flags', 'segments'], + [UserRole.USER_MANAGER]: ['segments'], + [UserRole.READER]: [], + }; + test.each(Object.values(UserRole))('applies the %s role to single and batch deletion', async (role) => { await db.getRepository(User).update(SYSTEM_USER_EMAIL, { role }); for (const entity of entities) { const [single, batch] = await create(entity, 2); - await request(app).delete(`/api/${entity}/${single.id}`).expect(200); - const { body } = await request(app) - .post(route(entity)) - .send({ ids: [batch.id] }) - .expect(200); - expect(body.results).toEqual([{ id: batch.id, outcome: 'deleted' }]); - expect(await db.getRepository(model[entity]).countBy({ id: In([single.id, batch.id]) })).toBe(0); + if (permitted[role].includes(entity)) { + await request(app).delete(`/api/${entity}/${single.id}`).expect(200); + const { body } = await request(app) + .post(route(entity)) + .send({ ids: [batch.id] }) + .expect(200); + expect(body.results).toEqual([{ id: batch.id, outcome: 'deleted' }]); + expect(await db.getRepository(model[entity]).countBy({ id: In([single.id, batch.id]) })).toBe(0); + } else { + await request(app).delete(`/api/${entity}/${single.id}`).expect(403); + await request(app) + .post(route(entity)) + .send({ ids: [batch.id] }) + .expect(403); + expect(await db.getRepository(model[entity]).countBy({ id: In([single.id, batch.id]) })).toBe(2); + } } + expect(await db.getRepository(ExperimentAuditLog).countBy({ type: In(Object.values(deletionAudit)) })).toBe( + permitted[role].filter((entity) => entity !== 'segments').length * 2 + ); }); test.each(entities)('%s deletes owned lists and members while preserving public children', async (entity) => { @@ -217,25 +245,16 @@ export function registerBatchDeleteTests(connections: () => [DataSource, DataSou test.each(entities)('%s deletes with one pooled connection', async (entity) => { const [row] = await create(entity); const list = await ownedList(entity, row.id); - let flagId: string; let experimentId: string; if (entity === 'segments') { - const [flag] = await create('flags'); + // A list of an archived experiment does not make a segment Used, but its precomputed row still updates. const [experiment] = await create('experiments'); - flagId = flag.id; experimentId = experiment.id; - const flagList = await ownedList('flags', flagId, row.id); - await db.getRepository(FeatureFlagSegmentInclusion).update({ segmentId: flagList.id }, { enabled: true }); + await db.getRepository(Experiment).update(experimentId, { state: EXPERIMENT_STATE.ARCHIVED }); await ownedList('experiments', experimentId, row.id); await db.getRepository(IndividualForSegment).insert({ segmentId: row.id, userId: 'deleted-member' }); const logger = new UpgradeLogger(); - await Container.get(FeatureFlagPrecomputedSegmentService).recomputeForFlag(flagId, logger); await Container.get(ExperimentPrecomputedSegmentService).recomputeForExperiment(experimentId, logger); - expect( - await db.getRepository(FeatureFlagPrecomputedSegment).findOneBy({ featureFlagId: flagId }) - ).toMatchObject({ - inclusionIds: expect.arrayContaining(['deleted-member']), - }); expect(await db.getRepository(ExperimentPrecomputedSegment).findOneBy({ experimentId })).toMatchObject({ inclusionIds: expect.arrayContaining(['deleted-member']), }); @@ -261,12 +280,6 @@ export function registerBatchDeleteTests(connections: () => [DataSource, DataSou expect(await db.getRepository(model[entity]).countBy({ id: row.id })).toBe(0); expect(await db.getRepository(Segment).countBy({ id: list.id })).toBe(0); if (entity === 'segments') { - expect( - await db.getRepository(FeatureFlagPrecomputedSegment).findOneBy({ featureFlagId: flagId }) - ).toMatchObject({ - inclusionIds: ['batch-member'], - exclusionIds: [], - }); expect(await db.getRepository(ExperimentPrecomputedSegment).findOneBy({ experimentId })).toMatchObject({ inclusionIds: ['batch-member'], exclusionIds: [], @@ -408,31 +421,20 @@ export function registerBatchDeleteTests(connections: () => [DataSource, DataSou } }); - test.each([false, true])('updates shared owners after segment deletion (recompute fails: %s)', async (fails) => { + test.each([false, true])('updates archived owners after segment deletion (recompute fails: %s)', async (fails) => { const rows = await create('segments', 2); - const [flag] = await create('flags'); const [experiment] = await create('experiments'); + await db.getRepository(Experiment).update(experiment.id, { state: EXPERIMENT_STATE.ARCHIVED }); const members = rows.map((row) => `member-${row.id}`); await db .getRepository(IndividualForSegment) .insert(rows.map((row, index) => ({ segmentId: row.id, userId: members[index] }))); - await db.getRepository(FeatureFlagSegmentInclusion).insert( - rows.map((row) => ({ - featureFlagId: flag.id, - segmentId: row.id, - enabled: true, - listType: STANDARD_LIST_TYPE.SEGMENT, - })) - ); await db .getRepository(ExperimentSegmentInclusion) .insert(rows.map((row) => ({ experimentId: experiment.id, segmentId: row.id }))); - const flags = Container.get(FeatureFlagPrecomputedSegmentService); const experiments = Container.get(ExperimentPrecomputedSegmentService); - const logger = new UpgradeLogger(); - await flags.recomputeForFlag(flag.id, logger); - await experiments.recomputeForExperiment(experiment.id, logger); - if (fails) jest.spyOn(flags, 'recomputeForFlag').mockRejectedValueOnce(new Error('Recompute failed')); + await experiments.recomputeForExperiment(experiment.id, new UpgradeLogger()); + if (fails) jest.spyOn(experiments, 'recomputeForExperiment').mockRejectedValueOnce(new Error('Recompute failed')); const { body } = await request(app) .post(route('segments')) @@ -447,12 +449,6 @@ export function registerBatchDeleteTests(connections: () => [DataSource, DataSou expect(await db.getRepository(Segment).countBy({ id: rows[1].id })).toBe(1); } else { expect(body.results).toEqual(rows.map((row) => ({ id: row.id, outcome: 'deleted' }))); - expect( - await db.getRepository(FeatureFlagPrecomputedSegment).findOneBy({ featureFlagId: flag.id }) - ).toMatchObject({ - inclusionIds: [], - exclusionIds: [], - }); expect( await db.getRepository(ExperimentPrecomputedSegment).findOneBy({ experimentId: experiment.id }) ).toMatchObject({ @@ -462,40 +458,47 @@ export function registerBatchDeleteTests(connections: () => [DataSource, DataSou } }); - test('recomputes owners attached before the segment deletion lock is acquired', async () => { - const [segment] = await create('segments'); - const [flag] = await create('flags'); - const [experiment] = await create('experiments'); - await db.getRepository(IndividualForSegment).insert({ segmentId: segment.id, userId: 'deleted-member' }); - const flags = Container.get(FeatureFlagPrecomputedSegmentService); - const experiments = Container.get(ExperimentPrecomputedSegmentService); - const logger = new UpgradeLogger(); - const findForDeletion = DeletionRepository.prototype.findForDeletion; - jest - .spyOn(DeletionRepository.prototype, 'findForDeletion') - .mockImplementationOnce(async function (this: DeletionRepository, ...args) { - // Commit a list edit on another connection just before the deletion acquires its target lock. - const flagList = await ownedList('flags', flag.id, segment.id, writer); - await writer.getRepository(FeatureFlagSegmentInclusion).update({ segmentId: flagList.id }, { enabled: true }); - await ownedList('experiments', experiment.id, segment.id, writer); - await flags.recomputeForFlag(flag.id, logger); - await experiments.recomputeForExperiment(experiment.id, logger); - return findForDeletion.apply(this, args); - }); - - const { body } = await request(app) - .post(route('segments')) - .send({ ids: [segment.id] }) - .expect(200); - expect(body.results).toEqual([{ id: segment.id, outcome: 'deleted' }]); - expect(await db.getRepository(Segment).countBy({ id: segment.id })).toBe(0); - expect(await db.getRepository(FeatureFlagPrecomputedSegment).findOneBy({ featureFlagId: flag.id })).toMatchObject( - { inclusionIds: ['batch-member'], exclusionIds: [] } - ); - expect( - await db.getRepository(ExperimentPrecomputedSegment).findOneBy({ experimentId: experiment.id }) - ).toMatchObject({ inclusionIds: ['batch-member'], exclusionIds: [] }); - }); + test.each(['single', 'batch'])( + '%s deletion refuses a target that becomes ineligible before its deletion lock', + async (mode) => { + for (const entity of entities) { + const [row] = await create(entity); + const [flag] = await create('flags'); + const findForDeletion = DeletionRepository.prototype.findForDeletion; + jest + .spyOn(DeletionRepository.prototype, 'findForDeletion') + .mockImplementationOnce(async function (this: DeletionRepository, ...args) { + // Commit a change on another connection just before the deletion acquires its target lock. + if (entity === 'experiments') { + await writer.getRepository(Experiment).update(row.id, { state: EXPERIMENT_STATE.ENROLLING }); + } else if (entity === 'flags') { + await writer.getRepository(FeatureFlag).update(row.id, { status: FEATURE_FLAG_STATUS.ENABLED }); + } else { + await ownedList('flags', flag.id, row.id, writer); + } + return findForDeletion.apply(this, args); + }); + const reasonCode = + entity === 'segments' ? DeletionReasonCode.SEGMENT_IN_USE : DeletionReasonCode.STATE_NOT_DELETABLE; + if (mode === 'single') { + const { body } = await request(app).delete(`/api/${entity}/${row.id}`).expect(409); + expect(body).toMatchObject({ + type: SERVER_ERROR.DELETION_NOT_ALLOWED, + message: expect.stringContaining(reasonCode), + }); + } else { + const { body } = await request(app) + .post(route(entity)) + .send({ ids: [row.id] }) + .expect(200); + expect(body.results).toEqual([{ id: row.id, outcome: 'refused', reasonCode }]); + } + expect(await db.getRepository(model[entity]).countBy({ id: row.id })).toBe(1); + jest.restoreAllMocks(); + } + expect(await db.getRepository(ExperimentAuditLog).countBy({ type: In(Object.values(deletionAudit)) })).toBe(0); + } + ); test('keeps a committed segment deletion when post-commit cache invalidation fails', async () => { const rows = await create('segments', 2); @@ -545,75 +548,149 @@ export function registerBatchDeleteTests(connections: () => [DataSource, DataSou } ); - test.each(entities)( - '%s single-delete route keeps its existing roles, cleanup and success response', - async (entity) => { - await db.getRepository(User).update(SYSTEM_USER_EMAIL, { role: UserRole.READER }); - const [row] = await create(entity); - const [child] = await create('segments'); - const list = await ownedList(entity, row.id, child.id); - const { body } = await request(app).delete(`/api/${entity}/${row.id}`).expect(200); - expect(body).toEqual( - entity === 'segments' ? expect.objectContaining({ id: row.id }) : [expect.objectContaining({ id: row.id })] - ); - expect(await db.getRepository(model[entity]).countBy({ id: row.id })).toBe(0); - expect(await db.getRepository(Segment).countBy({ id: list.id })).toBe(0); - expect(await db.getRepository(IndividualForSegment).countBy({ segmentId: list.id })).toBe(0); - expect(await db.getRepository(Segment).countBy({ id: child.id })).toBe(1); - } - ); + test.each(entities)('%s single-delete route keeps its existing cleanup and success response', async (entity) => { + const [row] = await create(entity); + const [child] = await create('segments'); + const list = await ownedList(entity, row.id, child.id); + const { body } = await request(app).delete(`/api/${entity}/${row.id}`).expect(200); + expect(body).toEqual( + entity === 'segments' ? expect.objectContaining({ id: row.id }) : [expect.objectContaining({ id: row.id })] + ); + expect(await db.getRepository(model[entity]).countBy({ id: row.id })).toBe(0); + expect(await db.getRepository(Segment).countBy({ id: list.id })).toBe(0); + expect(await db.getRepository(IndividualForSegment).countBy({ segmentId: list.id })).toBe(0); + expect(await db.getRepository(Segment).countBy({ id: child.id })).toBe(1); + }); - test.each(['single', 'batch'])('experiment %s deletion allows every experiment state', async (mode) => { - const states = [ - ...new Set( - Object.values(EXPERIMENT_STATE).map((state) => EXPERIMENT_STATE_INTERNAL_NAME_OVERRIDES[state] || state) - ), - ]; - const rows = await create('experiments', states.length); + test.each(['single', 'batch'])('experiment %s deletion applies the experiment state rules', async (mode) => { + const rows = await create('experiments', storedStates.length); for (const [index, row] of rows.entries()) { - await db.getRepository(Experiment).update(row.id, { state: states[index] }); + await db.getRepository(Experiment).update(row.id, { state: storedStates[index] }); } + const deletable = rows.filter((_, index) => deletableStates.includes(storedStates[index])); if (mode === 'single') { - for (const row of rows) await request(app).delete(`/api/experiments/${row.id}`).expect(200); + for (const [index, row] of rows.entries()) { + await request(app) + .delete(`/api/experiments/${row.id}`) + .expect(deletableStates.includes(storedStates[index]) ? 200 : 409); + } } else { const { body } = await request(app) .post(route('experiments')) .send({ ids: rows.map(({ id }) => id) }) .expect(200); - expect(body.results).toEqual(rows.map(({ id }) => ({ id, outcome: 'deleted' }))); + expect(body.results).toEqual( + rows.map(({ id }, index) => + deletableStates.includes(storedStates[index]) + ? { id, outcome: 'deleted' } + : { id, outcome: 'refused', reasonCode: DeletionReasonCode.STATE_NOT_DELETABLE } + ) + ); } - expect(await db.getRepository(Experiment).countBy({ id: In(rows.map(({ id }) => id)) })).toBe(0); + const remaining = await db.getRepository(Experiment).findBy({ id: In(rows.map(({ id }) => id)) }); + expect(remaining.map(({ state }) => state).sort()).toEqual( + storedStates.filter((state) => !deletableStates.includes(state)).sort() + ); expect(await db.getRepository(ExperimentAuditLog).countBy({ type: LOG_TYPE.EXPERIMENT_DELETED })).toBe( - rows.length + deletable.length ); }); - test.each(['single', 'batch'])('%s deletion allows Enabled flags and Used segments', async (mode) => { + test.each(['single', 'batch'])('%s deletion refuses Enabled flags and Used segments unchanged', async (mode) => { for (const entity of ['flags', 'segments'] as const) { const [row] = await create(entity); const list = await ownedList(entity, row.id); - let ownerId: string; + let reasonCode = DeletionReasonCode.STATE_NOT_DELETABLE; if (entity === 'flags') { await db.getRepository(FeatureFlag).update(row.id, { status: FEATURE_FLAG_STATUS.ENABLED }); } else { + reasonCode = DeletionReasonCode.SEGMENT_IN_USE; const [owner] = await create('flags'); - ownerId = owner.id; await ownedList('flags', owner.id, row.id); } + const reset = jest.spyOn(Container.get(CacheService), 'resetPrefixCache'); if (mode === 'single') { - await request(app).delete(`/api/${entity}/${row.id}`).expect(200); + await request(app).delete(`/api/${entity}/${row.id}`).expect(409); } else { const { body } = await request(app) .post(route(entity)) .send({ ids: [row.id] }) .expect(200); - expect(body.results).toEqual([{ id: row.id, outcome: 'deleted' }]); + expect(body.results).toEqual([{ id: row.id, outcome: 'refused', reasonCode }]); } - expect(await db.getRepository(model[entity]).countBy({ id: row.id })).toBe(0); - expect(await db.getRepository(Segment).countBy({ id: list.id })).toBe(0); - expect(await db.getRepository(IndividualForSegment).countBy({ segmentId: list.id })).toBe(0); - if (ownerId) expect(await db.getRepository(FeatureFlag).countBy({ id: ownerId })).toBe(1); + expect(reset).not.toHaveBeenCalled(); + expect(await db.getRepository(model[entity]).countBy({ id: row.id })).toBe(1); + expect(await db.getRepository(Segment).countBy({ id: list.id })).toBe(1); + expect(await db.getRepository(IndividualForSegment).countBy({ segmentId: list.id })).toBe(1); + jest.restoreAllMocks(); } + expect(await db.getRepository(ExperimentAuditLog).countBy({ type: LOG_TYPE.FEATURE_FLAG_DELETED })).toBe(0); + }); + + test('segment usage follows the Used status: nested lists and active owners refuse, archived owners allow', async () => { + const [parent, nested, viaExperiment, viaArchived] = await create('segments', 4); + // A member of another segment's list is Used. + await ownedList('segments', parent.id, nested.id); + const [active, archived] = await create('experiments', 2); + await db.getRepository(Experiment).update(archived.id, { state: EXPERIMENT_STATE.ARCHIVED }); + await ownedList('experiments', active.id, viaExperiment.id); + await ownedList('experiments', archived.id, viaArchived.id); + const ids = [nested.id, viaExperiment.id, viaArchived.id, parent.id]; + const { body } = await request(app).post(route('segments')).send({ ids }).expect(200); + expect(body.results).toEqual([ + { id: nested.id, outcome: 'refused', reasonCode: DeletionReasonCode.SEGMENT_IN_USE }, + { id: viaExperiment.id, outcome: 'refused', reasonCode: DeletionReasonCode.SEGMENT_IN_USE }, + { id: viaArchived.id, outcome: 'deleted' }, + // Owning a list does not make the owner itself Used. + { id: parent.id, outcome: 'deleted' }, + ]); + }); + + test('isSegmentInUse agrees with the Used status shown by getSegmentStatus', async () => { + // The deletion policy evaluates one locked segment; the segment list labels every segment at once. + // They are separate implementations of one rule, so pin them to each other across every kind of usage. + const [unused, viaActive, viaArchived, viaFlag, owner, member, nestedMember, direct, viaGlobalExclude] = + await create('segments', 9); + const [active, archived] = await create('experiments', 2); + await db.getRepository(Experiment).update(archived.id, { state: EXPERIMENT_STATE.ARCHIVED }); + const [flag] = await create('flags'); + await ownedList('experiments', active.id, viaActive.id); + await ownedList('experiments', archived.id, viaArchived.id); + await ownedList('flags', flag.id, viaFlag.id); + await ownedList('segments', owner.id, member.id); + await ownedList('segments', member.id, nestedMember.id); + // A public segment attached directly to an experiment, without a private list in between. + await db.getRepository(ExperimentSegmentInclusion).insert({ experimentId: active.id, segmentId: direct.id }); + const globalExclude = await db.getRepository(Segment).findOneBy({ type: SEGMENT_TYPE.GLOBAL_EXCLUDE }); + await ownedList('segments', globalExclude.id, viaGlobalExclude.id); + + const { segmentsData } = await Container.get(SegmentService).getAllSegmentWithStatus(new UpgradeLogger()); + const repository = new DeletionRepository(DeletionRepository, db.manager); + const fixtures = [unused, viaActive, viaArchived, viaFlag, owner, member, nestedMember, direct, viaGlobalExclude]; + const statusUsed = fixtures.map( + ({ id }) => + (segmentsData as SegmentWithStatus[]).find((segment) => segment.id === id)?.status === SEGMENT_STATUS.USED + ); + const inUse = await Promise.all(fixtures.map(({ id }) => repository.isSegmentInUse(id, db.manager))); + expect(inUse).toEqual(statusUsed); + expect(statusUsed).toEqual([false, true, false, true, false, true, true, true, true]); + }); + + test('reports each item of a mixed batch truthfully and continues past refusals', async () => { + const [deletable, running, last] = await create('experiments', 3); + await db.getRepository(Experiment).update(running.id, { state: EXPERIMENT_STATE.ENROLLING }); + const missing = randomUUID(); + const ids = [deletable.id, running.id, missing, last.id]; + const { body } = await request(app).post(route('experiments')).send({ ids }).expect(200); + expect(body.results).toEqual([ + { id: deletable.id, outcome: 'deleted' }, + { id: running.id, outcome: 'refused', reasonCode: DeletionReasonCode.STATE_NOT_DELETABLE }, + { id: missing, outcome: 'not_found', reasonCode: DeletionReasonCode.NOT_FOUND }, + { id: last.id, outcome: 'deleted' }, + ]); + expect(await db.getRepository(Experiment).findOneBy({ id: running.id })).toMatchObject({ + state: EXPERIMENT_STATE.ENROLLING, + }); }); test.each(entities)('%s single-delete preserves its original missing-target response', async (entity) => { @@ -623,22 +700,32 @@ export function registerBatchDeleteTests(connections: () => [DataSource, DataSou if (entity === 'experiments') expect(body.message).toBe('Experiment not found.'); }); - test.each(['single', 'batch'])('%s deletion allows private and global-exclude segments', async (mode) => { - for (const type of [SEGMENT_TYPE.PRIVATE, SEGMENT_TYPE.GLOBAL_EXCLUDE]) { - const [row] = await create('segments'); - await db.getRepository(Segment).update(row.id, { type }); - if (mode === 'single') { - await request(app).delete(`/api/segments/${row.id}`).expect(200); - } else { - const { body } = await request(app) - .post(route('segments')) - .send({ ids: [row.id] }) - .expect(200); - expect(body.results).toEqual([{ id: row.id, outcome: 'deleted' }]); + test.each(['single', 'batch'])( + '%s deletion allows unused private segments and refuses global-exclude', + async (mode) => { + for (const type of [SEGMENT_TYPE.PRIVATE, SEGMENT_TYPE.GLOBAL_EXCLUDE]) { + const [row] = await create('segments'); + await db.getRepository(Segment).update(row.id, { type }); + const refused = type === SEGMENT_TYPE.GLOBAL_EXCLUDE; + if (mode === 'single') { + await request(app) + .delete(`/api/segments/${row.id}`) + .expect(refused ? 409 : 200); + } else { + const { body } = await request(app) + .post(route('segments')) + .send({ ids: [row.id] }) + .expect(200); + expect(body.results).toEqual([ + refused + ? { id: row.id, outcome: 'refused', reasonCode: DeletionReasonCode.GLOBAL_EXCLUDE_PROTECTED } + : { id: row.id, outcome: 'deleted' }, + ]); + } + expect(await db.getRepository(Segment).countBy({ id: row.id })).toBe(refused ? 1 : 0); } - expect(await db.getRepository(Segment).countBy({ id: row.id })).toBe(0); } - }); + ); test.each(entities)( '%s private-list endpoint still deletes lists and preserves their public children', diff --git a/packages/backend/test/unit/controllers/ExperimentController.test.ts b/packages/backend/test/unit/controllers/ExperimentController.test.ts index 234e3359b5..b6af4db0d3 100644 --- a/packages/backend/test/unit/controllers/ExperimentController.test.ts +++ b/packages/backend/test/unit/controllers/ExperimentController.test.ts @@ -1,4 +1,5 @@ import { BatchDeleteService } from '../../../src/api/services/BatchDeleteService'; +import { DeletionPolicyService } from '../../../src/api/services/DeletionPolicyService'; import app from '../../utils/expressApp'; import request from 'supertest'; import { configureLogger } from '../../utils/logger'; @@ -31,6 +32,10 @@ describe('Experiment Controller Testing', () => { routingUseContainer(Container); classValidatorUseContainer(Container); Container.set(BatchDeleteService, {} as BatchDeleteService); + Container.set(DeletionPolicyService, { + assertRoleCanDelete: jest.fn(), + singleDeletionTransaction: jest.fn(), + } as unknown as DeletionPolicyService); // set mock container Container.set(ExperimentService, new ExperimentServiceMock()); diff --git a/packages/backend/test/unit/controllers/ExperimentControllerAdaptiveConfig.test.ts b/packages/backend/test/unit/controllers/ExperimentControllerAdaptiveConfig.test.ts index 72d3cd13ab..d629903805 100644 --- a/packages/backend/test/unit/controllers/ExperimentControllerAdaptiveConfig.test.ts +++ b/packages/backend/test/unit/controllers/ExperimentControllerAdaptiveConfig.test.ts @@ -30,6 +30,7 @@ describe('ExperimentController adaptive config wiring', () => { {} as any, {} as any, adaptiveExperimentConfigDispatcher, + {} as any, {} as any ); }); diff --git a/packages/backend/test/unit/controllers/FeatureFlagController.test.ts b/packages/backend/test/unit/controllers/FeatureFlagController.test.ts index 3cfc4084b8..a0795ec13c 100644 --- a/packages/backend/test/unit/controllers/FeatureFlagController.test.ts +++ b/packages/backend/test/unit/controllers/FeatureFlagController.test.ts @@ -1,4 +1,5 @@ import { BatchDeleteService } from '../../../src/api/services/BatchDeleteService'; +import { DeletionPolicyService } from '../../../src/api/services/DeletionPolicyService'; import app from '../../utils/expressApp'; import request from 'supertest'; import { configureLogger } from '../../utils/logger'; @@ -20,6 +21,10 @@ describe('Feature Flag Controller Testing', () => { routingUseContainer(Container); classValidatorUseContainer(Container); Container.set(BatchDeleteService, {} as BatchDeleteService); + Container.set(DeletionPolicyService, { + assertRoleCanDelete: jest.fn(), + singleDeletionTransaction: jest.fn(), + } as unknown as DeletionPolicyService); Container.set(FeatureFlagService, new FeatureFlagServiceMock()); Container.set(ExperimentUserService, new ExperimentUserServiceMock()); diff --git a/packages/backend/test/unit/controllers/SegmentController.test.ts b/packages/backend/test/unit/controllers/SegmentController.test.ts index 9c7436ad71..b02dd401d9 100644 --- a/packages/backend/test/unit/controllers/SegmentController.test.ts +++ b/packages/backend/test/unit/controllers/SegmentController.test.ts @@ -1,4 +1,5 @@ import { BatchDeleteService } from '../../../src/api/services/BatchDeleteService'; +import { DeletionPolicyService } from '../../../src/api/services/DeletionPolicyService'; import app from '../../utils/expressApp'; import request from 'supertest'; import { configureLogger } from '../../utils/logger'; @@ -16,6 +17,10 @@ describe('Segment Controller Testing', () => { routingUseContainer(Container); classValidatorUseContainer(Container); Container.set(BatchDeleteService, {} as BatchDeleteService); + Container.set(DeletionPolicyService, { + assertRoleCanDelete: jest.fn(), + singleDeletionTransaction: jest.fn(), + } as unknown as DeletionPolicyService); // set mock container Container.set(SegmentService, new SegmentServiceMock()); diff --git a/packages/backend/test/unit/services/BatchDeleteService.test.ts b/packages/backend/test/unit/services/BatchDeleteService.test.ts index b09ad4348d..7d6b617d7a 100644 --- a/packages/backend/test/unit/services/BatchDeleteService.test.ts +++ b/packages/backend/test/unit/services/BatchDeleteService.test.ts @@ -1,6 +1,15 @@ import { randomUUID } from 'crypto'; import { DataSource, EntityManager, QueryRunner } from 'typeorm'; -import { BatchDeleteEntity, CACHE_PREFIX, DeletionReasonCode, UserRole } from 'upgrade_types'; +import { ForbiddenError } from 'routing-controllers'; +import { + BatchDeleteEntity, + CACHE_PREFIX, + DeletionReasonCode, + EXPERIMENT_STATE, + FEATURE_FLAG_STATUS, + SEGMENT_TYPE, + UserRole, +} from 'upgrade_types'; import { BatchDeleteService } from '../../../src/api/services/BatchDeleteService'; import { ExperimentService } from '../../../src/api/services/ExperimentService'; import { FeatureFlagService } from '../../../src/api/services/FeatureFlagService'; @@ -9,6 +18,8 @@ import { FeatureFlagPrecomputedSegmentService } from '../../../src/api/services/ import { ExperimentPrecomputedSegmentService } from '../../../src/api/services/ExperimentPrecomputedSegmentService'; import { UpgradeLogger } from '../../../src/lib/logger/UpgradeLogger'; import { DeletionRepository } from '../../../src/api/repositories/DeletionRepository'; +import { DeletionPolicyService } from '../../../src/api/services/DeletionPolicyService'; +import { PermissionService } from '../../../src/api/services/PermissionService'; describe('BatchDeleteService transaction outcomes', () => { const entities: BatchDeleteEntity[] = ['experiments', 'flags', 'segments']; @@ -24,6 +35,13 @@ describe('BatchDeleteService transaction outcomes', () => { let createQueryRunner: jest.Mock; let work: jest.Mock; let configureRunner: (runner: QueryRunner, index: number) => void; + let deletionRepository: DeletionRepository; + // One locked projection that every entity's policy allows: Inactive, Disabled, an unused public segment. + const deletable = { + state: EXPERIMENT_STATE.INACTIVE, + status: FEATURE_FLAG_STATUS.DISABLED, + type: SEGMENT_TYPE.PUBLIC, + }; beforeEach(() => { (logger.error as jest.Mock).mockClear(); @@ -39,7 +57,7 @@ describe('BatchDeleteService transaction outcomes', () => { }, manager: { getRepository: jest.fn(() => ({ - findOne: jest.fn(async ({ where }) => ({ id: where.id })), + findOne: jest.fn(async ({ where }) => ({ id: where.id, ...deletable })), })), }, connect: jest.fn().mockResolvedValue(undefined), @@ -72,12 +90,15 @@ describe('BatchDeleteService transaction outcomes', () => { }) ), }; + deletionRepository = new DeletionRepository(DeletionRepository, {} as EntityManager); + jest.spyOn(deletionRepository, 'isSegmentInUse').mockResolvedValue(false); service = new BatchDeleteService( { createQueryRunner } as unknown as DataSource, experiments as unknown as ExperimentService, flags as unknown as FeatureFlagService, segments as unknown as SegmentService, - new DeletionRepository(DeletionRepository, {} as EntityManager) + deletionRepository, + new DeletionPolicyService({} as DataSource, deletionRepository, new PermissionService()) ); }); afterEach(() => { @@ -246,6 +267,60 @@ describe('BatchDeleteService transaction outcomes', () => { expect(mutations).toEqual(ids.slice(1)); }); + const refusals: [BatchDeleteEntity, object, DeletionReasonCode][] = [ + ['experiments', { state: EXPERIMENT_STATE.ENROLLING }, DeletionReasonCode.STATE_NOT_DELETABLE], + ['flags', { status: FEATURE_FLAG_STATUS.ENABLED }, DeletionReasonCode.STATE_NOT_DELETABLE], + ['segments', { type: SEGMENT_TYPE.GLOBAL_EXCLUDE }, DeletionReasonCode.GLOBAL_EXCLUDE_PROTECTED], + ]; + test.each(refusals)( + '%s refuses an ineligible locked target without mutation and continues', + async (entity, fields, reasonCode) => { + configureRunner = (runner, index) => { + if (index !== 1) return; + (runner.manager.getRepository as jest.Mock).mockReturnValue({ + findOne: jest.fn(async ({ where }) => ({ id: where.id, ...deletable, ...fields })), + }); + }; + const result = await service.delete(entity, ids, user, logger); + expect(result.results).toEqual([ + { id: ids[0], outcome: 'deleted' }, + { id: ids[1], outcome: 'refused', reasonCode }, + { id: ids[2], outcome: 'deleted' }, + ]); + expect(mutations).toEqual([ids[0], ids[2]]); + expect(runners[1].rollbackTransaction).toHaveBeenCalledTimes(1); + expect(runners[1].commitTransaction).not.toHaveBeenCalled(); + expect(logger.error).not.toHaveBeenCalled(); + } + ); + + test('refuses a Used segment using the usage read inside the locked transaction', async () => { + (deletionRepository.isSegmentInUse as jest.Mock).mockImplementation(async (id) => id === ids[0]); + const result = await service.delete('segments', ids, user, logger); + expect(result.results[0]).toEqual({ + id: ids[0], + outcome: 'refused', + reasonCode: DeletionReasonCode.SEGMENT_IN_USE, + }); + expect(result.results.slice(1).map((item) => item.outcome)).toEqual(['deleted', 'deleted']); + expect(deletionRepository.isSegmentInUse).toHaveBeenCalledWith(ids[0], runners[0].manager); + }); + + test.each([ + [UserRole.READER, 'experiments'], + [UserRole.READER, 'flags'], + [UserRole.READER, 'segments'], + [UserRole.USER_MANAGER, 'experiments'], + [UserRole.USER_MANAGER, 'flags'], + ] as [UserRole, BatchDeleteEntity][])( + 'rejects a %s deleting %s before opening any transaction', + async (role, entity) => { + await expect(service.delete(entity, ids, { ...user, role }, logger)).rejects.toBeInstanceOf(ForbiddenError); + expect(createQueryRunner).not.toHaveBeenCalled(); + expect(mutations).toEqual([]); + } + ); + test('reports a lock timeout only after a confirmed rollback', async () => { work.mockRejectedValue(Object.assign(new Error('lock wait'), { code: '55P03' })); const result = await service.delete('flags', ids, user, logger); diff --git a/packages/backend/test/unit/services/DeletionPolicyService.test.ts b/packages/backend/test/unit/services/DeletionPolicyService.test.ts new file mode 100644 index 0000000000..480f0f545a --- /dev/null +++ b/packages/backend/test/unit/services/DeletionPolicyService.test.ts @@ -0,0 +1,122 @@ +import { ForbiddenError } from 'routing-controllers'; +import { DataSource, EntityManager } from 'typeorm'; +import { + BatchDeleteEntity, + DeletionReasonCode, + EXPERIMENT_STATE, + FEATURE_FLAG_STATUS, + SEGMENT_TYPE, + SERVER_ERROR, + UserRole, +} from 'upgrade_types'; +import { DeletionRepository } from '../../../src/api/repositories/DeletionRepository'; +import { DeletionPolicyService, DeletionRefusedError } from '../../../src/api/services/DeletionPolicyService'; +import { PermissionService } from '../../../src/api/services/PermissionService'; + +describe('DeletionPolicyService', () => { + const manager = {} as EntityManager; + let repository: { findForDeletion: jest.Mock; isSegmentInUse: jest.Mock }; + let policy: DeletionPolicyService; + + beforeEach(() => { + repository = { findForDeletion: jest.fn(), isSegmentInUse: jest.fn().mockResolvedValue(false) }; + const dataSource = { transaction: jest.fn((work) => work(manager)) } as unknown as DataSource; + policy = new DeletionPolicyService( + dataSource, + repository as unknown as DeletionRepository, + new PermissionService() + ); + }); + + const allowed: Record = { + [UserRole.ADMIN]: ['experiments', 'flags', 'segments'], + [UserRole.CREATOR]: ['experiments', 'flags', 'segments'], + [UserRole.USER_MANAGER]: ['segments'], + [UserRole.READER]: [], + }; + test.each( + Object.values(UserRole).flatMap((role) => + (['experiments', 'flags', 'segments'] as BatchDeleteEntity[]).map((entity) => ({ + role, + entity, + permitted: allowed[role].includes(entity), + })) + ) + )('$role deleting $entity is permitted: $permitted', ({ role, entity, permitted }) => { + const check = () => policy.assertRoleCanDelete(entity, { email: 'user@example.com', role } as any); + if (permitted) expect(check).not.toThrow(); + else expect(check).toThrow(ForbiddenError); + }); + + test('refuses a caller with no user or no role', () => { + expect(() => policy.assertRoleCanDelete('segments', undefined)).toThrow(ForbiddenError); + expect(() => policy.assertRoleCanDelete('segments', { email: 'user@example.com' } as any)).toThrow(ForbiddenError); + }); + + const deletableStates = [ + EXPERIMENT_STATE.DRAFT, + EXPERIMENT_STATE.INACTIVE, + EXPERIMENT_STATE.COMPLETED, + EXPERIMENT_STATE.CANCELLED, + EXPERIMENT_STATE.ARCHIVED, + ]; + test.each(Object.values(EXPERIMENT_STATE))('experiment state %s', async (state) => { + expect(await policy.getRefusalReason('experiments', { id: 'e', state }, manager)).toBe( + deletableStates.includes(state) ? undefined : DeletionReasonCode.STATE_NOT_DELETABLE + ); + }); + + test.each(Object.values(FEATURE_FLAG_STATUS))('feature flag status %s', async (status) => { + expect(await policy.getRefusalReason('flags', { id: 'f', status }, manager)).toBe( + status === FEATURE_FLAG_STATUS.ENABLED ? DeletionReasonCode.STATE_NOT_DELETABLE : undefined + ); + }); + + test('segments: refuses global-exclude without a usage read, then Used, and allows unused', async () => { + expect(await policy.getRefusalReason('segments', { id: 's', type: SEGMENT_TYPE.GLOBAL_EXCLUDE }, manager)).toBe( + DeletionReasonCode.GLOBAL_EXCLUDE_PROTECTED + ); + expect(repository.isSegmentInUse).not.toHaveBeenCalled(); + repository.isSegmentInUse.mockResolvedValueOnce(true); + expect(await policy.getRefusalReason('segments', { id: 's', type: SEGMENT_TYPE.PUBLIC }, manager)).toBe( + DeletionReasonCode.SEGMENT_IN_USE + ); + expect(repository.isSegmentInUse).toHaveBeenCalledWith('s', manager); + expect(await policy.getRefusalReason('segments', { id: 's', type: SEGMENT_TYPE.PUBLIC }, manager)).toBe(undefined); + }); + + describe('singleDeletionTransaction', () => { + test('refuses before running the deletion work', async () => { + repository.findForDeletion.mockResolvedValue({ id: 'f', status: FEATURE_FLAG_STATUS.ENABLED }); + const work = jest.fn(); + const error = await policy + .singleDeletionTransaction( + 'flags', + 'f' + )(work) + .catch((e) => e); + expect(error).toBeInstanceOf(DeletionRefusedError); + expect(error).toMatchObject({ + httpCode: 409, + type: SERVER_ERROR.DELETION_NOT_ALLOWED, + reasonCode: DeletionReasonCode.STATE_NOT_DELETABLE, + }); + expect(repository.findForDeletion).toHaveBeenCalledWith('flags', 'f', manager); + expect(work).not.toHaveBeenCalled(); + }); + + test('runs the deletion work in the locking transaction when allowed', async () => { + repository.findForDeletion.mockResolvedValue({ id: 'f', status: FEATURE_FLAG_STATUS.DISABLED }); + const work = jest.fn().mockResolvedValue('deleted'); + expect(await policy.singleDeletionTransaction('flags', 'f')(work)).toBe('deleted'); + expect(work).toHaveBeenCalledWith(manager); + }); + + test('leaves a missing target to the entity service', async () => { + repository.findForDeletion.mockResolvedValue(null); + const work = jest.fn().mockResolvedValue(undefined); + await policy.singleDeletionTransaction('experiments', 'e')(work); + expect(work).toHaveBeenCalledWith(manager); + }); + }); +}); diff --git a/packages/frontend/projects/upgrade/src/app/core/auth/auth.service.ts b/packages/frontend/projects/upgrade/src/app/core/auth/auth.service.ts index b200c492a9..7fe2c90732 100644 --- a/packages/frontend/projects/upgrade/src/app/core/auth/auth.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/auth/auth.service.ts @@ -13,6 +13,7 @@ import { } from './store/auth.selectors'; import { UserPermission } from './store/auth.models'; import { BehaviorSubject, filter, take } from 'rxjs'; +import { ROLE_PERMISSIONS } from 'upgrade_types'; import { AUTH_CONSTANTS, GoogleAuthJWTPayload, User, UserRole } from '../users/store/users.model'; import { ENV, Environment } from '../../../environments/environment-types'; import jwt_decode from 'jwt-decode'; @@ -176,56 +177,8 @@ export class AuthService { } setUserPermissions(role: UserRole): void { - switch (role) { - // Permissions for managing queries will be same as experiments - case UserRole.ADMIN: - this.userPermissions$.next({ - experiments: { create: true, read: true, update: true, delete: true }, - stratifications: { create: true, read: true, update: true, delete: true }, - users: { create: true, read: true, update: true, delete: true }, - logs: { create: true, read: true, update: true, delete: true }, - manageRoles: { create: true, read: true, update: true, delete: true }, - featureFlags: { create: true, read: true, update: true, delete: true }, - metrics: { create: true, read: true, update: true, delete: true }, - segments: { create: true, read: true, update: true, delete: true }, - }); - break; - case UserRole.CREATOR: - this.userPermissions$.next({ - experiments: { create: true, read: true, update: true, delete: true }, - stratifications: { create: true, read: true, update: true, delete: true }, - users: { create: true, read: true, update: true, delete: true }, - logs: { create: false, read: true, update: false, delete: false }, - manageRoles: { create: false, read: true, update: false, delete: false }, - featureFlags: { create: true, read: true, update: true, delete: true }, - metrics: { create: true, read: true, update: true, delete: true }, - segments: { create: true, read: true, update: true, delete: true }, - }); - break; - case UserRole.USER_MANAGER: - this.userPermissions$.next({ - experiments: { create: false, read: true, update: false, delete: false }, - stratifications: { create: false, read: true, update: false, delete: false }, - users: { create: true, read: true, update: true, delete: true }, - logs: { create: false, read: true, update: false, delete: false }, - manageRoles: { create: false, read: true, update: false, delete: false }, - featureFlags: { create: false, read: true, update: false, delete: false }, - metrics: { create: false, read: true, update: false, delete: false }, - segments: { create: false, read: true, update: true, delete: true }, - }); - break; - case UserRole.READER: - this.userPermissions$.next({ - experiments: { create: false, read: true, update: false, delete: false }, - stratifications: { create: false, read: true, update: false, delete: false }, - users: { create: false, read: true, update: false, delete: false }, - logs: { create: false, read: true, update: false, delete: false }, - manageRoles: { create: false, read: true, update: false, delete: false }, - featureFlags: { create: false, read: true, update: false, delete: false }, - metrics: { create: false, read: true, update: false, delete: false }, - segments: { create: false, read: true, update: false, delete: false }, - }); - break; - } + // The backend enforces the same shared table. + const permissions = ROLE_PERMISSIONS[role]; + if (permissions) this.userPermissions$.next(permissions); } } diff --git a/packages/frontend/projects/upgrade/src/app/core/auth/store/auth.models.ts b/packages/frontend/projects/upgrade/src/app/core/auth/store/auth.models.ts index 17531522ab..bdaaf674a5 100755 --- a/packages/frontend/projects/upgrade/src/app/core/auth/store/auth.models.ts +++ b/packages/frontend/projects/upgrade/src/app/core/auth/store/auth.models.ts @@ -1,23 +1,8 @@ +import { RolePermissions } from 'upgrade_types'; import { AppState } from '../../core.module'; import { User } from '../../users/store/users.model'; -interface CRUD { - create: boolean; - read: boolean; - update: boolean; - delete: boolean; -} - -export interface UserPermission { - experiments: CRUD; - stratifications: CRUD; - users: CRUD; - logs: CRUD; - manageRoles: CRUD; - featureFlags: CRUD; - metrics: CRUD; - segments: CRUD; -} +export type UserPermission = RolePermissions; export interface AuthState { isLoggedIn: boolean; diff --git a/packages/frontend/projects/upgrade/src/app/core/batch-actions/batch-actions.helpers.ts b/packages/frontend/projects/upgrade/src/app/core/batch-actions/batch-actions.helpers.ts index 441b50371d..846bf60e22 100644 --- a/packages/frontend/projects/upgrade/src/app/core/batch-actions/batch-actions.helpers.ts +++ b/packages/frontend/projects/upgrade/src/app/core/batch-actions/batch-actions.helpers.ts @@ -1,9 +1,9 @@ import { BatchDeleteEntity, BatchDeleteResult, + DELETABLE_EXPERIMENT_STATES, DeletionReasonCode, EXPERIMENT_STATE, - EXPERIMENT_STATE_DISPLAY_NAME_OVERRIDES, FEATURE_FLAG_STATUS, SEGMENT_STATUS, } from 'upgrade_types'; @@ -23,16 +23,9 @@ export function selectionItem(row: { return { id: row.id, name: row.name, stateOrStatus: row.state || row.status }; } +/** Mirrors the backend deletion policy, which uses the same shared state list. */ export function getExperimentDeletionReason(state?: EXPERIMENT_STATE): DeletionEligibilityReasonCode | undefined { - const displayState = EXPERIMENT_STATE_DISPLAY_NAME_OVERRIDES[state] || state; - return [ - EXPERIMENT_STATE.DRAFT, - EXPERIMENT_STATE.INACTIVE, - EXPERIMENT_STATE.COMPLETED, - EXPERIMENT_STATE.ARCHIVED, - ].includes(displayState) - ? undefined - : DeletionEligibilityReasonCode.EXPERIMENT_ACTIVE; + return DELETABLE_EXPERIMENT_STATES.includes(state) ? undefined : DeletionEligibilityReasonCode.EXPERIMENT_ACTIVE; } export function localDeletionReason( @@ -69,7 +62,7 @@ export const confirmedRemovedIds = (result?: BatchDeleteResult) => []; export function validateBatchDeleteResponse(result: BatchDeleteResult, ids: string[]): BatchDeleteResult { - const outcomes = ['deleted', 'not_found', 'failed', 'unknown', 'not_attempted']; + const outcomes = ['deleted', 'not_found', 'refused', 'failed', 'unknown', 'not_attempted']; if (!result || !Array.isArray(result.results) || result.results.length !== ids.length) { throw new Error('Incomplete batch deletion response'); } @@ -89,6 +82,8 @@ export function batchDeleteResultCounts(state: RootBatchDeleteState) { return { deleted: results.filter((item) => item.outcome === 'deleted').length, absent: results.filter((item) => item.outcome === 'not_found').length, + // The server refused an item the list showed as deletable, e.g. it started running after the list loaded. + refused: results.filter((item) => item.outcome === 'refused').length, uncertain: results.some((item) => item.outcome === 'unknown'), failed: results.filter((item) => item.outcome === 'failed').length, notAttempted: results.filter((item) => item.outcome === 'not_attempted').length, @@ -112,6 +107,10 @@ export function batchDeleteResultMessage( parts.push( translate(`batch-delete.result.absent.${counts.absent === 1 ? 'one' : 'other'}`, { count: counts.absent }) ); + if (counts.refused) + parts.push( + translate(`batch-delete.result.refused.${counts.refused === 1 ? 'one' : 'other'}`, { count: counts.refused }) + ); if (counts.failed) parts.push(translate('batch-delete.result.failed')); if (counts.notAttempted) parts.push( diff --git a/packages/frontend/projects/upgrade/src/app/core/batch-actions/batch-actions.integration.spec.ts b/packages/frontend/projects/upgrade/src/app/core/batch-actions/batch-actions.integration.spec.ts index d3a393c718..4ff4e4033e 100644 --- a/packages/frontend/projects/upgrade/src/app/core/batch-actions/batch-actions.integration.spec.ts +++ b/packages/frontend/projects/upgrade/src/app/core/batch-actions/batch-actions.integration.spec.ts @@ -380,6 +380,27 @@ describe.each(fixtures)('$entity batch store/effects integration', (config) => { expect(batch().confirmation).toBeNull(); }); + it('deletes past a server-refused item, keeps it selected and warns that it was skipped', () => { + selectRows(); + const snapshot = prepare(); + store.dispatch(actions.batchDeleteRequested({ snapshot })); + response.next({ + results: [ + { id: rows[0].id, outcome: 'deleted' }, + { id: rows[1].id, outcome: 'refused', reasonCode: 'state_not_deletable' }, + { id: rows[2].id, outcome: 'deleted' }, + ], + }); + expect(Object.keys(batch().selectedById)).toEqual([rows[1].id]); + expect(batchDeleteResultCounts(batch())).toMatchObject({ deleted: 2, refused: 1, failed: 0, notAttempted: 0 }); + const noun = { experiments: 'experiments', flags: 'feature flags', segments: 'segments' }[config.entity]; + expect(notifications.showWarning).toHaveBeenCalledWith( + `2 ${noun} deleted. 1 item is no longer eligible for deletion and was skipped.` + ); + expect(notifications.showSuccess).not.toHaveBeenCalled(); + expect(notifications.showError).not.toHaveBeenCalled(); + }); + it.each([0, 1])('reports success when %i of two items are deleted and the rest were already absent', (deleted) => { selectRows(2); const snapshot = prepare(); diff --git a/packages/frontend/projects/upgrade/src/assets/i18n/en.json b/packages/frontend/projects/upgrade/src/assets/i18n/en.json index 7dc7b4b196..0bbf0872cc 100644 --- a/packages/frontend/projects/upgrade/src/assets/i18n/en.json +++ b/packages/frontend/projects/upgrade/src/assets/i18n/en.json @@ -77,6 +77,8 @@ "batch-delete.success.segments.other": "{{deleted}} segments deleted.", "batch-delete.result.absent.one": "1 item was already absent.", "batch-delete.result.absent.other": "{{count}} items were already absent.", + "batch-delete.result.refused.one": "1 item is no longer eligible for deletion and was skipped.", + "batch-delete.result.refused.other": "{{count}} items are no longer eligible for deletion and were skipped.", "batch-delete.result.failed": "1 item could not be deleted.", "batch-delete.result.notAttempted.one": "1 item was not attempted.", "batch-delete.result.notAttempted.other": "{{count}} items were not attempted.", diff --git a/packages/types/src/BatchActions/index.ts b/packages/types/src/BatchActions/index.ts index e7cdb725b6..d26f06d425 100644 --- a/packages/types/src/BatchActions/index.ts +++ b/packages/types/src/BatchActions/index.ts @@ -1,3 +1,5 @@ +import { EXPERIMENT_STATE } from '../Experiment/enums'; + export interface BatchEntityIdsRequest { ids: string[]; } @@ -8,11 +10,27 @@ export enum DeletionReasonCode { LOCK_TIMEOUT = 'lock_timeout', OUTCOME_UNKNOWN = 'outcome_unknown', POST_DELETE_FAILED = 'post_delete_failed', + // Policy refusals: the target exists but its current state or usage does not allow deletion. + STATE_NOT_DELETABLE = 'state_not_deletable', + SEGMENT_IN_USE = 'segment_in_use', + GLOBAL_EXCLUDE_PROTECTED = 'global_exclude_protected', } export type BatchDeleteEntity = 'experiments' | 'flags' | 'segments'; -export type BatchDeleteItemOutcome = 'deleted' | 'not_found' | 'failed' | 'unknown' | 'not_attempted'; +export type BatchDeleteItemOutcome = 'deleted' | 'not_found' | 'refused' | 'failed' | 'unknown' | 'not_attempted'; + +/** + * Stored experiment states that allow deletion: Draft, Inactive, Completed (stored as Cancelled) and Archived. + * Running, Paused, Preview and Scheduled experiments are not deletable. + */ +export const DELETABLE_EXPERIMENT_STATES: EXPERIMENT_STATE[] = [ + EXPERIMENT_STATE.DRAFT, + EXPERIMENT_STATE.INACTIVE, + EXPERIMENT_STATE.COMPLETED, + EXPERIMENT_STATE.CANCELLED, + EXPERIMENT_STATE.ARCHIVED, +]; export interface BatchDeleteItemResult { id: string; diff --git a/packages/types/src/Experiment/enums.ts b/packages/types/src/Experiment/enums.ts index 7b93d6a03b..eb0e136c38 100644 --- a/packages/types/src/Experiment/enums.ts +++ b/packages/types/src/Experiment/enums.ts @@ -108,6 +108,7 @@ export enum SERVER_ERROR { TOKEN_VALIDATION_FAILED = 'JWT Token validation failed', MIGRATION_ERROR = 'Error in migration', EMAIL_SEND_ERROR = 'Email send error', + DELETION_NOT_ALLOWED = 'Deletion is not allowed for the current state or usage of this item', CONDITION_NOT_FOUND = 'Condition not found', EXPERIMENT_ID_MISSING_FOR_SHARED_DECISIONPOINT = 'Experiment ID not provided for shared Decision Point', INVALID_EXPERIMENT_ID_FOR_SHARED_DECISIONPOINT = 'Experiment ID provided is invalid for shared Decision Point', diff --git a/packages/types/src/Permissions/index.ts b/packages/types/src/Permissions/index.ts new file mode 100644 index 0000000000..a57595089e --- /dev/null +++ b/packages/types/src/Permissions/index.ts @@ -0,0 +1,76 @@ +import { UserRole } from '../Experiment/enums'; + +export type PermissionAction = 'create' | 'read' | 'update' | 'delete'; + +export type PermissionResource = + | 'experiments' + | 'stratifications' + | 'users' + | 'logs' + | 'manageRoles' + | 'featureFlags' + | 'metrics' + | 'segments'; + +export type ResourcePermissions = Record; + +export type RolePermissions = Record; + +const ALL: ResourcePermissions = { create: true, read: true, update: true, delete: true }; +const READ_ONLY: ResourcePermissions = { create: false, read: true, update: false, delete: false }; + +/** + * What each role may do to each resource. The frontend uses this to show or hide actions; the backend enforces it. + * Permissions for managing queries are the same as for experiments. + */ +export const ROLE_PERMISSIONS: Record = { + [UserRole.ADMIN]: { + experiments: ALL, + stratifications: ALL, + users: ALL, + logs: ALL, + manageRoles: ALL, + featureFlags: ALL, + metrics: ALL, + segments: ALL, + }, + [UserRole.CREATOR]: { + experiments: ALL, + stratifications: ALL, + users: ALL, + logs: READ_ONLY, + manageRoles: READ_ONLY, + featureFlags: ALL, + metrics: ALL, + segments: ALL, + }, + [UserRole.USER_MANAGER]: { + experiments: READ_ONLY, + stratifications: READ_ONLY, + users: ALL, + logs: READ_ONLY, + manageRoles: READ_ONLY, + featureFlags: READ_ONLY, + metrics: READ_ONLY, + segments: { create: false, read: true, update: true, delete: true }, + }, + [UserRole.READER]: { + experiments: READ_ONLY, + stratifications: READ_ONLY, + users: READ_ONLY, + logs: READ_ONLY, + manageRoles: READ_ONLY, + featureFlags: READ_ONLY, + metrics: READ_ONLY, + segments: READ_ONLY, + }, +}; + +/** Whether a role may perform an action on a resource. An unknown or missing role is permitted nothing. */ +export function isPermitted( + role: UserRole | undefined, + resource: PermissionResource, + action: PermissionAction +): boolean { + return !!role && !!ROLE_PERMISSIONS[role]?.[resource][action]; +} diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 75d8ae6ca9..8f05175842 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -101,7 +101,16 @@ export { BatchDeleteItemOutcome, BatchDeleteItemResult, BatchDeleteResult, + DELETABLE_EXPERIMENT_STATES, } from './BatchActions'; +export { + PermissionAction, + PermissionResource, + ResourcePermissions, + RolePermissions, + ROLE_PERMISSIONS, + isPermitted, +} from './Permissions'; export { Prior, BinaryRewardAllowedValue, From 854b21df423f02a0f0067cc2ddcb0ff988b8ef14 Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Wed, 11 Nov 2026 15:28:58 -0500 Subject: [PATCH 2/3] reduce enum label character count --- packages/types/src/Experiment/enums.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/types/src/Experiment/enums.ts b/packages/types/src/Experiment/enums.ts index eb0e136c38..b7ca7d2b28 100644 --- a/packages/types/src/Experiment/enums.ts +++ b/packages/types/src/Experiment/enums.ts @@ -108,7 +108,7 @@ export enum SERVER_ERROR { TOKEN_VALIDATION_FAILED = 'JWT Token validation failed', MIGRATION_ERROR = 'Error in migration', EMAIL_SEND_ERROR = 'Email send error', - DELETION_NOT_ALLOWED = 'Deletion is not allowed for the current state or usage of this item', + DELETION_NOT_ALLOWED = 'Deletion is not allowed for the current state of this item', CONDITION_NOT_FOUND = 'Condition not found', EXPERIMENT_ID_MISSING_FOR_SHARED_DECISIONPOINT = 'Experiment ID not provided for shared Decision Point', INVALID_EXPERIMENT_ID_FOR_SHARED_DECISIONPOINT = 'Experiment ID provided is invalid for shared Decision Point', From 77fc0a6b61493613e41892131eadfb7fb7e07419 Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Wed, 11 Nov 2026 16:54:45 -0500 Subject: [PATCH 3/3] Collect segment owners under the deletion lock on the single route The single DELETE /segments/:segmentId route passed awaitPostCommit=false, which sent it down the path that resolves affected flags and experiments before the transaction takes the target lock, so an owner attached in that gap was never recomputed. Drop awaitPostCommit (SegmentService.deleteSegment is back to its dev form): the single route now supplies its locking transaction like the batch route, so owners are collected after the lock and recomputation is awaited after commit. Adds an integration test that attaches an archived experiment just before the lock and checks its precomputed membership is updated. Co-Authored-By: Claude Opus 5.5 --- packages/backend/CLAUDE.md | 8 +++--- .../src/api/controllers/SegmentController.ts | 3 +-- .../src/api/services/SegmentService.ts | 9 ++----- .../test/integration/BatchDelete/index.ts | 26 +++++++++++++++++++ 4 files changed, 33 insertions(+), 13 deletions(-) diff --git a/packages/backend/CLAUDE.md b/packages/backend/CLAUDE.md index ef986b4d88..b380cf29cb 100644 --- a/packages/backend/CLAUDE.md +++ b/packages/backend/CLAUDE.md @@ -114,10 +114,10 @@ Segment inclusion/exclusion for **feature flags** is precomputed and stored flat | Private list added to a shared segment | `SegmentService.addList` → `scheduleRecomputeForSegment` | | Private list removed from a shared segment | `SegmentService.deleteList` → `scheduleRecomputeForSegment` | | Segment members/structure updated | `SegmentService.addSegmentDataWithPipeline` → `scheduleRecomputeForSegment` | -| Segment deleted entirely | `SegmentService.deleteSegment` → `withRecompute` for single deletion; batch deletion collects affected flag IDs after the target lock and **before** deletion, then awaits recomputation **after** commit | +| Segment deleted entirely | `SegmentService.deleteSegment` → the single and batch deletion routes collect affected flag IDs after the target lock and **before** deletion, then await recomputation **after** commit; internal callers (`deleteList`) use `withRecompute` | | Server startup | `app.ts` → `backfillMissingFlags` — backfills any flag with no row | -Ordinary write paths use **fire-and-forget** recomputation. The `import*` paths await `recomputeForFlag` so "import complete" means the rows are ready. Batch segment deletion also awaits post-commit recomputation for both flags and experiments before processing the next segment. `SegmentService.deleteSegment` selects this awaited branch when supplied with the batch transaction executor; ordinary callers retain background recomputation. +Ordinary write paths use **fire-and-forget** recomputation. The `import*` paths await `recomputeForFlag` so "import complete" means the rows are ready. Segment deletion through the single and batch routes also awaits post-commit recomputation for both flags and experiments (a batch finishes it before processing the next segment). `SegmentService.deleteSegment` selects this awaited branch when supplied with a deletion transaction executor; internal callers retain background recomputation. ### Key invariant @@ -152,10 +152,10 @@ Experiment join tables (`ExperimentSegmentInclusion` / `ExperimentSegmentExclusi | Experiment lists imported | `ExperimentService.importExperimentLists` → `await recomputeForExperiment` after the import transaction commits | | Experiment context changed (deletes all its lists) | `ExperimentService.updateExperimentInDB` → `scheduleRecomputeForExperiments` after commit (recomputes to empty; `deleteAllListsFromExperiment` deletes the private segments directly, so the precomputed row would otherwise keep stale IDs) | | Shared segment members/structure changed | `SegmentService.addList` / `deleteList` / `addSegmentDataWithPipeline` → `scheduleRecomputeForSegment` for **both** the flag and experiment services | -| Segment deleted entirely | `SegmentService.deleteSegment` → collects affected experiment IDs **before** deletion and recomputes **after** commit; single deletion schedules background work, while batch deletion collects owners after the target lock and awaits recomputation | +| Segment deleted entirely | `SegmentService.deleteSegment` → collects affected experiment IDs **before** deletion and recomputes **after** commit; the single and batch deletion routes collect owners after the target lock and await recomputation, while internal callers schedule background work | | Server startup | `app.ts` → `backfillExperimentPrecomputedSegments` (guarded by `.catch` — a missing table never crashes startup) | -Same invariant as feature flags: recompute **after** the change commits; for deletes, collect affected experiment IDs **before**. Write-path recomputes are fire-and-forget except the `create` / import paths and batch segment deletion, which await recomputation. A batch finishes each segment's post-commit updates before deleting the next segment. +Same invariant as feature flags: recompute **after** the change commits; for deletes, collect affected experiment IDs **before**. Write-path recomputes are fire-and-forget except the `create` / import paths and segment deletion through the single and batch deletion routes, which await recomputation. A batch finishes each segment's post-commit updates before deleting the next segment. ### INCLUDE_ALL semantics (resolved) diff --git a/packages/backend/src/api/controllers/SegmentController.ts b/packages/backend/src/api/controllers/SegmentController.ts index 0babea13c3..095f9b8ee8 100644 --- a/packages/backend/src/api/controllers/SegmentController.ts +++ b/packages/backend/src/api/controllers/SegmentController.ts @@ -685,8 +685,7 @@ export class SegmentController { return this.segmentService.deleteSegment( segmentId, request.logger, - this.deletionPolicyService.singleDeletionTransaction('segments', segmentId), - false + this.deletionPolicyService.singleDeletionTransaction('segments', segmentId) ); } diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index 3f828bf60e..715e15f24d 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -536,21 +536,16 @@ export class SegmentService { return this.addSegmentDataWithPipeline(segment, logger, transactionalEntityManager, skipScheduleRecompute); } - /** - * `awaitPostCommit` (the batch path) collects owners inside the supplied transaction after the target lock and - * awaits their recomputation after commit; otherwise recomputation runs in the background. - */ public async deleteSegment( id: string, logger: UpgradeLogger, - executeTransaction?: DeletionTransaction, - awaitPostCommit = executeTransaction !== undefined + executeTransaction?: DeletionTransaction ): Promise { logger.info({ message: `Delete segment by id. segmentId: ${id}` }); const transaction: DeletionTransaction = executeTransaction || ((work) => this.dataSource.transaction(work)); - if (awaitPostCommit) { + if (executeTransaction) { let affectedFlagIds: string[]; let affectedExperimentIds: string[]; const deletedSegment = await transaction(async (transactionalEntityManager) => { diff --git a/packages/backend/test/integration/BatchDelete/index.ts b/packages/backend/test/integration/BatchDelete/index.ts index 1d84ac55c3..56d8bf26a5 100644 --- a/packages/backend/test/integration/BatchDelete/index.ts +++ b/packages/backend/test/integration/BatchDelete/index.ts @@ -500,6 +500,32 @@ export function registerBatchDeleteTests(connections: () => [DataSource, DataSou } ); + test('single segment deletion recomputes an owner attached before its deletion lock', async () => { + const [segment] = await create('segments'); + const [experiment] = await create('experiments'); + // An archived owner does not make the segment Used, so the deletion is allowed and must update this owner. + await db.getRepository(Experiment).update(experiment.id, { state: EXPERIMENT_STATE.ARCHIVED }); + await db.getRepository(IndividualForSegment).insert({ segmentId: segment.id, userId: 'deleted-member' }); + const findForDeletion = DeletionRepository.prototype.findForDeletion; + jest + .spyOn(DeletionRepository.prototype, 'findForDeletion') + .mockImplementationOnce(async function (this: DeletionRepository, ...args) { + // Commit a list edit on another connection just before the deletion acquires its target lock. + await ownedList('experiments', experiment.id, segment.id, writer); + await Container.get(ExperimentPrecomputedSegmentService).recomputeForExperiment( + experiment.id, + new UpgradeLogger() + ); + return findForDeletion.apply(this, args); + }); + + await request(app).delete(`/api/segments/${segment.id}`).expect(200); + expect(await db.getRepository(Segment).countBy({ id: segment.id })).toBe(0); + expect( + await db.getRepository(ExperimentPrecomputedSegment).findOneBy({ experimentId: experiment.id }) + ).toMatchObject({ inclusionIds: ['batch-member'], exclusionIds: [] }); + }); + test('keeps a committed segment deletion when post-commit cache invalidation fails', async () => { const rows = await create('segments', 2); const cache = Container.get(CacheService);