From e9945235edb19fbea729843f305ae22784525acc Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 6 Jul 2021 15:40:23 +0200 Subject: [PATCH] :sparkles: Improvements on auth and login. --- backend/src/app/http/oauth.clj | 118 +++++++++++----------- backend/src/app/loggers/audit.clj | 25 ++++- backend/src/app/main.clj | 5 +- backend/src/app/rpc.clj | 19 ++-- backend/src/app/rpc/mutations/profile.clj | 23 +++-- 5 files changed, 105 insertions(+), 85 deletions(-) diff --git a/backend/src/app/http/oauth.clj b/backend/src/app/http/oauth.clj index 72a2d11412..528d90427f 100644 --- a/backend/src/app/http/oauth.clj +++ b/backend/src/app/http/oauth.clj @@ -12,6 +12,7 @@ [app.common.uri :as u] [app.config :as cf] [app.db :as db] + [app.loggers.audit :as audit] [app.rpc.queries.profile :as profile] [app.util.http :as http] [app.util.logging :as l] @@ -22,55 +23,6 @@ [cuerdas.core :as str] [integrant.core :as ig])) -(defn redirect-response - [uri] - {:status 302 - :headers {"location" (str uri)} - :body ""}) - -(defn generate-error-redirect - [cfg error] - (let [uri (-> (u/uri (:public-uri cfg)) - (assoc :path "/#/auth/login") - (assoc :query (u/map->query-string {:error "unable-to-auth" :hint (ex-message error)})))] - (redirect-response uri))) - -(defn register-profile - [{:keys [rpc] :as cfg} info] - (let [method-fn (get-in rpc [:methods :mutation :login-or-register]) - profile (method-fn info)] - (cond-> profile - (some? (:invitation-token info)) - (assoc :invitation-token (:invitation-token info))))) - -(defn generate-redirect - [{:keys [tokens session] :as cfg} request info profile] - (if profile - (let [sxf ((:create session) (:id profile)) - token (or (:invitation-token info) - (tokens :generate {:iss :auth - :exp (dt/in-future "15m") - :profile-id (:id profile)})) - params {:token token} - - uri (-> (u/uri (:public-uri cfg)) - (assoc :path "/#/auth/verify-token") - (assoc :query (u/map->query-string params)))] - (->> (redirect-response uri) - (sxf request))) - (let [info (assoc info - :iss :prepared-register - :exp (dt/in-future {:hours 48})) - token (tokens :generate info) - params (d/without-nils - {:token token - :fullname (:fullname info)}) - uri (-> (u/uri (:public-uri cfg)) - (assoc :path "/#/auth/register/validate") - (assoc :query (u/map->query-string params)))] - (redirect-response uri)))) - - (defn- build-redirect-uri [{:keys [provider] :as cfg}] (let [public (u/uri (:public-uri cfg))] @@ -198,6 +150,63 @@ {} params)) +(defn- retrieve-profile + [{:keys [pool] :as cfg} info] + (with-open [conn (db/open pool)] + (some->> (:email info) + (profile/retrieve-profile-data-by-email conn) + (profile/populate-additional-data conn) + (profile/decode-profile-row)))) + +(defn- redirect-response + [uri] + {:status 302 + :headers {"location" (str uri)} + :body ""}) + +(defn- generate-error-redirect + [cfg error] + (let [uri (-> (u/uri (:public-uri cfg)) + (assoc :path "/#/auth/login") + (assoc :query (u/map->query-string {:error "unable-to-auth" :hint (ex-message error)})))] + (redirect-response uri))) + +(defn- generate-redirect + [{:keys [tokens session audit] :as cfg} request info profile] + (if profile + (let [sxf ((:create session) (:id profile)) + token (or (:invitation-token info) + (tokens :generate {:iss :auth + :exp (dt/in-future "15m") + :profile-id (:id profile)})) + params {:token token} + + uri (-> (u/uri (:public-uri cfg)) + (assoc :path "/#/auth/verify-token") + (assoc :query (u/map->query-string params)))] + + (when (fn? audit) + (audit :cmd :submit + :type "mutation" + :name "login" + :profile-id (:id profile) + :ip-addr (audit/parse-client-ip request) + :props (audit/profile->props profile))) + + (->> (redirect-response uri) + (sxf request))) + (let [info (assoc info + :iss :prepared-register + :exp (dt/in-future {:hours 48})) + token (tokens :generate info) + params (d/without-nils + {:token token + :fullname (:fullname info)}) + uri (-> (u/uri (:public-uri cfg)) + (assoc :path "/#/auth/register/validate") + (assoc :query (u/map->query-string params)))] + (redirect-response uri)))) + (defn- auth-handler [{:keys [tokens] :as cfg} {:keys [params] :as request}] (let [invitation (:invitation-token params) @@ -211,13 +220,6 @@ {:status 200 :body {:redirect-uri uri}})) -(defn- retrieve-profile - [{:keys [pool] :as cfg} info] - (with-open [conn (db/open pool)] - (some->> (:email info) - (profile/retrieve-profile-data-by-email conn) - (profile/populate-additional-data conn)))) - (defn- callback-handler [cfg request] (try @@ -238,7 +240,7 @@ (s/def ::tokens fn?) (s/def ::rpc map?) -(defmethod ig/pre-init-spec :app.http.oauth/handlers [_] +(defmethod ig/pre-init-spec ::handler [_] (s/keys :req-un [::public-uri ::session ::tokens ::rpc ::db/pool])) (defn wrap-handler @@ -253,7 +255,7 @@ (-> (assoc @cfg :provider provider) (handler request))))) -(defmethod ig/init-key :app.http.oauth/handlers +(defmethod ig/init-key ::handler [_ cfg] (let [cfg (initialize cfg)] {:handler (wrap-handler cfg auth-handler) diff --git a/backend/src/app/loggers/audit.clj b/backend/src/app/loggers/audit.clj index 835a8acff9..dbdff89208 100644 --- a/backend/src/app/loggers/audit.clj +++ b/backend/src/app/loggers/audit.clj @@ -7,6 +7,7 @@ (ns app.loggers.audit "Services related to the user activity (audit log)." (:require + [app.common.data :as d] [app.common.exceptions :as ex] [app.common.spec :as us] [app.common.transit :as t] @@ -20,9 +21,22 @@ [app.worker :as wrk] [clojure.core.async :as a] [clojure.spec.alpha :as s] + [cuerdas.core :as str] [integrant.core :as ig] [lambdaisland.uri :as u])) +(defn parse-client-ip + [{:keys [headers] :as request}] + (or (some-> (get headers "x-forwarded-for") (str/split ",") first) + (get headers "x-real-ip") + (get request :remote-addr))) + +(defn profile->props + [profile] + (-> profile + (select-keys [:is-active :is-muted :auth-backend :email :default-team-id :default-project-id :fullname :lang]) + (d/without-nils))) + (defn clean-props [{:keys [profile-id] :as event}] (letfn [(clean-common [props] @@ -88,11 +102,12 @@ :cause res))) (recur))) - (fn [& [cmd & params]] - (case cmd - :stop (a/close! input) - :submit (when-not (a/offer! input (first params)) - (l/warn :msg "activity channel is full"))))))) + (fn [& {:keys [cmd] :as params}] + (let [params (dissoc params :cmd)] + (case cmd + :stop (a/close! input) + :submit (when-not (a/offer! input params) + (l/warn :msg "activity channel is full")))))))) (defn- persist-events diff --git a/backend/src/app/main.clj b/backend/src/app/main.clj index da576cf624..dc43559107 100644 --- a/backend/src/app/main.clj +++ b/backend/src/app/main.clj @@ -90,7 +90,7 @@ :tokens (ig/ref :app.tokens/tokens) :public-uri (cf/get :public-uri) :metrics (ig/ref :app.metrics/metrics) - :oauth (ig/ref :app.http.oauth/handlers) + :oauth (ig/ref :app.http.oauth/handler) :assets (ig/ref :app.http.assets/handlers) :storage (ig/ref :app.storage/storage) :sns-webhook (ig/ref :app.http.awsns/handler) @@ -107,11 +107,12 @@ :app.http.feedback/handler {:pool (ig/ref :app.db/pool)} - :app.http.oauth/handlers + :app.http.oauth/handler {:rpc (ig/ref :app.rpc/rpc) :session (ig/ref :app.http.session/session) :pool (ig/ref :app.db/pool) :tokens (ig/ref :app.tokens/tokens) + :audit (ig/ref :app.loggers.audit/collector) :public-uri (cf/get :public-uri)} ;; RLimit definition for password hashing diff --git a/backend/src/app/rpc.clj b/backend/src/app/rpc.clj index 947936940f..09a2a636a9 100644 --- a/backend/src/app/rpc.clj +++ b/backend/src/app/rpc.clj @@ -89,12 +89,6 @@ (rlm/execute rlinst (f cfg params)))) f)) -(defn- parse-client-ip - [{:keys [headers] :as request}] - (or (some-> (get headers "x-forwarded-for") (str/split ",") first) - (get headers "x-real-ip") - (get request :remote-addr))) - (defn- wrap-impl [{:keys [audit] :as cfg} f mdata] (let [f (wrap-with-rlimits cfg f mdata) @@ -124,12 +118,13 @@ (:profile-id result) (::audit/profile-id resultm)) props (d/merge params (::audit/props resultm))] - (audit :submit {:type (::type cfg) - :name (or (::audit/name resultm) - (::sv/name mdata)) - :profile-id profile-id - :ip-addr (parse-client-ip request) - :props props}))) + (audit :cmd :submit + :type (::type cfg) + :name (or (::audit/name resultm) + (::sv/name mdata)) + :profile-id profile-id + :ip-addr (audit/parse-client-ip request) + :props (audit/profile->props props)))) result)))) diff --git a/backend/src/app/rpc/mutations/profile.clj b/backend/src/app/rpc/mutations/profile.clj index b40cf802a6..17c52869a7 100644 --- a/backend/src/app/rpc/mutations/profile.clj +++ b/backend/src/app/rpc/mutations/profile.clj @@ -85,6 +85,11 @@ {:update false :valid false}))) +(defn decode-profile-row + [{:keys [props] :as profile}] + (cond-> profile + (db/pgobject? props "jsonb") + (assoc :props (db/decode-transit-pgobject props)))) ;; --- MUTATION: Prepare Register @@ -154,8 +159,8 @@ (check-profile-existence! conn params) (let [profile (->> params (create-profile conn) - (create-profile-relations conn))] - + (create-profile-relations conn) + (decode-profile-row))] (sid/load-initial-project! conn profile) (cond @@ -174,7 +179,7 @@ (with-meta resp {:transform-response ((:create session) (:id profile)) :before-complete (annotate-profile-register metrics) - ::audit/props (:props profile) + ::audit/props (audit/profile->props profile) ::audit/profile-id (:id profile)})) ;; If auth backend is different from "penpot" means user is @@ -184,7 +189,7 @@ (with-meta (profile/strip-private-attrs profile) {:transform-response ((:create session) (:id profile)) :before-complete (annotate-profile-register metrics) - ::audit/props (:props profile) + ::audit/props (audit/profile->props profile) ::audit/profile-id (:id profile)}) ;; In all other cases, send a verification email. @@ -208,7 +213,7 @@ (with-meta profile {:before-complete (annotate-profile-register metrics) - ::audit/props (:props profile) + ::audit/props (audit/profile->props profile) ::audit/profile-id (:id profile)})))))) (defn create-profile @@ -249,7 +254,7 @@ :is-demo is-demo}] (try (-> (db/insert! conn :profile params) - (update :props db/decode-transit-pgobject)) + (decode-profile-row)) (catch org.postgresql.util.PSQLException e (let [state (.getSQLState e)] (if (not= state "23505") @@ -314,8 +319,8 @@ (let [profile (->> (profile/retrieve-profile-data-by-email conn email) (validate-profile) (profile/strip-private-attrs) - (profile/populate-additional-data conn)) - profile (update profile :props db/decode-transit-pgobject)] + (profile/populate-additional-data conn) + (decode-profile-row))] (if-let [token (:invitation-token params)] ;; If the request comes with an invitation token, this means ;; that user wants to accept it with different user. A very @@ -330,10 +335,12 @@ token (tokens :generate claims)] (with-meta {:invitation-token token} {:transform-response ((:create session) (:id profile)) + ::audit/props (audit/profile->props profile) ::audit/profile-id (:id profile)})) (with-meta profile {:transform-response ((:create session) (:id profile)) + ::audit/props (audit/profile->props profile) ::audit/profile-id (:id profile)})))))) ;; --- MUTATION: Logout