Untangle LDAP circular dependency (#32314)

* untangle ldap circular dependency

* only try to require ee code when ee is available

* LDAP login e2e tests

* fix eslint errors

* Update e2e/test/scenarios/admin/settings/sso/ldap.cy.spec.js

Co-authored-by: Nemanja Glumac <31325167+nemanjaglumac@users.noreply.github.com>

* update command in e2e-ldap-helpers.js

---------

Co-authored-by: Nemanja Glumac <31325167+nemanjaglumac@users.noreply.github.com>

.github/workflows/e2e-tests.yml

@@ -125,9 +125,15 @@ jobs:
           username: ${{ secrets.DOCKERHUB_USERNAME }}
           password: ${{ secrets.DOCKERHUB_TOKEN }}
       openldap:
-        image: osixia/openldap:1.5.0
+        image: bitnami/openldap:2.6.4
         ports:
-          - "389:389"
+          - 389:389
+        env:
+          LDAP_ADMIN_PASSWORD: adminpass
+          LDAP_USERS: user01@example.org,user02@example.org
+          LDAP_PASSWORDS: 123456,123465
+          LDAP_ROOT: dc=example,dc=org
+          LDAP_PORT_NUMBER: 389
         credentials:
           username: ${{ secrets.DOCKERHUB_USERNAME }}
           password: ${{ secrets.DOCKERHUB_TOKEN }}

e2e/support/helpers/e2e-ldap-helpers.js

 /**
- * Make sure you have the ldap test server running locally:
- * `docker run -p 389:389 osixia/openldap:1.5.0`
+  Make sure you have the ldap test server running locally:
+    docker run -p 389:389 \
+      --env LDAP_ADMIN_PASSWORD=adminpass \
+      --env LDAP_USERS=user01@example.org,user02@example.org \
+      --env LDAP_PASSWORDS=123456,123465 \
+      --env LDAP_ROOT=dc=example,dc=org \
+      --env LDAP_PORT_NUMBER=389 \
+      bitnami/openldap:2.6.4
  */
 export const setupLdap = () => {
   cy.log("Set up LDAP mock server");
@@ -10,7 +16,10 @@ export const setupLdap = () => {
     "ldap-host": "localhost",
     "ldap-port": "389",
     "ldap-bind-dn": "cn=admin,dc=example,dc=org",
-    "ldap-password": "admin",
-    "ldap-user-base": "dc=example,dc=org",
+    "ldap-password": "adminpass",
+    "ldap-user-base": "ou=users,dc=example,dc=org",
+    "ldap-attribute-email": "uid",
+    "ldap-attribute-firstname": "sn",
+    "ldap-attribute-lastname": "sn",
   });
 };

e2e/test/scenarios/admin/settings/sso/ldap.cy.spec.js

@@ -2,8 +2,10 @@ import {
   modal,
   popover,
   restore,
+  describeEE,
   setupLdap,
   typeAndBlurUsingLabel,
+  setTokenFeatures,
 } from "e2e/support/helpers";
 
 import {
@@ -21,6 +23,7 @@ describe(
       cy.intercept("PUT", "/api/setting").as("updateSettings");
       cy.intercept("PUT", "/api/setting/*").as("updateSetting");
       cy.intercept("PUT", "/api/ldap/settings").as("updateLdapSettings");
+      cy.intercept("POST", "/api/dataset").as("dataset");
     });
 
     it("should setup ldap (metabase#16173)", () => {
@@ -118,6 +121,20 @@ describe(
       cy.findByText("Password").should("be.visible");
     });
 
+    it("should allow user login on OSS when LDAP is enabled", () => {
+      setupLdap();
+      cy.signOut();
+      cy.visit("/auth/login");
+      cy.findByLabelText("Username or email address").type(
+        "user01@example.org",
+      );
+      cy.findByLabelText("Password").type("123456");
+      cy.button("Sign in").click();
+      cy.findByTestId("main-navbar-root").within(() => {
+        cy.findByText("Home").should("exist");
+      });
+    });
+
     describe("Group Mappings Widget", () => {
       beforeEach(() => {
         cy.intercept("GET", "/api/setting").as("getSettings");
@@ -141,6 +158,44 @@ describe(
   },
 );
 
+describeEE("LDAP EE", { tags: "@external" }, () => {
+  beforeEach(() => {
+    restore();
+    cy.signInAsAdmin();
+    setTokenFeatures("all");
+  });
+
+  it("should allow user login on EE when LDAP is enabled", () => {
+    setupLdap();
+    cy.signOut();
+    cy.visit("/auth/login");
+    cy.findByLabelText("Username or email address").type("user01@example.org");
+    cy.findByLabelText("Password").type("123456");
+    cy.button("Sign in").click();
+    cy.findByTestId("main-navbar-root").within(() => {
+      cy.findByText("Home").should("exist");
+    });
+
+    cy.signOut();
+    cy.signInAsAdmin();
+
+    // Check that attributes are synced
+    cy.visit("/admin/people");
+    cy.get(".ContentTable").within(() => {
+      cy.findByText("Bar1 Bar1")
+        .closest("tr")
+        .within(() => {
+          cy.icon("ellipsis").click();
+        });
+    });
+    popover().within(() => {
+      cy.findByText("Edit user").click();
+    });
+    cy.findByDisplayValue("uid").should("exist");
+    cy.findByDisplayValue("homedirectory").should("exist");
+  });
+});
+
 const getLdapCard = () => {
   return cy.findByText("LDAP").parent().parent();
 };
@@ -153,6 +208,6 @@ const enterLdapSettings = () => {
   typeAndBlurUsingLabel("LDAP Host", "localhost");
   typeAndBlurUsingLabel("LDAP Port", "389");
   typeAndBlurUsingLabel("Username or DN", "cn=admin,dc=example,dc=org");
-  typeAndBlurUsingLabel("Password", "admin");
-  typeAndBlurUsingLabel("User search base", "dc=example,dc=org");
+  typeAndBlurUsingLabel("Password", "adminpass");
+  typeAndBlurUsingLabel("User search base", "ou=users,dc=example,dc=org");
 };

enterprise/backend/src/metabase_enterprise/enhancements/integrations/ldap.clj

@@ -3,7 +3,6 @@
   (:require
    [metabase.integrations.common :as integrations.common]
    [metabase.integrations.ldap.default-implementation :as default-impl]
-   [metabase.integrations.ldap.interface :as i]
    [metabase.models.interface :as mi]
    [metabase.models.setting :as setting :refer [defsetting]]
    [metabase.models.user :as user :refer [User]]
@@ -19,7 +18,7 @@
    (com.unboundid.ldap.sdk LDAPConnectionPool)))
 
 (def ^:private EEUserInfo
-  (assoc i/UserInfo :attributes (s/maybe {s/Keyword s/Any})))
+  (assoc default-impl/UserInfo :attributes (s/maybe {s/Keyword s/Any})))
 
 (defsetting ldap-sync-user-attributes
   (deferred-tru "Should we sync user attributes when someone logs in via LDAP?")
@@ -65,7 +64,7 @@
   :feature :sso
   [ldap-connection :- LDAPConnectionPool
    username        :- su/NonBlankString
-   settings        :- i/LDAPSettings]
+   settings        :- default-impl/LDAPSettings]
   (when-let [result (default-impl/search ldap-connection username settings)]
     (when-let [user-info (default-impl/ldap-search-result->user-info
                           ldap-connection
@@ -78,7 +77,7 @@
   "Using the `user-info` (from `find-user`) get the corresponding Metabase user, creating it if necessary."
   :feature :sso
   [{:keys [first-name last-name email groups attributes], :as user-info} :- EEUserInfo
-   {:keys [sync-groups?], :as settings}                                  :- i/LDAPSettings]
+   {:keys [sync-groups?], :as settings}                                  :- default-impl/LDAPSettings]
   (let [user (or (attribute-synced-user user-info)
                  (-> (user/create-new-ldap-auth-user! {:first_name       first-name
                                                        :last_name        last-name

src/metabase/integrations/ldap.clj

@@ -2,8 +2,8 @@
   (:require
    [cheshire.core :as json]
    [clj-ldap.client :as ldap]
+   [metabase.config :as config]
    [metabase.integrations.ldap.default-implementation :as default-impl]
-   [metabase.integrations.ldap.interface :as i]
    [metabase.models.interface :as mi]
    [metabase.models.setting :as setting :refer [defsetting]]
    [metabase.models.user :refer [User]]
@@ -19,7 +19,8 @@
 
 ;; Load the EE namespace up front so that the extra Settings it defines are available immediately.
 ;; Otherwise, this would only happen the first time `find-user` or `fetch-or-create-user!` is called.
-(u/ignore-exceptions (classloader/require ['metabase-enterprise.enhancements.integrations.ldap]))
+(when config/ee-available?
+ (classloader/require 'metabase-enterprise.enhancements.integrations.ldap))
 
 (defsetting ldap-host
   (deferred-tru "Server hostname."))
@@ -224,7 +225,7 @@
    :group-base           (ldap-group-base)
    :group-mappings       (ldap-group-mappings)})
 
-(s/defn find-user :- (s/maybe i/UserInfo)
+(s/defn find-user :- (s/maybe default-impl/UserInfo)
   "Get user information for the supplied username."
   ([username :- su/NonBlankString]
    (with-ldap-connection [conn]
@@ -235,5 +236,5 @@
 
 (s/defn fetch-or-create-user! :- (mi/InstanceOf User)
   "Using the `user-info` (from [[find-user]]) get the corresponding Metabase user, creating it if necessary."
-  [user-info :- i/UserInfo]
+  [user-info :- default-impl/UserInfo]
   (default-impl/fetch-or-create-user! user-info (ldap-settings)))

src/metabase/integrations/ldap/default_implementation.clj

@@ -4,7 +4,6 @@
    [clj-ldap.client :as ldap]
    [clojure.string :as str]
    [metabase.integrations.common :as integrations.common]
-   [metabase.integrations.ldap.interface :as i]
    [metabase.models.interface :as mi]
    [metabase.models.user :as user :refer [User]]
    [metabase.public-settings.premium-features
@@ -18,6 +17,30 @@
 
 (set! *warn-on-reflection* true)
 
+(def UserInfo
+  "Schema for LDAP User info as returned by `user-info` and used as input to `fetch-or-create-user!`."
+  {:dn         su/NonBlankString
+   :first-name (s/maybe su/NonBlankString)
+   :last-name  (s/maybe su/NonBlankString)
+   :email      su/Email
+   :groups     [su/NonBlankString]
+   s/Keyword   s/Any})
+
+(def LDAPSettings
+  "Options passed to LDAP integration implementations. These are just the various LDAP Settings from
+  `metabase.integrations.ldap`, packaged up as a single map so implementations don't need to fetch Setting values
+  directly."
+  {:first-name-attribute su/NonBlankString
+   :last-name-attribute  su/NonBlankString
+   :email-attribute      su/NonBlankString
+   :sync-groups?         s/Bool
+   :user-base            su/NonBlankString
+   :user-filter          su/NonBlankString
+   :group-base           (s/maybe su/NonBlankString)
+   :group-mappings       (s/maybe {DN [su/IntGreaterThanZero]})
+   s/Keyword             s/Any})
+
+
 ;;; --------------------------------------------------- find-user ----------------------------------------------------
 
 (def ^:private filter-placeholder
@@ -30,7 +53,7 @@
   "Search for a LDAP user with `username`."
   [ldap-connection                 :- LDAPConnectionPool
    username                        :- su/NonBlankString
-   {:keys [user-base user-filter]} :- i/LDAPSettings]
+   {:keys [user-base user-filter]} :- LDAPSettings]
   (some-> (first
            (ldap/search
             ldap-connection
@@ -55,7 +78,7 @@
   [ldap-connection         :- LDAPConnectionPool
    dn                      :- su/NonBlankString
    uid                     :- (s/maybe su/NonBlankString)
-   {:keys [group-base]}    :- i/LDAPSettings
+   {:keys [group-base]}    :- LDAPSettings
    group-membership-filter :- su/NonBlankString]
   (when group-base
     (let [results (ldap/search
@@ -65,7 +88,7 @@
                     :filter (process-group-membership-filter group-membership-filter dn uid)})]
       (map :dn results))))
 
-(s/defn ldap-search-result->user-info :- (s/maybe i/UserInfo)
+(s/defn ldap-search-result->user-info :- (s/maybe UserInfo)
   "Convert the result "
   [ldap-connection               :- LDAPConnectionPool
    {:keys [dn uid], :as result}  :- su/Map
@@ -73,7 +96,7 @@
            last-name-attribute
            email-attribute
            sync-groups?]
-    :as   settings}              :- i/LDAPSettings
+    :as   settings}              :- LDAPSettings
    group-membership-filter       :- su/NonBlankString]
   (let [{first-name (keyword first-name-attribute)
          last-name  (keyword last-name-attribute)
@@ -89,12 +112,12 @@
                        (user-groups ldap-connection dn uid settings group-membership-filter)
                        []))}))
 
-(defenterprise-schema find-user :- (s/maybe i/UserInfo)
+(defenterprise-schema find-user :- (s/maybe UserInfo)
   "Get user information for the supplied username."
   metabase-enterprise.enhancements.integrations.ldap
   [ldap-connection :- LDAPConnectionPool
    username        :- su/NonBlankString
-   settings        :- i/LDAPSettings]
+   settings        :- LDAPSettings]
   (when-let [result (search ldap-connection username settings)]
     (ldap-search-result->user-info ldap-connection result settings group-membership-filter)))
 
@@ -104,7 +127,7 @@
 (s/defn ldap-groups->mb-group-ids :- #{su/IntGreaterThanZero}
   "Translate a set of a user's group DNs to a set of MB group IDs using the configured mappings."
   [ldap-groups              :- (s/maybe [su/NonBlankString])
-   {:keys [group-mappings]} :- (select-keys i/LDAPSettings [:group-mappings s/Keyword])]
+   {:keys [group-mappings]} :- (select-keys LDAPSettings [:group-mappings s/Keyword])]
   (-> group-mappings
       (select-keys (map #(DN. (str %)) ldap-groups))
       vals
@@ -113,7 +136,7 @@
 
 (s/defn all-mapped-group-ids :- #{su/IntGreaterThanZero}
   "Returns the set of all MB group IDs that have configured mappings."
-  [{:keys [group-mappings]} :- (select-keys i/LDAPSettings [:group-mappings s/Keyword])]
+  [{:keys [group-mappings]} :- (select-keys LDAPSettings [:group-mappings s/Keyword])]
   (-> group-mappings
       vals
       flatten
@@ -122,8 +145,8 @@
 (defenterprise-schema fetch-or-create-user! :- (mi/InstanceOf User)
   "Using the `user-info` (from `find-user`) get the corresponding Metabase user, creating it if necessary."
   metabase-enterprise.enhancements.integrations.ldap
-  [{:keys [first-name last-name email groups]} :- i/UserInfo
-   {:keys [sync-groups?], :as settings}        :- i/LDAPSettings]
+  [{:keys [first-name last-name email groups]} :- UserInfo
+   {:keys [sync-groups?], :as settings}        :- LDAPSettings]
   (let [user     (t2/select-one [User :id :last_login :first_name :last_name :is_active]
                    :%lower.email (u/lower-case-en email))
         new-user (if user

src/metabase/integrations/ldap/interface.clj

-(ns metabase.integrations.ldap.interface
-  "There are separate EE and OSS versions of the LDAP integration; this namespace defines a common protocol both
-  implementations conform to."
-  (:require
-   [metabase.plugins.classloader :as classloader]
-   [metabase.util :as u]
-   [metabase.util.schema :as su]
-   [schema.core :as s])
-  (:import
-   (com.unboundid.ldap.sdk DN)))
-
-;; Load the EE namespace up front so that the extra Settings it defines are available immediately.
-;; Otherwise, this would only happen the first time one of the functions defined using `defenterprise` is called.
-(u/ignore-exceptions (classloader/require ['metabase-enterprise.enhancements.integrations.ldap]))
-
-(def UserInfo
-  "Schema for LDAP User info as returned by `user-info` and used as input to `fetch-or-create-user!`."
-  {:dn         su/NonBlankString
-   :first-name (s/maybe su/NonBlankString)
-   :last-name  (s/maybe su/NonBlankString)
-   :email      su/Email
-   :groups     [su/NonBlankString]
-   s/Keyword   s/Any})
-
-(def LDAPSettings
-  "Options passed to LDAP integration implementations. These are just the various LDAP Settings from
-  `metabase.integrations.ldap`, packaged up as a single map so implementations don't need to fetch Setting values
-  directly."
-  {:first-name-attribute su/NonBlankString
-   :last-name-attribute  su/NonBlankString
-   :email-attribute      su/NonBlankString
-   :sync-groups?         s/Bool
-   :user-base            su/NonBlankString
-   :user-filter          su/NonBlankString
-   :group-base           (s/maybe su/NonBlankString)
-   :group-mappings       (s/maybe {DN [su/IntGreaterThanZero]})
-   s/Keyword             s/Any})