diff --git a/.serena/memories/backend/websocket-subscriptions.md b/.serena/memories/backend/websocket-subscriptions.md index f9a3991cf5..1de07fe5c5 100644 --- a/.serena/memories/backend/websocket-subscriptions.md +++ b/.serena/memories/backend/websocket-subscriptions.md @@ -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. 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). +`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). `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 26bb09b7c7..597c115c1e 100644 --- a/backend/src/app/http/websocket.clj +++ b/backend/src/app/http/websocket.clj @@ -17,6 +17,7 @@ [app.common.uuid :as uuid] [app.config :as cf] [app.db :as db] + [app.db.sql :as sql] [app.http.session :as session] [app.metrics :as mtx] [app.msgbus :as mbus] @@ -194,12 +195,12 @@ "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 + file side. Selects 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} - {:app.db.sql/columns [:id :deleted-at]})) + {::sql/columns [:id :deleted-at]})) (teams/has-read-permissions? cfg profile-id team-id))) (defn close-file-subscription diff --git a/backend/src/app/rpc/commands/teams.clj b/backend/src/app/rpc/commands/teams.clj index 789e0b82a1..e3a2794086 100644 --- a/backend/src/app/rpc/commands/teams.clj +++ b/backend/src/app/rpc/commands/teams.clj @@ -258,13 +258,14 @@ [cfg profile-id default-team-id params] (when-let [team-id (perms/resolve-team-id cfg params)] (when (nitrate/organization-owner-of-team? cfg profile-id team-id) + ;; resolve-team-id already proved the team row live; the guard + ;; that used to live here (when-not is-row-deleted?) was dead. (when-let [team (db/get* cfg :team {:id team-id})] - (when-not (db/is-row-deleted? team) - (-> team - (decode-row) - (merge perms/viewer-role-flags) - (assoc :is-default (= team-id default-team-id)) - (process-permissions))))))) + (-> team + (decode-row) + (merge perms/viewer-role-flags) + (assoc :is-default (= team-id default-team-id)) + (process-permissions)))))) (defn get-team [cfg & {:keys [profile-id team-id project-id file-id] :as params}] diff --git a/backend/src/app/rpc/permissions.clj b/backend/src/app/rpc/permissions.clj index 574e368a9c..8dcf79c604 100644 --- a/backend/src/app/rpc/permissions.clj +++ b/backend/src/app/rpc/permissions.clj @@ -136,12 +136,12 @@ 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]}] - (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))) + (let [resolved-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 resolved-team-id + (some? (db/get* cfg :team {:id resolved-team-id}))) + resolved-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 5e2090122d..85e915e2b1 100644 --- a/backend/test/backend_tests/http_websocket_test.clj +++ b/backend/test/backend_tests/http_websocket_test.clj @@ -6,7 +6,6 @@ (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] @@ -888,41 +887,6 @@ (t/testing "the team subscription is dropped by the interval" (t/is (wait-until #(nil? (subscribed-team conn)) 3000))))))) -(t/deftest a-failing-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)}))] - - ;; Access still holds, but the check itself blows up. - (with-redefs [files/has-read-permissions? - (fn [& _] (throw (RuntimeException. "db down")))] - (Thread/sleep 500) - (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)] diff --git a/backend/test/backend_tests/rpc_organization_owner_permissions_test.clj b/backend/test/backend_tests/rpc_organization_owner_permissions_test.clj index 13c7f8120f..6967dddac6 100644 --- a/backend/test/backend_tests/rpc_organization_owner_permissions_test.clj +++ b/backend/test/backend_tests/rpc_organization_owner_permissions_test.clj @@ -6,11 +6,14 @@ (ns backend-tests.rpc-organization-owner-permissions-test (:require + [app.common.time :as ct] [app.common.uuid :as uuid] [app.config :as cf] + [app.db :as db] [app.msgbus :as mbus] [app.nitrate :as nitrate] [app.rpc :as-alias rpc] + [app.rpc.permissions :as perms] [backend-tests.helpers :as th] [clojure.test :as t])) @@ -247,3 +250,36 @@ :id (:id target-team)})] (t/is (th/ex-info? (:error out))) (t/is (th/ex-of-type? (:error out) :not-found))))))) + +(t/deftest resolve-team-id-refuses-soft-deleted-ancestors + (let [owner (th/create-profile* 1 {:is-active true}) + editor (th/create-profile* 2 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)}) + project (th/create-project* 1 {:profile-id (:id editor) + :team-id (:id team)}) + file (th/create-file* 1 {:profile-id (:id editor) + :project-id (:id project)})] + + (t/testing "a live file/project/team resolves in every branch" + (t/is (= (:id team) (perms/resolve-team-id th/*system* {:file-id (:id file)}))) + (t/is (= (:id team) (perms/resolve-team-id th/*system* {:project-id (:id project)}))) + (t/is (= (:id team) (perms/resolve-team-id th/*system* {:team-id (:id team)})))) + + (t/testing "a soft-deleted file refuses the file branch" + (db/update! th/*system* :file {:deleted-at (ct/now)} {:id (:id file)}) + (t/is (nil? (perms/resolve-team-id th/*system* {:file-id (:id file)}))) + (t/is (= (:id team) (perms/resolve-team-id th/*system* {:team-id (:id team)}))) + (db/update! th/*system* :file {:deleted-at nil} {:id (:id file)})) + + (t/testing "a soft-deleted project refuses the file and the project branch" + (db/update! th/*system* :project {:deleted-at (ct/now)} {:id (:id project)}) + (t/is (nil? (perms/resolve-team-id th/*system* {:file-id (:id file)}))) + (t/is (nil? (perms/resolve-team-id th/*system* {:project-id (:id project)}))) + (t/is (= (:id team) (perms/resolve-team-id th/*system* {:team-id (:id team)}))) + (db/update! th/*system* :project {:deleted-at nil} {:id (:id project)})) + + (t/testing "a soft-deleted team refuses every branch" + (db/update! th/*system* :team {:deleted-at (ct/now)} {:id (:id team)}) + (t/is (nil? (perms/resolve-team-id th/*system* {:file-id (:id file)}))) + (t/is (nil? (perms/resolve-team-id th/*system* {:project-id (:id project)}))) + (t/is (nil? (perms/resolve-team-id th/*system* {:team-id (:id team)})))))) diff --git a/docs/technical-guide/developer/architecture/backend.md b/docs/technical-guide/developer/architecture/backend.md index 6bef007c38..5a26d9d8b0 100644 --- a/docs/technical-guide/developer/architecture/backend.md +++ b/docs/technical-guide/developer/architecture/backend.md @@ -63,13 +63,16 @@ Rel(backend_app, redis, "Notifies", "pub/sub") ▸ tasks/ main.clj config.clj + health.clj http.clj + jobs.clj metrics.clj migrations.clj - notifications.clj + nitrate.clj rpc.clj setup.clj srepl.clj + system.clj worker.clj ... ``` @@ -84,7 +87,7 @@ Rel(backend_app, redis, "Notifies", "pub/sub") * rpc is the main module to handle the RPC API calls. * 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 + is re-checked and dropped. rpc/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