From 59d5817d1be13805597cdd7fadb399480967d696 Mon Sep 17 00:00:00 2001 From: elhombretecla Date: Fri, 23 Oct 2026 13:12:11 +0200 Subject: [PATCH] :bug: Give branch access through the team role alone 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. --- backend/src/app/migrations.clj | 8 +- .../sql/0807-del-file-branch-profile-rels.sql | 11 +++ .../sql/0808-mod-file-branch-project.sql | 22 +++++ backend/src/app/rpc/commands/files_branch.clj | 26 +++--- backend/src/app/rpc/commands/management.clj | 22 ++++- .../backend_tests/rpc_file_branch_test.clj | 93 +++++++++++++++++++ 6 files changed, 164 insertions(+), 18 deletions(-) create mode 100644 backend/src/app/migrations/sql/0807-del-file-branch-profile-rels.sql create mode 100644 backend/src/app/migrations/sql/0808-mod-file-branch-project.sql diff --git a/backend/src/app/migrations.clj b/backend/src/app/migrations.clj index ddb2502c23..2dcdf803b7 100644 --- a/backend/src/app/migrations.clj +++ b/backend/src/app/migrations.clj @@ -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] diff --git a/backend/src/app/migrations/sql/0807-del-file-branch-profile-rels.sql b/backend/src/app/migrations/sql/0807-del-file-branch-profile-rels.sql new file mode 100644 index 0000000000..6be81c66e3 --- /dev/null +++ b/backend/src/app/migrations/sql/0807-del-file-branch-profile-rels.sql @@ -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; diff --git a/backend/src/app/migrations/sql/0808-mod-file-branch-project.sql b/backend/src/app/migrations/sql/0808-mod-file-branch-project.sql new file mode 100644 index 0000000000..3ddd5546d8 --- /dev/null +++ b/backend/src/app/migrations/sql/0808-mod-file-branch-project.sql @@ -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; diff --git a/backend/src/app/rpc/commands/files_branch.clj b/backend/src/app/rpc/commands/files_branch.clj index 8dfd51f4c2..8bca07333f 100644 --- a/backend/src/app/rpc/commands/files_branch.clj +++ b/backend/src/app/rpc/commands/files_branch.clj @@ -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)}) diff --git a/backend/src/app/rpc/commands/management.clj b/backend/src/app/rpc/commands/management.clj index 62d5a88330..378bf88f89 100644 --- a/backend/src/app/rpc/commands/management.clj +++ b/backend/src/app/rpc/commands/management.clj @@ -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]) diff --git a/backend/test/backend_tests/rpc_file_branch_test.clj b/backend/test/backend_tests/rpc_file_branch_test.clj index fed5b53d70..7486638688 100644 --- a/backend/test/backend_tests/rpc_file_branch_test.clj +++ b/backend/test/backend_tests/rpc_file_branch_test.clj @@ -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