From d4c775b1f4985be4e279348ce027ef7402f8a498 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 16 Oct 2024 17:15:30 +0200 Subject: [PATCH 1/4] :bug: Fix unexpected rare condition exception on rpc cond middleware --- backend/src/app/rpc/cond.clj | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/backend/src/app/rpc/cond.clj b/backend/src/app/rpc/cond.clj index 3fe03c821..bf724d13f 100644 --- a/backend/src/app/rpc/cond.clj +++ b/backend/src/app/rpc/cond.clj @@ -48,20 +48,20 @@ (str "W/\"" (encode s) "\"")) (defn wrap - [_ f {:keys [::get-object ::key-fn ::reuse-key?] :as mdata}] + [_ f {:keys [::get-object ::key-fn ::reuse-key?] :or {reuse-key? true} :as mdata}] (if (and (ifn? get-object) (ifn? key-fn)) (do (l/trc :hint "instrumenting method" :service (::sv/name mdata)) (fn [cfg {:keys [::key] :as params}] (if *enabled* - (let [key' (when (or key reuse-key?) + (let [key' (when (or reuse-key? key) (some->> (get-object cfg params) (key-fn params) (fmt-key)))] (if (and (some? key) (= key key')) (fn [_] {::rres/status 304}) (let [result (f cfg params) etag (or (and reuse-key? key') - (some-> result meta ::key fmt-key) - (some-> result key-fn fmt-key))] + (some->> result meta ::key fmt-key) + (some->> result (key-fn params) fmt-key))] (rph/with-header result "etag" etag)))) (f cfg params)))) f)) From 22d7cfc7fa1dc2e662a1c3e69ba09e37bd281d8f Mon Sep 17 00:00:00 2001 From: Pablo Alba Date: Tue, 15 Oct 2024 16:06:33 +0200 Subject: [PATCH 2/4] :bug: Fix missing permissions on file cache --- backend/src/app/rpc/commands/files.clj | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/backend/src/app/rpc/commands/files.clj b/backend/src/app/rpc/commands/files.clj index dcd54e7ce..c8aa56204 100644 --- a/backend/src/app/rpc/commands/files.clj +++ b/backend/src/app/rpc/commands/files.clj @@ -17,6 +17,7 @@ [app.common.schema.desc-js-like :as-alias smdj] [app.common.types.components-list :as ctkl] [app.common.types.file :as ctf] + [app.common.uri :as uri] [app.config :as cf] [app.db :as db] [app.db.sql :as-alias sql] @@ -272,14 +273,23 @@ (let [opts (assoc opts ::sql/columns [:id :modified-at :deleted-at :revn :data-ref-id :data-backend])] (db/get cfg :file {:id id} opts))) +(defn- get-minimal-file-with-perms + [cfg {:keys [:id ::rpc/profile-id]}] + (let [mfile (get-minimal-file cfg id) + perms (get-permissions cfg profile-id id)] + (assoc mfile :permissions perms))) + (defn get-file-etag - [{:keys [::rpc/profile-id]} {:keys [modified-at revn]}] - (str profile-id (dt/format-instant modified-at :iso) revn)) + [{:keys [::rpc/profile-id]} {:keys [modified-at revn permissions]}] + (str profile-id "/" revn "/" + (dt/format-instant modified-at :iso) + "/" + (uri/map->query-string permissions))) (sv/defmethod ::get-file "Retrieve a file by its ID. Only authenticated users." {::doc/added "1.17" - ::cond/get-object #(get-minimal-file %1 (:id %2)) + ::cond/get-object #(get-minimal-file-with-perms %1 %2) ::cond/key-fn get-file-etag ::sm/params schema:get-file ::sm/result schema:file-with-permissions} @@ -308,8 +318,7 @@ (binding [pmap/*load-fn* (partial feat.fdata/load-pointer cfg id)] (update file :data feat.fdata/process-pointers deref)) file)] - - (vary-meta file assoc ::cond/key (get-file-etag params file))))))) + file))))) ;; --- COMMAND QUERY: get-file-fragment (by id) From 40d7bb04b470ef036429e3592407bfa14130fd1f Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 16 Oct 2024 17:35:47 +0200 Subject: [PATCH 3/4] :sparkles: Reuse permission from rpc/cond middleware for get-file rpc method --- backend/src/app/rpc/commands/files.clj | 10 +++++++++- backend/src/app/rpc/cond.clj | 11 ++++++++--- .../test/backend_tests/rpc_cond_middleware_test.clj | 1 - 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/backend/src/app/rpc/commands/files.clj b/backend/src/app/rpc/commands/files.clj index c8aa56204..1ca28fd64 100644 --- a/backend/src/app/rpc/commands/files.clj +++ b/backend/src/app/rpc/commands/files.clj @@ -295,8 +295,16 @@ ::sm/result schema:file-with-permissions} [cfg {:keys [::rpc/profile-id id project-id] :as params}] (db/tx-run! cfg (fn [{:keys [::db/conn] :as cfg}] - (let [perms (get-permissions conn profile-id id)] + ;; The COND middleware makes initial request for a file and + ;; permissions when the incoming request comes with an + ;; ETAG. When ETAG does not matches, the request is resolved + ;; and this code is executed, in this case the permissions + ;; will be already prefetched and we just reuse them instead + ;; of making an additional database queries. + (let [perms (or (:permissions (::cond/object params)) + (get-permissions conn profile-id id))] (check-read-permissions! perms) + (let [team (teams/get-team conn :profile-id profile-id :project-id project-id diff --git a/backend/src/app/rpc/cond.clj b/backend/src/app/rpc/cond.clj index bf724d13f..2c79d8f66 100644 --- a/backend/src/app/rpc/cond.clj +++ b/backend/src/app/rpc/cond.clj @@ -54,11 +54,16 @@ (l/trc :hint "instrumenting method" :service (::sv/name mdata)) (fn [cfg {:keys [::key] :as params}] (if *enabled* - (let [key' (when (or reuse-key? key) - (some->> (get-object cfg params) (key-fn params) (fmt-key)))] + (let [object (when (some? key) + (get-object cfg params)) + key' (when (some? object) + (->> object (key-fn params) (fmt-key)))] (if (and (some? key) (= key key')) (fn [_] {::rres/status 304}) - (let [result (f cfg params) + (let [params (if (some? object) + (assoc params ::object object) + params) + result (f cfg params) etag (or (and reuse-key? key') (some->> result meta ::key fmt-key) (some->> result (key-fn params) fmt-key))] diff --git a/backend/test/backend_tests/rpc_cond_middleware_test.clj b/backend/test/backend_tests/rpc_cond_middleware_test.clj index e74a9c549..e737fc5f5 100644 --- a/backend/test/backend_tests/rpc_cond_middleware_test.clj +++ b/backend/test/backend_tests/rpc_cond_middleware_test.clj @@ -39,7 +39,6 @@ (t/is (nil? error)) (t/is (map? result)) (t/is (contains? (meta result) :app.http/headers)) - (t/is (contains? (meta result) :app.rpc.cond/key)) (let [etag (-> result meta :app.http/headers (get "etag")) {:keys [error result]} (th/command! (assoc params ::cond/key etag))] From 790f6ce4edbeb0a6f1645c305d5109586688a4f2 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 16 Oct 2024 17:37:53 +0200 Subject: [PATCH 4/4] :lipstick: Add cosmetic changes to get-file rpc method --- backend/src/app/rpc/commands/files.clj | 61 +++++++++++++------------- 1 file changed, 30 insertions(+), 31 deletions(-) diff --git a/backend/src/app/rpc/commands/files.clj b/backend/src/app/rpc/commands/files.clj index 1ca28fd64..cdf4a0fb9 100644 --- a/backend/src/app/rpc/commands/files.clj +++ b/backend/src/app/rpc/commands/files.clj @@ -292,41 +292,40 @@ ::cond/get-object #(get-minimal-file-with-perms %1 %2) ::cond/key-fn get-file-etag ::sm/params schema:get-file - ::sm/result schema:file-with-permissions} - [cfg {:keys [::rpc/profile-id id project-id] :as params}] - (db/tx-run! cfg (fn [{:keys [::db/conn] :as cfg}] - ;; The COND middleware makes initial request for a file and - ;; permissions when the incoming request comes with an - ;; ETAG. When ETAG does not matches, the request is resolved - ;; and this code is executed, in this case the permissions - ;; will be already prefetched and we just reuse them instead - ;; of making an additional database queries. - (let [perms (or (:permissions (::cond/object params)) - (get-permissions conn profile-id id))] - (check-read-permissions! perms) + ::sm/result schema:file-with-permissions + ::db/transaction true} + [{:keys [::db/conn] :as cfg} {:keys [::rpc/profile-id id project-id] :as params}] + ;; The COND middleware makes initial request for a file and + ;; permissions when the incoming request comes with an + ;; ETAG. When ETAG does not matches, the request is resolved + ;; and this code is executed, in this case the permissions + ;; will be already prefetched and we just reuse them instead + ;; of making an additional database queries. + (let [perms (or (:permissions (::cond/object params)) + (get-permissions conn profile-id id))] + (check-read-permissions! perms) - (let [team (teams/get-team conn - :profile-id profile-id - :project-id project-id - :file-id id) + (let [team (teams/get-team conn + :profile-id profile-id + :project-id project-id + :file-id id) - file (-> (get-file cfg id :project-id project-id) - (assoc :permissions perms) - (check-version!)) + file (-> (get-file cfg id :project-id project-id) + (assoc :permissions perms) + (check-version!))] - _ (-> (cfeat/get-team-enabled-features cf/flags team) - (cfeat/check-client-features! (:features params)) - (cfeat/check-file-features! (:features file) (:features params))) + (-> (cfeat/get-team-enabled-features cf/flags team) + (cfeat/check-client-features! (:features params)) + (cfeat/check-file-features! (:features file) (:features params))) - ;; This operation is needed for backward comapatibility with frontends that - ;; does not support pointer-map resolution mechanism; this just resolves the - ;; pointers on backend and return a complete file. - file (if (and (contains? (:features file) "fdata/pointer-map") - (not (contains? (:features params) "fdata/pointer-map"))) - (binding [pmap/*load-fn* (partial feat.fdata/load-pointer cfg id)] - (update file :data feat.fdata/process-pointers deref)) - file)] - file))))) + ;; This operation is needed for backward comapatibility with frontends that + ;; does not support pointer-map resolution mechanism; this just resolves the + ;; pointers on backend and return a complete file. + (if (and (contains? (:features file) "fdata/pointer-map") + (not (contains? (:features params) "fdata/pointer-map"))) + (binding [pmap/*load-fn* (partial feat.fdata/load-pointer cfg id)] + (update file :data feat.fdata/process-pointers deref)) + file)))) ;; --- COMMAND QUERY: get-file-fragment (by id)