diff --git a/.serena/memories/backend/auth-permissions-product-domains.md b/.serena/memories/backend/auth-permissions-product-domains.md index c3057118d5..130cd62609 100644 --- a/.serena/memories/backend/auth-permissions-product-domains.md +++ b/.serena/memories/backend/auth-permissions-product-domains.md @@ -15,6 +15,7 @@ - `app.rpc.permissions` provides predicate/check factories. Failed permission checks intentionally raise `:not-found` / `:object-not-found`, not an authorization-specific error, to avoid leaking object existence. - Team role flags are normalized as owner > admin > editor > viewer. Owner/admin imply edit; any membership row implies read. +- File/project role rows only count with a live team membership: the file/project permission queries require a matching `team_profile_rel` row, and leaving or removing a member deletes their team-scoped `file`/`project`/`team-project-profile-rel` rows (GHSA-v9r9-h77c-55m2). - File/project/comment checks are implemented in the owning command namespaces, often via helpers imported from `files`, `teams`, or `projects`; do not bypass those helpers with direct DB lookups unless preserving their not-found semantics. - Comment permission includes both logged-in state and the file/team comment policy. Shared viewer paths may pass `share-id`; preserve that path when changing comment queries. diff --git a/backend/src/app/binfile/common.clj b/backend/src/app/binfile/common.clj index 3e4402be92..e2aed9a837 100644 --- a/backend/src/app/binfile/common.clj +++ b/backend/src/app/binfile/common.clj @@ -344,8 +344,13 @@ fpr.can_edit from file_profile_rel as fpr inner join file as f on (f.id = fpr.file_id) + inner join project as p on (p.id = f.project_id) where fpr.file_id = ? and fpr.profile_id = ? + and exists (select 1 + from team_profile_rel as tpr + where tpr.team_id = p.team_id + and tpr.profile_id = fpr.profile_id) union all select tpr.is_owner, tpr.is_admin, @@ -361,8 +366,13 @@ ppr.can_edit from project_profile_rel as ppr inner join file as f on (f.project_id = ppr.project_id) + inner join project as p on (p.id = ppr.project_id) where f.id = ? - and ppr.profile_id = ?") + and ppr.profile_id = ? + and exists (select 1 + from team_profile_rel as tpr + where tpr.team_id = p.team_id + and tpr.profile_id = ppr.profile_id)") (defn- get-file-permissions* [conn profile-id file-id] diff --git a/backend/src/app/migrations.clj b/backend/src/app/migrations.clj index 2edb8614d5..13f99170fe 100644 --- a/backend/src/app/migrations.clj +++ b/backend/src/app/migrations.clj @@ -499,7 +499,10 @@ :fn (mg/resource "app/migrations/sql/0152-improve-uuid-defaults-and-drop-extension.sql")} {:name "0152-rename-version-and-add-indexes-to-server-error-report" - :fn (mg/resource "app/migrations/sql/0152-rename-version-and-add-indexes-to-server-error-report.sql")}]) + :fn (mg/resource "app/migrations/sql/0152-rename-version-and-add-indexes-to-server-error-report.sql")} + + {:name "0153-del-orphan-profile-rels-after-team-leave" + :fn (mg/resource "app/migrations/sql/0153-del-orphan-profile-rels-after-team-leave.sql")}]) (defn apply-migrations! [pool name migrations] diff --git a/backend/src/app/migrations/sql/0153-del-orphan-profile-rels-after-team-leave.sql b/backend/src/app/migrations/sql/0153-del-orphan-profile-rels-after-team-leave.sql new file mode 100644 index 0000000000..41f30d7796 --- /dev/null +++ b/backend/src/app/migrations/sql/0153-del-orphan-profile-rels-after-team-leave.sql @@ -0,0 +1,27 @@ +-- Remove team-scoped role rows whose profile is no longer a member of +-- the team that owns the project/file. Leftover from before +-- leave-team/delete-team-member cleaned them up (GHSA-v9r9-h77c-55m2). +-- Rows with a live team membership are kept. + +DELETE FROM file_profile_rel AS fpr +USING file AS f +INNER JOIN project AS p ON (p.id = f.project_id) +WHERE fpr.file_id = f.id + AND NOT EXISTS (SELECT 1 + FROM team_profile_rel AS tpr + WHERE tpr.team_id = p.team_id + AND tpr.profile_id = fpr.profile_id); + +DELETE FROM project_profile_rel AS ppr +USING project AS p +WHERE ppr.project_id = p.id + AND NOT EXISTS (SELECT 1 + FROM team_profile_rel AS tpr + WHERE tpr.team_id = p.team_id + AND tpr.profile_id = ppr.profile_id); + +DELETE FROM team_project_profile_rel AS tppr +WHERE NOT EXISTS (SELECT 1 + FROM team_profile_rel AS tpr + WHERE tpr.team_id = tppr.team_id + AND tpr.profile_id = tppr.profile_id); diff --git a/backend/src/app/rpc/commands/projects.clj b/backend/src/app/rpc/commands/projects.clj index 05d5ae79aa..ab678db840 100644 --- a/backend/src/app/rpc/commands/projects.clj +++ b/backend/src/app/rpc/commands/projects.clj @@ -41,8 +41,13 @@ ppr.is_admin, ppr.can_edit from project_profile_rel as ppr + inner join project as p on (p.id = ppr.project_id) where ppr.project_id = ? - and ppr.profile_id = ?") + and ppr.profile_id = ? + and exists (select 1 + from team_profile_rel as tpr + where tpr.team_id = p.team_id + and tpr.profile_id = ppr.profile_id)") (defn- get-permissions [conn profile-id project-id] @@ -143,7 +148,12 @@ inner join team as t on (t.id = p2.team_id) where p2.id in (select project_id from project_profile_rel as ppr + inner join project as p on (p.id = ppr.project_id) where ppr.profile_id = ? + and exists (select 1 + from team_profile_rel as tpr + where tpr.team_id = p.team_id + and tpr.profile_id = ppr.profile_id) and (ppr.can_edit = true or ppr.is_owner = true or ppr.is_admin = true)) diff --git a/backend/src/app/rpc/commands/search.clj b/backend/src/app/rpc/commands/search.clj index 7b60e6db30..463bfb4abd 100644 --- a/backend/src/app/rpc/commands/search.clj +++ b/backend/src/app/rpc/commands/search.clj @@ -32,6 +32,10 @@ where ppr.profile_id = ? and p.team_id = ? and (p.deleted_at is null) + and exists (select 1 + from team_profile_rel as tpr + where tpr.team_id = p.team_id + and tpr.profile_id = ppr.profile_id) and (ppr.is_admin = true or ppr.is_owner = true or ppr.can_edit = true) diff --git a/backend/src/app/rpc/commands/teams.clj b/backend/src/app/rpc/commands/teams.clj index 9c13d6b37d..1a329773f2 100644 --- a/backend/src/app/rpc/commands/teams.clj +++ b/backend/src/app/rpc/commands/teams.clj @@ -732,6 +732,37 @@ ;; --- Mutation: Leave Team +;; Remove every team-scoped relation of a profile that leaves (or is +;; removed from) a team. File and project roles hang from the team +;; membership: keeping them would let the former member keep reading +;; and editing files through `sql:file-permissions`. Pins are personal +;; state of the same team, so they go too. Other teams are untouched, +;; and rejoining grants fresh roles. Runs inside the caller's +;; transaction. +(def ^:private sql:delete-team-file-rels + "DELETE FROM file_profile_rel AS fpr + USING file AS f + INNER JOIN project AS p ON (p.id = f.project_id) + WHERE fpr.file_id = f.id + AND p.team_id = ? + AND fpr.profile_id = ?") + +(def ^:private sql:delete-team-project-rels + "DELETE FROM project_profile_rel AS ppr + USING project AS p + WHERE ppr.project_id = p.id + AND p.team_id = ? + AND ppr.profile_id = ?") + +(defn- delete-team-rels + [conn profile-id team-id] + (db/exec! conn [sql:delete-team-file-rels team-id profile-id]) + (db/exec! conn [sql:delete-team-project-rels team-id profile-id]) + (db/delete! conn :team-project-profile-rel {:profile-id profile-id + :team-id team-id}) + (db/delete! conn :team-profile-rel {:profile-id profile-id + :team-id team-id})) + (defn leave-team [{:keys [::db/conn ::mbus/msgbus]} {:keys [profile-id id reassign-to]}] (let [perms (get-permissions conn profile-id id) @@ -782,9 +813,7 @@ :code :owner-cant-leave-team :hint "releasing owner before leave")) - (db/delete! conn :team-profile-rel - {:profile-id profile-id - :team-id id}) + (delete-team-rels conn profile-id id) nil)) @@ -966,8 +995,7 @@ (ex/raise :type :validation :code :cant-remove-owner)) - (db/delete! conn :team-profile-rel {:profile-id member-id - :team-id team-id}) + (delete-team-rels conn member-id team-id) ;; A removed member that owns the organization of this team keeps ;; read-only access to it, so instead of kicking them out we degrade diff --git a/backend/test/backend_tests/helpers.clj b/backend/test/backend_tests/helpers.clj index 25aa8b20bc..a9dd895176 100644 --- a/backend/test/backend_tests/helpers.clj +++ b/backend/test/backend_tests/helpers.clj @@ -299,9 +299,7 @@ ([params] (create-project-role* *system* params)) ([system {:keys [project-id profile-id role] :or {role :owner}}] (dm/with-open [conn (db/open system)] - (#'teams/create-project-role conn {:project-id project-id - :profile-id profile-id - :role role})))) + (#'teams/create-project-role conn profile-id project-id role)))) (defn create-file-role* ([params] (create-file-role* *system* params)) diff --git a/backend/test/backend_tests/rpc_team_test.clj b/backend/test/backend_tests/rpc_team_test.clj index 1c30415db2..0665c92570 100644 --- a/backend/test/backend_tests/rpc_team_test.clj +++ b/backend/test/backend_tests/rpc_team_test.clj @@ -1471,3 +1471,315 @@ (t/is (not (th/success? out))) (t/is (th/ex-of-type? (:error out) :not-found)) (t/is (th/ex-of-code? (:error out) :member-does-not-exist))))) + +(t/deftest leaver-loses-file-access + ;; GHSA-v9r9-h77c-55m2: leaving the team must revoke the file and + ;; project roles granted in that team. + (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)})] + + (th/create-team-role* {:team-id (:id team) + :profile-id (:id editor) + :role :editor}) + + (let [project (th/create-project* 1 {:profile-id (:id editor) + :team-id (:id team)}) + file (th/create-file* 1 {:profile-id (:id editor) + :project-id (:id project)})] + + (t/testing "editor can rename and read summary before leaving" + (let [out (th/command! {::th/type :rename-file + ::rpc/profile-id (:id editor) + :id (:id file) + :name "renamed"})] + (t/is (th/success? out))) + (let [out (th/command! {::th/type :get-file-summary + ::rpc/profile-id (:id editor) + :id (:id file)})] + (t/is (th/success? out)))) + + (t/testing "leave team" + (let [out (th/command! {::th/type :leave-team + ::rpc/profile-id (:id editor) + :id (:id team)})] + (t/is (th/success? out)))) + + (t/testing "renaming after leaving fails with not-found" + (let [out (th/command! {::th/type :rename-file + ::rpc/profile-id (:id editor) + :id (:id file) + :name "renamed-again"}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + + (t/testing "file summary after leaving fails with not-found" + (let [out (th/command! {::th/type :get-file-summary + ::rpc/profile-id (:id editor) + :id (:id file)}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + + (t/testing "get-file after leaving fails with not-found" + (let [out (th/command! {::th/type :get-file + ::rpc/profile-id (:id editor) + :id (:id file) + :components-v2 true}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found))))))) + +(t/deftest removed-member-loses-file-access + ;; Same as above through the expulsion path. + (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)})] + + (th/create-team-role* {:team-id (:id team) + :profile-id (:id editor) + :role :editor}) + + (let [project (th/create-project* 1 {:profile-id (:id editor) + :team-id (:id team)}) + file (th/create-file* 1 {:profile-id (:id editor) + :project-id (:id project)})] + + (t/testing "remove member" + (let [out (th/command! {::th/type :delete-team-member + ::rpc/profile-id (:id owner) + :team-id (:id team) + :member-id (:id editor)})] + (t/is (th/success? out)))) + + (t/testing "renaming after removal fails with not-found" + (let [out (th/command! {::th/type :rename-file + ::rpc/profile-id (:id editor) + :id (:id file) + :name "renamed-again"}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + + (t/testing "file summary after removal fails with not-found" + (let [out (th/command! {::th/type :get-file-summary + ::rpc/profile-id (:id editor) + :id (:id file)}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found))))))) + +(t/deftest orphan-file-role-without-team-membership-grants-nothing + ;; Defense in depth: a file role row without a live team membership + ;; (leftover from before the leave-team cleanup) grants no access. + (let [owner (th/create-profile* 1 {:is-active true}) + stranger (th/create-profile* 2 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)}) + project (th/create-project* 1 {:profile-id (:id owner) + :team-id (:id team)}) + file (th/create-file* 1 {:profile-id (:id owner) + :project-id (:id project)})] + + ;; Plant an orphan file role for a profile that was never a member. + (th/create-file-role* {:file-id (:id file) + :profile-id (:id stranger) + :role :editor}) + + (t/testing "orphan file role cannot rename" + (let [out (th/command! {::th/type :rename-file + ::rpc/profile-id (:id stranger) + :id (:id file) + :name "renamed"}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + + (t/testing "orphan file role cannot read summary" + (let [out (th/command! {::th/type :get-file-summary + ::rpc/profile-id (:id stranger) + :id (:id file)}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))))) + +(t/deftest leaver-loses-project-access + ;; Same revocation at project level (covers sql:project-permissions). + (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)})] + + (th/create-team-role* {:team-id (:id team) + :profile-id (:id editor) + :role :editor}) + + (let [project (th/create-project* 1 {:profile-id (:id editor) + :team-id (:id team)})] + + (t/testing "leave team" + (let [out (th/command! {::th/type :leave-team + ::rpc/profile-id (:id editor) + :id (:id team)})] + (t/is (th/success? out)))) + + (t/testing "get-project after leaving fails with not-found" + (let [out (th/command! {::th/type :get-project + ::rpc/profile-id (:id editor) + :id (:id project)}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + + (t/testing "rename-project after leaving fails with not-found" + (let [out (th/command! {::th/type :rename-project + ::rpc/profile-id (:id editor) + :id (:id project) + :name "renamed"}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found))))))) + +(t/deftest leaver-vanishes-from-search + ;; The old team's files are no longer searchable by the leaver. + (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)})] + + (th/create-team-role* {:team-id (:id team) + :profile-id (:id editor) + :role :editor}) + + (let [project (th/create-project* 1 {:profile-id (:id editor) + :team-id (:id team)}) + file (th/create-file* 1 {:profile-id (:id editor) + :project-id (:id project) + :name "searchable-file"})] + + (t/testing "member finds the file before leaving" + (let [out (th/command! {::th/type :search-files + ::rpc/profile-id (:id editor) + :team-id (:id team) + :search-term "searchable"})] + (t/is (th/success? out)) + (t/is (= (:id file) (-> out :result first :id))))) + + (t/testing "leave team" + (let [out (th/command! {::th/type :leave-team + ::rpc/profile-id (:id editor) + :id (:id team)})] + (t/is (th/success? out)))) + + (t/testing "search on the old team fails with not-found" + (let [out (th/command! {::th/type :search-files + ::rpc/profile-id (:id editor) + :team-id (:id team) + :search-term "searchable"}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found))))))) + +(t/deftest pins-are-cleaned-on-leave + ;; The project pin (team-project-profile-rel) goes with the leaver. + (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)})] + + (th/create-team-role* {:team-id (:id team) + :profile-id (:id editor) + :role :editor}) + + (let [project (th/create-project* 1 {:profile-id (:id editor) + :team-id (:id team)})] + + (t/testing "pin the project" + (let [out (th/command! {::th/type :update-project-pin + ::rpc/profile-id (:id editor) + :id (:id project) + :team-id (:id team) + :is-pinned true})] + (t/is (th/success? out))) + (t/is (= 1 (count (th/db-query :team-project-profile-rel + {:team-id (:id team) + :profile-id (:id editor)}))))) + + (t/testing "leave team removes the pin row" + (let [out (th/command! {::th/type :leave-team + ::rpc/profile-id (:id editor) + :id (:id team)})] + (t/is (th/success? out))) + (t/is (= 0 (count (th/db-query :team-project-profile-rel + {:team-id (:id team) + :profile-id (:id editor)})))))))) + +(t/deftest leaving-one-team-keeps-other-team-access + ;; Deletes are scoped by team: leaving A keeps B's files usable. + (let [owner (th/create-profile* 1 {:is-active true}) + editor (th/create-profile* 2 {:is-active true}) + team-a (th/create-team* 1 {:profile-id (:id owner)}) + team-b (th/create-team* 2 {:profile-id (:id owner)})] + + (th/create-team-role* {:team-id (:id team-a) + :profile-id (:id editor) + :role :editor}) + (th/create-team-role* {:team-id (:id team-b) + :profile-id (:id editor) + :role :editor}) + + (let [project-a (th/create-project* 1 {:profile-id (:id editor) + :team-id (:id team-a)}) + file-a (th/create-file* 1 {:profile-id (:id editor) + :project-id (:id project-a)}) + project-b (th/create-project* 2 {:profile-id (:id editor) + :team-id (:id team-b)}) + file-b (th/create-file* 2 {:profile-id (:id editor) + :project-id (:id project-b)})] + + (t/testing "leave team A" + (let [out (th/command! {::th/type :leave-team + ::rpc/profile-id (:id editor) + :id (:id team-a)})] + (t/is (th/success? out)))) + + (t/testing "team A file is gone" + (let [out (th/command! {::th/type :rename-file + ::rpc/profile-id (:id editor) + :id (:id file-a) + :name "renamed"}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + + (t/testing "team B file still works" + (let [out (th/command! {::th/type :rename-file + ::rpc/profile-id (:id editor) + :id (:id file-b) + :name "renamed"})] + (t/is (th/success? out))))))) + +(t/deftest orphan-project-role-grants-nothing-and-lists-nothing + ;; Mirror of the file orphan test at project level, including the + ;; get-all-projects listing (covers the F1 guard). + (let [owner (th/create-profile* 1 {:is-active true}) + stranger (th/create-profile* 2 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)}) + project (th/create-project* 1 {:profile-id (:id owner) + :team-id (:id team)})] + + ;; Plant an orphan project role for a profile that was never a member. + (th/create-project-role* {:project-id (:id project) + :profile-id (:id stranger) + :role :editor}) + + (t/testing "orphan project role cannot read the project" + (let [out (th/command! {::th/type :get-project + ::rpc/profile-id (:id stranger) + :id (:id project)}) + error (:error out)] + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + + (t/testing "orphan project is not listed" + (let [out (th/command! {::th/type :get-all-projects + ::rpc/profile-id (:id stranger)})] + (t/is (th/success? out)) + (t/is (nil? (some #(= (:id project) (:id %)) (:result out))))))))