Skip to content

Commit 3d5b0c3

Browse files
committed
clean up
1 parent c0e44d6 commit 3d5b0c3

File tree

9 files changed

+204
-116
lines changed

9 files changed

+204
-116
lines changed

scripts/ci/test_helm.py

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2388,7 +2388,9 @@ def get_test_suite() -> list[TestCase]:
23882388
all_of(
23892389
LDAPValidators.ldap_enabled(),
23902390
ConfigMapValidators.configmap_has_key("PHOENIX_LDAP_HOST", "ldap.corp.com"),
2391-
ConfigMapValidators.configmap_has_key("PHOENIX_LDAP_USER_SEARCH_BASE", "OU=Users,DC=corp,DC=com"),
2391+
ConfigMapValidators.configmap_has_key(
2392+
"PHOENIX_LDAP_USER_SEARCH_BASE", "OU=Users,DC=corp,DC=com"
2393+
),
23922394
),
23932395
),
23942396
TestCase(
@@ -2409,7 +2411,9 @@ def get_test_suite() -> list[TestCase]:
24092411
'--set auth.ldap.enabled=true --set auth.ldap.host=ldap.corp.com --set-string "auth.ldap.userSearchBase=OU=Users\\,DC=corp\\,DC=com" --set-string "auth.ldap.bindDn=CN=svc-phoenix\\,OU=Service Accounts\\,DC=corp\\,DC=com" --set auth.ldap.bindPassword=secret123',
24102412
all_of(
24112413
LDAPValidators.ldap_enabled(),
2412-
LDAPValidators.ldap_bind_config("CN=svc-phoenix,OU=Service Accounts,DC=corp,DC=com"),
2414+
LDAPValidators.ldap_bind_config(
2415+
"CN=svc-phoenix,OU=Service Accounts,DC=corp,DC=com"
2416+
),
24132417
LDAPValidators.ldap_bind_password_in_secret(),
24142418
),
24152419
),
@@ -2449,10 +2453,12 @@ def get_test_suite() -> list[TestCase]:
24492453
),
24502454
TestCase(
24512455
"LDAP with group role mappings",
2452-
'--set auth.ldap.enabled=true --set auth.ldap.host=ldap.corp.com --set-string "auth.ldap.userSearchBase=OU=Users\\,DC=corp\\,DC=com" --set-string "auth.ldap.groupRoleMappings=[{\\\"group_dn\\\":\\\"CN=Admins\\,DC=corp\\,DC=com\\\"\\,\\\"role\\\":\\\"ADMIN\\\"}]"',
2456+
'--set auth.ldap.enabled=true --set auth.ldap.host=ldap.corp.com --set-string "auth.ldap.userSearchBase=OU=Users\\,DC=corp\\,DC=com" --set-string "auth.ldap.groupRoleMappings=[{\\"group_dn\\":\\"CN=Admins\\,DC=corp\\,DC=com\\"\\,\\"role\\":\\"ADMIN\\"}]"',
24532457
all_of(
24542458
LDAPValidators.ldap_enabled(),
2455-
LDAPValidators.ldap_group_role_mappings('[{"group_dn":"CN=Admins,DC=corp,DC=com","role":"ADMIN"}]'),
2459+
LDAPValidators.ldap_group_role_mappings(
2460+
'[{"group_dn":"CN=Admins,DC=corp,DC=com","role":"ADMIN"}]'
2461+
),
24562462
),
24572463
),
24582464
TestCase(
@@ -2510,7 +2516,9 @@ def get_test_suite() -> list[TestCase]:
25102516
'--set auth.ldap.enabled=true --set-string "auth.ldap.host=dc1.corp.com\\,dc2.corp.com\\,dc3.corp.com" --set-string "auth.ldap.userSearchBase=OU=Users\\,DC=corp\\,DC=com"',
25112517
all_of(
25122518
LDAPValidators.ldap_enabled(),
2513-
ConfigMapValidators.configmap_has_key("PHOENIX_LDAP_HOST", "dc1.corp.com,dc2.corp.com,dc3.corp.com"),
2519+
ConfigMapValidators.configmap_has_key(
2520+
"PHOENIX_LDAP_HOST", "dc1.corp.com,dc2.corp.com,dc3.corp.com"
2521+
),
25142522
),
25152523
),
25162524
# Ingress

src/phoenix/config.py

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
from __future__ import annotations
22

3+
import json
34
import logging
45
import os
56
import re
@@ -1588,11 +1589,6 @@ def from_env(cls) -> Optional["LDAPConfig"]:
15881589
ValueError: If configuration is invalid
15891590
json.JSONDecodeError: If GROUP_ROLE_MAPPINGS is not valid JSON
15901591
"""
1591-
import json
1592-
import logging
1593-
1594-
logger = logging.getLogger(__name__)
1595-
15961592
host = getenv(ENV_PHOENIX_LDAP_HOST)
15971593
if not host:
15981594
return None
@@ -1639,6 +1635,16 @@ def from_env(cls) -> Optional["LDAPConfig"]:
16391635
f"Got: '{mapping['role']}'"
16401636
)
16411637

1638+
# Require at least one role mapping to prevent silent authentication failures
1639+
# Without mappings, all LDAP users would be denied access with only a debug log,
1640+
# which is confusing for operators. Fail fast at startup with clear guidance.
1641+
if not group_role_mappings_list:
1642+
raise ValueError(
1643+
f"{ENV_PHOENIX_LDAP_GROUP_ROLE_MAPPINGS} must contain at least one mapping. "
1644+
f'Example: \'[{{"group_dn": "*", "role": "MEMBER"}}]\' '
1645+
f"(wildcard '*' grants MEMBER role to all authenticated LDAP users)"
1646+
)
1647+
16421648
# Validate TLS mode
16431649
tls_mode_str = getenv(ENV_PHOENIX_LDAP_TLS_MODE, "starttls").lower()
16441650
if tls_mode_str not in ("starttls", "ldaps"):
@@ -1709,8 +1715,6 @@ def from_env(cls) -> Optional["LDAPConfig"]:
17091715
)
17101716

17111717
# Validate file paths exist
1712-
import os
1713-
17141718
for env_var, file_path in [
17151719
(ENV_PHOENIX_LDAP_TLS_CA_CERT_FILE, tls_ca_cert_file),
17161720
(ENV_PHOENIX_LDAP_TLS_CLIENT_CERT_FILE, tls_client_cert_file),
@@ -1724,6 +1728,14 @@ def from_env(cls) -> Optional["LDAPConfig"]:
17241728
attr_display_name = getenv(ENV_PHOENIX_LDAP_ATTR_DISPLAY_NAME, "displayName")
17251729
attr_unique_id = getenv(ENV_PHOENIX_LDAP_ATTR_UNIQUE_ID)
17261730

1731+
# Validate required attribute is not empty
1732+
# (getenv returns "" if explicitly set to empty string, not the default)
1733+
if not attr_email:
1734+
raise ValueError(
1735+
f"{ENV_PHOENIX_LDAP_ATTR_EMAIL} cannot be empty. "
1736+
f"This attribute is required to identify users. Default: 'mail'"
1737+
)
1738+
17271739
# Validate attribute names don't contain spaces
17281740
# LDAP attribute names (e.g., objectGUID, entryUUID, mail) never contain spaces.
17291741
# A space in the config is likely a typo that would cause silent failures.

src/phoenix/db/models.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1602,7 +1602,7 @@ def LDAPUser(
16021602
*,
16031603
email: str,
16041604
username: str,
1605-
ldap_dn: str | None = None,
1605+
unique_id: str | None = None,
16061606
user_role_id: int | None = None,
16071607
) -> OAuth2User:
16081608
"""Factory function to create an LDAP user stored as OAuth2User.
@@ -1615,7 +1615,7 @@ def LDAPUser(
16151615
Args:
16161616
email: User's email address
16171617
username: User's display name
1618-
ldap_dn: User's LDAP Distinguished Name (stored in oauth2_user_id)
1618+
unique_id: User's LDAP unique ID (stored in oauth2_user_id)
16191619
user_role_id: Phoenix role ID (ADMIN, MEMBER, VIEWER)
16201620
16211621
Returns:
@@ -1627,7 +1627,7 @@ def LDAPUser(
16271627
email=email,
16281628
username=username,
16291629
oauth2_client_id=LDAP_CLIENT_ID_MARKER,
1630-
oauth2_user_id=ldap_dn,
1630+
oauth2_user_id=unique_id,
16311631
user_role_id=user_role_id,
16321632
)
16331633

src/phoenix/server/api/routers/auth.py

Lines changed: 80 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -57,24 +57,55 @@
5757
partition_seconds=60,
5858
active_partitions=2,
5959
)
60-
login_rate_limiter = fastapi_ip_rate_limiter(
61-
rate_limiter,
62-
paths=[
60+
61+
62+
def create_auth_router(ldap_enabled: bool = False) -> APIRouter:
63+
"""Create auth router with all authentication endpoints.
64+
65+
Creates a fresh router instance each time to avoid global state issues
66+
(e.g., route accumulation in tests).
67+
68+
Security: Only registers the /ldap/login endpoint when LDAP is actually configured.
69+
This prevents information disclosure and reduces attack surface.
70+
71+
Args:
72+
ldap_enabled: Whether LDAP authentication is configured
73+
74+
Returns:
75+
APIRouter: Authentication router with all endpoints registered
76+
"""
77+
# Build rate limiter paths based on configuration
78+
rate_limited_paths = [
6379
"/auth/login",
64-
"/auth/ldap/login",
6580
"/auth/logout",
6681
"/auth/refresh",
6782
"/auth/password-reset-email",
6883
"/auth/password-reset",
69-
],
70-
)
84+
]
85+
if ldap_enabled:
86+
rate_limited_paths.append("/auth/ldap/login")
87+
88+
login_rate_limiter = fastapi_ip_rate_limiter(rate_limiter, paths=rate_limited_paths)
89+
auth_dependencies = [Depends(login_rate_limiter)] if not get_env_disable_rate_limit() else []
90+
91+
router = APIRouter(prefix="/auth", include_in_schema=False, dependencies=auth_dependencies)
92+
93+
# Register all authentication endpoints
94+
router.add_api_route("/login", _login, methods=["POST"])
95+
router.add_api_route("/logout", _logout, methods=["GET"])
96+
router.add_api_route("/refresh", _refresh_tokens, methods=["POST"])
97+
router.add_api_route("/password-reset-email", _initiate_password_reset, methods=["POST"])
98+
router.add_api_route("/password-reset", _reset_password, methods=["POST"])
99+
100+
# Conditionally add LDAP endpoint only if configured
101+
if ldap_enabled:
102+
router.add_api_route("/ldap/login", _ldap_login, methods=["POST"])
71103

72-
auth_dependencies = [Depends(login_rate_limiter)] if not get_env_disable_rate_limit() else []
73-
router = APIRouter(prefix="/auth", include_in_schema=False, dependencies=auth_dependencies)
104+
return router
74105

75106

76-
@router.post("/login")
77-
async def login(request: Request) -> Response:
107+
async def _login(request: Request) -> Response:
108+
"""Authenticate user via email/password and return access/refresh tokens."""
78109
if get_env_disable_basic_auth():
79110
raise HTTPException(status_code=403)
80111
data = await request.json()
@@ -110,10 +141,8 @@ async def login(request: Request) -> Response:
110141
return await _create_auth_response(request, user)
111142

112143

113-
@router.get("/logout")
114-
async def logout(
115-
request: Request,
116-
) -> Response:
144+
async def _logout(request: Request) -> Response:
145+
"""Log out user by revoking tokens and clearing cookies."""
117146
token_store: TokenStore = request.app.state.get_token_store()
118147
user_id = None
119148
if isinstance(user := request.user, PhoenixUser):
@@ -138,8 +167,8 @@ async def logout(
138167
return response
139168

140169

141-
@router.post("/refresh")
142-
async def refresh_tokens(request: Request) -> Response:
170+
async def _refresh_tokens(request: Request) -> Response:
171+
"""Refresh access and refresh tokens."""
143172
if (refresh_token := request.cookies.get(PHOENIX_REFRESH_TOKEN_COOKIE_NAME)) is None:
144173
raise HTTPException(status_code=401, detail="Missing refresh token")
145174
token_store: TokenStore = request.app.state.get_token_store()
@@ -176,8 +205,8 @@ async def refresh_tokens(request: Request) -> Response:
176205
return await _create_auth_response(request, user)
177206

178207

179-
@router.post("/password-reset-email")
180-
async def initiate_password_reset(request: Request) -> Response:
208+
async def _initiate_password_reset(request: Request) -> Response:
209+
"""Send password reset email to user."""
181210
if get_env_disable_basic_auth():
182211
raise HTTPException(status_code=403)
183212
data = await request.json()
@@ -220,8 +249,8 @@ async def initiate_password_reset(request: Request) -> Response:
220249
return Response(status_code=204)
221250

222251

223-
@router.post("/password-reset")
224-
async def reset_password(request: Request) -> Response:
252+
async def _reset_password(request: Request) -> Response:
253+
"""Reset user password using a valid reset token."""
225254
if get_env_disable_basic_auth():
226255
raise HTTPException(status_code=403)
227256
data = await request.json()
@@ -258,6 +287,37 @@ async def reset_password(request: Request) -> Response:
258287
return response
259288

260289

290+
async def _ldap_login(request: Request) -> Response:
291+
"""Authenticate user via LDAP and return access/refresh tokens."""
292+
# Use cached authenticator instance to avoid re-parsing TLS config on every request
293+
authenticator: LDAPAuthenticator | None = getattr(request.app.state, "ldap_authenticator", None)
294+
295+
if not authenticator:
296+
raise HTTPException(
297+
status_code=503, detail="LDAP authentication is not configured on this server"
298+
)
299+
300+
data = await request.json()
301+
username = data.get("username")
302+
password = data.get("password")
303+
304+
if not username or not password:
305+
raise HTTPException(status_code=401, detail="Username and password required")
306+
307+
# Authenticate against LDAP (reused authenticator, already parsed TLS config)
308+
user_info = await authenticator.authenticate(username, password)
309+
310+
if not user_info:
311+
# Generic error message to prevent username enumeration
312+
raise HTTPException(status_code=401, detail="Invalid username and/or password")
313+
314+
# Get or create user in Phoenix database
315+
async with request.app.state.db() as session:
316+
user = await get_or_create_ldap_user(session, user_info, authenticator.config)
317+
318+
return await _create_auth_response(request, user)
319+
320+
261321
async def _create_auth_response(request: Request, user: models.User) -> Response:
262322
"""
263323
Creates access and refresh tokens for the user and sets them as cookies in the response.
@@ -300,54 +360,3 @@ async def _create_auth_response(request: Request, user: models.User) -> Response
300360
status_code=401,
301361
detail="Invalid token",
302362
)
303-
304-
305-
def create_auth_router(ldap_enabled: bool = False) -> APIRouter:
306-
"""Create auth router with conditional LDAP endpoint registration.
307-
308-
Security: Only registers the /ldap/login endpoint when LDAP is actually configured.
309-
This prevents information disclosure and reduces attack surface.
310-
311-
Args:
312-
ldap_enabled: Whether LDAP authentication is configured
313-
314-
Returns:
315-
APIRouter: Authentication router (with or without LDAP endpoint)
316-
"""
317-
# router already has all non-LDAP endpoints registered via decorators
318-
# Conditionally add LDAP endpoint only if configured
319-
if ldap_enabled:
320-
router.add_api_route("/ldap/login", ldap_login, methods=["POST"])
321-
322-
return router
323-
324-
325-
async def ldap_login(request: Request) -> Response:
326-
"""Authenticate user via LDAP and return access/refresh tokens."""
327-
# Use cached authenticator instance to avoid re-parsing TLS config on every request
328-
authenticator: LDAPAuthenticator | None = getattr(request.app.state, "ldap_authenticator", None)
329-
330-
if not authenticator:
331-
raise HTTPException(
332-
status_code=503, detail="LDAP authentication is not configured on this server"
333-
)
334-
335-
data = await request.json()
336-
username = data.get("username")
337-
password = data.get("password")
338-
339-
if not username or not password:
340-
raise HTTPException(status_code=401, detail="Username and password required")
341-
342-
# Authenticate against LDAP (reused authenticator, already parsed TLS config)
343-
user_info = await authenticator.authenticate(username, password)
344-
345-
if not user_info:
346-
# Generic error message to prevent username enumeration
347-
raise HTTPException(status_code=401, detail="Invalid username and/or password")
348-
349-
# Get or create user in Phoenix database
350-
async with request.app.state.db() as session:
351-
user = await get_or_create_ldap_user(session, user_info, authenticator.config)
352-
353-
return await _create_auth_response(request, user)

0 commit comments

Comments
 (0)