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