From 645954bc7c12be0d1d80bd050f392b62fac7679a Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Thu, 25 Feb 2021 17:45:39 +0100 Subject: [PATCH] :bug: Fix issues on files and project rpc methods. --- backend/src/app/rpc/mutations/files.clj | 1 + backend/src/app/rpc/mutations/projects.clj | 11 +- backend/tests/app/tests/helpers.clj | 141 ++++----- .../tests/app/tests/test_services_files.clj | 277 ++++++++++++------ .../app/tests/test_services_projects.clj | 83 +++++- 5 files changed, 333 insertions(+), 180 deletions(-) diff --git a/backend/src/app/rpc/mutations/files.clj b/backend/src/app/rpc/mutations/files.clj index dce05f643..82cd365d7 100644 --- a/backend/src/app/rpc/mutations/files.clj +++ b/backend/src/app/rpc/mutations/files.clj @@ -155,6 +155,7 @@ :hint "A file cannot be linked to itself")) (db/with-atomic [conn pool] (files/check-edition-permissions! conn profile-id file-id) + (files/check-edition-permissions! conn profile-id library-id) (link-file-to-library conn params))) (def sql:link-file-to-library diff --git a/backend/src/app/rpc/mutations/projects.clj b/backend/src/app/rpc/mutations/projects.clj index de92bb054..deaf42321 100644 --- a/backend/src/app/rpc/mutations/projects.clj +++ b/backend/src/app/rpc/mutations/projects.clj @@ -14,6 +14,7 @@ [app.config :as cfg] [app.db :as db] [app.rpc.queries.projects :as proj] + [app.rpc.queries.teams :as teams] [app.tasks :as tasks] [app.util.services :as sv] [app.util.time :as dt] @@ -38,13 +39,14 @@ :opt-un [::id])) (sv/defmethod ::create-project - [{:keys [pool] :as cfg} params] + [{:keys [pool] :as cfg} {:keys [profile-id team-id] :as params}] (db/with-atomic [conn pool] - (let [proj (create-project conn params) - params (assoc params :project-id (:id proj))] + (teams/check-edition-permissions! conn profile-id team-id) + (let [project (create-project conn params) + params (assoc params :project-id (:id project))] (create-project-profile conn params) (create-team-project-profile conn params) - (assoc proj :is-pinned true)))) + (assoc project :is-pinned true)))) (defn create-project [conn {:keys [id team-id name default?] :as params}] @@ -92,6 +94,7 @@ (sv/defmethod ::update-project-pin [{:keys [pool] :as cfg} {:keys [id profile-id team-id is-pinned] :as params}] (db/with-atomic [conn pool] + (proj/check-edition-permissions! conn profile-id id) (db/exec-one! conn [sql:update-project-pin team-id id profile-id is-pinned is-pinned]) nil)) diff --git a/backend/tests/app/tests/helpers.clj b/backend/tests/app/tests/helpers.clj index a11295263..188d0454a 100644 --- a/backend/tests/app/tests/helpers.clj +++ b/backend/tests/app/tests/helpers.clj @@ -39,12 +39,11 @@ (def ^:dynamic *pool* nil) (def config - (merge {:redis-uri "redis://redis/1" + (merge cfg/config + {:redis-uri "redis://redis/1" :database-uri "postgresql://postgres/penpot_test" :storage-fs-directory "/tmp/app/storage" - :migrations-verbose false} - cfg/config)) - + :migrations-verbose false})) (defn state-init [next] @@ -108,48 +107,7 @@ [prefix & args] (uuid/namespaced uuid/zero (apply str prefix args))) - -(defn create-profile - [conn i] - (let [params {:id (mk-uuid "profile" i) - :fullname (str "Profile " i) - :email (str "profile" i ".test@nodomain.com") - :password "123123" - :is-demo true}] - (->> (#'profile/create-profile conn params) - (#'profile/create-profile-relations conn)))) - -(defn create-team - [conn profile-id i] - (let [id (mk-uuid "team" i) - team (#'teams/create-team conn {:id id - :profile-id profile-id - :name (str "team" i)})] - (#'teams/create-team-profile conn - {:team-id id - :profile-id profile-id - :is-owner true - :is-admin true - :can-edit true}) - team)) - -(defn create-project - [conn profile-id team-id i] - (#'projects/create-project conn {:id (mk-uuid "project" i) - :profile-id profile-id - :team-id team-id - :name (str "project" i)})) - -(defn create-file - [conn profile-id project-id is-shared i] - (#'files/create-file conn {:id (mk-uuid "file" i) - :profile-id profile-id - :project-id project-id - :is-shared is-shared - :name (str "file" i)})) - - -;; --- NEW HELPERS +;; --- FACTORIES (defn create-profile* ([i] (create-profile* *pool* i {})) @@ -202,6 +160,60 @@ :can-edit true}) team))) +(defn link-file-to-library* + ([params] (link-file-to-library* *pool* params)) + ([conn {:keys [file-id library-id] :as params}] + (#'files/link-file-to-library conn {:file-id file-id :library-id library-id}))) + +(defn create-complaint-for + [conn {:keys [id created-at type]}] + (db/insert! conn :profile-complaint-report + {:profile-id id + :created-at (or created-at (dt/now)) + :type (name type) + :content (db/tjson {})})) + +(defn create-global-complaint-for + [conn {:keys [email type created-at]}] + (db/insert! conn :global-complaint-report + {:email email + :type (name type) + :created-at (or created-at (dt/now)) + :content (db/tjson {})})) + + +(defn create-team-permission* + ([params] (create-team-permission* *pool* params)) + ([conn {:keys [team-id profile-id is-owner is-admin can-edit] + :or {is-owner true is-admin true can-edit true}}] + (db/insert! conn :team-profile-rel {:team-id team-id + :profile-id profile-id + :is-owner is-owner + :is-admin is-admin + :can-edit can-edit}))) + +(defn create-project-permission* + ([params] (create-project-permission* *pool* params)) + ([conn {:keys [project-id profile-id is-owner is-admin can-edit] + :or {is-owner true is-admin true can-edit true}}] + (db/insert! conn :project-profile-rel {:project-id project-id + :profile-id profile-id + :is-owner is-owner + :is-admin is-admin + :can-edit can-edit}))) + +(defn create-file-permission* + ([params] (create-file-permission* *pool* params)) + ([conn {:keys [file-id profile-id is-owner is-admin can-edit] + :or {is-owner true is-admin true can-edit true}}] + (db/insert! conn :project-profile-rel {:file-id file-id + :profile-id profile-id + :is-owner is-owner + :is-admin is-admin + :can-edit can-edit}))) + + +;; --- RPC HELPERS (defn handle-error [^Throwable err] @@ -209,14 +221,6 @@ (handle-error (.getCause err)) err)) -(defmacro try-on - [expr] - `(try - (let [result# (deref ~expr)] - [nil result#]) - (catch Exception e# - [(handle-error e#) nil]))) - (defmacro try-on! [expr] `(try @@ -226,16 +230,6 @@ {:error (handle-error e#) :result nil}))) -(defmacro try! - [expr] - `(try - {:error nil - :result ~expr} - (catch Exception e# - {:error (handle-error e#) - :result nil}))) - - (defn mutation! [{:keys [::type] :as data}] (let [method-fn (get-in *system* [:app.rpc/rpc :methods :mutation type])] @@ -248,7 +242,7 @@ (try-on! (method-fn (dissoc data ::type))))) -;; --- Utils +;; --- UTILS (defn print-error! [error] @@ -317,23 +311,6 @@ ([key] (get (merge config data) key)) ([key default] (get (merge config data) key default)))) -(defn create-complaint-for - [conn {:keys [id created-at type]}] - (db/insert! conn :profile-complaint-report - {:profile-id id - :created-at (or created-at (dt/now)) - :type (name type) - :content (db/tjson {})})) - -(defn create-global-complaint-for - [conn {:keys [email type created-at]}] - (db/insert! conn :global-complaint-report - {:email email - :type (name type) - :created-at (or created-at (dt/now)) - :content (db/tjson {})})) - - (defn reset-mock! [m] (reset! m @(mk/make-mock {}))) diff --git a/backend/tests/app/tests/test_services_files.clj b/backend/tests/app/tests/test_services_files.clj index bfd87f287..4de3d8c5f 100644 --- a/backend/tests/app/tests/test_services_files.clj +++ b/backend/tests/app/tests/test_services_files.clj @@ -120,102 +120,209 @@ (t/is (= 0 (count result)))))) )) -(defn- create-file-media-object - [{:keys [profile-id file-id]}] - (let [mfile {:filename "sample.jpg" - :tempfile (th/tempfile "app/tests/_files/sample.jpg") - :content-type "image/jpeg" - :size 312043} - params {::th/type :upload-file-media-object - :profile-id profile-id - :file-id file-id - :is-local true - :name "testfile" - :content mfile} - out (th/mutation! params)] - (t/is (nil? (:error out))) - (:result out))) - -(defn- update-file - [{:keys [profile-id file-id changes revn] :or {revn 0}}] - (let [params {::th/type :update-file - :id file-id - :session-id (uuid/random) - :profile-id profile-id - :revn revn - :changes changes} - out (th/mutation! params)] - (t/is (nil? (:error out))) - (:result out))) - (t/deftest file-media-gc-task - (let [task (:app.tasks.file-media-gc/handler th/*system*) - storage (:app.storage/storage th/*system*) + (letfn [(create-file-media-object [{:keys [profile-id file-id]}] + (let [mfile {:filename "sample.jpg" + :tempfile (th/tempfile "app/tests/_files/sample.jpg") + :content-type "image/jpeg" + :size 312043} + params {::th/type :upload-file-media-object + :profile-id profile-id + :file-id file-id + :is-local true + :name "testfile" + :content mfile} + out (th/mutation! params)] + (t/is (nil? (:error out))) + (:result out))) - 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}) + (update-file [{:keys [profile-id file-id changes revn] :or {revn 0}}] + (let [params {::th/type :update-file + :id file-id + :session-id (uuid/random) + :profile-id profile-id + :revn revn + :changes changes} + out (th/mutation! params)] + (t/is (nil? (:error out))) + (:result out)))] - fmo1 (create-file-media-object {:profile-id (:id prof) - :file-id (:id file)}) - fmo2 (create-file-media-object {:profile-id (:id prof) - :file-id (:id file)}) - shid (uuid/random) + (let [storage (:app.storage/storage th/*system*) - ures (update-file - {:file-id (:id file) - :profile-id (:id prof) - :revn 0 - :changes - [{:type :add-obj - :page-id (first (get-in file [:data :pages])) - :id shid - :parent-id uuid/zero - :frame-id uuid/zero - :obj {:id shid - :name "image" - :frame-id uuid/zero - :parent-id uuid/zero - :type :image - :metadata {:id (:id fmo1)}}}]})] + profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared false}) - ;; run the task inmediatelly - (let [res (task {})] - (t/is (= 0 (:processed res)))) + fmo1 (create-file-media-object {:profile-id (:id profile) + :file-id (:id file)}) + fmo2 (create-file-media-object {:profile-id (:id profile) + :file-id (:id file)}) + shid (uuid/random) - ;; make the file ellegible for GC waiting 300ms - (th/sleep 300) + ures (update-file + {:file-id (:id file) + :profile-id (:id profile) + :revn 0 + :changes + [{:type :add-obj + :page-id (first (get-in file [:data :pages])) + :id shid + :parent-id uuid/zero + :frame-id uuid/zero + :obj {:id shid + :name "image" + :frame-id uuid/zero + :parent-id uuid/zero + :type :image + :metadata {:id (:id fmo1)}}}]})] - ;; run the task again - (let [res (task {})] - (t/is (= 1 (:processed res)))) + ;; run the task inmediatelly + (let [task (:app.tasks.file-media-gc/handler th/*system*) + res (task {})] + (t/is (= 0 (:processed res)))) - ;; Retrieve file and check trimmed attribute - (let [row (db/exec-one! th/*pool* ["select * from file where id = ?" (:id file)])] - (t/is (:has-media-trimmed row))) + ;; make the file ellegible for GC waiting 300ms (configured + ;; timeout for testing) + (th/sleep 300) - ;; check file media objects - (let [fmos (db/exec! th/*pool* ["select * from file_media_object where file_id = ?" (:id file)])] - (t/is (= 1 (count fmos)))) + ;; run the task again + (let [task (:app.tasks.file-media-gc/handler th/*system*) + res (task {})] + (t/is (= 1 (:processed res)))) - ;; The underlying storage objects are still available. - (t/is (some? (sto/get-object storage (:media-id fmo2)))) - (t/is (some? (sto/get-object storage (:thumbnail-id fmo2)))) - (t/is (some? (sto/get-object storage (:media-id fmo1)))) - (t/is (some? (sto/get-object storage (:thumbnail-id fmo1)))) + ;; retrieve file and check trimmed attribute + (let [row (db/exec-one! th/*pool* ["select * from file where id = ?" (:id file)])] + (t/is (true? (:has-media-trimmed row)))) - ;; but if we pass the touched gc task two of them should disappear - (let [task (:app.storage/gc-touched-task th/*system*) - res (task {})] - (t/is (= 0 (:freeze res))) - (t/is (= 2 (:delete res))) + ;; check file media objects + (let [rows (db/exec! th/*pool* ["select * from file_media_object where file_id = ?" (:id file)])] + (t/is (= 1 (count rows)))) - (t/is (nil? (sto/get-object storage (:media-id fmo2)))) - (t/is (nil? (sto/get-object storage (:thumbnail-id fmo2)))) + ;; The underlying storage objects are still available. + (t/is (some? (sto/get-object storage (:media-id fmo2)))) + (t/is (some? (sto/get-object storage (:thumbnail-id fmo2)))) (t/is (some? (sto/get-object storage (:media-id fmo1)))) - (t/is (some? (sto/get-object storage (:thumbnail-id fmo1))))) + (t/is (some? (sto/get-object storage (:thumbnail-id fmo1)))) + + ;; but if we pass the touched gc task two of them should disappear + (let [task (:app.storage/gc-touched-task th/*system*) + res (task {})] + (t/is (= 0 (:freeze res))) + (t/is (= 2 (:delete res))) + + (t/is (nil? (sto/get-object storage (:media-id fmo2)))) + (t/is (nil? (sto/get-object storage (:thumbnail-id fmo2)))) + (t/is (some? (sto/get-object storage (:media-id fmo1)))) + (t/is (some? (sto/get-object storage (:thumbnail-id fmo1))))) + + ))) + +(t/deftest permissions-checks-creating-file + (let [profile1 (th/create-profile* 1) + profile2 (th/create-profile* 2) + + data {::th/type :create-file + :profile-id (:id profile2) + :project-id (:default-project-id profile1) + :name "foobar" + :is-shared false} + out (th/mutation! data) + error (:error out)] + + ;; (th/print-result! out) + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + +(t/deftest permissions-checks-rename-file + (let [profile1 (th/create-profile* 1) + profile2 (th/create-profile* 2) + + file (th/create-file* 1 {:project-id (:default-project-id profile1) + :profile-id (:id profile1)}) + data {::th/type :rename-file + :id (:id file) + :profile-id (:id profile2) + :name "foobar"} + out (th/mutation! data) + error (:error out)] + + ;; (th/print-result! out) + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + +(t/deftest permissions-checks-delete-file + (let [profile1 (th/create-profile* 1) + profile2 (th/create-profile* 2) + + file (th/create-file* 1 {:project-id (:default-project-id profile1) + :profile-id (:id profile1)}) + data {::th/type :delete-file + :profile-id (:id profile2) + :id (:id file)} + out (th/mutation! data) + error (:error out)] + + ;; (th/print-result! out) + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + +(t/deftest permissions-checks-set-file-shared + (let [profile1 (th/create-profile* 1) + profile2 (th/create-profile* 2) + file (th/create-file* 1 {:project-id (:default-project-id profile1) + :profile-id (:id profile1)}) + data {::th/type :set-file-shared + :profile-id (:id profile2) + :id (:id file) + :is-shared true} + out (th/mutation! data) + error (:error out)] + + ;; (th/print-result! out) + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + +(t/deftest permissions-checks-link-to-library-1 + (let [profile1 (th/create-profile* 1) + profile2 (th/create-profile* 2) + file1 (th/create-file* 1 {:project-id (:default-project-id profile1) + :profile-id (:id profile1) + :is-shared true}) + file2 (th/create-file* 2 {:project-id (:default-project-id profile1) + :profile-id (:id profile1)}) + + data {::th/type :link-file-to-library + :profile-id (:id profile2) + :file-id (:id file2) + :library-id (:id file1)} + + out (th/mutation! data) + error (:error out)] + + ;; (th/print-result! out) + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + +(t/deftest permissions-checks-link-to-library-2 + (let [profile1 (th/create-profile* 1) + profile2 (th/create-profile* 2) + file1 (th/create-file* 1 {:project-id (:default-project-id profile1) + :profile-id (:id profile1) + :is-shared true}) + + file2 (th/create-file* 2 {:project-id (:default-project-id profile2) + :profile-id (:id profile2)}) + + data {::th/type :link-file-to-library + :profile-id (:id profile2) + :file-id (:id file2) + :library-id (:id file1)} + + out (th/mutation! data) + error (:error out)] + + ;; (th/print-result! out) + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) - )) diff --git a/backend/tests/app/tests/test_services_projects.clj b/backend/tests/app/tests/test_services_projects.clj index 4d638a41b..1b7f4f08b 100644 --- a/backend/tests/app/tests/test_services_projects.clj +++ b/backend/tests/app/tests/test_services_projects.clj @@ -19,15 +19,15 @@ (t/use-fixtures :once th/state-init) (t/use-fixtures :each th/database-reset) -(t/deftest projects-crud - (let [prof (th/create-profile* 1) - team (th/create-team* 1 {:profile-id (:id prof)}) +(t/deftest projects-simple-crud + (let [profile (th/create-profile* 1) + team (th/create-team* 1 {:profile-id (:id profile)}) project-id (uuid/next)] ;; crate project (let [data {::th/type :create-project :id project-id - :profile-id (:id prof) + :profile-id (:id profile) :team-id (:id team) :name "test project"} out (th/mutation! data)] @@ -40,7 +40,7 @@ ;; query a list of projects (let [data {::th/type :projects :team-id (:id team) - :profile-id (:id prof)} + :profile-id (:id profile)} out (th/query! data)] ;; (th/print-result! out) @@ -54,7 +54,7 @@ (let [data {::th/type :rename-project :id project-id :name "renamed project" - :profile-id (:id prof)} + :profile-id (:id profile)} out (th/mutation! data)] ;; (th/print-result! out) (t/is (nil? (:error out))) @@ -63,7 +63,7 @@ ;; retrieve project (let [data {::th/type :project :id project-id - :profile-id (:id prof)} + :profile-id (:id profile)} out (th/query! data)] ;; (th/print-result! out) (t/is (nil? (:error out))) @@ -73,7 +73,7 @@ ;; delete project (let [data {::th/type :delete-project :id project-id - :profile-id (:id prof)} + :profile-id (:id profile)} out (th/mutation! data)] ;; (th/print-result! out) @@ -83,10 +83,75 @@ ;; query a list of projects after delete" (let [data {::th/type :projects :team-id (:id team) - :profile-id (:id prof)} + :profile-id (:id profile)} out (th/query! data)] ;; (th/print-result! out) (t/is (nil? (:error out))) (let [result (:result out)] (t/is (= 0 (count result))))) )) + +(t/deftest permissions-checks-create-project + (let [profile1 (th/create-profile* 1) + profile2 (th/create-profile* 2) + + data {::th/type :create-project + :profile-id (:id profile2) + :team-id (:default-team-id profile1) + :name "test project"} + out (th/mutation! data) + error (:error out)] + + ;; (th/print-result! out) + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + +(t/deftest permissions-checks-rename-project + (let [profile1 (th/create-profile* 1) + profile2 (th/create-profile* 2) + project (th/create-project* 1 {:team-id (:default-team-id profile1) + :profile-id (:id profile1)}) + data {::th/type :rename-project + :id (:id project) + :profile-id (:id profile2) + :name "foobar"} + out (th/mutation! data) + error (:error out)] + + ;; (th/print-result! out) + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + +(t/deftest permissions-checks-delete-project + (let [profile1 (th/create-profile* 1) + profile2 (th/create-profile* 2) + project (th/create-project* 1 {:team-id (:default-team-id profile1) + :profile-id (:id profile1)}) + data {::th/type :delete-project + :id (:id project) + :profile-id (:id profile2)} + out (th/mutation! data) + error (:error out)] + + ;; (th/print-result! out) + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) + +(t/deftest permissions-checks-delete-project + (let [profile1 (th/create-profile* 1) + profile2 (th/create-profile* 2) + project (th/create-project* 1 {:team-id (:default-team-id profile1) + :profile-id (:id profile1)}) + data {::th/type :update-project-pin + :id (:id project) + :team-id (:default-team-id profile1) + :profile-id (:id profile2) + :is-pinned true} + + out (th/mutation! data) + error (:error out)] + + ;; (th/print-result! out) + (t/is (th/ex-info? error)) + (t/is (th/ex-of-type? error :not-found)))) +