Merge pull request #3461 from metabase/fix-google-auth-reënabling-users

Fix reënabling Google Auth users if Google Auth is disabled 

src/metabase/api/session.clj

@@ -77,7 +77,7 @@
   (throttle/check (forgot-password-throttlers :ip-address) remote-address)
   (throttle/check (forgot-password-throttlers :email)      email)
   ;; Don't leak whether the account doesn't exist, just pretend everything is ok
-  (when-let [{user-id :id, google-auth? :google_auth} (db/select-one ['User :id :google_auth] :email email)]
+  (when-let [{user-id :id, google-auth? :google_auth} (db/select-one ['User :id :google_auth] :email email, :is_active true)]
     (let [reset-token        (set-user-password-reset-token! user-id)
           password-reset-url (str (public-settings/site-url request) "/auth/reset_password/" reset-token)]
       (email/send-password-reset-email email google-auth? server-name password-reset-url)
@@ -93,7 +93,7 @@
   [^String token]
   (when-let [[_ user-id] (re-matches #"(^\d+)_.+$" token)]
     (let [user-id (Integer/parseInt user-id)]
-      (when-let [{:keys [reset_token reset_triggered], :as user} (db/select-one [User :id :last_login :reset_triggered :reset_token], :id user-id)]
+      (when-let [{:keys [reset_token reset_triggered], :as user} (db/select-one [User :id :last_login :reset_triggered :reset_token], :id user-id, :is_active true)]
         ;; Make sure the plaintext token matches up with the hashed one for this user
         (when (u/ignore-exceptions
                 (creds/bcrypt-verify token reset_token))
@@ -134,7 +134,7 @@
 ;; or `metabase.integrations.auth.google` if we decide to add more 3rd-party SSO options
 
 (defsetting google-auth-client-id
-  "Client ID for Google Auth SSO.")
+  "Client ID for Google Auth SSO. If this is set, Google Auth is considered to be enabled.")
 
 (defsetting google-auth-auto-create-accounts-domain
   "When set, allow users to sign up on their own if their Google account email address is from this domain.")
@@ -148,8 +148,7 @@
         (throw (ex-info "Email is not verified." {:status-code 400}))))))
 
 ;; TODO - are these general enough to move to `metabase.util`?
-(defn- email->domain   "ABC" ^String
-  [email]
+(defn- email->domain ^String [email]
   (last (re-find #"^.*@(.*$)" email)))
 
 (defn- email-in-domain? ^Boolean [email domain]

src/metabase/api/user.clj

@@ -2,9 +2,11 @@
   (:require [cemerick.friend.credentials :as creds]
             [compojure.core :refer [defroutes GET DELETE POST PUT]]
             [metabase.api.common :refer :all]
+            [metabase.api.session :as session-api]
             [metabase.db :as db]
             [metabase.email.messages :as email]
-            [metabase.models.user :refer [User create-user! set-user-password! set-user-password-reset-token! form-password-reset-url]]))
+            [metabase.models.user :refer [User create-user! set-user-password! set-user-password-reset-token! form-password-reset-url]]
+            [metabase.util :as u]))
 
 (defn- check-self-or-superuser
   "Check that USER-ID is `*current-user-id*` or that `*current-user*` is a superuser, or throw a 403."
@@ -18,24 +20,30 @@
   []
   (db/select [User :id :first_name :last_name :email :is_superuser :google_auth :last_login], :is_active true))
 
+(defn- reäctivate-user! [existing-user first-name last-name]
+  (when-not (:is_active existing-user)
+    (db/update! User (u/get-id existing-user)
+      :first_name    first-name
+      :last_name     last-name
+      :is_active     true
+      :is_superuser  false
+      ;; if the user orignally logged in via Google Auth and it's no longer enabled, convert them into a regular user (see Issue #3323)
+      :google_auth   (boolean (and (:google_auth existing-user)
+                                   (session-api/google-auth-client-id))))) ; if google-auth-client-id is set it means Google Auth is enabled
+  ;; now return the existing user whether they were originally active or not
+  (User (u/get-id existing-user)))
+
 
 (defendpoint POST "/"
-  "Create a new `User`."
+  "Create a new `User`, or or reäctivate an existing one."
   [:as {{:keys [first_name last_name email password]} :body}]
   {first_name [Required NonEmptyString]
    last_name  [Required NonEmptyString]
    email      [Required Email]}
   (check-superuser)
-  (if-let [existing-user (db/select-one [User :id :is_active], :email email)]
-    (do (when-not (:is_active existing-user)
-          ;; this user already exists but is inactive, so simply reactivate the account
-          (db/update! User (:id existing-user)
-            :first_name    first_name
-            :last_name     last_name
-            :is_active     true
-            :is_superuser  false))
-        ;; now return the existing user whether they were originally active or not
-        (User (:id existing-user)))
+  (if-let [existing-user (db/select-one [User :id :is_active :google_auth], :email email)]
+    ;; this user already exists but is inactive, so simply reactivate the account
+    (reäctivate-user! existing-user first_name last_name)
     ;; new user account, so create it
     (create-user! first_name last_name email, :password password, :send-welcome true, :invitor @*current-user*)))
 

test/metabase/api/user_test.clj

@@ -272,3 +272,18 @@
 ;; Check that non-superusers are denied access to resending invites
 (expect "You don't have permissions to do that."
   ((user->client :rasta) :post 403 (format "user/%d/send_invite" (user->id :crowberto))))
+
+
+;; test that when disabling Google auth if a user gets disabled and reënabled they are no longer Google Auth (Bug #3323)
+(expect
+  {:is_active true, :google_auth false}
+  (tu/with-temporary-setting-values [google-auth-client-id "ABCDEFG"]
+    (tu/with-temp User [user {:google_auth true}]
+      (db/update! User (u/get-id user)
+        :is_active false)
+      (tu/with-temporary-setting-values [google-auth-client-id nil]
+        ((user->client :crowberto) :post 200 "user"
+         {:first_name "Cam"
+          :last_name  "Era"
+          :email      (:email user)})
+        (db/select-one [User :is_active :google_auth] :id (u/get-id user))))))