Skip to content

[fix][populator] keep a template while other apps still have environments from it - #7890

Open
ar2rsawseen wants to merge 1 commit into
masterfrom
fix/populator-environment-delete-scope
Open

[fix][populator] keep a template while other apps still have environments from it#7890
ar2rsawseen wants to merge 1 commit into
masterfrom
fix/populator-environment-delete-scope

Conversation

@ar2rsawseen

Copy link
Copy Markdown
Member

Removing a populator template deleted every app's environment records for it, selecting by templateId alone, while deleting environment users only for the app making the request:

// users: scoped to the requesting app
{_id: {$regex: new RegExp("^" + params.qstring.app_id + "_" + template_id)}}

// records: every app
{templateId: ob.params.qstring.template_id}

Two deletes in the same code path, disagreeing about scope. Another app was left holding generated users with no record to manage them by.

That app could not recover from it either. An environment is only ever reachable through its template — the templates table marks which templates have environments, and the environment view is routed by template id:

if (self.environments.filter(x => x.templateId === item._id).length) { item.hasEnvironment = true; }
self.filterByEnvironmentOptions = envs.filter(x => x.templateId === self.templateId)

So once the shared template is gone there is no row to match and no route to follow. Those users cannot be listed, reused or removed.

Why keep the template rather than cascade

Deleting the other apps' rows would tidy the database at the cost of discarding another app's records at the request of someone with no access to that app. So the remove path now looks for environments belonging to other apps first and refuses, saying how many. Each app clears its own through /o/populator/environment/remove, which already exists, is already app-scoped, and is already wired into the environment view — so there is no new UI and no new concept.

The environment delete that follows is now scoped to the requesting app too, matching every other query against that collection (environment/check, environment/list, environment/remove, the app delete/reset/clear hooks and the age-based cleanup job were all already scoped — this line was the only exception). The check above it means no other app has one at that point; if one appears in between, leaving a row for a cleanup job to collect beats deleting someone else's.

Behaviour change

A template delete that would previously have destroyed other apps' environment records now returns 400 with the count. Deleting a template nobody else has environments for is unchanged.

Templates stay shared

Any member with the populator feature on any app can still list, use, edit and remove templates. That is deliberate — a template describes how to generate synthetic data and holds no collected data — so the intent is now recorded in a comment beside the code, since an unscoped query is otherwise indistinguishable from a missing check.

Verification

plugins/populator/tests.js is supertest-based against a live instance and has no fixture for a second app, so this is not covered there. Instead the function was lifted and run against stubbed collections to exercise both paths: with another app's environment present it returns 400, names the count, and issues no template or environment delete at all; with none present the template is removed, the response is 200, and the environment delete carries both templateId and appId. node --check and npx eslint clean.

…ents from it

Removing a template deleted every app's environment records for it, by templateId
alone, while deleting environment users only for the app making the request. The
two deletes in the same code path disagreed about scope, so another app was left
holding generated users with no record to manage them by.

That other app could not recover from it either. An environment is only ever
reachable through its template: the templates table marks which templates have
environments, and the environment view is routed by template id. Once the shared
template is gone there is no row to match and no route to follow, so those users
could not be listed, reused or removed.

Deleting them here instead would be worse, since it discards another app's
records at the request of someone with no access to that app. So the template is
kept: the remove path first looks for environments belonging to other apps and
refuses with a message saying how many, and each app clears its own through
/o/populator/environment/remove, which already exists and is already app-scoped.

The environment delete that follows is now scoped to the requesting app as well,
matching every other query against that collection. The check above it means no
other app has one at that point; if one appears in between, leaving a row for a
cleanup job to collect beats deleting someone else's.

Templates themselves stay shared, which is deliberate: any member with the
populator feature on any app can list, use, edit and remove them. That intent is
now written down next to the code, since an unscoped query is otherwise
indistinguishable from a missing check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant