diff --git a/.serena/memories/backend/auth-permissions-product-domains.md b/.serena/memories/backend/auth-permissions-product-domains.md index c0667250f1..4ca476b892 100644 --- a/.serena/memories/backend/auth-permissions-product-domains.md +++ b/.serena/memories/backend/auth-permissions-product-domains.md @@ -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. diff --git a/.serena/memories/backend/websocket-subscriptions.md b/.serena/memories/backend/websocket-subscriptions.md index 13348a1a05..f9a3991cf5 100644 --- a/.serena/memories/backend/websocket-subscriptions.md +++ b/.serena/memories/backend/websocket-subscriptions.md @@ -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). diff --git a/backend/src/app/http/websocket.clj b/backend/src/app/http/websocket.clj index 418525655d..26bb09b7c7 100644 --- a/backend/src/app/http/websocket.clj +++ b/backend/src/app/http/websocket.clj @@ -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] diff --git a/backend/src/app/rpc/permissions.clj b/backend/src/app/rpc/permissions.clj index 77653017db..574e368a9c 100644 --- a/backend/src/app/rpc/permissions.clj +++ b/backend/src/app/rpc/permissions.clj @@ -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 diff --git a/backend/test/backend_tests/http_websocket_test.clj b/backend/test/backend_tests/http_websocket_test.clj index 5b5fb443a3..5e2090122d 100644 --- a/backend/test/backend_tests/http_websocket_test.clj +++ b/backend/test/backend_tests/http_websocket_test.clj @@ -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)))))))) diff --git a/docs/technical-guide/developer/architecture/backend.md b/docs/technical-guide/developer/architecture/backend.md index eb0517e9eb..6bef007c38 100644 --- a/docs/technical-guide/developer/architecture/backend.md +++ b/docs/technical-guide/developer/architecture/backend.md @@ -82,9 +82,11 @@ Rel(backend_app, redis, "Notifies", "pub/sub") * migrations contains the SQL scripts that define the database schema, in the form of a sequence of migrations. * rpc is the main module to handle the RPC API calls. -* notifications.clj 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. +* http/websocket.clj manages the websocket endpoint: clients subscribe + to open files or teams and the server keeps the subscriptions; revoked access + is re-checked and dropped. notifications.clj publishes + access-change events on the bus so the websocket re-checks the subscriptions + it owns. * setup initializes the environment (loads config variables, sets up the database, executes migrations, loads initial data, etc). * srepl sets up an interactive REPL shell, with some useful commands to be