From 7d9be674322fbcb7e23836110742880505c90036 Mon Sep 17 00:00:00 2001
From: Carter <35710697+cmintey@users.noreply.github.com>
Date: Sun, 12 Mar 2023 15:36:32 -0500
Subject: [PATCH] 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 | 20 +-
.../installation/backend-config.md | 30 +--
docs/docs/overrides/api.html | 2 +-
mealie/core/security/security.py | 40 +++-
mealie/core/settings/settings.py | 4 +-
template.env | 1 +
.../user_tests/test_user_login.py | 192 +++++++++++++++++-
tests/unit_tests/test_security.py | 15 +-
8 files changed, 276 insertions(+), 28 deletions(-)
diff --git a/.github/workflows/partial-backend.yml b/.github/workflows/partial-backend.yml
index 600d0cf0f094..b4f2697bae68 100644
--- a/.github/workflows/partial-backend.yml
+++ b/.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
diff --git a/docs/docs/documentation/getting-started/installation/backend-config.md b/docs/docs/documentation/getting-started/installation/backend-config.md
index f4ba501036fb..263ba0d87914 100644
--- a/docs/docs/documentation/getting-started/installation/backend-config.md
+++ b/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
-
-| Variables | Default | Description |
-| ------------------- | :-----: | ------------------------------------------------------------------------------------------------------------------ |
-| 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_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_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_PASSWORD | None | The 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))`).
**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_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_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 |
+| Variables | Default | Description |
+| -------------------- | :-----: | ----------------------------------------------------------------------------------------------------------------------------------- |
+| 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_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_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_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 | Optional password for the bind user used in LDAP_QUERY_BIND |
+| 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_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_MAIL_ATTRIBUTE | mail | The LDAP attribute that maps to the user's email |
diff --git a/docs/docs/overrides/api.html b/docs/docs/overrides/api.html
index 85f0cd975a58..3457007ea0f1 100644
--- a/docs/docs/overrides/api.html
+++ b/docs/docs/overrides/api.html
@@ -14,7 +14,7 @@
diff --git a/mealie/core/security/security.py b/mealie/core/security/security.py
index 40276075e964..3b0823ef0913 100644
--- a/mealie/core/security/security.py
+++ b/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(
- settings.LDAP_BASE_DN,
- ldap.SCOPE_SUBTREE,
- settings.LDAP_USER_FILTER.format(
+ user_filter = ""
+ if settings.LDAP_USER_FILTER:
+ user_filter = 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]
diff --git a/mealie/core/settings/settings.py b/mealie/core/settings/settings.py
index e7bcc345ac12..37f93738caa8 100644
--- a/mealie/core/settings/settings.py
+++ b/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,
diff --git a/template.env b/template.env
index 2c346b7786d7..aad72dba3a1a 100644
--- a/template.env
+++ b/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=""
diff --git a/tests/integration_tests/user_tests/test_user_login.py b/tests/integration_tests/user_tests/test_user_login.py
index 8eadd1a1c294..b7e67309cd33 100644
--- a/tests/integration_tests/user_tests/test_user_login.py
+++ b/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()
diff --git a/tests/unit_tests/test_security.py b/tests/unit_tests/test_security.py
index f9384f6eedb2..0b43be2f37fa 100644
--- a/tests/unit_tests/test_security.py
+++ b/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()