3pp-mirror/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2025-08-06 05:58:29 +02:00
Code Issues Projects Releases Packages Wiki Activity

♻️ Fix some fundamental bugs on climit module

Browse source 
The climit previously of this commit is heavily used inside a
transactions, so in heavy contention operation such that file thumbnail
creation can cause a db pool exhaust.

This commit fixes this issue setting up a better resource limiting
mechanism that works outside the transactions so, contention will
no longer hold an open connection/transaction.

It also adds general improvement to the traceability to the climit
mechanism: it now properly logs the profile-id that is currently
cause some contention on specific resources.

It also add a general/root climit that is applied to all requests
so if someone start making abussive requests, we can clearly detect
it.
This commit is contained in:
Andrey Antukh 2024-01-27 22:33:52 +01:00
parent 658c26014b
commit a5c6d78ee5
12 changed files with 291 additions and 212 deletions
Download patch file Download diff file Expand all files Collapse all files

58
backend/src/app/rpc/commands/auth.clj
View file

@ -21,6 +21,7 @@
[app.loggers.audit :as audit]
[app.main :as-alias main]
[app.rpc :as-alias rpc]
[app.rpc.climit :as-alias climit]
[app.rpc.commands.profile :as profile]
[app.rpc.commands.teams :as teams]
[app.rpc.doc :as-alias doc]
@ -39,7 +40,7 @@
;; ---- COMMAND: login with password
(defn login-with-password
[{:keys [::db/pool] :as cfg} {:keys [email password] :as params}]
[cfg {:keys [email password] :as params}]
(when-not (or (contains? cf/flags :login)
(contains? cf/flags :login-with-password))
@ -47,7 +48,7 @@
:code :login-disabled
:hint "login is disabled in this instance"))
(letfn [(check-password [conn profile password]
(letfn [(check-password [cfg profile password]
(if (= (:password profile) "!")
(ex/raise :type :validation
:code :account-without-password
@ -57,10 +58,10 @@
(l/trc :hint "updating profile password"
:id (str (:id profile))
:email (:email profile))
(profile/update-profile-password! conn (assoc profile :password password)))
(profile/update-profile-password! cfg (assoc profile :password password)))
(:valid result))))
(validate-profile [conn profile]
(validate-profile [cfg profile]
(when-not profile
(ex/raise :type :validation
:code :wrong-credentials))
@ -70,7 +71,7 @@
(when (:is-blocked profile)
(ex/raise :type :restriction
:code :profile-blocked))
(when-not (check-password conn profile password)
(when-not (check-password cfg profile password)
(ex/raise :type :validation
:code :wrong-credentials))
(when-let [deleted-at (:deleted-at profile)]
@ -78,27 +79,29 @@
(ex/raise :type :validation
:code :wrong-credentials)))
profile)]
profile)
(db/with-atomic [conn pool]
(let [profile (->> (profile/get-profile-by-email conn email)
(validate-profile conn)
(profile/strip-private-attrs))
(login [{:keys [::db/conn] :as cfg}]
(let [profile (->> (profile/get-profile-by-email conn email)
(validate-profile cfg)
(profile/strip-private-attrs))
invitation (when-let [token (:invitation-token params)]
(tokens/verify (::main/props cfg) {:token token :iss :team-invitation}))
invitation (when-let [token (:invitation-token params)]
(tokens/verify (::main/props cfg) {:token token :iss :team-invitation}))
;; If invitation member-id does not matches the profile-id, we just proceed to ignore the
;; invitation because invitations matches exactly; and user can't login with other email and
;; accept invitation with other email
response (if (and (some? invitation) (= (:id profile) (:member-id invitation)))
{:invitation-token (:invitation-token params)}
(assoc profile :is-admin (let [admins (cf/get :admins)]
(contains? admins (:email profile)))))]
(-> response
(rph/with-transform (session/create-fn cfg (:id profile)))
(rph/with-meta {::audit/props (audit/profile->props profile)
::audit/profile-id (:id profile)}))))))
;; If invitation member-id does not matches the profile-id, we just proceed to ignore the
;; invitation because invitations matches exactly; and user can't login with other email and
;; accept invitation with other email
response (if (and (some? invitation) (= (:id profile) (:member-id invitation)))
{:invitation-token (:invitation-token params)}
(assoc profile :is-admin (let [admins (cf/get :admins)]
(contains? admins (:email profile)))))]
(-> response
(rph/with-transform (session/create-fn cfg (:id profile)))
(rph/with-meta {::audit/props (audit/profile->props profile)
::audit/profile-id (:id profile)}))))]
(db/tx-run! cfg login)))
(def schema:login-with-password
[:map {:title "login-with-password"}
@ -110,6 +113,7 @@
"Performs authentication using penpot password."
{::rpc/auth false
::doc/added "1.15"
::climit/id :auth/global
::sm/params schema:login-with-password}
[cfg params]
(login-with-password cfg params))
@ -149,7 +153,8 @@
(sv/defmethod ::recover-profile
{::rpc/auth false
::doc/added "1.15"
::sm/params schema:recover-profile}
::sm/params schema:recover-profile
::climit/id :auth/global}
[cfg params]
(recover-profile cfg params))
@ -360,7 +365,6 @@
{::audit/type "fact"
::audit/name "register-profile-retry"
::audit/profile-id id}))
(cond
;; If invitation token comes in params, this is because the
;; user comes from team-invitation process; in this case,
@ -402,7 +406,6 @@
{::audit/replace-props (audit/profile->props profile)
::audit/profile-id (:id profile)})))))
(def schema:register-profile
[:map {:title "register-profile"}
[:token schema:token]
@ -411,7 +414,8 @@
(sv/defmethod ::register-profile
{::rpc/auth false
::doc/added "1.15"
::sm/params schema:register-profile}
::sm/params schema:register-profile
::climit/id :auth/global}
[{:keys [::db/pool] :as cfg} params]
(db/with-atomic [conn pool]
(-> (assoc cfg ::db/conn conn)

14
backend/src/app/rpc/commands/files_thumbnails.clj
View file

@ -285,12 +285,10 @@
(sv/defmethod ::create-file-object-thumbnail
{::doc/added "1.19"
::doc/module :files
::climit/id :file-thumbnail-ops/by-profile
::climit/key-fn ::rpc/profile-id
::climit/id [[:file-thumbnail-ops/by-profile ::rpc/profile-id]
[:file-thumbnail-ops/global]]
::rtry/enabled true
::rtry/when rtry/conflict-exception?
::audit/skip true
::sm/params schema:create-file-object-thumbnail}
@ -332,8 +330,8 @@
{::doc/added "1.19"
::doc/module :files
::doc/deprecated "1.20"
::climit/id :file-thumbnail-ops
::climit/key-fn ::rpc/profile-id
::climit/id [[:file-thumbnail-ops/by-profile ::rpc/profile-id]
[:file-thumbnail-ops/global]]
::audit/skip true}
[cfg {:keys [::rpc/profile-id file-id object-id]}]
(db/tx-run! cfg (fn [{:keys [::db/conn] :as cfg}]
@ -408,8 +406,8 @@
{::doc/added "1.19"
::doc/module :files
::audit/skip true
::climit/id :file-thumbnail-ops
::climit/key-fn ::rpc/profile-id
::climit/id [[:file-thumbnail-ops/by-profile ::rpc/profile-id]
[:file-thumbnail-ops/global]]
::rtry/enabled true
::rtry/when rtry/conflict-exception?
::sm/params schema:create-file-thumbnail}

18
backend/src/app/rpc/commands/files_update.clj
View file

@ -35,7 +35,8 @@
[app.util.services :as sv]
[app.util.time :as dt]
[app.worker :as-alias wrk]
[clojure.set :as set]))
[clojure.set :as set]
[promesa.exec :as px]))
;; --- SCHEMA
@ -132,8 +133,8 @@
;; database.
(sv/defmethod ::update-file
{::climit/id :update-file/by-profile
::climit/key-fn ::rpc/profile-id
{::climit/id [[:update-file/by-profile ::rpc/profile-id]
[:update-file/global]]
::webhooks/event? true
::webhooks/batch-timeout (dt/duration "2m")
::webhooks/batch-key (webhooks/key-fn ::rpc/profile-id :id)
@ -232,13 +233,9 @@
(defn- update-file*
[{:keys [::db/conn ::wrk/executor] :as cfg}
{:keys [profile-id file changes session-id ::created-at skip-validate] :as params}]
(let [;; Process the file data in the CLIMIT context; scheduling it
;; to be executed on a separated executor for avoid to do the
;; CPU intensive operation on vthread.
update-fdata-fn (partial update-file-data cfg file changes skip-validate)
file (-> (climit/configure cfg :update-file/global)
(climit/run! update-fdata-fn executor))]
(let [;; Process the file data on separated thread for avoid to do
;; the CPU intensive operation on vthread.
file (px/invoke! executor (partial update-file-data cfg file changes skip-validate))]
(db/insert! conn :file-change
{:id (uuid/next)
@ -306,7 +303,6 @@
(fmg/migrate-file))
file)
;; WARNING: this ruins performance; maybe we need to find
;; some other way to do general validation
libs (when (and (or (contains? cf/flags :file-validation)

13
backend/src/app/rpc/commands/fonts.clj
View file

@ -16,7 +16,7 @@
[app.loggers.webhooks :as-alias webhooks]
[app.media :as media]
[app.rpc :as-alias rpc]
[app.rpc.climit :as climit]
[app.rpc.climit :as-alias climit]
[app.rpc.commands.files :as files]
[app.rpc.commands.projects :as projects]
[app.rpc.commands.teams :as teams]
@ -26,7 +26,8 @@
[app.storage :as sto]
[app.util.services :as sv]
[app.util.time :as dt]
[app.worker :as-alias wrk]))
[app.worker :as-alias wrk]
[promesa.exec :as px]))
(def valid-weight #{100 200 300 400 500 600 700 800 900 950})
(def valid-style #{"normal" "italic"})
@ -87,6 +88,8 @@
(sv/defmethod ::create-font-variant
{::doc/added "1.18"
::climit/id [[:process-font/by-profile ::rpc/profile-id]
[:process-font/global]]
::webhooks/event? true
::sm/params schema:create-font-variant}
[cfg {:keys [::rpc/profile-id team-id] :as params}]
@ -100,7 +103,7 @@
(create-font-variant cfg (assoc params :profile-id profile-id))))))
(defn create-font-variant
[{:keys [::sto/storage ::db/conn] :as cfg} {:keys [data] :as params}]
[{:keys [::sto/storage ::db/conn ::wrk/executor]} {:keys [data] :as params}]
(letfn [(generate-missing! [data]
(let [data (media/run {:cmd :generate-fonts :input data})]
(when (and (not (contains? data "font/otf"))
@ -152,9 +155,7 @@
:otf-file-id (:id otf)
:ttf-file-id (:id ttf)}))]
(let [data (-> (climit/configure cfg :process-font/global)
(climit/run! (partial generate-missing! data)
(::wrk/executor cfg)))
(let [data (px/invoke! executor (partial generate-missing! data))
assets (persist-fonts-files! data)
result (insert-font-variant! assets)]
(vary-meta result assoc ::audit/replace-props (update params :data (comp vec keys))))))

45
backend/src/app/rpc/commands/media.clj
View file

@ -27,7 +27,8 @@
[app.worker :as-alias wrk]
[clojure.spec.alpha :as s]
[cuerdas.core :as str]
[datoteka.io :as io]))
[datoteka.io :as io]
[promesa.exec :as px]))
(def default-max-file-size
(* 1024 1024 10)) ; 10 MiB
@ -56,20 +57,25 @@
:opt-un [::id]))
(sv/defmethod ::upload-file-media-object
{::doc/added "1.17"}
{::doc/added "1.17"
::climit/id [[:process-image/by-profile ::rpc/profile-id]
[:process-image/global]]}
[{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id file-id content] :as params}]
(let [cfg (update cfg ::sto/storage media/configure-assets-storage)]
(files/check-edition-permissions! pool profile-id file-id)
(media/validate-media-type! content)
(media/validate-media-size! content)
(let [object (db/run! cfg #(create-file-media-object % 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 [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}))))))
(defn- big-enough-for-thumbnail?
"Checks if the provided image info is big enough for
@ -144,12 +150,10 @@
(assoc ::image (process-main-image info)))))
(defn create-file-media-object
[{:keys [::sto/storage ::db/conn ::wrk/executor] :as cfg}
[{:keys [::sto/storage ::db/conn ::wrk/executor]}
{:keys [id file-id is-local name content]}]
(let [result (-> (climit/configure cfg :process-image/global)
(climit/run! (partial process-image content) executor))
(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))]
@ -183,7 +187,7 @@
[{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id file-id] :as params}]
(let [cfg (update cfg ::sto/storage media/configure-assets-storage)]
(files/check-edition-permissions! pool profile-id file-id)
(db/run! cfg #(create-file-media-object-from-url % params))))
(create-file-media-object-from-url cfg (assoc params :profile-id profile-id))))
(defn download-image
[{:keys [::http/client]} uri]
@ -235,7 +239,16 @@
params (-> params
(assoc :content content)
(assoc :name (or name (:filename content))))]
(create-file-media-object cfg params)))
;; NOTE: we use the climit here in a dynamic invocation because we
;; don't want saturate the process-image limit with IO (download
;; of external image)
(-> cfg
(assoc ::climit/id [[:process-image/by-profile (:profile-id params)]
[:process-image/global]])
(assoc ::climit/profile-id (:profile-id params))
(assoc ::climit/label "create-file-media-object-from-url")
(climit/invoke! db/run! cfg create-file-media-object params))))
;; --- Clone File Media object (Upload and create from url)

56
backend/src/app/rpc/commands/profile.clj
View file

@ -28,7 +28,8 @@
[app.util.services :as sv]
[app.util.time :as dt]
[app.worker :as-alias wrk]
[cuerdas.core :as str]))
[cuerdas.core :as str]
[promesa.exec :as px]))
(declare check-profile-existence!)
(declare decode-row)
@ -137,25 +138,24 @@
[:old-password {:optional true} [:maybe [::sm/word-string {:max 500}]]]]))
(sv/defmethod ::update-profile-password
{:doc/added "1.0"
{::doc/added "1.0"
::sm/params schema:update-profile-password
::sm/result :nil}
::climit/id :auth/global}
[cfg {:keys [::rpc/profile-id password] :as params}]
[{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id password] :as params}]
(db/with-atomic [conn pool]
(let [cfg (assoc cfg ::db/conn conn)
profile (validate-password! cfg (assoc params :profile-id profile-id))
session-id (::session/id params)]
(db/tx-run! cfg (fn [cfg]
(let [profile (validate-password! cfg (assoc params :profile-id profile-id))
session-id (::session/id params)]
(when (= (str/lower (:email profile))
(str/lower (:password params)))
(ex/raise :type :validation
:code :email-as-password
:hint "you can't use your email as password"))
(when (= (str/lower (:email profile))
(str/lower (:password params)))
(ex/raise :type :validation
:code :email-as-password
:hint "you can't use your email as password"))
(update-profile-password! conn (assoc profile :password password))
(invalidate-profile-session! cfg profile-id session-id)
nil)))
(update-profile-password! cfg (assoc profile :password password))
(invalidate-profile-session! cfg profile-id session-id)
nil))))
(defn- invalidate-profile-session!
"Removes all sessions except the current one."
@ -173,10 +173,10 @@
profile))
(defn update-profile-password!
[conn {:keys [id password] :as profile}]
[{:keys [::db/conn] :as cfg} {:keys [id password] :as profile}]
(when-not (db/read-only? conn)
(db/update! conn :profile
{:password (auth/derive-password password)}
{:password (derive-password cfg password)}
{:id id})
nil))
@ -203,6 +203,7 @@
(defn update-profile-photo
[{:keys [::db/pool ::sto/storage] :as cfg} {:keys [profile-id file] :as params}]
(let [photo (upload-photo cfg params)
profile (db/get-by-id pool :profile profile-id ::sql/for-update true)]
@ -241,8 +242,11 @@
(defn upload-photo
[{:keys [::sto/storage ::wrk/executor] :as cfg} {:keys [file]}]
(let [params (-> (climit/configure cfg :process-image/global)
(climit/run! (partial generate-thumbnail! file) executor))]
(let [params (-> cfg
(assoc ::climit/id :process-image/global)
(assoc ::climit/label "upload-photo")
(assoc ::climit/executor executor)
(climit/invoke! generate-thumbnail! file))]
(sto/put-object! storage params)))
@ -438,17 +442,13 @@
(into {} (filter (fn [[k _]] (simple-ident? k))) props))
(defn derive-password
[cfg password]
[{:keys [::wrk/executor]} password]
(when password
(-> (climit/configure cfg :derive-password/global)
(climit/run! (partial auth/derive-password password)
(::wrk/executor cfg)))))
(px/invoke! executor (partial auth/derive-password password))))
(defn verify-password
[cfg password password-data]
(-> (climit/configure cfg :derive-password/global)
(climit/run! (partial auth/verify-password password password-data)
(::wrk/executor cfg))))
[{:keys [::wrk/executor]} password password-data]
(px/invoke! executor (partial auth/verify-password password password-data)))
(defn decode-row
[{:keys [props] :as row}]