diff --git a/.serena/memories/backend/websocket-subscriptions.md b/.serena/memories/backend/websocket-subscriptions.md index 8b84cdfb8b..13348a1a05 100644 --- a/.serena/memories/backend/websocket-subscriptions.md +++ b/.serena/memories/backend/websocket-subscriptions.md @@ -11,7 +11,7 @@ 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 the same `has-read-permissions?` predicates the subscribe handlers authorize with — and `sql:file-permissions` additionally filters `deleted_at` on file, project and team, so a soft-deleted resource grants nothing. 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 (read-only via `get-organization-owner-permissions`) 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. +- **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`. 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. +`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). `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 44b58a8a22..418525655d 100644 --- a/backend/src/app/http/websocket.clj +++ b/backend/src/app/http/websocket.clj @@ -179,6 +179,24 @@ :cause cause) true))) +(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." + [cfg profile-id file-id] + (and (some? (db/get* cfg :file {:id file-id})) + (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." + [cfg profile-id team-id] + (and (some? (db/get* cfg :team {:id team-id})) + (teams/has-read-permissions? cfg profile-id team-id))) + (defn close-file-subscription "Tears down the file subscription held by the connection `wsp`, if it is subscribed to `file-id`. @@ -354,7 +372,7 @@ (swap! state assoc ::team-subscription subs)) (start-relay wsp channel - #(teams/has-read-permissions? cfg profile-id team-id) + #(team-readable? cfg profile-id team-id) #(close-team-subscription cfg wsp team-id) nil) @@ -378,7 +396,7 @@ (swap! state assoc ::file-subscription subs)) (start-relay wsp fch - #(files/has-read-permissions? cfg profile-id file-id) + #(file-readable? cfg profile-id file-id) #(close-file-subscription cfg wsp file-id) (fn [{:keys [type]}] (when (or (= :join-file type) @@ -460,12 +478,12 @@ "Re-checks every subscription held by the connections of `profile-id` and closes the ones that are no longer authorized. - The event is the trigger, the fresh permission check is the decision. - Reusing the same predicates that `:subscribe-file` and - `:subscribe-team` authorize with means the watcher needs no second copy - of the permission rules: a downgrade to viewer keeps read access and - so keeps the subscription, and a non-member organization owner keeps - the read-only access it is entitled to. + 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. Returns the number of subscriptions it closed." [cfg profile-id] @@ -476,13 +494,13 @@ fsub (get subs ::file-subscription) tsub (get subs ::team-subscription)] (when (and fsub - (not (files/has-read-permissions? cfg profile-id - (:file-id fsub)))) + (not (file-readable? cfg profile-id + (:file-id fsub)))) (close-file-subscription cfg wsp (:file-id fsub)) (vswap! closed inc)) (when (and tsub - (not (teams/has-read-permissions? cfg profile-id - (:team-id tsub)))) + (not (team-readable? cfg profile-id + (:team-id tsub)))) (close-team-subscription cfg wsp (:team-id tsub)) (vswap! closed inc))))) @closed)) diff --git a/backend/test/backend_tests/http_websocket_test.clj b/backend/test/backend_tests/http_websocket_test.clj index e11ba9b150..5b5fb443a3 100644 --- a/backend/test/backend_tests/http_websocket_test.clj +++ b/backend/test/backend_tests/http_websocket_test.clj @@ -546,10 +546,14 @@ (let [conn (-> (open-registered-connection (:id editor)) (subscribe :subscribe-file {:file-id (:id file)}))] - (th/command! {::th/type :delete-team-member - ::rpc/profile-id (:id owner) - :team-id (:id team) - :member-id (:id editor)}) + ;; Access is taken away the way an external change would take + ;; it: a direct row delete fires no command and publishes no + ;; 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)]) (t/testing "the event is not applied until it is announced" (t/is (= (:id file) (subscribed-file conn)))) @@ -1028,6 +1032,35 @@ (t/testing "a member left behind in the source team loses the subscription" (t/is (wait-until #(nil? (subscribed-file conn)) 3000)))))) +(t/deftest moving-a-project-closes-source-team-subscriptions + (let [owner (th/create-profile* 1 {:is-active true}) + editor (th/create-profile* 2 {:is-active true}) + team-a (th/create-team* 1 {:profile-id (:id owner)}) + team-b (th/create-team* 2 {:profile-id (:id owner)})] + (th/create-team-role* {:team-id (:id team-a) + :profile-id (:id editor) + :role :editor}) + (let [project-a (th/create-project* 1 {:profile-id (:id owner) + :team-id (:id team-a)}) + project-b (th/create-project* 2 {:profile-id (:id owner) + :team-id (:id team-b)}) + file (th/create-file* 1 {:profile-id (:id editor) + :project-id (:id project-a)}) + conn (-> (open-registered-connection (:id editor)) + (subscribe :subscribe-file {:file-id (:id file)}) + (subscribe :subscribe-team {:team-id (:id team-a)}))] + + (th/command! {::th/type :move-project + ::rpc/profile-id (:id owner) + :team-id (:id team-b) + :project-id (:id project-a)}) + + (t/testing "the file subscription drops: the file only lives in team-b now" + (t/is (wait-until #(nil? (subscribed-file conn)) 3000))) + + (t/testing "the team subscription survives: the editor is still a member of team-a" + (t/is (= (:id team-a) (subscribed-team conn))))))) + (t/deftest close-team-subscription-is-idempotent (let [profile (th/create-profile* 1 {:is-active true}) team (th/create-team* 1 {:profile-id (:id profile)}) @@ -1126,3 +1159,42 @@ (t/testing "the read-only subscription survives" (t/is (some? (subscribed-file conn)))))))) + +(t/deftest soft-deleted-file-closes-org-owner-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)] + ;; Access is taken away the way an external change would take + ;; it: a direct row soft-delete fires no command and publishes no + ;; announcement. Membership SQL grants nothing through a deleted + ;; file, but the org-owner fallback does not look at deleted_at, + ;; so only the existence gate can close the subscription. + (db/update! th/*system* :file {:deleted-at (ct/now)} {:id (:id file)}) + + (t/testing "revalidation closes the 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-subscription + (let [{:keys [owner editor team file]} (editor-in-team! 1 2)] + + (let [conn (-> (open-registered-connection (:id editor)) + (subscribe :subscribe-team {:team-id (:id team)}))] + + ;; Same shape as the file case: the org-owner fallback grants + ;; read-only access without looking at the team's deleted_at. + (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 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-team conn))))))))