Refactors to use username/password and adds fallback auth.

frontend/src/metabase/auth/auth.js

@@ -6,7 +6,7 @@ import { push } from "react-router-redux";
 import MetabaseCookies from "metabase/lib/cookies";
 import MetabaseUtils from "metabase/lib/utils";
 import MetabaseAnalytics from "metabase/lib/analytics";
-import MetabaseSettings from "metabase/lib/settings.js";
+import MetabaseSettings from "metabase/lib/settings";
 
 import { clearGoogleAuthCredentials } from "metabase/lib/auth";
 
@@ -20,14 +20,12 @@ export const LOGIN = "metabase/auth/LOGIN";
 export const login = createThunkAction(LOGIN, function(credentials, redirectUrl) {
     return async function(dispatch, getState) {
 
-        if (!MetabaseSettings.ldapEnabled() && !MetabaseUtils.validEmail(credentials.email)) {
+        if (!MetabaseSettings.ldapEnabled() && !MetabaseUtils.validEmail(credentials.username)) {
             return {'data': {'errors': {'email': "Please enter a valid formatted email address."}}};
         }
 
         try {
-            let newSession = MetabaseSettings.ldapEnabled()
-                ? await SessionApi.createWithLdap(credentials)
-                : await SessionApi.create(credentials);
+            let newSession = await SessionApi.create(credentials);
 
             // since we succeeded, lets set the session cookie
             MetabaseCookies.setSessionCookie(newSession.id);

frontend/src/metabase/auth/containers/LoginApp.jsx

@@ -11,7 +11,8 @@ import FormField from "metabase/components/form/FormField.jsx";
 import FormLabel from "metabase/components/form/FormLabel.jsx";
 import FormMessage from "metabase/components/form/FormMessage.jsx";
 import LogoIcon from "metabase/components/LogoIcon.jsx";
-import Settings from "metabase/lib/settings.js";
+import Settings from "metabase/lib/settings";
+import Utils from "metabase/lib/utils";
 
 
 import * as authActions from "../auth";
@@ -42,11 +43,9 @@ export default class LoginApp extends Component {
     validateForm() {
         let { credentials } = this.state;
 
-        let valid = Settings.ldapEnabled()
-            ? !!credentials.username
-            : !!credentials.email;
+        let valid = true;
 
-        if (!credentials.password) {
+        if (!credentials.username || !credentials.password) {
             valid = false;
         }
 
@@ -130,19 +129,11 @@ export default class LoginApp extends Component {
 
                             <FormMessage formError={loginError && loginError.data.message ? loginError : null} ></FormMessage>
 
-                            { Settings.ldapEnabled() ? (
-                                <FormField key="username" fieldName="username" formError={loginError}>
-                                    <FormLabel title={"Username or Email address"}  fieldName={"username"} formError={loginError} />
-                                    <input className="Form-input Form-offset full py1" name="username" placeholder="youlooknicetoday@email.com" type="text" onChange={(e) => this.onChange("username", e.target.value)} autoFocus />
-                                    <span className="Form-charm"></span>
-                                </FormField>
-                            ) : (
-                                <FormField key="email" fieldName="email" formError={loginError}>
-                                    <FormLabel title={"Email address"}  fieldName={"email"} formError={loginError} />
-                                    <input className="Form-input Form-offset full py1" name="email" placeholder="youlooknicetoday@email.com" type="text" onChange={(e) => this.onChange("email", e.target.value)} autoFocus />
-                                    <span className="Form-charm"></span>
-                                </FormField>
-                            )}
+                            <FormField key="username" fieldName="username" formError={loginError}>
+                                <FormLabel title={Settings.ldapEnabled() ? "Username or email address" : "Email address"} fieldName={"username"} formError={loginError} />
+                                <input className="Form-input Form-offset full py1" name="username" placeholder="youlooknicetoday@email.com" type="text" onChange={(e) => this.onChange("username", e.target.value)} autoFocus />
+                                <span className="Form-charm"></span>
+                            </FormField>
 
                             <FormField key="password" fieldName="password" formError={loginError}>
                                 <FormLabel title={"Password"}  fieldName={"password"} formError={loginError} />
@@ -160,9 +151,7 @@ export default class LoginApp extends Component {
                                 <button className={cx("Button Grid-cell", {'Button--primary': this.state.valid})} disabled={!this.state.valid}>
                                     Sign in
                                 </button>
-                                { (!Settings.ldapEnabled()) &&
-                                    <Link to={"/auth/forgot_password"+(this.state.credentials.email ? "?email="+this.state.credentials.email : "")} className="Grid-cell py2 sm-py0 text-grey-3 md-text-right text-centered flex-full link" onClick={(e) => { window.OSX ? window.OSX.resetPassword() : null }}>I seem to have forgotten my password</Link>
-                                }
+                                <Link to={"/auth/forgot_password"+(Utils.validEmail(this.state.credentials.username) ? "?email="+this.state.credentials.username : "")} className="Grid-cell py2 sm-py0 text-grey-3 md-text-right text-centered flex-full link" onClick={(e) => { window.OSX ? window.OSX.resetPassword() : null }}>I seem to have forgotten my password</Link>
                             </div>
                         </form>
                     </div>

frontend/src/metabase/services.js

@@ -172,7 +172,6 @@ export const LabelApi = {
 export const SessionApi = {
     create:                     POST("/api/session"),
     createWithGoogleAuth:       POST("/api/session/google_auth"),
-    createWithLdap:             POST("/api/session/ldap_auth"),
     delete:                   DELETE("/api/session"),
     properties:                  GET("/api/session/properties"),
     forgot_password:            POST("/api/session/forgot_password"),

frontend/src/metabase/user/components/UpdateUserDetails.jsx

@@ -83,7 +83,7 @@ export default class UpdateUserDetails extends Component {
     render() {
         const { updateUserResult, user } = this.props;
         const { formError, valid } = this.state;
-        const managed = user.google_auth
+        const managed = user.google_auth || user.ldap_auth;
 
         return (
             <div>
@@ -101,7 +101,7 @@ export default class UpdateUserDetails extends Component {
                     </FormField>
 
                     <FormField fieldName="email" formError={formError}>
-                        <FormLabel title={ managed ? "Sign in with Google Email address" : "Email address"} fieldName="email" formError={formError} ></FormLabel>
+                        <FormLabel title={ user.google_auth ? "Sign in with Google Email address" : "Email address"} fieldName="email" formError={formError} ></FormLabel>
                         <input
                             ref="email"
                             className={

src/metabase/api/session.clj

@@ -33,28 +33,39 @@
       :user_id (:id user))
     (events/publish-event! :user-login {:user_id (:id user), :session_id <>, :first_login (not (boolean (:last_login user)))})))
 
+(defn- ldap-fetch-or-create-user! [first-name last-name email password]
+ (if-let [user (or (db/select-one [User :id :last_login] :email email) ; TODO - Update the user's password
+                   (user/create-new-ldap-auth-user! first-name last-name email password))]
+   {:id (create-session! user)}))
 
 ;;; ## API Endpoints
 
 (def ^:private login-throttlers
-  {:email      (throttle/make-throttler :email)
-   :username   (throttle/make-throttler :username)
-   :ip-address (throttle/make-throttler :email, :attempts-threshold 50)}) ; IP Address doesn't have an actual UI field so just show error by email
+  {:username   (throttle/make-throttler :username)
+   :ip-address (throttle/make-throttler :username, :attempts-threshold 50)}) ; IP Address doesn't have an actual UI field so just show error by username
 
 (defendpoint POST "/"
   "Login."
-  [:as {{:keys [email password]} :body, remote-address :remote-addr}]
-  {email    su/Email
+  [:as {{:keys [username password]} :body, remote-address :remote-addr}]
+  {username su/NonBlankString
    password su/NonBlankString}
   (throttle/check (login-throttlers :ip-address) remote-address)
-  (throttle/check (login-throttlers :email)      email)
-  (let [user (db/select-one [User :id :password_salt :password :last_login], :email email, :is_active true)]
+  (throttle/check (login-throttlers :username)   username)
+  (or
+    ;; First try LDAP if it's enabled
+    (when (ldap/ldap-configured?)
+      (when-let [{:keys [first-name last-name email]} (ldap/auth-user username password)]
+        (ldap-fetch-or-create-user! first-name last-name email password)))
+
+    ;; Then try local authentication
+    (when-let [user (db/select-one [User :id :password_salt :password :last_login], :email username, :is_active true)]
+      (when (pass/verify-password password (:password_salt user) (:password user))
+        {:id (create-session! user)}))
+
+    ;; If both fail complain about it
     ;; Don't leak whether the account doesn't exist or the password was incorrect
-    (when-not (and user
-                   (pass/verify-password password (:password_salt user) (:password user)))
-      (throw (ex-info "Password did not match stored password." {:status-code 400
-                                                                 :errors      {:password "did not match stored password"}})))
-    {:id (create-session! user)}))
+    (throw (ex-info "Password did not match stored password." {:status-code 400
+                                                               :errors      {:password "did not match stored password"}}))))
 
 
 (defendpoint DELETE "/"
@@ -197,24 +208,4 @@
     (google-auth-fetch-or-create-user! given_name family_name email)))
 
 
-;;; ------------------------------------------------------------ LDAP AUTH ------------------------------------------------------------
-
-(defn- ldap-auth-fetch-or-create-user! [first-name last-name email]
- (if-let [user (or (db/select-one [User :id :last_login] :email email)
-                   (user/create-new-ldap-auth-user! first-name last-name email))]
-   {:id (create-session! user)}))
-
-(defendpoint POST "/ldap_auth"
-  ;; TODO - merge this with the regular POST endpoint
-  "Login with LDAP auth."
-  [:as {{:keys [username password]} :body, remote-address :remote-addr}]
-  {username su/NonBlankString
-   password su/NonBlankString}
-  (throttle/check (login-throttlers :ip-address) remote-address)
-  (throttle/check (login-throttlers :username)   username)
-  (if-let [{:keys [first-name last-name email]} (ldap/auth-user username password)]
-    (ldap-auth-fetch-or-create-user! first-name last-name email) ; TODO - we should probably "cache" the password here to do something like Hybrid LDAP in JIRA
-    (throw (ex-info "Invalid username or password." {:status-code 400}))))
-
-
 (define-routes)

src/metabase/api/user.clj

@@ -21,7 +21,7 @@
 (defendpoint GET "/"
   "Fetch a list of all active `Users` for the admin People page."
   []
-  (db/select [User :id :first_name :last_name :email :is_superuser :google_auth :last_login], :is_active true))
+  (db/select [User :id :first_name :last_name :email :is_superuser :google_auth :ldap_auth :last_login], :is_active true))
 
 (defn- reactivate-user! [existing-user first-name last-name]
   (when-not (:is_active existing-user)

src/metabase/integrations/ldap.clj

@@ -25,7 +25,7 @@
   "The password to bind with.")
 
 (defsetting ldap-base
-  "Search base for users, will be searched recursively.")
+  "Search base for users. (Will be searched recursively)")
 
 (defsetting ldap-user-filter
   "Filter to use for looking up a specific user, the placeholder {login} will be replaced by the user supplied login."
@@ -59,12 +59,12 @@
                  :ssl?      (= (ldap-security) "ssl")
                  :startTLS? (= (ldap-security) "tls")}))
 
-(defn- with-connection [f]
-  (when (ldap-configured?)) ; Step 1 in providing fallback authentication ?
-    (let [conn (ldap-connection)]
-      (try
-        (f conn)
-        (finally (ldap/close conn)))))
+(defn with-connection [f & args]
+  "Applied `f` with a connection pool followed by `args`"
+  (let [conn (ldap-connection)]
+    (try
+      (apply f conn args)
+      (finally (ldap/close conn)))))
 
 (defn escape-value [value]
   "Escapes a value for use in an LDAP filter expression."
@@ -73,7 +73,7 @@
 (defn get-user-info
   "Gets user information for the supplied username."
   ([username]
-    (with-connection #(get-user-info % username)))
+    (with-connection get-user-info username))
   ([conn username]
     (let [fname-attr (keyword (ldap-attribute-firstname))
           lname-attr (keyword (ldap-attribute-lastname))
@@ -90,7 +90,7 @@
 (defn auth-user
   "Authenticates the user with an LDAP bind operation. Returns the user information when successful, nil otherwise."
   ([username password]
-    (with-connection #(auth-user % username password)))
+    (with-connection auth-user username password))
   ([conn username password]
     ;; first figure out the user even exists, we also need the DN to reliably bind with LDAP
     (when-let [{:keys [dn], :as user} (get-user-info conn username)]

src/metabase/middleware.clj

@@ -119,7 +119,7 @@
       response-unauthentic)))
 
 (def ^:private current-user-fields
-  (vec (concat [User :is_active :google_auth] (models/default-fields User))))
+  (vec (concat [User :is_active :google_auth :ldap_auth] (models/default-fields User))))
 
 (defn bind-current-user
   "Middleware that binds `metabase.api.common/*current-user*`, `*current-user-id*`, `*is-superuser?*`, and `*current-user-permissions-set*`.

test/metabase/api/session_test.clj

@@ -22,11 +22,11 @@
       (tu/is-uuid-string? (:id (client :post 200 "session" (user->credentials :rasta))))))
 
 ;; Test for required params
-(expect {:errors {:email "value must be a valid email address."}}
+(expect {:errors {:username "value must be a non-blank string."}}
   (client :post 400 "session" {}))
 
 (expect {:errors {:password "value must be a non-blank string."}}
-  (client :post 400 "session" {:email "anything@metabase.com"}))
+  (client :post 400 "session" {:username "anything@metabase.com"}))
 
 ;; Test for inactive user (user shouldn't be able to login if :is_active = false)
 ;; Return same error as incorrect password to avoid leaking existence of user
@@ -41,9 +41,9 @@
 ;; Test that people get blocked from attempting to login if they try too many times
 ;; (Check that throttling works at the API level -- more tests in the throttle library itself: https://github.com/metabase/throttle)
 (expect
-    [{:errors {:email "Too many attempts! You must wait 15 seconds before trying again."}}
-     {:errors {:email "Too many attempts! You must wait 15 seconds before trying again."}}]
-  (let [login #(client :post 400 "session" {:email "fakeaccount3000@metabase.com", :password "toucans"})]
+    [{:errors {:username "Too many attempts! You must wait 15 seconds before trying again."}}
+     {:errors {:username "Too many attempts! You must wait 15 seconds before trying again."}}]
+  (let [login #(client :post 400 "session" {:username "fakeaccount3000@metabase.com", :password "toucans"})]
     ;; attempt to log in 10 times
     (dorun (repeatedly 10 login))
     ;; throttling should now be triggered
@@ -72,7 +72,7 @@
     (db/update! User (user->id :rasta), :reset_token nil, :reset_triggered nil)
     (assert (not (reset-fields-set?)))
     ;; issue reset request (token & timestamp should be saved)
-    ((user->client :rasta) :post 200 "session/forgot_password" {:email (:email (user->credentials :rasta))})
+    ((user->client :rasta) :post 200 "session/forgot_password" {:email (:username (user->credentials :rasta))})
     ;; TODO - how can we test email sent here?
     (reset-fields-set?)))
 
@@ -97,9 +97,9 @@
       (let [token (u/prog1 (str id "_" (java.util.UUID/randomUUID))
                     (db/update! User id, :reset_token <>))
             creds {:old {:password (:old password)
-                         :email    email}
+                         :username email}
                    :new {:password (:new password)
-                         :email    email}}]
+                         :username email}}]
         ;; Check that creds work
         (client :post 200 "session" (:old creds))
 

test/metabase/api/user_test.clj

@@ -208,7 +208,7 @@
 ;; Test that a User can change their password (superuser and non-superuser)
 (defn- user-can-reset-password? [superuser?]
   (tt/with-temp User [user {:password "def", :is_superuser (boolean superuser?)}]
-    (let [creds           {:email (:email user), :password "def"}
+    (let [creds           {:username (:email user), :password "def"}
           hashed-password (db/select-one-field :password User, :email (:email user))]
       ;; use API to reset the users password
       (http/client creds :put 200 (format "user/%d/password" (:id user)) {:password     "abc123!!DEF"
@@ -246,7 +246,7 @@
                                  :last_name  (random-name)
                                  :email      "def@metabase.com"
                                  :password   "def123"}]
-    (let [creds {:email    "def@metabase.com"
+    (let [creds {:username "def@metabase.com"
                  :password "def123"}]
       [(metabase.http-client/client creds :put 200 (format "user/%d/qbnewb" id))
        (db/select-one-field :is_qbnewb User, :id id)])))

test/metabase/http_client.clj

@@ -61,15 +61,15 @@
 (declare client)
 
 (defn authenticate
-  "Authenticate a test user with EMAIL and PASSWORD, returning their Metabase Session token;
+  "Authenticate a test user with USERNAME and PASSWORD, returning their Metabase Session token;
    or throw an Exception if that fails."
-  [{:keys [email password], :as credentials}]
-  {:pre [(string? email) (string? password)]}
-  (println "Authenticating" email) ; DEBUG
+  [{:keys [username password], :as credentials}]
+  {:pre [(string? username) (string? password)]}
+  (println "Authenticating" username) ; DEBUG
   (try
     (:id (client :post 200 "session" credentials))
     (catch Throwable e
-      (log/error "Failed to authenticate with email:" email "and password:" password ":" (.getMessage e)))))
+      (log/error "Failed to authenticate with username:" username "and password:" password ":" (.getMessage e)))))
 
 
 ;;; client
@@ -140,7 +140,7 @@
 
   Args:
 
-   *  CREDENTIALS           Optional map of `:email` and `:password` or `X-METABASE-SESSION` token of a User who we should perform the request as
+   *  CREDENTIALS           Optional map of `:username` and `:password` or `X-METABASE-SESSION` token of a User who we should perform the request as
    *  METHOD                `:get`, `:post`, `:delete`, or `:put`
    *  EXPECTED-STATUS-CODE  When passed, throw an exception if the response has a different status code.
    *  URL                   Base URL of the request, which will be appended to `*url-prefix*`. e.g. `card/1/favorite`

test/metabase/test/data/users.clj

@@ -110,12 +110,14 @@
      (:id (fetch-user username)))))
 
 (defn user->credentials
-  "Return a map with `:email` and `:password` for User with USERNAME.
+  "Return a map with `:username` and `:password` for User with USERNAME.
 
-    (user->credentials :rasta) -> {:email \"rasta@metabase.com\", :password \"blueberries\"}"
+    (user->credentials :rasta) -> {:username \"rasta@metabase.com\", :password \"blueberries\"}"
   [username]
   {:pre [(contains? usernames username)]}
-  (select-keys (user->info username) [:email :password]))
+  (let [{:keys [email password]} (user->info username)]
+    {:username email
+     :password password}))
 
 (def ^{:arglists '([id])} id->user
   "Reverse of `user->id`.