feat: LDAP Improvements and E2E testing (#2199)

* add option to enable starttls for ldap

* add integration test for ldap service

* document new, optional environment variable

* fix: support anonymous bind

* id and mail attributes in LDAP_USER_FILTER should be implied

* remove print statement

.github/workflows/partial-backend.yml

@@ -13,7 +13,7 @@ jobs:
     strategy:
       fail-fast: true
       matrix:
-        # Database ENV Variablse as Specified by Mealie
+        # Database ENV Variables as Specified by Mealie
         Database: [sqlite, postgres]
 
     # Services
@@ -27,6 +27,12 @@ jobs:
         options: --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 5
         ports:
           - 5432:5432
+      ldap:
+        image: rroemhild/test-openldap
+        ports:
+          - 10389:10389
+          - 10636:10636
+
     # Steps
     steps:
       - name: Check out repository
@@ -82,5 +88,17 @@ jobs:
         env:
           DB_ENGINE: ${{ matrix.Database }}
           POSTGRES_SERVER: localhost
+          LDAP_AUTH_ENABLED: True
+          LDAP_SERVER_URL: ldap://localhost:10389
+          LDAP_TLS_INSECURE: true
+          LDAP_ENABLE_STARTTLS: false
+          LDAP_BASE_DN: "ou=people,dc=planetexpress,dc=com"
+          LDAP_QUERY_BIND: "cn=admin,dc=planetexpress,dc=com"
+          LDAP_QUERY_PASSWORD: "GoodNewsEveryone"
+          LDAP_USER_FILTER: "(&(|({id_attribute}={input})({mail_attribute}={input}))(|(memberOf=cn=ship_crew,ou=people,dc=planetexpress,dc=com)(memberOf=cn=admin_staff,ou=people,dc=planetexpress,dc=com)))"
+          LDAP_ADMIN_FILTER: "memberOf=cn=admin_staff,ou=people,dc=planetexpress,dc=com"
+          LDAP_ID_ATTRIBUTE: uid
+          LDAP_NAME_ATTRIBUTE: cn
+          LDAP_MAIL_ATTRIBUTE: mail
         run: |
           make backend-test

docs/docs/documentation/getting-started/installation/backend-config.md

@@ -60,18 +60,18 @@ Changing the webworker settings may cause unforeseen memory leak issues with Mea
 
 ### LDAP
 
-<!-- prettier-ignore -->
+| Variables            | Default | Description                                                                                                                         |
-| Variables           | Default | Description                                                                                                        |
+| -------------------- | :-----: | ----------------------------------------------------------------------------------------------------------------------------------- |
-| ------------------- | :-----: | ------------------------------------------------------------------------------------------------------------------ |
+| LDAP_AUTH_ENABLED    |  False  | Authenticate via an external LDAP server in addidion to built-in Mealie auth                                                        |
-| LDAP_AUTH_ENABLED   |  False  | Authenticate via an external LDAP server in addidion to built-in Mealie auth                                       |
+| LDAP_SERVER_URL      |  None   | LDAP server URL (e.g. ldap://ldap.example.com)                                                                                      |
-| LDAP_SERVER_URL     |  None   | LDAP server URL (e.g. ldap://ldap.example.com)                                                                     |
+| LDAP_TLS_INSECURE    |  False  | Do not verify server certificate when using secure LDAP                                                                             |
-| LDAP_TLS_INSECURE   |  False  | Do not verify server certificate when using secure LDAP                                                            |
+| LDAP_TLS_CACERTFILE  |  None   | File path to Certificate Authority used to verify server certificate (e.g. `/path/to/ca.crt`)                                       |
-| LDAP_TLS_CACERTFILE |  None   | File path to Certificate Authority used to verify server certificate (e.g. `/path/to/ca.crt`)                      |
+| LDAP_ENABLE_STARTTLS |  False  | Optional. Use STARTTLS to connect to the server                                                                                     |
-| LDAP_BASE_DN        |  None   | Starting point when searching for users authentication (e.g. `CN=Users,DC=xx,DC=yy,DC=de`)                         |
+| LDAP_BASE_DN         |  None   | Starting point when searching for users authentication (e.g. `CN=Users,DC=xx,DC=yy,DC=de`)                                          |
-| LDAP_QUERY_BIND     |  None   | A bind user for LDAP search queries (e.g. `cn=admin,cn=users,dc=example,dc=com`)                                   |
+| LDAP_QUERY_BIND      |  None   | Optional bind user for LDAP search queries (e.g. `cn=admin,cn=users,dc=example,dc=com`). If `None` then anonymous bind will be used |
-| LDAP_QUERY_PASSWORD |  None   | The password for the bind user used in LDAP_QUERY_BIND                                                             |
+| LDAP_QUERY_PASSWORD  |  None   | Optional password for the bind user used in LDAP_QUERY_BIND                                                                         |
-| LDAP_USER_FILTER    |  None   |  The LDAP search filter to find users (e.g. `(&(|({id_attribute}={input})({mail_attribute}={input}))(objectClass=person))`).<br/> **Note** `id_attribute` and `mail_attribute` will be replaced with `LDAP_ID_ATTRIBUTE` and `LDAP_MAIL_ATTRIBUTE`, respectively. `input` will be replaced with either the username or email the user logs in with.                                                                                                                  |
+| LDAP_USER_FILTER     |  None   | Optional LDAP filter to narrow down eligible users (e.g. `(memberOf=cn=mealie_user,dc=example,dc=com)`)                             |
-| LDAP_ADMIN_FILTER   |  None   | Optional LDAP filter, which tells Mealie the LDAP user is an admin (e.g. `(memberOf=cn=admins,dc=example,dc=com)`) |
+| LDAP_ADMIN_FILTER    |  None   | Optional LDAP filter, which tells Mealie the LDAP user is an admin (e.g. `(memberOf=cn=admins,dc=example,dc=com)`)                  |
-| LDAP_ID_ATTRIBUTE   |   uid   | The LDAP attribute that maps to the user's id                                                                      |
+| LDAP_ID_ATTRIBUTE    |   uid   | The LDAP attribute that maps to the user's id                                                                                       |
-| LDAP_NAME_ATTRIBUTE |  name   | The LDAP attribute that maps to the user's name                                                                    |
+| LDAP_NAME_ATTRIBUTE  |  name   | The LDAP attribute that maps to the user's name                                                                                     |
-| LDAP_MAIL_ATTRIBUTE |  mail   | The LDAP attribute that maps to the user's email                                                                   |
+| LDAP_MAIL_ATTRIBUTE  |  mail   | The LDAP attribute that maps to the user's email                                                                                    |

mealie/core/security/security.py

@@ -4,6 +4,7 @@ from pathlib import Path
 
 from jose import jwt
 
+from mealie.core import root_logger
 from mealie.core.config import get_app_settings
 from mealie.core.security.hasher import get_hasher
 from mealie.db.models.users.users import AuthMethod
@@ -14,6 +15,8 @@ from mealie.services.user_services.user_service import UserService
 
 ALGORITHM = "HS256"
 
+logger = root_logger.get_logger("security")
+
 
 class UserLockedOut(Exception):
     ...
@@ -64,27 +67,52 @@ def user_from_ldap(db: AllRepositories, username: str, password: str) -> Private
         conn.set_option(ldap.OPT_X_TLS_CACERTFILE, settings.LDAP_TLS_CACERTFILE)
         conn.set_option(ldap.OPT_X_TLS_NEWCTX, 0)
 
+    if settings.LDAP_ENABLE_STARTTLS:
+        conn.start_tls_s()
+
     # Use query user for the search instead of the logged in user
     # This prevents the need for every user to have query permissions in LDAP
     try:
         conn.simple_bind_s(settings.LDAP_QUERY_BIND, settings.LDAP_QUERY_PASSWORD)
     except (ldap.INVALID_CREDENTIALS, ldap.NO_SUCH_OBJECT):
+        logger.error("[LDAP] Unable to bind to with provided user/password")
         return False
 
     # Search "username" against "cn" attribute for Linux, "sAMAccountName" attribute
     # for Windows and "mail" attribute for email addresses. The "mail" attribute is
     # required to obtain the user's DN for the LDAP_ADMIN_FILTER.
-    user_entry = conn.search_s(
+    user_filter = ""
-        settings.LDAP_BASE_DN,
+    if settings.LDAP_USER_FILTER:
-        ldap.SCOPE_SUBTREE,
+        user_filter = settings.LDAP_USER_FILTER.format(
-        settings.LDAP_USER_FILTER.format(
             id_attribute=settings.LDAP_ID_ATTRIBUTE, mail_attribute=settings.LDAP_MAIL_ATTRIBUTE, input=username
-        ),
+        )
-        [settings.LDAP_ID_ATTRIBUTE, settings.LDAP_NAME_ATTRIBUTE, settings.LDAP_MAIL_ATTRIBUTE],
+    # Don't assume the provided search filter has (|({id_attribute}={input})({mail_attribute}={input}))
+    search_filter = "(&(|({id_attribute}={input})({mail_attribute}={input})){filter})".format(
+        id_attribute=settings.LDAP_ID_ATTRIBUTE,
+        mail_attribute=settings.LDAP_MAIL_ATTRIBUTE,
+        input=username,
+        filter=user_filter,
     )
 
+    user_entry = None
+    try:
+        user_entry = conn.search_s(
+            settings.LDAP_BASE_DN,
+            ldap.SCOPE_SUBTREE,
+            search_filter,
+            [settings.LDAP_ID_ATTRIBUTE, settings.LDAP_NAME_ATTRIBUTE, settings.LDAP_MAIL_ATTRIBUTE],
+        )
+    except ldap.FILTER_ERROR:
+        logger.error("[LDAP] Bad user search filter")
+
     if not user_entry:
         conn.unbind_s()
+        logger.error("[LDAP] No user was found with the provided user filter")
+        return False
+
+    if len(user_entry) > 1:
+        conn.unbind_s()
+        logger.error("[LDAP] Multiple users found with the provided user filter")
         return False
 
     user_dn, user_attr = user_entry[0]

mealie/core/settings/settings.py

@@ -130,6 +130,7 @@ class AppSettings(BaseSettings):
     LDAP_SERVER_URL: NoneStr = None
     LDAP_TLS_INSECURE: bool = False
     LDAP_TLS_CACERTFILE: NoneStr = None
+    LDAP_ENABLE_STARTTLS: bool = False
     LDAP_BASE_DN: NoneStr = None
     LDAP_QUERY_BIND: NoneStr = None
     LDAP_QUERY_PASSWORD: NoneStr = None
@@ -145,9 +146,6 @@ class AppSettings(BaseSettings):
         required = {
             self.LDAP_SERVER_URL,
             self.LDAP_BASE_DN,
-            self.LDAP_USER_FILTER,
-            self.LDAP_QUERY_BIND,
-            self.LDAP_QUERY_PASSWORD,
             self.LDAP_ID_ATTRIBUTE,
             self.LDAP_MAIL_ATTRIBUTE,
             self.LDAP_NAME_ATTRIBUTE,

template.env

@@ -39,6 +39,7 @@ LDAP_AUTH_ENABLED=False
 # LDAP_SERVER_URL=""
 # LDAP_TLS_INSECURE=False
 # LDAP_TLS_CACERTFILE=
+# LDAP_ENABLE_STARTTLS=False
 # LDAP_BASE_DN=""
 # LDAP_QUERY_BIND=""
 # LDAP_QUERY_PASSWORD=""

tests/integration_tests/user_tests/test_user_login.py

@@ -1,6 +1,7 @@
-import json
+import os
 
 from fastapi.testclient import TestClient
+import pytest
 
 from mealie.core.config import get_app_settings
 from mealie.repos.repository_factory import AllRepositories
@@ -57,3 +58,192 @@ def test_user_lockout_after_bad_attemps(api_client: TestClient, unique_user: Tes
     user_service = UserService(database)
     user = database.users.get_one(unique_user.user_id)
     user_service.unlock_user(user)
+
+
+@pytest.mark.skipif(not os.environ.get("GITHUB_ACTIONS", False), reason="requires ldap service in github actions")
+def test_ldap_user_login(api_client: TestClient):
+    form_data = {"username": "bender", "password": "bender"}
+    response = api_client.post(api_routes.auth_token, data=form_data)
+
+    assert response.status_code == 200
+
+    data = response.json()
+    assert data is not None
+    assert data.get("access_token") is not None
+
+    response = api_client.get(api_routes.users_self, headers={"Authorization": f"Bearer {data.get('access_token')}"})
+    assert response.status_code == 200
+
+    data = response.json()
+    assert data.get("username") == "bender"
+    assert data.get("fullName") == "Bender Bending Rodríguez"
+    assert data.get("email") == "bender@planetexpress.com"
+    assert data.get("admin") is False
+
+
+@pytest.mark.skipif(not os.environ.get("GITHUB_ACTIONS", False), reason="requires ldap service in github actions")
+def test_ldap_user_login_bad_password(api_client: TestClient):
+    form_data = {"username": "bender", "password": "BAD_PASS"}
+    response = api_client.post(api_routes.auth_token, data=form_data)
+
+    assert response.status_code == 401
+
+
+@pytest.mark.skipif(not os.environ.get("GITHUB_ACTIONS", False), reason="requires ldap service in github actions")
+def test_ldap_admin_login(api_client: TestClient):
+    form_data = {"username": "professor", "password": "professor"}
+    response = api_client.post(api_routes.auth_token, data=form_data)
+
+    assert response.status_code == 200
+
+    data = response.json()
+    assert data is not None
+    assert data.get("access_token") is not None
+
+    response = api_client.get(api_routes.users_self, headers={"Authorization": f"Bearer {data.get('access_token')}"})
+    assert response.status_code == 200
+
+    data = response.json()
+    assert data.get("username") == "professor"
+    assert data.get("fullName") == "Hubert J. Farnsworth"
+    assert data.get("email") in ["professor@planetexpress.com", "hubert@planetexpress.com"]
+    assert data.get("admin") is True
+
+
+@pytest.mark.skipif(not os.environ.get("GITHUB_ACTIONS", False), reason="requires ldap service in github actions")
+def test_ldap_user_not_in_filter(api_client: TestClient):
+    form_data = {"username": "amy", "password": "amy"}
+    response = api_client.post(api_routes.auth_token, data=form_data)
+
+    assert response.status_code == 401
+
+
+@pytest.mark.skipif(not os.environ.get("GITHUB_ACTIONS", False), reason="requires ldap service in github actions")
+def test_ldap_user_login_starttls(api_client: TestClient):
+    settings = get_app_settings()
+    settings.LDAP_ENABLE_STARTTLS = True
+
+    form_data = {"username": "bender", "password": "bender"}
+    response = api_client.post(api_routes.auth_token, data=form_data)
+
+    assert response.status_code == 200
+
+    data = response.json()
+    assert data is not None
+    assert data.get("access_token") is not None
+
+    response = api_client.get(api_routes.users_self, headers={"Authorization": f"Bearer {data.get('access_token')}"})
+    assert response.status_code == 200
+
+    data = response.json()
+    assert data.get("username") == "bender"
+    assert data.get("fullName") == "Bender Bending Rodríguez"
+    assert data.get("email") == "bender@planetexpress.com"
+    assert data.get("admin") is False
+
+    get_app_settings.cache_clear()
+
+
+@pytest.mark.skipif(not os.environ.get("GITHUB_ACTIONS", False), reason="requires ldap service in github actions")
+def test_ldap_user_login_anonymous_bind(api_client: TestClient):
+    settings = get_app_settings()
+    settings.LDAP_QUERY_BIND = None
+    settings.LDAP_QUERY_PASSWORD = None
+
+    form_data = {"username": "bender", "password": "bender"}
+    response = api_client.post(api_routes.auth_token, data=form_data)
+
+    assert response.status_code == 200
+
+    data = response.json()
+    assert data is not None
+    assert data.get("access_token") is not None
+
+    response = api_client.get(api_routes.users_self, headers={"Authorization": f"Bearer {data.get('access_token')}"})
+    assert response.status_code == 200
+
+    data = response.json()
+    assert data.get("username") == "bender"
+    assert data.get("fullName") == "Bender Bending Rodríguez"
+    assert data.get("email") == "bender@planetexpress.com"
+    assert data.get("admin") is False
+
+    get_app_settings.cache_clear()
+
+
+@pytest.mark.skipif(not os.environ.get("GITHUB_ACTIONS", False), reason="requires ldap service in github actions")
+def test_ldap_user_login_no_filter(api_client: TestClient):
+    settings = get_app_settings()
+    settings.LDAP_USER_FILTER = None
+
+    form_data = {"username": "amy", "password": "amy"}
+    response = api_client.post(api_routes.auth_token, data=form_data)
+
+    assert response.status_code == 200
+
+    data = response.json()
+    assert data is not None
+    assert data.get("access_token") is not None
+
+    response = api_client.get(api_routes.users_self, headers={"Authorization": f"Bearer {data.get('access_token')}"})
+    assert response.status_code == 200
+
+    data = response.json()
+    assert data.get("username") == "amy"
+    assert data.get("fullName") == "Amy Wong"
+    assert data.get("email") == "amy@planetexpress.com"
+    assert data.get("admin") is False
+
+    get_app_settings.cache_clear()
+
+
+@pytest.mark.skipif(not os.environ.get("GITHUB_ACTIONS", False), reason="requires ldap service in github actions")
+def test_ldap_user_login_simple_filter(api_client: TestClient):
+    settings = get_app_settings()
+    settings.LDAP_USER_FILTER = "(memberOf=cn=ship_crew,ou=people,dc=planetexpress,dc=com)"
+
+    form_data = {"username": "bender", "password": "bender"}
+    response = api_client.post(api_routes.auth_token, data=form_data)
+
+    assert response.status_code == 200
+
+    data = response.json()
+    assert data is not None
+    assert data.get("access_token") is not None
+
+    response = api_client.get(api_routes.users_self, headers={"Authorization": f"Bearer {data.get('access_token')}"})
+    assert response.status_code == 200
+
+    data = response.json()
+    assert data.get("username") == "bender"
+    assert data.get("fullName") == "Bender Bending Rodríguez"
+    assert data.get("email") == "bender@planetexpress.com"
+    assert data.get("admin") is False
+
+    get_app_settings.cache_clear()
+
+
+@pytest.mark.skipif(not os.environ.get("GITHUB_ACTIONS", False), reason="requires ldap service in github actions")
+def test_ldap_user_login_complex_filter(api_client: TestClient):
+    settings = get_app_settings()
+    settings.LDAP_USER_FILTER = "(&(objectClass=inetOrgPerson)(|(memberOf=cn=ship_crew,ou=people,dc=planetexpress,dc=com)(memberOf=cn=admin_staff,ou=people,dc=planetexpress,dc=com)))"
+
+    form_data = {"username": "professor", "password": "professor"}
+    response = api_client.post(api_routes.auth_token, data=form_data)
+
+    assert response.status_code == 200
+
+    data = response.json()
+    assert data is not None
+    assert data.get("access_token") is not None
+
+    response = api_client.get(api_routes.users_self, headers={"Authorization": f"Bearer {data.get('access_token')}"})
+    assert response.status_code == 200
+
+    data = response.json()
+    assert data.get("username") == "professor"
+    assert data.get("fullName") == "Hubert J. Farnsworth"
+    assert data.get("email") in ["professor@planetexpress.com", "hubert@planetexpress.com"]
+    assert data.get("admin") is True
+
+    get_app_settings.cache_clear()

tests/unit_tests/test_security.py

@@ -52,11 +52,18 @@ class LdapConnMock:
             self.app_settings.LDAP_NAME_ATTRIBUTE,
             self.app_settings.LDAP_MAIL_ATTRIBUTE,
         ]
-        assert filter == self.app_settings.LDAP_USER_FILTER.format(
+        user_filter = self.app_settings.LDAP_USER_FILTER.format(
             id_attribute=self.app_settings.LDAP_ID_ATTRIBUTE,
             mail_attribute=self.app_settings.LDAP_MAIL_ATTRIBUTE,
             input=self.user,
         )
+        search_filter = "(&(|({id_attribute}={input})({mail_attribute}={input})){filter})".format(
+            id_attribute=self.app_settings.LDAP_ID_ATTRIBUTE,
+            mail_attribute=self.app_settings.LDAP_MAIL_ATTRIBUTE,
+            input=self.user,
+            filter=user_filter,
+        )
+        assert filter == search_filter
         assert dn == self.app_settings.LDAP_BASE_DN
         assert scope == ldap.SCOPE_SUBTREE
 
@@ -77,6 +84,9 @@ class LdapConnMock:
     def unbind_s(self):
         pass
 
+    def start_tls_s(self):
+        pass
+
 
 def setup_env(monkeypatch: MonkeyPatch):
     user = random_string(10)
@@ -204,6 +214,9 @@ def test_ldap_disabled(monkeypatch: MonkeyPatch):
         def unbind_s(self):
             pass
 
+        def start_tls_s(self):
+            pass
+
     def ldap_initialize_mock(url):
         assert url == ""
         return LdapConnMock()