Repository navigation
test(permissions): a member limited to owned records deletes only what they or their team own - #4969
Merged
Merged
Conversation
…t they or their team own A delete by a member whose grants are scoped to Owned keeps to the records they or one of their teams own, as a list and an update do. Two suites hold that: - OwnedRecordsDeleteEveryKind (Common Test): for every kind the Owned scope narrows - the kinds with owners of their own, the records that belong to one, and the records read through one - the query a delete runs with reaches the owned records only, and the same grants over the whole project reach every record. - OwnedRecordsDeletePostgres (Postgres Schema Drift): through the monitor routes, the real user middleware and MonitorService's hooks on a migrated Postgres - by id (both routes) and by query, for every grant a member may hold scoped to Owned (a custom role's included), mixed scopes, owner rows that cannot be added to or taken from a monitor the member may not read, a monitor's status timeline and tests, a status page's announcements and templates, the requests of a viewer, another project, no session, across projects and an API key, and every other kind with owners of its own. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hey hold, formatted Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
…y the ones they own A grant scoped to Owned beside a broader read - Monitor Viewer over the whole project, the Viewer role, Read Monitor, or a read limited to a label - is a common 'see everything, manage your own' shape. The owned-records delete suite now holds that shape too: through the routes and by query, a member who may read a monitor they do not own is still refused its delete, and deletes only the monitors they or their team own. Covered for a custom role's copies as well. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this holds
A team permission scoped to Owned "only applies to resources where this team, or the user acting, is listed as an owner" (Permissions docs, Scope). A delete has to keep to that exactly as a list and an update do: a member whose monitor permissions are all scoped to Owned deletes the monitors they or one of their teams own, and no other.
Master already does this. Since #4526 a delete is narrowed by the same record rule as a read and an update (
BasePermission.addRecordScopeToQuery, asked byDeletePermission.checkDeletePermission). Since #4552 nobody can add themselves or their team as an owner of a record they may not read. But no test held a delete of an owned-scope record end to end, by id, through the routes. This PR adds those tests. Tests only: no product change.Tests added
OwnedRecordsDeletePostgres(100 tests, new step in Postgres Schema Drift)Every request goes through the route, the real user middleware (it reads the member's teams, permission rows, custom role copies and API keys from the tables), the real
MonitorServicewith its delete hooks, and the real permission pipeline, query serializer and TypeORM, on a migrated Postgres. Only the session token, caches, SSO lookups, audit trail, workflow and realtime triggers, and Valkey locks are stood in for.CustomRoleSync). For each one:POST /monitor/:id/delete-itemand byDELETE /monitor/:id;404 Monitor not found.on both routes, and stays with its owner rows;MonitorService.deleteByandhardDeleteBywith the member's own props remove only the owned monitors.404, because a delete needs the read;/monitor-owner-user) or their team (/monitor-owner-team) as an owner of a monitor they may not read is refused, and no owner row is written. Moving an owner row of their own monitor onto another monitor is refused. Removing another team's owners of a monitor they may not read is refused. The monitor still can't be deleted.404;is-multi-tenant-query) is refused even for an owned monitor;OwnerTableRegistrykinds: incidents, alerts, status pages, dashboards, services, hosts, clusters, databases, queues, workflows…), throughBaseAPIand a plain service: by id, owned records go and others are answered404and stay; by query, only owned records go. Rows are filled from the migrated schema's own column list, so a new kind is swept without editing the suite.OwnedRecordsDeleteEveryKind(294 tests, Common Test)For every kind the Owned scope narrows, it checks the query
DeletePermission.checkDeletePermissionruns with, for a member holding every scope-limitable delete and read permission of that kind (and the read of the record it is read through), all scoped to Owned. The query reaches the owned record and not another. The same grants over the whole project reach every record. A kind that no scope-limitable permission deletes is refused. The kinds swept:@OwnedThroughrecords, through the record they belong to;@CanAccessIfCanReadOnrecords read through a kind with owners, through that record (its owner rows among them).The two kinds read through a list of status pages are pinned exactly, with their Postgres test above. New kinds join the sweep automatically.
Proof the tests catch a regression (mutation checks, each restored)
DeletePermissionnarrowed by labels only): 74/100 Postgres tests and 131/294 guard tests fail.OwnedScopePermission.addOwnedRecordsToQuery): 83/100 Postgres tests (lists and hard deletes included) and 74/294 guard tests fail.CreatePermission.checkParentPermission): exactly the two "becoming an owner first" tests fail.What changes for existing customers
Nothing. Tests and a CI step only.
What I ran
BILLING_ENABLED=trueand the plan definitions fromtest-setup.sh).ee/hidden.npm run compile-testsin packages/Common (all three programs).npx jest Guardin packages/Common (132 suites).Tests/Ops(47 suites, includingPostgresSuitesRunInCi).No failures seen on master in what I ran.
Review
A second pass took the branch and tried to get a monitor delete past the Owned scope some other way, on a freshly migrated Postgres through the real routes.
What I tried, and what holds on master:
POST /monitor/:id/delete-item, byDELETE /monitor/:id, and bydeleteByquery. A monitor they may read but not own is refused ("You do not have permission to delete this monitor."). Covered for a custom role's copies too. These are the tests added in this pass (10), and both mutation checks catch them: with the old labels-only delete rule, and with the owner narrowing made a no-op, all ten fail.Follow-up (not a change here, pre-existing on master): who may add or remove a resource's owners is gated on reading the resource, not yet narrowed by the same Owned scope. So a member whose management grant is Owned-scoped but who can read more monitors can still name themselves an owner of a monitor they do not own. This is a change to owner-management scope across every kind that carries owners - a product decision of its own - so it is tracked as a separate hardening task, not folded into this tests-only change.
All 110 Postgres tests and 294 guard tests pass before and after merging master (billing on and off for the guard);
compile-testsis clean;npx jest Guardis 132 suites green; prettier, eslint and the ReDoS check are clean.🤖 Generated with Claude Code