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

🐛 Give branch access through the team role alone

Browse Source 
A branch must be open to every member of the team of its source file,
by role: owners, admins and editors open and edit it, viewers read it.
Two things got in the way.

Branch creation granted its creator the owner role on the branch file
(a file_profile_rel row), so a team editor became owner and admin of
their own branch. create-file-branch grants no file role any more: a
branch takes its permissions from the team and project roles alone.
materialize-file-branch, which turns a branch into an ordinary file,
now grants the owner role to the profile that runs it, as creating a
file does. Migration 0807 deletes the file roles of existing branches;
materialised files are no longer branches and keep theirs.

move-files moved a file but left its branches in the old project. The
permissions of a branch come from the project it lives in, so after a
move to another team its members got not-found and the workspace asked
them to request access. move-files now moves the branches of every
moved file with it, deleted branch rows included since their files
live on until the GC, and the broken library links cleanup already
covers them. It refuses to move a branch file on its own
(:cant-move-a-branch), which would part it from its source. Migration
0808 puts every branch back in the project of its source file and
drops the links of those branches to libraries of their old team.

Assigning a reviewer to a design review grants no access: the email
and the dashboard notification only let the reviewer know.

Tests cover a branch created with no file role, the creator keeping
editor rights, another editor saving to the branch, a viewer reading
it, the owner role granted on materialise, a lone branch refused by
move-files and an editor of the new team opening a moved branch.
This commit is contained in:
elhombretecla 2026-10-23 13:12:11 +02:00
parent c1dec5d4f0
commit 59d5817d1b
6 changed files with 164 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
backend/src/app/migrations.clj
View File

@ -545,7 +545,13 @@
:fn mg0805/migrate}
{:name "0806-add-file-branch-merge-table"
:fn (mg/resource "app/migrations/sql/0806-add-file-branch-merge-table.sql")}])
:fn (mg/resource "app/migrations/sql/0806-add-file-branch-merge-table.sql")}
{:name "0807-del-file-branch-profile-rels"
:fn (mg/resource "app/migrations/sql/0807-del-file-branch-profile-rels.sql")}
{:name "0808-mod-file-branch-project"
:fn (mg/resource "app/migrations/sql/0808-mod-file-branch-project.sql")}])
(defn apply-migrations!
[pool name migrations]

11
backend/src/app/migrations/sql/0807-del-file-branch-profile-rels.sql Normal file
View File

@ -0,0 +1,11 @@
-- A branch takes its permissions from the team and project roles alone:
-- every editor of main can open and edit it. Branch creation used to
-- grant its creator the owner role on the branch file, which lifted a
-- team editor to owner of that branch. Drop those grants. A branch that
-- was materialised is an ordinary file now (`is_branch` false) and
-- keeps its roles.
DELETE FROM file_profile_rel AS fpr
USING file AS f
WHERE f.id = fpr.file_id
AND f.is_branch IS TRUE;

22
backend/src/app/migrations/sql/0808-mod-file-branch-project.sql Normal file
View File

@ -0,0 +1,22 @@
-- A branch lives in the project of its source file: its permissions come
-- from that project and its team. Moving a file used to leave its
-- branches behind in the old project, so the members of the new team
-- could not open them. Put every branch back next to its source file.
UPDATE file AS bf
SET project_id = sf.project_id
FROM file_branch AS fb
JOIN file AS sf ON (sf.id = fb.source_file_id)
WHERE bf.id = fb.branch_file_id
AND bf.project_id <> sf.project_id;
-- A moved branch may link libraries of its old team: drop those links,
-- as moving a file does.
DELETE FROM file_library_rel AS rel
USING file AS f, project AS p, file AS lf, project AS lp
WHERE rel.file_id = f.id
AND f.is_branch IS TRUE
AND p.id = f.project_id
AND lf.id = rel.library_file_id
AND lp.id = lf.project_id
AND lp.team_id <> p.team_id;

26
backend/src/app/rpc/commands/files_branch.clj
View File

@ -42,6 +42,7 @@
[app.rpc.climit :as-alias climit]
[app.rpc.commands.files :as files]
[app.rpc.commands.files-branch-policies :as bp]
[app.rpc.commands.files-create :as fcreate]
[app.rpc.commands.files-update :as fupd]
[app.rpc.commands.teams :as teams]
[app.rpc.doc :as-alias doc]
@ -535,23 +536,16 @@
;; 3. Mark the new file row as a branch so it is
;; hidden from the project/team file listings.
;; No file role is granted: a branch takes its
;; permissions from the team (and project) roles
;; alone, so every editor of main can open and edit
;; it, and its creator gets no extra rights.
(db/update! conn :file
{:is-branch true}
{:id branch-id}
{::db/return-keys false})
;; 4. Grant the creator ownership (same grant
;; duplicate-file used to make).
(when (uuid? profile-id)
(db/insert! conn :file-profile-rel
{:file-id branch-id
:profile-id profile-id
:is-owner true
:is-admin true
:can-edit true}
{::db/return-keys false}))
;; 5. The branch resolves libraries exactly like
;; 4. The branch resolves libraries exactly like
;; main (component instances from shared libraries
;; must keep resolving on the derived state).
(doseq [rel (bfc/get-files-rels cfg #{file-id})]
@ -565,7 +559,7 @@
:library-file-id (:library-file-id rel)
:synced-at (or (:synced-at rel) ts)})))
;; 6. Persist the branch metadata. `base-revn`
;; 5. Persist the branch metadata. `base-revn`
;; tracks MAIN's revision counter and
;; `base-branch-revn` the BRANCH file's one. Each
;; starts at its own side's revn (main's current
@ -1711,6 +1705,12 @@
{:id branch-file-id}
{::db/return-keys false})
;; a branch carries no file roles; as an ordinary file it
;; gets the owner role its maker would get on creation
(fcreate/create-file-role! conn {:file-id branch-file-id
:profile-id profile-id
:role :owner})
;; the payload replaced the log
(db/delete! conn :file-branch-change {:branch-id (:id branch)})

22
backend/src/app/rpc/commands/management.clj
View File

@ -254,10 +254,18 @@
;; --- COMMAND: Move file
(def sql:get-files
"select id, features, project_id from file where id = ANY(?)")
"select id, features, project_id, is_branch from file where id = ANY(?)")
;; A branch lives in the project of its source file: its permissions
;; come from that project and its team, so the branches of a moved file
;; move with it. The deleted branch rows are included, their files still
;; exist until the GC takes them.
(def sql:move-files
"update file set project_id = ? where id = ANY(?)")
"update file set project_id = ?
where id = ANY(?)
or id in (select fb.branch_file_id
from file_branch as fb
where fb.source_file_id = ANY(?))")
(def sql:delete-broken-relations
"with broken as (
@ -289,6 +297,12 @@
:code :cant-move-to-same-project
:hint "Unable to move a file to the same project"))
;; a branch only moves together with its source file
(when (some :is-branch files)
(ex/raise :type :validation
:code :cant-move-a-branch
:hint "Unable to move a branch file on its own"))
;; Check if we have permissions on the destination project
(proj/check-edition-permissions! conn profile-id project-id)
@ -306,8 +320,8 @@
(doseq [file files]
(cfeat/check-file-features! features (:features file)))))
;; move all files to the project
(db/exec-one! conn [sql:move-files project-id fids])
;; move all files, and their branches, to the project
(db/exec-one! conn [sql:move-files project-id fids fids])
;; delete possible broken relations on moved files
(db/exec-one! conn [sql:delete-broken-relations pids])

93
backend/test/backend_tests/rpc_file_branch_test.clj
View File

@ -1362,6 +1362,94 @@
;; the colour differ from the new base and lands in the op log
(t/is (empty? (th/db-query :file-branch-change {:branch-id branch-id})))))))
(t/deftest branch-permissions-come-from-the-team-role
(with-redefs [cf/flags (conj cf/flags :branching)]
(let [owner (th/create-profile* 1 {:is-active true})
editor1 (th/create-profile* 2 {:is-active true})
editor2 (th/create-profile* 3 {:is-active true})
viewer (th/create-profile* 4 {:is-active true})
team-id (:default-team-id owner)
file (th/create-file* 1 {:profile-id (:id owner)
:project-id (:default-project-id owner)})
_ (doseq [[profile role] [[editor1 :editor] [editor2 :editor] [viewer :viewer]]]
(th/create-team-role* {:team-id team-id
:profile-id (:id profile)
:role role}))
create (:result (th/command! {::th/type :create-file-branch
::rpc/profile-id (:id editor1)
:file-id (:id file)
:name "team-roles"}))
branch-file-id (:branch-file-id create)
perms-of (fn [profile]
(db/run! th/*system*
#(bfc/get-file-permissions % (:id profile) branch-file-id)))]
(t/testing "creating a branch grants no file role"
(t/is (empty? (th/db-query :file-profile-rel {:file-id branch-file-id}))))
(t/testing "the creator keeps the rights of a team editor"
(let [perms (perms-of editor1)]
(t/is (true? (:can-edit perms)))
(t/is (false? (:is-admin perms)))
(t/is (false? (:is-owner perms)))))
(t/testing "another team editor can edit the branch"
(let [bf (th/db-get :file {:id branch-file-id})
out (th/command! {::th/type :update-file
::rpc/profile-id (:id editor2)
:id branch-file-id
:session-id (uuid/random)
:revn (:revn bf)
:vern (:vern bf)
:features cfeat/supported-features
:changes [{:type :add-page :id (uuid/random) :name "by-editor2"}]})]
(t/is (nil? (:error out)))))
(t/testing "a team viewer can read the branch but not edit it"
(let [perms (perms-of viewer)]
(t/is (true? (:can-read perms)))
(t/is (false? (:can-edit perms))))))))
(t/deftest branches-move-with-their-source-file
(with-redefs [cf/flags (conj cf/flags :branching)]
(let [owner (th/create-profile* 1 {:is-active true})
editor (th/create-profile* 2 {:is-active true})
team (th/create-team* 1 {:profile-id (:id owner)})
project (th/create-project* 1 {:team-id (:id team)
:profile-id (:id owner)})
file (th/create-file* 1 {:profile-id (:id owner)
:project-id (:default-project-id owner)})
_ (th/create-team-role* {:team-id (:id team)
:profile-id (:id editor)
:role :editor})
create (:result (th/command! {::th/type :create-file-branch
::rpc/profile-id (:id owner)
:file-id (:id file)
:name "travels"}))
branch-file-id (:branch-file-id create)]
(t/testing "a branch cannot be moved on its own"
(let [out (th/command! {::th/type :move-files
::rpc/profile-id (:id owner)
:project-id (:id project)
:ids #{branch-file-id}})]
(t/is (th/ex-of-code? (:error out) :cant-move-a-branch))))
(t/testing "moving the source file takes its branches along"
(let [out (th/command! {::th/type :move-files
::rpc/profile-id (:id owner)
:project-id (:id project)
:ids #{(:id file)}})]
(t/is (nil? (:error out))))
(t/is (= (:id project) (:project-id (th/db-get :file {:id branch-file-id})))))
(t/testing "an editor of the new team can open the branch"
(let [out (th/command! {::th/type :get-file
::rpc/profile-id (:id editor)
:id branch-file-id})]
(t/is (nil? (:error out)))
(t/is (true? (-> out :result :permissions :can-edit))))))))
(t/deftest materialize-branch-into-an-ordinary-file
(with-redefs [cf/flags (conj cf/flags :branching)]
(let [profile (th/create-profile* 1 {:is-active true})
@ -1407,6 +1495,11 @@
(let [row (th/db-get :file {:id branch-file-id})]
(t/is (false? (:is-branch row))))
;; as an ordinary file it gets the owner role of its maker
(let [rel (th/db-get :file-profile-rel {:file-id branch-file-id
:profile-id (:id profile)})]
(t/is (true? (:is-owner rel))))
;; the payload lands where the ordinary write path puts it, which
;; is the `file_data` row of type "main" rather than the legacy
;; `file.data` column