decentraland / decentraland/worlds-content-server

fix: store permission scope explicitly instead of inferring it from an empty parcel list

Open
#520 0 comments 0 reactions 0 assignees View on GitHub
2-medium bug
Dominant language
TypeScript
Stars
0
Forks
7
Avg merge
1d 9h
Merged PRs (30d)
5

Description

## Problem

Parcel-scoped permissions encode their scope as an absence rather than as a stored property: a
permission with no parcels recorded against it is treated as having no parcel restrictions. The two
states are indistinguishable in the database, so the API has no way to represent "restricted to no
parcels" — a permission that exists is either unrestricted or restricted to a non-empty set.

The consequence is that `DELETE /world/:world_name/permissions/:permission_name/address/:address/parcels`
can widen the permission it is meant to narrow. Removing the last parcel from an address's permission
does not reduce that address to no access; it returns the permission to its unrestricted state, which
passes every scope check in the service — scene deployment, undeploy, and world settings updates. The
request returns `204` and emits no event, so the owner gets no signal that anything was widened.

This is reachable through ordinary owner actions, not only by deliberately emptying a list. There is
no atomic way to move a collaborator from one parcel set to another (see #443), so a remove-then-add
sequence passes through the unrestricted state, and stays there if the add is deferred or fails. The
same applies to an owner narrowing a collaborator's scope over time until it reaches zero, or
emptying the list intending to withdraw access — withdrawing access actually requires the separate
`DELETE /world/:world_name/permissions/:permission_name/:address`, which removes the record.

## Expected Behaviour

Removing parcels from a permission must never leave the address with more access than it had. Which
direction we take is open:

1. **Revoke on empty** — if a removal would empty the parcel set, delete the permission record and
emit the permission-revoked event, mirroring the existing revoke route. Probably closest to what
an owner emptying a grant intends.
2. **Reject on empty** — return `400` and point the caller at the revoke route, so that neither
outcome is a silent state change.
3. **Store the scope explicitly** — persist the scope on the permission record (a migration adding a
boolean, backfilled from the current parcel counts) so that "no parcels recorded" can no longer
mean "no restrictions". This removes the same inference from the permission reads and the address
queries that share it, and composes with either 1 or 2.

Whichever we choose, the integration coverage for the parcels routes currently asserts the present
behaviour and will need updating, and the OpenAPI description of the parcels routes should state what
an empty parcel set means.

## Related

Found while reviewing, same root cause or same blind spot — worth handling together:

- The parcel-adding path will also create a permission record with no parcels when handed an empty
list, producing the same unrestricted state. Request validation prevents this over HTTP; internal
callers are unguarded.
- No event is emitted when the parcel scope of an *existing* permission changes. The granted event
fires only when the permission record is newly created, and the removal path never notifies at all,
so changes to a collaborator's footprint are unobservable downstream.
- Coordinate normalization is inconsistent between the parcel write paths and the address-query read
path, so non-canonical coordinates can fail to match stored parcels on that query.

Internal notes with the full code-path detail are available on request.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.