diff --git a/backend/src/app/rpc/commands/comments.clj b/backend/src/app/rpc/commands/comments.clj index 6a926d1e98..b9c6002eb4 100644 --- a/backend/src/app/rpc/commands/comments.clj +++ b/backend/src/app/rpc/commands/comments.clj @@ -231,8 +231,11 @@ ::sm/params schema:get-comment-threads} [cfg {:keys [::rpc/profile-id file-id share-id] :as params}] (db/run! cfg (fn [{:keys [::db/conn] :as cfg}] - (files/check-comment-permissions! cfg profile-id file-id share-id) - (get-comment-threads conn profile-id file-id)))) + (let [perms (files/check-comment-permissions! cfg profile-id file-id share-id) + threads (get-comment-threads conn profile-id file-id)] + (if (= :share-link (:type perms)) + (filterv #(contains? (:pages perms) (:page-id %)) threads) + threads))))) (defn- get-comment-threads-sql [where] @@ -329,9 +332,15 @@ ::sm/params schema:get-comment-thread} [cfg {:keys [::rpc/profile-id file-id id share-id] :as params}] (db/run! cfg (fn [{:keys [::db/conn] :as cfg}] - (files/check-comment-permissions! cfg profile-id file-id share-id) - (some-> (db/exec-one! conn [sql:get-comment-thread profile-id file-id id]) - (decode-row))))) + (let [perms (files/check-comment-permissions! cfg profile-id file-id share-id) + thread (some-> (db/exec-one! conn [sql:get-comment-thread profile-id file-id id]) + (decode-row))] + (when (and thread (= :share-link (:type perms))) + (when-not (contains? (:pages perms) (:page-id thread)) + (ex/raise :type :not-found + :code :object-not-found + :hint "not found"))) + thread)))) ;; --- COMMAND: Retrieve Comments @@ -348,8 +357,13 @@ ::sm/params schema:get-comments} [cfg {:keys [::rpc/profile-id thread-id share-id]}] (db/run! cfg (fn [{:keys [::db/conn] :as cfg}] - (let [{:keys [file-id]} (get-comment-thread conn thread-id)] - (files/check-comment-permissions! cfg profile-id file-id share-id) + (let [{:keys [file-id page-id]} (get-comment-thread conn thread-id) + perms (files/check-comment-permissions! cfg profile-id file-id share-id)] + (when (and (= :share-link (:type perms)) + (not (contains? (:pages perms) page-id))) + (ex/raise :type :not-found + :code :object-not-found + :hint "not found")) (get-comments conn thread-id))))) (def sql:get-comments diff --git a/backend/src/app/rpc/commands/files.clj b/backend/src/app/rpc/commands/files.clj index 323ddf6c3f..435b83afff 100644 --- a/backend/src/app/rpc/commands/files.clj +++ b/backend/src/app/rpc/commands/files.clj @@ -95,18 +95,23 @@ (def check-read-permissions! (perms/make-check-fn has-read-permissions?)) -;; A user has comment permissions if she has read permissions, or -;; explicit comment permissions through the share-id +;; A user has comment permissions if: +;; - For :membership type: they have read permissions OR explicit comment permissions +;; - For :share-link type: they must have explicit comment permissions (who-comment=all) +;; This prevents share-link holders with who-comment=team from bypassing the restriction (defn check-comment-permissions! [cfg profile-id file-id share-id] - (let [perms (perms/get-file-read-permissions cfg profile-id file-id share-id) - can-read (has-read-permissions? perms) - can-comment (has-comment-permissions? perms)] - (when-not (or can-read can-comment) + (let [perms (perms/get-file-read-permissions cfg profile-id file-id share-id) + allowed? (if (= :share-link (:type perms)) + (has-comment-permissions? perms) + (or (has-read-permissions? perms) + (has-comment-permissions? perms)))] + (when-not allowed? (ex/raise :type :not-found :code :object-not-found - :hint "not found")))) + :hint "not found")) + perms)) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; QUERY COMMANDS diff --git a/backend/test/backend_tests/rpc_comment_test.clj b/backend/test/backend_tests/rpc_comment_test.clj index 8724cbfdfa..dd99cb73fc 100644 --- a/backend/test/backend_tests/rpc_comment_test.clj +++ b/backend/test/backend_tests/rpc_comment_test.clj @@ -285,3 +285,196 @@ (let [threads (th/db-query :comment-thread {:file-id (:id file-1)})] (t/is (= 0 (count threads))))))))) + +(t/deftest share-link-who-comment-team-cannot-comment + (let [owner (th/create-profile* 1 {:is-active true}) + outsider (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 (:id project)}) + page-id (get-in file [:data :pages 0]) + + share (th/command! {::th/type :create-share-link + ::rpc/profile-id (:id owner) + :file-id (:id file) + :pages #{page-id} + :who-comment "team" + :who-inspect "all"}) + share-id (get-in share [:result :id])] + + (t/testing "outsider with who-comment=team share-link cannot get-comment-threads" + (let [out (th/command! {::th/type :get-comment-threads + ::rpc/profile-id (:id outsider) + :file-id (:id file) + :share-id share-id})] + (t/is (not (th/success? out))) + (t/is (= :not-found (th/ex-type (:error out)))))) + + (t/testing "outsider with who-comment=team share-link cannot create-comment-thread" + (let [out (th/command! {::th/type :create-comment-thread + ::rpc/profile-id (:id outsider) + :file-id (:id file) + :page-id page-id + :position (gpt/point 0) + :content "outsider comment" + :frame-id uuid/zero + :share-id share-id})] + (t/is (not (th/success? out))) + (t/is (= :not-found (th/ex-type (:error out)))))))) + +(t/deftest share-link-who-comment-all-can-comment + (let [owner (th/create-profile* 1 {:is-active true}) + outsider (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 (:id project)}) + page-id (get-in file [:data :pages 0]) + + share (th/command! {::th/type :create-share-link + ::rpc/profile-id (:id owner) + :file-id (:id file) + :pages #{page-id} + :who-comment "all" + :who-inspect "all"}) + share-id (get-in share [:result :id])] + + (t/testing "outsider with who-comment=all share-link can get-comment-threads" + (let [out (th/command! {::th/type :get-comment-threads + ::rpc/profile-id (:id outsider) + :file-id (:id file) + :share-id share-id})] + (t/is (th/success? out)))) + + (t/testing "outsider with who-comment=all share-link can create-comment-thread" + (let [out (th/command! {::th/type :create-comment-thread + ::rpc/profile-id (:id outsider) + :file-id (:id file) + :page-id page-id + :position (gpt/point 0) + :content "outsider comment" + :frame-id uuid/zero + :share-id share-id})] + (t/is (th/success? out)))))) + +(t/deftest share-link-page-scope-enforced + (let [owner (th/create-profile* 1 {:is-active true}) + outsider (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 (:id project)}) + + page-a (get-in file [:data :pages 0]) + page-b (uuid/random) + + _ (th/command! {::th/type :update-file + ::rpc/profile-id (:id owner) + :id (:id file) + :session-id (uuid/random) + :revn 0 + :vern 0 + :changes [{:type :add-page + :id page-b + :page {:id page-b + :name "Page B" + :options {} + :objects {}}}]}) + + thread-a (th/command! {::th/type :create-comment-thread + ::rpc/profile-id (:id owner) + :file-id (:id file) + :page-id page-a + :position (gpt/point 0) + :content "comment on page A" + :frame-id uuid/zero}) + thread-b (th/command! {::th/type :create-comment-thread + ::rpc/profile-id (:id owner) + :file-id (:id file) + :page-id page-b + :position (gpt/point 0) + :content "comment on page B" + :frame-id uuid/zero}) + + thread-a-id (get-in thread-a [:result :id]) + thread-b-id (get-in thread-b [:result :id]) + + share (th/command! {::th/type :create-share-link + ::rpc/profile-id (:id owner) + :file-id (:id file) + :pages #{page-a} + :who-comment "all" + :who-inspect "all"}) + share-id (get-in share [:result :id])] + + (t/testing "share-link holder can get-comment-threads for shared page only" + (let [out (th/command! {::th/type :get-comment-threads + ::rpc/profile-id (:id outsider) + :file-id (:id file) + :share-id share-id}) + result (:result out)] + (t/is (th/success? out)) + (t/is (= 1 (count result))) + (t/is (= page-a (:page-id (first result)))))) + + (t/testing "share-link holder cannot get-comment-thread for unshared page" + (let [out (th/command! {::th/type :get-comment-thread + ::rpc/profile-id (:id outsider) + :file-id (:id file) + :id thread-b-id + :share-id share-id})] + (t/is (not (th/success? out))) + (t/is (= :not-found (th/ex-type (:error out)))))) + + (t/testing "share-link holder can get-comment-thread for shared page" + (let [out (th/command! {::th/type :get-comment-thread + ::rpc/profile-id (:id outsider) + :file-id (:id file) + :id thread-a-id + :share-id share-id})] + (t/is (th/success? out)))) + + (t/testing "share-link holder cannot get-comments for thread on unshared page" + (let [out (th/command! {::th/type :get-comments + ::rpc/profile-id (:id outsider) + :thread-id thread-b-id + :share-id share-id})] + (t/is (not (th/success? out))) + (t/is (= :not-found (th/ex-type (:error out)))))))) + +(t/deftest membership-can-still-comment + (let [owner (th/create-profile* 1 {:is-active true}) + member (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 member) + :role :editor}) + project (th/create-project* 1 {:team-id (:id team) + :profile-id (:id owner)}) + file (th/create-file* 1 {:profile-id (:id owner) + :project-id (:id project)}) + page-id (get-in file [:data :pages 0])] + + (t/testing "team member can get-comment-threads without share-id" + (let [out (th/command! {::th/type :get-comment-threads + ::rpc/profile-id (:id member) + :file-id (:id file)})] + (t/is (th/success? out)))) + + (t/testing "team member can create-comment-thread without share-id" + (let [out (th/command! {::th/type :create-comment-thread + ::rpc/profile-id (:id member) + :file-id (:id file) + :page-id page-id + :position (gpt/point 0) + :content "member comment" + :frame-id uuid/zero})] + (t/is (th/success? out))))))