feat(auth): Add LDAP authentication support#1621
Open
adamoutler wants to merge 3 commits intonetalertx:mainfrom
Open
feat(auth): Add LDAP authentication support#1621adamoutler wants to merge 3 commits intonetalertx:mainfrom
adamoutler wants to merge 3 commits intonetalertx:mainfrom
Conversation
* feat: add LDAP login feature (7-step sprint)" Steps completed: - DB: ensure_Users table migration - server/auth/: base, local_provider, ldap_provider, manager modules - POST /api/auth/login endpoint + LoginRequest/LoginResponse schemas - front/plugins/auth_ldap/config.json LDAP plugin - front/index.php: LDAP username field + PHP-to-API auth path - en_us.json: Login_Username key - requirements.txt: add ldap3 - test/backend/test_auth_providers.py: unit tests - test/api_endpoints/test_auth_endpoints.py: login endpoint tests Agent-Logs-Url: https://github.com/adamoutler/NetAlertX/sessions/9ae46385-4752-4a5f-a7f5-00d1efaf8ece Co-authored-by: adamoutler <527919+adamoutler@users.noreply.github.com> * fix: address code review and CodeQL findings" - Remove duplicate 'Login Attempt' comment block in index.php - Rename misleading $ldap_port_line to $ldap_enabled_line - Add nosec B324 + explanatory comment to SHA-256 usage in local_provider.py (legacy comparison, not new hash storage) Agent-Logs-Url: https://github.com/adamoutler/NetAlertX/sessions/9ae46385-4752-4a5f-a7f5-00d1efaf8ece Co-authored-by: adamoutler <527919+adamoutler@users.noreply.github.com> * Enhance LDAP: fallback to local admin, add UI test endpoint, fix login bugs, add brute-force delay * Security Patch: Fix StartTLS bind sequence, add TLS validation, and improve diagnostic tooling * Security & Feature Update: Address LDAP vulnerabilities and add idle notification - Implemented IP-based rate limiting to prevent DoS attacks. - Added direct bind support to LDAP configuration. - Fixed fallback logic in AuthManager to prevent downgrade attacks. - Added a security warning for disabled TLS verification. - Added client IP to authentication logs for Fail2Ban compatibility. - Implemented a notification when all system processes become idle. * feat(auth): Add comprehensive environment variable support for LDAP configuration - Parse environment variables for all LDAP options in `ldap_provider.py` - Prepend environment checks for `LDAP_ENABLED` in PHP frontend (`index.php` and `security.php`) to immediately respect Docker overrides - Update `docker-compose.yml` and `.env` templates with all available LDAP options - Add dedicated documentation in `docs/LDAP_CONFIGURATION.md` detailing OpenLDAP and Active Directory examples - Add unit tests for environment-based configuration priority - Remove stale log and report artifacts from the repository * fix(auth): Address code review feedback - Normalize template variable casing - Change default docker-compose networking to to support ARP scanning - Ensure uses trusted proxy limits - Update LDAP connection logic to fail-closed on failure - Make Local fallback reject empty usernames - Restrict local fallback on LDAP miss to admin - Throttled Idle notifications to once per hour - Use for executing custom plugins - Ensure UI tests fail rather than skip when LDAP fields are absent - Fix unit tests' patching mechanisms for AuthManager * fix(auth): Address final CodeRabbit review comments - Remove stray ticket_idle_notification.md file - Update .env template to use CHANGE_ME for LDAP bind password and remove extra blank line - Seed was_idle to True in main.py to avoid immediate startup idle notification - Fix markdown language tags in LDAP_CONFIGURATION.md - Expect 405 Method Not Allowed in test_login_endpoint_requires_post - Clean up f-strings and inline returns in test_auth_providers.py - Use 'is' instead of '==' for bool type checking in ldap_provider.py - Add default to usrCreatedAt and constraint to usrRole in db_upgrade.py - Type LoginResponse.provider as Literal['local', 'ldap'] - Pin ldap3>=2.9 in requirements.txt - Remove shell quotes from LDAP default test command config * test(auth): Add CI support for LDAP testing - Adds an explicitly togglable flag to the comprehensive GitHub workflow - Installs the dependency automatically into the local test environments via * fix(auth): Remove f-string prefix in logger and format inline returns * test(auth): Mock helper.mylog to prevent trailing background threads --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: adamoutler <527919+adamoutler@users.noreply.github.com> Co-authored-by: Adam Outler <adamoutler@gmail.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a pluggable authentication system (LDAP + local), a new POST /api/auth/login endpoint with rate limiting, frontend CSRF/HMAC remember-me changes, Users table creation, LDAP plugin/config/docs/tests, plugin test command execution, Docker/CI adjustments, and multiple unit/integration/UI tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Frontend as index.php
participant APIServer as /api/auth/login
participant AuthMgr as AuthManager
participant LDAP as LDAP Server
participant LocalDB as Local auth (SETPWD)
User->>Browser: enter credentials + CSRF
Browser->>Frontend: POST login form
Frontend->>Frontend: validate CSRF
Frontend->>APIServer: POST /api/auth/login {username,password}
APIServer->>APIServer: check per-IP rate limit
APIServer->>AuthMgr: authenticate(username,password)
alt LDAP enabled
AuthMgr->>LDAP: service bind -> search (or direct bind)
LDAP-->>AuthMgr: success / user DN
AuthMgr->>LDAP: bind as user (check password)
LDAP-->>AuthMgr: credential OK
AuthMgr-->>APIServer: return success (provider: ldap)
else LDAP user not found and fallback allowed
AuthMgr->>LocalDB: verify local hash for admin fallback
LocalDB-->>AuthMgr: success/failure
AuthMgr-->>APIServer: return result (provider: local or fail)
else LDAP error or explicit reject
AuthMgr-->>APIServer: return failure
end
APIServer-->>Frontend: JSON response
Frontend->>Browser: set HMAC remember cookie / redirect on success
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (6)
test/ui/test_ui_ldap_login.py (1)
10-12: Use an explicit post-login wait instead of a fixed sleep.
time.sleep(2)is CI-flaky, and the current assertion can pass on any non-index error page. Wait for a known authenticated route/state.♻️ Proposed refactor
-import time import pytest from selenium.webdriver.common.by import By +from selenium.webdriver.support.ui import WebDriverWait @@ - # Wait for page to respond to form submission + # Wait for page to respond to form submission print("Waiting for login to complete...") - time.sleep(2) - wait_for_page_load(driver, timeout=5) + WebDriverWait(driver, 10).until( + lambda d: "/devices.php" in d.current_url or not d.find_elements(By.NAME, "loginpassword") + ) @@ - assert '/devices.php' in driver.current_url or '/index.php' not in driver.current_url, \ + assert '/devices.php' in driver.current_url, \ f"Expected redirect after successful LDAP login, got {driver.current_url}"Also applies to: 39-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/test_ui_ldap_login.py` around lines 10 - 12, Replace the flaky time.sleep(2) calls in test_ui_ldap_login.py with an explicit Selenium wait for a known authenticated state (e.g., wait for the post-login URL or a dashboard element) using WebDriverWait and expected_conditions; locate the occurrences by searching for time.sleep(...) and the post-login assertion in this file and change them to WebDriverWait(driver, timeout).until(EC.visibility_of_element_located((By.<SOME_LOCATOR>, "<locator>"))) or until URL contains the authenticated route to ensure the test only proceeds when the app is truly logged in.front/plugins/auth_ldap/test_ldap.py (1)
47-78: Initializeexit_codeoutside the conditional for robustness.
exit_codeis only assigned inside theif args.test_user:branch (line 62), but is referenced unconditionally on line 73. Today this works because theandshort-circuits onargs.test_userfirst, but a later refactor that reorders or splits that condition will trip aNameError. Initializing it up front removes the foot-gun:Proposed diff
print(f"[LDAP Test] Base DN: {cfg['base_dn']}") print(f"[LDAP Test] User Filter: {cfg['user_filter']}") - + + exit_code = 0 if args.test_user: from auth.ldap_provider import _escape_ldap_filter safe_username = _escape_ldap_filter(args.test_user) search_filter = cfg["user_filter"].replace("{username}", safe_username) print(f"[LDAP Test] Searching for user '{args.test_user}' with filter: {search_filter}") conn.search( search_base=cfg["base_dn"], search_filter=search_filter, search_scope=ldap3.SUBTREE, attributes=[cfg["username_attr"]], size_limit=2, ) entries = conn.entries - exit_code = 0 if len(entries) == 1: print(f"[LDAP Test] ✅ SUCCESS: Found user DN: {entries[0].entry_dn}") elif len(entries) == 0: print("[LDAP Test] ❌ ERROR: User not found") exit_code = 1 else: print(f"[LDAP Test] ❌ ERROR: Found multiple ({len(entries)}) entries for user") exit_code = 1 conn.unbind() - if args.test_user and exit_code: + if exit_code: sys.exit(exit_code)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/plugins/auth_ldap/test_ldap.py` around lines 47 - 78, Initialize exit_code before the conditional that checks args.test_user to avoid a potential NameError later; specifically, add exit_code = 0 prior to the "if args.test_user:" block in test_ldap.py so that the later conditional "if args.test_user and exit_code:" (and any future refactors) can safely reference exit_code, while leaving the existing logic in the args.test_user branch (setting exit_code = 1 on error) and the conn.unbind()/sys.exit behavior unchanged.test/backend/test_auth_providers.py (2)
149-149: Lint nits flagged by Ruff.
- Line 149:
f"uid=alice,dc=example,dc=com"has no placeholders — drop thefprefix (Ruff F541).- Lines 277-278: two statements on one line via
if … : return …(Ruff E701) — split onto separate lines for readability.Proposed diff
- fake_entries = [MagicMock(entry_dn=f"uid=alice,dc=example,dc=com") for _ in range(search_entries)] + fake_entries = [MagicMock(entry_dn="uid=alice,dc=example,dc=com") for _ in range(search_entries)] @@ def mock_settings(key): - if key == "SETPWD_enable_password": return True + if key == "SETPWD_enable_password": + return True return FalseAlso applies to: 277-278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/backend/test_auth_providers.py` at line 149, Remove the unnecessary f-string in the fake_entries list comprehension (the literal "uid=alice,dc=example,dc=com" should not use an f-prefix) and split any condensed single-line if-return statements around the test functions at the block containing lines like `if ...: return ...` into two lines (an `if` line and a separate `return` line) to satisfy linters; look for the list comprehension that builds fake_entries (uses MagicMock(entry_dn=...)) and the test(s) containing the compressed `if ...: return ...` statements and update them accordingly.
246-251: Patch target doesn't match the code path actually exercised.
AuthManager.get_provider()reads theenabledflag viaLdapProvider()._read_config().get("enabled", False)— it never callsauth.manager.get_setting_valuedirectly. So thepatch("auth.manager.get_setting_value", return_value=False)here has no effect on the branch under test; the test only passes coincidentally because_read_config()happens to default toenabled=Falsewhen no env/DB is configured in the test environment.For parity with
test_uses_ldap_when_ldap_enabled(line 255), patchauth.manager.LdapProvider._read_configdirectly:Proposed diff
def test_uses_local_when_ldap_disabled(self): from auth.manager import AuthManager - with patch("auth.manager.get_setting_value", return_value=False): + with patch("auth.manager.LdapProvider._read_config", return_value={"enabled": False}): manager = AuthManager() provider = manager.get_provider() assert provider.name == "local"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/backend/test_auth_providers.py` around lines 246 - 251, The test test_uses_local_when_ldap_disabled is patching the wrong symbol; AuthManager.get_provider checks LdapProvider()._read_config().get("enabled", False) not auth.manager.get_setting_value, so change the patch to target auth.manager.LdapProvider._read_config (or LdapProvider._read_config in the same import path used in the test) and have it return a dict with "enabled": False; this mirrors test_uses_ldap_when_ldap_enabled and ensures AuthManager.get_provider observes LDAP disabled via LdapProvider._read_config rather than relying on default behavior.server/plugin.py (1)
236-288: Minor hygiene in_run_test_cmd.A few small polish items (none are blockers):
output[:2000] if len(output) > 2000 else outputis equivalent to justoutput[:2000].import shlexis tucked inside the function; moving it to the top with the other stdlib imports keeps imports uniform.plugin["data_source"](line 212 inhandle_test) willKeyErrorfor a malformed plugin manifest — usingplugin.get("data_source")with a clear error branch would match the defensive style you used forplugin.get("settings", [])in_run_test_cmd.- The broad
except Exceptionon line 276 (Ruff BLE001) is appropriate here — it's a test affordance that must surface any failure as a notification — but consider loggingtype(e).__name__as well to make the notification more diagnosable.Proposed diff
- import shlex try: output = subprocess.check_output( shlex.split(cmd_str), @@ - except Exception as e: - output = f"[Test] {prefix}: Error running test: {e}" + except Exception as e: + output = f"[Test] {prefix}: Error running test: {type(e).__name__}: {e}" output = output.strip() mylog("minimal", [f"[Test] {prefix} output:\n{output}"]) # Send result as in-app notification (truncate for safety) - safe_output = output[:2000] if len(output) > 2000 else output + safe_output = output[:2000]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/plugin.py` around lines 236 - 288, In _run_test_cmd: simplify the truncation to use output[:2000] unconditionally, move the shlex import out of the function to the module-level imports, and when catching the broad exception in _run_test_cmd include the exception type name (type(e).__name__) in the notification/log message; additionally, in the handle_test path that currently uses plugin["data_source"], switch to plugin.get("data_source") and add a clear error branch (write_notification/log and return) when data_source is missing to avoid KeyError — use these symbol names (_run_test_cmd, handle_test, plugin.get("settings", [])) to locate the changes.test/server/test_ldap_provider_env.py (1)
1-64: LGTM — env precedence and type coercion coverage.Good coverage of both the no-env (DB-backed) and env-override paths, with realistic string values to exercise the
bool/intcoercion in_read_config. One small note:test_read_config_no_envdoes not also assert the env path'sbind_password/bind_dndefaults (empty) nor that the secret lookup does not accidentally read/run/secrets/ldap_bind_passwordon the CI host — if a CI runner happens to have that path, the password assertion elsewhere would drift. Consider patchingos.path.isfilein both tests for full isolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/server/test_ldap_provider_env.py` around lines 1 - 64, Patch both tests to mock os.path.isfile to always return False to prevent reading /run/secrets on CI and keep isolation: add `@patch`("os.path.isfile", return_value=False) to test_read_config_no_env and test_read_config_with_env (update the test function signatures to accept the new mock in the correct order), and in test_read_config_no_env also assert that cfg["bind_dn"] and cfg["bind_password"] are empty/defaults so the secret lookup didn't populate them; the impacted symbol to change is the test functions (test_read_config_no_env, test_read_config_with_env) and these changes ensure LdapProvider._read_config is exercised without reading host secrets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env:
- Around line 14-18: Update the commented LDAP example to use
encrypted-by-default settings: change the example variables (LDAP_PORT,
LDAP_USE_SSL, LDAP_USE_START_TLS, LDAP_TLS_VERIFY_CERT, LDAP_CA_CERT_PATH) so
StartTLS is enabled by default (e.g., LDAP_PORT=389, LDAP_USE_SSL=false,
LDAP_USE_START_TLS=true) and ensure LDAP_TLS_VERIFY_CERT=true and
LDAP_CA_CERT_PATH is present; alternatively provide a second LDAPS example
(LDAP_PORT=636, LDAP_USE_SSL=true, LDAP_USE_START_TLS=false) so users have a
secure default choice.
In @.github/workflows/run-all-tests.yml:
- Around line 26-29: The new boolean input run_ldap is defined but not used when
composing the PATHS variable in the workflow; update the path-building logic so
PATHS includes the LDAP test paths when inputs.run_ldap (or the workflow input
named run_ldap) is true. Locate the PATHS composition/patterns in the path
builder step (the code that reads inputs.run_all and other run_* flags and
produces PATHS) and add a branch that appends the LDAP-specific path patterns
when run_ldap is enabled, ensuring the same pattern is applied for both the
individual-run flow and the run_all flow so selecting run_ldap actually affects
which tests run.
- Around line 100-102: Remove the malformed duplicated cleanup command where the
second "docker stop netalertx-test-container || true" is concatenated on the
same line as "docker rm netalertx-test-container || true"; replace that line
with a single proper "docker stop netalertx-test-container || true" followed by
a separate "docker rm netalertx-test-container || true" (i.e., keep one stop and
one rm invocation, each on their own line) so the stop/rm cleanup commands parse
and run correctly.
In `@docker-compose.yml`:
- Around line 3-11: Enable host networking and ARP flux sysctls in
docker-compose.yml by adding network_mode: host to the service and configuring
the sysctls block to set ARP mitigation values (e.g.
net.ipv4.conf.all.arp_filter=1, net.ipv4.conf.default.arp_filter=1,
net.ipv4.conf.all.arp_announce=2, net.ipv4.conf.default.arp_announce=2,
net.ipv4.conf.all.arp_ignore=1, net.ipv4.conf.default.arp_ignore=1) so that
passive LAN discovery (ARP/NBNS/mDNS) works like the production compose files;
update the service entry that currently has ports and image/container_name to
include network_mode: host and the sysctls keys.
In `@front/index.php`:
- Around line 127-134: Ensure $api_token is resolved to the effective runtime
value (not the possibly-empty value read from app.conf) before using it to build
the remember-me HMAC and before adding the LDAP Authorization header; replace
direct use of the raw $api_token with the resolved token (the same value other
frontend code uses as a fallback) when computing $cookie_value (used with
COOKIE_SAVE_LOGIN_NAME) and when setting the "Authorization: Bearer ..." header
so both the HMAC and LDAP calls use the correct token.
- Around line 38-40: The code that sets $ldap_enabled from
getConfigLine('/^LDAP_enabled.*=/', $configLines) only treats the
trimmed/lowercased value as enabled when it equals 'true' but should also accept
'1'; update the condition that assigns $ldap_enabled (using
$ldap_enabled_line[1]) so it trims and lowercases the value and treats it as
enabled when it equals 'true' OR equals '1' (e.g., normalize with
strtolower(trim(...)) and check === 'true' || === '1') in the block where
$ldap_enabled_line is handled.
- Around line 48-57: Update the fallback GraphQL/API port used for LDAP
authentication: change the default assignment of $graphql_port from 20211 to
20212 and update the adjacent comment to reflect that 20212 is the API default;
locate the logic around getConfigLine('/^GRAPHQL_PORT.*=/', $configLines), the
$gql_line parsing and validation, and the $ldap_login_url construction to ensure
the LDAP POST target uses the corrected default port (20212) when GRAPHQL_PORT
is not set.
In `@front/php/templates/auth.php`:
- Around line 32-40: The code assumes config entries exist and can index them,
which can emit warnings and allows an empty API_TOKEN to make the HMAC unkeyed;
update the auth path to validate presence and shape before using values: check
that $config_file_lines_pw is non-empty and that explode("'",
$config_file_lines_pw[0]) returns at least 2 elements (use that element as
$nax_Password), ensure $config_file_lines_token yields a non-empty $api_token,
and only compute $expected_cookie = hash_hmac('sha256', $nax_Password,
$api_token) and perform hash_equals when both secrets are present; if any of
these checks fail, return a 401 response (send proper 401 header and exit)
instead of proceeding or emitting warnings.
In `@server/__main__.py`:
- Around line 130-132: The code resets is_idle_this_loop to False whenever
conf.last_scan_run + 1 minute < conf.loop_start_time (and similarly in the block
at the other occurrence), which treats every scheduler tick as work; instead,
only clear the idle edge when real work ran. Remove the unconditional time-based
assignment and instead clear is_idle_this_loop = False only in the code paths
that perform actual work (e.g., where scans, workflows, or user-driven tasks are
executed) — or set a dedicated flag like work_performed and set
is_idle_this_loop = False when work_performed is true; apply the same change for
the analogous block that currently repeats this behavior (the other occurrence
around the maintenance/uptime check).
- Around line 274-279: The idle-notification call can raise and crash main; wrap
the write_notification("All processes are idle", "info") and the update to
last_idle_notification_ts in a try/except block so failures are best-effort and
do not stop the loop: catch Exception as e, log the error with mylog (e.g.
mylog("error", ["[MAIN] failed to send idle notification:", str(e)])), and only
assign last_idle_notification_ts = now when write_notification succeeds; leave
the surrounding idle-check logic (is_idle_this_loop, was_idle,
IDLE_NOTIFICATION_COOLDOWN_HOURS) unchanged.
In `@server/api_server/api_server_start.py`:
- Around line 1964-2060: The _get_client_ip/trusted_proxies logic assumes
callers are only local PHP and includes an invalid "localhost" entry; update
_get_client_ip to remove "localhost" from trusted_proxies and make
trusted_proxies configurable (e.g., read from an env/config var or add known
proxy IPs) so real reverse proxies can be trusted or documented, and instruct
ops to ensure the fronting proxy preserves/appends X-Forwarded-For; also replace
all del FAILED_LOGINS[...] patterns in api_auth_login (prune loop and
clearing-after-success) with FAILED_LOGINS.pop(ip, None) or
FAILED_LOGINS.pop(client_ip, None) while keeping the _failed_logins_lock usage
to satisfy lint rule RUF051 and avoid KeyError.
In `@server/auth/ldap_provider.py`:
- Around line 289-291: The mylog calls in auth/ldap_provider.py are logging
unsanitized user-controlled and directory-provided identifiers (e.g., username
and the DN used later) which can lead to log injection; update the f-strings in
the mylog invocations (the one referencing username, entries and search_filter
and the later one referencing the DN variable) to pass those values through the
existing _sanitize_for_log(...) helper before interpolation (e.g., replace
username and the DN with _sanitize_for_log(username) and _sanitize_for_log(dn)
respectively) so logs never contain raw control characters or unescaped input.
- Around line 279-288: The LDAP search call currently ignores the boolean result
of conn.search(); change the code around conn.search(...) and the subsequent
entries handling so you check its return value and treat a False result as an
operation failure (same pattern used for bind failures around the earlier bind
call). If conn.search(...) returns False, surface/log the failure using
conn.result (or return an error/None to fail closed) instead of falling through
to inspect conn.entries; only proceed to read conn.entries and apply the
len(entries) != 1 check when conn.search(...) returned True.
In `@server/auth/manager.py`:
- Around line 32-77: The LDAP branch in authenticate uses a magic error string
and a hardcoded "admin" check; replace the literal "User not found" with a
shared sentinel (e.g. add LdapProvider.USER_NOT_FOUND or an
AuthResult.error_code and use that in LdapProvider._resolve_user_dn and
auth.manager.authenticate) and remove the hardcoded username policy by
delegating the fallback decision to LocalProvider (add
LocalProvider.is_fallback_allowed(username) and call it instead of checking
username == "admin"); also add a short comment near
LocalProvider.is_fallback_allowed explaining the policy.
In `@test_patch.py`:
- Around line 1-9: This file is executed at import time and must be converted
into a proper pytest test or excluded from discovery; wrap the logic in a test
function (e.g., def test_get_provider_with_ldap_enabled():) that uses
patch("auth.manager.LdapProvider._read_config", return_value={"enabled": True})
as a context manager or fixture, instantiate AuthManager(), call
AuthManager.get_provider(), and assert expected behavior (e.g., provider.name ==
"ldap" or non-None); alternatively rename the file so it doesn't match pytest's
test_*.py pattern or move it to a scripts/ directory to avoid import-time side
effects.
In `@test/api_endpoints/test_auth_endpoints.py`:
- Around line 79-134: All login tests must include the internal Bearer
authorization and ensure LDAP is disabled for local-provider flows; update the
tests (test_login_valid_local_credentials, test_login_wrong_local_password,
test_login_missing_password_field, test_login_missing_username_field,
test_login_empty_body, test_login_no_json_content_type) to send the required
Authorization: Bearer <internal-token> header and, for local-provider scenarios,
patch auth.manager.get_setting_value to return False (as already done in some
tests) so the request is authorized and provider logic runs; ensure the header
uses the app's configured internal token (or the test helper that exposes it)
rather than omitting auth so assertions check validation/provider behavior
instead of 403.
In `@test/backend/test_auth_providers.py`:
- Around line 311-313: Remove the stray string literal "local" that follows the
assertion on result.provider in test_auth_providers.py (the orphaned expression
on the line after assert result.provider == "local"); either delete that bare
literal or replace it with the intended assertion if a duplicate/alternate check
was meant, ensuring only meaningful assertions remain (inspect the test around
result.provider to confirm no other assertion was intended).
In `@test/docker_tests/test_ldap_ui.py`:
- Line 65: Replace the incorrect config key written in the test: change
occurrences where the test writes "LDAP_login_attribute" to the provider's
actual key "LDAP_username_attribute" (the f.write call in test_ldap_ui.py), and
update the test to assert that non-default values (e.g., "sAMAccountName") are
correctly applied through the app.conf wiring so the provider reads the
configured username attribute rather than relying on the default.
- Around line 162-167: The subprocess.run calls that execute Docker commands
(the call using variables cmd with stdout/stderr/text) need a timeout to avoid
hanging CI; add a timeout argument (e.g. timeout=30) to these subprocess.run
invocations and wrap them in try/except for subprocess.TimeoutExpired (for the
occurrences around the shown call and the other similar calls around lines
177-183 and 192-198) so the test fails fast—on TimeoutExpired, call pytest.fail
or raise a clear exception including the command and timeout info to surface the
failure immediately.
---
Nitpick comments:
In `@front/plugins/auth_ldap/test_ldap.py`:
- Around line 47-78: Initialize exit_code before the conditional that checks
args.test_user to avoid a potential NameError later; specifically, add exit_code
= 0 prior to the "if args.test_user:" block in test_ldap.py so that the later
conditional "if args.test_user and exit_code:" (and any future refactors) can
safely reference exit_code, while leaving the existing logic in the
args.test_user branch (setting exit_code = 1 on error) and the
conn.unbind()/sys.exit behavior unchanged.
In `@server/plugin.py`:
- Around line 236-288: In _run_test_cmd: simplify the truncation to use
output[:2000] unconditionally, move the shlex import out of the function to the
module-level imports, and when catching the broad exception in _run_test_cmd
include the exception type name (type(e).__name__) in the notification/log
message; additionally, in the handle_test path that currently uses
plugin["data_source"], switch to plugin.get("data_source") and add a clear error
branch (write_notification/log and return) when data_source is missing to avoid
KeyError — use these symbol names (_run_test_cmd, handle_test,
plugin.get("settings", [])) to locate the changes.
In `@test/backend/test_auth_providers.py`:
- Line 149: Remove the unnecessary f-string in the fake_entries list
comprehension (the literal "uid=alice,dc=example,dc=com" should not use an
f-prefix) and split any condensed single-line if-return statements around the
test functions at the block containing lines like `if ...: return ...` into two
lines (an `if` line and a separate `return` line) to satisfy linters; look for
the list comprehension that builds fake_entries (uses MagicMock(entry_dn=...))
and the test(s) containing the compressed `if ...: return ...` statements and
update them accordingly.
- Around line 246-251: The test test_uses_local_when_ldap_disabled is patching
the wrong symbol; AuthManager.get_provider checks
LdapProvider()._read_config().get("enabled", False) not
auth.manager.get_setting_value, so change the patch to target
auth.manager.LdapProvider._read_config (or LdapProvider._read_config in the same
import path used in the test) and have it return a dict with "enabled": False;
this mirrors test_uses_ldap_when_ldap_enabled and ensures
AuthManager.get_provider observes LDAP disabled via LdapProvider._read_config
rather than relying on default behavior.
In `@test/server/test_ldap_provider_env.py`:
- Around line 1-64: Patch both tests to mock os.path.isfile to always return
False to prevent reading /run/secrets on CI and keep isolation: add
`@patch`("os.path.isfile", return_value=False) to test_read_config_no_env and
test_read_config_with_env (update the test function signatures to accept the new
mock in the correct order), and in test_read_config_no_env also assert that
cfg["bind_dn"] and cfg["bind_password"] are empty/defaults so the secret lookup
didn't populate them; the impacted symbol to change is the test functions
(test_read_config_no_env, test_read_config_with_env) and these changes ensure
LdapProvider._read_config is exercised without reading host secrets.
In `@test/ui/test_ui_ldap_login.py`:
- Around line 10-12: Replace the flaky time.sleep(2) calls in
test_ui_ldap_login.py with an explicit Selenium wait for a known authenticated
state (e.g., wait for the post-login URL or a dashboard element) using
WebDriverWait and expected_conditions; locate the occurrences by searching for
time.sleep(...) and the post-login assertion in this file and change them to
WebDriverWait(driver,
timeout).until(EC.visibility_of_element_located((By.<SOME_LOCATOR>,
"<locator>"))) or until URL contains the authenticated route to ensure the test
only proceeds when the app is truly logged in.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b94c7f0-709f-4375-9b5d-a02260c3f05e
⛔ Files ignored due to path filters (1)
docker_build.logis excluded by!**/*.log
📒 Files selected for processing (32)
.env.github/workflows/run-all-tests.ymldocker-compose.ymldocs/LDAP_CONFIGURATION.mdfront/index.phpfront/php/templates/auth.phpfront/php/templates/language/en_us.jsonfront/php/templates/security.phpfront/plugins/auth_ldap/config.jsonfront/plugins/auth_ldap/test_ldap.pymkdocs.ymlrequirements.txtscripts/run_tests_in_docker_environment.shserver/__main__.pyserver/api_server/api_server_start.pyserver/api_server/openapi/schemas.pyserver/auth/__init__.pyserver/auth/base.pyserver/auth/ldap_provider.pyserver/auth/local_provider.pyserver/auth/manager.pyserver/database.pyserver/db/db_upgrade.pyserver/plugin.pytest/api_endpoints/test_auth_endpoints.pytest/backend/test_auth_providers.pytest/docker_tests/test_ldap_ui.pytest/server/test_ldap_provider.pytest/server/test_ldap_provider_env.pytest/server/test_plugin_handle_test.pytest/ui/test_ui_ldap_login.pytest_patch.py
jokob-sk
requested changes
Apr 18, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/docker_tests/test_ldap_ui.py (1)
84-127: Drop the obsoleteversion: "3.8"compose key.Recent Docker Compose emits a warning (
the attribute 'version' is obsolete) that pollutes CI output and is ignored by the spec. Minor housekeeping.Proposed fix
compose_config = { - "version": "3.8", "volumes": { data_volume_name: {} },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/docker_tests/test_ldap_ui.py` around lines 84 - 127, The compose_config dict includes the obsolete "version": "3.8" key which triggers compose warnings; remove the "version" entry from the compose_config literal (the top-level key in the compose_config dict) and leave the rest of the dict (volumes, services, etc.) intact so the generated compose payload remains valid for Docker Compose without emitting the obsolete-key warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/docker_tests/test_ldap_ui.py`:
- Line 151: The test currently builds container_name using
f"{project_name}-netalertx-1" which assumes Compose v2 naming; update the logic
that sets container_name (and any later uses in docker exec/docker logs calls)
to detect the real container name at runtime by querying Docker for a matching
container (e.g., run a docker ps/name filter or check for both
f"{project_name}-netalertx-1" and f"{project_name}_netalertx_1") and pick
whichever exists; apply this detection where container_name is used and in the
docker logs calls so tests work with both Compose v1 and v2 naming conventions.
---
Nitpick comments:
In `@test/docker_tests/test_ldap_ui.py`:
- Around line 84-127: The compose_config dict includes the obsolete "version":
"3.8" key which triggers compose warnings; remove the "version" entry from the
compose_config literal (the top-level key in the compose_config dict) and leave
the rest of the dict (volumes, services, etc.) intact so the generated compose
payload remains valid for Docker Compose without emitting the obsolete-key
warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aab8dafb-d01f-4e4d-8c0a-811f31924a0f
📒 Files selected for processing (2)
.github/workflows/run-all-tests.ymltest/docker_tests/test_ldap_ui.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/run-all-tests.yml
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
test/api_endpoints/test_auth_endpoints.py (1)
79-104:⚠️ Potential issue | 🟠 MajorForce LDAP off in the local-login tests.
These tests still depend on the real LDAP config because
AuthManagerreadsLdapProvider._read_config()before local auth. If LDAP is enabled in the test environment, the assertions can fail or hit LDAP unexpectedly.🧪 Proposed fix
def test_login_valid_local_credentials(client, api_token): """POST /api/auth/login with correct local password returns 200.""" - with patch("auth.local_provider.get_setting_value", return_value=_DEFAULT_PW_HASH), \ + with patch("auth.manager.LdapProvider._read_config", return_value={"enabled": False}), \ + patch("auth.local_provider.get_setting_value", return_value=_DEFAULT_PW_HASH), \ patch("auth.manager.get_setting_value", return_value=False): resp = client.post( "/api/auth/login", headers=auth_headers(api_token), json={"username": "admin", "password": _DEFAULT_PW}, @@ def test_login_wrong_local_password(client, api_token): """POST /api/auth/login with incorrect password returns 401.""" - with patch("auth.local_provider.get_setting_value", return_value=_DEFAULT_PW_HASH), \ + with patch("auth.manager.LdapProvider._read_config", return_value={"enabled": False}), \ + patch("auth.local_provider.get_setting_value", return_value=_DEFAULT_PW_HASH), \ patch("auth.manager.get_setting_value", return_value=False): resp = client.post( "/api/auth/login", headers=auth_headers(api_token), json={"username": "admin", "password": "totally_wrong"},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/api_endpoints/test_auth_endpoints.py` around lines 79 - 104, The tests test_login_valid_local_credentials and test_login_wrong_local_password rely on local auth but still trigger real LDAP reads via AuthManager during setup; patch LdapProvider._read_config (e.g. patch("auth.ldap_provider.LdapProvider._read_config", return_value=None)) or patch the AuthManager LDAP-check helper (e.g. patch("auth.manager.AuthManager.is_ldap_enabled", return_value=False)) inside both tests so LDAP is forced off before posting, keeping the existing patches for local settings and ensuring the tests never touch real LDAP config.test/docker_tests/test_ldap_ui.py (2)
145-146:⚠️ Potential issue | 🟡 MinorResolve Compose containers dynamically instead of hard-coding v2 names.
The test still assumes
"{project_name}-service-1"names. That breaks under legacy Compose naming or compatibility mode, and Line 180 reintroduces the same hard-coded value beforedocker exec. Usedocker compose ps -q <service>once and reuse those IDs forexec/logs.Proposed adjustment
+ def _compose_container_id(service: str) -> str: + res = subprocess.run( + ["docker", "compose", "-f", str(compose_file), "-p", project_name, "ps", "-q", service], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + timeout=30, + check=False, + ) + assert res.returncode == 0 and res.stdout.strip(), ( + f"Could not resolve {service} container: {res.stderr}" + ) + return res.stdout.strip().splitlines()[0] + - container_name = f"{project_name}-netalertx-1" - ldap_container = f"{project_name}-ldap-1" + container_name = _compose_container_id("netalertx") + ldap_container = _compose_container_id("ldap") @@ - container_name = f"{project_name}-netalertx-1" @@ - ldap_logs_cmd = ["docker", "logs", f"{project_name}-ldap-1"] + ldap_logs_cmd = ["docker", "logs", ldap_container]To verify whether the current runner is exposed to this naming mismatch:
#!/bin/bash # Read-only check: report Docker Compose implementation and compatibility mode. set -u (docker compose version || docker-compose version || true) 2>&1 printf 'COMPOSE_COMPATIBILITY=%s\n' "${COMPOSE_COMPATIBILITY:-}"Also applies to: 180-188, 212-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/docker_tests/test_ldap_ui.py` around lines 145 - 146, The test hard-codes Compose container names via container_name and ldap_container (e.g., f"{project_name}-netalertx-1") which breaks under different Compose naming modes; replace those hard-coded values by resolving container IDs once using "docker compose ps -q <service>" (for services like netalertx and ldap) and store the returned IDs in variables (used later where docker exec and docker logs are called) so all subsequent docker exec/docker logs calls use the resolved IDs instead of hard-coded names; update references around container_name, ldap_container and any docker exec/docker logs usages (the blocks around lines where docker exec is invoked) to reuse those variables.
153-178:⚠️ Potential issue | 🟠 MajorReplace fixed sleeps with bounded readiness checks before seeding LDAP.
depends_onandtime.sleep(10)do not guarantee OpenLDAP is ready, and theldapaddexec has no timeout. This can either fail flakes early or hang CI if the container wedges. Also,docker network connectcurrently ignores non-zero failures becausecheck=Falsedoes not raise.Proposed hardening
print(f"Connecting test container {test_container_id} to network {network_name}...") - try: - subprocess.run(["docker", "network", "connect", network_name, test_container_id], check=False) - except Exception as e: - print(f"Failed to connect network: {e}") + connect_res = subprocess.run( + ["docker", "network", "connect", network_name, test_container_id], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + timeout=30, + check=False, + ) + if connect_res.returncode != 0 and "already exists" not in connect_res.stderr.lower(): + print(f"Failed to connect network: {connect_res.stderr}") @@ - # Give LDAP and NetAlertX a few seconds to come up - time.sleep(10) + # Wait for LDAP bind readiness before seeding users. + ldap_ready = False + for _ in range(30): + ready_res = subprocess.run( + ["docker", "exec", ldap_container, "ldapwhoami", "-x", "-D", "cn=admin,dc=example,dc=org", "-w", "admin"], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + timeout=10, + check=False, + ) + if ready_res.returncode == 0: + ldap_ready = True + break + time.sleep(2) + assert ldap_ready, f"LDAP did not become ready: {ready_res.stderr}" @@ - # Seed LDAP via docker exec print(f"Seeding LDAP users into {ldap_container}...") + print(f"Seeding LDAP users into {ldap_container}...") subprocess.run( ["docker", "exec", "-i", ldap_container, "ldapadd", "-x", "-D", "cn=admin,dc=example,dc=org", "-w", "admin"], input=LDIF_CONTENT, text=True, - check=True + check=True, + timeout=30, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/docker_tests/test_ldap_ui.py` around lines 153 - 178, Replace the fixed sleeps and unchecked docker calls with bounded readiness checks: make the docker network connect call raise on failure (use check semantics or explicitly handle subprocess.returncode instead of check=False) for the subprocess.run(["docker","network","connect", ...]) call; replace time.sleep(10) and the unconditional ldapadd exec with a loop that polls OpenLDAP readiness (e.g., attempt a short ldapsearch or TCP connect to ldap port) and only runs subprocess.run([... "docker","exec", ldap_container, "ldapadd", ...], check=True, timeout=30) after the service responds, and add a timeout parameter to the ldapadd subprocess.run to avoid hanging; keep the existing NetAlertX socket-wait loop but treat failures as hard errors if LDAP readiness never succeeds within a bounded retry window.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/api_endpoints/test_auth_endpoints.py`:
- Around line 79-104: The tests test_login_valid_local_credentials and
test_login_wrong_local_password rely on local auth but still trigger real LDAP
reads via AuthManager during setup; patch LdapProvider._read_config (e.g.
patch("auth.ldap_provider.LdapProvider._read_config", return_value=None)) or
patch the AuthManager LDAP-check helper (e.g.
patch("auth.manager.AuthManager.is_ldap_enabled", return_value=False)) inside
both tests so LDAP is forced off before posting, keeping the existing patches
for local settings and ensuring the tests never touch real LDAP config.
In `@test/docker_tests/test_ldap_ui.py`:
- Around line 145-146: The test hard-codes Compose container names via
container_name and ldap_container (e.g., f"{project_name}-netalertx-1") which
breaks under different Compose naming modes; replace those hard-coded values by
resolving container IDs once using "docker compose ps -q <service>" (for
services like netalertx and ldap) and store the returned IDs in variables (used
later where docker exec and docker logs are called) so all subsequent docker
exec/docker logs calls use the resolved IDs instead of hard-coded names; update
references around container_name, ldap_container and any docker exec/docker logs
usages (the blocks around lines where docker exec is invoked) to reuse those
variables.
- Around line 153-178: Replace the fixed sleeps and unchecked docker calls with
bounded readiness checks: make the docker network connect call raise on failure
(use check semantics or explicitly handle subprocess.returncode instead of
check=False) for the subprocess.run(["docker","network","connect", ...]) call;
replace time.sleep(10) and the unconditional ldapadd exec with a loop that polls
OpenLDAP readiness (e.g., attempt a short ldapsearch or TCP connect to ldap
port) and only runs subprocess.run([... "docker","exec", ldap_container,
"ldapadd", ...], check=True, timeout=30) after the service responds, and add a
timeout parameter to the ldapadd subprocess.run to avoid hanging; keep the
existing NetAlertX socket-wait loop but treat failures as hard errors if LDAP
readiness never succeeds within a bounded retry window.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d82d64e4-60a9-44a8-a5fb-3d0bb9bef869
📒 Files selected for processing (2)
test/api_endpoints/test_auth_endpoints.pytest/docker_tests/test_ldap_ui.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
server/api_server/api_server_start.py (1)
1964-1971:⚠️ Potential issue | 🟠 MajorMake trusted proxy handling configurable for correct rate-limit keys.
This still assumes PHP is the only trusted caller. If NetAlertX is deployed behind a reverse proxy in front of both PHP and Flask,
request.remote_addrcan collapse to the proxy IP and all users share one failed-login bucket. Keep localhost trusted for the PHP hop, but allow documented/configured proxy IPs for real deployments.#!/bin/bash # Verify whether trusted proxy configuration already exists elsewhere. # Expect: either no matches, or a documented setting/env var that can be wired into _get_client_ip. rg -n -C3 'trusted_prox|TRUSTED_PROXY|TRUSTED_PROXIES|X-Forwarded-For|ProxyFix'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api_server/api_server_start.py` around lines 1964 - 1971, The _get_client_ip function currently hardcodes trusted_proxies to localhost; change it to read a configurable list (e.g., from an environment variable named TRUSTED_PROXIES or from your app config like config.trusted_proxies) while always keeping {"127.0.0.1","::1"} included, then use that combined set in the existing logic to decide when to trust X-Forwarded-For; ensure the function (_get_client_ip) still splits and strips forwarded values and falls back to request.remote_addr when remote_addr is not in the configured trusted set.server/auth/ldap_provider.py (1)
280-288:⚠️ Potential issue | 🟠 MajorCheck
conn.search()before readingconn.entries.A failed LDAP search still falls through to
conn.entries, so operational failures can be returned asUSER_NOT_FOUNDand trigger local fallback behavior instead of failing closed. This is still the same unresolved search-result handling issue.Proposed fix
- conn.search( + search_success = conn.search( search_base=cfg["base_dn"], search_filter=search_filter, search_scope=ldap3.SUBTREE, attributes=[cfg["username_attr"]], size_limit=2, ) + + if not search_success: + mylog("none", [f"[auth.ldap] LDAP search failed: {conn.result}"]) + raise ConnectionError(f"LDAP search failed: {conn.result}") entries = conn.entriesldap3 Connection.search return value false operation failed default SYNC strategy🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/auth/ldap_provider.py` around lines 280 - 288, The code calls conn.search(...) and immediately reads conn.entries, which can hide failures; update the logic in the LDAP lookup (the block around conn.search, conn.entries and search_filter usage) to check the boolean return value of conn.search(...) and abort on failure instead of using conn.entries: if conn.search(...) returns False, log or surface conn.result (or conn.last_error) and raise/return an appropriate error (fail closed) rather than proceeding to read conn.entries; only read conn.entries when conn.search(...) returned True and then handle size_limit / multiple entries as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@front/index.php`:
- Around line 128-150: The function login_user currently marks the session
authenticated before validating API_TOKEN; change the flow in login_user so you
resolve and validate $resolved_api_token (using $api_token or
getSettingValue('API_TOKEN')) first and throw RuntimeException if empty, and
only after successful validation call session_regenerate_id(true), set
$_SESSION['login'] = 1, generate $_SESSION['csrf_token'], and then set the
remember-me cookie (using hash_hmac with $nax_Password and $resolved_api_token);
this ensures no partial authentication state is written if API_TOKEN is missing.
- Around line 36-42: The UI currently only treats 'true' or '1' as enabling LDAP
($env_ldap and $ldap_enabled_value), causing inconsistency with the backend
which accepts 'yes'; update the checks in the login page so both branches accept
'yes' as enabled too: in the $env_ldap branch update the conditional that sets
$ldap_enabled to also consider strtolower(trim($env_ldap)) === 'yes', and in the
config parsing branch include strtolower(trim($ldap_enabled_line[1])) === 'yes'
when computing $ldap_enabled (references: $env_ldap, $ldap_enabled,
$ldap_enabled_line, getConfigLine, $ldap_enabled_value).
In `@server/api_server/api_server_start.py`:
- Around line 2014-2017: The notification currently writes raw client_ip into
write_notification; sanitize the IP before persisting by passing client_ip
through the existing _sanitize_for_log (or a dedicated sanitize_ip helper) and
use that sanitized value in the message passed to write_notification and any
similar calls around write_notification in the same file (including the block
referenced at lines ~2041-2044). Locate occurrences where write_notification is
called with client_ip and replace the raw variable with the sanitized result so
no untrusted X-Forwarded-For data is stored directly.
In `@test/backend/test_auth_providers.py`:
- Line 149: Replace the unnecessary f-string used when constructing fake_entries
(MagicMock(entry_dn=f"uid=alice,dc=example,dc=com")) with a plain string literal
("uid=alice,dc=example,dc=com") to satisfy Ruff, and convert the one-line if
statement later in the same test file into a standard multi-line if block with
the body on the following indented line (rather than a single-line body) so Ruff
no longer flags it; locate these by searching for the fake_entries variable and
the single-line if in test_auth_providers.py and adjust accordingly.
- Around line 246-267: The tests assume LDAP is disabled via get_setting_value
but AuthManager also reads LDAP_enabled via LdapProvider._read_config; update
the failing tests to patch LdapProvider._read_config to return {"enabled":
False} (or patch it alongside get_setting_value) so the AuthManager uses the
local branch; find references to AuthManager.get_provider,
AuthManager.authenticate and LdapProvider._read_config in the tests and add the
patch for LdapProvider._read_config in those cases.
In `@test/ui/test_ui_ldap_login.py`:
- Around line 41-49: The test currently allows any non-'/index.php' redirect
which can hide error pages; change the assertion to require the explicit success
route by asserting that '/devices.php' is in driver.current_url (remove the "or
'/index.php' not in driver.current_url" alternative) after calling
wait_for_page_load(driver, timeout=5); use the existing wait_for_page_load and
driver.current_url checks (in test_ui_ldap_login.py around wait_for_page_load
and driver.current_url) so the test only passes when the browser actually lands
on /devices.php.
---
Duplicate comments:
In `@server/api_server/api_server_start.py`:
- Around line 1964-1971: The _get_client_ip function currently hardcodes
trusted_proxies to localhost; change it to read a configurable list (e.g., from
an environment variable named TRUSTED_PROXIES or from your app config like
config.trusted_proxies) while always keeping {"127.0.0.1","::1"} included, then
use that combined set in the existing logic to decide when to trust
X-Forwarded-For; ensure the function (_get_client_ip) still splits and strips
forwarded values and falls back to request.remote_addr when remote_addr is not
in the configured trusted set.
In `@server/auth/ldap_provider.py`:
- Around line 280-288: The code calls conn.search(...) and immediately reads
conn.entries, which can hide failures; update the logic in the LDAP lookup (the
block around conn.search, conn.entries and search_filter usage) to check the
boolean return value of conn.search(...) and abort on failure instead of using
conn.entries: if conn.search(...) returns False, log or surface conn.result (or
conn.last_error) and raise/return an appropriate error (fail closed) rather than
proceeding to read conn.entries; only read conn.entries when conn.search(...)
returned True and then handle size_limit / multiple entries as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e285046b-6115-45d0-932f-dc3540dc2c61
📒 Files selected for processing (10)
.envfront/index.phpfront/php/templates/auth.phpscripts/run_tests_in_docker_environment.shserver/__main__.pyserver/api_server/api_server_start.pyserver/auth/ldap_provider.pyserver/auth/manager.pytest/backend/test_auth_providers.pytest/ui/test_ui_ldap_login.py
✅ Files skipped from review due to trivial changes (1)
- .env
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/run_tests_in_docker_environment.sh
- front/php/templates/auth.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
test/ui/test_ui_login.py (2)
10-10: Unusedsubprocessimport.
subprocessis imported but never referenced in this file (per the summary and a scan of the diff). Remove it to avoid Ruff/flake8 noise.Proposed fix
import time -import subprocess import pytest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/test_ui_login.py` at line 10, The file defines an unused import named subprocess in test/ui/test_ui_login.py; remove the unused "import subprocess" statement so the module no longer imports subprocess, which will silence Ruff/flake8 warnings (search for the import line referencing subprocess and delete it).
111-111: Ruff RUF001: replace ambiguous Unicodeℹ.Use ASCII (e.g.,
[i]or drop the prefix) to silence the linter and avoid terminal-rendering issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/test_ui_login.py` at line 111, Replace the ambiguous Unicode character in the print statement inside test_ui_login.py (the line that prints "ℹ Password not determinable from config, falling back to default password...") with an ASCII alternative such as "[i] Password not determinable from config, falling back to default password..." or remove the prefix entirely to satisfy Ruff RUF001 and avoid terminal-rendering issues; update the print call text accordingly in the test_ui_login.py file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/docker_tests/test_ldap_ui.py`:
- Around line 53-58: The code hardcodes "/workspaces/NetAlertX"; replace that
logic so workspace_dir is resolved relative to this test file instead of an
absolute path: compute a project_root from pathlib.Path(__file__).resolve() (by
walking up parents until you reach the repo root or the containing "test"
folder), then set base_dir = project_root / "test" / "tmp_ldap_ui" (falling back
to tmp_path if needed) and update the pytest invocation that currently targets
the absolute "/workspaces/NetAlertX/test/ui/test_ui_ldap_login.py" to use the
computed project_root / "test" / "ui" / "test_ui_ldap_login.py"; change
references to workspace_dir, base_dir and the pytest target string accordingly.
- Line 62: The subprocess.run(["chmod", "-R", "777", str(base_dir /
"test_data")]) call can fail silently; modify the invocation in
test/docker_tests/test_ldap_ui.py (the subprocess.run call that sets permissions
on base_dir / "test_data") to pass check=True (or explicitly inspect and handle
the returncode) so the test fails immediately with a clear error if chmod fails
rather than surfacing as a later obscure runtime error.
- Around line 149-157: The current use of socket.gethostname() to derive the
Docker container name and the silent subprocess.run(..., check=False) for
"docker network connect" is fragile and can let tests proceed with an
unresolvable hostname; update the setup to detect whether we are running in a
container (e.g., inspect /proc/self/cpuset or /proc/1/cpuset and fallback to
/etc/hostname), extract the real container ID/name from those files instead of
socket.gethostname(), and only attempt docker network connect when that
detection indicates a container; when running subprocess.run for the "docker
network connect" command handle failures explicitly (use check=True or check the
CompletedProcess.returncode and raise a clear exception) and additionally verify
the resulting hostname resolves (use socket.getaddrinfo or
socket.create_connection with a short timeout) before continuing so the test
fails fast with a clear error if network connect/hostname resolution did not
succeed.
- Around line 177-185: Fix the broken inline-comment and dead assignment: split
the merged line so print("--- END LOGS ---\n") and print(f"Seeding LDAP users
into {ldap_container}...") are separate statements, ensure the
subprocess.run([...], input=LDIF_CONTENT, ...) call is correctly indented inside
the intended for/else block (so LDAP seeding runs only when intended), move or
split the comment so it doesn't swallow code (also split the later one-liner
comment near the end), and remove the redundant reassignment of container_name =
f"{project_name}-netalertx-1" (keep the original assignment and delete the dead
duplicate).
In `@test/plugins/test_fritzbox.py`:
- Around line 260-265: The test hardcodes the old fallback "02:00:00:00:00:01"
but create_guest_wifi_device(fc, host) now uses the supplied host as the
fallback seed when NewMACAddress is empty; update the assertion to expect the
host-seeded deterministic fallback instead of the stale hardcoded value by
comparing device["mac_address"] to the fallback generated for "fritz.box" (i.e.,
compute the expected fallback using the same host-seeding logic or helper used
by create_guest_wifi_device and assert equality).
In `@test/ui/test_ui_login.py`:
- Around line 21-59: The fixture ensure_web_protection currently drops any
existing SETPWD_password and uses a case-sensitive check for enabled state; fix
it by reading and saving the original SETPWD_enable_password and the original
SETPWD_password value when you scan the file (store in variables like
was_enabled and original_password), perform a case-insensitive check (e.g.,
lower() and look for "true" without relying on exact capitalization or quotes)
to decide whether to modify the file, and when tearing down restore the original
lines: if original_password is non-empty write back the original SETPWD_password
line and restore SETPWD_enable_password to its original value, otherwise write
SETPWD_enable_password='false'; also ensure the setup branch does not clobber an
existing password by only inserting the default password when no
original_password was found.
---
Nitpick comments:
In `@test/ui/test_ui_login.py`:
- Line 10: The file defines an unused import named subprocess in
test/ui/test_ui_login.py; remove the unused "import subprocess" statement so the
module no longer imports subprocess, which will silence Ruff/flake8 warnings
(search for the import line referencing subprocess and delete it).
- Line 111: Replace the ambiguous Unicode character in the print statement
inside test_ui_login.py (the line that prints "ℹ Password not determinable from
config, falling back to default password...") with an ASCII alternative such as
"[i] Password not determinable from config, falling back to default password..."
or remove the prefix entirely to satisfy Ruff RUF001 and avoid
terminal-rendering issues; update the print call text accordingly in the
test_ui_login.py file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 91e826dc-706d-4f38-84ee-ed1054d76ca1
📒 Files selected for processing (5)
front/php/templates/security.phptest/docker_tests/test_ldap_ui.pytest/plugins/test_fritzbox.pytest/ui/test_ui_login.pytest/ui/test_ui_maintenance.py
🚧 Files skipped from review as they are similar to previous changes (1)
- front/php/templates/security.php
adamoutler
force-pushed
the
ldap-login
branch
2 times, most recently
from
15667ed to
8d3aefe
Compare
jokob-sk
requested changes
Apr 20, 2026
…eedback - Security: Added prominent warning to LDAP config regarding default 'admin:123456' credentials and fallback risk. - Security: Enabled LDAP StartTLS by default in docker-compose.yml and added secure example to .env. - Security: Added regex sanitization to X-Forwarded-For IP parsing and LDAP identifiers to prevent log injection. - Security: Prevented authentication in PHP frontend if API_TOKEN is not configured. - Auth: Made rate-limiting (max attempts and lockout time) configurable via settings. - Auth: Re-added 'localhost' to trusted proxies to maintain compatibility with external integrations. - Auth: Replaced magic strings with LdapProvider.USER_NOT_FOUND sentinel for robust fallback logic. - Workflow: Fixed malformed cleanup command and re-wired run_ldap input in run-all-tests.yml. - CI/CD: Resolved test permission issues and SocketBlockedErrors by allowing socket access in docker tests. - CI/CD: Added timeouts to Docker subprocess calls to prevent CI hangs. - Maintenance: Reverted unrelated changes in docker-compose.yml, server/__main__.py, and server/plugin.py to minimize scope creep.
jokob-sk
requested changes
Apr 21, 2026
| "description": [ | ||
| { | ||
| "language_code": "en_us", | ||
| "string": "Configure LDAP / Active Directory authentication as an alternative to the local password. When enabled, the login form accepts a username and password which are validated against the LDAP server. The local password (SETPWD) continues to work as a fallback when LDAP is disabled. <br><br><b>Credential security:</b> Store the bind password via the <code>LDAP_BIND_PASSWORD</code> environment variable or a Docker secret rather than in <code>app.conf</code>. Always enable TLS certificate verification and use LDAPS or StartTLS in production." |
Collaborator
There was a problem hiding this comment.
I'd just keep "Configure LDAP / Active Directory authentication as an alternative to the local password. " the rest should be documented in the settings/readme.md of the plugin. that readme is then automatically linked in the settings sections, e.g:
| "show_ui": false | ||
| } | ||
| ], | ||
| "permissions": {} |
| <?php | ||
|
|
||
| require_once $_SERVER['DOCUMENT_ROOT'].'/php/server/db.php'; | ||
| require_once $_SERVER['DOCUMENT_ROOT'].'/php/server/util.php'; |
Collaborator
There was a problem hiding this comment.
add this to your learning @coderabbitai - to double check against security advisories
| <?php | ||
|
|
||
| require_once $_SERVER['DOCUMENT_ROOT'].'/php/server/db.php'; | ||
| require_once $_SERVER['DOCUMENT_ROOT'].'/php/server/util.php'; |
jokob-sk
requested changes
Apr 27, 2026
| plugObj.status == "missing-in-last-scan" | ||
| or tmpObjFromEvent.status == "watched-changed" | ||
| ): | ||
| if plugObj.status == "missing-in-last-scan" or tmpObjFromEvent.status == "watched-changed": |
Collaborator
There was a problem hiding this comment.
these seem to be causing merge conflicts now
Member
Author
There was a problem hiding this comment.
Resolved locally. The merge-conflict hunk in server/plugin.py has been cleaned up and validated; this will be in the next commit.
| @@ -0,0 +1,574 @@ | |||
| { | |||
Collaborator
There was a problem hiding this comment.
README.md missing in the plugin folder
Member
Author
There was a problem hiding this comment.
Added front/plugins/auth_ldap/README.md with setup, settings, security notes, usage, and troubleshooting so the plugin now has a source-of-truth README. This will be in the next commit.
| value = str(get_setting_value(key_name) or "").strip() | ||
| if value: | ||
| mylog("warning", [f"[auth.ldap] {key_name} is stored in app.conf (plaintext). " | ||
| "Consider using the environment variable or Docker secrets instead."]) |
Collaborator
There was a problem hiding this comment.
do not suggest env variables - app.conf should be treated as sensitive data
|
|
||
| # Invalidate secondary cache from helper since conf.mySettings changed | ||
| try: | ||
| import helper |
Member
Author
There was a problem hiding this comment.
Resolved the inline import in server/initialise.py by moving the helper import to module scope. This will be in the next commit.
| # -------------------------- | ||
| # Auth endpoint | ||
| # -------------------------- | ||
| @app.route("/api/auth/login", methods=["POST"]) |
Collaborator
There was a problem hiding this comment.
@coderabbitai check if any security implications for this api path
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get the actual login endpoint implementation and rate-limiting helpers
rg -n "api_auth_login|FAILED_LOGINS|MAX_FAILED_ATTEMPTS|LOCKOUT_TIME|_get_client_ip|_sanitize_for_log|_failed_logins_lock" server/api_server/api_server_start.py -A 5 -B 2
echo "---CORS SECTION---"
rg -n "cors_origins|CORS_ORIGINS|\*" server/api_server/api_server_start.py -A 2 -B 2
echo "---LOGIN ROUTE---"
rg -n "api/auth/login" server/api_server/ -r -A 30
Length of output: 2044
🏁 Script executed:
#!/bin/bash
echo "=== LOGIN ENDPOINT ==="
rg -n "api/auth/login" server/api_server/api_server_start.py -A 40
echo ""
echo "=== RATE LIMITING ==="
rg -n "FAILED_LOGINS|MAX_FAILED_ATTEMPTS|LOCKOUT_TIME|_get_client_ip|_sanitize_for_log|failed_logins" server/
echo ""
echo "=== IS_AUTHORIZED FUNCTION ==="
rg -n "def is_authorized" server/api_server/api_server_start.py -A 20
echo ""
echo "=== CORS CONFIG ==="
sed -n '128,155p' server/api_server/api_server_start.py
echo ""
echo "=== LOGIN FUNCTION FULL ==="
rg -n "def login|def api_auth_login" server/api_server/api_server_start.py -A 30
Length of output: 4726
@jokob-sk Here is a detailed security review of the /api/auth/login path and its surrounding infrastructure:
🔐 Security Analysis: /api/auth/login
1. 🚨 No Rate Limiting / Brute-Force Protection (Critical)
The login function (Line 1953) calls AuthManager().authenticate() with no throttling. The AI summary described rate limiting with FAILED_LOGINS, MAX_FAILED_ATTEMPTS, and LOCKOUT_TIME, but none of these exist anywhere in the codebase — they were not committed. Any attacker who knows the API_TOKEN (which PHP must expose to call this endpoint) can make unlimited guesses against LDAP or local credentials.
Recommendation: Add per-IP rate limiting (e.g., flask-limiter or in-memory tracking with a lock) before this ships.
2. ⚠️ Login Endpoint Requires a Pre-Shared API_TOKEN (auth_callable=is_authorized)
Line 1951 gates the login route behind is_authorized, which checks the Bearer API_TOKEN. This creates a circular trust problem: the PHP frontend must know API_TOKEN to call the login API, making API_TOKEN effectively a shared secret embedded in the PHP process. If that token is ever logged or exposed, it bypasses the is_authorized gate entirely.
A login endpoint typically must be publicly reachable (with rate limiting as the guard), or isolated on a private internal network — not gated by a shared application token.
3. ⚠️ Silent Username Default to "admin" (Line 1954)
username = (payload.username or "admin").strip() if payload is not None else "admin"An empty string username silently becomes "admin". This means a caller sending {"username": "", "password": "..."} is silently redirected to attempt admin authentication, with no indication this happened. This could mask misconfigured clients and conflate their failures with actual admin brute-force attempts.
4. 🚨 CORS Wildcard + supports_credentials=True (Misconfiguration)
"*" # Allow all origins as last resort
...
CORS(app, resources={r"/*": {"origins": _cors_origins}}, supports_credentials=True, ...)The CORS spec and flask-cors explicitly forbid returning Access-Control-Allow-Origin: * when Access-Control-Allow-Credentials: true is set. Browsers will reject such responses, but the "*" wildcard in the list combined with credentialed requests can produce inconsistent behavior depending on the flask-cors version and browser. The "*" entry should be removed from the fallback list entirely.
5. ⚠️ Auth Token Accepted via Query String (Line 178)
query_token = request.args.get("token", "")Tokens in query strings appear in server logs, browser history, reverse-proxy logs, and Referer headers. For SSE streaming endpoints this may be unavoidable, but the token accepted via query string applies to all routes including /api/auth/login. Consider restricting query-string token auth to SSE-only routes.
6. ℹ️ Provider Disclosed in 401 Response (Line 1973)
"provider": result.provider,The 401 failure body reveals which authentication backend ("local" or "ldap") is configured, which is reconnaissance information for an attacker. Consider omitting or generalizing this field in failure responses.
Summary Table
| # | Issue | Severity |
|---|---|---|
| 1 | No brute-force / rate limiting | 🔴 Critical |
| 2 | Login endpoint gated by API_TOKEN | 🟠 High |
| 3 | Empty username silently becomes "admin" |
🟡 Medium |
| 4 | CORS wildcard "*" + supports_credentials=True |
🟠 High |
| 5 | Bearer token accepted via query string (all routes) | 🟡 Medium |
| 6 | provider field disclosed in 401 responses |
🔵 Low |
| import ldap3 # noqa: PLC0415 (deferred to avoid hard dep at import time) | ||
| except ImportError: | ||
| mylog("none", ["[auth.ldap] ldap3 package is not installed"]) | ||
| return AuthResult.fail(self.name, "LDAP library not available") |
Collaborator
There was a problem hiding this comment.
if any of tehse strings are shown in teh UI they should be added in en_us.json so they can be translated
@coderabbitai add learning about user-ui displayed text (logs are always in english only (so text in mylog calls is english only)
Contributor
There was a problem hiding this comment.
@jokob-sk, great point! In this snippet, the AuthResult.fail(...) error strings ("Username and password are required" and "LDAP library not available") are returned from authenticate() and can flow all the way to the login UI response — those would need translation keys in en_us.json if rendered to the user. The mylog(...) string ("[auth.ldap] ldap3 package is not installed") is log-only, so English-only is fine there.
The full ldap_provider.py has additional AuthResult.fail / AuthResult.ok error strings (e.g., "User not found", "LDAP infrastructure error", "Invalid credentials", etc.) that should similarly be reviewed — any that bubble up to the UI login page should have corresponding translation keys added to en_us.json.
I've added the learning as requested:
✏️ Learnings added
Learnt from: jokob-sk
Repo: netalertx/NetAlertX PR: 1621
File: server/auth/ldap_provider.py:97-97
Timestamp: 2026-04-27T00:51:56.385Z
Learning: In the NetAlertX codebase (Python server-side code), text inside `mylog(...)` calls is always in English only and does NOT need to be added to `en_us.json`. However, any string that is displayed in the user-facing UI (e.g., error messages returned in API responses that are rendered on the login page or elsewhere in the front-end) MUST be added as a translation key to `front/php/templates/language/en_us.json` so it can be translated via Weblate.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: jokob-sk
Repo: netalertx/NetAlertX PR: 1435
File: front/php/templates/language/sv_sv.json:300-305
Timestamp: 2026-01-20T23:53:07.712Z
Learning: Empty translation strings in non-English language files (e.g., sv_sv.json, de_de.json, etc.) should be ignored. Translations are managed externally via Weblate, and only en_us.json needs to contain actual translation values as the source language file.
Member
Author
There was a problem hiding this comment.
Added the current LDAP user-facing strings to front/php/templates/language/en_us.json so they have source-language entries for later UI mapping. This will be in the next commit.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
|
||
| return default_value | ||
|
|
||
| def get_secret(key_name: str) -> str: |
Collaborator
There was a problem hiding this comment.
unsure what thsi function is doing - all variables/settings should go thru app.conf - we can look at securing (encoding app.conf, introducing hashed setting values) later if needed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces a comprehensive LDAP and Active Directory authentication provider for NetAlertX.
Key features include:
(Authored in collaboration with AI)
Summary by CodeRabbit
New Features
Security Enhancements
Documentation
Tests