feat: LDAP / Active Directory login provider#2
Merged
adamoutler merged 12 commits intomainfrom Apr 18, 2026
Merged
Conversation
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>
- 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>
Copilot created this pull request from a session on behalf of
adamoutler
April 8, 2026 15:24
View session
Owner
|
@copilot resolve conflicts |
…n-feature # Conflicts: # requirements.txt # server/api_server/api_server_start.py Co-authored-by: adamoutler <527919+adamoutler@users.noreply.github.com>
Author
Conflicts resolved in
All 18 auth unit tests pass on the merged branch. |
adamoutler
approved these changes
Apr 8, 2026
…n bugs, add brute-force delay
…mprove diagnostic tooling
…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.
…onfiguration - 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
Owner
|
@coderabbitai Review the PR |
|
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements LDAP/Active Directory authentication support for NetAlertX with a pluggable provider system, secure session management via HMAC-validated cookies and CSRF tokens, rate-limited API login endpoint, new user database schema, comprehensive test coverage, and idle process notifications. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Frontend<br/>(index.php)
participant APIServer as API Server<br/>(api_server_start.py)
participant AuthMgr as AuthManager
participant LDAPProv as LdapProvider
participant LDAPServer as LDAP Server
participant LocalProv as LocalProvider
User->>Frontend: Enter username + password
Frontend->>Frontend: Validate CSRF token
Frontend->>APIServer: POST /api/auth/login<br/>(username, password)
APIServer->>APIServer: Check rate limit
APIServer->>AuthMgr: authenticate(username, password)
rect rgba(100, 150, 255, 0.5)
Note over AuthMgr: Check LDAP_enabled config
alt LDAP Enabled
AuthMgr->>LDAPProv: authenticate(username, password)
LDAPProv->>LDAPServer: Connect (LDAPS/StartTLS)
LDAPServer-->>LDAPProv: Connection ready
LDAPProv->>LDAPServer: Resolve user DN or direct bind
LDAPServer-->>LDAPProv: User DN found
LDAPProv->>LDAPServer: Bind as user with password
alt LDAP Success
LDAPServer-->>LDAPProv: Bind successful
LDAPProv-->>AuthMgr: AuthResult.ok(user, "ldap")
else LDAP User Not Found
LDAPServer-->>LDAPProv: No user
LDAPProv-->>AuthMgr: AuthResult.fail("user not found")
AuthMgr->>LocalProv: authenticate(username, password)
LocalProv-->>AuthMgr: AuthResult.ok/fail
else LDAP Failure
LDAPServer-->>LDAPProv: Bind failed
LDAPProv-->>AuthMgr: AuthResult.fail("invalid credentials")
end
else LDAP Disabled
AuthMgr->>LocalProv: authenticate(username, password)
LocalProv->>LocalProv: Verify SHA-256 hash
LocalProv-->>AuthMgr: AuthResult.ok(user, "local")
end
end
AuthMgr-->>APIServer: AuthResult
alt Authentication Success
APIServer->>APIServer: Clear rate limit counter
APIServer-->>Frontend: HTTP 200 + LoginResponse
Frontend->>Frontend: Set HMAC-validated cookie
Frontend->>User: Redirect to /devices.php
else Authentication Failure
APIServer->>APIServer: Increment rate limit counter
APIServer-->>Frontend: HTTP 401 + error
Frontend->>User: Show login error
end
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}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
ticket_idle_notification.md-1-8 (1)
1-8:⚠️ Potential issue | 🟡 MinorLooks like a stray planning artifact — should this be committed?
ticket_idle_notification.mdreads like an internal TODO/ticket for the very feature added in this PR (server/__main__.pyidle notification). It isn't referenced frommkdocs.ymlso it won't appear in the rendered docs, and it clutters the repo root.Consider moving the tracking to a GitHub Issue and removing this file, or — if you want to keep design notes — place it under
docs/and wire it intomkdocs.yml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ticket_idle_notification.md` around lines 1 - 8, ticket_idle_notification.md appears to be a stray planning artifact for the idle-notification feature implemented in server/__main__.py (see the idle notification trigger and write_notification integration); remove this file from the repo root and either (a) create a GitHub Issue to track the work, or (b) if you want to keep design notes, move the content into docs/ and add it to mkdocs.yml so it's included in site builds and not left as clutter in the root..env-22-22 (1)
22-22:⚠️ Potential issue | 🟡 MinorAvoid a realistic-looking password placeholder.
LDAP_BIND_PASSWORD=supersecretreads like a real credential in copy-pasted configs and — more practically — users sometimes leave template defaults in place. Use an obvious placeholder so misconfiguration is visible at a glance.🔧 Suggested change
-# LDAP_BIND_PASSWORD=supersecret +# LDAP_BIND_PASSWORD=CHANGE_ME🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env at line 22, The env template uses a realistic-looking password value for LDAP_BIND_PASSWORD which can be accidentally left in place; change the value for LDAP_BIND_PASSWORD to an obviously dummy placeholder (e.g. LDAP_BIND_PASSWORD=CHANGE_ME or LDAP_BIND_PASSWORD=<SET_LDAP_BIND_PASSWORD>) so it’s clearly not a real secret, and update any related README/example usage to reference the new placeholder..env-26-28 (1)
26-28:⚠️ Potential issue | 🟡 MinorRemove the extra blank line (dotenv-linter).
Static analysis flags an
ExtraBlankLineat line 27.🔧 Suggested change
# LDAP_USERNAME_ATTRIBUTE=uid - `#DEVELOPMENT` VARIABLES🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env around lines 26 - 28, Remove the extra blank line between the header and subsequent entries in the .env file (the blank line after "#DEVELOPMENT VARIABLES") to satisfy dotenv-linter's ExtraBlankLine rule; edit the .env block containing the "#DEVELOPMENT VARIABLES" header and delete the empty line so the header is immediately followed by the next variable or comment.server/__main__.py-103-106 (1)
103-106:⚠️ Potential issue | 🟡 MinorInitial startup will immediately emit an "all idle" notification.
was_idle = Falseis set before the first loop, so the first iteration in whichis_idle_this_loopstaysTrue(which is very likely right after startup onceimportConfigsandupdate_apiare the only work done and the 1-minute scan gate hasn't elapsed yet) will immediately firewrite_notification("All processes are idle", "info"). This creates a confusing “system idle” notification moments after the user starts the app.Consider seeding
was_idle = Trueinitially (or gating the first N iterations) so the notification only fires after the system has actually done work and then returned to idle.🔧 Suggested change
- was_idle = False + # Treat startup as "already notified" so we don't emit an idle notification + # before the system has done any real work. + was_idle = True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/__main__.py` around lines 103 - 106, The loop currently initializes was_idle = False causing the very first idle check to emit an “All processes are idle” notification; change the initialization to seed was_idle = True (or add a first-run gate counter) so the first idle detection doesn't immediately call write_notification("All processes are idle", "info")—update the variable initialization near was_idle and adjust any logic that depends on is_idle_this_loop so notifications only fire after the system has performed work and then returned to idle.docs/LDAP_CONFIGURATION.md-70-76 (1)
70-76:⚠️ Potential issue | 🟡 MinorAdd languages to the fenced filter examples.
markdownlintflags these fences; usetextorldifso docs checks stay green.📝 Proposed fix
- ``` + ```text (&(memberOf=CN=NetAlertX,OU=Groups,DC=example,DC=com)(sAMAccountName={username}))@@
(&(sAMAccountName=my_username)(sAMAccountName={username}))</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/LDAP_CONFIGURATION.mdaround lines 70 - 76, Update the fenced code
blocks that show LDAP filters so they include a language tag (e.g., text or
ldif) to satisfy markdownlint; specifically add a language after the backticks
for the filters
"(&(memberOf=CN=NetAlertX,OU=Groups,DC=example,DC=com)(sAMAccountName={username}))"
and "(&(sAMAccountName=my_username)(sAMAccountName={username}))" so the fences
becometext (orldif) instead of plain ```.</details> </blockquote></details> <details> <summary>test/api_endpoints/test_auth_endpoints.py-137-140 (1)</summary><blockquote> `137-140`: _⚠️ Potential issue_ | _🟡 Minor_ **Update test to expect `405` instead of `404` for GET requests to POST-only route.** The `/api/auth/login` route is registered with `methods=["POST"]` only. Standard Flask behavior returns `405 Method Not Allowed` (not `404`) when a client makes a GET request to a registered route that doesn't support that method. <details> <summary>Proposed fix</summary> ```diff def test_login_endpoint_requires_post(client): - """GET /api/auth/login should return 404 (route not registered for GET).""" + """GET /api/auth/login should return 405 because the route is POST-only.""" resp = client.get("/api/auth/login") - assert resp.status_code == 404 + assert resp.status_code == 405 ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@test/api_endpoints/test_auth_endpoints.py` around lines 137 - 140, The test test_login_endpoint_requires_post currently asserts that a GET to "/api/auth/login" returns 404 but the route is registered with methods=["POST"], so update the assertion in test_login_endpoint_requires_post to expect status_code 405 instead of 404; change the assertion for resp.status_code in that function to assert resp.status_code == 405 to match Flask's Method Not Allowed behavior. ``` </details> </blockquote></details> <details> <summary>front/plugins/auth_ldap/config.json-20-24 (1)</summary><blockquote> `20-24`: _⚠️ Potential issue_ | _🟡 Minor_ **Align the security copy with actual fallback behavior.** The description says the local password works as fallback when LDAP is disabled, but `AuthManager` also falls back when LDAP is enabled and the user is not found unless `disable_local_admin` is enabled. Make this explicit so admins understand the default security posture. <details> <summary>📝 Suggested wording adjustment</summary> ```diff - "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." + "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. Unless local admin is disabled below, the local password (SETPWD) may still be accepted as a fallback for users not found in LDAP. <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." ``` </details> Also applies to: 304-308 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@front/plugins/auth_ldap/config.json` around lines 20 - 24, Update the LDAP description to accurately reflect fallback behavior: state that local password (SETPWD) is not only a fallback when LDAP is disabled but also used when LDAP is enabled and a user record is missing unless AuthManager is configured with disable_local_admin=true; keep the credential-security guidance about using LDAP_BIND_PASSWORD or Docker secrets and enforcing TLS/LDAPS/StartTLS. Reference AuthManager and the disable_local_admin setting so admins know the default security posture and how to change it. ``` </details> </blockquote></details> <details> <summary>server/auth/ldap_provider.py-286-289 (1)</summary><blockquote> `286-289`: _⚠️ Potential issue_ | _🟡 Minor_ **Use the existing log sanitizer consistently.** These log paths include raw `username`, `search_filter`, and `user_dn`; a username containing control characters can forge log lines. `_sanitize_for_log()` already exists—apply it here too. <details> <summary>🛡️ Proposed fix</summary> ```diff mylog("verbose", [ - f"[auth.ldap] User '{username}' not found " - f"(got {len(entries)} entries for filter {search_filter})" + f"[auth.ldap] User '{_sanitize_for_log(username)}' not found " + f"(got {len(entries)} entries for filter {_sanitize_for_log(search_filter)})" ]) @@ - mylog("verbose", [f"[auth.ldap] User bind failed for DN '{user_dn}': {conn.result}"]) + mylog("verbose", [f"[auth.ldap] User bind failed for DN '{_sanitize_for_log(user_dn)}': {conn.result}"]) ``` </details> Also applies to: 316-316 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@server/auth/ldap_provider.py` around lines 286 - 289, The mylog calls in ldap_provider.py are logging raw username, search_filter, and user_dn which can contain control chars; use the existing _sanitize_for_log() before interpolating these values. Change the mylog invocation(s) that reference username and search_filter (and the other call that logs user_dn) to pass sanitized_username = _sanitize_for_log(username), sanitized_filter = _sanitize_for_log(search_filter) (and sanitized_user_dn = _sanitize_for_log(user_dn)) and interpolate those sanitized variables into the f-strings so all logged values go through _sanitize_for_log() consistently. ``` </details> </blockquote></details> <details> <summary>test/backend/test_auth_providers.py-139-139 (1)</summary><blockquote> `139-139`: _⚠️ Potential issue_ | _🟡 Minor_ **Fix the Ruff violations in the new test file.** Line 139 has an unnecessary f-string, and the inline `if: return` statements are flagged by E701. <details> <summary>🧹 Proposed fix</summary> ```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 == "LDAP_enabled": return True - if key == "SETPWD_enable_password": return True - if key == "LDAP_disable_local_admin": return True + if key == "LDAP_enabled": + return True + if key == "SETPWD_enable_password": + return True + if key == "LDAP_disable_local_admin": + return True return False @@ def mock_settings(key): - if key == "LDAP_enabled": return True - if key == "SETPWD_enable_password": return True - if key == "LDAP_disable_local_admin": return False + if key == "LDAP_enabled": + return True + if key == "SETPWD_enable_password": + return True + if key == "LDAP_disable_local_admin": + return False return False ``` </details> Also applies to: 266-269, 284-287 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@test/backend/test_auth_providers.py` at line 139, The test file has ruff violations: remove the unnecessary f-string used in MagicMock(entry_dn=...) by changing f"uid=alice,dc=example,dc=com" to a plain string "uid=alice,dc=example,dc=com" (affects the fake_entries list creation using MagicMock and any similar MagicMock(entry_dn=...) occurrences), and replace any inline single-line conditional returns (patterns like if <cond>: return) with a proper multi-line if block (if <cond>: newline return) in the test functions so they no longer trigger E701; update the instances referenced around the fake_entries use and the nearby tests that contain inline if:return statements accordingly. ``` </details> </blockquote></details> <details> <summary>server/auth/manager.py-61-61 (1)</summary><blockquote> `61-61`: _⚠️ Potential issue_ | _🟡 Minor_ **Remove the unnecessary f-string prefix.** Line 61 has no interpolation and is flagged by Ruff F541. ```diff - mylog("verbose", [f"[auth.manager] User not found in LDAP, but local fallback is disabled."]) + mylog("verbose", ["[auth.manager] User not found in LDAP, but local fallback is disabled."]) ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@server/auth/manager.py` at line 61, The logged message in the mylog call uses an unnecessary f-string (Ruff F541); update the call to mylog("verbose", ["[auth.manager] User not found in LDAP, but local fallback is disabled."]) by removing the leading f before the quoted string so it is a plain string literal; locate the mylog invocation in auth.manager (the message shown) and make this small change. ``` </details> </blockquote></details> <details> <summary>server/auth/ldap_provider.py-166-185 (1)</summary><blockquote> `166-185`: _⚠️ Potential issue_ | _🟡 Minor_ **Replace `==` with `is` for type comparisons (Ruff E721).** `type_cast == bool` violates PEP 8 style conventions; use identity comparison `type_cast is bool` for type singleton objects (lines 169 and 179). <details> <summary>Suggested changes</summary> ```diff - if type_cast == bool: + if type_cast is bool: return str(env_val).lower() in ("true", "1", "yes") @@ - if type_cast == bool: + if type_cast is bool: return str(db_val).lower() in ("true", "1", "yes") if isinstance(db_val, str) else bool(db_val) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@server/auth/ldap_provider.py` around lines 166 - 185, In get_env_or_setting, replace the equality comparisons against the bool type (currently written as "type_cast == bool") with identity comparisons ("type_cast is bool") in both the environment-value branch and the db-value branch so type singleton comparison uses "is"; update the two occurrences referencing the type_cast variable to use "is" while keeping the existing boolean parsing logic unchanged. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>server/db/db_upgrade.py (1)</summary><blockquote> `651-663`: **`usrCreatedAt` is `NOT NULL` with no default — inserts without an explicit value will fail.** Every row must supply `usrCreatedAt` or the insert raises `NOT NULL constraint failed`. Since the rest of the schema uses app-provided UTC strings, either add a sensible default or rely on callers — but the current shape is a foot-gun given that seeding/bootstrap code for Users isn't in this PR. Consider adding `DEFAULT (strftime('%Y-%m-%d %H:%M:%S','now'))` so the column is self-populating and consistent with the UTC convention used elsewhere in this module. <details> <summary>♻️ Proposed change</summary> ```diff - usrCreatedAt TEXT NOT NULL, + usrCreatedAt TEXT NOT NULL DEFAULT (strftime('%Y-%m-%d %H:%M:%S','now')), usrLastLogin TEXT ``` </details> Also consider: `usrRole` is a free-form `TEXT` with no `CHECK` constraint, unlike `usrEnabled`. If only `'admin'` / `'user'` (or similar) are valid, a `CHECK (usrRole IN (...))` would mirror the style already used for `usrEnabled` and prevent typos from authentication code writing arbitrary roles. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@server/db/db_upgrade.py` around lines 651 - 663, The Users table definition sets usrCreatedAt as NOT NULL with no default which will break inserts; update the CREATE TABLE statement that defines Users to give usrCreatedAt a UTC default (e.g. DEFAULT (strftime('%Y-%m-%d %H:%M:%S','now'))) so rows auto-populate created timestamps, and add a CHECK on usrRole (e.g. CHECK (usrRole IN ('admin','user'))) to constrain role values similar to usrEnabled; locate and modify the CREATE TABLE block that declares usrID, usrUsername, usrPasswordHash, usrRole, usrAuthProvider, usrEnabled, usrCreatedAt, and usrLastLogin to apply these changes. ``` </details> </blockquote></details> <details> <summary>server/api_server/openapi/schemas.py (1)</summary><blockquote> `171-184`: **Consider using `Literal["local", "ldap"]` for `provider`.** The description constrains values to `'local'` or `'ldap'`, but the field is typed as free-form `Optional[str]`. Using a `Literal` would enforce the contract in both validation and generated OpenAPI/MCP schemas, mirroring the pattern already used elsewhere in this file (e.g. `ALLOWED_NMAP_MODES`, `NOTIFICATION_LEVELS`). <details> <summary>♻️ Suggested change</summary> ```diff - username: Optional[str] = Field(None, description="Authenticated username") - provider: Optional[str] = Field(None, description="Provider that authenticated the user: 'local' or 'ldap'") + username: Optional[str] = Field(None, description="Authenticated username") + provider: Optional[Literal["local", "ldap"]] = Field( + None, description="Provider that authenticated the user" + ) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@server/api_server/openapi/schemas.py` around lines 171 - 184, The LoginResponse.provider is typed as Optional[str] but constrained to "local" or "ldap"; change its annotation to Optional[Literal["local", "ldap"]] to enforce validation and correct OpenAPI schema, add the required Literal import (from typing or typing_extensions consistent with other enums in this module), and update any examples if needed in LoginResponse.model_config to match the Literal values; locate the provider field on the LoginResponse class to apply these changes. ``` </details> </blockquote></details> <details> <summary>requirements.txt (1)</summary><blockquote> `37-37`: **Pin the `ldap3` dependency to a minimum version.** `ldap3` is on the authentication critical path (LDAP bind, TLS/StartTLS). An unpinned dependency means a future breaking release could silently regress auth behavior. Pin to at least a floor version (e.g., `ldap3>=2.9`) to match the pinning style used for `pydantic` and `fritzconnection` elsewhere in the file. <details> <summary>🔧 Suggested change</summary> ```diff -ldap3 +ldap3>=2.9 ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@requirements.txt` at line 37, Update the unpinned ldap3 entry in requirements.txt to a minimum pinned version (for example change "ldap3" to "ldap3>=2.9") so the dependency is not floating; follow the same pinning style used for other packages like pydantic and fritzconnection to ensure future breaking releases do not regress authentication behavior. ``` </details> </blockquote></details> <details> <summary>front/plugins/auth_ldap/config.json (1)</summary><blockquote> `552-552`: **Avoid shell-style quoting in plugin CMD placeholders.** `server/plugin.py` expands this command and then calls `cmd_str.split()`, so quotes are not honored. A test username containing spaces will be split into multiple argv entries. Prefer no embedded quotes here, or update the runner to use `shlex.split()`. ```diff - "default_value": "python3 /app/front/plugins/auth_ldap/test_ldap.py --test-user=\"{LDAP_test_username}\"", + "default_value": "python3 /app/front/plugins/auth_ldap/test_ldap.py --test-user={LDAP_test_username}", ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@front/plugins/auth_ldap/config.json` at line 552, The default_value in front/plugins/auth_ldap/config.json embeds shell-style quotes which are lost because server/plugin.py uses cmd_str.split(); update the config to remove embedded quotes so placeholders won't rely on shell parsing (e.g., change --test-user="..." to --test-user={LDAP_test_username}) or alternatively modify server/plugin.py to parse commands robustly by replacing cmd_str.split() with shlex.split(cmd_str) so quoted placeholders are preserved; locate the "default_value" string in the config and the cmd_str.split() usage in server/plugin.py (or both) and apply one of these fixes. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env:
- Around line 11-26: The .env template uses UPPERCASE LDAP_* keys but
ldap_provider.py reads mixed-case names (e.g. LDAP_enabled, LDAP_server,
LDAP_port, LDAP_use_ssl, LDAP_use_start_tls, LDAP_tls_verify_cert,
LDAP_ca_cert_path, LDAP_disable_local_admin, LDAP_direct_bind_format,
LDAP_bind_dn, LDAP_bind_password, LDAP_base_dn, LDAP_user_filter,
LDAP_username_attribute), so update the .env file to replace the uppercase keys
with those exact mixed-case keys to match what ldap_provider.py reads; ensure
each key name in the file exactly matches the getenv keys in ldap_provider.py so
environment values are picked up on case-sensitive systems.In
@docker-compose.yml:
- Around line 3-11: The docker-compose change removed host networking
(network_mode: host) which breaks ARP/LAN scanning and hard-coded the port
mappings ("20211:20211", "20212:20212") instead of honoring configurable PORT /
GRAPHQL_PORT; either restore network_mode: host as the default for the netalertx
service (re-enable the existing network_mode: host comment) to preserve
ARP-based discovery, or keep bridge mode but update the ports section to use the
configured environment variables (PORT and GRAPHQL_PORT) and add a clarifying
comment that bridge mode reduces ARP-based discovery; target the service block
containing container_name: netalertx, network_mode, and ports when making the
change.In
@docs/LDAP_CONFIGURATION.md:
- Around line 50-54: Update the "Boot-Once Configuration" paragraph so it does
not describe saving LDAP_BIND_PASSWORD in the Web UI as "safe"; instead state
that the UI persists the bind password in plaintext (a convenience only) and
recommend using Docker secrets or environment injection (e.g.,
LDAP_BIND_PASSWORD via /run/secrets or env at deploy time) for long-lived
deployments; retain the TLS/CA notes (LDAP_TLS_VERIFY_CERT, LDAP_CA_CERT_PATH)
and explicitly call out the plaintext tradeoff for the Web UI (Settings > LDAP
Authentication) and that the UI fallback is a convenience, not a secure secret
store.In
@front/php/templates/auth.php:
- Around line 36-40: The current logic computes $api_token as '' when API_TOKEN
is missing, which allows HMAC validation with an empty key; change the flow in
the code that builds $api_token and validates the cookie (symbols: $api_token,
API_TOKEN, $expected_cookie, $nax_Password, $CookieSaveLoginName) so that if
API_TOKEN is not found or $api_token is empty you skip HMAC validation entirely
and treat "remember me" as unavailable (optionally clear/unset
$_COOKIE[$CookieSaveLoginName] or ignore it) instead of computing hash_hmac with
an empty key and accepting the cookie.In
@front/php/templates/language/en_us.json:
- Line 398: Add the missing "Login_Username" translation key to all locale JSON
files that lack it (the other 23 locale files such as de_de.json, es_es.json,
fr_fr.json, it_it.json, pt_pt.json, pt_br.json, ca_ca.json, tr_tr.json,
sv_sv.json, nb_no.json, pl_pl.json, cs_cz.json, fi_fi.json, ru_ru.json,
uk_ua.json, ja_jp.json, zh_cn.json, vi_vn.json, id_id.json, ar_ar.json,
fa_fa.json, he_il.json) by inserting "Login_Username": "Username" (or the proper
localized translation) into each file so LDAP displays the translated label
instead of the raw key; if a translator isn't available, use the English
placeholder value to maintain UX consistency.In
@front/php/templates/security.php:
- Around line 57-63: The logout cookie deletion uses '!empty($_SERVER['HTTPS'])'
which treats "off" as secure; update the Secure flag expression in the setcookie
call for COOKIE_SAVE_LOGIN_NAME to use the same HTTPS detection used elsewhere
(e.g. replace '!empty($_SERVER['HTTPS'])' with a check like
'isset($_SERVER['HTTPS']) && strtolower($_SERVER['HTTPS']) !== "off"' or the
project's equivalent helper) so the removal cookie will not set Secure when the
request is over plain HTTP.In
@front/plugins/auth_ldap/test_ldap.py:
- Around line 47-69: The LDAP test block currently prints errors for "user not
found" and "multiple entries" but always exits zero; modify the args.test_user
handling so you track the lookup outcome (e.g., a boolean or exit_code variable)
when inspecting entries after conn.search, call conn.unbind() as you already do,
and then exit with non-zero (e.g., sys.exit(1)) on failure cases (len(entries)
== 0 or >1) and sys.exit(0) on success; reference the args.test_user branch, the
conn.search / entries check, and conn.unbind to ensure unbind runs before
calling sys.exit.In
@server/__main__.py:
- Around line 272-278: The idle notification is noisy; change the idle
transition handling so write_notification is only called if the system has just
transitioned to idle and the last idle notification was sent more than a
cooldown (e.g., 1 hour) ago. Add a module-level variable like
last_idle_notification_ts and a constant IDLE_NOTIFICATION_COOLDOWN_HOURS, then
in the block that checks is_idle_this_loop/was_idle (using the existing was_idle
and is_idle_this_loop) only call write_notification and update
last_idle_notification_ts when was_idle is False and now -
last_idle_notification_ts > cooldown; still set was_idle True on the transition
and reset was_idle False when not idle. Use mylog("verbose", ...) unchanged for
verbosity.In
@server/api_server/api_server_start.py:
- Around line 1964-1969: The _get_client_ip function currently trusts any
X-Forwarded-For header; change it to only accept X-Forwarded-For when the
immediate remote_addr is a trusted proxy (e.g., loopback/known proxy IPs) and
otherwise fall back to request.remote_addr; update the function _get_client_ip
to check request.remote_addr against a trusted set (including 127.0.0.1/::1 and
configured proxy IPs) before parsing X-Forwarded-For, and ensure any callers
that perform login throttling use this hardened _get_client_ip; also update the
PHP front/index.php caller to include the browser IP (e.g., forward the client
IP to the API call in a header only when making server-side requests) so UI
logins are attributed to the real browser IP rather than 127.0.0.1.In
@server/auth/ldap_provider.py:
- Around line 246-249: The code calls conn.start_tls() when cfg["use_start_tls"]
and not cfg["use_ssl"] but ignores its return value so a failed TLS negotiation
can go unnoticed; update the logic around conn.start_tls() and conn.bind() to
check the boolean result of conn.start_tls() and fail closed (abort/raise/log
and do not call conn.bind()) if start_tls() returns False or throws, ensuring
credentials are never sent without successful TLS; reference the
cfg["use_start_tls"], cfg["use_ssl"], conn.start_tls(), and conn.bind() symbols
when applying the change.In
@server/auth/local_provider.py:
- Around line 33-35: Add explicit empty-username rejection in the authenticate
flow: in authenticate(...) (and the other similar block at lines referenced)
check if not username and immediately return AuthResult.fail(self.name,
"Username must not be empty") before any password checks or local-fallback logic
so an empty username cannot succeed (e.g., when password matches "admin").
Ensure both occurrences use the same validation and message.In
@server/auth/manager.py:
- Around line 52-59: The fallback to LocalProvider() is currently unguarded and
lets any LDAP-missing username log in with the local password; update the logic
in the authenticate flow that checks ldap_result.error == "User not found" so
you only call LocalProvider().authenticate(username, password) when the
requested username matches the intended local admin identity (e.g., compare
username to the canonical/local admin username from configuration or
canonicalize the supplied username first), otherwise do not attempt local auth
and return/propagate the ldap failure; ensure you keep the existing logging via
mylog and continue to sanitize username with _sanitize_for_log when logging.In
@server/plugin.py:
- Around line 256-270: The command string replacement logic for plugin settings
(variables built into cmd_str via prefix/wildcard from plugin.get("settings"))
is being split with cmd_str.split(), which preserves literal quotes and breaks
quoted args; change the subprocess invocation to parse cmd_str with
shlex.split() before passing to subprocess.check_output (still using shell=False
and preserving stderr/timeout behavior) so quoted values like --test-user="jdoe"
become the correct token --test-user=jdoe; ensure shlex is imported and replace
the cmd_str.split() call referenced in the subprocess.check_output invocation.In
@test/backend/test_auth_providers.py:
- Around line 236-278: The tests currently patch auth.manager.get_setting_value
but LdapProvider reads LDAP_enabled via its own _read_config(), so update the
tests (e.g., test_uses_ldap_when_ldap_enabled and
test_authenticate_uses_ldap_when_ldap_enabled) to patch
LdapProvider._read_config to return a config dict where "LDAP_enabled" (and any
other relevant flags like "SETPWD_enable_password" and
"LDAP_disable_local_admin") are True; similarly patch _read_config to return
LDAP_enabled False for the LDAP-disabled tests; keep existing patches for
LocalProvider.authenticate / LdapProvider.authenticate and use AuthManager as
before so the LDAP branches are actually exercised.In
@test/docker_tests/test_ldap_ui.py:
- Around line 56-70: The test writes LDAP settings into config_file but the
compose service mounts a fresh named volume over the container data path so
those settings never reach the container; update the docker-compose test setup
to bind-mount the prepared tmp_path data directory into the container data path
(instead of/alongside the named volume) so the edited app.conf is used at
runtime, and also rename the stale LDAP config keys in the writes (the lines
that set LDAP_enabled, LDAP_server, LDAP_port, LDAP_base_dn, LDAP_user_filter,
LDAP_login_attribute, LDAP_bind_dn, LDAP_bind_password, LDAP_use_ssl,
LDAP_use_starttls) to the current expected key names used by the app; apply the
same mount and key-name fixes for the second block referenced around lines
113-119.In
@test/ui/test_ui_ldap_login.py:
- Around line 25-28: The test currently calls pytest.skip when the LDAP username
field isn't found; instead make it fail so the LDAP integration path is
verified: replace the skip branch that checks username_field (result of
driver.find_elements(By.NAME, "loginusername")) with a failure (e.g.,
pytest.fail or an assertion) including a clear message that the LDAP login field
"loginusername" is missing and that LDAP UI rendering/setup likely failed;
update any surrounding logic in test_ui_ldap_login.py that depends on
username_field to assume the element must exist.
Minor comments:
In @.env:
- Line 22: The env template uses a realistic-looking password value for
LDAP_BIND_PASSWORD which can be accidentally left in place; change the value for
LDAP_BIND_PASSWORD to an obviously dummy placeholder (e.g.
LDAP_BIND_PASSWORD=CHANGE_ME or LDAP_BIND_PASSWORD=<SET_LDAP_BIND_PASSWORD>) so
it’s clearly not a real secret, and update any related README/example usage to
reference the new placeholder.- Around line 26-28: Remove the extra blank line between the header and
subsequent entries in the .env file (the blank line after "#DEVELOPMENT
VARIABLES") to satisfy dotenv-linter's ExtraBlankLine rule; edit the .env block
containing the "#DEVELOPMENT VARIABLES" header and delete the empty line so the
header is immediately followed by the next variable or comment.In
@docs/LDAP_CONFIGURATION.md:
- Around line 70-76: Update the fenced code blocks that show LDAP filters so
they include a language tag (e.g., text or ldif) to satisfy markdownlint;
specifically add a language after the backticks for the filters
"(&(memberOf=CN=NetAlertX,OU=Groups,DC=example,DC=com)(sAMAccountName={username}))"
and "(&(sAMAccountName=my_username)(sAMAccountName={username}))" so the fences
becometext (orldif) instead of plain ```.In
@front/plugins/auth_ldap/config.json:
- Around line 20-24: Update the LDAP description to accurately reflect fallback
behavior: state that local password (SETPWD) is not only a fallback when LDAP is
disabled but also used when LDAP is enabled and a user record is missing unless
AuthManager is configured with disable_local_admin=true; keep the
credential-security guidance about using LDAP_BIND_PASSWORD or Docker secrets
and enforcing TLS/LDAPS/StartTLS. Reference AuthManager and the
disable_local_admin setting so admins know the default security posture and how
to change it.In
@server/__main__.py:
- Around line 103-106: The loop currently initializes was_idle = False causing
the very first idle check to emit an “All processes are idle” notification;
change the initialization to seed was_idle = True (or add a first-run gate
counter) so the first idle detection doesn't immediately call
write_notification("All processes are idle", "info")—update the variable
initialization near was_idle and adjust any logic that depends on
is_idle_this_loop so notifications only fire after the system has performed work
and then returned to idle.In
@server/auth/ldap_provider.py:
- Around line 286-289: The mylog calls in ldap_provider.py are logging raw
username, search_filter, and user_dn which can contain control chars; use the
existing _sanitize_for_log() before interpolating these values. Change the mylog
invocation(s) that reference username and search_filter (and the other call that
logs user_dn) to pass sanitized_username = _sanitize_for_log(username),
sanitized_filter = _sanitize_for_log(search_filter) (and sanitized_user_dn =
_sanitize_for_log(user_dn)) and interpolate those sanitized variables into the
f-strings so all logged values go through _sanitize_for_log() consistently.- Around line 166-185: In get_env_or_setting, replace the equality comparisons
against the bool type (currently written as "type_cast == bool") with identity
comparisons ("type_cast is bool") in both the environment-value branch and the
db-value branch so type singleton comparison uses "is"; update the two
occurrences referencing the type_cast variable to use "is" while keeping the
existing boolean parsing logic unchanged.In
@server/auth/manager.py:
- Line 61: The logged message in the mylog call uses an unnecessary f-string
(Ruff F541); update the call to mylog("verbose", ["[auth.manager] User not found
in LDAP, but local fallback is disabled."]) by removing the leading f before the
quoted string so it is a plain string literal; locate the mylog invocation in
auth.manager (the message shown) and make this small change.In
@test/api_endpoints/test_auth_endpoints.py:
- Around line 137-140: The test test_login_endpoint_requires_post currently
asserts that a GET to "/api/auth/login" returns 404 but the route is registered
with methods=["POST"], so update the assertion in
test_login_endpoint_requires_post to expect status_code 405 instead of 404;
change the assertion for resp.status_code in that function to assert
resp.status_code == 405 to match Flask's Method Not Allowed behavior.In
@test/backend/test_auth_providers.py:
- Line 139: The test file has ruff violations: remove the unnecessary f-string
used in MagicMock(entry_dn=...) by changing f"uid=alice,dc=example,dc=com" to a
plain string "uid=alice,dc=example,dc=com" (affects the fake_entries list
creation using MagicMock and any similar MagicMock(entry_dn=...) occurrences),
and replace any inline single-line conditional returns (patterns like if :
return) with a proper multi-line if block (if : newline return) in the
test functions so they no longer trigger E701; update the instances referenced
around the fake_entries use and the nearby tests that contain inline if:return
statements accordingly.In
@ticket_idle_notification.md:
- Around line 1-8: ticket_idle_notification.md appears to be a stray planning
artifact for the idle-notification feature implemented in server/main.py
(see the idle notification trigger and write_notification integration); remove
this file from the repo root and either (a) create a GitHub Issue to track the
work, or (b) if you want to keep design notes, move the content into docs/ and
add it to mkdocs.yml so it's included in site builds and not left as clutter in
the root.
Nitpick comments:
In@front/plugins/auth_ldap/config.json:
- Line 552: The default_value in front/plugins/auth_ldap/config.json embeds
shell-style quotes which are lost because server/plugin.py uses cmd_str.split();
update the config to remove embedded quotes so placeholders won't rely on shell
parsing (e.g., change --test-user="..." to --test-user={LDAP_test_username}) or
alternatively modify server/plugin.py to parse commands robustly by replacing
cmd_str.split() with shlex.split(cmd_str) so quoted placeholders are preserved;
locate the "default_value" string in the config and the cmd_str.split() usage in
server/plugin.py (or both) and apply one of these fixes.In
@requirements.txt:
- Line 37: Update the unpinned ldap3 entry in requirements.txt to a minimum
pinned version (for example change "ldap3" to "ldap3>=2.9") so the dependency is
not floating; follow the same pinning style used for other packages like
pydantic and fritzconnection to ensure future breaking releases do not regress
authentication behavior.In
@server/api_server/openapi/schemas.py:
- Around line 171-184: The LoginResponse.provider is typed as Optional[str] but
constrained to "local" or "ldap"; change its annotation to
Optional[Literal["local", "ldap"]] to enforce validation and correct OpenAPI
schema, add the required Literal import (from typing or typing_extensions
consistent with other enums in this module), and update any examples if needed
in LoginResponse.model_config to match the Literal values; locate the provider
field on the LoginResponse class to apply these changes.In
@server/db/db_upgrade.py:
- Around line 651-663: The Users table definition sets usrCreatedAt as NOT NULL
with no default which will break inserts; update the CREATE TABLE statement that
defines Users to give usrCreatedAt a UTC default (e.g. DEFAULT
(strftime('%Y-%m-%d %H:%M:%S','now'))) so rows auto-populate created timestamps,
and add a CHECK on usrRole (e.g. CHECK (usrRole IN ('admin','user'))) to
constrain role values similar to usrEnabled; locate and modify the CREATE TABLE
block that declares usrID, usrUsername, usrPasswordHash, usrRole,
usrAuthProvider, usrEnabled, usrCreatedAt, and usrLastLogin to apply these
changes.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `eb390fbc-076e-4e3c-a362-f51f6a5f6a89` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 2e8b793b4415961b55c6396e6f6fe2f3d3314e97 and 57d90045712c49d9a92a22a0dcb25b0b1e5dbdc8. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `docker_build.log` is excluded by `!**/*.log` </details> <details> <summary>📒 Files selected for processing (30)</summary> * `.env` * `docker-compose.yml` * `docs/LDAP_CONFIGURATION.md` * `front/index.php` * `front/php/templates/auth.php` * `front/php/templates/language/en_us.json` * `front/php/templates/security.php` * `front/plugins/auth_ldap/config.json` * `front/plugins/auth_ldap/test_ldap.py` * `mkdocs.yml` * `requirements.txt` * `server/__main__.py` * `server/api_server/api_server_start.py` * `server/api_server/openapi/schemas.py` * `server/auth/__init__.py` * `server/auth/base.py` * `server/auth/ldap_provider.py` * `server/auth/local_provider.py` * `server/auth/manager.py` * `server/database.py` * `server/db/db_upgrade.py` * `server/plugin.py` * `test/api_endpoints/test_auth_endpoints.py` * `test/backend/test_auth_providers.py` * `test/docker_tests/test_ldap_ui.py` * `test/server/test_ldap_provider.py` * `test/server/test_ldap_provider_env.py` * `test/server/test_plugin_handle_test.py` * `test/ui/test_ui_ldap_login.py` * `ticket_idle_notification.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+11
to
+26
| # LDAP Authentication Settings | ||
| # LDAP_ENABLED=true | ||
| # LDAP_SERVER=ldap.example.com | ||
| # LDAP_PORT=389 | ||
| # LDAP_USE_SSL=false | ||
| # LDAP_USE_START_TLS=false | ||
| # LDAP_TLS_VERIFY_CERT=true | ||
| # LDAP_CA_CERT_PATH=/etc/ssl/certs/ca-certificates.crt | ||
| # LDAP_DISABLE_LOCAL_ADMIN=false | ||
| # LDAP_DIRECT_BIND_FORMAT=uid={username},ou=users,dc=example,dc=com | ||
| # LDAP_BIND_DN=cn=readonly,dc=example,dc=com | ||
| # LDAP_BIND_PASSWORD=supersecret | ||
| # LDAP_BASE_DN=ou=users,dc=example,dc=com | ||
| # LDAP_USER_FILTER=(uid={username}) | ||
| # LDAP_USERNAME_ATTRIBUTE=uid | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare the env keys declared in .env against what ldap_provider.py reads from os.environ.
set -e
echo "=== Keys declared in .env (LDAP_*) ==="
grep -oE '^# ?LDAP_[A-Za-z_]+' .env | sed 's/^# *//' | sort -u
echo
echo "=== os.environ.* / os.getenv(...) usages in auth/ldap code ==="
rg -nP --type=py "os\.(environ(\.get)?\[?|getenv)\s*\(?['\"]LDAP_[A-Za-z_]+['\"]" server/auth/ front/plugins/auth_ldap/ 2>/dev/null || true
echo
echo "=== Any LDAP_* literal in server/auth ==="
rg -nP --type=py "['\"]LDAP_[A-Za-z_]+['\"]" server/auth/Repository: adamoutler/NetAlertX
Length of output: 2090
Fix env variable names to match what ldap_provider.py actually reads.
The .env template uses UPPERCASE keys (LDAP_ENABLED, LDAP_SERVER, LDAP_PORT, etc.), but ldap_provider.py reads mixed-case keys (LDAP_enabled, LDAP_server, LDAP_port, etc.). On case-sensitive systems, these won't match—users will set values in the template but the application will silently ignore them.
Update the .env keys to use the correct mixed-case naming: LDAP_enabled, LDAP_server, LDAP_port, LDAP_use_ssl, LDAP_use_start_tls, LDAP_tls_verify_cert, LDAP_ca_cert_path, LDAP_disable_local_admin, LDAP_direct_bind_format, LDAP_bind_dn, LDAP_bind_password, LDAP_base_dn, LDAP_user_filter, LDAP_username_attribute.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env around lines 11 - 26, The .env template uses UPPERCASE LDAP_* keys but
ldap_provider.py reads mixed-case names (e.g. LDAP_enabled, LDAP_server,
LDAP_port, LDAP_use_ssl, LDAP_use_start_tls, LDAP_tls_verify_cert,
LDAP_ca_cert_path, LDAP_disable_local_admin, LDAP_direct_bind_format,
LDAP_bind_dn, LDAP_bind_password, LDAP_base_dn, LDAP_user_filter,
LDAP_username_attribute), so update the .env file to replace the uppercase keys
with those exact mixed-case keys to match what ldap_provider.py reads; ensure
each key name in the file exactly matches the getenv keys in ldap_provider.py so
environment values are picked up on case-sensitive systems.
Comment on lines
+3
to
+11
| # network_mode: host # Use host networking for ARP scanning and other services | ||
| build: | ||
| context: . # Build context is the current directory | ||
| dockerfile: Dockerfile # Specify the Dockerfile to use | ||
| image: netalertx:latest | ||
| container_name: netalertx # The name when you docker contiainer ls | ||
| ports: | ||
| - "20211:20211" | ||
| - "20212:20212" |
There was a problem hiding this comment.
Preserve host networking or make the bridge-mode tradeoff explicit.
Changing the default from host networking to fixed bridge port publishing can break LAN/ARP scanning, and the new mappings also ignore configurable PORT / GRAPHQL_PORT values. If bridge mode is intentional, map the configured ports and document that ARP-based discovery is reduced; otherwise keep host networking as the default.
🐛 Possible fixes
Keep the existing scanning-oriented default:
- # network_mode: host # Use host networking for ARP scanning and other services
+ network_mode: host # Use host networking for ARP scanning and other services
@@
- ports:
- - "20211:20211"
- - "20212:20212"Or, if bridge mode is the desired default, at least honor the configured ports:
ports:
- - "20211:20211"
- - "20212:20212"
+ - "${PORT:-20211}:${PORT:-20211}"
+ - "${GRAPHQL_PORT:-20212}:${GRAPHQL_PORT:-20212}"Also applies to: 65-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.yml` around lines 3 - 11, The docker-compose change removed
host networking (network_mode: host) which breaks ARP/LAN scanning and
hard-coded the port mappings ("20211:20211", "20212:20212") instead of honoring
configurable PORT / GRAPHQL_PORT; either restore network_mode: host as the
default for the netalertx service (re-enable the existing network_mode: host
comment) to preserve ARP-based discovery, or keep bridge mode but update the
ports section to use the configured environment variables (PORT and
GRAPHQL_PORT) and add a clarifying comment that bridge mode reduces ARP-based
discovery; target the service block containing container_name: netalertx,
network_mode, and ports when making the change.
Comment on lines
+50
to
+54
| ## Security Best Practices | ||
| 1. Always enable `LDAP_TLS_VERIFY_CERT` in production to prevent Man-in-the-Middle attacks. | ||
| 2. Mount your domain's CA certificate into the container via a Docker volume and set `LDAP_CA_CERT_PATH`. | ||
| 3. Provide `LDAP_BIND_PASSWORD` via environment variables or Docker secrets (`/run/secrets/ldap_bind_password`) rather than the Web UI, which stores it in plaintext. | ||
| 4. **Boot-Once Configuration:** To avoid exposing secrets like `LDAP_BIND_PASSWORD` permanently in your `docker-compose.yml` or `.env` file, you can set the environment variables for the initial deployment, log in, save the configuration in the Web UI (Settings > LDAP Authentication), and then remove the environment variables and restart the container. NetAlertX will safely fall back to the saved configuration. |
There was a problem hiding this comment.
Do not call Web UI fallback “safe” for bind passwords.
Line 53 correctly says the UI stores LDAP_BIND_PASSWORD in plaintext, but Line 54 then recommends saving it there as a safe way to remove secrets from compose/env files. Prefer Docker secrets or environment injection for long-lived deployments, and describe UI persistence as a convenience with plaintext-storage tradeoffs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/LDAP_CONFIGURATION.md` around lines 50 - 54, Update the "Boot-Once
Configuration" paragraph so it does not describe saving LDAP_BIND_PASSWORD in
the Web UI as "safe"; instead state that the UI persists the bind password in
plaintext (a convenience only) and recommend using Docker secrets or environment
injection (e.g., LDAP_BIND_PASSWORD via /run/secrets or env at deploy time) for
long-lived deployments; retain the TLS/CA notes (LDAP_TLS_VERIFY_CERT,
LDAP_CA_CERT_PATH) and explicitly call out the plaintext tradeoff for the Web UI
(Settings > LDAP Authentication) and that the UI fallback is a convenience, not
a secure secret store.
Comment on lines
+236
to
+278
| def test_uses_local_when_ldap_disabled(self): | ||
| from auth.manager import AuthManager | ||
| with patch("auth.manager.get_setting_value", return_value=False): | ||
| manager = AuthManager() | ||
| provider = manager.get_provider() | ||
| assert provider.name == "local" | ||
|
|
||
| def test_uses_ldap_when_ldap_enabled(self): | ||
| from auth.manager import AuthManager | ||
| with patch("auth.manager.get_setting_value", return_value=True): | ||
| manager = AuthManager() | ||
| provider = manager.get_provider() | ||
| assert provider.name == "ldap" | ||
|
|
||
| def test_authenticate_uses_local_when_ldap_disabled(self): | ||
| from auth.manager import AuthManager | ||
| from auth.base import AuthResult | ||
|
|
||
| manager = AuthManager() | ||
| with patch("auth.manager.get_setting_value", return_value=False), \ | ||
| patch("auth.manager.LocalProvider.authenticate", return_value=AuthResult.ok("admin", "local")) as mock_auth: | ||
| result = manager.authenticate("admin", "pass") | ||
|
|
||
| mock_auth.assert_called_once_with("admin", "pass") | ||
| assert result.success is True | ||
|
|
||
| def test_authenticate_uses_ldap_when_ldap_enabled(self): | ||
| from auth.manager import AuthManager | ||
| from auth.base import AuthResult | ||
|
|
||
| def mock_settings(key): | ||
| if key == "LDAP_enabled": return True | ||
| if key == "SETPWD_enable_password": return True | ||
| if key == "LDAP_disable_local_admin": return True | ||
| return False | ||
|
|
||
| manager = AuthManager() | ||
| with patch("auth.manager.get_setting_value", side_effect=mock_settings), \ | ||
| patch("auth.manager.LdapProvider.authenticate", return_value=AuthResult.ok("admin", "ldap")) as mock_auth: | ||
| result = manager.authenticate("admin", "pass") | ||
|
|
||
| mock_auth.assert_called_once_with("admin", "pass") | ||
| assert result.success is True |
There was a problem hiding this comment.
Patch LdapProvider._read_config() in AuthManager tests.
These tests patch auth.manager.get_setting_value, but LDAP_enabled is read through LdapProvider()._read_config(), not that imported helper. Patch _read_config() directly so the LDAP-enabled branches are actually exercised.
🧪 Proposed test fix pattern
def test_uses_ldap_when_ldap_enabled(self):
from auth.manager import AuthManager
- with patch("auth.manager.get_setting_value", return_value=True):
+ with patch("auth.manager.LdapProvider._read_config", return_value={"enabled": True}):
manager = AuthManager()
provider = manager.get_provider()
assert provider.name == "ldap"
@@
manager = AuthManager()
- with patch("auth.manager.get_setting_value", side_effect=mock_settings), \
+ with patch("auth.manager.LdapProvider._read_config", return_value={"enabled": True, "disable_local_admin": True}), \
+ patch("auth.manager.get_setting_value", side_effect=mock_settings), \
patch("auth.manager.LdapProvider.authenticate", return_value=AuthResult.ok("admin", "ldap")) as mock_auth:
result = manager.authenticate("admin", "pass")
@@
manager = AuthManager()
- with patch("auth.manager.get_setting_value", side_effect=mock_settings), \
+ with patch("auth.manager.LdapProvider._read_config", return_value={"enabled": True, "disable_local_admin": False}), \
+ patch("auth.manager.get_setting_value", side_effect=mock_settings), \
patch("auth.manager.LdapProvider.authenticate", return_value=AuthResult.fail("ldap", "User not found")), \
patch("auth.manager.LocalProvider.authenticate", return_value=AuthResult.ok("admin", "local")) as mock_local_auth:
result = manager.authenticate("admin", "pass")Also applies to: 280-298
🧰 Tools
🪛 Ruff (0.15.10)
[error] 267-267: Multiple statements on one line (colon)
(E701)
[error] 268-268: Multiple statements on one line (colon)
(E701)
[error] 269-269: Multiple statements on one line (colon)
(E701)
🤖 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 236 - 278, The tests
currently patch auth.manager.get_setting_value but LdapProvider reads
LDAP_enabled via its own _read_config(), so update the tests (e.g.,
test_uses_ldap_when_ldap_enabled and
test_authenticate_uses_ldap_when_ldap_enabled) to patch
LdapProvider._read_config to return a config dict where "LDAP_enabled" (and any
other relevant flags like "SETPWD_enable_password" and
"LDAP_disable_local_admin") are True; similarly patch _read_config to return
LDAP_enabled False for the LDAP-disabled tests; keep existing patches for
LocalProvider.authenticate / LdapProvider.authenticate and use AuthManager as
before so the LDAP branches are actually exercised.
- 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
Owner
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
- 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
- Adds an explicitly togglable flag to the comprehensive GitHub workflow - Installs the dependency automatically into the local test environments via
Owner
|
@coderabbitai Review the PR |
|
✅ Actions performedReview triggered.
|
Owner
|
@coderabbitai review All outstanding review comments have been addressed and pushed to this branch. Here is the confirmation for each issue:
The branch is fully updated and all local tests pass. |
|
Kicking off a fresh review of the updated branch now! 🚀 (づ｡◕‿‿◕｡)づ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Actions performedReview triggered.
|
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.
📌 Description
Adds LDAP/AD as an optional authentication backend alongside the existing local SHA-256 password. When enabled, the login form gains a username field and credentials are validated against the configured LDAP server via the Python backend. The local password path is fully preserved as a fallback.
Auth module (
server/auth/)base.py—AuthProviderABC +AuthResultdataclass; clean interface for adding future providerslocal_provider.py— wraps existing SHA-256/SETPWD_passwordcheck in the provider contractldap_provider.py— search-then-bind vialdap3; RFC 4515 filter escaping; StartTLS + LDAPS; OpenLDAP and AD compatible (uid={username}/sAMAccountName={username})manager.py— dispatches toLdapProviderorLocalProviderbased onLDAP_enabledat request time (no restart needed to toggle)API
POST /api/auth/login— intentionally unauthenticated endpoint; accepts{username, password}; returns{success, username, provider}; failed attempts logged viawrite_notificationLoginRequest/LoginResponsePydantic models added toopenapi/schemas.pyalongside the newLanguagesResponseandPluginStatsResponsemodels introduced bymainPlugin (
front/plugins/auth_ldap/config.json)unique_prefix: LDAP) exposing all 9 settings via the Settings UI: server, port, SSL/StartTLS, service-account bind DN + password, base DN, user search filter, username attributePHP frontend (
front/index.php)LDAP_enabledfromapp.confat render time; conditionally shows aloginusernamefield; callsPOST /api/auth/loginviafile_get_contentson the loopback when LDAP is on; unmodified SHA-256 path when offDB
ensure_Users— idempotentCREATE TABLE IF NOT EXISTS Users(username, role, provider, enabled, timestamps); called frominitDB; never dropped on restartDependencies
ldap3added torequirements.txtalongsidefritzconnection>=1.15.1(added bymainfor the FritzBox plugin)🔍 Related Issues
📋 Type of Change
📷 Screenshots or Logs (if applicable)
Login form with LDAP enabled — username field appears above the password field only when
LDAP_enabled = trueinapp.conf.Example LDAP plugin configuration:
AD equivalent:
🧪 Testing Steps
test/backend/test_auth_providers.pycoveringAuthResult,LocalProvider(correct/wrong/empty/missing hash),LdapProvider(success, service-bind fail, user not found, wrong user password, missing server, filter escaping, missingldap3), andAuthManagerdispatchtest/api_endpoints/test_auth_endpoints.py: valid credentials → 200, wrong password → 401, missing fields → 422, wrong content-type → 415, GET → 404✅ Checklist
🙋 Additional Notes
local_provider.pyis flagged by CodeQL (py/weak-sensitive-data-hashing). This is a legacy-comparison: the hash is computed by the existing PHP login page and stored inapp.conf; we are replicating the check, not introducing new hash storage. Annotated with# nosec B324. Migrating to Argon2/bcrypt requires a coordinated PHP change and is out of scope here.POST /api/auth/loginhas no rate limiting. It is called server-side (PHP → loopback) not directly from the browser, which limits exposure. A Nginxlimit_reqrule is recommended before exposing this container port externally.Userstable is scaffolding for future multi-user support; no user management UI is included in this PR.origin/mainwere resolved:requirements.txtretains bothldap3andfritzconnection>=1.15.1;api_server_start.pyretains all four schema imports (LoginRequest,LoginResponse,LanguagesResponse,PluginStatsResponse).Summary by CodeRabbit
Release Notes
New Features
Security Improvements
Documentation
Other Improvements