Skip to content

Commit

Permalink
feat: LDAP Improvements and E2E testing (#2199)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
cmintey authored Mar 12, 2023
1 parent 93eb2af commit 7d9be67
Show file tree
Hide file tree
Showing 8 changed files with 276 additions and 28 deletions.
20 changes: 19 additions & 1 deletion .github/workflows/partial-backend.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -60,18 +60,18 @@ Changing the webworker settings may cause unforeseen memory leak issues with Mea

### LDAP

<!-- prettier-ignore -->
| 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))`).<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_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 |
2 changes: 1 addition & 1 deletion docs/docs/overrides/api.html

Large diffs are not rendered by default.

40 changes: 34 additions & 6 deletions mealie/core/security/security.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -14,6 +15,8 @@

ALGORITHM = "HS256"

logger = root_logger.get_logger("security")


class UserLockedOut(Exception):
...
Expand Down Expand Up @@ -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]
Expand Down
4 changes: 1 addition & 3 deletions mealie/core/settings/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -145,9 +146,6 @@ def LDAP_ENABLED(self) -> bool:
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,
Expand Down
1 change: 1 addition & 0 deletions template.env
Original file line number Diff line number Diff line change
Expand Up @@ -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=""
Expand Down
192 changes: 191 additions & 1 deletion tests/integration_tests/user_tests/test_user_login.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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") == "[email protected]"
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 ["[email protected]", "[email protected]"]
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") == "[email protected]"
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") == "[email protected]"
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") == "[email protected]"
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") == "[email protected]"
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 ["[email protected]", "[email protected]"]
assert data.get("admin") is True

get_app_settings.cache_clear()
Loading

0 comments on commit 7d9be67

Please sign in to comment.