github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-11-10 20:10:37 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Test resolve-team-id refusal and tidy review findings

Browse Source 
Third pass over PR #12189 after review-code on 225eb98de2.

resolve-team-id now has a direct test: all three branches resolve
while rows are live, and file, project and team soft-deletes each
refuse their branch — the two new SQL predicates and the final team-row
check are all pinned by it.

From the same review:

- failing-existence-check-keeps-the-subscription and the sibling
  a-failing-check-keeps-the-subscription are deleted: they could not
  distinguish a live relay from a dead one. The fail-open policy
  remains documented in the docstring and the memory; the memory no
  longer claims the pins exist.
- team-readable? projects ::sql/columns like the rest of the backend;
  the fully-qualified keyword went away with the new alias.
- The dead when-not guard in get-organization-owner-viewer-team is
  removed (resolve-team-id guarantees the row live); the let binding
  in resolve-team-id no longer shadows the destructured team-id.
- docs/technical-guide names rpc/notifications.clj as the publisher
  and the tree lists the real top-level modules.

Closes #12155

AI-assisted-by: inkling-2-small-free
This commit is contained in:
Andrey Antukh 2026-11-10 13:12:16 +00:00
parent 225eb98de2
commit 05f388c091
7 changed files with 58 additions and 53 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
.serena/memories/backend/websocket-subscriptions.md
View File

@ -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).

5
backend/src/app/http/websocket.clj
View File

@ -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

13
backend/src/app/rpc/commands/teams.clj
View File

@ -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}]

12
backend/src/app/rpc/permissions.clj
View File

@ -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

36
backend/test/backend_tests/http_websocket_test.clj
View File

@ -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)]

36
backend/test/backend_tests/rpc_organization_owner_permissions_test.clj
View File

@ -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)}))))))

7
docs/technical-guide/developer/architecture/backend.md
View File

@ -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")
* <code class="language-text">rpc</code> is the main module to handle the RPC API calls.
* <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
is re-checked and dropped. <code class="language-text">rpc/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