diff --git a/.serena/memories/frontend/plugin-api-to-cljs-binding.md b/.serena/memories/frontend/plugin-api-to-cljs-binding.md index fc641a85e7..8c460d270b 100644 --- a/.serena/memories/frontend/plugin-api-to-cljs-binding.md +++ b/.serena/memories/frontend/plugin-api-to-cljs-binding.md @@ -23,7 +23,8 @@ - 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. - `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/frontend/src/app/plugins/api.cljs b/frontend/src/app/plugins/api.cljs index 226b324c1f..98100b827c 100644 --- a/frontend/src/app/plugins/api.cljs +++ b/frontend/src/app/plugins/api.cljs @@ -64,6 +64,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 @@ -201,6 +202,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 @@ -257,6 +259,7 @@ :uploadMediaUrl (fn [name url] + (r/check-writable) (cond (not (string? name)) (u/not-valid plugin-id :uploadMedia-name name) @@ -276,6 +279,7 @@ :uploadMediaData (fn [name data mime-type] + (r/check-writable) (let [file-id (:current-file-id @st/state)] (js/Promise. (fn [resolve reject] @@ -309,6 +313,7 @@ :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) @@ -339,6 +344,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)) @@ -378,6 +384,7 @@ :createPath (fn [] + (r/check-writable) (let [page (dsh/lookup-page @st/state) shape (cts/setup-shape {:type :path @@ -395,6 +402,7 @@ :createText (fn [text] + (r/check-writable) (cond (not (string? text)) (u/not-valid plugin-id :createText text) @@ -432,6 +440,7 @@ :createShapeFromSvg (fn [svg-string] + (r/check-writable) (cond (not (dwm/valid-svg-string? svg-string)) (u/not-valid plugin-id :createShapeFromSvg svg-string) @@ -446,6 +455,7 @@ :createShapeFromSvgWithImages (fn [svg-string] + (r/check-writable) (js/Promise. (fn [resolve reject] (cond @@ -466,6 +476,7 @@ :createBoolean (fn [bool-type shapes] + (r/check-writable) (let [bool-type (keyword bool-type)] (cond (not (contains? cts/bool-types bool-type)) @@ -595,6 +606,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}) @@ -632,6 +644,7 @@ :alignHorizontal (fn [shapes direction] + (r/check-writable) (let [dir (case direction "left" :hleft "center" :hcenter @@ -650,6 +663,7 @@ :alignVertical (fn [shapes direction] + (r/check-writable) (let [dir (case direction "top" :vtop "center" :vcenter @@ -668,6 +682,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") @@ -678,6 +693,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") @@ -688,6 +704,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") @@ -703,6 +720,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/register.cljs b/frontend/src/app/plugins/register.cljs index c70b4f9e7a..3e1be7a356 100644 --- a/frontend/src/app/plugins/register.cljs +++ b/frontend/src/app/plugins/register.cljs @@ -259,13 +259,37 @@ (.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] (or (= plugin-id "00000000-0000-0000-0000-000000000000") (= plugin-id mcp-plugin-id) (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 2eaa390e5a..148b17054a 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/read_only_test.cljs b/frontend/test/frontend_tests/plugins/read_only_test.cljs new file mode 100644 index 0000000000..566193e3a8 --- /dev/null +++ b/frontend/test/frontend_tests/plugins/read_only_test.cljs @@ -0,0 +1,93 @@ +;; 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.main.data.helpers :as dsh] + [app.main.store :as st] + [app.plugins.api :as api] + [app.plugins.register :as r] + [cljs.test :as t :include-macros true] + [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")))))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 83f1f71195..3868f08487 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -84,6 +84,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] @@ -243,6 +244,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 be9177619c..68f5cf2ed1 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 de0f09ed73..bc46db545d 100644 --- a/plugins/CHANGELOG.md +++ b/plugins/CHANGELOG.md @@ -12,6 +12,7 @@ ### 🩹 Fixes +- **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:** `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.