From d47c0299343f3df6cf59d678d978cf685f04a37b Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Tue, 3 Nov 2026 10:04:13 +0000 Subject: [PATCH] :sparkles: Make plugin writes fail while the workspace is read-only A read-only workspace (saves failing, a user who cannot edit, a version preview) only hid the editing tools: plugins, the MCP among them, still changed shapes, and those edits were never saved. Every plugin write now throws a read-only error, whatever the throwValidationErrors flag, the same way a write to a removed shape does. `check-permission` with a write permission runs the new `check-writable`, which covers most writers. Writers without a permission check (most context methods such as create*, group, align or flatten, the z-order methods, applyTypography on a text range, removing a ruler guide) call it themselves, and shape setters get it through `check-shape-writable`. Reads, comments and undo blocks keep working, so a block opened before a lock still closes. The MCP instructions tell the agent what the read-only error means. --- .serena/memories/clojure/idioms.md | 1 + .../frontend/plugin-api-to-cljs-binding.md | 4 +- .serena/memories/frontend/testing.md | 1 + .../src/app/main/data/workspace/media.cljs | 6 +- frontend/src/app/plugins/api.cljs | 28 ++- frontend/src/app/plugins/history.cljs | 6 +- frontend/src/app/plugins/library.cljs | 2 + frontend/src/app/plugins/management.cljs | 21 +- frontend/src/app/plugins/register.cljs | 26 ++- frontend/src/app/plugins/ruler_guides.cljs | 1 + frontend/src/app/plugins/shape.cljs | 6 +- frontend/src/app/plugins/text.cljs | 1 + frontend/src/app/plugins/utils.cljs | 15 +- .../frontend_tests/plugins/library_test.cljs | 35 ++- .../plugins/management_test.cljs | 4 +- .../plugins/read_only_test.cljs | 211 ++++++++++++++++++ frontend/test/frontend_tests/runner.cljs | 2 + .../server/data/initial_instructions.md | 5 + plugins/CHANGELOG.md | 2 + 19 files changed, 352 insertions(+), 25 deletions(-) create mode 100644 frontend/test/frontend_tests/plugins/read_only_test.cljs diff --git a/.serena/memories/clojure/idioms.md b/.serena/memories/clojure/idioms.md index 7e81fba7ac..3fa120aa62 100644 --- a/.serena/memories/clojure/idioms.md +++ b/.serena/memories/clojure/idioms.md @@ -8,6 +8,7 @@ Behaviors confirmed against the language/stdlib — do not re-derive from assump - The analyzer reads `:async` only from the fn name meta and the `fn` operator meta; list-level meta is ignored (would make a MetaFn). `(fn ^:async [] …)` puts the meta on argv (pre/post only, NOT async). - Valid: `(defn ^:async f)`, `(defn- ^:async f)`, `(^:async fn [] …)` (the latter is what stock `t/async` generates itself). - `^:async` fns never throw synchronously (rejected promises instead); `try/catch/finally` supported; continuations are microtasks, timers and RxJS schedulers are macrotasks (FIFO). +- Inside an `^:async` fn, a nested call that needs a temp binding (e.g. calling a local fn `(g (f x))`, whose arg gets a `G__` binding) compiles to `(await (async function(){…})())`: `await` unwraps a returned promise, so `(settled (upload (make-ctx)))` blocks until the upload promise settles instead of passing the promise on (a test that pushes the answer later hangs, idle). Bind each step in its own `let` binding. - `!` suffix allowed cases (only exceptions to the no-new-`!` rule in `mem:clojure/design-rules`): mutating public `app.db` API (`insert!`, `update!`, `delete!`, `run!`, `tx-run!`, `exec!`, ...) and fns whose contract is to work over transients (stdlib `conj!`, `assoc!`, `dissoc!`, `disj!`, `persistent!` and custom helpers over them); any further case must be registered here with justification before use. - Penpot extends `clojure.core/Inst` in `app.common.time` ( JVM-only section), so `inst-ms` also accepts `Duration` (returns `.toMillis`, NOT epoch millis), `FileTime` and `OffsetDateTime`. It only works where that namespace is loaded; on a bare `Duration`, prefer explicit `.toMillis`, which reads as what it is (an amount, not an instant). - `cuerdas.core/index-of` returns `nil` (not `-1`) when the substring is absent, like `clojure.string/index-of`. `(pos? nil)` **throws on the JVM** but evaluates to `false` in CLJS (JS coercion), so `(if (pos? (str/index-of s "\n")) …)` works in the browser and crashes the backend. Guard with `some?` before `pos?` (this was `app.common.exceptions/first-line`). diff --git a/.serena/memories/frontend/plugin-api-to-cljs-binding.md b/.serena/memories/frontend/plugin-api-to-cljs-binding.md index fc641a85e7..22d7efea5b 100644 --- a/.serena/memories/frontend/plugin-api-to-cljs-binding.md +++ b/.serena/memories/frontend/plugin-api-to-cljs-binding.md @@ -23,7 +23,9 @@ - Public Plugin API objects are lightweight handles, not durable snapshots. Most getters locate fresh state from `app.main.store/state` using hidden `$id`, `$file`, `$page`, etc. - `not-valid` logs by default but throws when the plugin flag `throwValidationErrors` is enabled. The MCP execute-code handler deliberately enables that flag while running code. -- A write to a removed shape always throws `Shape not found`, whatever that flag: setters through the shape proxy's `:on-set u/check-shape-exists` (runs outside the error handling), and methods that must not vanish silently (`resize`, `rotate`) call `u/check-shape-located` first. Methods run inside `:on-error (u/handle-error ...)`, which logs instead of throwing when the flag is off, so it rethrows errors whose `ex-data :type` is `::u/shape-not-found`. +- A write to a removed shape always throws `Shape not found`, whatever that flag: setters through the shape proxy's `:on-set u/check-shape-writable` (runs outside the error handling), and methods that must not vanish silently (`resize`, `rotate`) call `u/check-shape-located` first. Methods run inside `:on-error (u/handle-error ...)`, which logs instead of throwing when the flag is off, so it rethrows errors whose `ex-data :type` is `::u/shape-not-found` or `::r/read-only`. +- A write in a read-only workspace (`[:workspace-global :read-only?]`: save-failure lock, user without edit rights, version preview) always throws, whatever that flag. `r/check-permission` with `content:write`/`library:write` calls `r/check-writable` first, which covers most writers; writers without a permission check (most top-level context methods in `api.cljs`: `create*`, `group`, `align*`, …; z-order methods; text range `applyTypography`; ruler guide `remove`) call `r/check-writable` themselves, and shape setters get it through `u/check-shape-writable`. A new writer without a write-permission check must call it too. `r/has-permission?` is the check without it, used by the undo blocks so a block opened before a lock still closes. Comments (`comment:write`) stay writable: they do not go through file saves. An async writer calls it again after each server await, before the next server write or commit (`rx/tap` on the response stream; `dwm/create-svg-shape-with-images` takes it as `:check-writable`), so a lock that lands mid-way rejects the promise with the read-only error. `r/has-permission?` is the check without it, used by the undo blocks so a block opened before a lock still closes. Comments (`comment:write`) stay writable: they do not go through file saves. +- `penpotMgmt` (`app.plugins.management`) checks permissions with `r/has-permission?` (`granted?`), never `r/check-permission`: its calls act on files through the backend, often from the dashboard with no file open, so the workspace read-only check (`check-writable`) must not block them. - `naturalChildOrdering` and `throwValidationErrors` are stored per plugin under `[:plugins :flags plugin-id ...]`; changing default behavior affects automation and MCP diagnostics. - Plugin data is stored under keyword namespaces: private data uses `(keyword "plugin" plugin-id)`, shared data uses `(keyword "shared" namespace)`. diff --git a/.serena/memories/frontend/testing.md b/.serena/memories/frontend/testing.md index 7c6bfec777..37f5304395 100644 --- a/.serena/memories/frontend/testing.md +++ b/.serena/memories/frontend/testing.md @@ -39,6 +39,7 @@ To assert over emitted event sequences, observe termination: subscribe through ` - Auto-answering mocks lose deadline expressiveness (can't time answers), need teardown timer-cancellation, and post-teardown deliveries hit real implementations — explicit pushes + async delivery + `wait-for` won on every axis. - Teardown belongs to the terminal continuation, never to `finally`-around-triggers (it would dispose in-flight flows). - A body that awaits must be `(^:async fn …)` even if the rest is sync; sync sequences are atomic vs the event loop. +- The `:esm` test build cannot load the CommonJS `sax` module behind `tubax`, so SVG parsing fails in tests (`dwm/valid-svg-string?` answers false, `dwm/svg->clj` errors): mock both with the parsed data. From `frontend/`: - Full unit test run (always builds, suppressed output): `pnpm run test:quiet`. diff --git a/frontend/src/app/main/data/workspace/media.cljs b/frontend/src/app/main/data/workspace/media.cljs index 979b84bc4d..71ac10d86a 100644 --- a/frontend/src/app/main/data/workspace/media.cljs +++ b/frontend/src/app/main/data/workspace/media.cljs @@ -491,7 +491,10 @@ :ignore-selection? true :change-selection? false})))))) (defn create-svg-shape-with-images - [file-id id name svg-string position on-success on-error] + "Uploads the images of `svg-string` and adds it as shape `id`. + `check-writable` runs once the images are uploaded, before the shape is + added; when it throws, no shape is added and `on-error` gets the error." + [file-id id name svg-string position {:keys [check-writable on-success on-error]}] (ptk/reify ::create-svg-shape-with-images ptk/WatchEvent (watch [_ _ _] @@ -501,6 +504,7 @@ (fn [svg-data] (->> (svg/upload-images svg-data file-id) (rx/map #(assoc svg-data :image-data %))))) + (rx/tap (fn [_] (check-writable))) (rx/map (fn [svg-data] (svg/add-svg-shapes diff --git a/frontend/src/app/plugins/api.cljs b/frontend/src/app/plugins/api.cljs index c2a745cf13..92ca262da5 100644 --- a/frontend/src/app/plugins/api.cljs +++ b/frontend/src/app/plugins/api.cljs @@ -65,6 +65,7 @@ ;; (defn create-shape [plugin-id type] + (r/check-writable) (let [page (dsh/lookup-page @st/state) shape (cts/setup-shape {:type type :x 0 :y 0 @@ -205,6 +206,7 @@ :replaceColor (fn [shapes old-color new-color] + (r/check-writable) (let [old-color (parser/parse-color-data old-color) new-color (parser/parse-color-data new-color)] (cond @@ -261,6 +263,7 @@ :uploadMediaUrl (fn [name url] + (r/check-writable) (cond (not (string? name)) (u/not-valid plugin-id :uploadMedia-name name) @@ -274,12 +277,15 @@ (fn [resolve reject] (->> (dwm/upload-media-url name file-id url) (rx/take 1) + ;; Saves may have failed while the upload ran + (rx/tap (fn [_] (r/check-writable))) (rx/map format/format-image) (rx/tap #(st/emit! (se/event plugin-id "add-media"))) (rx/subs! resolve reject))))))) :uploadMediaData (fn [name data mime-type] + (r/check-writable) (let [file-id (:current-file-id @st/state)] (js/Promise. (fn [resolve reject] @@ -307,12 +313,15 @@ :on-image identity :on-svg identity}) (rx/take 1) + ;; Saves may have failed while the upload ran + (rx/tap (fn [_] (r/check-writable))) (rx/map format/format-image) (rx/tap #(st/emit! (se/event plugin-id "add-media"))) (rx/subs! resolve reject)))))) :group (fn [shapes] + (r/check-writable) (let [valid-shapes? (and (array? shapes) (every? shape/shape-proxy? shapes)) file-id (:current-file-id @st/state) page-id (:current-page-id @st/state) @@ -343,6 +352,7 @@ :ungroup (fn [group & rest] + (r/check-writable) (let [valid-group? (shape/shape-proxy? group) valid-rest? (every? shape/shape-proxy? rest) shapes (when (and valid-group? valid-rest?) (concat [group] rest)) @@ -382,6 +392,7 @@ :createPath (fn [] + (r/check-writable) (let [page (dsh/lookup-page @st/state) shape (cts/setup-shape {:type :path @@ -399,6 +410,7 @@ :createText (fn [text] + (r/check-writable) (cond (not (string? text)) (u/not-valid plugin-id :createText text) @@ -436,6 +448,7 @@ :createShapeFromSvg (fn [svg-string] + (r/check-writable) (cond (not (dwm/valid-svg-string? svg-string)) (u/not-valid plugin-id :createShapeFromSvg svg-string) @@ -450,6 +463,7 @@ :createShapeFromSvgWithImages (fn [svg-string] + (r/check-writable) (js/Promise. (fn [resolve reject] (cond @@ -464,12 +478,15 @@ page-id (:current-page-id @st/state)] (st/emit! (dwm/create-svg-shape-with-images file-id id "svg" svg-string (gpt/point 0 0) - #(resolve (shape/shape-proxy plugin-id file-id page-id id)) - reject) + {;; Saves may have failed while the images uploaded + :check-writable r/check-writable + :on-success #(resolve (shape/shape-proxy plugin-id file-id page-id id)) + :on-error reject}) (se/event plugin-id "create-shape" :type :text))))))) :createBoolean (fn [bool-type shapes] + (r/check-writable) (let [bool-type (keyword bool-type)] (cond (not (contains? cts/bool-types bool-type)) @@ -599,6 +616,7 @@ :createPage (fn [] + (r/check-writable) (let [file-id (:current-file-id @st/state) id (uuid/next)] (st/emit! (-> (dw/create-page {:page-id id :file-id file-id}) @@ -636,6 +654,7 @@ :alignHorizontal (fn [shapes direction] + (r/check-writable) (let [dir (case direction "left" :hleft "center" :hcenter @@ -654,6 +673,7 @@ :alignVertical (fn [shapes direction] + (r/check-writable) (let [dir (case direction "top" :vtop "center" :vcenter @@ -672,6 +692,7 @@ :distributeHorizontal (fn [shapes] + (r/check-writable) (cond (or (not (array? shapes)) (not (every? shape/shape-proxy? shapes))) (u/not-valid plugin-id :distributeHorizontal-shapes "Not valid shapes") @@ -682,6 +703,7 @@ :distributeVertical (fn [shapes] + (r/check-writable) (cond (or (not (array? shapes)) (not (every? shape/shape-proxy? shapes))) (u/not-valid plugin-id :distributeVertical-shapes "Not valid shapes") @@ -692,6 +714,7 @@ :flatten (fn [shapes] + (r/check-writable) (cond (or (not (array? shapes)) (not (every? shape/shape-proxy? shapes))) (u/not-valid plugin-id :flatten-shapes "Not valid shapes") @@ -707,6 +730,7 @@ :createVariantFromComponents (fn [shapes] + (r/check-writable) (cond (or (not (array? shapes)) (not (seq shapes)) diff --git a/frontend/src/app/plugins/history.cljs b/frontend/src/app/plugins/history.cljs index 68eb122506..54996cddee 100644 --- a/frontend/src/app/plugins/history.cljs +++ b/frontend/src/app/plugins/history.cljs @@ -20,10 +20,12 @@ (obj/reify {:name "HistorySubcontext"} :$plugin {:enumerable false :get (fn [] plugin-id)} + ;; Undo blocks write nothing, so they work in a read-only workspace: + ;; a block opened before a lock can still be closed :undoBlockBegin (fn [] (cond - (not (r/check-permission plugin-id "content:write")) + (not (r/has-permission? plugin-id "content:write")) (u/not-valid plugin-id :resize "Plugin doesn't have 'content:write' permission") :else @@ -34,7 +36,7 @@ :undoBlockFinish (fn [block-id] (cond - (not (r/check-permission plugin-id "content:write")) + (not (r/has-permission? plugin-id "content:write")) (u/not-valid plugin-id :resize "Plugin doesn't have 'content:write' permission") (not block-id) diff --git a/frontend/src/app/plugins/library.cljs b/frontend/src/app/plugins/library.cljs index e7a91f9236..faec7fad16 100644 --- a/frontend/src/app/plugins/library.cljs +++ b/frontend/src/app/plugins/library.cljs @@ -1392,6 +1392,8 @@ team-id (:current-team-id @st/state) library-id (uuid/parse library-id)] (->> (rp/cmd! :get-team-shared-files {:team-id team-id}) + ;; Saves may have failed while the list loaded + (rx/tap (fn [_] (r/check-writable))) (rx/subs! (fn [shared-files] ;; Only published libraries of the team can be linked diff --git a/frontend/src/app/plugins/management.cljs b/frontend/src/app/plugins/management.cljs index 727939b8b7..91671991f8 100644 --- a/frontend/src/app/plugins/management.cljs +++ b/frontend/src/app/plugins/management.cljs @@ -90,6 +90,13 @@ (when (<= 1 (count value) 250) value)))) +(defn- granted? + "True when the plugin was granted `permission`. Management calls act on + files through the backend, not on the open workspace, so the workspace's + read-only state does not apply to them." + [plugin-id permission] + (r/has-permission? plugin-id permission)) + (defn- permission-error [permission] (js/Error. (str "Permission " permission " is not granted"))) @@ -110,7 +117,7 @@ "Resolves with the single value of the stream built by `make-stream` when the plugin has `permission`." [plugin-id permission make-stream] - (if (r/check-permission plugin-id permission) + (if (granted? plugin-id permission) (js/Promise. (fn [resolve reject] (rx/subs! resolve #(reject (backend-error %)) (make-stream)))) @@ -294,7 +301,7 @@ (fn [value] (let [name (valid-name value)] (cond - (not (r/check-permission plugin-id "manage:teams")) + (not (granted? plugin-id "manage:teams")) (u/not-valid plugin-id :name "Plugin doesn't have 'manage:teams' permission") (nil? name) @@ -340,7 +347,7 @@ (fn [value] (let [name (valid-name value)] (cond - (not (r/check-permission plugin-id "manage:projects")) + (not (granted? plugin-id "manage:projects")) (u/not-valid plugin-id :name "Plugin doesn't have 'manage:projects' permission") (nil? name) @@ -355,7 +362,7 @@ :set (fn [value] (cond - (not (r/check-permission plugin-id "manage:projects")) + (not (granted? plugin-id "manage:projects")) (u/not-valid plugin-id :pinned "Plugin doesn't have 'manage:projects' permission") (not (boolean? value)) @@ -465,7 +472,7 @@ (fn [value] (let [name (valid-name value)] (cond - (not (r/check-permission plugin-id "content:write")) + (not (granted? plugin-id "content:write")) (u/not-valid plugin-id :name "Plugin doesn't have 'content:write' permission") (nil? name) @@ -480,7 +487,7 @@ :set (fn [value] (cond - (not (r/check-permission plugin-id "library:write")) + (not (granted? plugin-id "library:write")) (u/not-valid plugin-id :shared "Plugin doesn't have 'library:write' permission") (not (boolean? value)) @@ -492,7 +499,7 @@ :open (fn [] - (if (r/check-permission plugin-id "content:read") + (if (granted? plugin-id "content:read") (open-file (str (:id @data)) #js {:teamId (str (:team-id @data))}) (js/Promise.reject (permission-error "content:read")))) diff --git a/frontend/src/app/plugins/register.cljs b/frontend/src/app/plugins/register.cljs index 0c591eeb28..072fb386a4 100644 --- a/frontend/src/app/plugins/register.cljs +++ b/frontend/src/app/plugins/register.cljs @@ -270,7 +270,8 @@ (.error js/console "Rollback install failed:" err2)))))) (.error js/console "Failed to remove plugin:" err)))))))) -(defn check-permission +(defn has-permission? + "True when the plugin was granted `permission`." [plugin-id permission] (cond (= plugin-id "00000000-0000-0000-0000-000000000000") true @@ -278,6 +279,29 @@ :else (let [{:keys [permissions]} (dm/get-in @registry [:data plugin-id])] (contains? permissions permission)))) +(defn check-writable + "Throws while the workspace is read-only: saves are failing, the user + cannot edit or a version is being previewed. A write made then would be + lost, so it fails whatever the plugin's validation flag." + [] + (let [wglobal (:workspace-global @st/state)] + (when (true? (:read-only? wglobal)) + (throw (ex-info (if (some? (:persistence-lock wglobal)) + "[PENPOT PLUGIN] The workspace is read-only because changes cannot be saved right now" + "[PENPOT PLUGIN] The workspace is read-only") + {:type ::read-only}))))) + +(def ^:private write-permissions + #{"content:write" "library:write"}) + +(defn check-permission + "True when the plugin was granted `permission`. A write permission also + runs `check-writable`, which throws while the workspace is read-only." + [plugin-id permission] + (when (contains? write-permissions permission) + (check-writable)) + (has-permission? plugin-id permission)) + (defn get-plugin-data [state plugin-id] (get-in state [:profile :props :plugins :data plugin-id])) diff --git a/frontend/src/app/plugins/ruler_guides.cljs b/frontend/src/app/plugins/ruler_guides.cljs index 56dd051f1e..701a6a41d5 100644 --- a/frontend/src/app/plugins/ruler_guides.cljs +++ b/frontend/src/app/plugins/ruler_guides.cljs @@ -124,6 +124,7 @@ :remove (fn [] + (r/check-writable) (cond (not (u/page-active? page-id)) (u/not-valid plugin-id :remove "Cannot modify a page that is not currently active") diff --git a/frontend/src/app/plugins/shape.cljs b/frontend/src/app/plugins/shape.cljs index 1a73d116a8..a313ae1ee6 100644 --- a/frontend/src/app/plugins/shape.cljs +++ b/frontend/src/app/plugins/shape.cljs @@ -365,7 +365,7 @@ (let [data (u/locate-shape file-id page-id id)] (-> (obj/reify {:name "ShapeProxy" - :on-set u/check-shape-exists + :on-set u/check-shape-writable :on-error (u/handle-error plugin-id)} :$plugin {:enumerable false :get (fn [] plugin-id)} :$id {:enumerable false :get (fn [] id)} @@ -1472,18 +1472,22 @@ :bringForward (fn [] + (r/check-writable) (st/emit! (dw/vertical-order-selected (z-order-location plugin-id file-id page-id id :up) [id]))) :sendBackward (fn [] + (r/check-writable) (st/emit! (dw/vertical-order-selected (z-order-location plugin-id file-id page-id id :down) [id]))) :bringToFront (fn [] + (r/check-writable) (st/emit! (dw/vertical-order-selected (z-order-location plugin-id file-id page-id id :top) [id]))) :sendToBack (fn [] + (r/check-writable) (st/emit! (dw/vertical-order-selected (z-order-location plugin-id file-id page-id id :bottom) [id]))) ;; COMPONENTS diff --git a/frontend/src/app/plugins/text.cljs b/frontend/src/app/plugins/text.cljs index 66b2f2fb88..0be157b613 100644 --- a/frontend/src/app/plugins/text.cljs +++ b/frontend/src/app/plugins/text.cljs @@ -431,6 +431,7 @@ :applyTypography (fn [typography] + (r/check-writable) (let [typography (u/proxy->library-typography typography) attrs (-> typography (assoc :typography-ref-file file-id) diff --git a/frontend/src/app/plugins/utils.cljs b/frontend/src/app/plugins/utils.cljs index 9b4f7f2cb9..a9a6479de4 100644 --- a/frontend/src/app/plugins/utils.cljs +++ b/frontend/src/app/plugins/utils.cljs @@ -20,6 +20,7 @@ [app.main.data.helpers :as dsh] [app.main.data.workspace.tokens.library-edit :as dwtl] [app.main.store :as st] + [app.plugins.register :as r] [app.util.object :as obj] [cuerdas.core :as str])) @@ -165,11 +166,14 @@ (when (nil? (locate-shape file-id page-id id)) (throw (shape-not-found id)))) -(defn check-shape-exists - "`check-shape-located` for the shape behind `proxy`." +(defn check-shape-writable + "Throws when the shape behind `proxy` no longer exists (see + `check-shape-located`) or the workspace is read-only (see + `r/check-writable`)." [proxy] (when (nil? (proxy->shape proxy)) - (throw (shape-not-found (obj/get proxy "$id"))))) + (throw (shape-not-found (obj/get proxy "$id")))) + (r/check-writable)) (defn inside-component-copy? "True when `shape` is nested inside a component copy. The copy root itself is @@ -601,8 +605,9 @@ (let [explain (-> cause ex-data ::sm/explain) throw? (throw-validation-errors? plugin-id)] (cond - ;; A write to a removed shape always reaches the plugin - (= ::shape-not-found (-> cause ex-data :type)) + ;; A write to a removed shape or a read-only workspace always + ;; reaches the plugin + (contains? #{::shape-not-found ::r/read-only} (-> cause ex-data :type)) (throw cause) ;; If it's a clojure error we throw as a validation error diff --git a/frontend/test/frontend_tests/plugins/library_test.cljs b/frontend/test/frontend_tests/plugins/library_test.cljs index 2f8ecf6a82..982a62c566 100644 --- a/frontend/test/frontend_tests/plugins/library_test.cljs +++ b/frontend/test/frontend_tests/plugins/library_test.cljs @@ -457,7 +457,8 @@ library-id (uuid/next) stream (rx/subject) events (atom []) - state (atom {:current-file-id file-id :current-team-id team-id})] + state (atom {:current-file-id file-id :current-team-id team-id + :permissions {:can-edit true}})] (await (mock/with-mocks* {st/state state @@ -490,7 +491,8 @@ events (atom [])] (await (mock/with-mocks* - {st/state (atom {:current-file-id file-id :current-team-id (uuid/next)}) + {st/state (atom {:current-file-id file-id :current-team-id (uuid/next) + :permissions {:can-edit true}}) st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) rp/cmd! (shared-files-cmd [{:id (uuid/next) :name "Other published"}]) r/check-permission (mock/stub (constantly true))} @@ -504,7 +506,8 @@ events (atom [])] (await (mock/with-mocks* - {st/state (atom {:current-file-id file-id :current-team-id (uuid/next)}) + {st/state (atom {:current-file-id file-id :current-team-id (uuid/next) + :permissions {:can-edit true}}) st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) rp/cmd! (shared-files-cmd [{:id file-id :name "Current file"}]) r/check-permission (mock/stub (constantly true))} @@ -558,3 +561,29 @@ (into #{} (map #(obj/get % "$id")) (.-connected context)))) (t/is (= #{(str visited-id) (str other-id)} (into #{} (map #(.-id %)) (await (.availableLibraries context)))))))))) + +;; Scenario: a library is linked; while the list of published libraries +;; loads, a save fails and locks the workspace. Proves: the library is not +;; linked and the promise rejects with the read-only error. +(t/deftest ^:async connect-library-is-refused-when-a-save-fails-meanwhile + (let [library-id (uuid/next) + state (atom {:current-file-id (uuid/next) + :current-team-id (uuid/next) + :permissions {:can-edit true}}) + answer (rx/subject) + events (atom [])] + (await + (mock/with-mocks* + {st/state state + st/emit! (mock/stub (fn [& emitted] (swap! events into emitted))) + rp/cmd! (mock/stub (fn [cmd params] + (swap! mock/rpc-calls conj {:cmd cmd :params params}) + (->> answer (rx/take 1) (rx/observe-on :async))))} + (let [context (library/library-subcontext plugin-id) + result (settled (.connectLibrary context (str library-id)))] + (await (async/wait-for #(seq @mock/rpc-calls) "the published libraries are requested")) + (swap! state assoc-in [:workspace-global :persistence-lock] {:read-only? false}) + (rx/push! answer [{:id library-id :name "Published"}]) + (let [{:keys [rejected]} (await result)] + (t/is (str/includes? (ex-message rejected) "cannot be saved")) + (t/is (empty? @events) "the library is not linked"))))))) diff --git a/frontend/test/frontend_tests/plugins/management_test.cljs b/frontend/test/frontend_tests/plugins/management_test.cljs index 75889e3ec5..15d04f8f7f 100644 --- a/frontend/test/frontend_tests/plugins/management_test.cljs +++ b/frontend/test/frontend_tests/plugins/management_test.cljs @@ -306,7 +306,7 @@ [file* "name" nil] [file* "shared" 1]]] (t/is (thrown? js/Error (obj/set! proxy key value)) key)) - (with-redefs [r/check-permission (mock/stub (constantly false))] + (with-redefs [r/has-permission? (mock/stub (constantly false))] (doseq [[proxy key value permission] [[team* "name" "Brand" "manage:teams"] [project* "name" "Icons" "manage:projects"] [project* "pinned" true "manage:projects"] @@ -399,7 +399,7 @@ (t/is (= "Expected a name with 1 to 250 characters" (await (message (.duplicate file* #js {:name " "}))))) (t/is (= "Expected a project" (await (message (.moveTo file* #js {:id "invalid"}))))) (t/is (= "Expected a team" (await (message (.moveTo project* nil))))) - (with-redefs [r/check-permission (mock/stub (constantly false))] + (with-redefs [r/has-permission? (mock/stub (constantly false))] (doseq [[promise permission] [[(.createTeam (management/create-context plugin-id) #js {:name "Brand"}) "manage:teams"] [(.listProjects team*) "content:read"] [(.createProject team* #js {:name "Icons"}) "manage:projects"] diff --git a/frontend/test/frontend_tests/plugins/read_only_test.cljs b/frontend/test/frontend_tests/plugins/read_only_test.cljs new file mode 100644 index 0000000000..46dfa8db92 --- /dev/null +++ b/frontend/test/frontend_tests/plugins/read_only_test.cljs @@ -0,0 +1,211 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns frontend-tests.plugins.read-only-test + "Plugin writes in a read-only workspace." + (:require + [app.common.test-helpers.files :as cthf] + [app.common.uuid :as uuid] + [app.main.data.helpers :as dsh] + [app.main.data.workspace.media :as dwm] + [app.main.repo :as rp] + [app.main.store :as st] + [app.plugins.api :as api] + [app.plugins.register :as r] + [beicon.v2.core :as rx] + [cljs.test :as t :include-macros true] + [frontend-tests.helpers.async :as async] + [frontend-tests.helpers.mock :as mock] + [frontend-tests.helpers.state :as ths] + [frontend-tests.helpers.wasm :as thw])) + +;; Its validation flag is off, so only errors that always throw reach it +(def ^:private plugin-id "00000000-0000-0000-0000-000000000000") + +(defn- set-read-only + [store read-only?] + (swap! store assoc-in [:workspace-global :read-only?] read-only?)) + +(defn- shape-count + [store] + (count (dsh/lookup-page-objects @store))) + +(defn- with-context + "Calls `f` with a store holding a sample file and a plugin context on it." + [f] + (thw/with-wasm-mocks* + (fn [] + (let [store (ths/setup-store (cthf/sample-file :file1 :page-label :page1))] + (set! st/state store) + (f store (api/create-context plugin-id)))))) + +(t/deftest writes-throw-in-a-read-only-workspace + (with-context + (fn [store ^js context] + (let [^js board (.createBoard context)] + (set-read-only store true) + + (t/testing "a shape setter" + (t/is (thrown-with-msg? js/Error #"read-only" (set! (.-name board) "Renamed"))) + (t/is (not= "Renamed" (.-name board)))) + + (t/testing "a shape method" + (t/is (thrown-with-msg? js/Error #"read-only" (.bringToFront board)))) + + (t/testing "a context method without a permission check" + (let [before (shape-count store)] + (t/is (thrown-with-msg? js/Error #"read-only" (.createRectangle context))) + (t/is (= before (shape-count store))))) + + (t/testing "a context method with a permission check" + (t/is (thrown-with-msg? js/Error #"read-only" (.createPage (.-currentFile context))))))))) + +(t/deftest writes-work-again-once-the-workspace-is-editable + (with-context + (fn [store ^js context] + (let [^js board (.createBoard context)] + (set-read-only store true) + (set-read-only store false) + (set! (.-name board) "Renamed") + (t/is (= "Renamed" (.-name board))))))) + +(t/deftest a-save-failure-lock-says-why-the-workspace-is-read-only + (with-context + (fn [store ^js context] + (swap! store update :workspace-global assoc + :read-only? true + :persistence-lock {:read-only? false}) + (t/is (thrown-with-msg? js/Error #"cannot be saved" (.createBoard context)))))) + +(t/deftest reads-and-undo-blocks-work-in-a-read-only-workspace + (with-context + (fn [store ^js context] + (let [^js board (.createBoard context)] + (set-read-only store true) + (t/is (string? (.-name board))) + (let [block (.undoBlockBegin (.-history context))] + (t/is (some? block)) + (t/is (nil? (.undoBlockFinish (.-history context) block)))))))) + +(t/deftest read-only-keeps-permission-checks-of-reads + (with-context + (fn [store _] + (set-read-only store true) + (t/is (true? (r/check-permission plugin-id "content:read"))) + (t/is (true? (r/has-permission? plugin-id "content:write"))) + (t/is (thrown? js/Error (r/check-permission plugin-id "library:write")))))) + +;; Writes that wait for the server before they change the file + +(defn- settled + "Resolves with `{:resolved value}` or `{:rejected cause}` once `p` settles, + or with `{:timeout true}` when it stays pending for 2 seconds." + [p] + (js/Promise.race + #js [(.then p (fn [v] {:resolved v}) (fn [e] {:rejected e})) + (js/Promise. (fn [resolve] (js/setTimeout #(resolve {:timeout true}) 2000)))])) + +(defn- lock-saves + "Locks `store` as a failing save does." + [store] + (swap! store update :workspace-global assoc + :read-only? true + :persistence-lock {:read-only? false})) + +(defn- held-rpc + "An `rp/cmd!` that records each command in `calls` and answers with the + next value pushed to `answer`." + [calls answer] + (mock/stub (fn [cmd _] + (swap! calls conj cmd) + (->> answer (rx/take 1) (rx/observe-on :async))))) + +(def ^:private media-object + {:id (uuid/next) :name "image" :width 10 :height 10 :mtype "image/png"}) + +(def ^:private svg-with-image + (str "" + "" + "")) + +(defn- parsed-svg-with-image + "What `dwm/svg->clj` gives for `svg-with-image`. The test build cannot + load the XML parser, a CommonJS module." + [[name _]] + (rx/of {:tag :svg + :name name + :attrs {:xmlns "http://www.w3.org/2000/svg" :width "10" :height "10"} + :content [{:tag :image + :attrs {:href "data:image/png;base64,iVBORw0KGgo=" :width "10" :height "10"} + :content []}]})) + +(defn- read-only-error? + [cause] + (boolean (some->> cause ex-message (re-find #"cannot be saved")))) + +;; Scenario: an SVG with an image is created; while its image uploads a +;; save fails and locks the workspace. Proves: the shape is not added and +;; the promise rejects with the read-only error. +(t/deftest ^:async an-svg-whose-upload-ends-after-a-save-failure-is-refused + (thw/setup-wasm-mocks!) + (try + (let [store (ths/setup-store (cthf/sample-file :file1 :page-label :page1)) + calls (atom []) + answer (rx/subject)] + (await + (mock/with-mocks* + {st/state store + rp/cmd! (held-rpc calls answer) + dwm/valid-svg-string? (constantly true) + dwm/svg->clj (mock/stub parsed-svg-with-image)} + (let [^js context (api/create-context plugin-id) + before (shape-count store) + result (settled (.createShapeFromSvgWithImages context svg-with-image))] + (await (async/wait-for #(= [:upload-file-media-object] @calls) "the image upload starts")) + (lock-saves store) + (rx/push! answer media-object) + (let [{:keys [rejected]} (await result)] + (t/is (read-only-error? rejected)) + (await (async/settle)) + (t/is (= before (shape-count store)) "no shape is added")))))) + (finally + (thw/teardown-wasm-mocks!)))) + +;; Uploads `upload` with the context; while the request for `cmd` runs, a +;; save fails and locks the workspace. +(defn- ^:async upload-during-a-save-failure + [upload cmd] + (let [store (ths/setup-store (cthf/sample-file :file1 :page-label :page1)) + calls (atom []) + answer (rx/subject)] + (await + (mock/with-mocks* + {st/state store + rp/cmd! (held-rpc calls answer)} + ;; Bound one by one: a nested call with a local fn would compile to + ;; an awaited IIFE, which waits for the upload itself + (let [context (api/create-context plugin-id) + result (settled (upload context))] + (await (async/wait-for #(= [cmd] @calls) "the upload starts")) + (lock-saves store) + (rx/push! answer media-object) + (let [{:keys [rejected]} (await result)] + (t/is (read-only-error? rejected) (str cmd)))))))) + +;; Scenario: an image is uploaded from data and from a URL; while each +;; upload runs a save fails and locks the workspace. Proves: the promise +;; rejects with the read-only error. The media object stays on the server. +(t/deftest ^:async media-uploaded-after-a-save-failure-is-refused + (thw/setup-wasm-mocks!) + (try + (await (upload-during-a-save-failure + #(.uploadMediaData ^js % "image" (js/Uint8Array. #js [1 2 3]) "image/png") + :upload-file-media-object)) + (await (upload-during-a-save-failure + #(.uploadMediaUrl ^js % "image" "https://example.com/image.png") + :create-file-media-object-from-url)) + (finally + (thw/teardown-wasm-mocks!)))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 56b2dd7028..67d1fdd43f 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -91,6 +91,7 @@ [frontend-tests.plugins.page-active-validation-test] [frontend-tests.plugins.page-test] [frontend-tests.plugins.parser-test] + [frontend-tests.plugins.read-only-test] [frontend-tests.plugins.register-test] [frontend-tests.plugins.shape-bugfixes-test] [frontend-tests.plugins.text-test] @@ -281,6 +282,7 @@ 'frontend-tests.plugins.page-active-validation-test 'frontend-tests.plugins.page-test 'frontend-tests.plugins.parser-test + 'frontend-tests.plugins.read-only-test 'frontend-tests.plugins.register-test 'frontend-tests.plugins.shape-bugfixes-test 'frontend-tests.plugins.text-test diff --git a/mcp/packages/server/data/initial_instructions.md b/mcp/packages/server/data/initial_instructions.md index 66c2c642da..4050e084bb 100644 --- a/mcp/packages/server/data/initial_instructions.md +++ b/mcp/packages/server/data/initial_instructions.md @@ -24,6 +24,11 @@ So if you need to read the result of your executions/observe properties affected before trying to observe changes. Every `execude_code` call waits for updates before applying your code, so you never need to call `waitForLayoutUpdate` at the beginning of your code. +## Read-Only Mode + +While saves are failing, the workspace becomes read-only: every write through the API throws an error saying that the +workspace is read-only. Do not retry in a loop; wait and try again later, or tell the user. + # The Structure of Penpot Designs A Penpot design ultimately consists of shapes. diff --git a/plugins/CHANGELOG.md b/plugins/CHANGELOG.md index 7b667790fc..7c7aecbb57 100644 --- a/plugins/CHANGELOG.md +++ b/plugins/CHANGELOG.md @@ -18,6 +18,8 @@ - **plugin-types, plugin-runtime:** Dashboard property setters now update values after backend success and log failures without interrupting the workspace. File moves reject projects in other teams, backend rejections include their type and code, and `project.fileCount` stays at its listing snapshot. Duplicated projects include their file count and pinned state. - **plugin-runtime:** Cancel plugin loads before they execute after unloading or logout, and allow reopening plugins that close during startup. - **plugins-runtime**: `library.connectLibrary()` now resolves once the library's assets are loaded; it used to resolve with an empty library the first time. `library.connected` and `library.availableLibraries()` ignore files opened earlier in the same tab, which used to show up as connected and be left out of the available libraries. +- **plugins-runtime:** Writes now fail with an error while the workspace is read-only (saves failing, a user who cannot edit, a version preview), whatever the `throwValidationErrors` flag. They used to change the workspace with edits that would never be saved. Undo blocks still open and close. +- **plugins-runtime:** Writes that wait for the server (`createShapeFromSvgWithImages`, `uploadMediaData`, `uploadMediaUrl`, `connectLibrary`) check again when the server answers. If the workspace turned read-only meanwhile (a save failed), the promise rejects with the read-only error: no shape is added and no library is linked. An image the server already stored stays in the file's media, unused. - **plugins-runtime:** `LibraryComponent.remove()` now rejects linked-library and missing component handles, and does nothing for an already deleted component. - **plugins-runtime**: An interaction obtained from `Shape.interactions` now keeps addressing that interaction instead of the position it held when the array was read. Removing every interaction of a shape from a single read removes all of them rather than leaving some behind, and writing through a held interaction after an earlier one is removed no longer lands on a different interaction. - **plugins-runtime**: `Shape.removeInteraction()` now rejects an interaction belonging to a different shape with a validation error, instead of removing whichever interaction sat at the same position on the target shape.