Add `login_attributes` to `core_user`

resources/migrations/000_migrations.yaml

@@ -4068,6 +4068,17 @@ databaseChangeLog:
               name: fields_hash
               type: text
               remarks: 'Computed hash of all of the fields associated to this table'
+  - changeSet:
+      id: 77
+      author: senior
+      comment: 'Added 0.30.0'
+      changes:
+        - addColumn:
+            tableName: core_user
+            columns:
+              name: login_attributes
+              type: text
+              remarks: 'JSON serialized map with attributes used for row level permissions'
   - changeSet:
       id: 79
       author: camsaul

src/metabase/api/session.clj

@@ -209,16 +209,19 @@
     (throw (ex-info (tru "You''ll need an administrator to create a Metabase account before you can use Google to log in.")
              {:status-code 428}))))
 
-(defn- google-auth-create-new-user! [first-name last-name email]
+(s/defn ^:private google-auth-create-new-user!
+  [{:keys [email] :as new-user} :- user/NewUser]
   (check-autocreate-user-allowed-for-email email)
   ;; this will just give the user a random password; they can go reset it if they ever change their mind and want to
   ;; log in without Google Auth; this lets us keep the NOT NULL constraints on password / salt without having to make
   ;; things hairy and only enforce those for non-Google Auth users
-  (user/create-new-google-auth-user! first-name last-name email))
+  (user/create-new-google-auth-user! new-user))
 
 (defn- google-auth-fetch-or-create-user! [first-name last-name email]
   (if-let [user (or (db/select-one [User :id :last_login] :email email)
-                    (google-auth-create-new-user! first-name last-name email))]
+                    (google-auth-create-new-user! {:first_name first-name
+                                                   :last_name  last-name
+                                                   :email      email}))]
     {:id (create-session! user)}))
 
 (api/defendpoint POST "/google_auth"

src/metabase/api/user.clj

@@ -9,6 +9,7 @@
             [metabase.models.user :as user :refer [User]]
             [metabase.util :as u]
             [metabase.util.schema :as su]
+            [puppetlabs.i18n.core :refer [tru]]
             [schema.core :as s]
             [toucan.db :as db]))
 
@@ -53,14 +54,16 @@
 
 (api/defendpoint POST "/"
   "Create a new `User`, return a 400 if the email address is already taken"
-  [:as {{:keys [first_name last_name email password]} :body}]
-  {first_name su/NonBlankString
-   last_name  su/NonBlankString
-   email      su/Email}
+  [:as {{:keys [first_name last_name email password login_attributes] :as body} :body}]
+  {first_name       su/NonBlankString
+   last_name        su/NonBlankString
+   email            su/Email
+   login_attributes (s/maybe user/LoginAttributes)}
   (api/check-superuser)
   (api/checkp (not (db/exists? User :email email))
-    "email" "Email address already in use.")
-  (let [new-user-id (u/get-id (user/invite-user! first_name last_name email password @api/*current-user*))]
+    "email" (tru "Email address already in use."))
+  (let [new-user-id (u/get-id (user/invite-user! (select-keys body [:first_name :last_name :email :password :login_attributes])
+                                                 @api/*current-user*))]
     (fetch-user :id new-user-id)))
 
 (api/defendpoint GET "/current"
@@ -78,22 +81,24 @@
 
 (api/defendpoint PUT "/:id"
   "Update an existing, active `User`."
-  [id :as {{:keys [email first_name last_name is_superuser]} :body}]
-  {email      su/Email
-   first_name (s/maybe su/NonBlankString)
-   last_name  (s/maybe su/NonBlankString)}
+  [id :as {{:keys [email first_name last_name is_superuser login_attributes] :as body} :body}]
+  {email            (s/maybe su/Email)
+   first_name       (s/maybe su/NonBlankString)
+   last_name        (s/maybe su/NonBlankString)
+   login_attributes (s/maybe user/LoginAttributes)}
   (check-self-or-superuser id)
   ;; only allow updates if the specified account is active
   (api/check-404 (db/exists? User, :id id, :is_active true))
   ;; can't change email if it's already taken BY ANOTHER ACCOUNT
   (api/checkp (not (db/exists? User, :email email, :id [:not= id]))
-    "email" "Email address already associated to another user.")
-  (api/check-500 (db/update-non-nil-keys! User id
-                   :email        email
-                   :first_name   first_name
-                   :last_name    last_name
-                   :is_superuser (when (:is_superuser @api/*current-user*)
-                                   is_superuser)))
+    "email" (tru "Email address already associated to another user."))
+  (api/check-500
+   (db/update! User id
+     (u/select-keys-when body
+       :present #{:login_attributes}
+       :non-nil (set (concat [:first_name :last_name :email]
+                             (when (:is_superuser @api/*current-user*)
+                               [:is_superuser]))))))
   (fetch-user :id id))
 
 (api/defendpoint PUT "/:id/reactivate"
@@ -104,7 +109,7 @@
     (api/check-404 user)
     ;; Can only reactivate inactive users
     (api/check (not (:is_active user))
-      [400 {:message "Not able to reactivate an active user"}])
+      [400 {:message (tru "Not able to reactivate an active user")}])
     (reactivate-user! user)))
 
 
@@ -119,7 +124,7 @@
     (when-not (:is_superuser @api/*current-user*)
       (api/checkp (creds/bcrypt-verify (str (:password_salt user) old_password) (:password user))
         "old_password"
-        "Invalid password")))
+        (tru "Invalid password"))))
   (user/set-password! id password)
   ;; return the updated User
   (fetch-user :id id))

src/metabase/integrations/ldap.clj

@@ -209,7 +209,10 @@
   "Using the `user-info` (from `find-user`) get the corresponding Metabase user, creating it if necessary."
   [{:keys [first-name last-name email groups]} password]
   (let [user (or (db/select-one [User :id :last_login] :email email)
-             (user/create-new-ldap-auth-user! first-name last-name email password))]
+                 (user/create-new-ldap-auth-user! {:first_name first-name
+                                                   :last_name  last-name
+                                                   :email      email
+                                                   :password   password}))]
     (u/prog1 user
       (when password
         (user/set-password! (:id user) password))

src/metabase/models/user.clj

 (ns metabase.models.user
   (:require [cemerick.friend.credentials :as creds]
-            [clojure.string :as s]
+            [clojure.string :as str]
             [clojure.tools.logging :as log]
             [metabase
              [public-settings :as public-settings]
@@ -9,7 +9,10 @@
             [metabase.models
              [permissions-group :as group]
              [permissions-group-membership :as perm-membership :refer [PermissionsGroupMembership]]]
-            [metabase.util.date :as du]
+            [metabase.util
+             [date :as du]
+             [schema :as su]]
+            [schema.core :as s]
             [toucan
              [db :as db]
              [models :as models]])
@@ -23,7 +26,7 @@
   (assert (u/email? email)
     (format "Not a valid email: '%s'" email))
   (assert (and (string? password)
-               (not (s/blank? password))))
+               (not (str/blank? password))))
   (assert (not (:password_salt user))
     "Don't try to pass an encrypted password to (insert! User). Password encryption is handled by pre-insert.")
   (let [salt     (str (UUID/randomUUID))
@@ -102,7 +105,7 @@
 
 (def all-user-fields
   "Seq of all the columns stored for a user"
-  (vec (concat default-user-fields [:google_auth :ldap_auth :is_active :updated_at])))
+  (vec (concat default-user-fields [:google_auth :ldap_auth :is_active :updated_at :login_attributes])))
 
 (u/strict-extend (class User)
   models/IModel
@@ -114,7 +117,8 @@
           :post-insert    post-insert
           :pre-update     pre-update
           :post-select    post-select
-          :pre-delete     pre-delete}))
+          :pre-delete     pre-delete
+          :types          (constantly {:login_attributes :json})}))
 
 
 ;;; --------------------------------------------------- Helper Fns ---------------------------------------------------
@@ -127,42 +131,51 @@
         join-url    (str (form-password-reset-url reset-token) "#new")]
     (email/send-new-user-email! new-user invitor join-url)))
 
-(defn invite-user!
+(def LoginAttributes
+  "Login attributes, currently not collected for LDAP or Google Auth. Will ultimately be stored as JSON"
+  {su/KeywordOrString (s/cond-pre s/Str s/Num)})
+
+(def NewUser
+  "Required/optionals parameters needed to create a new user (for any backend)"
+  {:first_name                        su/NonBlankString
+   :last_name                         su/NonBlankString
+   :email                             su/Email
+   (s/optional-key :password)         (s/maybe su/NonBlankString)
+   (s/optional-key :login_attributes) (s/maybe LoginAttributes)
+   (s/optional-key :google_auth)      s/Bool
+   (s/optional-key :ldap_auth)        s/Bool})
+
+(def ^:private Invitor
+  "Map with info about the admin admin creating the user, used in the new user notification code"
+  {:email      su/Email
+   :first_name su/NonBlankString
+   s/Any       s/Any})
+
+(s/defn ^:private insert-new-user!
+  "Creates a new user, defaulting the password when not provided"
+  [new-user :- NewUser]
+  (db/insert! User (update new-user :password #(or % (str (UUID/randomUUID))))))
+
+(s/defn invite-user!
   "Convenience function for inviting a new `User` and sending out the welcome email."
-  [first-name last-name email-address password invitor]
-  {:pre [(string? first-name) (string? last-name) (u/email? email-address) (u/maybe? string? password) (map? invitor)]}
+  [new-user :- NewUser, invitor :- Invitor]
   ;; create the new user
-  (u/prog1 (db/insert! User
-             :email       email-address
-             :first_name  first-name
-             :last_name   last-name
-             :password    (or password (str (UUID/randomUUID))))
+  (u/prog1 (insert-new-user! new-user)
     (send-welcome-email! <> invitor)))
 
-(defn create-new-google-auth-user!
+(s/defn create-new-google-auth-user!
   "Convenience for creating a new user via Google Auth. This account is considered active immediately; thus all active
   admins will recieve an email right away."
-  [first-name last-name email-address]
-  {:pre [(string? first-name) (string? last-name) (u/email? email-address)]}
-  (u/prog1 (db/insert! User
-             :email       email-address
-             :first_name  first-name
-             :last_name   last-name
-             :password    (str (UUID/randomUUID))
-             :google_auth true)
+  [new-user :- NewUser]
+  (u/prog1 (insert-new-user! (assoc new-user :google_auth true))
     ;; send an email to everyone including the site admin if that's set
     (email/send-user-joined-admin-notification-email! <>, :google-auth? true)))
 
-(defn create-new-ldap-auth-user!
+(s/defn create-new-ldap-auth-user!
   "Convenience for creating a new user via LDAP. This account is considered active immediately; thus all active admins
   will recieve an email right away."
-  [first-name last-name email-address password]
-  {:pre [(string? first-name) (string? last-name) (u/email? email-address)]}
-  (db/insert! User :email      email-address
-                   :first_name first-name
-                   :last_name  last-name
-                   :password   password
-                   :ldap_auth  true))
+  [new-user :- NewUser]
+  (insert-new-user! (assoc new-user :ldap_auth true)))
 
 (defn set-password!
   "Updates the stored password for a specified `User` by hashing the password with a random salt."

test/metabase/api/session_test.clj

@@ -216,7 +216,9 @@
 (expect
   clojure.lang.ExceptionInfo
   (tu/with-temporary-setting-values [google-auth-auto-create-accounts-domain "sf-toucannery.com"]
-    (#'session-api/google-auth-create-new-user! "Rasta" "Toucan" "rasta@metabase.com")))
+    (#'session-api/google-auth-create-new-user! {:first_name "Rasta"
+                                                 :last_name  "Toucan"
+                                                 :email      "rasta@metabase.com"})))
 
 ;; should totally work if the email domains match up
 (expect
@@ -224,7 +226,9 @@
   (et/with-fake-inbox
     (tu/with-temporary-setting-values [google-auth-auto-create-accounts-domain "sf-toucannery.com"
                                        admin-email                             "rasta@toucans.com"]
-      (select-keys (u/prog1 (#'session-api/google-auth-create-new-user! "Rasta" "Toucan" "rasta@sf-toucannery.com")
+      (select-keys (u/prog1 (#'session-api/google-auth-create-new-user! {:first_name "Rasta"
+                                                                         :last_name  "Toucan"
+                                                                         :email      "rasta@sf-toucannery.com"})
                      (db/delete! User :id (:id <>))) ; make sure we clean up after ourselves !
                    [:first_name :last_name :email]))))
 

test/metabase/api/user_test.clj

@@ -22,15 +22,16 @@
 (expect (get middleware/response-unauthentic :body) (http/client :get 401 "user/current"))
 
 (def ^:private user-defaults
-  {:ldap_auth    false
-   :is_active    true
-   :is_superuser false
-   :google_auth  false
-   :is_qbnewb    true
-   :id           true
-   :last_login   nil
-   :updated_at   true
-   :date_joined  true})
+  {:ldap_auth        false
+   :is_active        true
+   :is_superuser     false
+   :google_auth      false
+   :is_qbnewb        true
+   :id               true
+   :last_login       nil
+   :updated_at       true
+   :date_joined      true
+   :login_attributes nil})
 
 (def ^:private test-users
   (map #(merge user-defaults %)
@@ -81,16 +82,18 @@
       email     (str user-name "@metabase.com")]
   (expect
     (merge user-defaults
-           {:email        email
-            :first_name   user-name
-            :last_name    user-name
-            :common_name  (str user-name " " user-name)})
+           {:email            email
+            :first_name       user-name
+            :last_name        user-name
+            :common_name      (str user-name " " user-name)
+            :login_attributes {:test "value"}})
     (et/with-fake-inbox
       (try
         (tu/boolean-ids-and-timestamps
-         ((user->client :crowberto) :post 200 "user" {:first_name user-name
-                                                      :last_name  user-name
-                                                      :email      email}))
+         ((user->client :crowberto) :post 200 "user" {:first_name       user-name
+                                                      :last_name        user-name
+                                                      :email            email
+                                                      :login_attributes {:test "value"}}))
         (finally
           ;; clean up after ourselves
           (db/delete! User :email email))))))
@@ -98,7 +101,7 @@
 ;; Test that reactivating a disabled account works
 (expect
   ;; create a random inactive user
-  (tt/with-temp User [ user {:is_active false}]
+  (tt/with-temp User [user {:is_active false}]
     ;; now try creating the same user again, should re-activiate the original
     ((user->client :crowberto) :put 200 (format "user/%s/reactivate" (u/get-id user))
      {:first_name (:first_name user)
@@ -208,6 +211,16 @@
                                                                    :email     "cam.eron@metabase.com"}))
        (user)])))
 
+;; Test that we can update login attributes after a user has been created
+(expect
+  (merge user-defaults
+         {:is_superuser     true,            :email       "testuser@metabase.com", :first_name "Test",
+          :login_attributes {:test "value"}, :common_name "Test User",             :last_name  "User"})
+  (tt/with-temp User [{user-id :id} {:first_name "Test", :last_name "User", :email "testuser@metabase.com", :is_superuser true}]
+    (tu/boolean-ids-and-timestamps
+     ((user->client :crowberto) :put 200 (str "user/" user-id) {:email            "testuser@metabase.com"
+                                                                :login_attributes {:test "value"}}))))
+
 ;; ## PUT /api/user/:id
 ;; Test that updating a user's email to an existing inactive user's email fails
 (expect

test/metabase/models/user_test.clj

@@ -61,11 +61,15 @@
     (email-test/with-fake-inbox
       (let [new-user-email      (tu/random-email)
             new-user-first-name (tu/random-name)
-            new-user-last-name  (tu/random-name)]
+            new-user-last-name  (tu/random-name)
+            new-user            {:first_name new-user-first-name
+                                 :last_name  new-user-last-name
+                                 :email      new-user-email
+                                 :password   password}]
         (try
           (if google-auth?
-            (user/create-new-google-auth-user! new-user-first-name new-user-last-name new-user-email)
-            (user/invite-user!                 new-user-first-name new-user-last-name new-user-email password invitor))
+            (user/create-new-google-auth-user! (dissoc new-user :password))
+            (user/invite-user!                 new-user invitor))
           (when accept-invite?
             (maybe-accept-invite! new-user-email))
           (sent-emails new-user-email new-user-first-name new-user-last-name)
@@ -73,13 +77,15 @@
           (finally
             (db/delete! User :email new-user-email)))))))
 
+(def ^:private default-invitor
+  {:email "crowberto@metabase.com", :is_active true, :first_name "Crowberto"})
 
 ;; admin shouldn't get email saying user joined until they accept the invite (i.e., reset their password)
 (expect
   {"<New User>"             ["You're invited to join Metabase's Metabase"]}
   (do
     (test-users/delete-temp-users!)
-    (invite-user-accept-and-check-inboxes! :invitor {:email "crowberto@metabase.com", :is_active true}, :accept-invite? false)))
+    (invite-user-accept-and-check-inboxes! :invitor default-invitor, :accept-invite? false)))
 
 ;; admin should get an email when a new user joins...
 (expect
@@ -87,7 +93,7 @@
    "crowberto@metabase.com" ["<New User> accepted their Metabase invite"]}
   (do
     (test-users/delete-temp-users!)
-    (invite-user-accept-and-check-inboxes! :invitor {:email "crowberto@metabase.com", :is_active true})))
+    (invite-user-accept-and-check-inboxes! :invitor default-invitor)))
 
 ;; ...including the site admin if it is set...
 (expect
@@ -96,7 +102,7 @@
    "cam@metabase.com"       ["<New User> accepted their Metabase invite"]}
   (tu/with-temporary-setting-values [admin-email "cam@metabase.com"]
     (test-users/delete-temp-users!)
-    (invite-user-accept-and-check-inboxes! :invitor {:email "crowberto@metabase.com", :is_active true})))
+    (invite-user-accept-and-check-inboxes! :invitor default-invitor)))
 
 ;; ... but if that admin is inactive they shouldn't get an email
 (expect