mirror of
https://github.com/penpot/penpot.git
synced 2026-11-10 12:00:37 +00:00
🐛 Mirror the subscribe gates in the revocation re-checks
Addresses the second review round on PR #12189 (pabloalba). The re-checks only asked whether the profile could read. The membership legs of sql:file-permissions filter soft-deleted rows since the F2 fix, but the organization-owner fallback grants read-only access over HTTP without looking at deleted_at, so a soft-deleted file or team kept an expelled org owner subscribed indefinitely. The re-checks now gate on existence as well: file-readable? and team-readable? require a live row (db/get*, non-throwing, soft-deleted rows excluded) plus the read permission, mirroring the :subscribe-file gates and extending them to teams. Both relays and revalidate-profile-subscriptions use them; the subscribe handlers keep their exact behavior. Also from the same review: - watcher-consumes-revocation-events-from-the-bus no longer races the command's after-commit announcement: it takes access away with a direct row delete and drives the watcher with a synthetic bus event, instead of deleting through the command and publishing a second notify-* that would mask a broken command announcement. - moving-a-project-closes-source-team-subscriptions pins the move-project e2e: the file subscription drops, the source-team subscription survives (membership unchanged). - soft-deleted-file/team-closes-org-owner-subscription cover the new gate with red-before/green-after tests. mem:backend/websocket-subscriptions updated. Closes #12155 AI-assisted-by: inkling-2-small-free
This commit is contained in:
Andrey Antukh
parent
f7f4cbb301
commit
87ca7aca53
@ -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).
|
||||
|
||||
|
||||
@ -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))
|
||||
|
||||
@ -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))))))))
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user