diff --git a/frontend/src/metabase/admin/settings/components/widgets/LdapGroupMappingsWidget.jsx b/frontend/src/metabase/admin/settings/components/widgets/LdapGroupMappingsWidget.jsx
index 1274f30a206e91a2c29e37612dab584ce2f95e44..35846c6e1f3529b17b50234c6690e456e47512d6 100644
--- a/frontend/src/metabase/admin/settings/components/widgets/LdapGroupMappingsWidget.jsx
+++ b/frontend/src/metabase/admin/settings/components/widgets/LdapGroupMappingsWidget.jsx
@@ -30,6 +30,7 @@ type State = {
showAddRow: boolean,
groups: Object[],
mappings: { [string]: number[] },
+ saveError: ?Object,
};
export default class LdapGroupMappingsWidget extends React.Component {
@@ -43,6 +44,7 @@ export default class LdapGroupMappingsWidget extends React.Component {
showAddRow: false,
groups: [],
mappings: {},
+ saveError: null,
};
}
@@ -117,13 +119,24 @@ export default class LdapGroupMappingsWidget extends React.Component {
SettingsApi.put({ key: "ldap-group-mappings", value: mappings }).then(
() => {
onChangeSetting("ldap-group-mappings", mappings);
- this.setState({ showEditModal: false, showAddRow: false });
+ this.setState({
+ showEditModal: false,
+ showAddRow: false,
+ saveError: null,
+ });
},
+ e => this.setState({ saveError: e }),
);
};
render() {
- const { showEditModal, showAddRow, groups, mappings } = this.state;
+ const {
+ showEditModal,
+ showAddRow,
+ groups,
+ mappings,
+ saveError,
+ } = this.state;
return (
<div className="flex align-center">
@@ -178,6 +191,11 @@ export default class LdapGroupMappingsWidget extends React.Component {
</AdminContentTable>
</div>
<ModalFooter>
+ {saveError && saveError.data && saveError.data.message ? (
+ <span className="text-error text-bold">
+ {saveError.data.message}
+ </span>
+ ) : null}
<Button
type="button"
onClick={this._cancelClick}
diff --git a/src/metabase/integrations/ldap.clj b/src/metabase/integrations/ldap.clj
index 89453b4844690de0777325b56b636e021f48a3f0..a7593b6218ccff9ab373ed1bdb673ce2f5970c98 100644
--- a/src/metabase/integrations/ldap.clj
+++ b/src/metabase/integrations/ldap.clj
@@ -1,5 +1,6 @@
(ns metabase.integrations.ldap
- (:require [clj-ldap.client :as ldap]
+ (:require [cheshire.core :as json]
+ [clj-ldap.client :as ldap]
[clojure
[set :as set]
[string :as str]]
@@ -10,7 +11,7 @@
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[toucan.db :as db])
- (:import [com.unboundid.ldap.sdk LDAPConnectionPool LDAPException]))
+ (:import [com.unboundid.ldap.sdk DN Filter LDAPConnectionPool LDAPException]))
(def ^:private filter-placeholder
"{login}")
@@ -70,10 +71,17 @@
(tru "Search base for groups, not required if your LDAP directory provides a ''memberOf'' overlay. (Will be searched recursively)"))
(defsetting ldap-group-mappings
- ;; Should be in the form: {"cn=Some Group,dc=...": [1, 2, 3]} where keys are LDAP groups and values are lists of MB groups IDs
+ ;; Should be in the form: {"cn=Some Group,dc=...": [1, 2, 3]} where keys are LDAP group DNs and values are lists of MB groups IDs
(tru "JSON containing LDAP to Metabase group mappings.")
:type :json
- :default {})
+ :default {}
+ :getter (fn []
+ (json/parse-string (setting/get-string :ldap-group-mappings) #(DN. (str %))))
+ :setter (fn [new-value]
+ (doseq [k (keys new-value)]
+ (when-not (DN/isValidDN (name k))
+ (throw (IllegalArgumentException. (str (tru "{0} is not a valid DN." (name k)))))))
+ (setting/set-json! :ldap-group-mappings new-value)))
(defn ldap-configured?
"Check if LDAP is enabled and that the mandatory settings are configured."
@@ -96,11 +104,6 @@
:password (ldap-password)
:security (ldap-security)}))
-(defn- escape-value
- "Escapes a value for use in an LDAP filter expression."
- [value]
- (str/replace value #"(?:^\s|\s$|[,\\\#\+<>;\"=\*\(\)\\0])" (comp (partial format "\\%02X") int first)))
-
(defn- get-connection
"Connects to LDAP with the currently set settings and returns the connection."
^LDAPConnectionPool []
@@ -116,23 +119,20 @@
"Will translate a set of DNs to a set of MB group IDs using the configured mappings."
[ldap-groups]
(-> (ldap-group-mappings)
- (select-keys (map keyword ldap-groups))
+ (select-keys (map #(DN. (str %)) ldap-groups))
vals
flatten
set))
(defn- get-user-groups
"Retrieve groups for a supplied DN."
- ([dn]
+ ([^String dn]
(with-connection get-user-groups dn))
- ([conn dn]
+ ([conn ^String dn]
(when (ldap-group-base)
- (let [results (ldap/search conn (ldap-group-base) {:scope :sub
- :filter (str "member=" (escape-value dn))
- :attributes [:dn :distinguishedName]})]
- (filter some?
- (for [result results]
- (or (:dn result) (:distinguishedName result))))))))
+ (let [results (ldap/search conn (ldap-group-base) {:scope :sub
+ :filter (Filter/createEqualityFilter "member" dn)})]
+ (map :dn results)))))
(def ^:private user-base-error {:status :ERROR, :message "User search base does not exist or is unreadable"})
(def ^:private group-base-error {:status :ERROR, :message "Group search base does not exist or is unreadable"})
@@ -171,22 +171,22 @@
(defn find-user
"Gets user information for the supplied username."
- ([username]
+ ([^String username]
(with-connection find-user username))
- ([conn username]
+ ([conn ^String username]
(let [fname-attr (keyword (ldap-attribute-firstname))
lname-attr (keyword (ldap-attribute-lastname))
email-attr (keyword (ldap-attribute-email))]
(when-let [[result] (ldap/search conn (ldap-user-base) {:scope :sub
- :filter (str/replace (ldap-user-filter) filter-placeholder (escape-value username))
- :attributes [:dn :distinguishedName fname-attr lname-attr email-attr :memberOf]
+ :filter (str/replace (ldap-user-filter) filter-placeholder (Filter/encodeValue username))
+ :attributes [fname-attr lname-attr email-attr :memberOf]
:size-limit 1})]
- (let [dn (or (:dn result) (:distinguishedName result))
+ (let [dn (:dn result)
fname (get result fname-attr)
lname (get result lname-attr)
email (get result email-attr)]
;; Make sure we got everything as these are all required for new accounts
- (when-not (or (empty? dn) (empty? fname) (empty? lname) (empty? email))
+ (when-not (some empty? [fname lname email])
;; ActiveDirectory (and others?) will supply a `memberOf` overlay attribute for groups
;; Otherwise we have to make the inverse query to get them
(let [groups (when (ldap-group-sync)
diff --git a/test/metabase/integrations/ldap_test.clj b/test/metabase/integrations/ldap_test.clj
index 36878abf2a1fdb5b91bb48513c38f23a3c0130c5..eae1b63d34d0c58c736016a29611e133f0456b85 100644
--- a/test/metabase/integrations/ldap_test.clj
+++ b/test/metabase/integrations/ldap_test.clj
@@ -1,7 +1,8 @@
(ns metabase.integrations.ldap-test
(:require [expectations :refer [expect]]
[metabase.integrations.ldap :as ldap]
- [metabase.test.integrations.ldap :as ldap.test]))
+ [metabase.test.integrations.ldap :as ldap.test]
+ [metabase.test.util :as tu]))
(defn- get-ldap-details []
{:host "localhost"
@@ -14,14 +15,6 @@
;; See test_resources/ldap.ldif for fixtures
-(expect
- "\\20\\2AJohn \\28Dude\\29 Doe\\5C"
- (#'ldap/escape-value " *John (Dude) Doe\\"))
-
-(expect
- "John\\2BSmith@metabase.com"
- (#'ldap/escape-value "John+Smith@metabase.com"))
-
;; The connection test should pass with valid settings
(expect
{:status :SUCCESS}
@@ -111,3 +104,11 @@
:groups ["cn=Accounting,ou=Groups,dc=metabase,dc=com"]}
(ldap.test/with-ldap-server
(ldap/find-user "John.Smith@metabase.com")))
+
+;; LDAP group matching should identify Metabase groups using DN equality rules
+(expect
+ #{1 2 3}
+ (tu/with-temporary-setting-values [ldap-group-mappings {"cn=accounting,ou=groups,dc=metabase,dc=com" [1 2]
+ "cn=shipping,ou=groups,dc=metabase,dc=com" [2 3]}]
+ (#'ldap/ldap-groups->mb-group-ids ["CN=Accounting,OU=Groups,DC=metabase,DC=com"
+ "CN=Shipping,OU=Groups,DC=metabase,DC=com"])))