🐛 Refuse dead teams in resolve-team-id (PR #12189 review)

Second review round on PR #12189 (pabloalba).

The file re-check added in 87ca7aca53 stops at the file row, but the
organization-owner fallback grants read-only access to non-member org
owners via Nitrate over HTTP, which knows nothing about Penpot
soft-deletion. resolve-team-id resolved the team from unfiltered SQL,
so an expelled org owner kept the file subscription open for an
indefinitely soft-deleted file, project or team. Now both SQL lookups
filter deleted_at, and resolve-team-id checks the team row itself
(db/get*, which filters soft-deleted rows) for every caller — teams,
projects and files alike.

Also from the same review:

- file-readable? now uses the same sql:get-minimal-file as the
  subscribe gate (non-throwing via throw-if-not-exists false), and
  team-readable? projects id and deleted-at, so db/get* can filter.
- The test suite extracts the thrice-duplicated raw delete into
  delete-team-membership!; a sibling of the fail-open pin now redefs
  the existence probe to throw; memory and docstring aligned with the
  code.
- docs/technical-guide/developer/architecture/backend.md attributed
  the websocket to notifications.clj; it now names app.http.websocket
  as the owner and notifications.clj as the revocation publisher.
- mem:backend/auth-permissions-product-domains documents the fallback
  mechanism and the resolve-team-id contract.

Closes #12155

AI-assisted-by: inkling-2-small-free
This commit is contained in:
Andrey Antukh 2026-11-10 12:38:45 +00:00
parent 87ca7aca53
commit 225eb98de2
6 changed files with 116 additions and 33 deletions

View File

@ -24,6 +24,7 @@
- `app.rpc.permissions` provides predicate/check factories. Failed permission checks intentionally raise `:not-found` / `:object-not-found`, not an authorization-specific error, to avoid leaking object existence.
- Team role flags are normalized as owner > admin > editor > viewer. Owner/admin imply edit; any membership row implies read.
- File/project role rows only count with a live team membership: the file/project permission queries require a matching `team_profile_rel` row, and leaving or removing a member deletes their team-scoped `file`/`project`/`team-project-profile-rel` rows (GHSA-v9r9-h77c-55m2).
- The organization-owner fallback (`get-organization-owner-permissions` via Nitrate over HTTP) grants read-only access for a non-member organization owner. It resolves the owning team through `resolve-team-id`, which refuses soft-deleted file/project/team: the file and project lookups filter `deleted_at`, and a final `db/get*` check covers the team row itself — `nil` means no access. `db/get*` only filters soft-deleted rows when `deleted_at` is in the projection. The re-checks of open websocket subscriptions rely on this (`mem:backend/websocket-subscriptions`).
- File/project/comment checks are implemented in the owning command namespaces, often via helpers imported from `files`, `teams`, or `projects`; do not bypass those helpers with direct DB lookups unless preserving their not-found semantics.
- Comment permission includes both logged-in state and the file/team comment policy. Shared viewer paths may pass `share-id`; preserve that path when changing comment queries.

View File

@ -11,8 +11,8 @@
A subscription is authorized once, at `:subscribe-file`/`:subscribe-team`, and that decision used to be trusted for the whole connection life (GHSA-m53j-2766-6jqw, medium). Two independent mechanisms now bound it:
- **Announcement (primary).** A mutation that can change access publishes on `app.rpc.notifications/internal-revocation-topic` (`notify-permissions-changed` for one profile, `notify-team-permissions-changed` for one team id — the watcher resolves the members itself, so a large team is one event, not N). The `::revocation-watcher` Integrant component in `app.http.websocket` subscribes and calls `revalidate-profile-subscriptions`, which looks the profile up in `:by-profile` and re-checks every subscription with `file-readable?`/`team-readable?`: `db/get*` existence (non-throwing, soft-deleted rows excluded for `:file` and `:team`) AND the `has-read-permissions?` predicates — and `sql:file-permissions` additionally filters `deleted_at` on file, project and team. The existence gate mirrors the `:subscribe-file` gates; it closes the organization-owner fallback, which grants read-only access without looking at `deleted_at` (expelled org owner, soft-deleted resource). The event is only the trigger; the fresh check is the decision, which is what keeps a downgrade to `viewer` (still readable) and a non-member organization owner on a live resource from being mistaken for a revocation. Both publishers defer through `db/after-commit`, because the watcher checks on another connection and would otherwise see the pre-commit state. `:subscribe-team` deliberately has no existence gate of its own; only the re-checks add one, matching the file side (user decision, PR #12189 review).
- **Interval (safety net).** `start-relay` runs the relay as a `go-loop` that waits on the subscription channel and a one-shot tick channel together via `sp/alts!` with `:priority true` on the tick: when both hold something, the tick wins, so a steady stream of messages can never push the re-check away. The cadence is `subscription-revalidation-interval` (default `default-revalidation-interval`, 5 min, a code constant in `app.http.websocket` and **not** in the `config.clj` default map, same rule as session lifetimes). The deadline is fixed per cadence: a single tick channel covers a whole cadence and message turns reuse it (minting a fresh timeout per turn would both churn timers and starve under load exactly like an in-loop timer does); a new one is minted only after a tick fires. It covers what no announcement can: Nitrate transferring organization ownership (an HTTP call to another service) and rows deleted straight over SQL. Closing the channel ends the loop either way, because a take on a closed channel is always ready with nil — every way a subscription can end closes it, so a finished relay never keeps polling. Cost is two or three permission queries per subscription per interval, plus one Nitrate call per team per interval when the `:admin-console` flag is on (cached 30 s); one `timeout-chan` per subscription per cadence on promesa's default scheduler (one or two threads shared with every other scheduled task) — do not shorten the interval without sizing that. A tick abandoned at teardown (the subscription closed while it was pending) still fires its scheduled close harmlessly. Two places fail open, deliberately: `still-authorized?` (a transient DB error logs and keeps the subscription; `InterruptedException` is rethrown) and the `catch Throwable` in `watch-revocations`.
- **Announcement (primary).** A mutation that can change access publishes on `app.rpc.notifications/internal-revocation-topic` (`notify-permissions-changed` for one profile, `notify-team-permissions-changed` for one team id — the watcher resolves the members itself, so a large team is one event, not N). The `::revocation-watcher` Integrant component in `app.http.websocket` subscribes and calls `revalidate-profile-subscriptions`, which looks the profile up in `:by-profile` and re-checks every subscription with `file-readable?`/`team-readable?`: existence (the file gate is the same `sql:get-minimal-file` as the `:subscribe-file` gate, made non-throwing via `{:db/throw-if-not-exists false}`; the team gate is `db/get*` with `::sql/columns [:id :deleted-at]` — `deleted_at` must be in the projection or the soft-delete filter is a no-op) AND the `has-read-permissions?` predicates — and `sql:file-permissions` additionally filters `deleted_at` on file, project and team, as do `sql:get-team-id-for-file`/`-project` plus a final team-row check in `resolve-team-id`, so the organization-owner fallback grants nothing through soft-deleted file/project/team. The existence gate closes that fallback (expelled org owner, deleted resource). The event is only the trigger; the fresh check is the decision, which is what keeps a downgrade to `viewer` (still readable) and a non-member organization owner on a live resource from being mistaken for a revocation. Both publishers defer through `db/after-commit`, because the watcher checks on another connection and would otherwise see the pre-commit state. `:subscribe-team` deliberately has no existence gate of its own; only the re-checks add one, matching the file side (user decision, PR #12189 review).
- **Interval (safety net).** `start-relay` runs the relay as a `go-loop` that waits on the subscription channel and a one-shot tick channel together via `sp/alts!` with `:priority true` on the tick: when both hold something, the tick wins, so a steady stream of messages can never push the re-check away. The cadence is `subscription-revalidation-interval` (default `default-revalidation-interval`, 5 min, a code constant in `app.http.websocket` and **not** in the `config.clj` default map, same rule as session lifetimes). The deadline is fixed per cadence: a single tick channel covers a whole cadence and message turns reuse it (minting a fresh timeout per turn would both churn timers and starve under load exactly like an in-loop timer does); a new one is minted only after a tick fires. It covers what no announcement can: Nitrate transferring organization ownership (an HTTP call to another service) and rows deleted straight over SQL. Closing the channel ends the loop either way, because a take on a closed channel is always ready with nil — every way a subscription can end closes it, so a finished relay never keeps polling. Cost is three or four queries per subscription per interval (existence row + predicates), plus one Nitrate call per team per interval when the `:admin-console` flag is on (cached 30 s); one `timeout-chan` per subscription per cadence on promesa's default scheduler (one or two threads shared with every other scheduled task) — do not shorten the interval without sizing that. A tick abandoned at teardown (the subscription closed while it was pending) still fires its scheduled close harmlessly. Two places fail open, deliberately: `still-authorized?` (a transient DB error logs and keeps the subscription; `InterruptedException` is rethrown) and the `catch Throwable` in `watch-revocations`.
Which mutations announce: `delete-team-member`, `leave-team`, `update-team-member-role` (member only), `delete-team`, `delete-file`, `delete-project`, `move-files`, `move-project` (one team event each). Anything not listed here is covered only by the interval. `delete-share-link` deliberately does not: a share-link visitor never passes the websocket subscribe check, because `check-read-permissions!` is called without a `share-id` and that arity only considers team membership.
@ -32,7 +32,7 @@ Which mutations announce: `delete-team-member`, `leave-team`, `update-team-membe
## Tests
`backend/test/backend_tests/http_websocket_test.clj` drives the real Redis-backed msgbus, so the revocation tests are end-to-end: register the connection in the real registry, run the real command, wait for the subscription to drop, then assert no content arrives. A connection built without `register-connection` is invisible to the watcher, so the watcher-based tests must go through `open-registered-connection`. Interval tests shorten the config via `with-redefs` on `cf/get` rather than waiting minutes. `with-clean-registry` runs as an `:each` fixture, so no test can leak a live relay loop into the next one. `captured-events` forwards the keyword args through to the real `mbus/pub!` — rebuilding them positionally publishes to the topic `:message` with a `nil` payload and silently untests every other notice. Interval tests take access away with a direct row delete, never with a command: a command announces, so the test would pass even with the interval removed. The resubscribe-counting test is the one that proves finished relays stop polling. The soft-delete + org-owner re-check tests pair a direct row soft-delete with `with-redefs` on `nitrate/organization-owner-of-team?`: before the existence gate, the org-owner fallback kept the subscription open on a deleted resource. `watcher-consumes-revocation-events-from-the-bus` takes access away with a direct row delete and drives the watcher with a synthetic bus event — never a command and never a second `notify-*` — so the "still subscribed" assertion cannot race an after-commit hook and a broken command announcement cannot mask a broken watcher. `moving-a-project-closes-source-team-subscriptions` pins the move-project asymmetry: the file subscription drops, the source-team subscription survives (the member is still a member).
`backend/test/backend_tests/http_websocket_test.clj` drives the real Redis-backed msgbus, so the revocation tests are end-to-end: register the connection in the real registry, run the real command, wait for the subscription to drop, then assert no content arrives. A connection built without `register-connection` is invisible to the watcher, so the watcher-based tests must go through `open-registered-connection`. Interval tests shorten the config via `with-redefs` on `cf/get` rather than waiting minutes. `with-clean-registry` runs as an `:each` fixture, so no test can leak a live relay loop into the next one. `captured-events` forwards the keyword args through to the real `mbus/pub!` — rebuilding them positionally publishes to the topic `:message` with a `nil` payload and silently untests every other notice. Interval tests take access away with a direct row delete, never with a command: a command announces, so the test would pass even with the interval removed. The resubscribe-counting test is the one that proves finished relays stop polling. The soft-delete + org-owner re-check tests pair a direct row soft-delete with `with-redefs` on `nitrate/organization-owner-of-team?`: before the existence gate, the org-owner fallback kept the subscription open on a deleted resource. The soft-deleted-project/team-closes-org-owner-file-subscription tests pin the root fix in `app.rpc.permissions` (`resolve-team-id` refuses dead file/project/team for every caller of the fallback). `failing-existence-check-keeps-the-subscription` pins the fail-open policy for the existence probe itself. `watcher-consumes-revocation-events-from-the-bus` takes access away with a direct row delete and drives the watcher with a synthetic bus event — never a command and never a second `notify-*` — so the "still subscribed" assertion cannot race an after-commit hook and a broken command announcement cannot mask a broken watcher. `moving-a-project-closes-source-team-subscriptions` pins the move-project asymmetry: the file subscription drops, the source-team subscription survives (the member is still a member).
`revocation-watcher-schema-resolves-every-ref` covers the `::revocation-watcher` params schema, which no other test compiles. It existed broken for a while: the three injected keys were written as bare keywords, which is a valid malli *property* position and not a key, so the schema raised `:malli.core/invalid-schema` — invisible unless `PENPOT_FLAGS=enable-backend-asserts` was set, and a crash of the whole system build rather than one test (see `mem:backend/testing` on the flag).

View File

@ -182,19 +182,24 @@
(defn- file-readable?
"Non-throwing mirror of the `:subscribe-file` gates: the file must
still exist (a soft-deleted row grants nothing) and the profile must
read it. Used by the re-checks, where throwing is not an option: the
fresh answer is the decision."
read it. Same `sql:get-minimal-file` as the subscribe gate, with the
existence failure turned into a boolean — the fresh answer is the
decision."
[cfg profile-id file-id]
(and (some? (db/get* cfg :file {:id file-id}))
(and (some? (bfc/get-minimal-file cfg file-id
{::db/throw-if-not-exists false}))
(files/has-read-permissions? cfg profile-id file-id)))
(defn- team-readable?
"Non-throwing mirror of the file re-check shape, for teams: the team
must still exist (a soft-deleted row grants nothing) and the profile
must read it. `:subscribe-team` has no existence gate of its own; the
re-check adds one, matching the file side."
"Same shape for teams: the team must still exist (a soft-deleted row
grants nothing) and the profile must read it. `:subscribe-team` has
no existence gate of its own; the re-check adds one, matching the
file side. Projects only `:id` and `:deleted-at` — `db/get*` filters
soft-deleted rows through the `deleted_at` column, so it must be
in the projection."
[cfg profile-id team-id]
(and (some? (db/get* cfg :team {:id team-id}))
(and (some? (db/get* cfg :team {:id team-id}
{:app.db.sql/columns [:id :deleted-at]}))
(teams/has-read-permissions? cfg profile-id team-id)))
(defn close-file-subscription
@ -480,10 +485,11 @@
The event is the trigger, the fresh check is the decision.
`file-readable?` and `team-readable?` mirror the gates of
`:subscribe-file`: resource exists (a soft-deleted row grants
nothing) and the profile reads it. The organization-owner fallback
keeps a downgrade to viewer and a non-member owner from being mistaken
for revocations — but not a soft-deleted resource.
`:subscribe-file` and extend them to teams: resource exists
(a soft-deleted row grants nothing) and the profile reads it.
The organization-owner fallback keeps a downgrade to viewer and a
non-member owner from being mistaken for revocations — but not a
soft-deleted resource.
Returns the number of subscriptions it closed."
[cfg profile-id]

View File

@ -108,13 +108,16 @@
:can-edit false})
(def ^:private sql:get-team-id-for-project
"SELECT team_id FROM project WHERE id = ?")
"SELECT team_id FROM project WHERE id = ?
AND deleted_at IS NULL")
(def ^:private sql:get-team-id-for-file
"SELECT p.team_id
FROM file AS f
JOIN project AS p ON (p.id = f.project_id)
WHERE f.id = ?")
WHERE f.id = ?
AND f.deleted_at IS NULL
AND p.deleted_at IS NULL")
(defn get-team-id-for-project
[cfg project-id]
@ -127,11 +130,18 @@
(:team-id)))
(defn resolve-team-id
"Resolves the team that owns the project/file `params` references, or
`params`' own `:team-id`. A soft-deleted file/project/team resolves to
nil: no caller may rely on a dead team for access decisions. The team
row is checked for all branches — the SQL filters cover file and
project, this final check covers the team itself."
[cfg {:keys [team-id project-id file-id]}]
(cond
(some? team-id) team-id
(some? project-id) (get-team-id-for-project cfg project-id)
(some? file-id) (get-team-id-for-file cfg file-id)))
(let [team-id (cond (some? team-id) team-id
(some? project-id) (get-team-id-for-project cfg project-id)
(some? file-id) (get-team-id-for-file cfg file-id))]
(when (and team-id
(some? (db/get* cfg :team {:id team-id})))
team-id)))
(defn get-organization-owner-permissions
"When `profile-id` is a non-member owner of the organization that owns

View File

@ -6,6 +6,7 @@
(ns backend-tests.http-websocket-test
(:require
[app.binfile.common :as bfc]
[app.common.schema :as sm]
[app.common.time :as ct]
[app.common.uuid :as uuid]
@ -453,6 +454,15 @@
:project-id (:id project)})]
{:owner owner :editor editor :team team :file file})))
(defn- delete-team-membership!
"Takes `profile-id` out of `team-id` the way an external change would:
a direct row delete. Fires no command and publishes no announcement,
so no `db/after-commit` hook can race the test."
[team-id profile-id]
(db/exec! th/*system*
["delete from team_profile_rel where team_id = ? and profile_id = ?"
team-id profile-id]))
(t/deftest revocation-closes-subscriptions-that-are-no-longer-authorized
(let [{:keys [owner editor team file]} (editor-in-team! 1 2)]
@ -551,9 +561,7 @@
;; announcement, so the "still subscribed" check cannot race an
;; after-commit hook, and the command's own announcement cannot
;; mask a broken watcher.
(db/exec! th/*system*
["delete from team_profile_rel where team_id = ? and profile_id = ?"
(:id team) (:id editor)])
(delete-team-membership! (:id team) (:id editor))
(t/testing "the event is not applied until it is announced"
(t/is (= (:id file) (subscribed-file conn))))
@ -817,9 +825,7 @@
;; it: a direct row delete fires no command and publishes no
;; announcement, so the interval is the only thing that can
;; close the subscription.
(db/exec! th/*system*
["delete from team_profile_rel where team_id = ? and profile_id = ?"
(:id team) (:id editor)])
(delete-team-membership! (:id team) (:id editor))
(t/testing "the interval re-check drops it anyway"
(t/is (wait-until #(nil? (subscribed-file conn)) 3000)))
@ -877,9 +883,7 @@
;; Same external-change shape as above: no command, no
;; announcement, only the interval.
(db/exec! th/*system*
["delete from team_profile_rel where team_id = ? and profile_id = ?"
(:id team) (:id editor)])
(delete-team-membership! (:id team) (:id editor))
(t/testing "the team subscription is dropped by the interval"
(t/is (wait-until #(nil? (subscribed-team conn)) 3000)))))))
@ -901,6 +905,24 @@
(t/testing "a failing check keeps the subscription"
(t/is (some? (subscribed-file conn)))))))))
(t/deftest failing-existence-check-keeps-the-subscription
(let [{:keys [editor file]} (editor-in-team! 1 2)]
(with-redefs [cf/get (fn [k default]
(if (= :subscription-revalidation-interval k)
(ct/duration {:millis 150})
default))]
(let [conn (-> (open-registered-connection (:id editor))
(subscribe :subscribe-file {:file-id (:id file)}))]
;; The existence probe blows up; still-authorized? fails open,
;; so a transient database error keeps a legitimate subscription.
(with-redefs [bfc/get-minimal-file
(fn [& _] (throw (RuntimeException. "db down")))]
(Thread/sleep 500)
(t/testing "a failing existence check keeps the subscription"
(t/is (some? (subscribed-file conn)))))))))
(t/deftest a-false-check-closes-the-subscription
(let [{:keys [editor file]} (editor-in-team! 1 2)]
@ -1198,3 +1220,45 @@
th/*system* (:id editor)))))
(t/testing "the read-only subscription is gone"
(t/is (nil? (subscribed-team conn))))))))
(t/deftest soft-deleted-project-closes-org-owner-file-subscription
(let [{:keys [owner editor team file]} (editor-in-team! 1 2)]
(let [conn (-> (open-registered-connection (:id editor))
(subscribe :subscribe-file {:file-id (:id file)}))]
;; The admin-console flag is off in tests, so Nitrate never grants
;; this; stand in for an organization owner expelled from the team.
(with-redefs [nitrate/organization-owner-of-team? (fn [& _] true)]
;; A direct row soft-delete fires no command and publishes no
;; announcement. Membership SQL denies through a deleted project,
;; but the org-owner fallback resolves the team without any
;; deleted_at filter, so only the root fix can close the file
;; subscription.
(db/update! th/*system* :project {:deleted-at (ct/now)}
{:id (:project-id file)})
(t/testing "revalidation closes the file subscription although Nitrate still grants read-only"
(t/is (= 1 (ws/revalidate-profile-subscriptions
th/*system* (:id editor)))))
(t/testing "the read-only subscription is gone"
(t/is (nil? (subscribed-file conn))))))))
(t/deftest soft-deleted-team-closes-org-owner-file-subscription
(let [{:keys [owner editor team file]} (editor-in-team! 1 2)]
(let [conn (-> (open-registered-connection (:id editor))
(subscribe :subscribe-file {:file-id (:id file)}))]
;; Same shape with the ancestor unavailable at the team level:
;; membership denies (t.deleted_at is null), the org-owner
;; fallback grants, and only resolve-team-id's team-row check
;; can close the file subscription.
(with-redefs [nitrate/organization-owner-of-team? (fn [& _] true)]
(db/update! th/*system* :team {:deleted-at (ct/now)} {:id (:id team)})
(t/testing "revalidation closes the file subscription although Nitrate still grants read-only"
(t/is (= 1 (ws/revalidate-profile-subscriptions
th/*system* (:id editor)))))
(t/testing "the read-only subscription is gone"
(t/is (nil? (subscribed-file conn))))))))

View File

@ -82,9 +82,11 @@ Rel(backend_app, redis, "Notifies", "pub/sub")
* <code class="language-text">migrations</code> contains the SQL scripts that define the database schema, in
the form of a sequence of migrations.
* <code class="language-text">rpc</code> is the main module to handle the RPC API calls.
* <code class="language-text">notifications.clj</code> is the main module that manages the websocket. It allows
clients to subscribe to open files, intercepts update RPC calls and notify
them to all subscribers of the file.
* <code class="language-text">http/websocket.clj</code> manages the websocket endpoint: clients subscribe
to open files or teams and the server keeps the subscriptions; revoked access
is re-checked and dropped. <code class="language-text">notifications.clj</code> publishes
access-change events on the bus so the websocket re-checks the subscriptions
it owns.
* <code class="language-text">setup</code> initializes the environment (loads config variables, sets up the
database, executes migrations, loads initial data, etc).
* <code class="language-text">srepl</code> sets up an interactive REPL shell, with some useful commands to be