mirror of
https://github.com/penpot/penpot.git
synced 2026-11-05 17:40:39 +00:00
✨ 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.
This commit is contained in:
parent
73337d6af8
commit
4c9d30ef6a
@ -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)`.
|
||||
|
||||
|
||||
@ -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))
|
||||
|
||||
@ -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)
|
||||
|
||||
@ -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]))
|
||||
|
||||
@ -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")
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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)
|
||||
|
||||
@ -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
|
||||
|
||||
93
frontend/test/frontend_tests/plugins/read_only_test.cljs
Normal file
93
frontend/test/frontend_tests/plugins/read_only_test.cljs
Normal file
@ -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"))))))
|
||||
@ -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
|
||||
|
||||
@ -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.
|
||||
|
||||
@ -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.
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user