Add admin dashboard foundation with RBAC and security PIN (#64)#82
Merged
hrx01-dev merged 4 commits intoJun 20, 2026
Merged
Conversation
Introduce an internal /admin portal built on Firebase Auth + Firestore. - RBAC across super_admin / frontend_dev / backend_dev / qa_delivery with a fine-grained permission matrix, enforced both client-side (route guards, can()) and server-side in firestore.rules. - Dynamic, role-filtered sidebar and a responsive admin layout. - Pages: login, dashboard, projects, clients, messages, audit, settings, plus an unauthorized screen. - Security PIN (PBKDF2 / Web Crypto) gating sensitive actions, with session-based verification plus first-time setup and re-verify flows. - Typed Firestore data layer (parsers + realtime hooks) and append-only, unforgeable audit logging. - firestore.rules: RBAC enforcement, hardened public messages create rule, owner-scoped admin self-updates, immutable audit_logs. - docs/ADMIN.md: architecture, schema, setup, and security model. - Wire Firestore (db) into firebase.ts and scope the landing splash so it no longer blocks /admin and auth routes.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughIntroduces a complete ChangesAdmin Portal Foundation
Sequence Diagram(s)sequenceDiagram
participant Browser
participant AdminApp
participant AdminProvider
participant Firestore
participant PinGateProvider
participant PinDialog
Browser->>AdminApp: navigate /admin/*
AdminApp->>AdminProvider: wrap routes
AdminProvider->>Firestore: onSnapshot admins/{uid}
Firestore-->>AdminProvider: AdminProfile (or missing → bootstrap provision)
AdminProvider-->>AdminApp: isAdmin, can(permission)
AdminApp->>PinGateProvider: wrap protected routes
Note over AdminApp: ProtectedAdminRoute checks isAdmin
Note over AdminApp: RequirePermission checks can(permission)
Browser->>PinGateProvider: sensitive action triggered
PinGateProvider->>PinDialog: open PIN challenge
PinDialog-->>PinGateProvider: onSuccess / onCancel
PinGateProvider-->>Browser: execute or abort action
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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)
Comment |
…d-rbac # Conflicts: # src/Firebase/firebase.ts # src/app/App.tsx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (1)
src/admin/pages/Settings.tsx (1)
34-39: ⚡ Quick winMirror UI lock invariants inside mutation handlers.
Self-protection and “last enabled super admin” safeguards are expressed in UI state (
locked), but should also be enforced in handlers so future UI changes can’t bypass these constraints accidentally.Suggested refactor
async function handleRoleChange(row: AdminProfile, newRole: AdminRole) { + if (row.uid === currentAdmin.uid) { + toast.error("You can't change your own role."); + return; + } if (newRole === row.role) return; if (row.role === "super_admin" && !row.disabled && enabledSuperCount <= 1) { toast.error("You can't change the role of the last enabled super admin."); return; }async function handleToggleDisabled(row: AdminProfile) { + if (row.uid === currentAdmin.uid) { + toast.error("You can't change your own status."); + return; + } if (row.role === "super_admin" && !row.disabled && enabledSuperCount <= 1) { toast.error("You can't disable the last enabled super admin."); return; }Also applies to: 62-66, 154-164
🤖 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/admin/pages/Settings.tsx` around lines 34 - 39, The validation checks in handleRoleChange for preventing role changes to the last enabled super admin should be enforced in the actual mutation handler that performs the backend update, not just in the UI event handler. Move the constraint validation logic (checking if newRole equals current role, if it's the last enabled super admin, and if the admin is locked) from handleRoleChange into the underlying mutation function or API call to ensure these safeguards remain enforced even if the UI implementation changes. Apply the same refactoring pattern to all similar role and status change handlers throughout the Settings component.
🤖 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/ADMIN.md`:
- Around line 243-246: Update the documentation entry for the `/admin` route to
accurately reflect all possible redirect behaviors based on user authentication
and authorization status, rather than only showing the happy path redirect to
`/admin/dashboard`. The entry should clarify that unauthenticated users are
redirected to `/admin/login`, non-admin authenticated users are redirected to
`/admin/unauthorized`, and only authenticated admin users are redirected to
`/admin/dashboard`. Replace the current single-line description with a more
detailed explanation or conditional behavior specification that covers all three
scenarios.
- Around line 17-33: The fenced code block that starts with "Browser ──▶
AuthProvider (Firebase Auth)" is missing a language specifier, which violates
markdownlint rule MD040. Add the language identifier "text" to the opening fence
of this code block so it reads as three backticks followed by "text" instead of
just three backticks alone. This will properly declare the code block language
and satisfy the linter requirement.
- Around line 220-226: Add required blank lines around the admin configuration
table in the ADMIN.md file. Insert a blank line before the table that begins
with the header row containing Field, Type, and Value columns, and insert
another blank line after the table ends at the `createdAt` timestamp row. This
addresses the markdown linting rule MD058 which requires proper spacing around
tables for readability.
In `@firestore.rules`:
- Line 137: The current `allow update: if can('messages:reply');` rule in the
firestore.rules file permits full-document rewrites, allowing moderators to
modify immutable fields such as name, email, body, and createdAt. To fix this,
modify the update rule to include field-level validation that ensures only
mutable fields (such as reply status or moderator notes) can be updated while
protecting immutable fields like name, email, body, and createdAt from being
altered. Use Firestore security rule syntax to check the difference between the
original resource.data and the incoming request.resource.data, allowing only
changes to approved mutable fields.
In `@src/admin/components/AdminLayout.tsx`:
- Around line 19-25: The mobile sidebar overlay div in AdminLayout lacks
keyboard accessibility and has no Escape key handler to close the modal. Add an
onKeyDown event handler to the overlay div (the one with className using cn()
and aria-hidden={!mobileOpen}) that listens for the Escape key and closes the
sidebar by setting mobileOpen to false. Additionally, ensure the overlay div has
appropriate ARIA attributes (such as role="dialog" or similar) to properly
convey its modal nature to assistive technologies. Apply these keyboard and
accessibility improvements to all mobile sidebar overlay divs referenced in the
comment (around lines 19-25, 34-43, and 48-53).
In `@src/admin/components/AdminSidebar.tsx`:
- Around line 22-26: The handleSignOut function lacks error handling for the
signOut(auth) call, which means if it rejects, the function throws unhandled and
skips the navigate call, leaving the UI in a broken state without user feedback.
Wrap the signOut(auth) call in a try-catch block to explicitly handle failures,
log the error appropriately, and provide user feedback (such as an error message
or toast notification) while ensuring the navigation or error state is handled
gracefully.
In `@src/admin/context/AdminContext.tsx`:
- Around line 33-35: When the currentUser changes, the admin state should be
reset immediately to prevent stale role data from being exposed briefly. In the
AdminContext, after setting setDocLoading(true) and setError(null) at the
beginning of the effect that listens to currentUser changes, add a call to reset
the admin state (setAdmin) to null or an initial/empty value. This ensures that
role and can() methods will not reflect the previous user's permissions while
the new user's admin data is being fetched. Apply this fix at both locations
mentioned: around line 33-35 and also at lines 56-62.
In `@src/admin/context/PinProvider.tsx`:
- Around line 42-49: The PinProvider component does not properly settle the
pending promise created by challenge() when it unmounts, which can leave callers
hanging indefinitely. Add a cleanup function to the useEffect hooks mentioned
(including the one with admin?.uid dependency and the one referenced in lines
53-59) that settles the promise stored in resolverRef.current by calling it with
an appropriate value (such as false) if it exists before the component is
removed from the DOM. This ensures that any in-flight challenge() calls receive
a resolution and do not hang.
- Around line 70-74: The isVerified value on line 70 becomes stale after the TTL
expires because it only updates when the component re-renders, and Date.now() is
only evaluated at render time. To fix this, add a useEffect hook that calculates
the remaining time until verifiedUntil expires and sets up a timeout to trigger
a state update (like setting a dummy state or using a date ticker) when that
timeout completes, ensuring isVerified automatically reflects the current
verification status without requiring external state changes to force a
re-render.
In `@src/admin/lib/collections.ts`:
- Around line 73-74: The pinIterations field in the collections parser accepts
any number value without validation, which allows invalid values like NaN,
Infinity, and negative numbers to pass through to PIN verification flows that
use this as a PBKDF2 work factor. Add validation logic in the pinIterations
assignment to ensure the value is a valid positive integer (or reasonable
positive number) and reject invalid values by returning undefined or throwing an
error. The validation should check for NaN, Infinity, negative values, and
optionally enforce an upper bound to prevent performance issues.
- Line 94: The budget value assignment currently uses typeof check which allows
NaN and Infinity values to pass through since they are technically of type
"number" in JavaScript. Replace the typeof check with Number.isFinite() to
ensure the budget value is a valid finite number, filtering out both NaN and
Infinity values before assigning them to the budget property.
In `@src/admin/pages/AdminLogin.tsx`:
- Around line 47-51: The handleSignOut function does not handle errors from the
signOut call, which can leave unhandled promise rejections and provide no user
feedback on failure. Wrap the await signOut(auth) call in a try-catch block,
catching any errors and setting the error state with an appropriate error
message before setting busy to false. This fix should be applied to both the
handleSignOut function around line 47 and the similar sign-out handler around
line 90-95.
In `@src/admin/pages/Projects.tsx`:
- Around line 104-111: The budget validation in the handleCreate function only
checks if the parsed budget is a finite number using
Number.isFinite(parsedBudget), but does not reject negative values. While the
input field has a min="0" attribute for UI-level validation, the backend logic
must also validate that the budget is non-negative. Add an additional check to
ensure parsedBudget is greater than or equal to 0 in the conditional expression
that determines whether to include the budget field when adding the document to
projectsCollection. Apply this same validation fix to all locations where budget
is being persisted (including the second location mentioned at lines 391-396).
In `@src/admin/pages/Unauthorized.tsx`:
- Around line 12-15: The handleSignOut function lacks error handling around the
signOut(auth) call, which can throw an error and leave the UI in a broken state
without user feedback. Wrap the signOut(auth) call in a try-catch block, and in
the catch block, handle the error gracefully by logging it and providing user
feedback through a toast notification or error message before proceeding with
the navigation to the login page, ensuring the user is always aware of what
happened.
In `@src/admin/rbac/permissions.ts`:
- Around line 82-91: The SENSITIVE_PERMISSIONS array is missing settings:view,
which means settings access is not being gated behind PIN verification as
required by issue `#64`. Add settings:view to the SENSITIVE_PERMISSIONS array to
mark it as a PIN-sensitive capability, and update the settings route guard in
src/admin/AdminApp.tsx to verify an active PIN session in addition to checking
permissions, ensuring that accessing /admin/settings requires PIN verification
rather than just permission-based access.
---
Nitpick comments:
In `@src/admin/pages/Settings.tsx`:
- Around line 34-39: The validation checks in handleRoleChange for preventing
role changes to the last enabled super admin should be enforced in the actual
mutation handler that performs the backend update, not just in the UI event
handler. Move the constraint validation logic (checking if newRole equals
current role, if it's the last enabled super admin, and if the admin is locked)
from handleRoleChange into the underlying mutation function or API call to
ensure these safeguards remain enforced even if the UI implementation changes.
Apply the same refactoring pattern to all similar role and status change
handlers throughout the Settings component.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 15dd739d-913b-45c5-99ee-a340c02f4606
📒 Files selected for processing (43)
.env.exampledocs/ADMIN.mdfirebase.jsonfirestore.indexes.jsonfirestore.rulessrc/Firebase/firebase.tssrc/admin/AdminApp.tsxsrc/admin/components/AdminLayout.tsxsrc/admin/components/AdminLoading.tsxsrc/admin/components/AdminSidebar.tsxsrc/admin/components/EmptyState.tsxsrc/admin/components/PageHeader.tsxsrc/admin/components/PinDialog.tsxsrc/admin/components/RoleBadge.tsxsrc/admin/components/StatCard.tsxsrc/admin/components/guards/ProtectedAdminRoute.tsxsrc/admin/components/guards/RequirePermission.tsxsrc/admin/context/AdminContext.tsxsrc/admin/context/AdminContextObject.tssrc/admin/context/PinContextObject.tssrc/admin/context/PinProvider.tsxsrc/admin/context/useAdmin.tssrc/admin/context/usePinGate.tssrc/admin/hooks/useAdminData.tssrc/admin/hooks/useSensitiveAction.tssrc/admin/lib/audit.tssrc/admin/lib/authError.tssrc/admin/lib/collections.tssrc/admin/lib/format.tssrc/admin/lib/pin.tssrc/admin/pages/AdminLogin.tsxsrc/admin/pages/Audit.tsxsrc/admin/pages/Clients.tsxsrc/admin/pages/Dashboard.tsxsrc/admin/pages/Messages.tsxsrc/admin/pages/Projects.tsxsrc/admin/pages/Settings.tsxsrc/admin/pages/Unauthorized.tsxsrc/admin/rbac/navigation.tssrc/admin/rbac/permissions.tssrc/admin/rbac/roles.tssrc/admin/types.tssrc/app/App.tsx
Comment on lines
+220
to
+226
| | Field | Type | Value | | ||
| | --- | --- | --- | | ||
| | `email` | string | the admin's email | | ||
| | `displayName` | string | e.g. `Harsh Goswami` | | ||
| | `role` | string | `super_admin` | | ||
| | `disabled` | boolean | `false` | | ||
| | `createdAt` | timestamp | now | |
There was a problem hiding this comment.
Add required blank lines around the bootstrap table.
The table beginning at Line 220 is not surrounded by blank lines (MD058).
Suggested patch
2. **Firestore → Start collection** `admins` → **Document ID = that UID**, fields:
+
| Field | Type | Value |
| --- | --- | --- |
| `email` | string | the admin's email |
| `displayName` | string | e.g. `Harsh Goswami` |
| `role` | string | `super_admin` |
| `disabled` | boolean | `false` |
| `createdAt` | timestamp | now |
+
3. Visit `/admin/login`, sign in — you now have full access and can add the rest🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 220-220: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 226-226: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 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 `@docs/ADMIN.md` around lines 220 - 226, Add required blank lines around the
admin configuration table in the ADMIN.md file. Insert a blank line before the
table that begins with the header row containing Field, Type, and Value columns,
and insert another blank line after the table ends at the `createdAt` timestamp
row. This addresses the markdown linting rule MD058 which requires proper
spacing around tables for readability.
Source: Linters/SAST tools
| ); | ||
|
|
||
| allow read: if can('messages:view'); | ||
| allow update: if can('messages:reply'); |
There was a problem hiding this comment.
Lock message updates to mutable fields only.
allow update: if can('messages:reply') currently permits full-document rewrites, so moderators can alter immutable inbound fields (name, email, body, createdAt) and overwrite original submissions.
Suggested rule tightening
match /messages/{id} {
@@
- allow update: if can('messages:reply');
+ allow update: if can('messages:reply')
+ && request.resource.data.diff(resource.data).affectedKeys().hasOnly([
+ 'status'
+ ]);📝 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.
Suggested change
| allow update: if can('messages:reply'); | |
| allow update: if can('messages:reply') | |
| && request.resource.data.diff(resource.data).affectedKeys().hasOnly([ | |
| 'status' | |
| ]); |
🤖 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 `@firestore.rules` at line 137, The current `allow update: if
can('messages:reply');` rule in the firestore.rules file permits full-document
rewrites, allowing moderators to modify immutable fields such as name, email,
body, and createdAt. To fix this, modify the update rule to include field-level
validation that ensures only mutable fields (such as reply status or moderator
notes) can be updated while protecting immutable fields like name, email, body,
and createdAt from being altered. Use Firestore security rule syntax to check
the difference between the original resource.data and the incoming
request.resource.data, allowing only changes to approved mutable fields.
Harsh-goswami-103
force-pushed
the
feat/admin-dashboard-rbac
branch
from
932e58b to
51db70d
Compare
Allow viewing /admin without a Firebase backend during local development. - VITE_ADMIN_DEV_MOCK (.env.local) gates a fake super_admin + demo data, hard-gated on import.meta.env.DEV so it can never activate in a production build. The mock args are gated too, so the demo data and fake credentials are tree-shaken out of the prod bundle (verified: clean dist). - AdminContext injects the mock admin synchronously when enabled; behavior is unchanged when disabled. Data hooks serve demo collections in mock mode. - Documented in .env.example and docs/ADMIN.md (5.5). Off by default — the toggle lives only in the git-ignored .env.local.
kumudranjan6127-debug
added a commit
to kumudranjan6127-debug/Servio
that referenced
this pull request
Jun 20, 2026
…d @google/generative-ai upstream/main added AI estimation (PR hrx01-dev#83) and admin RBAC (PR hrx01-dev#82) and reverted the broken vite-plugin-ssg prerender step (PR hrx01-dev#87). DashboardLayout conflict resolved: keep our Moon/Sun theme toggle imports alongside upstream's Sparkles/Settings2 for the new nav items. Install @google/generative-ai@^0.24.1 required by the new estimationService. Closes hrx01-dev#71 — theme toggle is already implemented in both the sidebar and mobile header of DashboardLayout; this merge makes it available to all authenticated users on the latest codebase. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.
Summary
Implements the admin dashboard foundation (issue #64): an internal
/adminportal built on Firebase Authentication + Firestore, with role-based access control, dynamic navigation, route protection, a security-PIN layer for sensitive actions, and append-only audit logging.Closes #64
What's included
super_admin/frontend_dev/backend_dev/qa_deliverywith a fine-grained permission matrix, enforced client-side (route guards +can()) and server-side infirestore.rules.ProtectedAdminRoute,RequirePermission, real-timeadmins/{uid}role loading, and an unauthorized screen.messagescreate rule, owner-scoped admin self-updates, immutableaudit_logs.docs/ADMIN.md— architecture, Firestore schema, setup, and security model.Quality
npm run typecheck,npm run lint, andnpm run buildall pass.backend_dev, PIN edge cases, write-failure feedback, and scoping the landing splash so it no longer blocks/adminand auth routes.Setup required before use (Firebase console, project
servio-0)admins/{uid}document withrole: "super_admin",disabled: false,email,displayName(rules only let an existing super admin create others). Seedocs/ADMIN.md§5.3.firebase deploy --only firestore:rules.Firebase web config is read from
.env.local(git-ignored); see.env.example.Notes / follow-ups
docs/ADMIN.md§8.Summary by CodeRabbit
Release Notes
/adminwith login, dashboard, projects, clients, messages, audit log, and settings pages.some of the glimpse