diff --git a/packages/backend/CLAUDE.md b/packages/backend/CLAUDE.md index 305ee2c201..b380cf29cb 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. @@ -105,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 @@ -143,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/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..095f9b8ee8 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,25 @@ 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) + ); } /** 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/test/integration/BatchDelete/index.ts b/packages/backend/test/integration/BatchDelete/index.ts index 24b2d1680d..56d8bf26a5 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,36 +458,69 @@ export function registerBatchDeleteTests(connections: () => [DataSource, DataSou } }); - test('recomputes owners attached before the segment deletion lock is acquired', async () => { + 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('single segment deletion recomputes an owner attached before its deletion lock', async () => { const [segment] = await create('segments'); - const [flag] = await create('flags'); 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 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); + await Container.get(ExperimentPrecomputedSegmentService).recomputeForExperiment( + experiment.id, + new UpgradeLogger() + ); 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' }]); + 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(FeatureFlagPrecomputedSegment).findOneBy({ featureFlagId: flag.id })).toMatchObject( - { inclusionIds: ['batch-member'], exclusionIds: [] } - ); expect( await db.getRepository(ExperimentPrecomputedSegment).findOneBy({ experimentId: experiment.id }) ).toMatchObject({ inclusionIds: ['batch-member'], exclusionIds: [] }); @@ -545,75 +574,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 +726,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..b7ca7d2b28 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 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,