From 96facc510064acb821a2ff2ac380b4fcaef6859f Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 15 Feb 2022 15:33:59 +0100 Subject: [PATCH] :recycle: Refactor invitation flow Enfoces security and make the flow more deterministic. --- backend/src/app/loggers/audit.clj | 101 +++++++++--------- backend/src/app/rpc.clj | 1 - backend/src/app/rpc/mutations/profile.clj | 75 ++++++------- backend/src/app/rpc/mutations/teams.clj | 13 ++- .../src/app/rpc/mutations/verify_token.clj | 68 +++--------- backend/src/app/tokens.clj | 3 +- .../src/app/main/ui/auth/verify_token.cljs | 12 ++- 7 files changed, 117 insertions(+), 156 deletions(-) diff --git a/backend/src/app/loggers/audit.clj b/backend/src/app/loggers/audit.clj index 7432aa9f7..1c113bb14 100644 --- a/backend/src/app/loggers/audit.clj +++ b/backend/src/app/loggers/audit.clj @@ -41,33 +41,26 @@ (defn clean-props [{:keys [profile-id] :as event}] - (letfn [(clean-common [props] - (-> props - (dissoc :session-id) - (dissoc :password) - (dissoc :old-password) - (dissoc :token))) + (let [invalid-keys #{:session-id + :password + :old-password + :token} + xform (comp + (remove (fn [kv] + (qualified-keyword? (first kv)))) + (remove (fn [kv] + (contains? invalid-keys (first kv)))) + (remove (fn [[k v]] + (and (= k :profile-id) + (= v profile-id)))) + (filter (fn [[_ v]] + (or (string? v) + (keyword? v) + (uuid? v) + (boolean? v) + (number? v)))))] - (clean-profile-id [props] - (cond-> props - (= profile-id (:profile-id props)) - (dissoc :profile-id))) - - (clean-complex-data [props] - (reduce-kv (fn [props k v] - (cond-> props - (or (string? v) - (uuid? v) - (boolean? v) - (number? v)) - (assoc k v) - - (keyword? v) - (assoc k (name v)))) - {} - props))] - - (update event :props #(-> % clean-common clean-profile-id clean-complex-data)))) + (update event :props #(into {} xform %)))) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; HTTP Handler @@ -82,11 +75,11 @@ (s/def ::timestamp dt/instant?) (s/def ::context (s/map-of ::us/keyword any?)) -(s/def ::event +(s/def ::frontend-event (s/keys :req-un [::type ::name ::props ::timestamp ::profile-id] :opt-un [::context])) -(s/def ::events (s/every ::event)) +(s/def ::frontend-events (s/every ::event)) (defmethod ig/init-key ::http-handler [_ {:keys [executor pool] :as cfg}] @@ -98,7 +91,7 @@ (when (contains? cf/flags :audit-log) (let [events (->> (:events params) (remove #(not= profile-id (:profile-id %))) - (us/conform ::events)) + (us/conform ::frontend-events)) ip-addr (parse-client-ip request) cfg (-> cfg (assoc :source "frontend") @@ -147,9 +140,14 @@ (defmethod ig/pre-init-spec ::collector [_] (s/keys :req-un [::db/pool ::wrk/executor])) -(def event-xform +(s/def ::ip-addr string?) +(s/def ::backend-event + (s/keys :req-un [::type ::name ::profile-id] + :opt-un [::ip-addr ::props])) + +(def ^:private backend-event-xform (comp - (filter :profile-id) + (filter #(us/valid? ::backend-event %)) (map clean-props))) (defmethod ig/init-key ::collector @@ -166,42 +164,41 @@ (constantly nil)) :else - (let [input (a/chan 512 event-xform) + (let [input (a/chan 512 backend-event-xform) buffer (aa/batch input {:max-batch-size 100 :max-batch-age (* 10 1000) ; 10s :init []})] - (l/info :hint "audit log collector initialized") (a/go-loop [] (when-let [[_type events] (a/ params - (dissoc :cmd) - (assoc :tracked-at (dt/now)))] - (case cmd - :stop (a/close! input) - :submit (when-not (a/offer! input params) - (l/warn :msg "activity channel is full")))))))) + (case cmd + :stop + (a/close! input) + :submit + (let [params (-> params + (dissoc :cmd) + (assoc :tracked-at (dt/now)))] + (when-not (a/offer! input params) + (l/warn :hint "activity channel is full")))))))) (defn- persist-events [{:keys [pool executor] :as cfg} events] (letfn [(event->row [event] - (when (:profile-id event) - [(uuid/next) - (:name event) - (:type event) - (:profile-id event) - (:tracked-at event) - (some-> (:ip-addr event) db/inet) - (db/tjson (:props event)) - "backend"]))] + [(uuid/next) + (:name event) + (:type event) + (:profile-id event) + (:tracked-at event) + (some-> (:ip-addr event) db/inet) + (db/tjson (:props event)) + "backend"])] (aa/with-thread executor (when (seq events) (db/with-atomic [conn pool] diff --git a/backend/src/app/rpc.clj b/backend/src/app/rpc.clj index 0f390bbcb..ff346aece 100644 --- a/backend/src/app/rpc.clj +++ b/backend/src/app/rpc.clj @@ -113,7 +113,6 @@ :profile-id profile-id :ip-addr (audit/parse-client-ip request) :props props))) - result)) mdata))) diff --git a/backend/src/app/rpc/mutations/profile.clj b/backend/src/app/rpc/mutations/profile.clj index d100f0ff8..94732858f 100644 --- a/backend/src/app/rpc/mutations/profile.clj +++ b/backend/src/app/rpc/mutations/profile.clj @@ -157,23 +157,22 @@ (check-profile-existence! conn params) - (let [is-active (or (:is-active params) - (contains? cf/flags :insecure-register)) - profile (->> (assoc params :is-active is-active) - (create-profile conn) - (create-profile-relations conn) - (decode-profile-row))] + (let [is-active (or (:is-active params) + (contains? cf/flags :insecure-register)) + profile (->> (assoc params :is-active is-active) + (create-profile conn) + (create-profile-relations conn) + (decode-profile-row)) + + invitation (when-let [token (:invitation-token params)] + (tokens :verify {:token token :iss :team-invitation}))] + (cond - ;; If invitation token comes in params, this is because the - ;; user comes from team-invitation process; in this case, - ;; regenerate token and send back to the user a new invitation - ;; token (and mark current session as logged). - (some? (:invitation-token params)) - (let [token (:invitation-token params) - claims (tokens :verify {:token token :iss :team-invitation}) - claims (assoc claims - :member-id (:id profile) - :member-email (:email profile)) + ;; If invitation token comes in params, this is because the user comes from team-invitation process; + ;; in this case, regenerate token and send back to the user a new invitation token (and mark current + ;; session as logged). This happens only if the invitation email matches with the register email. + (and (some? invitation) (= (:email profile) (:member-email invitation))) + (let [claims (assoc invitation :member-id (:id profile)) token (tokens :generate claims) resp {:invitation-token token}] (with-meta resp @@ -311,32 +310,26 @@ profile)] (db/with-atomic [conn pool] - (let [profile (->> (profile/retrieve-profile-data-by-email conn email) - (validate-profile) - (profile/strip-private-attrs) - (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 - ;; strange case but still can happen. In this case, we - ;; proceed in the same way as in register: regenerate the - ;; invitation token and return it to the user for proper - ;; invitation acceptation. - (let [claims (tokens :verify {:token token :iss :team-invitation}) - claims (assoc claims - :member-id (:id profile) - :member-email (:email profile)) - 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)})) + (let [profile (->> (profile/retrieve-profile-data-by-email conn email) + (validate-profile) + (profile/strip-private-attrs) + (profile/populate-additional-data conn) + (decode-profile-row)) - (with-meta profile - {:transform-response ((:create session) (:id profile)) - ::audit/props (audit/profile->props profile) - ::audit/profile-id (:id profile)})))))) + invitation (when-let [token (:invitation-token params)] + (tokens :verify {: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 loging with other email and + ;; accept invitation with other email + response (if (and (some? invitation) (= (:id profile) (:member-id invitation))) + {:invitation-token (:invitation-token params)} + profile)] + + (with-meta response + {:transform-response ((:create session) (:id profile)) + ::audit/props (audit/profile->props profile) + ::audit/profile-id (:id profile)}))))) ;; --- MUTATION: Logout diff --git a/backend/src/app/rpc/mutations/teams.clj b/backend/src/app/rpc/mutations/teams.clj index e6cc7288b..c54ae7e61 100644 --- a/backend/src/app/rpc/mutations/teams.clj +++ b/backend/src/app/rpc/mutations/teams.clj @@ -379,8 +379,7 @@ :code :member-is-muted :hint "looks like the profile has reported repeatedly as spam or has permanent bounces")) - ;; Secondly check if the invited member email is part of the - ;; global spam/bounce report. + ;; Secondly check if the invited member email is part of the global spam/bounce report. (when (eml/has-bounce-reports? conn email) (ex/raise :type :validation :code :email-has-permanent-bounces @@ -403,13 +402,21 @@ (s/and ::create-team (s/keys :req-un [::emails ::role]))) (sv/defmethod ::create-team-and-invite-members - [{:keys [pool] :as cfg} {:keys [profile-id emails role] :as params}] + [{:keys [pool audit] :as cfg} {:keys [profile-id emails role] :as params}] (db/with-atomic [conn pool] (let [team (create-team conn params) profile (db/get-by-id conn :profile profile-id)] ;; Create invitations for all provided emails. (doseq [email emails] + (audit :cmd :submit + :type "mutation" + :name "create-team-invitation" + :profile-id profile-id + :props {:email email + :role role + :profile-id profile-id}) + (create-team-invitation (assoc cfg :conn conn diff --git a/backend/src/app/rpc/mutations/verify_token.clj b/backend/src/app/rpc/mutations/verify_token.clj index 106580316..0b1eb87d6 100644 --- a/backend/src/app/rpc/mutations/verify_token.clj +++ b/backend/src/app/rpc/mutations/verify_token.clj @@ -118,77 +118,39 @@ (assoc member :is-active true))) (defmethod process-token :team-invitation - [{:keys [session] :as cfg} {:keys [profile-id token]} {:keys [member-id] :as claims}] + [cfg {:keys [profile-id token]} {:keys [member-id] :as claims}] (us/assert ::team-invitation-claims claims) (cond ;; This happens when token is filled with member-id and current - ;; user is already logged in with some account. - (and (uuid? profile-id) - (uuid? member-id)) + ;; user is already logged in with exactly invited account. + (and (uuid? profile-id) (uuid? member-id) (= member-id profile-id)) (let [profile (accept-invitation cfg claims)] - (if (= member-id profile-id) - ;; If the current session is already matches the invited - ;; member, then just return the token and leave the frontend - ;; app redirect to correct team. - (assoc claims :state :created) - - ;; If the session does not matches the invited member, replace - ;; the session with a new one matching the invited member. - ;; This technique should be considered secure because the - ;; user clicking the link he already has access to the email - ;; account. - (with-meta - (assoc claims :state :created) - {:transform-response ((:create session) member-id) - ::audit/name "accept-team-invitation" - ::audit/props (merge - (audit/profile->props profile) - {:team-id (:team-id claims) - :role (:role claims)}) - ::audit/profile-id profile-id}))) - - ;; This happens when member-id is not filled in the invitation but - ;; the user already has an account (probably with other mail) and - ;; is already logged-in. - (and (uuid? profile-id) - (nil? member-id)) - (let [profile (accept-invitation cfg (assoc claims :member-id profile-id))] (with-meta (assoc claims :state :created) {::audit/name "accept-team-invitation" - ::audit/props (merge - (audit/profile->props profile) - {:team-id (:team-id claims) - :role (:role claims)}) - ::audit/profile-id profile-id})) - - ;; This happens when member-id is filled but the accessing user is - ;; not logged-in. In this case we proceed to accept invitation and - ;; leave the user logged-in. - (and (nil? profile-id) - (uuid? member-id)) - (let [profile (accept-invitation cfg claims)] - (with-meta - (assoc claims :state :created) - {:transform-response ((:create session) member-id) - ::audit/name "accept-team-invitation" ::audit/props (merge (audit/profile->props profile) {:team-id (:team-id claims) :role (:role claims)}) ::audit/profile-id member-id})) - ;; In this case, we wait until frontend app redirect user to - ;; registration page, the user is correctly registered and the - ;; register mutation call us again with the same token to finally - ;; create the corresponding team-profile relation from the first - ;; condition of this if. + ;; This case means that invitation token does not match with + ;; registred user, so we need to indicate to frontend to redirect + ;; it to register page. + (nil? member-id) + {:invitation-token token + :iss :team-invitation + :redirect-to :auth-register + :state :pending} + + ;; In all other cases, just tell to fontend to redirect the user + ;; to the login page. :else {:invitation-token token :iss :team-invitation + :redirect-to :auth-login :state :pending})) - ;; --- Default (defmethod process-token :default diff --git a/backend/src/app/tokens.clj b/backend/src/app/tokens.clj index efff646d1..532055c90 100644 --- a/backend/src/app/tokens.clj +++ b/backend/src/app/tokens.clj @@ -7,6 +7,7 @@ (ns app.tokens "Tokens generation service." (:require + [app.common.data :as d] [app.common.exceptions :as ex] [app.common.spec :as us] [app.common.transit :as t] @@ -17,7 +18,7 @@ (defn- generate [cfg claims] - (let [payload (t/encode claims)] + (let [payload (-> claims d/without-nils t/encode)] (jwe/encrypt payload (::secret cfg) {:alg :a256kw :enc :a256gcm}))) (defn- verify diff --git a/frontend/src/app/main/ui/auth/verify_token.cljs b/frontend/src/app/main/ui/auth/verify_token.cljs index 119a4cd40..1f6a7c9aa 100644 --- a/frontend/src/app/main/ui/auth/verify_token.cljs +++ b/frontend/src/app/main/ui/auth/verify_token.cljs @@ -41,13 +41,15 @@ [tdata] (case (:state tdata) :created - (st/emit! (dm/success (tr "auth.notifications.team-invitation-accepted")) - (du/fetch-profile) - (rt/nav :dashboard-projects {:team-id (:team-id tdata)})) + (st/emit! + (dm/success (tr "auth.notifications.team-invitation-accepted")) + (du/fetch-profile) + (rt/nav :dashboard-projects {:team-id (:team-id tdata)})) :pending - (let [token (:invitation-token tdata)] - (st/emit! (rt/nav :auth-register {} {:invitation-token token}))))) + (let [token (:invitation-token tdata) + route-id (:redirect-to tdata :auth-register)] + (st/emit! (rt/nav route-id {} {:invitation-token token}))))) (defmethod handle-token :default [_tdata]