Enforce deletion permissions and state rules across single and batch APIs by bcb37 · Pull Request #3421 · CarnegieLearningWeb/UpGrade

bcb37 · 2026-11-11T20:30:45Z

Resolves #3326

Changes

Single and batch deletion of experiments, feature flags and segments now go through one backend policy. Before, these rules were only enforced in the UI.

Role: a caller whose role can't delete that entity type gets 403 for the whole request (single or batch), before any target is locked. Roles come from a shared ROLE_PERMISSIONS table, moved from the frontend's auth.service.ts into upgrade_types; the frontend now reads it too, so the two tables can't drift.
State and usage: checked by DeletionPolicyService on fields read under the target's deletion row lock, inside the deletion transaction. There is no earlier unlocked read.
- Experiments: deletable only in Draft, Inactive, Completed (including stored Cancelled) and Archived.
- Feature flags: not deletable while Enabled.
- Segments: not deletable while Used, and global-exclude segments are never deletable.
Single routes: a refusal returns 409 with type: DELETION_NOT_ALLOWED. Missing targets keep their existing responses. DELETE /segments/:segmentId now receives @CurrentUser().
Batch routes: a refused item is reported with a new refused outcome and a reason code (state_not_deletable, segment_in_use or global_exclude_protected), and the batch continues, as it does for not_found. A refused item writes no audit log and triggers no cache reset or recompute.
Used segments: DeletionRepository.isSegmentInUse() checks one segment with a single query and mirrors the Used status from SegmentService.getSegmentStatus(). An integration test pins the two together across every kind of usage. (getSegmentStatus() reads every list, so it isn't reused here; the batch path deliberately avoids those reads.)
Frontend: accepts the refused outcome; refused items stay selected, and the snackbar says they were skipped. The UI already blocks ineligible selections, so refused only appears when the list is out of date (for example, an experiment started after the page loaded) or for callers outside the UI.
Internal deletions (rolling back a failed create, removing a private list) still call the entity services without the policy, deliberately.

Behavior changes for existing callers

Deleting through the API now requires an allowed role. With auth off, the request runs as the seeded system user, which is ADMIN. If that user doesn't exist, the request is refused.
A segment on a flag's list, or on a non-archived experiment's list, can no longer be deleted through these routes.
Load tests (upgrade-automated-tests): they need an account with an allowed role, and test items that can be deleted (for example, stop experiments and disable flags) before cleanup. That repo isn't changed here.

Tests

Backend unit: new DeletionPolicyService tests (role × entity, every experiment state, flag status, segment rules, single-delete transaction), plus refused/role cases in BatchDeleteService.
Integration: inverted the assertions that every role and state could delete. Added coverage for refusals on both routes, targets that become ineligible just before the lock, a mixed [deletable, running, missing, deletable] batch, nested and archived-owner segment usage, and the isSegmentInUse ↔ getSegmentStatus agreement.
Frontend: spec for a batch result with a refused item.

🤖 Generated with Claude Code

…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 <noreply@anthropic.com>

Copilot

🟡 Changes recommended

Concurrency gaps can allow deletion of a segment that becomes used before commit, while owner discovery outside the transaction can leave archived experiment membership stale.

2 open issues

What changed in this PR

Moves deletion permissions and eligibility checks from UI-only restrictions into shared backend enforcement for experiments, feature flags, and segments, addressing #3326.

Changes:

Shares role permissions between frontend and backend, returning 403 for unauthorized deletion requests.
Adds transactional state and usage checks, with 409 responses for single refusals and per-item refused results for batches.
Updates frontend refusal handling, API documentation, and regression coverage.

File	Description
`packages/types/src/Permissions/index.ts`	Defines shared role permissions and permission checks.
`packages/types/src/index.ts`	Exports permission and deletion-policy definitions.
`packages/types/src/Experiment/enums.ts`	Adds the deletion-refusal error type.
`packages/types/src/BatchActions/index.ts`	Adds refusal outcomes, reasons, and deletable states.
`packages/frontend/projects/upgrade/src/assets/i18n/en.json`	Adds skipped-item notification messages.
`packages/frontend/projects/upgrade/src/app/core/batch-actions/batch-actions.integration.spec.ts`	Tests refused selection retention and warnings.
`packages/frontend/projects/upgrade/src/app/core/batch-actions/batch-actions.helpers.ts`	Handles refused results and shared state eligibility.
`packages/frontend/projects/upgrade/src/app/core/auth/store/auth.models.ts`	Reuses the shared permission type.
`packages/frontend/projects/upgrade/src/app/core/auth/auth.service.ts`	Reads permissions from the shared table.
`packages/backend/test/unit/services/DeletionPolicyService.test.ts`	Tests role, state, usage, and transaction rules.
`packages/backend/test/unit/services/BatchDeleteService.test.ts`	Tests batch refusals and request-level authorization.
`packages/backend/test/unit/controllers/SegmentController.test.ts`	Adds the deletion-policy mock.
`packages/backend/test/unit/controllers/FeatureFlagController.test.ts`	Adds the deletion-policy mock.
`packages/backend/test/unit/controllers/ExperimentControllerAdaptiveConfig.test.ts`	Updates controller construction for the new dependency.
`packages/backend/test/unit/controllers/ExperimentController.test.ts`	Adds the deletion-policy mock.
`packages/backend/test/integration/BatchDelete/index.ts`	Covers enforcement, mixed batches, usage, and cleanup.
`packages/backend/src/api/services/SegmentService.ts`	Makes post-commit waiting independently configurable.
`packages/backend/src/api/services/PermissionService.ts`	Enforces shared permissions with 403 errors.
`packages/backend/src/api/services/DeletionPolicyService.ts`	Centralizes deletion eligibility and single-route transactions.
`packages/backend/src/api/services/BatchDeleteService.ts`	Checks permissions and continues past refused items.
`packages/backend/src/api/repositories/DeletionRepository.ts`	Reads locked policy fields and queries segment usage.
`packages/backend/src/api/middlewares/ErrorHandlerMiddleware.ts`	Preserves deletion-refusal error responses.
`packages/backend/src/api/controllers/SegmentController.ts`	Adds caller-role checks and policy-controlled deletion.
`packages/backend/src/api/controllers/FeatureFlagController.ts`	Applies the deletion policy and documents refusals.
`packages/backend/src/api/controllers/ExperimentController.ts`	Applies the deletion policy and documents refusals.
`packages/backend/CLAUDE.md`	Documents enforcement rules and internal exemptions.

_{Review effort: Balanced}

danoswaltCL · 2026-11-11T21:16:04Z

   *   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.


"Execution failures stop the remaining items."

I guess this is pre-existing and i didn't catch it before, but I don't think one error should stop the execution, other items could delete successfully and there's no reason i can think of to stop that from happening, is there? It can return the non-delete reason and the UI can show the reason for items that didn't delete.

It's unlikely that this will ever happen from a UI interaction, since batch deletes are disabled as long as at least one item isn't deletable. But, yeah, I figured for an API call (like one made by load tests) it makes sense to delete what's deletable and report what isn't.

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 <noreply@anthropic.com>

Copilot

🟢 Approval recommended

No active review issues remain for the change centralizing deletion permissions and eligibility checks in the backend.

No open issues 🎉

2 resolved since last review

_{Review effort: Balanced}

bcb37 and others added 2 commits November 11, 2026 11:21

reduce enum label character count

854b21d

bcb37 requested a balanced review from Copilot and removed request for Copilot November 11, 2026 20:30

Copilot started reviewing on behalf of bcb37 November 11, 2026 20:57

View session

Copilot AI reviewed Nov 11, 2026

View reviewed changes

Comment thread packages/backend/src/api/repositories/DeletionRepository.ts Resolved

Comment thread packages/backend/src/api/controllers/SegmentController.ts Resolved

danoswaltCL reviewed Nov 11, 2026

View reviewed changes

bcb37 requested a balanced review from Copilot November 11, 2026 21:56

Copilot started reviewing on behalf of bcb37 November 11, 2026 21:57

View session

Copilot AI reviewed Nov 11, 2026

View reviewed changes

bcb37 requested a review from danoswaltCL November 11, 2026 22:01

Navigation Menu

Repository navigation

Enforce deletion permissions and state rules across single and batch APIs - #3421

Enforce deletion permissions and state rules across single and batch APIs#3421
bcb37 wants to merge 3 commits into
devfrom
feature/3326-enforce-deletion-policy

bcb37 commented Nov 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

danoswaltCL Nov 11, 2026

Uh oh!

bcb37 Nov 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

bcb37 commented Nov 11, 2026

Changes

Behavior changes for existing callers

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

danoswaltCL Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

bcb37 Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants