Merge pull request #5170 from penpot/palba-fix-missing-permisions-on-file-etag-cache

🐛 Fix missing permissions on file cache

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,44 +273,59 @@
   (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]}]
+  [{:keys [::rpc/profile-id]} {:keys [modified-at revn permissions]}]
-  (str profile-id (dt/format-instant modified-at :iso) revn))
+  (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}
+   ::sm/result schema:file-with-permissions
-  [cfg {:keys [::rpc/profile-id id project-id] :as params}]
+   ::db/transaction true}
-  (db/tx-run! cfg (fn [{:keys [::db/conn] :as cfg}]
+  [{:keys [::db/conn] :as cfg} {:keys [::rpc/profile-id id project-id] :as params}]
-                    (let [perms (get-permissions conn profile-id id)]
+  ;; The COND middleware makes initial request for a file and
-                      (check-read-permissions! perms)
+  ;; permissions when the incoming request comes with an
-                      (let [team (teams/get-team conn
+  ;; ETAG. When ETAG does not matches, the request is resolved
-                                                 :profile-id profile-id
+  ;; and this code is executed, in this case the permissions
-                                                 :project-id project-id
+  ;; will be already prefetched and we just reuse them instead
-                                                 :file-id id)
+  ;; of making an additional database queries.
+  (let [perms (or (:permissions (::cond/object params))
+                  (get-permissions conn profile-id id))]
+    (check-read-permissions! perms)
 
-                            file (-> (get-file cfg id :project-id project-id)
+    (let [team (teams/get-team conn
-                                     (assoc :permissions perms)
+                               :profile-id profile-id
-                                     (check-version!))
+                               :project-id project-id
+                               :file-id id)
 
-                            _    (-> (cfeat/get-team-enabled-features cf/flags team)
+          file (-> (get-file cfg id :project-id project-id)
-                                     (cfeat/check-client-features! (:features params))
+                   (assoc :permissions perms)
-                                     (cfeat/check-file-features! (:features file) (:features params)))
+                   (check-version!))]
 
-                            ;; This operation is needed for backward comapatibility with frontends that
+      (-> (cfeat/get-team-enabled-features cf/flags team)
-                            ;; does not support pointer-map resolution mechanism; this just resolves the
+          (cfeat/check-client-features! (:features params))
-                            ;; pointers on backend and return a complete file.
+          (cfeat/check-file-features! (:features file) (:features params)))
-                            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)]
 
-                        (vary-meta file assoc ::cond/key (get-file-etag params 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)
 

backend/src/app/rpc/cond.clj

@@ -48,20 +48,25 @@
   (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 [object (when (some? key)
-                       (some->> (get-object cfg params) (key-fn params) (fmt-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 meta ::key fmt-key)
-                               (some-> result key-fn fmt-key))]
+                               (some->> result (key-fn params) fmt-key))]
                 (rph/with-header result "etag" etag))))
           (f cfg params))))
     f))

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))]