feat: add session login + per-user API keys to NutWatch - #50
Merged
Conversation
Replace the single-secret NUTWATCH_API_KEY env var with a proper authentication system: accounts (admin/viewer roles) with session login for the dashboard and per-user API keys for scripts. The app stays fully open (bootstrap mode) until the first admin account is created via the first-run Setup page or manage.py create-admin. Backend: - New services/auth_db.py: SQLite store for accounts and API keys (pbkdf2:sha256 passwords, SHA-256 hashed keys, soft-delete revoke) - Rewritten auth.py: resolve_principal() checks session cookie then Bearer key; @require_admin/@require_auth/@require_admin_strict decorators with bootstrap-open fallback when zero accounts exist - New routes/auth.py: setup, login, logout, /me, accounts CRUD, API key CRUD endpoints - New manage.py: create-admin, reset-password, list-accounts CLI for bootstrap and lockout recovery - config.py: removed NUTWATCH_API_KEY; added NUTWATCH_SECRET_KEY (auto-generated and persisted in auth DB) and NUTWATCH_SESSION_COOKIE_SECURE - All existing route decorators updated: read endpoints use @require_auth (any authenticated principal), mutating endpoints stay @require_admin; viewer role gets 403 on writes Frontend: - AuthProvider: loads auth status, gates App.tsx rendering between Setup -> Login -> AppLayout based on bootstrap state - New Setup.tsx (first-run admin creation with Skip), Login.tsx, Accounts.tsx (admin CRUD), AccountModal.tsx, ApiKeys.tsx (self- service), ApiKeyModal.tsx (shows raw key once), LogoMark.tsx - api.ts: 401 handler resets account state mid-session; optional in-memory bearer token fallback for non-cookie clients - Role-gated UI: Dashboard System Actions, UpsCard/UpsDevices mutations, Notifications editor, ConfigFiles save, HooksSection editor, Users CRUD, WakeOnLan targets/mappings all hidden for viewers - Sidebar: user chip with role badge + logout, Accounts and API Keys nav entries - Users tab renamed to NUT Users with info box distinguishing from dashboard Accounts Tests: - New test_services_auth_db.py (account/key CRUD, login, revoke) - New test_routes_auth.py (setup, login, rate-limiting, accounts, apikeys) - New test_manage_cli.py (create-admin, reset-password, list-accounts) - Rewritten test_auth.py: session, Bearer key, bootstrap-open, revoked keys, inactive accounts, key-inherits-owner-role, role gating - test_routes.py: viewer-role coverage for all read/mutate endpoints - Frontend: AuthProvider, Setup, Login, Accounts, ApiKeys, updated Dashboard/WakeOnLan/UpsCard tests Docs: updated AGENTS.md, README.md, CONTRIBUTING.md, .coderabbit.yaml; added docs/auth-plan.md; updated docs/modularization-plan.md
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughNutWatch now uses database-backed accounts, session login, and per-user API keys instead of a single environment Bearer token. The backend adds auth storage, endpoints, and CLI tooling; several read-only routes accept any authenticated user; the frontend adds auth gating and account/API-key management; tests and docs were updated throughout. ChangesAccount-backed auth migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant AuthProvider
participant api.ts
participant routes/auth.py
participant auth_db.py
Browser->>AuthProvider: mount
AuthProvider->>api.ts: GET AUTH_STATUS
api.ts->>routes/auth.py: /api/auth/status
routes/auth.py->>auth_db.py: count_accounts()
auth_db.py-->>routes/auth.py: bootstrapped state
routes/auth.py-->>AuthProvider: bootstrapped/authenticated
alt setup required
AuthProvider-->>Browser: render Setup
Browser->>AuthProvider: setupAdmin()
AuthProvider->>api.ts: POST AUTH_SETUP
api.ts->>routes/auth.py: /api/auth/setup
routes/auth.py->>auth_db.py: create_initial_admin()
auth_db.py-->>routes/auth.py: admin account
routes/auth.py-->>AuthProvider: account + session
else login required
AuthProvider-->>Browser: render Login
Browser->>AuthProvider: login()
AuthProvider->>api.ts: POST AUTH_LOGIN
api.ts->>routes/auth.py: /api/auth/login
routes/auth.py->>auth_db.py: verify_login()
auth_db.py-->>routes/auth.py: account
routes/auth.py-->>AuthProvider: account + session
else authenticated
AuthProvider-->>Browser: render AppLayout
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/backend/routes/system.py (1)
24-31: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep raw config reads admin-only unless secrets are masked.
This endpoint returns
contentdirectly; broadening it torequire_authlets viewer accounts read sensitive NUT config content unlessget_config()masks every secret-bearing file. Keep this route onrequire_adminor return a sanitized viewer-safe representation. As per path instructions, “Passwords are masked (••••••) in API responses.”Proposed fix
-@require_auth +@require_admin def get_config_handler(filename):🤖 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 `@src/backend/routes/system.py` around lines 24 - 31, The get_config_handler route currently uses require_auth while returning raw content from get_config, which can expose sensitive config data to non-admin users. Update the route to use require_admin for admin-only access, or ensure get_config returns a sanitized viewer-safe version before exposing it. Keep the check in get_config_handler aligned with ALLOWED_CONFIGS and the existing require_auth/require_admin authorization symbols.Source: Path instructions
src/frontend/src/components/WakeOnLan.tsx (2)
355-368: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSame
colSpanmismatch in the Event Mappings empty state.Header now has 4 or 5
<th>depending onisAdmin(Line 364), but the empty-state row hardcodescolSpan={5}(Line 368).🩹 Proposed fix
- ? <tr><td colSpan={5} className="empty">No event mappings configured.</td></tr> + ? <tr><td colSpan={isAdmin ? 5 : 4} className="empty">No event mappings configured.</td></tr>🤖 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 `@src/frontend/src/components/WakeOnLan.tsx` around lines 355 - 368, The Event Mappings empty-state row in WakeOnLan.tsx uses a hardcoded colspan that does not match the number of table headers when isAdmin changes. Update the empty-state cell in the mappings table to compute its colSpan from the same isAdmin condition used for the Actions column, matching the header in the table render so the layout stays aligned.
317-328: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmpty-state
colSpandoesn't account for the now-conditional Actions column.The "Targets" header row renders 4
<th>for viewers and 5 for admins (Line 324), but the empty-state row still hardcodescolSpan={5}(Line 328), leaving a stray implicit column for viewer accounts.🩹 Proposed fix
- ? <tr><td colSpan={5} className="empty">No WOL targets configured.</td></tr> + ? <tr><td colSpan={isAdmin ? 5 : 4} className="empty">No WOL targets configured.</td></tr>🤖 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 `@src/frontend/src/components/WakeOnLan.tsx` around lines 317 - 328, The empty-state row in WakeOnLan’s targets table hardcodes a column span that no longer matches the conditional Actions column in the table header. Update the empty-state <td> in the table rendering logic so its colSpan is derived from whether isAdmin is true, matching the <thead> and keeping viewer/admin layouts aligned.
🤖 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 `@docs/modularization-plan.md`:
- Around line 83-84: The `### auth.py` heading in the modularization plan is
missing the required blank line below it, triggering MD022. Update the markdown
around the `auth.py` section so the heading is separated from the following
paragraph by one blank line, keeping the content under that heading intact.
In `@src/backend/auth.py`:
- Line 50: The bootstrap gate in auth should not depend on total accounts; it
must stay open until an admin exists. Update the checks in the auth flow that
currently use count_accounts() in the bootstrap decision to use an
admin-specific helper such as admin_exists() or count_admin_accounts(), and
apply the same change in the related bootstrap/recovery branches referenced by
the auth logic so non-admin-only databases do not prematurely lock setup.
- Around line 21-34: The auth.resolve_principal flow currently returns None
immediately when session["account_id"] exists but does not resolve to an active
account, preventing valid Bearer authentication from being checked. Update
resolve_principal so it only returns the session account when
get_account_by_id() yields an active account, and otherwise falls through to the
Authorization header parsing and resolve_api_key() fallback. Keep the existing
session-first order, but ensure inactive, deleted, or missing session principals
do not block API key auth.
In `@src/backend/config.py`:
- Line 19: NUTWATCH_SECRET_KEY is currently accepted from the environment with
any value, which can leave Flask session signing too weak; update the config
loading in config.py so the secret key is validated before use and the app fails
fast when an override is present but not sufficiently strong. Add the check near
the NUTWATCH_SECRET_KEY assignment, and ensure the resulting setting used by
Flask only accepts a strong value or raises an explicit startup error.
In `@src/backend/manage.py`:
- Line 57: The cmd_list_accounts callback has an unused args parameter that
triggers the Ruff lint. Update the cmd_list_accounts function signature to use
_args instead of args so it still matches the callback interface while clearly
marking the parameter as intentionally unused.
- Around line 79-84: The admin CLI currently accepts sensitive passwords via the
--password option in the cmd_create_admin and reset-password flows, which should
be removed. Update the argument parsing in manage.py so these commands prompt
interactively by default, and if automation is needed, add a safer stdin-based
option such as --password-stdin instead of a command-line password flag. Make
sure the affected parser setup and the cmd_create_admin handler, along with the
reset-password command wiring, are updated consistently.
In `@src/backend/routes/auth.py`:
- Around line 38-47: The first-admin bootstrap flow in auth should be made
atomic so concurrent setup requests cannot both see zero accounts and create
multiple admins. Move the “no accounts exist” guard out of the route logic in
auth.py and into an auth_db.create_initial_admin() helper in auth_db.py that
performs the existence check and admin insert within the same SQLite
transaction. Update the setup route to call that helper instead of
count_accounts() followed by create_account(), and ensure the helper preserves
the bootstrap rule that once the first admin exists, auth is enforced.
- Around line 94-105: The create_account_handler route currently allows
unauthenticated bootstrap account creation through require_admin, which can
bypass the intended first-admin setup flow. Update the auth route to use
require_admin_strict for /api/accounts or add an explicit count_accounts() guard
in create_account_handler that rejects zero-account bootstrap attempts and
directs callers to /api/auth/setup for the initial admin. Keep the fix centered
in create_account_handler and align it with the bootstrap rule enforced by
auth_db.
- Around line 111-129: The account update/deactivate handlers allow changing an
admin to inactive or a non-admin role without verifying that at least one active
admin remains, which can lock out admin access. Add a guard in
update_account_handler and deactivate_account_handler around
auth_db.update_account() to reject role changes or deactivation that would
remove the last active admin, using the existing account/admin lookup logic in
auth_db to check current active admins before applying the update.
In `@src/backend/routes/hooks.py`:
- Around line 18-19: The hooks endpoint currently uses require_auth, which
allows non-admin users to read sensitive hook bodies. Update the hooks route in
hooks_bp so only admins can access full hook content by switching the protection
to require_admin, or alternatively keep require_auth but return sanitized
metadata instead of the script body for non-admin callers. Make the change in
the route handling for the hook fetch endpoint so the access control matches the
sensitivity of hook contents.
In `@src/backend/routes/logs.py`:
- Around line 12-13: The `/api/logs/stream` endpoint in `logs_bp` is currently
protected only by `require_auth`, which allows viewer/API key accounts to access
raw service logs. Update the authorization on the logs streaming route (and any
related log endpoints in the same handler) to require admin-only access using
the existing auth/role checks, or add redaction plus strict rate/connection
limits before allowing non-admins. Keep the change localized around the
`logs_bp.route` handlers so the log stream cannot be opened by viewer accounts.
In `@src/backend/routes/upsmon.py`:
- Around line 9-12: The get_upsmon_config_handler endpoint is returning raw
MONITOR credentials from get_upsmon_config(), exposing sensitive
username/password data to authenticated users. Update this handler to either
restrict access to admins only or sanitize the returned config before jsonify by
redacting the password field (and any other secrets) while keeping the rest of
the UPSMon config intact.
In `@src/backend/routes/wol.py`:
- Around line 132-134: The `list_network_hosts` route is exposing
`wol_service.scan_network_hosts()` to any authenticated viewer, but this
operation should remain admin-only. Update the route in `list_network_hosts` to
use the admin authorization guard instead of `require_auth`, or change it to
return a cached scan result for non-admins. Keep the fix scoped to the
`list_network_hosts` endpoint and its auth decorator.
In `@src/backend/services/auth_db.py`:
- Around line 180-204: update_account() in auth_db must block changes that would
leave the system with no active admin account. Before applying role or is_active
updates, check whether the target account is the last active admin and reject
demotion or deactivation in that case, while still allowing other fields like
password to change. Use the existing update_account() flow, ROLES validation,
and the accounts lookup/query logic to enforce the bootstrap rule without
affecting the zero-accounts/open-access path.
- Around line 73-80: The secret-key initialization in the auth DB lookup is
vulnerable to a race when multiple processes hit the fresh database at once.
Update the secret-key path in the function that reads from meta and writes the
generated value to use an idempotent insert such as INSERT OR IGNORE, then
re-query the stored secret_key and return that value instead of assuming the
insert succeeded. Keep the fix localized to the secret-key creation logic in
auth_db.py so concurrent startups cannot fail with IntegrityError.
- Around line 58-67: The get_db() initializer currently relies on
sqlite3.connect(AUTH_DB), which can create the auth database with permissions
inherited from the process umask. Update get_db() to ensure the AUTH_DB file is
pre-created and chmod’d to 0600 before opening the sqlite3 connection, and make
sure the containing directory is also created with private permissions; use the
existing get_db(), AUTH_DB, and _ensure_schema() flow as the place to apply
these permission checks.
In `@src/backend/tests/test_auth.py`:
- Around line 16-20: Move the shared autouse auth DB patching fixture into a
common conftest.py so it is applied consistently across tests; the duplicated
_patch_auth_db logic in test_auth.py, test_manage_cli.py, and
test_routes_auth.py, plus the equivalent no_auth fixture in test_routes.py,
should be removed and replaced by the centralized fixture. Keep the behavior the
same by redirecting services.auth_db.AUTH_DB to a temp file and resetting
services.auth_db._schema_ready_for to None inside the shared fixture.
In `@src/frontend/src/__tests__/components/WakeOnLan.test.tsx`:
- Around line 234-254: The viewer-account test is duplicating auth setup instead
of reusing the shared withAuth helper. Update withAuth in the WakeOnLan test
suite to accept an optional account override, then use it in the viewer case so
AUTH_STATUS and AUTH_ME mocking stays centralized and consistent with the
existing helper pattern.
In `@src/frontend/src/App.tsx`:
- Around line 82-84: The /apikeys route in App.tsx is currently exposed even
when there is no account, while the Sidebar only links to it when an account
exists. Update App/AppLayout to destructure account from useAuth() alongside
isAdmin and gate the Route for ApiKeys the same way as the nav item, so direct
navigation without an account no longer renders the empty key page.
In `@src/frontend/src/AuthProvider.tsx`:
- Around line 104-108: Move the useAuth hook out of AuthProvider.tsx into a
separate module so the AuthProvider component file only exports the provider;
this preserves Fast Refresh behavior during edits. Keep AuthProvider and
AuthContext in the current module, define useAuth in its own file, and re-export
it from the appropriate barrel or entry point if needed so callers still import
the same hook name.
In `@src/frontend/src/components/AccountModal.tsx`:
- Around line 76-82: Prevent self-demotion in AccountModal: the Role selector in
AccountModal currently allows the signed-in user to change their own AccountRole
to viewer, which can immediately affect the active session. Update the Role
field logic in AccountModal (and its role state handling) so editing the current
account either disables role changes for self-edits or requires an explicit
confirmation before allowing a self-role downgrade.
In `@src/frontend/src/components/Accounts.tsx`:
- Around line 41-54: The account deactivation flow in handleDeactivate and the
self-role editing path in AccountModal need a guard against admin self-lockout.
Hide or disable actions that target the currently logged-in admin, and in the
backend reject any request that would deactivate or demote the last active admin
account. Use the existing identifiers handleDeactivate, AccountModal, and the
account mutation/API handlers to locate and enforce the check in both UI and
server-side validation.
In `@src/frontend/src/components/ApiKeyModal.tsx`:
- Around line 21-38: The ApiKeyModal.tsx handleCreate flow uses its own
try/catch with alert instead of the shared tryAlert pattern used in
Accounts.tsx, AccountModal.tsx, and ApiKeys.tsx. Refactor handleCreate to route
the POST API_KEYS mutation through tryAlert, keeping the same success path with
setCreated(result) and preserving the existing error message behavior via
errorMessage(err).
In `@src/frontend/src/components/Login.tsx`:
- Around line 37-44: The Login form fields need proper accessibility and
autofill support. Update the Username and Password controls in Login to connect
each label to its input using matching htmlFor and id values, and add
appropriate autoComplete hints to the inputs (for example, username and
current-password) so screen readers and password managers work correctly.
In `@src/frontend/src/components/Setup.tsx`:
- Around line 48-59: The Setup form fields are missing proper label-to-input
associations and autocomplete hints. Update the input elements in Setup to use
matching id/htmlFor pairs for the Username, Password, and Confirm Password
fields, and add appropriate autoComplete attributes to the related inputs. Keep
the changes localized to the Setup component’s field markup so the labels and
browser autofill behave consistently with Login.tsx.
---
Outside diff comments:
In `@src/backend/routes/system.py`:
- Around line 24-31: The get_config_handler route currently uses require_auth
while returning raw content from get_config, which can expose sensitive config
data to non-admin users. Update the route to use require_admin for admin-only
access, or ensure get_config returns a sanitized viewer-safe version before
exposing it. Keep the check in get_config_handler aligned with ALLOWED_CONFIGS
and the existing require_auth/require_admin authorization symbols.
In `@src/frontend/src/components/WakeOnLan.tsx`:
- Around line 355-368: The Event Mappings empty-state row in WakeOnLan.tsx uses
a hardcoded colspan that does not match the number of table headers when isAdmin
changes. Update the empty-state cell in the mappings table to compute its
colSpan from the same isAdmin condition used for the Actions column, matching
the header in the table render so the layout stays aligned.
- Around line 317-328: The empty-state row in WakeOnLan’s targets table
hardcodes a column span that no longer matches the conditional Actions column in
the table header. Update the empty-state <td> in the table rendering logic so
its colSpan is derived from whether isAdmin is true, matching the <thead> and
keeping viewer/admin layouts aligned.
🪄 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
Run ID: 03510ab9-361d-498b-8cec-68617d69c583
📒 Files selected for processing (61)
.coderabbit.yaml.gitignoreAGENTS.mdCONTRIBUTING.mdMakefileREADME.mddocs/auth-plan.mddocs/modularization-plan.mdscripts/setup.shsrc/backend/app.pysrc/backend/auth.pysrc/backend/config.pysrc/backend/manage.pysrc/backend/nutwatch.servicesrc/backend/routes/__init__.pysrc/backend/routes/auth.pysrc/backend/routes/history.pysrc/backend/routes/hooks.pysrc/backend/routes/logs.pysrc/backend/routes/system.pysrc/backend/routes/ups.pysrc/backend/routes/upsmon.pysrc/backend/routes/users.pysrc/backend/routes/wol.pysrc/backend/services/auth_db.pysrc/backend/tests/test_auth.pysrc/backend/tests/test_manage_cli.pysrc/backend/tests/test_routes.pysrc/backend/tests/test_routes_auth.pysrc/backend/tests/test_services_auth_db.pysrc/frontend/src/App.tsxsrc/frontend/src/AuthProvider.tsxsrc/frontend/src/__tests__/components/Accounts.test.tsxsrc/frontend/src/__tests__/components/ApiKeys.test.tsxsrc/frontend/src/__tests__/components/AuthProvider.test.tsxsrc/frontend/src/__tests__/components/Dashboard.test.tsxsrc/frontend/src/__tests__/components/Login.test.tsxsrc/frontend/src/__tests__/components/Setup.test.tsxsrc/frontend/src/__tests__/components/UpsCard.test.tsxsrc/frontend/src/__tests__/components/WakeOnLan.test.tsxsrc/frontend/src/api.tssrc/frontend/src/components/AccountModal.tsxsrc/frontend/src/components/Accounts.tsxsrc/frontend/src/components/ApiKeyModal.tsxsrc/frontend/src/components/ApiKeys.tsxsrc/frontend/src/components/ConfigFiles.tsxsrc/frontend/src/components/Dashboard.tsxsrc/frontend/src/components/HooksSection.tsxsrc/frontend/src/components/Login.tsxsrc/frontend/src/components/LogoMark.tsxsrc/frontend/src/components/Notifications.tsxsrc/frontend/src/components/Setup.tsxsrc/frontend/src/components/Sidebar.tsxsrc/frontend/src/components/UpsCard.tsxsrc/frontend/src/components/UpsDevices.tsxsrc/frontend/src/components/Users.tsxsrc/frontend/src/components/WakeOnLan.tsxsrc/frontend/src/constants/index.tssrc/frontend/src/styles/components.csssrc/frontend/src/types.tssrc/frontend/src/utils/alerts.ts
Close several security and reliability gaps in the authentication system: Backend: - Atomic admin creation: BEGIN IMMEDIATE + count-and-insert in one transaction prevents concurrent /api/auth/setup requests from creating multiple admins. - Last-admin guard: update_account raises ValueError if the only remaining active admin is being demoted or deactivated, preventing lockout. - Secret key: validate NUTWATCH_SECRET_KEY >= 32 chars at startup; use INSERT OR IGNORE for the auto-generated key so concurrent process starts converge on one value instead of racing. - resolve_principal: on stale session (account deleted/deactivated), fall through to Authorization header resolution instead of returning None. - Password hygiene: replace --password CLI arg with --password-stdin to prevent secrets from appearing in process listings. - File permissions: auth DB directory created 0700, DB file 0600. Frontend: - Prevent self-role-change in AccountModal (locking out your own admin access mid-session). - Hide the Deactivate button for the logged-in account in the Accounts list. - Gate /apikeys route on a logged-in account (not just admin check). - Add htmlFor + autoComplete attributes on Login and Setup form fields. - Fix colSpan mismatch on WOL empty-state rows when the admin Action column is hidden for viewers.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/src/App.tsx (1)
95-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a loading indicator instead of a blank screen.
if (loading) return null;renders nothing while auth state resolves, which can appear as a blank white flash on load.🤖 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 `@src/frontend/src/App.tsx` around lines 95 - 102, The AuthGate loading path currently returns nothing, causing a blank screen while auth state resolves. Update the AuthGate component to render a loading indicator or splash UI instead of returning null when loading is true, and keep the existing bootstrapped/skipped/account branching intact so the flow still routes to Setup, Login, or AppLayout using useAuth.
♻️ Duplicate comments (1)
src/backend/services/auth_db.py (1)
162-165: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBase setup completion on admin existence, not total accounts.
SELECT COUNT(*) FROM accountsmakes/api/auth/setupreport “setup already completed” for a viewer-only DB, even though no admin exists yet. Use an admin-specific check so setup remains available until the first admin account is created. As per coding guidelines, “Keep the bootstrap rule that zero accounts means everything is open; once the first admin exists, enforce auth.”Proposed fix
- row = conn.execute("SELECT COUNT(*) AS n FROM accounts").fetchone() + row = conn.execute( + "SELECT COUNT(*) AS n FROM accounts WHERE role = 'admin'" + ).fetchone()🤖 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 `@src/backend/services/auth_db.py` around lines 162 - 165, The setup completion check in auth_db should be based on whether an admin account exists, not whether any account exists. Update the setup flow in the logic around the SELECT COUNT(*) check so it looks specifically for an admin record before returning “setup already completed,” keeping `/api/auth/setup` available when only viewer accounts exist. Use the existing setup/bootstrap path in this service and adjust the condition so the first admin creation is the trigger that closes bootstrap access.Source: Coding guidelines
🤖 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 `@src/backend/services/auth_db.py`:
- Around line 239-245: The last-admin check in auth_db.py is not atomic because
count_active_admins() runs on a separate connection before the update commits,
allowing concurrent demotions/deactivations to bypass the guard. Update the
admin update flow around this check so the count and the role/is_active change
happen on the same connection and transaction under a write lock, and keep the
guard logic tied to the update path that handles demoting/deactivating the
current admin row.
In `@src/backend/tests/test_services_auth_db.py`:
- Around line 73-76: The broad pytest.raises(ValueError) checks in
test_update_account are too generic and can pass on unrelated errors; tighten
them by adding match assertions for the last-admin invariant. Update the two
update_account() assertions to verify the specific failure message in the
auth_db test so the intent is explicit and Ruff PT011 is satisfied.
---
Outside diff comments:
In `@src/frontend/src/App.tsx`:
- Around line 95-102: The AuthGate loading path currently returns nothing,
causing a blank screen while auth state resolves. Update the AuthGate component
to render a loading indicator or splash UI instead of returning null when
loading is true, and keep the existing bootstrapped/skipped/account branching
intact so the flow still routes to Setup, Login, or AppLayout using useAuth.
---
Duplicate comments:
In `@src/backend/services/auth_db.py`:
- Around line 162-165: The setup completion check in auth_db should be based on
whether an admin account exists, not whether any account exists. Update the
setup flow in the logic around the SELECT COUNT(*) check so it looks
specifically for an admin record before returning “setup already completed,”
keeping `/api/auth/setup` available when only viewer accounts exist. Use the
existing setup/bootstrap path in this service and adjust the condition so the
first admin creation is the trigger that closes bootstrap access.
🪄 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
Run ID: 98521fe7-e04f-47c5-8229-353c0ed9f7d9
📒 Files selected for processing (14)
docs/modularization-plan.mdsrc/backend/auth.pysrc/backend/config.pysrc/backend/manage.pysrc/backend/routes/auth.pysrc/backend/services/auth_db.pysrc/backend/tests/test_auth.pysrc/backend/tests/test_services_auth_db.pysrc/frontend/src/App.tsxsrc/frontend/src/components/AccountModal.tsxsrc/frontend/src/components/Accounts.tsxsrc/frontend/src/components/Login.tsxsrc/frontend/src/components/Setup.tsxsrc/frontend/src/components/WakeOnLan.tsx
Lock down hook content read and network-host scan to admin-only. Fix a race in update_account where concurrent admin demotions could leave zero active admins — use BEGIN IMMEDIATE to atomically guard the admin-count check and the update it protects. Pull React context definitions (Auth, Theme, Confirm, Modal) into separate use*.ts modules alongside their hooks so consumers can import types and hooks without pulling in the full provider tree. Centralize the duplicated _patch_auth_db pytest fixture in conftest. Add a Loading… fallback in AuthGate instead of rendering null when auth status is resolving.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/backend/tests/conftest.py`:
- Around line 7-13: Add the missing return type annotation on _patch_auth_db in
conftest.py; this helper only patches services.auth_db.AUTH_DB and
services.auth_db._schema_ready_for via monkeypatch and does not return a value,
so annotate it explicitly as returning None to satisfy static analysis.
In `@src/backend/tests/test_routes_auth.py`:
- Around line 17-18: The private test helper _clear_login_attempts is missing an
explicit return type annotation, which Ruff flags via ANN202. Update the
function signature for _clear_login_attempts to include the appropriate return
type, and keep the implementation unchanged since it only clears _login_attempts
and returns nothing.
🪄 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
Run ID: 3f1e5035-b580-49cc-8e3d-03c12772c9be
📒 Files selected for processing (43)
src/backend/routes/hooks.pysrc/backend/routes/wol.pysrc/backend/services/auth_db.pysrc/backend/tests/conftest.pysrc/backend/tests/test_auth.pysrc/backend/tests/test_manage_cli.pysrc/backend/tests/test_routes.pysrc/backend/tests/test_routes_auth.pysrc/backend/tests/test_services_auth_db.pysrc/frontend/src/App.tsxsrc/frontend/src/AuthProvider.tsxsrc/frontend/src/__tests__/components/AuthProvider.test.tsxsrc/frontend/src/__tests__/components/ConfirmDialog.test.tsxsrc/frontend/src/__tests__/components/Modal.test.tsxsrc/frontend/src/__tests__/components/WakeOnLan.test.tsxsrc/frontend/src/__tests__/components/theme.test.tsxsrc/frontend/src/components/AccountModal.tsxsrc/frontend/src/components/Accounts.tsxsrc/frontend/src/components/ApiKeyModal.tsxsrc/frontend/src/components/ApiKeys.tsxsrc/frontend/src/components/ConfigFiles.tsxsrc/frontend/src/components/ConfirmDialog.tsxsrc/frontend/src/components/Dashboard.tsxsrc/frontend/src/components/HookEditor.tsxsrc/frontend/src/components/HooksSection.tsxsrc/frontend/src/components/Login.tsxsrc/frontend/src/components/Modal.tsxsrc/frontend/src/components/Notifications.tsxsrc/frontend/src/components/RestartPromptModal.tsxsrc/frontend/src/components/Setup.tsxsrc/frontend/src/components/Sidebar.tsxsrc/frontend/src/components/ThemeSettings.tsxsrc/frontend/src/components/UpsDevices.tsxsrc/frontend/src/components/UpsModal.tsxsrc/frontend/src/components/UserModal.tsxsrc/frontend/src/components/Users.tsxsrc/frontend/src/components/WakeOnLan.tsxsrc/frontend/src/components/useConfirm.tssrc/frontend/src/components/useModal.tssrc/frontend/src/styles/base.csssrc/frontend/src/theme.tsxsrc/frontend/src/useAuth.tssrc/frontend/src/useTheme.ts
💤 Files with no reviewable changes (2)
- src/backend/tests/test_manage_cli.py
- src/backend/tests/test_auth.py
Added `-> None` return type annotations to pytest fixture functions in `conftest.py` and `test_routes_auth.py` for consistency with type-checked code style throughout the backend tests.
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
2 participants
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.
Replace the single-secret NUTWATCH_API_KEY env var with a proper authentication system: accounts (admin/viewer roles) with session login for the dashboard and per-user API keys for scripts. The app stays fully open (bootstrap mode) until the first admin account is created via the first-run Setup page or manage.py create-admin.
Backend:
Frontend:
Tests:
Docs: updated AGENTS.md, README.md, CONTRIBUTING.md, .coderabbit.yaml; added docs/auth-plan.md; updated docs/modularization-plan.md
Summary by CodeRabbit