🐛 Fix corner cases on invitation/signup flows.

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

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}))

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?

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

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" : {

frontend/resources/styles/main/partials/dashboard-sidebar.scss

@@ -354,7 +354,6 @@
   }
 
   input[type=submit] {
-    width: 120px;
     margin-bottom: 0px;
   }
 }

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

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)))))
 

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")])])

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}]

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"))}]]]]]]))