From f02bc82525a3bf2aa4154a5b8263f534f4cabe7b Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 15 Oct 2021 17:40:49 +0200 Subject: [PATCH] :sparkles: Make permissions subsystem more flexible. And fix some bugs related to permissions. --- backend/src/app/rpc.clj | 1 - backend/src/app/rpc/mutations/teams.clj | 8 ++-- backend/src/app/rpc/mutations/viewer.clj | 49 -------------------- backend/src/app/rpc/permissions.clj | 49 +++----------------- backend/src/app/rpc/queries/files.clj | 48 +++++++++++++++---- backend/src/app/rpc/queries/projects.clj | 27 ++++++++--- backend/src/app/rpc/queries/share_link.clj | 23 +++++++++ backend/src/app/rpc/queries/teams.clj | 41 +++++++++++++--- backend/src/app/rpc/queries/viewer.clj | 30 +++--------- backend/test/app/services_teams_test.clj | 6 ++- frontend/src/app/main/ui/dashboard/team.cljs | 32 +++++++------ frontend/src/app/main/ui/viewer/header.cljs | 10 ++-- 12 files changed, 162 insertions(+), 162 deletions(-) delete mode 100644 backend/src/app/rpc/mutations/viewer.clj create mode 100644 backend/src/app/rpc/queries/share_link.clj diff --git a/backend/src/app/rpc.clj b/backend/src/app/rpc.clj index 2df73c131..16fa2d1ad 100644 --- a/backend/src/app/rpc.clj +++ b/backend/src/app/rpc.clj @@ -169,7 +169,6 @@ 'app.rpc.mutations.files 'app.rpc.mutations.comments 'app.rpc.mutations.projects - 'app.rpc.mutations.viewer 'app.rpc.mutations.teams 'app.rpc.mutations.management 'app.rpc.mutations.ldap diff --git a/backend/src/app/rpc/mutations/teams.clj b/backend/src/app/rpc/mutations/teams.clj index 0d385fa2e..625a13af6 100644 --- a/backend/src/app/rpc/mutations/teams.clj +++ b/backend/src/app/rpc/mutations/teams.clj @@ -132,8 +132,8 @@ (sv/defmethod ::delete-team [{:keys [pool] :as cfg} {:keys [id profile-id] :as params}] (db/with-atomic [conn pool] - (let [perms (teams/check-edition-permissions! conn profile-id id)] - (when-not (some :is-owner perms) + (let [perms (teams/get-permissions conn profile-id id)] + (when-not (:is-owner perms) (ex/raise :type :validation :code :only-owner-can-delete-team)) @@ -300,7 +300,7 @@ (sv/defmethod ::invite-team-member [{:keys [pool tokens] :as cfg} {:keys [profile-id team-id email role] :as params}] (db/with-atomic [conn pool] - (let [perms (teams/check-edition-permissions! conn profile-id team-id) + (let [perms (teams/get-permissions conn profile-id team-id) profile (db/get-by-id conn :profile profile-id) member (profile/retrieve-profile-data-by-email conn email) team (db/get-by-id conn :team team-id) @@ -316,7 +316,7 @@ {:iss :profile-identity :profile-id (:id profile)})] - (when-not (some :is-admin perms) + (when-not (:is-admin perms) (ex/raise :type :validation :code :insufficient-permissions)) diff --git a/backend/src/app/rpc/mutations/viewer.clj b/backend/src/app/rpc/mutations/viewer.clj deleted file mode 100644 index 85beafa96..000000000 --- a/backend/src/app/rpc/mutations/viewer.clj +++ /dev/null @@ -1,49 +0,0 @@ -;; This Source Code Form is subject to the terms of the Mozilla Public -;; License, v. 2.0. If a copy of the MPL was not distributed with this -;; file, You can obtain one at http://mozilla.org/MPL/2.0/. -;; -;; Copyright (c) UXBOX Labs SL - -(ns app.rpc.mutations.viewer - (:require - [app.common.spec :as us] - [app.db :as db] - [app.rpc.queries.files :as files] - [app.util.services :as sv] - [buddy.core.codecs :as bc] - [buddy.core.nonce :as bn] - [clojure.spec.alpha :as s])) - -(s/def ::profile-id ::us/uuid) -(s/def ::file-id ::us/uuid) -(s/def ::page-id ::us/uuid) - -(s/def ::create-file-share-token - (s/keys :req-un [::profile-id ::file-id ::page-id])) - -(sv/defmethod ::create-file-share-token - [{:keys [pool] :as cfg} {:keys [profile-id file-id page-id] :as params}] - (db/with-atomic [conn pool] - (files/check-edition-permissions! conn profile-id file-id) - (let [token (-> (bn/random-bytes 16) - (bc/bytes->b64u) - (bc/bytes->str))] - (db/insert! conn :file-share-token - {:file-id file-id - :page-id page-id - :token token}) - {:token token}))) - - -(s/def ::token ::us/not-empty-string) -(s/def ::delete-file-share-token - (s/keys :req-un [::profile-id ::file-id ::token])) - -(sv/defmethod ::delete-file-share-token - [{:keys [pool] :as cfg} {:keys [profile-id file-id token]}] - (db/with-atomic [conn pool] - (files/check-edition-permissions! conn profile-id file-id) - (db/delete! conn :file-share-token - {:file-id file-id - :token token}) - nil)) diff --git a/backend/src/app/rpc/permissions.clj b/backend/src/app/rpc/permissions.clj index 9481bcd57..363f967e6 100644 --- a/backend/src/app/rpc/permissions.clj +++ b/backend/src/app/rpc/permissions.clj @@ -41,59 +41,24 @@ "A simple factory for edition permission predicate functions." [qfn] (us/assert fn? qfn) - (fn [& args] - (let [rows (apply qfn args)] - (when-not (or (empty? rows) - (not (or (some :can-edit rows) - (some :is-admin rows) - (some :is-owner rows)))) - rows)))) + (fn check + ([perms] (:can-edit perms)) + ([conn & args] (check (apply qfn conn args))))) (defn make-read-predicate-fn "A simple factory for read permission predicate functions." [qfn] (us/assert fn? qfn) - (fn [& args] - (let [rows (apply qfn args)] - (when (seq rows) - rows)))) + (fn check + ([perms] (:can-read perms)) + ([conn & args] (check (apply qfn conn args))))) (defn make-check-fn "Helper that converts a predicate permission function to a check function (function that raises an exception)." [pred] (fn [& args] - (when-not (seq (apply pred args)) + (when-not (apply pred args) (ex/raise :type :not-found :code :object-not-found :hint "not found")))) - - -;; TODO: the following functions are deprecated and replaced with the -;; new ones. Should not be used. - -(defn make-edition-check-fn - "A simple factory for edition permission check functions." - [qfn] - (us/assert fn? qfn) - (fn [& args] - (let [rows (apply qfn args)] - (if (or (empty? rows) - (not (or (some :can-edit rows) - (some :is-admin rows) - (some :is-owner rows)))) - (ex/raise :type :not-found - :code :object-not-found - :hint "not found") - rows)))) - -(defn make-read-check-fn - "A simple factory for read permission check functions." - [qfn] - (us/assert fn? qfn) - (fn [& args] - (let [rows (apply qfn args)] - (if-not (seq rows) - (ex/raise :type :not-found - :code :object-not-found) - rows)))) diff --git a/backend/src/app/rpc/queries/files.clj b/backend/src/app/rpc/queries/files.clj index 25ae081e1..c6974472e 100644 --- a/backend/src/app/rpc/queries/files.clj +++ b/backend/src/app/rpc/queries/files.clj @@ -12,6 +12,7 @@ [app.db :as db] [app.rpc.permissions :as perms] [app.rpc.queries.projects :as projects] + [app.rpc.queries.share-link :refer [retrieve-share-link]] [app.rpc.queries.teams :as teams] [app.storage.impl :as simpl] [app.util.blob :as blob] @@ -59,7 +60,7 @@ where f.id = ? and ppr.profile_id = ?") -(defn- retrieve-file-permissions +(defn retrieve-file-permissions [conn profile-id file-id] (when (and profile-id file-id) (db/exec! conn [sql:file-permissions @@ -67,11 +68,37 @@ file-id profile-id file-id profile-id]))) +(defn get-permissions + ([conn profile-id file-id] + (let [rows (retrieve-file-permissions conn profile-id file-id) + is-owner (boolean (some :is-owner rows)) + is-admin (boolean (some :is-admin rows)) + can-edit (boolean (some :can-edit rows))] + (when (seq rows) + {:type :membership + :is-owner is-owner + :is-admin (or is-owner is-admin) + :can-edit (or is-owner is-admin can-edit) + :can-read true}))) + ([conn profile-id file-id share-id] + (let [perms (get-permissions conn profile-id file-id) + ldata (retrieve-share-link conn file-id share-id)] + + ;; NOTE: in a future when share-link becomes more powerfull and + ;; will allow us specify which parts of the app is availabel, we + ;; will probably need to tweak this function in order to expose + ;; this flags to the frontend. + (cond + (some? perms) perms + (some? ldata) {:type :share-link + :can-read true + :flags (:flags ldata)})))) + (def has-edit-permissions? - (perms/make-edition-predicate-fn retrieve-file-permissions)) + (perms/make-edition-predicate-fn get-permissions)) (def has-read-permissions? - (perms/make-read-predicate-fn retrieve-file-permissions)) + (perms/make-read-predicate-fn get-permissions)) (def check-edition-permissions! (perms/make-check-fn has-edit-permissions?)) @@ -79,7 +106,6 @@ (def check-read-permissions! (perms/make-check-fn has-read-permissions?)) - ;; --- Query: Files search ;; TODO: this query need to a good refactor @@ -180,9 +206,12 @@ (sv/defmethod ::file [{:keys [pool] :as cfg} {:keys [profile-id id] :as params}] (db/with-atomic [conn pool] - (let [cfg (assoc cfg :conn conn)] - (check-edition-permissions! conn profile-id id) - (retrieve-file cfg id)))) + (let [cfg (assoc cfg :conn conn) + perms (get-permissions conn profile-id id)] + + (check-read-permissions! perms) + (some-> (retrieve-file cfg id) + (assoc :permissions perms))))) (s/def ::page (s/keys :req-un [::profile-id ::file-id])) @@ -217,7 +246,8 @@ (sv/defmethod ::page [{:keys [pool] :as cfg} {:keys [profile-id file-id strip-thumbnails]}] (db/with-atomic [conn pool] - (check-edition-permissions! conn profile-id file-id) + (check-read-permissions! conn profile-id file-id) + (let [cfg (assoc cfg :conn conn) file (retrieve-file cfg file-id) page-id (get-in file [:data :pages 0])] @@ -291,7 +321,7 @@ [{:keys [pool] :as cfg} {:keys [profile-id file-id] :as params}] (db/with-atomic [conn pool] (let [cfg (assoc cfg :conn conn)] - (check-edition-permissions! conn profile-id file-id) + (check-read-permissions! conn profile-id file-id) (retrieve-file-libraries cfg false file-id)))) ;; --- QUERY: team-recent-files diff --git a/backend/src/app/rpc/queries/projects.clj b/backend/src/app/rpc/queries/projects.clj index 29195ceb0..47886784a 100644 --- a/backend/src/app/rpc/queries/projects.clj +++ b/backend/src/app/rpc/queries/projects.clj @@ -31,18 +31,31 @@ where ppr.project_id = ? and ppr.profile_id = ?") -(defn- retrieve-project-permissions +(defn- get-permissions [conn profile-id project-id] - (db/exec! conn [sql:project-permissions - project-id profile-id - project-id profile-id])) + (let [rows (db/exec! conn [sql:project-permissions + project-id profile-id + project-id profile-id]) + is-owner (boolean (some :is-owner rows)) + is-admin (boolean (some :is-admin rows)) + can-edit (boolean (some :can-edit rows))] + (when (seq rows) + {:is-owner is-owner + :is-admin (or is-owner is-admin) + :can-edit (or is-owner is-admin can-edit) + :can-read true}))) + +(def has-edit-permissions? + (perms/make-edition-predicate-fn get-permissions)) + +(def has-read-permissions? + (perms/make-read-predicate-fn get-permissions)) (def check-edition-permissions! - (perms/make-edition-check-fn retrieve-project-permissions)) + (perms/make-check-fn has-edit-permissions?)) (def check-read-permissions! - (perms/make-read-check-fn retrieve-project-permissions)) - + (perms/make-check-fn has-read-permissions?)) ;; --- Query: Projects diff --git a/backend/src/app/rpc/queries/share_link.clj b/backend/src/app/rpc/queries/share_link.clj new file mode 100644 index 000000000..0bc567364 --- /dev/null +++ b/backend/src/app/rpc/queries/share_link.clj @@ -0,0 +1,23 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) UXBOX Labs SL + +(ns app.rpc.queries.share-link + (:require + [app.db :as db])) + +(defn decode-share-link-row + [row] + (-> row + (update :flags db/decode-pgarray #{}) + (update :pages db/decode-pgarray #{}))) + +(defn retrieve-share-link + [conn file-id share-id] + (some-> (db/get-by-params conn :share-link + {:id share-id :file-id file-id} + {:check-not-found false}) + (decode-share-link-row))) + diff --git a/backend/src/app/rpc/queries/teams.clj b/backend/src/app/rpc/queries/teams.clj index 0404d300d..407237237 100644 --- a/backend/src/app/rpc/queries/teams.clj +++ b/backend/src/app/rpc/queries/teams.clj @@ -24,16 +24,29 @@ where tpr.profile_id = ? and tpr.team_id = ?") -(defn- retrieve-team-permissions +(defn get-permissions [conn profile-id team-id] - (db/exec! conn [sql:team-permissions profile-id team-id])) + (let [rows (db/exec! conn [sql:team-permissions profile-id team-id]) + is-owner (boolean (some :is-owner rows)) + is-admin (boolean (some :is-admin rows)) + can-edit (boolean (some :can-edit rows))] + (when (seq rows) + {:is-owner is-owner + :is-admin (or is-owner is-admin) + :can-edit (or is-owner is-admin can-edit) + :can-read true}))) + +(def has-edit-permissions? + (perms/make-edition-predicate-fn get-permissions)) + +(def has-read-permissions? + (perms/make-read-predicate-fn get-permissions)) (def check-edition-permissions! - (perms/make-edition-check-fn retrieve-team-permissions)) + (perms/make-check-fn has-edit-permissions?)) (def check-read-permissions! - (perms/make-read-check-fn retrieve-team-permissions)) - + (perms/make-check-fn has-read-permissions?)) ;; --- Query: Teams @@ -60,10 +73,24 @@ and tp.profile_id = ? order by tp.created_at asc") +(defn process-permissions + [team] + (let [is-owner (:is-owner team) + is-admin (:is-admin team) + can-edit (:can-edit team) + permissions {:type :membership + :is-owner is-owner + :is-admin (or is-owner is-admin) + :can-edit (or is-owner is-admin can-edit)}] + (-> team + (dissoc :is-owner :is-admin :can-edit) + (assoc :permissions permissions)))) + (defn retrieve-teams [conn profile-id] (let [defaults (profile/retrieve-additional-data conn profile-id)] - (db/exec! conn [sql:teams (:default-team-id defaults) profile-id]))) + (->> (db/exec! conn [sql:teams (:default-team-id defaults) profile-id]) + (mapv process-permissions)))) ;; --- Query: Team (by ID) @@ -86,7 +113,7 @@ (when-not result (ex/raise :type :not-found :code :team-does-not-exist)) - result)) + (process-permissions result))) ;; --- Query: Team Members diff --git a/backend/src/app/rpc/queries/viewer.clj b/backend/src/app/rpc/queries/viewer.clj index aa31b0b65..9d35f9e07 100644 --- a/backend/src/app/rpc/queries/viewer.clj +++ b/backend/src/app/rpc/queries/viewer.clj @@ -10,29 +10,17 @@ [app.common.spec :as us] [app.db :as db] [app.rpc.queries.files :as files] + [app.rpc.queries.share-link :as slnk] [app.rpc.queries.teams :as teams] [app.util.services :as sv] [clojure.spec.alpha :as s])) ;; --- Query: View Only Bundle -(defn- decode-share-link-row - [row] - (-> row - (update :flags db/decode-pgarray #{}) - (update :pages db/decode-pgarray #{}))) - (defn- retrieve-project [conn id] (db/get-by-id conn :project id {:columns [:id :name :team-id]})) -(defn- retrieve-share-link - [{:keys [conn]} file-id id] - (some-> (db/get-by-params conn :share-link - {:id id :file-id file-id} - {:check-not-found false}) - (decode-share-link-row))) - (defn- retrieve-bundle [{:keys [conn] :as cfg} file-id] (let [file (files/retrieve-file cfg file-id) @@ -41,7 +29,7 @@ users (teams/retrieve-users conn (:team-id project)) links (->> (db/query conn :share-link {:file-id file-id}) - (mapv decode-share-link-row)) + (mapv slnk/decode-share-link-row)) fonts (db/query conn :team-font-variant {:team-id (:team-id project) @@ -64,8 +52,11 @@ [{:keys [pool] :as cfg} {:keys [profile-id file-id share-id] :as params}] (db/with-atomic [conn pool] (let [cfg (assoc cfg :conn conn) - bundle (retrieve-bundle cfg file-id) - slink (retrieve-share-link cfg file-id share-id)] + slink (slnk/retrieve-share-link conn file-id share-id) + perms (files/get-permissions conn profile-id file-id share-id) + + bundle (some-> (retrieve-bundle cfg file-id) + (assoc :permissions perms))] ;; When we have neither profile nor share, we just return a not ;; found response to the user. @@ -80,13 +71,6 @@ (files/check-read-permissions! conn profile-id file-id)) (cond-> bundle - ;; If we have current profile, put - (some? profile-id) - (as-> $ (let [edit? (boolean (files/has-edit-permissions? conn profile-id file-id)) - read? (boolean (files/has-read-permissions? conn profile-id file-id))] - (-> (assoc $ :permissions {:read read? :edit edit?}) - (cond-> (not edit?) (dissoc :share-links))))) - (some? slink) (assoc :share slink) diff --git a/backend/test/app/services_teams_test.clj b/backend/test/app/services_teams_test.clj index 2831ea4ee..6dd7470da 100644 --- a/backend/test/app/services_teams_test.clj +++ b/backend/test/app/services_teams_test.clj @@ -33,11 +33,13 @@ :role :editor :profile-id (:id profile1)}] - ;; (th/print-result! out) ;; invite external user without complaints (let [data (assoc data :email "foo@bar.com") out (th/mutation! data)] + + ;; (th/print-result! out) + (t/is (nil? (:result out))) (t/is (= 1 (:call-count (deref mock))))) @@ -111,6 +113,7 @@ :id (:id team) :profile-id (:id profile1)} out (th/mutation! params)] + ;; (th/print-result! out) (t/is (nil? (:error out)))) ;; query the list of teams after soft deletion @@ -133,7 +136,6 @@ :profile-id (:id profile1)} out (th/query! data)] ;; (th/print-result! out) - (t/is (nil? (:error out))) (let [result (:result out)] (t/is (= 0 (count result))))) diff --git a/frontend/src/app/main/ui/dashboard/team.cljs b/frontend/src/app/main/ui/dashboard/team.cljs index 4c87aa4d1..bf0a8bf5a 100644 --- a/frontend/src/app/main/ui/dashboard/team.cljs +++ b/frontend/src/app/main/ui/dashboard/team.cljs @@ -26,12 +26,13 @@ (mf/defc header {::mf/wrap [mf/memo]} - [{:keys [section] :as props}] + [{:keys [section team] :as props}] (let [go-members (st/emitf (dd/go-to-team-members)) go-settings (st/emitf (dd/go-to-team-settings)) - invite-member (st/emitf (modal/show {:type ::invite-member})) + invite-member (st/emitf (modal/show {:type ::invite-member :team team})) members-section? (= section :dashboard-team-members) - settings-section? (= section :dashboard-team-settings)] + settings-section? (= section :dashboard-team-settings) + permissions (:permissions team)] [:header.dashboard-header [:div.dashboard-title @@ -46,20 +47,21 @@ [:li {:class (when settings-section? "active")} [:a {:on-click go-settings} (tr "labels.settings")]]]] - (if members-section? + (if (and members-section? (:is-admin permissions)) [:a.btn-secondary.btn-small {:on-click invite-member} (tr "dashboard.invite-profile")] [:div])])) (defn get-available-roles - [] - [{:value "" :label (tr "labels.role")} - {:value "admin" :label (tr "labels.admin")} - {:value "editor" :label (tr "labels.editor")} - ;; Temporarily disabled viewer role - ;; https://tree.taiga.io/project/uxboxproject/issue/1083 - ;; {:value "viewer" :label (tr "labels.viewer")} - ]) + [permissions] + (->> [{:value "editor" :label (tr "labels.editor")} + (when (:is-admin permissions) + {:value "admin" :label (tr "labels.admin")}) + ;; Temporarily disabled viewer role + ;; https://tree.taiga.io/project/uxboxproject/issue/1083 + ;; {:value "viewer" :label (tr "labels.viewer")} + ] + (filterv identity))) (s/def ::email ::us/email) (s/def ::role ::us/keyword) @@ -69,8 +71,9 @@ (mf/defc invite-member-modal {::mf/register modal/components ::mf/register-as ::invite-member} - [] - (let [roles (mf/use-memo get-available-roles) + [{:keys [team]}] + (let [perms (:permissions team) + roles (mf/use-memo (mf/deps perms) #(get-available-roles perms)) initial (mf/use-memo (constantly {:role "editor"})) form (fm/use-form :spec ::invite-member-form :initial initial) @@ -262,6 +265,7 @@ (tr "dashboard.your-penpot") (:name team)))))) + (mf/use-effect (st/emitf (dd/fetch-team-members) (dd/fetch-team-stats))) diff --git a/frontend/src/app/main/ui/viewer/header.cljs b/frontend/src/app/main/ui/viewer/header.cljs index 7cf9f23ed..2c838251e 100644 --- a/frontend/src/app/main/ui/viewer/header.cljs +++ b/frontend/src/app/main/ui/viewer/header.cljs @@ -67,10 +67,10 @@ i/full-screen-off i/full-screen)] - (when (:edit permissions) + (when (:is-admin permissions) [:span.btn-primary {:on-click open-share-dialog} (tr "labels.share-prototype")]) - (when (:edit permissions) + (when (:can-edit permissions) [:span.btn-text-dark {:on-click go-to-workspace} (tr "labels.edit-file")])])) (mf/defc header-sitemap @@ -151,14 +151,16 @@ :alt (tr "viewer.header.interactions-section")} i/play] - (when (:edit permissions) + (when (:can-edit permissions) [:button.mode-zone-button.tooltip.tooltip-bottom {:on-click #(navigate :comments) :class (dom/classnames :active (= section :comments)) :alt (tr "viewer.header.comments-section")} i/chat]) - (when (:read permissions) + (when (or (= (:type permissions) :membership) + (and (= (:type permissions) :share-link) + (contains? (:flags permissions) :section-handoff))) [:button.mode-zone-button.tooltip.tooltip-bottom {:on-click #(navigate :handoff) :class (dom/classnames :active (= section :handoff))