From 4991cae5ad8a0638565341382c7b0452ec9f8c6e Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 16 Feb 2021 17:31:22 +0100 Subject: [PATCH] :bug: Fix corner cases on invitation/signup flows. --- CHANGES.md | 1 + backend/src/app/http/auth/google.clj | 124 +++++++++++------- backend/src/app/rpc/mutations/profile.clj | 42 ++---- .../src/app/rpc/mutations/verify_token.clj | 89 ++++++++----- frontend/resources/locales.json | 29 ++++ .../main/partials/dashboard-sidebar.scss | 1 - frontend/src/app/main/repo.cljs | 6 +- frontend/src/app/main/ui/auth/login.cljs | 14 +- frontend/src/app/main/ui/auth/register.cljs | 13 +- .../src/app/main/ui/auth/verify_token.cljs | 8 +- .../src/app/main/ui/dashboard/team_form.cljs | 45 ++++--- 11 files changed, 223 insertions(+), 149 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index b2e94b83f..0162c7f7e 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -19,6 +19,7 @@ - Have language change notification written in the new language [Taiga #1205](https://tree.taiga.io/project/penpot/issue/1205) - Properly handle errors on github, gitlab and ldap auth backends. - Properly mark profile auth backend (on first register/ auth with 3rd party auth provider). +- Fix corner cases on invitation/signup flows. ## 1.2.0-alpha diff --git a/backend/src/app/http/auth/google.clj b/backend/src/app/http/auth/google.clj index 653352d42..be6a0e5fe 100644 --- a/backend/src/app/http/auth/google.clj +++ b/backend/src/app/http/auth/google.clj @@ -5,7 +5,7 @@ ;; This Source Code Form is "Incompatible With Secondary Licenses", as ;; defined by the Mozilla Public License, v. 2.0. ;; -;; Copyright (c) 2020 UXBOX Labs SL +;; Copyright (c) 2020-2021 UXBOX Labs SL (ns app.http.auth.google (:require @@ -43,6 +43,7 @@ req {:method :post :headers {"content-type" "application/x-www-form-urlencoded"} :uri "https://oauth2.googleapis.com/token" + :timeout 6000 :body (u/map->query-string params)} res (http/send! req)] @@ -69,54 +70,85 @@ (log/error e "unexpected exception on get-user-info") nil))) -(defn- auth - [{:keys [tokens] :as cfg} _req] - (let [token (tokens :generate {:iss :google-oauth :exp (dt/in-future "15m")}) - params {:scope scope - :access_type "offline" - :include_granted_scopes true - :state token - :response_type "code" - :redirect_uri (build-redirect-url cfg) - :client_id (:client-id cfg)} - query (u/map->query-string params) - uri (-> (u/uri base-goauth-uri) - (assoc :query query))] +(defn- retrieve-info + [{:keys [tokens] :as cfg} request] + (let [token (get-in request [:params :state]) + 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)) + + (cond-> info + (some? (:invitation-token state)) + (assoc :invitation-token (:invitation-token state))))) + +(defn- register-profile + [{:keys [rpc] :as cfg} info] + (let [method-fn (get-in rpc [:methods :mutation :login-or-register]) + profile (method-fn {:email (:email info) + :backend "google" + :fullname (:fullname info)})] + (cond-> profile + (some? (:invitation-token info)) + (assoc :invitation-token (:invitation-token info))))) + +(defn- generate-redirect-uri + [{:keys [tokens] :as cfg} profile] + (let [token (or (:invitation-token profile) + (tokens :generate {:iss :auth + :exp (dt/in-future "15m") + :profile-id (:id profile)}))] + (-> (u/uri (:public-uri cfg)) + (assoc :path "/#/auth/verify-token") + (assoc :query (u/map->query-string {:token token}))))) + +(defn- generate-error-redirect-uri + [cfg] + (-> (u/uri (:public-uri cfg)) + (assoc :path "/#/auth/login") + (assoc :query (u/map->query-string {:error "unable-to-auth"})))) + +(defn- redirect-response + [uri] + {:status 302 + :headers {"location" (str uri)} + :body ""}) + +(defn- auth-handler + [{:keys [tokens] :as cfg} request] + (let [invitation (get-in request [:params :invitation-token]) + state (tokens :generate + {:iss :google-oauth + :invitation-token invitation + :exp (dt/in-future "15m")}) + params {:scope scope + :access_type "offline" + :include_granted_scopes true + :state state + :response_type "code" + :redirect_uri (build-redirect-url cfg) + :client_id (:client-id cfg)} + query (u/map->query-string params) + uri (-> (u/uri base-goauth-uri) + (assoc :query query))] + {:status 200 :body {:redirect-uri (str uri)}})) -(defn- callback - [{:keys [tokens rpc session] :as cfg} request] +(defn- callback-handler + [{:keys [session] :as cfg} request] (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) - :backend "google" - :fullname (:fullname info)}) - token (tokens :generate {:iss :auth - :exp (dt/in-future "15m") - :profile-id (:id profile)}) - uri (-> (u/uri (:public-uri cfg)) - (assoc :path "/#/auth/verify-token") - (assoc :query (u/map->query-string {:token token}))) - - sxf ((:create session) (:id profile)) - rsp {:status 302 :headers {"location" (str uri)} :body ""}] - (sxf request rsp)) + (let [info (retrieve-info cfg request) + profile (register-profile cfg info) + uri (generate-redirect-uri cfg profile) + sxf ((:create session) (:id profile))] + (sxf request (redirect-response uri))) (catch Exception _e - (let [uri (-> (u/uri (:public-uri cfg)) - (assoc :path "/#/auth/login") - (assoc :query (u/map->query-string {:error "unable-to-auth"})))] - {:status 302 - :headers {"location" (str uri)} - :body ""})))) + (-> (generate-error-redirect-uri cfg) + (redirect-response))))) (s/def ::client-id ::us/not-empty-string) (s/def ::client-secret ::us/not-empty-string) @@ -139,7 +171,7 @@ [_ cfg] (if (and (:client-id cfg) (:client-secret cfg)) - {:auth-handler #(auth cfg %) - :callback-handler #(callback cfg %)} + {:auth-handler #(auth-handler cfg %) + :callback-handler #(callback-handler cfg %)} {:auth-handler default-handler :callback-handler default-handler})) diff --git a/backend/src/app/rpc/mutations/profile.clj b/backend/src/app/rpc/mutations/profile.clj index 81ab78746..09a21079d 100644 --- a/backend/src/app/rpc/mutations/profile.clj +++ b/backend/src/app/rpc/mutations/profile.clj @@ -19,7 +19,6 @@ [app.media :as media] [app.rpc.mutations.projects :as projects] [app.rpc.mutations.teams :as teams] - [app.rpc.mutations.verify-token :refer [process-token]] [app.rpc.queries.profile :as profile] [app.storage :as sto] [app.tasks :as tasks] @@ -48,13 +47,13 @@ (declare create-profile-relations) (declare email-domain-in-whitelist?) -(s/def ::token ::us/not-empty-string) +(s/def ::invitation-token ::us/not-empty-string) (s/def ::register-profile (s/keys :req-un [::email ::password ::fullname] - :opt-un [::token])) + :opt-un [::invitation-token])) (sv/defmethod ::register-profile {:auth false :rlimit :password} - [{:keys [pool tokens session] :as cfg} {:keys [token] :as params}] + [{:keys [pool tokens session] :as cfg} params] (when-not (cfg/get :registration-enabled) (ex/raise :type :restriction :code :registration-disabled)) @@ -69,30 +68,18 @@ (create-profile-relations conn))] (create-profile-initial-data conn profile) - (if token - ;; If token comes in params, this is because the user comes - ;; from team-invitation process; in this case we revalidate - ;; the token and process the token claims again with the new - ;; profile data. + (if-let [token (:invitation-token params)] + ;; 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). (let [claims (tokens :verify {:token token :iss :team-invitation}) - claims (assoc claims :member-id (:id profile)) - params (assoc params :profile-id (:id profile)) - cfg (assoc cfg :conn conn)] - - (process-token cfg params claims) - - ;; Automatically mark the created profile as active because - ;; we already have the verification of email with the - ;; team-invitation token. - (db/update! conn :profile - {:is-active true} - {:id (:id profile)}) - - ;; Return profile data and create http session for - ;; automatically login the profile. - (with-meta (assoc profile - :is-active true - :claims claims) + 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))})) ;; If no token is provided, send a verification email @@ -117,7 +104,6 @@ :name (:fullname profile) :token vtoken :extra-data ptoken}) - profile))))) (defn email-domain-in-whitelist? diff --git a/backend/src/app/rpc/mutations/verify_token.clj b/backend/src/app/rpc/mutations/verify_token.clj index 357e20e90..dbeadaf7a 100644 --- a/backend/src/app/rpc/mutations/verify_token.clj +++ b/backend/src/app/rpc/mutations/verify_token.clj @@ -83,49 +83,78 @@ :internal.tokens.team-invitation/member-email] :opt-un [:internal.tokens.team-invitation/member-id])) +(defn- accept-invitation + [{:keys [conn] :as cfg} {:keys [member-id team-id role] :as claims}] + (let [params (merge {:team-id team-id + :profile-id member-id} + (teams/role->params role)) + member (profile/retrieve-profile conn member-id)] + + ;; Insert the invited member to the team + (db/insert! conn :team-profile-rel params {:on-conflict-do-nothing true}) + + ;; If profile is not yet verified, mark it as verified because + ;; accepting an invitation link serves as verification. + (when-not (:is-active member) + (db/update! conn :profile + {:is-active true} + {:id member-id})) + (assoc member :is-active true))) + (defmethod process-token :team-invitation - [{:keys [conn session] :as cfg} {:keys [profile-id token]} {:keys [member-id team-id role] :as claims}] + [{:keys [session] :as cfg} {:keys [profile-id token]} {:keys [member-id] :as claims}] (us/assert ::team-invitation-claims claims) - (if (uuid? member-id) - (let [params (merge {:team-id team-id - :profile-id member-id} - (teams/role->params role)) - claims (assoc claims :state :created) - member (profile/retrieve-profile conn member-id)] - - (db/insert! conn :team-profile-rel params - {:on-conflict-do-nothing true}) - - ;; If profile is not yet verified, mark it as verified because - ;; accepting an invitation link serves as verification. - (when-not (:is-active member) - (db/update! conn :profile - {:is-active true} - {:id member-id})) - - (if (and (uuid? profile-id) - (= member-id profile-id)) + (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)) + (do + (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. - claims + (assoc claims :status :created) - ;; If the session does not matches the invited member id, - ;; replace the session with a new one matching the invited - ;; member. This techinique should be considered secure because - ;; the user clicking the link he already has access to the - ;; email account. - (with-meta claims + ;; If the session does not matches the invited member, replace + ;; the session with a new one matching the invited member. + ;; This techinique should be considered secure because the + ;; user clicking the link he already has access to the email + ;; account. + (with-meta + (assoc claims :status :created) {:transform-response ((:create session) member-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)) + (do + (accept-invitation cfg (assoc claims :member-id profile-id)) + (assoc claims :state :created)) + + ;; 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)) + (do + (accept-invitation cfg claims) + (with-meta + (assoc claims :state :created) + {:transform-response ((:create session) member-id)})) + ;; In this case, we wait until frontend app redirect user to ;; registeration 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. - (assoc claims - :token token - :state :pending))) + :else + {:invitation-token token + :iss :team-invitation + :state :pending})) ;; --- Default diff --git a/frontend/resources/locales.json b/frontend/resources/locales.json index cce095e48..09c494db3 100644 --- a/frontend/resources/locales.json +++ b/frontend/resources/locales.json @@ -830,6 +830,8 @@ "es" : "Actualizado: %s" } }, + + "errors.google-auth-not-enabled" : { "translations" : { "en" : "Authentication with google disabled on backend", @@ -837,6 +839,13 @@ } }, + "errors.unexpected-token" : { + "translations" : { + "en" : "Unknown token", + "es" : "Token desconocido" + } + }, + "errors.profile-is-muted" : { "translations" : { "en" : "Your profile has emails muted (spam reports or high bounces).", @@ -1899,6 +1908,26 @@ "es" : "Quitar" } }, + "labels.create-team": { + "translations" : { + "en" : "Create new team", + "es" : "Crea un nuevo equipo" + } + }, + + "labels.update-team": { + "translations" : { + "en" : "Update team", + "es" : "Actualiza el equipo" + } + }, + "labels.rename-team": { + "translations" : { + "en" : "Rename team", + "es" : "Renomba el equipo" + } + }, + "labels.rename" : { "used-in" : [ "src/app/main/ui/dashboard/sidebar.cljs:314", "src/app/main/ui/dashboard/files.cljs:84", "src/app/main/ui/dashboard/grid.cljs:178" ], "translations" : { diff --git a/frontend/resources/styles/main/partials/dashboard-sidebar.scss b/frontend/resources/styles/main/partials/dashboard-sidebar.scss index eca4e70e2..801e91d5b 100644 --- a/frontend/resources/styles/main/partials/dashboard-sidebar.scss +++ b/frontend/resources/styles/main/partials/dashboard-sidebar.scss @@ -354,7 +354,6 @@ } input[type=submit] { - width: 120px; margin-bottom: 0px; } } diff --git a/frontend/src/app/main/repo.cljs b/frontend/src/app/main/repo.cljs index 45fa61166..cb914d078 100644 --- a/frontend/src/app/main/repo.cljs +++ b/frontend/src/app/main/repo.cljs @@ -81,19 +81,19 @@ (defmethod mutation :login-with-google [id params] (let [uri (str cfg/public-uri "/api/oauth/google")] - (->> (http/send! {:method :post :uri uri}) + (->> (http/send! {:method :post :uri uri :query params}) (rx/mapcat handle-response)))) (defmethod mutation :login-with-gitlab [id params] (let [uri (str cfg/public-uri "/api/oauth/gitlab")] - (->> (http/send! {:method :post :uri uri}) + (->> (http/send! {:method :post :uri uri :query params}) (rx/mapcat handle-response)))) (defmethod mutation :login-with-github [id params] (let [uri (str cfg/public-uri "/api/oauth/github")] - (->> (http/send! {:method :post :uri uri}) + (->> (http/send! {:method :post :uri uri :query params}) (rx/mapcat handle-response)))) (defmethod mutation :upload-file-media-object diff --git a/frontend/src/app/main/ui/auth/login.cljs b/frontend/src/app/main/ui/auth/login.cljs index 0a196b0aa..e9a4db5cf 100644 --- a/frontend/src/app/main/ui/auth/login.cljs +++ b/frontend/src/app/main/ui/auth/login.cljs @@ -5,7 +5,7 @@ ;; This Source Code Form is "Incompatible With Secondary Licenses", as ;; defined by the Mozilla Public License, v. 2.0. ;; -;; Copyright (c) 2020 UXBOX Labs SL +;; Copyright (c) 2020-2021 UXBOX Labs SL (ns app.main.ui.auth.login (:require @@ -33,25 +33,25 @@ (s/keys :req-un [::email ::password])) (defn- login-with-google - [event] + [event params] (dom/prevent-default event) - (->> (rp/mutation! :login-with-google {}) + (->> (rp/mutation! :login-with-google params) (rx/subs (fn [{:keys [redirect-uri] :as rsp}] (.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] + [event params] (dom/prevent-default event) - (->> (rp/mutation! :login-with-gitlab {}) + (->> (rp/mutation! :login-with-gitlab params) (rx/subs (fn [{:keys [redirect-uri] :as rsp}] (.replace js/location redirect-uri))))) (defn- login-with-github - [event] + [event params] (dom/prevent-default event) - (->> (rp/mutation! :login-with-github {}) + (->> (rp/mutation! :login-with-github params) (rx/subs (fn [{:keys [redirect-uri] :as rsp}] (.replace js/location redirect-uri))))) diff --git a/frontend/src/app/main/ui/auth/register.cljs b/frontend/src/app/main/ui/auth/register.cljs index 49605fcee..ec28f1ce7 100644 --- a/frontend/src/app/main/ui/auth/register.cljs +++ b/frontend/src/app/main/ui/auth/register.cljs @@ -81,11 +81,8 @@ (mf/use-callback (fn [form data] (reset! submitted? false) - (if (and (:is-active data) (:claims data)) - (let [message (tr "auth.notifications.team-invitation-accepted")] - (st/emit! (rt/nav :dashboard-projects {:team-id (get-in data [:claims :team-id])}) - (du/fetch-profile) - (dm/success message))) + (if-let [token (:invitation-token data)] + (st/emit! (rt/nav :auth-verify-token {} {:token token})) (st/emit! (rt/nav :auth-register-success {} {:email (:email data)}))))) on-submit @@ -161,19 +158,19 @@ (when cfg/google-client-id [:a.btn-ocean.btn-large.btn-google-auth - {:on-click login/login-with-google} + {:on-click #(login/login-with-google % params)} "Login with Google"]) (when cfg/gitlab-client-id [:a.btn-ocean.btn-large.btn-gitlab-auth - {:on-click login/login-with-gitlab} + {:on-click #(login/login-with-gitlab % params)} [:img.logo {:src "/images/icons/brand-gitlab.svg"}] (tr "auth.login-with-gitlab-submit")]) (when cfg/github-client-id [:a.btn-ocean.btn-large.btn-github-auth - {:on-click login/login-with-github} + {:on-click #(login/login-with-github % params)} [:img.logo {:src "/images/icons/brand-github.svg"}] (tr "auth.login-with-github-submit")])]) diff --git a/frontend/src/app/main/ui/auth/verify_token.cljs b/frontend/src/app/main/ui/auth/verify_token.cljs index 8e060aa99..349b451d0 100644 --- a/frontend/src/app/main/ui/auth/verify_token.cljs +++ b/frontend/src/app/main/ui/auth/verify_token.cljs @@ -58,12 +58,14 @@ (dm/success message))) :pending - (st/emit! (rt/nav :auth-register {} {:token (:token tdata)})))) + (let [token (:invitation-token tdata)] + (st/emit! (rt/nav :auth-register {} {:invitation-token token}))))) (defmethod handle-token :default [tdata] - (js/console.log "Unhandled token:" (pr-str tdata)) - (st/emit! (rt/nav :auth-login))) + (st/emit! + (rt/nav :auth-login) + (dm/warn (tr "errors.unexpected-token")))) (mf/defc verify-token [{:keys [route] :as props}] diff --git a/frontend/src/app/main/ui/dashboard/team_form.cljs b/frontend/src/app/main/ui/dashboard/team_form.cljs index 4db46c580..d743f76dc 100644 --- a/frontend/src/app/main/ui/dashboard/team_form.cljs +++ b/frontend/src/app/main/ui/dashboard/team_form.cljs @@ -18,10 +18,9 @@ [app.main.data.modal :as modal] [app.main.repo :as rp] [app.main.store :as st] - [app.main.ui.components.forms :refer [input submit-button form]] + [app.main.ui.components.forms :as fm] [app.main.ui.icons :as i] [app.util.dom :as dom] - [app.util.forms :as fm] [app.util.i18n :as i18n :refer [t tr]] [app.util.keyboard :as kbd] [app.util.object :as obj] @@ -90,28 +89,28 @@ [:div.modal-overlay [:div.modal-container.team-form-modal - [:div.modal-header - [:div.modal-header-title - (if team - [:h2 "Rename team"] - [:h2 "Create new team"])] - [:div.modal-close-button - {:on-click (st/emitf (modal/hide))} i/close]] + [:& fm/form {:form form + :on-submit on-submit} - [:div.modal-content.generic-form - [:form - [:& input {:type "text" - :form form - :name :name - :label "Enter new team name:"}]]] + [:div.modal-header + [:div.modal-header-title + (if team + [:h2 (tr "labels.rename-team")] + [:h2 (tr "labels.create-team")])] + [:div.modal-close-button + {:on-click (st/emitf (modal/hide))} i/close]] - [:div.modal-footer - [:div.action-buttons - [:& submit-button - {:form form - :on-click on-submit - :label (if team - "Update team" - "Create team")}]]]]])) + [:div.modal-content.generic-form + [:& fm/input {:type "text" + :form form + :name :name + :label "Enter new team name:"}]] + + [:div.modal-footer + [:div.action-buttons + [:& fm/submit-button + {:label (if team + (tr "labels.update-team") + (tr "labels.create-team"))}]]]]]]))