Phase 1: multi-user login + roles + per-user mailboxes - #89
Conversation
Optional multi-user account layer on the dashboard, fully backward-compatible with single-token installs. - accounts.py: JSON-backed users store (werkzeug password hashing, no new deps), CRUD + verify, roles (admin/user), extension + mailbox fields, chmod 600. - server.py: sessions carry the user; _current_user resolves session -> user (or TOKEN_ADMIN for token/first-run, gated on DASHBOARD_TOKEN so an install with users but no token can't be bypassed with an empty token). RBAC (_is_admin_only) gates user management, provider install, and settings writes. /login accepts username+password (dashboard token in the password field still works); login is rate-limited. New: /api/me, /api/users CRUD (admin). /voicemails is mailbox-scoped (admins see all). Login page gains a username field. - DASHBOARD_TOKEN stays the admin break-glass; token-only installs are unchanged. users.json gitignored. - Tests +12 (42 total): store, password login, token back-compat, RBAC 403s, unauth 401, mailbox scoping. Telephony (extensions, transfer, conference, softphone) is Phase 2+; this is the app-layer foundation only, no Twilio usage.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 52 minutes and 46 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughA new ChangesMulti-user Account Store and Server Auth
Sequence Diagram(s)sequenceDiagram
participant Client
participant server.py
participant accounts.py
participant users.json
Client->>server.py: POST /login (username, password)
server.py->>accounts.py: verify(username, password)
accounts.py->>users.json: _load()
users.json-->>accounts.py: user records
accounts.py-->>server.py: public user dict or None
server.py-->>Client: session cookie + 200 or 401
Client->>server.py: GET /api/me (session cookie)
server.py->>accounts.py: get(user_id)
accounts.py-->>server.py: public user dict
server.py-->>Client: {username, role, ...}
Client->>server.py: POST /api/users (admin session)
server.py->>accounts.py: create(username, password, role, ...)
accounts.py->>users.json: _save()
accounts.py-->>server.py: new public user dict
server.py-->>Client: 201 + new user record
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
| username = (data.get("username") or "").strip() | ||
| password = data.get("password") or data.get("token") or "" | ||
| ip = request.headers.get("X-Forwarded-For", request.remote_addr or "?").split(",")[0].strip() | ||
| if _login_locked(ip): | ||
| return jsonify({"status": "error", "detail": "too many attempts, try later"}), 429 | ||
| user, uid = None, None | ||
| if username: | ||
| user = accounts.verify(username, password) | ||
| uid = user["id"] if user else None | ||
| elif password and DASHBOARD_TOKEN and hmac.compare_digest(password, DASHBOARD_TOKEN): | ||
| user = TOKEN_ADMIN # back-compat: dashboard token in the password field | ||
| if user is None: | ||
| _record_login_fail(ip) | ||
| return jsonify({"status": "error"}), 401 |
There was a problem hiding this comment.
⚠️ Security: Login rate-limit bypassable via spoofed X-Forwarded-For
The dashboard login lockout in login() keys on request.headers.get("X-Forwarded-For", request.remote_addr or "?").split(",")[0] (server.py:935). The first X-Forwarded-For value is fully client-controlled, so an attacker brute-forcing a user's password or the DASHBOARD_TOKEN can send a different (random/rotating) X-Forwarded-For header on each request and never trip _login_locked, defeating the lockout entirely. The PIN tracker keys on caller which isn't spoofable, but this new path is.
Fix: only trust X-Forwarded-For when the request comes through a known/trusted reverse proxy; otherwise key the rate limiter on request.remote_addr (the real socket peer). If running behind a proxy, use a fixed trusted-proxy configuration (e.g. werkzeug ProxyFix with a known hop count) rather than blindly taking the leftmost XFF value.
Was this helpful? React with 👍 / 👎
| @dashboard_app.route("/api/users/<uid>", methods=["PATCH", "DELETE"]) | ||
| def api_user(uid): | ||
| if request.method == "DELETE": | ||
| if uid == g.user.get("id"): | ||
| return jsonify({"error": "cannot delete your own account"}), 400 | ||
| return jsonify({"status": "ok"}) if accounts.delete(uid) else (jsonify({"error": "not found"}), 404) | ||
| d = request.json or {} | ||
| allowed = {k: d[k] for k in ("display_name", "role", "extension", "mailbox", "active", "password") if k in d} | ||
| try: | ||
| user = accounts.update(uid, **allowed) | ||
| except ValueError as e: | ||
| return jsonify({"error": str(e)}), 400 | ||
| return jsonify(user) if user else (jsonify({"error": "not found"}), 404) |
There was a problem hiding this comment.
⚠️ Edge Case: Admin can lock out all admin access (last-admin demotion)
api_user (server.py:978-990) blocks deleting your own account, but it still allows an admin to PATCH their own role to user, set active=False on themselves, or demote/deactivate/delete the only other admin. accounts.update/delete perform no "at least one active admin must remain" check. If DASHBOARD_TOKEN is not set (a supported config once users exist — see _current_user), losing the last admin permanently disables user management, provider install, and settings writes with no recovery path.
Fix: before applying a PATCH that sets role!='admin' or active=False, or a DELETE, verify at least one other active admin remains, and reject self-demotion of the last admin with a 400. Consider enforcing this invariant inside accounts.update/accounts.delete so it can't be bypassed by future callers.
Was this helpful? React with 👍 / 👎
| def update(user_id, **fields): | ||
| with _lock: | ||
| users = _load() | ||
| for u in users: | ||
| if u["id"] != user_id: | ||
| continue | ||
| pw = fields.pop("password", None) | ||
| if pw: | ||
| u["password_hash"] = generate_password_hash(pw) | ||
| if "role" in fields and fields["role"] not in ROLES: | ||
| raise ValueError("invalid role") | ||
| for k in ("display_name", "role", "extension", "mailbox", "active"): | ||
| if k in fields and fields[k] is not None: | ||
| u[k] = fields[k] | ||
| _save(users) |
There was a problem hiding this comment.
💡 Bug: update() does not normalize mailbox like create()
accounts.create normalizes mailbox with (mailbox or username).strip().lower() (accounts.py:81), but accounts.update writes fields[k] verbatim for mailbox (accounts.py:101-103). Mailbox scoping in list_voicemails compares vm.get("mailbox", "general") == mailbox exactly (server.py:1025-1027). If an admin PATCHes a user's mailbox to e.g. "Alice" or " alice", scoping silently breaks and the user sees no voicemails. Normalize mailbox (and trim extension) on update the same way create does.
Was this helpful? React with 👍 / 👎
| def verify(username, password): | ||
| """Return the public user dict on valid credentials, else None.""" | ||
| with _lock: | ||
| for u in _load(): | ||
| if u["username"].lower() == (username or "").lower(): | ||
| if u.get("active", True) and check_password_hash(u["password_hash"], password or ""): | ||
| return _public(u) | ||
| return None | ||
| return None |
There was a problem hiding this comment.
💡 Security: Username enumeration via timing in verify()
accounts.verify (accounts.py:119-127) only calls check_password_hash when a matching username exists; for an unknown username it returns None immediately without doing any hashing work. The resulting timing difference lets an attacker enumerate valid usernames. To mitigate, perform a dummy check_password_hash against a constant fake hash when no user matches so the response time is comparable for valid and invalid usernames.
Was this helpful? React with 👍 / 👎
| def _save(users): | ||
| ACCOUNTS_FILE.write_text(json.dumps(users, indent=2)) | ||
| try: | ||
| ACCOUNTS_FILE.chmod(0o600) # contains password hashes | ||
| except OSError: | ||
| pass |
There was a problem hiding this comment.
💡 Bug: Non-atomic users.json write risks corruption/lost updates
_save (accounts.py:33-38) does ACCOUNTS_FILE.write_text(...) directly. The threading.RLock only serializes within a single process; if the dashboard ever runs with multiple workers/processes, concurrent read-modify-write cycles can drop updates, and a crash mid-write can leave a truncated/corrupt JSON file (which _load then silently treats as an empty user list, effectively losing all accounts). Write to a temp file and os.replace for atomicity, and consider file locking if multi-process is possible.
Was this helpful? React with 👍 / 👎
|
Note Your trial team has used its Gitar budget, so automatic reviews are paused. Upgrade now to unlock full capacity. Comment "Gitar review" to trigger a review manually. Code Review
|
| Compact |
|
Important
Your trial ends in 6 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.
Was this helpful? React with 👍 / 👎 | Gitar
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@accounts.py`:
- Around line 27-30: The _load() function currently catches both
FileNotFoundError and ValueError exceptions, treating both cases as empty
account lists. This masks malformed JSON files as if no users exist, which can
trigger unintended first-run admin initialization. Modify the exception handling
to only catch FileNotFoundError and return an empty list for that case, while
allowing ValueError (JSON parse errors) to propagate as exceptions. This way, a
missing file is treated as empty, but a corrupted users.json file will fail
explicitly rather than silently being interpreted as no configured users.
- Around line 101-103: The update() method is assigning raw values directly to
the user object for mailbox, extension, and active fields without applying the
same normalization and validation that the create() method performs. Review how
create() normalizes and validates these specific fields (mailbox, extension, and
active) to ensure consistent data handling, then apply the same normalization
and validation logic in the update() method before assigning the fields[k]
values to u[k] for these three fields.
- Around line 33-34: The _save function currently uses write_text() which is not
atomic and can result in a partially written or corrupted file if the process is
interrupted or crashes. Replace this with an atomic write pattern by first
writing the JSON data to a temporary file, then using atomic replacement
(os.replace or similar) to move the temp file to the final location. This
ensures that either the entire file is written successfully or the original file
remains untouched.
In `@server.py`:
- Around line 1024-1028: The `DELETE /voicemails/<sid>` and `GET
/voicemails/<sid>/audio` endpoints are missing the same mailbox authorization
check that was implemented in the `GET /voicemails` endpoint. For both
endpoints, retrieve the voicemail record by SID, then add a security check that
verifies the voicemail's mailbox matches the current user's mailbox (unless the
user is an admin with mailbox "*"). If the mailbox does not match, return a 403
Forbidden response before allowing the delete or audio retrieval operation to
proceed.
- Around line 932-935: Add input validation to check that the JSON payload is a
dictionary using isinstance(data, dict) before calling .get() methods on it, and
verify that the username value is a string before calling .strip() on it. This
prevents AttributeError exceptions when the JSON payload has an unexpected shape
(like a list or non-dict JSON) or when fields contain non-string values. Apply
this validation pattern consistently across all the affected request handlers in
the username extraction logic.
- Line 935: The IP extraction logic at line 935 for rate-limiting is vulnerable
because it directly trusts the client-controlled X-Forwarded-For header,
allowing attackers to bypass rate limits or poison lockouts. Replace the current
logic that prioritizes X-Forwarded-For with direct use of request.remote_addr
for rate-limit identity. Only use X-Forwarded-For if trusted proxy configuration
is explicitly implemented and validated in the application.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 957a3959-4c6e-45ac-98a6-55a7e2404587
📒 Files selected for processing (4)
.gitignoreaccounts.pyserver.pytests/test_accounts.py
| try: | ||
| return json.loads(ACCOUNTS_FILE.read_text()) | ||
| except (FileNotFoundError, ValueError): | ||
| return [] |
There was a problem hiding this comment.
Fail-closed on malformed users.json instead of silently treating it as empty.
_load() currently converts JSON parse errors into []. That can cascade into unintended auth behavior because server.py uses accounts.count() == 0 as the first-run admin condition. A malformed store should not be interpreted as “no users configured”.
Suggested fix
def _load():
try:
return json.loads(ACCOUNTS_FILE.read_text())
- except (FileNotFoundError, ValueError):
+ except FileNotFoundError:
return []
+ except ValueError as exc:
+ raise RuntimeError(f"Invalid accounts file: {ACCOUNTS_FILE}") from exc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| return json.loads(ACCOUNTS_FILE.read_text()) | |
| except (FileNotFoundError, ValueError): | |
| return [] | |
| def _load(): | |
| try: | |
| return json.loads(ACCOUNTS_FILE.read_text()) | |
| except FileNotFoundError: | |
| return [] | |
| except ValueError as exc: | |
| raise RuntimeError(f"Invalid accounts file: {ACCOUNTS_FILE}") from exc |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@accounts.py` around lines 27 - 30, The _load() function currently catches
both FileNotFoundError and ValueError exceptions, treating both cases as empty
account lists. This masks malformed JSON files as if no users exist, which can
trigger unintended first-run admin initialization. Modify the exception handling
to only catch FileNotFoundError and return an empty list for that case, while
allowing ValueError (JSON parse errors) to propagate as exceptions. This way, a
missing file is treated as empty, but a corrupted users.json file will fail
explicitly rather than silently being interpreted as no configured users.
| def _save(users): | ||
| ACCOUNTS_FILE.write_text(json.dumps(users, indent=2)) |
There was a problem hiding this comment.
Persist users.json atomically to avoid account-store corruption.
Direct write_text() can leave a partially written file on interruption/crash. For auth data, write-to-temp + atomic replace is safer.
Suggested fix
def _save(users):
- ACCOUNTS_FILE.write_text(json.dumps(users, indent=2))
+ tmp = ACCOUNTS_FILE.with_suffix(".json.tmp")
+ tmp.write_text(json.dumps(users, indent=2))
+ tmp.replace(ACCOUNTS_FILE)
try:
ACCOUNTS_FILE.chmod(0o600) # contains password hashes
except OSError:
pass📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _save(users): | |
| ACCOUNTS_FILE.write_text(json.dumps(users, indent=2)) | |
| def _save(users): | |
| tmp = ACCOUNTS_FILE.with_suffix(".json.tmp") | |
| tmp.write_text(json.dumps(users, indent=2)) | |
| tmp.replace(ACCOUNTS_FILE) | |
| try: | |
| ACCOUNTS_FILE.chmod(0o600) # contains password hashes | |
| except OSError: | |
| pass |
🧰 Tools
🪛 ast-grep (0.43.0)
[info] 33-33: use jsonify instead of json.dumps for JSON output
Context: json.dumps(users, indent=2)
Note: Security best practice.
(use-jsonify)
🪛 Ruff (0.15.17)
[warning] 33-33: Missing return type annotation for private function _save
Add return type annotation: None
(ANN202)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@accounts.py` around lines 33 - 34, The _save function currently uses
write_text() which is not atomic and can result in a partially written or
corrupted file if the process is interrupted or crashes. Replace this with an
atomic write pattern by first writing the JSON data to a temporary file, then
using atomic replacement (os.replace or similar) to move the temp file to the
final location. This ensures that either the entire file is written successfully
or the original file remains untouched.
| for k in ("display_name", "role", "extension", "mailbox", "active"): | ||
| if k in fields and fields[k] is not None: | ||
| u[k] = fields[k] |
There was a problem hiding this comment.
Normalize and validate updated fields the same way create() does.
update() currently writes raw values for mailbox, extension, and active. This can break downstream assumptions (e.g., mailbox matching and boolean active checks).
Suggested fix
- for k in ("display_name", "role", "extension", "mailbox", "active"):
- if k in fields and fields[k] is not None:
- u[k] = fields[k]
+ if "display_name" in fields and fields["display_name"] is not None:
+ u["display_name"] = str(fields["display_name"]).strip() or u["username"]
+ if "role" in fields and fields["role"] is not None:
+ u["role"] = fields["role"]
+ if "extension" in fields and fields["extension"] is not None:
+ u["extension"] = str(fields["extension"]).strip()
+ if "mailbox" in fields and fields["mailbox"] is not None:
+ u["mailbox"] = str(fields["mailbox"]).strip().lower()
+ if "active" in fields and fields["active"] is not None:
+ if not isinstance(fields["active"], bool):
+ raise ValueError("active must be a boolean")
+ u["active"] = fields["active"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for k in ("display_name", "role", "extension", "mailbox", "active"): | |
| if k in fields and fields[k] is not None: | |
| u[k] = fields[k] | |
| if "display_name" in fields and fields["display_name"] is not None: | |
| u["display_name"] = str(fields["display_name"]).strip() or u["username"] | |
| if "role" in fields and fields["role"] is not None: | |
| u["role"] = fields["role"] | |
| if "extension" in fields and fields["extension"] is not None: | |
| u["extension"] = str(fields["extension"]).strip() | |
| if "mailbox" in fields and fields["mailbox"] is not None: | |
| u["mailbox"] = str(fields["mailbox"]).strip().lower() | |
| if "active" in fields and fields["active"] is not None: | |
| if not isinstance(fields["active"], bool): | |
| raise ValueError("active must be a boolean") | |
| u["active"] = fields["active"] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@accounts.py` around lines 101 - 103, The update() method is assigning raw
values directly to the user object for mailbox, extension, and active fields
without applying the same normalization and validation that the create() method
performs. Review how create() normalizes and validates these specific fields
(mailbox, extension, and active) to ensure consistent data handling, then apply
the same normalization and validation logic in the update() method before
assigning the fields[k] values to u[k] for these three fields.
| data = request.json or {} | ||
| token = data.get("token", "") | ||
| if check_auth(token): | ||
| resp = jsonify({"status": "ok"}) | ||
| resp.set_cookie(AUTH_COOKIE, _new_session(), httponly=True, | ||
| samesite="Strict", secure=request.is_secure, max_age=SESSION_TTL) | ||
| return resp | ||
| return jsonify({"status": "error"}), 401 | ||
| username = (data.get("username") or "").strip() | ||
| password = data.get("password") or data.get("token") or "" | ||
| ip = request.headers.get("X-Forwarded-For", request.remote_addr or "?").split(",")[0].strip() |
There was a problem hiding this comment.
Validate JSON payload shape before using .get(...) and string operations.
These handlers assume object payloads and string fields. Non-object JSON (or non-string username) can raise runtime exceptions and return 500s.
Suggested fix pattern
- data = request.json or {}
- username = (data.get("username") or "").strip()
+ data = request.get_json(silent=True)
+ if not isinstance(data, dict):
+ return jsonify({"status": "error", "detail": "invalid JSON body"}), 400
+ username_raw = data.get("username", "")
+ if username_raw is not None and not isinstance(username_raw, str):
+ return jsonify({"status": "error", "detail": "username must be a string"}), 400
+ username = (username_raw or "").strip()Apply the same isinstance(data, dict) guard in /api/users and /api/users/<uid>.
As per coding guidelines, server.py: "Focus on security, input validation, and error handling. Check for hardcoded secrets."
Also applies to: 967-968, 984-985
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server.py` around lines 932 - 935, Add input validation to check that the
JSON payload is a dictionary using isinstance(data, dict) before calling .get()
methods on it, and verify that the username value is a string before calling
.strip() on it. This prevents AttributeError exceptions when the JSON payload
has an unexpected shape (like a list or non-dict JSON) or when fields contain
non-string values. Apply this validation pattern consistently across all the
affected request handlers in the username extraction logic.
Source: Coding guidelines
Security review flagged two issues in the multi-user code:
- [HIGH] IDOR: mailbox scoping was only on GET /voicemails, not the per-sid
handlers. A non-admin could DELETE or stream audio of any voicemail by SID,
and /export/* dumped every mailbox. Centralized _vm_visible() and applied it
to delete, audio, and both export routes (404 on mismatch, no existence leak).
- [MEDIUM] Login lockout keyed on attacker-controlled X-Forwarded-For, so an
attacker could rotate the header to evade it. Now keyed per-account
("user:<username>", un-spoofable) for password logins; for token logins, on the
client IP derived from XFF only via a trusted local proxy.
- Tests +4 (46 total): cross-mailbox delete/audio/export denied; account lockout
survives IP/header rotation.
First slice of the PBX direction (the rest — extensions, transfer, conference, softphones — is Phase 2+). App layer only; no Twilio usage.
What
An optional multi-user account layer, fully backward-compatible with single-
DASHBOARD_TOKENinstalls (the Mac mini keeps working unchanged).accounts.py— JSON-backed users store (werkzeugpassword hashing, no new deps), CRUD + verify, roles (admin/user),extension+mailboxfields,chmod 600, gitignored._current_user()resolves session → user, orTOKEN_ADMINfor token/first-run. The Bearer/?tokenpaths are gated onDASHBOARD_TOKENso an install that adds users but no token can't be bypassed with an empty token._is_admin_only()gates user management, provider install, and settings writes (403 for non-admins)./login— takes username+password; the dashboard token still works in the password field (blank username). Rate-limited.GET /api/me,GET/POST /api/users,PATCH/DELETE /api/users/<id>(admin)./voicemailsis scoped to the logged-in user's mailbox; admins (*) see all.Verified
+12 tests (42 total): accounts store, password login, token back-compat, RBAC 403s, unauth 401, mailbox scoping. Full suite green.
Not in this PR
The user-facing Users admin section + user-aware UI land with the dashboard rebuild. This is the backend + login page only.
Summary by CodeRabbit