From 0f78d333ca441f721d80ebe0e076cf44b17031c5 Mon Sep 17 00:00:00 2001 From: Carter Mintey Date: Mon, 27 Feb 2023 18:25:35 +0000 Subject: [PATCH 1/6] add option to enable starttls for ldap --- mealie/core/security/security.py | 3 +++ mealie/core/settings/settings.py | 1 + template.env | 1 + 3 files changed, 5 insertions(+) diff --git a/mealie/core/security/security.py b/mealie/core/security/security.py index 40276075e96..976e6a8ecf4 100644 --- a/mealie/core/security/security.py +++ b/mealie/core/security/security.py @@ -64,6 +64,9 @@ 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: diff --git a/mealie/core/settings/settings.py b/mealie/core/settings/settings.py index e7bcc345ac1..09d7675067e 100644 --- a/mealie/core/settings/settings.py +++ b/mealie/core/settings/settings.py @@ -130,6 +130,7 @@ def validate_smtp( 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 diff --git a/template.env b/template.env index 2c346b7786d..aad72dba3a1 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="" From 63fd52293daf0016bfca8351921f756c4552bb9d Mon Sep 17 00:00:00 2001 From: Carter Mintey Date: Mon, 27 Feb 2023 21:55:52 +0000 Subject: [PATCH 2/6] add integration test for ldap service --- .github/workflows/partial-backend.yml | 20 ++++- .../user_tests/test_user_login.py | 86 ++++++++++++++++++- tests/unit_tests/test_security.py | 6 ++ 3 files changed, 110 insertions(+), 2 deletions(-) diff --git a/.github/workflows/partial-backend.yml b/.github/workflows/partial-backend.yml index 600d0cf0f09..b4f2697bae6 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/tests/integration_tests/user_tests/test_user_login.py b/tests/integration_tests/user_tests/test_user_login.py index 8eadd1a1c29..3152d876a27 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,86 @@ 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_user_login_starttls(api_client: TestClient): + settings = get_app_settings() + settings.LDAP_ENABLE_STARTTLS = True + get_app_settings.cache_clear() + + 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_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 diff --git a/tests/unit_tests/test_security.py b/tests/unit_tests/test_security.py index f9384f6eedb..003dd21467b 100644 --- a/tests/unit_tests/test_security.py +++ b/tests/unit_tests/test_security.py @@ -77,6 +77,9 @@ def set_option(self, option, invalue): def unbind_s(self): pass + def start_tls_s(self): + pass + def setup_env(monkeypatch: MonkeyPatch): user = random_string(10) @@ -204,6 +207,9 @@ def set_option(self, option, invalue): def unbind_s(self): pass + def start_tls_s(self): + pass + def ldap_initialize_mock(url): assert url == "" return LdapConnMock() From 2fc415b75b51798a8fd6f9bf6e58d610964da314 Mon Sep 17 00:00:00 2001 From: Carter Mintey Date: Mon, 27 Feb 2023 22:02:23 +0000 Subject: [PATCH 3/6] document new, optional environment variable --- .../installation/backend-config.md | 29 ++++++++++--------- docs/docs/overrides/api.html | 2 +- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/docs/docs/documentation/getting-started/installation/backend-config.md b/docs/docs/documentation/getting-started/installation/backend-config.md index f4ba501036f..3aa26a0e651 100644 --- a/docs/docs/documentation/getting-started/installation/backend-config.md +++ b/docs/docs/documentation/getting-started/installation/backend-config.md @@ -61,17 +61,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 | 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 | diff --git a/docs/docs/overrides/api.html b/docs/docs/overrides/api.html index 85f0cd975a5..1963b3cef1d 100644 --- a/docs/docs/overrides/api.html +++ b/docs/docs/overrides/api.html @@ -14,7 +14,7 @@
From 6b5815b8992b73c593d13bc48c8e63289ab1af72 Mon Sep 17 00:00:00 2001 From: Carter Mintey Date: Wed, 8 Mar 2023 21:37:37 +0000 Subject: [PATCH 4/6] fix: support anonymous bind --- .../installation/backend-config.md | 28 +++++++++---------- docs/docs/overrides/api.html | 2 +- mealie/core/settings/settings.py | 2 -- .../user_tests/test_user_login.py | 26 +++++++++++++++++ 4 files changed, 41 insertions(+), 17 deletions(-) diff --git a/docs/docs/documentation/getting-started/installation/backend-config.md b/docs/docs/documentation/getting-started/installation/backend-config.md index 3aa26a0e651..270852b6eeb 100644 --- a/docs/docs/documentation/getting-started/installation/backend-config.md +++ b/docs/docs/documentation/getting-started/installation/backend-config.md @@ -61,18 +61,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_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 | 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 | +| 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 | 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 | +| 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 1963b3cef1d..f5086258e5a 100644 --- a/docs/docs/overrides/api.html +++ b/docs/docs/overrides/api.html @@ -14,7 +14,7 @@
diff --git a/mealie/core/settings/settings.py b/mealie/core/settings/settings.py index 09d7675067e..7766e72a4f6 100644 --- a/mealie/core/settings/settings.py +++ b/mealie/core/settings/settings.py @@ -147,8 +147,6 @@ def LDAP_ENABLED(self) -> bool: 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/tests/integration_tests/user_tests/test_user_login.py b/tests/integration_tests/user_tests/test_user_login.py index 3152d876a27..2c87e5d437e 100644 --- a/tests/integration_tests/user_tests/test_user_login.py +++ b/tests/integration_tests/user_tests/test_user_login.py @@ -114,6 +114,32 @@ def test_ldap_user_login_starttls(api_client: TestClient): 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_anonymous_bind(api_client: TestClient): + settings = get_app_settings() + settings.LDAP_QUERY_BIND = None + settings.LDAP_QUERY_PASSWORD = None + get_app_settings.cache_clear() + + 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_admin_login(api_client: TestClient): form_data = {"username": "professor", "password": "professor"} From 1a0a182d3481f2440b43aff7d4506d3a29e3717c Mon Sep 17 00:00:00 2001 From: Carter Mintey Date: Thu, 9 Mar 2023 18:21:35 +0000 Subject: [PATCH 5/6] id and mail attributes in LDAP_USER_FILTER should be implied --- .../installation/backend-config.md | 3 +- docs/docs/overrides/api.html | 2 +- mealie/core/security/security.py | 38 +++++-- mealie/core/settings/settings.py | 1 - .../user_tests/test_user_login.py | 98 +++++++++++++++++-- tests/unit_tests/test_security.py | 9 +- 6 files changed, 131 insertions(+), 20 deletions(-) diff --git a/docs/docs/documentation/getting-started/installation/backend-config.md b/docs/docs/documentation/getting-started/installation/backend-config.md index 270852b6eeb..263ba0d8791 100644 --- a/docs/docs/documentation/getting-started/installation/backend-config.md +++ b/docs/docs/documentation/getting-started/installation/backend-config.md @@ -60,7 +60,6 @@ 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 | @@ -71,7 +70,7 @@ Changing the webworker settings may cause unforeseen memory leak issues with Mea | 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 | 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_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 | diff --git a/docs/docs/overrides/api.html b/docs/docs/overrides/api.html index f5086258e5a..3457007ea0f 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 976e6a8ecf4..f82bebb36a4 100644 --- a/mealie/core/security/security.py +++ b/mealie/core/security/security.py @@ -4,6 +4,7 @@ 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 @@ ALGORITHM = "HS256" +logger = root_logger.get_logger("security") + class UserLockedOut(Exception): ... @@ -72,22 +75,45 @@ def user_from_ldap(db: AllRepositories, username: str, password: str) -> Private 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], + ) + print(user_filter) + # 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 7766e72a4f6..37f93738caa 100644 --- a/mealie/core/settings/settings.py +++ b/mealie/core/settings/settings.py @@ -146,7 +146,6 @@ def LDAP_ENABLED(self) -> bool: required = { self.LDAP_SERVER_URL, self.LDAP_BASE_DN, - self.LDAP_USER_FILTER, self.LDAP_ID_ATTRIBUTE, self.LDAP_MAIL_ATTRIBUTE, self.LDAP_NAME_ATTRIBUTE, diff --git a/tests/integration_tests/user_tests/test_user_login.py b/tests/integration_tests/user_tests/test_user_login.py index 2c87e5d437e..b7e67309cd3 100644 --- a/tests/integration_tests/user_tests/test_user_login.py +++ b/tests/integration_tests/user_tests/test_user_login.py @@ -89,11 +89,39 @@ def test_ldap_user_login_bad_password(api_client: TestClient): 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 - get_app_settings.cache_clear() form_data = {"username": "bender", "password": "bender"} response = api_client.post(api_routes.auth_token, data=form_data) @@ -113,14 +141,67 @@ def test_ldap_user_login_starttls(api_client: TestClient): 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) @@ -139,9 +220,14 @@ def test_ldap_user_login_anonymous_bind(api_client: TestClient): 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_admin_login(api_client: TestClient): +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) @@ -160,10 +246,4 @@ def test_ldap_admin_login(api_client: TestClient): 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 + get_app_settings.cache_clear() diff --git a/tests/unit_tests/test_security.py b/tests/unit_tests/test_security.py index 003dd21467b..0b43be2f37f 100644 --- a/tests/unit_tests/test_security.py +++ b/tests/unit_tests/test_security.py @@ -52,11 +52,18 @@ def search_s(self, dn, scope, filter, attrlist): 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 From fa46d864e64a69f2b48a920db9a3b3f8a0ce1233 Mon Sep 17 00:00:00 2001 From: Carter Mintey Date: Thu, 9 Mar 2023 18:28:23 +0000 Subject: [PATCH 6/6] remove print statement --- mealie/core/security/security.py | 1 - 1 file changed, 1 deletion(-) diff --git a/mealie/core/security/security.py b/mealie/core/security/security.py index f82bebb36a4..3b0823ef091 100644 --- a/mealie/core/security/security.py +++ b/mealie/core/security/security.py @@ -86,7 +86,6 @@ def user_from_ldap(db: AllRepositories, username: str, password: str) -> Private user_filter = settings.LDAP_USER_FILTER.format( id_attribute=settings.LDAP_ID_ATTRIBUTE, mail_attribute=settings.LDAP_MAIL_ATTRIBUTE, input=username ) - print(user_filter) # 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,