From aeb1ac41dadc43b80acb182428f023da2c20013e Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Thu, 5 Dec 2024 12:39:43 +0100 Subject: [PATCH 1/3] :bug: Prevent upload media objects to deleted files --- backend/src/app/rpc/commands/media.clj | 51 ++++++++++++------- backend/test/backend_tests/rpc_media_test.clj | 33 ++++++++++++ 2 files changed, 66 insertions(+), 18 deletions(-) diff --git a/backend/src/app/rpc/commands/media.clj b/backend/src/app/rpc/commands/media.clj index 69265c27f..f4913edb2 100644 --- a/backend/src/app/rpc/commands/media.clj +++ b/backend/src/app/rpc/commands/media.clj @@ -60,15 +60,25 @@ (media/validate-media-type! content) (media/validate-media-size! content) - (db/run! cfg (fn [cfg] - (let [object (create-file-media-object cfg params) - props {:name (:name params) - :file-id file-id - :is-local (:is-local params) - :size (:size content) - :mtype (:mtype content)}] - (with-meta object - {::audit/replace-props props}))))) + (db/run! cfg (fn [{:keys [::db/conn] :as cfg}] + ;; We get the minimal file for proper checking if + ;; file is not already deleted + (let [_ (files/get-minimal-file conn file-id) + mobj (create-file-media-object cfg params)] + + (db/update! conn :file + {:modified-at (dt/now) + :has-media-trimmed false} + {:id file-id} + {::db/return-keys false}) + + (with-meta mobj + {::audit/replace-props + {:name (:name params) + :file-id file-id + :is-local (:is-local params) + :size (:size content) + :mtype (:mtype content)}}))))) (defn- big-enough-for-thumbnail? "Checks if the provided image info is big enough for @@ -142,20 +152,14 @@ :always (assoc ::image (process-main-image info))))) -(defn create-file-media-object - [{:keys [::sto/storage ::db/conn ::wrk/executor]} +(defn- create-file-media-object + [{:keys [::sto/storage ::db/conn ::wrk/executor] :as cfg} {:keys [id file-id is-local name content]}] - (let [result (px/invoke! executor (partial process-image content)) image (sto/put-object! storage (::image result)) thumb (when-let [params (::thumb result)] (sto/put-object! storage params))] - (db/update! conn :file - {:modified-at (dt/now) - :has-media-trimmed false} - {:id file-id}) - (db/exec-one! conn [sql:create-file-media-object (or id (uuid/next)) file-id is-local name @@ -182,7 +186,18 @@ ::sm/params schema:create-file-media-object-from-url} [{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id file-id] :as params}] (files/check-edition-permissions! pool profile-id file-id) - (create-file-media-object-from-url cfg (assoc params :profile-id profile-id))) + ;; We get the minimal file for proper checking if file is not + ;; already deleted + (let [_ (files/get-minimal-file cfg file-id) + mobj (create-file-media-object-from-url cfg (assoc params :profile-id profile-id))] + + (db/update! pool :file + {:modified-at (dt/now) + :has-media-trimmed false} + {:id file-id} + {::db/return-keys false}) + + mobj)) (defn download-image [{:keys [::http/client]} uri] diff --git a/backend/test/backend_tests/rpc_media_test.clj b/backend/test/backend_tests/rpc_media_test.clj index 748c72683..3095a5c05 100644 --- a/backend/test/backend_tests/rpc_media_test.clj +++ b/backend/test/backend_tests/rpc_media_test.clj @@ -10,6 +10,7 @@ [app.db :as db] [app.rpc :as-alias rpc] [app.storage :as sto] + [app.util.time :as dt] [backend-tests.helpers :as th] [clojure.test :as t] [datoteka.fs :as fs])) @@ -245,3 +246,35 @@ (t/is (= "image/jpeg" (:mtype result))) (t/is (uuid? (:media-id result))) (t/is (uuid? (:thumbnail-id result)))))) + + +(t/deftest media-object-upload-command-when-file-is-deleted + (let [prof (th/create-profile* 1) + proj (th/create-project* 1 {:profile-id (:id prof) + :team-id (:default-team-id prof)}) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:default-project-id prof) + :is-shared false}) + + _ (th/db-update! :file + {:deleted-at (dt/now)} + {:id (:id file)}) + + mfile {:filename "sample.jpg" + :path (th/tempfile "backend_tests/test_files/sample.jpg") + :mtype "image/jpeg" + :size 312043} + + params {::th/type :upload-file-media-object + ::rpc/profile-id (:id prof) + :file-id (:id file) + :is-local true + :name "testfile" + :content mfile} + + out (th/command! params)] + + (let [error (:error out) + error-data (ex-data error)] + (t/is (th/ex-info? error)) + (t/is (= (:type error-data) :not-found))))) From 2f79d71262da574144157a310df7548f34572290 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 9 Dec 2024 10:15:13 +0100 Subject: [PATCH 2/3] :bug: Fix incorrect event handling on file-menu Don't wait team to be present for open the menu, because with slow connection speed it can cause unexpected ux glitche showing menu when the component inner request is resoved --- .../src/app/main/ui/dashboard/file_menu.cljs | 243 +++++++++--------- frontend/src/app/main/ui/dashboard/grid.cljs | 1 - 2 files changed, 116 insertions(+), 128 deletions(-) diff --git a/frontend/src/app/main/ui/dashboard/file_menu.cljs b/frontend/src/app/main/ui/dashboard/file_menu.cljs index 9a0926110..d618eb2a5 100644 --- a/frontend/src/app/main/ui/dashboard/file_menu.cljs +++ b/frontend/src/app/main/ui/dashboard/file_menu.cljs @@ -13,7 +13,6 @@ [app.main.data.exports.files :as fexp] [app.main.data.modal :as modal] [app.main.data.notifications :as ntf] - [app.main.refs :as refs] [app.main.repo :as rp] [app.main.store :as st] [app.main.ui.components.context-menu-a11y :refer [context-menu*]] @@ -57,9 +56,8 @@ (mf/defc file-menu* {::mf/props :obj} - [{:keys [files show on-edit on-menu-close top left navigate origin parent-id can-edit]}] + [{:keys [files on-edit on-menu-close top left navigate origin parent-id can-edit]}] (assert (seq files) "missing `files` prop") - (assert (boolean? show) "missing `show` prop") (assert (fn? on-edit) "missing `on-edit` prop") (assert (fn? on-menu-close) "missing `on-menu-close` prop") (assert (boolean? navigate) "missing `navigate` prop") @@ -74,12 +72,11 @@ multi? (> file-count 1) current-team-id (mf/use-ctx ctx/current-team-id) - teams (mf/use-state nil) - default-team (-> (mf/deref refs/teams) - (get current-team-id)) + teams* (mf/use-state nil) + teams (deref teams*) - current-team (or (get @teams current-team-id) default-team) - other-teams (remove #(= (:id %) current-team-id) (vals @teams)) + current-team (get teams current-team-id) + other-teams (remove #(= (:id %) current-team-id) (vals teams)) current-projects (remove #(= (:id %) (:project-id file)) (:projects current-team)) @@ -207,142 +204,134 @@ on-export-standard-files (mf/use-fn (mf/deps on-export-files) - (partial on-export-files :legacy-zip)) + (partial on-export-files :legacy-zip))] - ;; NOTE: this is used for detect if component is still mounted - mounted-ref (mf/use-ref true)] + (mf/with-effect [] + (->> (rp/cmd! :get-all-projects) + (rx/map group-by-team) + (rx/subs! #(reset! teams* %)))) - (mf/use-effect - (mf/deps show) - (fn [] - (when show - (->> (rp/cmd! :get-all-projects) - (rx/map group-by-team) - (rx/subs! #(when (mf/ref-val mounted-ref) - (reset! teams %))))))) + (let [sub-options + (concat + (for [project current-projects] + {:name (get-project-name project) + :id (get-project-id project) + :handler (on-move (:id current-team) + (:id project))}) + (when (seq other-teams) + [{:name (tr "dashboard.move-to-other-team") + :id "move-to-other-team" + :options + (for [team other-teams] + {:name (get-team-name team) + :id (get-project-id team) + :options + (for [sub-project (:projects team)] + {:name (get-project-name sub-project) + :id (get-project-id sub-project) + :handler (on-move (:id team) + (:id sub-project))})})}])) - (when current-team - (let [sub-options - (concat - (for [project current-projects] - {:name (get-project-name project) - :id (get-project-id project) - :handler (on-move (:id current-team) - (:id project))}) - (when (seq other-teams) - [{:name (tr "dashboard.move-to-other-team") - :id "move-to-other-team" - :options - (for [team other-teams] - {:name (get-team-name team) - :id (get-project-id team) - :options - (for [sub-project (:projects team)] - {:name (get-project-name sub-project) - :id (get-project-id sub-project) - :handler (on-move (:id team) - (:id sub-project))})})}])) + options + (if multi? + [(when can-edit + {:name (tr "dashboard.duplicate-multi" file-count) + :id "duplicate-multi" + :handler on-duplicate}) - options - (if multi? - [(when can-edit - {:name (tr "dashboard.duplicate-multi" file-count) - :id "duplicate-multi" - :handler on-duplicate}) + (when (and (or (seq current-projects) (seq other-teams)) can-edit) + {:name (tr "dashboard.move-to-multi" file-count) + :id "file-move-multi" + :options sub-options}) - (when (and (or (seq current-projects) (seq other-teams)) can-edit) - {:name (tr "dashboard.move-to-multi" file-count) - :id "file-move-multi" - :options sub-options}) + (when-not (contains? cf/flags :export-file-v3) + {:name (tr "dashboard.export-binary-multi" file-count) + :id "file-binary-export-multi" + :handler on-export-binary-files}) - (when-not (contains? cf/flags :export-file-v3) - {:name (tr "dashboard.export-binary-multi" file-count) - :id "file-binary-export-multi" - :handler on-export-binary-files}) + (when (contains? cf/flags :export-file-v3) + {:name (tr "dashboard.export-binary-multi" file-count) + :id "file-binary-export-multi" + :handler on-export-binary-files-v3}) - (when (contains? cf/flags :export-file-v3) - {:name (tr "dashboard.export-binary-multi" file-count) - :id "file-binary-export-multi" - :handler on-export-binary-files-v3}) + (when-not (contains? cf/flags :export-file-v3) + {:name (tr "dashboard.export-standard-multi" file-count) + :id "file-standard-export-multi" + :handler on-export-standard-files}) - (when-not (contains? cf/flags :export-file-v3) - {:name (tr "dashboard.export-standard-multi" file-count) - :id "file-standard-export-multi" - :handler on-export-standard-files}) + (when (and (:is-shared file) can-edit) + {:name (tr "labels.unpublish-multi-files" file-count) + :id "file-unpublish-multi" + :handler on-del-shared}) - (when (and (:is-shared file) can-edit) - {:name (tr "labels.unpublish-multi-files" file-count) - :id "file-unpublish-multi" - :handler on-del-shared}) + (when (and (not is-lib-page?) can-edit) + {:name :separator} + {:name (tr "labels.delete-multi-files" file-count) + :id "file-delete-multi" + :handler on-delete})] - (when (and (not is-lib-page?) can-edit) - {:name :separator} - {:name (tr "labels.delete-multi-files" file-count) - :id "file-delete-multi" - :handler on-delete})] + [{:name (tr "dashboard.open-in-new-tab") + :id "file-open-new-tab" + :handler on-new-tab} + (when (and (not is-search-page?) can-edit) + {:name (tr "labels.rename") + :id "file-rename" + :handler on-edit}) - [{:name (tr "dashboard.open-in-new-tab") - :id "file-open-new-tab" - :handler on-new-tab} - (when (and (not is-search-page?) can-edit) - {:name (tr "labels.rename") - :id "file-rename" - :handler on-edit}) + (when (and (not is-search-page?) can-edit) + {:name (tr "dashboard.duplicate") + :id "file-duplicate" + :handler on-duplicate}) - (when (and (not is-search-page?) can-edit) - {:name (tr "dashboard.duplicate") - :id "file-duplicate" - :handler on-duplicate}) + (when (and (not is-lib-page?) + (not is-search-page?) + (or (seq current-projects) (seq other-teams)) + can-edit) + {:name (tr "dashboard.move-to") + :id "file-move-to" + :options sub-options}) - (when (and (not is-lib-page?) - (not is-search-page?) - (or (seq current-projects) (seq other-teams)) - can-edit) - {:name (tr "dashboard.move-to") - :id "file-move-to" - :options sub-options}) + (when (and (not is-search-page?) + can-edit) + (if (:is-shared file) + {:name (tr "dashboard.unpublish-shared") + :id "file-del-shared" + :handler on-del-shared} + {:name (tr "dashboard.add-shared") + :id "file-add-shared" + :handler on-add-shared})) - (when (and (not is-search-page?) - can-edit) - (if (:is-shared file) - {:name (tr "dashboard.unpublish-shared") - :id "file-del-shared" - :handler on-del-shared} - {:name (tr "dashboard.add-shared") - :id "file-add-shared" - :handler on-add-shared})) + {:name :separator} - {:name :separator} + (when-not (contains? cf/flags :export-file-v3) + {:name (tr "dashboard.download-binary-file") + :id "download-binary-file" + :handler on-export-binary-files}) - (when-not (contains? cf/flags :export-file-v3) - {:name (tr "dashboard.download-binary-file") - :id "download-binary-file" - :handler on-export-binary-files}) + (when (contains? cf/flags :export-file-v3) + {:name (tr "dashboard.download-binary-file") + :id "download-binary-file" + :handler on-export-binary-files-v3}) - (when (contains? cf/flags :export-file-v3) - {:name (tr "dashboard.download-binary-file") - :id "download-binary-file" - :handler on-export-binary-files-v3}) + (when-not (contains? cf/flags :export-file-v3) + {:name (tr "dashboard.download-standard-file") + :id "download-standard-file" + :handler on-export-standard-files}) - (when-not (contains? cf/flags :export-file-v3) - {:name (tr "dashboard.download-standard-file") - :id "download-standard-file" - :handler on-export-standard-files}) + (when (and (not is-lib-page?) (not is-search-page?) can-edit) + {:name :separator}) - (when (and (not is-lib-page?) (not is-search-page?) can-edit) - {:name :separator}) + (when (and (not is-lib-page?) (not is-search-page?) can-edit) + {:name (tr "labels.delete") + :id "file-delete" + :handler on-delete})])] - (when (and (not is-lib-page?) (not is-search-page?) can-edit) - {:name (tr "labels.delete") - :id "file-delete" - :handler on-delete})])] - - [:> context-menu* - {:on-close on-menu-close - :show show - :fixed (or (not= top 0) (not= left 0)) - :min-width true - :top top - :left left - :options options - :origin parent-id}])))) + [:> context-menu* + {:on-close on-menu-close + :fixed (or (not= top 0) (not= left 0)) + :show true + :min-width true + :top top + :left left + :options options + :origin parent-id}]))) diff --git a/frontend/src/app/main/ui/dashboard/grid.cljs b/frontend/src/app/main/ui/dashboard/grid.cljs index 4c72b6852..dd4992c2c 100644 --- a/frontend/src/app/main/ui/dashboard/grid.cljs +++ b/frontend/src/app/main/ui/dashboard/grid.cljs @@ -406,7 +406,6 @@ ;; so the menu can be handled [:div {:style {:pointer-events "all"}} [:> file-menu* {:files (vals selected-files) - :show (:menu-open dashboard-local) :left (+ 24 (:x (:menu-pos dashboard-local))) :top (:y (:menu-pos dashboard-local)) :can-edit can-edit From a923d396030e82fdcb1df169ed008894bfc33078 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 9 Dec 2024 09:48:35 +0100 Subject: [PATCH 3/3] :bug: Fix incorrect teams query on profile deletion The current approach prevents profile deletion when there are some extra (soft)deleted teams where the profile is owner --- backend/src/app/rpc/commands/profile.clj | 7 +++++- .../test/backend_tests/rpc_profile_test.clj | 23 ++++++++++++++++--- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/backend/src/app/rpc/commands/profile.clj b/backend/src/app/rpc/commands/profile.clj index 57034c461..7c7ca3339 100644 --- a/backend/src/app/rpc/commands/profile.clj +++ b/backend/src/app/rpc/commands/profile.clj @@ -422,7 +422,9 @@ :deleted-at deleted-at :id profile-id}}) - (rph/with-transform {} (session/delete-fn cfg))))) + + (-> (rph/wrap nil) + (rph/with-transform (session/delete-fn cfg)))))) ;; --- HELPERS @@ -431,8 +433,11 @@ "WITH owner_teams AS ( SELECT tpr.team_id AS id FROM team_profile_rel AS tpr + JOIN team AS t ON (t.id = tpr.team_id) WHERE tpr.is_owner IS TRUE AND tpr.profile_id = ? + AND (t.deleted_at IS NULL OR + t.deleted_at > now()) ) SELECT tpr.team_id AS id, count(tpr.profile_id) - 1 AS participants diff --git a/backend/test/backend_tests/rpc_profile_test.clj b/backend/test/backend_tests/rpc_profile_test.clj index 1bd49db48..47e58adba 100644 --- a/backend/test/backend_tests/rpc_profile_test.clj +++ b/backend/test/backend_tests/rpc_profile_test.clj @@ -203,7 +203,24 @@ edata (ex-data error)] (t/is (th/ex-info? error)) (t/is (= (:type edata) :validation)) - (t/is (= (:code edata) :owner-teams-with-people)))))) + (t/is (= (:code edata) :owner-teams-with-people))) + + (let [params {::th/type :delete-team + ::rpc/profile-id (:id prof1) + :id (:id team1)} + out (th/command! params)] + ;; (th/print-result! out) + + (let [team (th/db-get :team {:id (:id team1)} {::db/remove-deleted false})] + (t/is (dt/instant? (:deleted-at team))))) + + ;; Request profile to be deleted + (let [params {::th/type :delete-profile + ::rpc/profile-id (:id prof1)} + out (th/command! params)] + ;; (th/print-result! out) + (t/is (nil? (:result out))) + (t/is (nil? (:error out))))))) (t/deftest profile-deletion-3 (let [prof1 (th/create-profile* 1) @@ -291,7 +308,7 @@ out (th/command! params)] ;; (th/print-result! out) - (t/is (= {} (:result out))) + (t/is (nil? (:result out))) (t/is (nil? (:error out)))) ;; query files after profile soft deletion @@ -336,7 +353,7 @@ ::rpc/profile-id (:id prof1)} out (th/command! params)] ;; (th/print-result! out) - (t/is (= {} (:result out))) + (t/is (nil? (:result out))) (t/is (nil? (:error out)))) (th/run-pending-tasks!)