Merge pull request #616 from penpot/niwinz/bugfixes-1

Bugfixes

CONTRIBUTING.md

@@ -31,8 +31,8 @@ in the changelog.**
 ## Pull requests ##
 
 If you want propose a change or bug fix with the Pull-Request system
-firstly you should carefully read the **Contributor License Aggreement**
-section and format your commits accordingly.
+firstly you should carefully read the **DCO** section and format your
+commits accordingly.
 
 If you intend to fix a bug it's fine to submit a pull request right
 away but we still recommend to file an issue detailing what you're

backend/src/app/config.clj

@@ -57,7 +57,6 @@
    :registration-domain-whitelist ""
 
    :telemetry-enabled false
-   :telemetry-with-taiga true
    :telemetry-uri "https://telemetry.penpot.app/"
 
    ;; LDAP auth disabled by default. Set ldap-auth-host to enable

backend/src/app/db.clj

@@ -72,7 +72,7 @@
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 (def initsql
-  (str "SET statement_timeout = 10000;\n"
+  (str "SET statement_timeout = 60000;\n"
        "SET idle_in_transaction_session_timeout = 120000;"))
 
 (defn- create-datasource-config

backend/src/app/http/auth/google.clj

@@ -35,51 +35,40 @@
 
 (defn- get-access-token
   [cfg code]
-  (let [params {:code code
-                :client_id (:client-id cfg)
-                :client_secret (:client-secret cfg)
-                :redirect_uri (build-redirect-url cfg)
-                :grant_type "authorization_code"}
-        req    {:method :post
-                :headers {"content-type" "application/x-www-form-urlencoded"}
-                :uri "https://oauth2.googleapis.com/token"
-                :body (uri/map->query-string params)}
-        res    (http/send! req)]
+  (try
+    (let [params {:code code
+                  :client_id (:client-id cfg)
+                  :client_secret (:client-secret cfg)
+                  :redirect_uri (build-redirect-url cfg)
+                  :grant_type "authorization_code"}
+          req    {:method :post
+                  :headers {"content-type" "application/x-www-form-urlencoded"}
+                  :uri "https://oauth2.googleapis.com/token"
+                  :body (uri/map->query-string params)}
+          res    (http/send! req)]
 
-    (when (not= 200 (:status res))
-      (ex/raise :type :internal
-                :code :invalid-response-from-google
-                :context {:status (:status res)
-                          :body (:body res)}))
+      (when (= 200 (:status res))
+        (-> (json/read-str (:body res))
+            (get "access_token"))))
 
-    (try
-      (let [data (json/read-str (:body res))]
-        (get data "access_token"))
-      (catch Throwable e
-        (log/error "unexpected error on parsing response body from google access token request" e)
-        nil))))
+    (catch Exception e
+      (log/error e "unexpected error on get-access-token")
+      nil)))
 
 (defn- get-user-info
   [token]
-  (let [req {:uri "https://openidconnect.googleapis.com/v1/userinfo"
-             :headers {"Authorization" (str "Bearer " token)}
-             :method :get}
-        res (http/send! req)]
-
-    (when (not= 200 (:status res))
-      (ex/raise :type :internal
-                :code :invalid-response-from-google
-                :context {:status (:status res)
-                          :body (:body res)}))
-
-    (try
-      (let [data (json/read-str (:body res))]
-        ;; (clojure.pprint/pprint data)
-        {:email (get data "email")
-         :fullname (get data "name")})
-      (catch Throwable e
-        (log/error "unexpected error on parsing response body from google access token request" e)
-        nil))))
+  (try
+    (let [req {:uri "https://openidconnect.googleapis.com/v1/userinfo"
+               :headers {"Authorization" (str "Bearer " token)}
+               :method :get}
+          res (http/send! req)]
+      (when (= 200 (:status res))
+        (let [data (json/read-str (:body res))]
+          {:email (get data "email")
+           :fullname (get data "name")})))
+    (catch Exception e
+      (log/error e "unexpected exception on get-user-info")
+      nil)))
 
 (defn- auth
   [{:keys [tokens] :as cfg} _req]
@@ -99,33 +88,39 @@
 
 (defn- callback
   [{:keys [tokens rpc session] :as cfg} request]
-  (let [token (get-in request [:params :state])
-        _     (tokens :verify {:token token :iss :google-oauth})
-        info  (some->> (get-in request [:params :code])
-                       (get-access-token cfg)
-                       (get-user-info))]
-
-    (when-not info
-      (ex/raise :type :authentication
-                :code :unable-to-authenticate-with-google))
-
-    (let [method-fn (get-in rpc [:methods :mutation :login-or-register])
+  (try
+    (let [token (get-in request [:params :state])
+          _     (tokens :verify {:token token :iss :google-oauth})
+          info  (some->> (get-in request [:params :code])
+                         (get-access-token cfg)
+                         (get-user-info))
+          _     (when-not info
+                  (ex/raise :type :internal
+                            :code :unable-to-auth))
+          method-fn (get-in rpc [:methods :mutation :login-or-register])
           profile   (method-fn {:email (:email info)
                                 :fullname (:fullname info)})
           uagent    (get-in request [:headers "user-agent"])
           token     (tokens :generate {:iss :auth
                                        :exp (dt/in-future "15m")
                                        :profile-id (:id profile)})
-
           uri       (-> (uri/uri (:public-uri cfg))
                         (assoc :path "/#/auth/verify-token")
                         (assoc :query (uri/map->query-string {:token token})))
+
           sid       (session/create! session {:profile-id (:id profile)
                                               :user-agent uagent})]
       {:status 302
        :headers {"location" (str uri)}
        :cookies (session/cookies session {:value sid})
-       :body ""})))
+       :body ""})
+    (catch Exception _e
+      (let [uri (-> (uri/uri (:public-uri cfg))
+                    (assoc :path "/#/auth/login")
+                    (assoc :query (uri/map->query-string {:error "unable-to-auth"})))]
+        {:status 302
+         :headers {"location" (str uri)}
+         :body ""}))))
 
 (s/def ::client-id ::us/not-empty-string)
 (s/def ::client-secret ::us/not-empty-string)

backend/src/app/main.clj

@@ -192,19 +192,19 @@
        :fn (ig/ref :app.tasks.file-media-gc/handler)}
 
       {:id "file-xlog-gc"
-       :cron #app/cron "0 0 */6 * * ?"  ;; every 2 hours
+       :cron #app/cron "0 0 */1 * * ?"  ;; hourly
        :fn (ig/ref :app.tasks.file-xlog-gc/handler)}
 
       {:id "storage-deleted-gc"
-       :cron #app/cron "0 0 */6 * * ?"  ;; every 6 hours
+       :cron #app/cron "0 0 1 */1 * ?"  ;; daily (1 hour shift)
        :fn (ig/ref :app.storage/gc-deleted-task)}
 
       {:id "storage-touched-gc"
-       :cron #app/cron "0 30 */6 * * ?"  ;; every 6 hours
+       :cron #app/cron "0 0 2 */1 * ?"  ;; daily (2 hour shift)
        :fn (ig/ref :app.storage/gc-touched-task)}
 
       {:id "storage-recheck"
-       :cron #app/cron "0 0 */6 * * ?"  ;; every 6 hours
+       :cron #app/cron "0 0 */1 * * ?"  ;; hourly
        :fn (ig/ref :app.storage/recheck-task)}
 
       {:id "tasks-gc"
@@ -260,7 +260,7 @@
     :app.tasks.file-xlog-gc/handler
     {:pool    (ig/ref :app.db/pool)
      :metrics (ig/ref :app.metrics/metrics)
-     :max-age (dt/duration {:hours 24})}
+     :max-age (dt/duration {:hours 48})}
 
     :app.tasks.telemetry/handler
     {:pool        (ig/ref :app.db/pool)

backend/src/app/media.clj

@@ -175,7 +175,12 @@
       (ex/raise :type :internal
                 :code :rlimit-not-configured
                 :hint ":image rlimit not configured"))
-    (rlm/execute rlimit (process params))))
+    (try
+      (rlm/execute rlimit (process params))
+      (catch org.im4java.core.InfoException e
+        (ex/raise :type :validation
+                  :code :invalid-image
+                  :cause e)))))
 
 ;; --- Utility functions
 

backend/src/app/migrations.clj

@@ -145,6 +145,9 @@
 
    {:name "0044-add-storage-refcount"
     :fn (mg/resource "app/migrations/sql/0044-add-storage-refcount.sql")}
+
+   {:name "0045-add-index-to-file-change-table"
+    :fn (mg/resource "app/migrations/sql/0045-add-index-to-file-change-table.sql")}
    ])
 
 

backend/src/app/migrations/sql/0045-add-index-to-file-change-table.sql

@@ -0,0 +1,2 @@
+CREATE INDEX file_change__created_at_idx
+    ON file_change (created_at);

backend/src/app/rpc/mutations/media.clj

@@ -38,7 +38,7 @@
 ;; --- Create File Media object (upload)
 
 (declare create-file-media-object)
-(declare select-file-for-update)
+(declare select-file)
 
 (s/def ::content ::media/upload)
 (s/def ::is-local ::us/boolean)
@@ -50,7 +50,7 @@
 (sv/defmethod ::upload-file-media-object
   [{:keys [pool] :as cfg} {:keys [profile-id file-id] :as params}]
   (db/with-atomic [conn pool]
-    (let [file (select-file-for-update conn file-id)]
+    (let [file (select-file conn file-id)]
       (teams/check-edition-permissions! conn profile-id (:team-id file))
       (-> (assoc cfg :conn conn)
           (create-file-media-object params)))))
@@ -66,9 +66,18 @@
   [info]
   (= (:mtype info) "image/svg+xml"))
 
+(defn- fetch-url
+  [url]
+  (try
+    (http/get! url {:as :byte-array})
+    (catch Exception e
+      (ex/raise :type :validation
+                :code :unable-to-access-to-url
+                :cause e))))
+
 (defn- download-media
   [{:keys [storage] :as cfg} url]
-  (let [result (http/get! url {:as :byte-array})
+  (let [result (fetch-url url)
         data   (:body result)
         mtype  (get (:headers result) "content-type")
         format (cm/mtype->format mtype)]
@@ -129,7 +138,7 @@
 (sv/defmethod ::create-file-media-object-from-url
   [{:keys [pool storage] :as cfg} {:keys [profile-id file-id url name] :as params}]
   (db/with-atomic [conn pool]
-    (let [file (select-file-for-update conn file-id)]
+    (let [file (select-file conn file-id)]
       (teams/check-edition-permissions! conn profile-id (:team-id file))
       (let [mobj    (download-media cfg url)
             content {:filename "tempfile"
@@ -152,7 +161,7 @@
 (sv/defmethod ::clone-file-media-object
   [{:keys [pool] :as cfg} {:keys [profile-id file-id] :as params}]
   (db/with-atomic [conn pool]
-    (let [file (select-file-for-update conn file-id)]
+    (let [file (select-file conn file-id)]
       (teams/check-edition-permissions! conn profile-id (:team-id file))
 
       (-> (assoc cfg :conn conn)
@@ -175,17 +184,17 @@
 
 ;; --- HELPERS
 
-(def ^:private sql:select-file-for-update
+(def ^:private
+  sql:select-file
   "select file.*,
           project.team_id as team_id
      from file
     inner join project on (project.id = file.project_id)
-    where file.id = ?
-      for update of file")
+    where file.id = ?")
 
-(defn- select-file-for-update
+(defn- select-file
   [conn id]
-  (let [row (db/exec-one! conn [sql:select-file-for-update id])]
+  (let [row (db/exec-one! conn [sql:select-file id])]
     (when-not row
       (ex/raise :type :not-found))
     row))

backend/src/app/srepl/main.clj

@@ -72,3 +72,17 @@
      (let [profile (prof/retrieve-profile-data-by-email conn user-email)
            profile (merge profile (prof/retrieve-additional-data conn (:id profile)))]
        (pid/create-profile-initial-data conn file profile)))))
+
+
+;; Migrate
+
+(defn update-file-data-blob-format
+  [system]
+  (db/with-atomic [conn (:app.db/pool system)]
+    (doseq [id (->> (db/exec! conn ["select id from file;"]) (map :id))]
+      (let [{:keys [data]} (db/get-by-id conn :file id {:columns [:id :data]})]
+        (prn "Updating file:" id)
+        (db/update! conn :file
+                    {:data (-> (blob/decode data)
+                               (blob/encode {:version 2}))}
+                    {:id id})))))

backend/src/app/svgparse.clj

@@ -121,11 +121,16 @@
 
 (defn parse
   [data]
-  (with-open [istream (IOUtils/toInputStream data "UTF-8")]
-    (xml/parse istream)))
+  (try
+    (with-open [istream (IOUtils/toInputStream data "UTF-8")]
+      (xml/parse istream))
+    (catch org.xml.sax.SAXParseException _e
+      (ex/raise :type :validation
+                :code :invalid-svg-file))))
 
 (defn process-request
   [{:keys [svgc] :as cfg} body]
   (let [data (slurp body)
         data (svgc data)]
     (parse data)))
+

backend/src/app/tasks/telemetry.clj

@@ -63,6 +63,7 @@
                                  :uri (:uri cfg)
                                  :headers {"content-type" "application/json"}
                                  :body (json/encode-str data)})]
+
     (when (not= 200 (:status response))
       (ex/raise :type :internal
                 :code :invalid-response-from-google
@@ -129,7 +130,7 @@
   [{:keys [conn version]}]
   (merge
    {:version version
-    :with-taiga  (:telemetry-with-taiga cfg/config)
+    :with-taiga  (:telemetry-with-taiga cfg/config false)
     :total-teams (retrieve-num-teams conn)
     :total-projects (retrieve-num-projects conn)
     :total-files (retrieve-num-files conn)}

backend/src/app/telemetry.clj

@@ -12,6 +12,7 @@
    [app.common.spec :as us]
    [app.db :as db]
    [app.http.middleware :refer [wrap-parse-request-body]]
+   [clojure.pprint :refer [pprint]]
    [clojure.spec.alpha :as s]
    [clojure.tools.logging :as log]
    [integrant.core :as ig]
@@ -87,7 +88,12 @@
     (catch Exception e
       ;; We don't want notify user of a error, just log it for posible
       ;; future investigation.
-      (log/warnf e "Unexpected error on telemetry.")))
+      (log/warn e (str "Unexpected error on telemetry:\n"
+                       (when-let [edata (ex-data e)]
+                         (str "ex-data: \n"
+                              (with-out-str (pprint edata))))
+                       (str "params: \n"
+                            (with-out-str (pprint params)))))))
   {:status 200
    :body "OK\n"})
 

frontend/resources/locales.json

@@ -830,6 +830,12 @@
       "es" : "Actualizado: %s"
     }
   },
+  "errors.google-auth-not-enabled" : {
+    "translations" : {
+      "en" : "Authentication with google disabled on backend",
+      "es" : "Autenticación con google esta dehabilitada en el servidor"
+    }
+  },
   "errors.auth.unauthorized" : {
     "used-in" : [ "src/app/main/ui/auth/login.cljs:89" ],
     "translations" : {

frontend/src/app/main/data/viewer.cljs

@@ -137,7 +137,8 @@
                  (d/index-by :id)
                  (assoc state :comment-threads)))
           (on-error [{:keys [type] :as err}]
-            (if (= :authentication type)
+            (if (or (= :authentication type)
+                    (= :not-found type))
               (rx/empty)
               (rx/throw err)))]
 

frontend/src/app/main/data/workspace/persistence.cljs

@@ -417,13 +417,22 @@
 (defn- handle-upload-error [on-error stream]
   (->> stream
        (rx/catch
-           (fn on-error [error]
+           (fn on-error* [error]
              (if (ex/ex-info? error)
-               (on-error (ex-data error))
+               (on-error* (ex-data error))
                (cond
+                 (= (:code error) :invalid-svg-file)
+                 (rx/of (dm/error (tr "errors.media-type-not-allowed")))
+
                  (= (:code error) :media-type-not-allowed)
                  (rx/of (dm/error (tr "errors.media-type-not-allowed")))
 
+                 (= (:code error) :ubable-to-access-to-url)
+                 (rx/of (dm/error (tr "errors.media-type-not-allowed")))
+
+                 (= (:code error) :invalid-image)
+                 (rx/of (dm/error (tr "errors.media-type-not-allowed")))
+
                  (= (:code error) :media-too-large)
                  (rx/of (dm/error (tr "errors.media-too-large")))
 

frontend/src/app/main/ui/auth/login.cljs

@@ -37,7 +37,9 @@
   (dom/prevent-default event)
   (->> (rp/mutation! :login-with-google {})
        (rx/subs (fn [{:keys [redirect-uri] :as rsp}]
-                  (.replace js/location redirect-uri)))))
+                  (.replace js/location redirect-uri))
+                (fn [{:keys [type] :as error}]
+                  (st/emit! (dm/error (tr "errors.google-auth-not-enabled")))))))
 
 (defn- login-with-gitlab
   [event]