Conversation
| await onUserUpdate(user); | ||
| unsubscribe = auth.onAuthStateChanged(async firebaseUser => { | ||
| if (firebaseUser) { | ||
| await onUserUpdate(firebaseUser as User); |
There was a problem hiding this comment.
Type casting like that can cause a lot of troubles. It hides type drift — prefer explicit mappers or using zod across the stack.
| user: result.user, | ||
| }; | ||
| createUserMutation.mutate(data); | ||
| const firebaseUser = result.user as User; |
| const navigate = useNavigate(); | ||
| const location = useLocation(); | ||
|
|
||
| const from = (location.state as { from?: string } | null)?.from ?? '/'; |
There was a problem hiding this comment.
This line mixes three concerns (cast, optional chaining, fallback) and duplicates the state shape. The as cast is also unchecked — malformed state would slip through.
Could we extract a small typed helper so the contract lives in one place?
|
|
||
| class UpdateRoleRequest { | ||
| @ApiProperty({ enum: UserRole }) | ||
| role: UserRole; |
There was a problem hiding this comment.
role is not validated at runtime. body.role is typed as UserRole, but TypeScript types are erased at runtime — nothing here stops a client from sending { "role": "superadmin" }. Because role is stored as a plain varchar (SQLite has no native enum), that string gets persisted as-is. Every subsequent RolesGuard comparison then fails closed (which happens to be safe today — 403), but the DB row is now in an invalid state and any future hierarchy / admin-implies-user logic will misbehave.
Two small changes fix it:
- Add
class-validatoron the DTO field. - Move
UpdateRoleRequestout of the controller intodto/update_role.ts— every other DTO in this project lives underdto/(seecreate_user.ts).
| role: UserRole; | |
| class UpdateRoleRequest { | |
| @ApiProperty({ enum: UserRole, enumName: 'UserRole' }) | |
| @IsEnum(UserRole) | |
| role: UserRole; | |
| } |
(You'll also need import { IsEnum } from 'class-validator'; at the top, and to confirm the global ValidationPipe is registered in main.ts — otherwise the decorator is decorative only.)
Follow-up in a separate commit: delete this inline class and replace with import { UpdateRoleRequest } from './dto/update_role';.
bohdanKulcyckyj
left a comment
bohdanKulcyckyj
left a comment
There was a problem hiding this comment.
Blockers before merge
Following up on my earlier review (the inline suggestion on UpdateRoleRequest validation is also still open). These are the remaining items that have to be fixed in this PR — deferring them is not safe:
- Migration — see inline on the migration file.
synchronize: truehas been carrying the schema; the new migration recreates all 12 tables and will fail on any existing DB. - DTO boundary on
/auth/*— see inline onauth.controller.ts. Both endpoints leakemployee_id; once a client ships we can't remove it. - Frontend zombie session — see inline on
AuthProvider.tsx. 404 from/auth/meis silently swallowed → user appears logged in but has no role. - First-admin bootstrap — a fresh DB has no admin, and the only promotion endpoint (
PATCH /users/:id/role) is itself admin-guarded. That's a chicken-and-egg: the deployed app cannot be administered until somebody edits the DB by hand. Please add either a one-shot seed script or anFIRST_ADMIN_EMAILenv-driven bootstrap insideupsertByGoogleWorkspaceToken. This has to be in this PR because #2 in this series is what introduced the role column. - PR description is empty. For a 30-file / +1129 change, please fill it in before merge: what's in scope, what's out of scope, testing notes, screenshots for the login page changes, and
Closes #1 #2 #3 #4 #5 #6so the issues auto-close. This isn't follow-up-able — the PR description is the artifact that ships.
Everything else I noticed (guard DB round-trip, duplicated RolesGuard provider registration, circular forwardRef, profiq-ui adoption, token-expiration handling, generated OpenAPI types, magic-string 'admin', missing AdminRoute / upsertByGoogleWorkspaceToken tests) I'm fine with tracking as separate follow-up issues — they don't block merge.
Process note for next time: please stop bundling 6 issues into one PR. Your per-commit discipline is good; apply the same discipline at the PR boundary (stacked PRs, or at minimum split migration / backend guard / UI / auth API). I flagged this in the earlier review too.
|
|
||
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query( | ||
| `CREATE TABLE "user" ("id" integer PRIMARY KEY AUTOINCREMENT NOT NULL, "name" varchar NOT NULL, "employee_id" varchar NOT NULL, "role" varchar NOT NULL DEFAULT ('user'))` |
There was a problem hiding this comment.
This migration recreates the entire schema from scratch — 12 tables, plus TypeORM's SQLite temporary_* rewrite dance later in the file. None of that is about adding role. It looks like typeorm migration:generate was run against an empty DB and the output was committed wholesale.
The project has been running on synchronize: true since inception, so on any existing database (staging, anyone's local, CI if it persists state) this CREATE TABLE "user" fails immediately — the table is already there.
This file has to be replaced with a one-liner incremental migration:
await queryRunner.query(
`ALTER TABLE "user" ADD COLUMN "role" varchar NOT NULL DEFAULT 'user'`
);Baselining the rest of the schema into migrations is a valuable thing to do, but it's a separate, well-scoped PR — not a side effect of adding one column.
Related: datasource.ts still runs migrationsRun only when NODE_ENV === 'production' and synchronize is still on in dev, so the e2e suite never exercises the migration. That's how this slipped through. Please flip that as part of the fix so CI actually runs migrations.
| req.firebaseUser | ||
| ); | ||
| if (user === null) { | ||
| throw new NotFoundException('User not found in employee directory'); |
There was a problem hiding this comment.
Two problems on this endpoint that have to be fixed before we ship a client:
1. employee_id should never cross the wire. The response type is User (the TypeORM entity), which means employee_id is serialized into every /auth/login and /auth/me response. Today employee_id is just an opaque directory key, but it's the exact kind of identifier that gets joined to HR data (payroll, performance, contracts) the first time Ops asks us for a "simple" report. Once it's on the browser it's in logs, in analytics payloads, in error-reporting tools like Sentry — and you can't reel it back in. The fix has to happen before this endpoint has real clients.
Please introduce a UserResponseDto (class-transformer @Expose on only the fields the frontend actually needs — id, name, role) and return that from both login here and getMe below. The frontend already hand-duplicates this shape as DbUser in services/auth/auth.ts, which is a symptom — the wire contract should be a deliberate DTO, not the entity leaking out whatever columns happen to exist.
2. Misleading 404 message. upsertByGoogleWorkspaceToken returns null for two distinct cases: (a) the Firebase token has no google.com identity, and (b) the user isn't in the employee directory. Both get mapped to 'User not found in employee directory' here, which is wrong for case (a) — the client can't tell "your SSO isn't Google" from "you're not an employee", which is a terrible debugging experience for whoever hits it.
Please either return a discriminated result from the service (e.g. { ok: false, reason: 'no-google-identity' | 'not-in-directory' }) or throw typed exceptions from the service and catch them here with appropriate messages.
Why both are merge-blockers: the DTO leak is hard to remove once clients depend on it (and actively dangerous as soon as HR data is in the picture), and the 404 message gets screenshotted into Slack during the first incident and then nobody trusts it.
| req.firebaseUser | ||
| ); | ||
| if (!user) { | ||
| throw new NotFoundException('User not found'); |
There was a problem hiding this comment.
/auth/me doesn't upsert — it only reads. That creates a race: right after a successful /auth/login on a fresh account, the next /auth/me call (on another tab, after a reload, after a token refresh) can 404 if anything went sideways with the upsert.
More importantly, the frontend currently swallows this 404 silently (see the AuthProvider.tsx comment) and the app enters a zombie state: user is set, role is null, nothing surfaces to the user.
Pick one of:
- Make
/auth/meupsert-if-missing (same code path as/auth/login), or - Have the frontend call
loginToBackendon 404 from/auth/meand retry.
I have a slight preference for the first — /auth/me being idempotent and self-healing means the frontend doesn't need to know about this edge case at all. But either is fine, as long as the contract is explicit and the zombie state is impossible.
(Same DTO-leak issue from line 36 applies here too.)
| return; | ||
| } | ||
| // User not in DB yet — will be created on next login | ||
| setRole(null); |
There was a problem hiding this comment.
The comment says "User not in DB yet — will be created on next login" but nothing schedules a next login. After onAuthStateChanged fires on app boot for a freshly-signed-up-but-never-login'd user, we end up with user set, role = null, no error surfaced, and no retry — a zombie session where the UI thinks you're logged in but you can't do anything.
The same pattern appears on line 99-100 (the login() catch): loginToBackend fails → setRole(null) → Login.tsx sees no thrown error → navigates into the app. Broken backend = silent zombie.
Two fixes needed here:
- On 404 from
/auth/me: callloginToBackend(firebaseUser)to force the upsert, then re-read. Or (preferred) make/auth/meupsert itself on the backend (see my comment onauth.controller.tsline 55) and then this whole branch is justcatch (err) { throw err }. - On any other error from
getMe/loginToBackend: surface it. Either return it fromlogin()soLogin.tsxcan display it, or callauth.signOut()so we don't sit in a half-authenticated state. TheForbiddenError → signOutpath above (line 47-50) is the right pattern; generalize it.
Why this is a merge-blocker: it's a user-facing correctness bug, not polish. A user who hits this path is stuck on a blank admin-less app with no way to recover except clearing storage.
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive authentication and role-based access control (RBAC) system. The backend adds Firebase-authenticated endpoints with bearer token verification, user role management, and admin-restricted routes protected by guards and decorators. The frontend integrates role-based UI rendering and admin route protection. Database migrations add the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Frontend Client
participant AuthCtrl as AuthController
participant AuthGuard
participant UserSvc as UserService
participant DB as Database
participant EmpSvc as EmployeeService
Client->>AuthCtrl: POST /auth/login<br/>(Bearer token)
AuthCtrl->>AuthGuard: Verify token
AuthGuard->>AuthGuard: Validate email domain
AuthGuard->>AuthCtrl: Attach firebaseUser
AuthCtrl->>UserSvc: upsertByGoogleWorkspaceToken(token)
alt Google identity present
UserSvc->>DB: Check user by employee_id
alt User exists
UserSvc->>UserSvc: Return existing user
else User missing
UserSvc->>EmpSvc: getEmployee(googleUid)
alt Employee found
EmpSvc->>UserSvc: Return employee
UserSvc->>DB: Create new User
UserSvc->>DB: Save user
UserSvc->>UserSvc: Return new user
else Employee not in directory
UserSvc->>UserSvc: Return not-in-directory error
end
end
else No Google identity
UserSvc->>UserSvc: Return no-google-identity error
end
UserSvc->>AuthCtrl: Return UpsertResult
AuthCtrl->>Client: HTTP 200 + UserResponseDto
sequenceDiagram
participant Client as Frontend Client
participant RolesGuard
participant UserCtrl as UserController
participant UserSvc as UserService
participant DB as Database
Client->>RolesGuard: PATCH /users/:id/role<br/>(Bearer token)
RolesGuard->>RolesGuard: Extract required roles<br/>from `@Roles` metadata
RolesGuard->>RolesGuard: Verify firebaseUser<br/>from request
RolesGuard->>UserSvc: getUserByGoogleWorkspaceUid(token)
UserSvc->>DB: Find user by token
alt User found & has required role
RolesGuard->>UserCtrl: Allow access
UserCtrl->>UserSvc: updateUserRole(id, role)
UserSvc->>DB: Update user.role
UserSvc->>UserCtrl: Return updated user
UserCtrl->>Client: HTTP 200 + User
else User missing or role insufficient
RolesGuard->>Client: HTTP 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 6
🧹 Nitpick comments (16)
frontend/src/lib/providers/auth/domain.ts (1)
3-8: Domain string duplicated with backend; consider config-driven value.
'profiq.com'is also hardcoded inbackend/src/auth/auth.guard.ts(Line 12:endsWith('@profiq.com')). Any future change (e.g., staging/alternate tenant) requires edits in both places and risks FE/BE drift. Consider sourcing this from env (e.g.,VITE_ALLOWED_EMAIL_DOMAIN) on the frontend and a matching backend config key.Also note:
email?.split('@')[1]picks the second segment rather than the last, so a pathological address likea@b@profiq.comwould resolve to'b'on the frontend but still pass the backend'sendsWith('@profiq.com')check — a minor FE/BE inconsistency. Usingemail.slice(email.lastIndexOf('@') + 1)would match backend semantics more closely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/providers/auth/domain.ts` around lines 3 - 8, DOMAIN is hardcoded and parsing uses split('@')[1], causing FE/BE drift and incorrect parsing for addresses with multiple '@'; change DOMAIN to read from the environment (e.g., import.meta.env.VITE_ALLOWED_EMAIL_DOMAIN with a sensible fallback) and update checkDomain to compute the domain using email.slice(email.lastIndexOf('@') + 1) (handle undefined/empty email) and compare lowercased/trimmed values against DOMAIN so frontend matches backend semantics; update references to DOMAIN and the checkDomain(UserInfo) function accordingly.backend/src/auth/auth.guard.ts (1)
12-16: Extract domain to shared configuration to avoid FE/BE drift.The
@profiq.comdomain is hardcoded here and inbackend/src/employee/employee.service.ts, while the frontend defines it as a constant infrontend/src/lib/providers/auth/domain.ts. Moving this to a centralized backend configuration (e.g.,backend/src/config/configuration.ts) or a shared constant would prevent duplication and make staging/multi-tenant deployments easier.Also consider having
AuthService.verifyToken()throw an exception on missing or invalid tokens instead of returning{ email: undefined }. Currently, the guard returnsfalse(which NestJS treats as403 Forbidden), but401 Unauthorizedwould be semantically more accurate for authentication failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/auth/auth.guard.ts` around lines 12 - 16, Hardcoded domain "@profiq.com" in the auth guard causes FE/BE drift and auth failures; extract this domain into a centralized backend config (e.g., add a DOMAIN or ALLOWED_EMAIL_DOMAIN constant in backend/src/config/configuration.ts) and import that constant into auth.guard.ts and employee.service.ts (referencing the token check in AuthGuard and any domain logic in EmployeeService) so both use the same source of truth. Also update AuthService.verifyToken() to throw an error for missing/invalid tokens instead of returning { email: undefined }, and change the guard (AuthGuard) to catch that error and rethrow a NestJS UnauthorizedException (401) rather than returning false, ensuring authentication failures produce 401s not 403s.backend/src/main.ts (1)
23-23: StrengthenValidationPipeconfiguration with security-focused options.
new ValidationPipe()with defaults allows unknown properties to pass through and does not transform payloads into DTO instances. For thePATCH :id/roleendpoint withUpdateRoleRequest, enablingwhitelist,forbidNonWhitelisted, andtransformis a recommended security hardening practice — it prevents clients from submitting unexpected fields, provides explicit feedback on invalid input, and ensures@IsEnum(UserRole)validates against properly coerced values.♻️ Suggested options
- app.useGlobalPipes(new ValidationPipe()); + app.useGlobalPipes( + new ValidationPipe({ + whitelist: true, + forbidNonWhitelisted: true, + transform: true, + }) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main.ts` at line 23, The global ValidationPipe is instantiated with defaults which allow unknown properties and do not transform payloads; update the instantiation where app.useGlobalPipes(new ValidationPipe()) is called to enable security-focused options: set whitelist: true, forbidNonWhitelisted: true, and transform: true so DTOs (e.g., UpdateRoleRequest) are transformed and extraneous fields are stripped/forbidden and validation like `@IsEnum`(UserRole) operates on coerced values; keep the same ValidationPipe usage but pass the options object to enforce these rules globally.frontend/src/components/navigation-menu-reference.tsx (1)
49-49: Optional: avoid hard-coded role string.Consider comparing against the
UserRoletype/constant from@/lib/contexts(or a shared constant) to avoid drift if the role string ever changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/navigation-menu-reference.tsx` at line 49, Replace the hard-coded role string check (role === 'admin') with a comparison against the shared UserRole constant/enum from your auth context; import UserRole from "@/lib/contexts" and use the appropriate member (e.g., UserRole.Admin or UserRole.ADMIN) in the conditional where the variable role is checked to prevent drift if the role string changes.backend/src/admin/admin.module.ts (1)
11-11: Optional:RolesGuardregistration inprovidersis redundant.
RolesGuardis applied via@UseGuards(AuthGuard, RolesGuard)at the controller level, which instantiates the class directly using the module's injector — it does not need to be listed inproviders. It's also already registered as a provider inUserModule. You can drop it here without affecting behavior.♻️ Proposed tweak
- providers: [AdminService, RolesGuard], + providers: [AdminService],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/admin/admin.module.ts` at line 11, Remove the redundant RolesGuard entry from the AdminModule providers array: in admin.module.ts, delete RolesGuard from the providers list (leaving AdminService) since RolesGuard is already provided in UserModule and applied via `@UseGuards`(AuthGuard, RolesGuard) at controller level; this keeps DI behavior unchanged while avoiding duplicate registration.frontend/src/components/ProtectedRoute.tsx (1)
20-24: Consider preserving search/hash alongside pathname.
state={{ from: location.pathname }}dropslocation.searchandlocation.hash, so post-login redirect won't restore query params (filters, tabs, etc.). Minor; passinglocation.pathname + location.search + location.hash(or the wholelocationobject) would be more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ProtectedRoute.tsx` around lines 20 - 24, The current redirect drops query and hash because ProtectedRoute sets state={{ from: location.pathname }}; update the Navigate state to include the full location (e.g., state={{ from: location.pathname + location.search + location.hash }} or simply state={{ from: location }}) so post-login redirects restore query params and fragment; change the state passed to Navigate in ProtectedRoute.tsx accordingly.frontend/src/components/AdminRoute.tsx (1)
18-27: Access Denied block is a dead-end UX.No link back to a safe route (e.g., home) and no semantic role/landmark. Consider adding a "Go home" link/button and wrapping in
<main role="main">or similar to help screen-reader users and accidental visitors recover.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/AdminRoute.tsx` around lines 18 - 27, The Access Denied block in the AdminRoute component (the conditional that checks role !== 'admin') should provide a recovery path and semantic landmark: wrap the returned JSX in a <main role="main"> landmark and add a visible "Go home" link or button that navigates to the safe route (e.g., "/") — ensure the control uses a semantic element (anchor or button) with an accessible label/aria-label and keyboard-focusable styling; update AdminRoute's return for non-admins to include these changes so screen-reader users and accidental visitors can recover.frontend/src/services/auth/auth.ts (1)
16-38: Optional: extract a small helper to remove duplication and guard against missingerror.message.
loginToBackendandgetMeare structurally identical except for method/path. Additionally,result.error.messageassumes the backend always returns{ message: ... }; if not, the thrown error will surface anundefinedmessage. A small helper removes the copy/paste and makes the fallback explicit.♻️ Proposed refactor
+async function callAuth( + user: User, + method: HttpMethod, + path: string +): Promise<DbUser> { + const client = new APIClient(user); + const result: APIResponse<DbUser> = await client.fetch(method, path); + if (!result.success) { + throw createError( + result.status_code, + result.error?.message ?? 'Request failed' + ); + } + return result.data; +} + -export async function loginToBackend(user: User): Promise<DbUser> { - const client = new APIClient(user); - const result: APIResponse<DbUser> = await client.fetch( - HttpMethod.Post, - '/auth/login' - ); - if (!result.success) { - throw createError(result.status_code, result.error.message); - } - return result.data; -} - -export async function getMe(user: User): Promise<DbUser> { - const client = new APIClient(user); - const result: APIResponse<DbUser> = await client.fetch( - HttpMethod.Get, - '/auth/me' - ); - if (!result.success) { - throw createError(result.status_code, result.error.message); - } - return result.data; -} +export const loginToBackend = (user: User) => + callAuth(user, HttpMethod.Post, '/auth/login'); + +export const getMe = (user: User) => + callAuth(user, HttpMethod.Get, '/auth/me');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/services/auth/auth.ts` around lines 16 - 38, Extract a small helper (e.g., handleApiResponse or unwrapApiResponse) that accepts an APIResponse<DbUser> returned from APIClient.fetch and centralizes the success check: if result.success return result.data, otherwise throw createError(result.status_code, result.error?.message ?? 'Unknown error'); then update loginToBackend and getMe to call this helper after their client.fetch calls instead of duplicating the success/error logic so you remove copy/paste and guard against missing error.message from APIResponse.backend/src/user/user.service.spec.ts (1)
29-105: Recommend adding tests forupsertByGoogleWorkspaceToken— especially theFIRST_ADMIN_EMAILbranch.
upsertByGoogleWorkspaceTokendecides who is provisioned asUserRole.Adminbased on aprocess.env.FIRST_ADMIN_EMAILmatch (seebackend/src/user/user.service.ts:69-97). That's a security-sensitive branch — please add tests that cover:
no-google-identity→ returns{ error: 'no-google-identity' }.- Existing user → returned as-is with their stored role (not re-assigned).
- Not in directory →
{ error: 'not-in-directory' }.- New user with email matching
FIRST_ADMIN_EMAIL→ saved withrole: UserRole.Admin.- New user with non-matching email → saved with
role: UserRole.User.FIRST_ADMIN_EMAILunset → never escalates to Admin.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/user/user.service.spec.ts` around lines 29 - 105, Add comprehensive unit tests for UserService.upsertByGoogleWorkspaceToken covering the FIRST_ADMIN_EMAIL branch: write tests for (1) token missing google.com identity returns { error: 'no-google-identity' }, (2) existing user in repository is returned unchanged (preserves stored role), (3) when employeeService cannot find user in directory return { error: 'not-in-directory' }, (4) new user whose email matches process.env.FIRST_ADMIN_EMAIL is saved with role UserRole.Admin, (5) new user whose email does not match is saved with role UserRole.User, and (6) when process.env.FIRST_ADMIN_EMAIL is unset no escalation to Admin occurs; use the existing mocks (mockRepository, mockEmployeeService) and methods (mockRepository.findOne, mockRepository.save, mockEmployeeService.getByWorkspaceId) and assert returned values and that save is called with expected role.backend/src/db/migrations/1776082215808-InitialSchema.ts (1)
1-16: Approved — thanks for addressing the earlier feedback.The migration is now a minimal incremental change instead of recreating the whole schema, which matches the intent of this PR and won't conflict with existing databases.
One small nit: the filename is still
1776082215808-InitialSchema.tswhile the class isAddRoleToUser1776082215808. Consider renaming the file to1776082215808-AddRoleToUser.tsto match and avoid confusion for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/db/migrations/1776082215808-InitialSchema.ts` around lines 1 - 16, Rename the migration file to match the class name to avoid confusion: change the filename from 1776082215808-InitialSchema.ts to 1776082215808-AddRoleToUser.ts and ensure the exported class name AddRoleToUser1776082215808 remains unchanged so tooling and humans can match file ↔ class (no code changes needed inside the file).frontend/src/lib/contexts.tsx (1)
9-35: LGTM — discriminated union variants consistently carryrole.Nit:
UserRoleduplicates the string values of the backendUserRoleenum (backend/src/user/user.entity.ts). If the enum is ever extended (e.g., amanagerrole), both sides need manual updates. Consider colocating a single source of truth (e.g., a generated types package or a shared constants file) if the project grows more roles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/contexts.tsx` around lines 9 - 35, The UserRole string union duplicates the backend UserRole enum which will drift if roles change; replace the local UserRole declaration by importing the shared/generated role type or constants and update usages (e.g., the UserRole type and any references inside AuthContextType) to use that single source of truth (or wire up a codegen step that emits a frontend type from the backend enum) so frontend and backend stay in sync.frontend/src/routes/Login.tsx (1)
16-21: Optional: constrainfromto internal paths.
getRedirectFromaccepts any string forfrom. Today the only producer isProtectedRoutepassinglocation.pathname, so this is safe. If a future caller ever forwards a user-controlled value (e.g. a?redirect=query param) into this state, a value like//evil.com/xwould be treated as an absolute URL bynavigate. A one-line guard keeps the contract robust against that drift.🛡️ Proposed hardening
function getRedirectFrom(state: unknown): string { const parsed = loginStateSchema.safeParse(state); - return parsed.success ? (parsed.data?.from ?? '/') : '/'; + const from = parsed.success ? parsed.data?.from : undefined; + return from && from.startsWith('/') && !from.startsWith('//') ? from : '/'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/Login.tsx` around lines 16 - 21, getRedirectFrom currently accepts any string for the "from" field (loginStateSchema) which could allow absolute URLs like "//evil.com" to be treated as safe redirects; tighten the schema or add a one-line guard in getRedirectFrom to only accept internal paths (e.g., require strings that start with a single "/" and do not start with "//" and do not contain "://"); update loginStateSchema (or validate inside getRedirectFrom) to enforce that constraint and fall back to "/" when the value fails validation so navigate never receives an external URL.backend/src/auth/auth.controller.ts (2)
22-32: Optional: lift theUserRoleimport to the top.Inline
import('@/user/user.entity').UserRoleworks but is harder to scan; a regularimport type { UserRole } from '@/user/user.entity'at the top keeps types consistent with the rest of the file. Even simpler: type the parameter asPick<User, 'id' | 'name' | 'role'>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/auth/auth.controller.ts` around lines 22 - 32, The inline type import for UserRole in toUserResponseDto makes the signature harder to read; replace the inline import by adding a top-level type import (e.g. import type { UserRole, User } from '@/user/user.entity') and then change the parameter type to either Pick<User, 'id' | 'name' | 'role'> or use UserRole explicitly, keeping the function name toUserResponseDto and return type UserResponseDto unchanged; update the file-level imports and the function signature accordingly so types are consistent and easier to scan.
41-81: Nice — previous review concerns are all addressed (DTO, discriminated errors,/auth/meupsert).Small follow-up:
loginandgetMeare byte-identical apart from the decorator. Extracting the upsert + error mapping keeps the two endpoints from drifting when the error matrix grows.♻️ Suggested dedup
+ private async upsertAndMap(req: FirebaseRequest): Promise<UserResponseDto> { + const result = await this.userService.upsertByGoogleWorkspaceToken( + req.firebaseUser + ); + if ('error' in result) { + if (result.error === 'no-google-identity') { + throw new BadRequestException( + 'Token does not contain a Google identity' + ); + } + throw new NotFoundException('User not found in employee directory'); + } + return toUserResponseDto(result.user); + } + `@Post`('login') `@HttpCode`(HttpStatus.OK) `@ApiOkResponse`({ type: UserResponseDto }) - async login(`@Req`() req: FirebaseRequest): Promise<UserResponseDto> { - const result = await this.userService.upsertByGoogleWorkspaceToken( - req.firebaseUser - ); - if ('error' in result) { - if (result.error === 'no-google-identity') { - throw new BadRequestException( - 'Token does not contain a Google identity' - ); - } - throw new NotFoundException('User not found in employee directory'); - } - return toUserResponseDto(result.user); - } + login(`@Req`() req: FirebaseRequest): Promise<UserResponseDto> { + return this.upsertAndMap(req); + } @@ `@Get`('me') `@ApiOkResponse`({ type: UserResponseDto }) - async getMe(`@Req`() req: FirebaseRequest): Promise<UserResponseDto> { - const result = await this.userService.upsertByGoogleWorkspaceToken( - req.firebaseUser - ); - if ('error' in result) { - if (result.error === 'no-google-identity') { - throw new BadRequestException( - 'Token does not contain a Google identity' - ); - } - throw new NotFoundException('User not found in employee directory'); - } - return toUserResponseDto(result.user); - } + getMe(`@Req`() req: FirebaseRequest): Promise<UserResponseDto> { + return this.upsertAndMap(req); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/auth/auth.controller.ts` around lines 41 - 81, login and getMe each call userService.upsertByGoogleWorkspaceToken(req.firebaseUser) and duplicate the same error mapping and return logic; extract that shared logic into a single private helper (e.g., resolveUserFromFirebase or handleUpsertAndMapErrors) that calls userService.upsertByGoogleWorkspaceToken, maps the discriminated errors ('no-google-identity' -> BadRequestException, other -> NotFoundException) and returns toUserResponseDto(result.user); then have both login and getMe simply call the new helper with req.firebaseUser to avoid drift and centralize error handling.frontend/src/lib/providers/auth/AuthProvider.tsx (1)
24-26:toUseris an identity function — consider dropping it or making it a real mapper.Today
toUser(fbUser)just returnsfbUserwith a narrowed type, which meansUserandFirebaseUserhave to stay structurally identical for this to keep compiling. Either:
- remove the helper and assign
FirebaseUserdirectly (the call sites already rely on structural compatibility), or- make
Usera genuinely narrower shape ({ uid, email, displayName, ... }) and havetoUseractually project to it — which gives you a real boundary against Firebase SDK drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/providers/auth/AuthProvider.tsx` around lines 24 - 26, toUser currently just returns its FirebaseUser input, providing no real mapping between FirebaseUser and your app-level User; either remove toUser and use FirebaseUser directly everywhere, or make User a narrower type (e.g., only uid, email, displayName, photoURL) and implement toUser to project fbUser -> { uid: fbUser.uid, email: fbUser.email, displayName: fbUser.displayName, ... } so call sites receive the concrete User shape; update any usages that assume structural identity to use the new User properties or the original FirebaseUser as appropriate and adjust types for toUser, User, and FirebaseUser accordingly.backend/src/user/user.service.ts (1)
56-97: Recommend extracting the Google UID extraction.
getUserByGoogleWorkspaceUidandupsertByGoogleWorkspaceTokenrepeat the same identity-array cast and?.[0]lookup. A private helper keeps the shape assumptions (and any futureiss-based fallbacks) in one place.♻️ Suggested helper
+ private extractGoogleUid(token: { + firebase?: { identities?: Record<string, unknown> }; + }): string | null { + const uid = ( + token.firebase?.identities?.['google.com'] as string[] | undefined + )?.[0]; + return uid ?? null; + } + async getUserByGoogleWorkspaceUid(token: { uid: string; firebase?: { identities?: Record<string, unknown> }; }): Promise<User | null> { - const googleUid = ( - token.firebase?.identities?.['google.com'] as string[] | undefined - )?.[0]; + const googleUid = this.extractGoogleUid(token); if (!googleUid) { return null; } return this.getUserByEmployeeId(googleUid); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/user/user.service.ts` around lines 56 - 97, Create a private helper (e.g., private getGoogleUidFromToken(token: { uid: string; firebase?: { identities?: Record<string, unknown> } }): string | undefined) that encapsulates the cast and first-element lookup currently duplicated in getUserByGoogleWorkspaceUid and upsertByGoogleWorkspaceToken (i.e., return (token.firebase?.identities?.['google.com'] as string[] | undefined)?.[0]). Replace the duplicated logic in getUserByGoogleWorkspaceUid to call this helper and return null when it yields undefined, and in upsertByGoogleWorkspaceToken call the helper and return { error: 'no-google-identity' } when it yields undefined; preserve all other behavior (use existing employeeService.getEmployee, role assignment, and repository.save).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Line 15: The FIRST_ADMIN_EMAIL check currently compares employee.email ===
process.env.FIRST_ADMIN_EMAIL case-sensitively which can silently fail; update
the comparison in backend/src/user/user.service.ts (the block that promotes the
first admin around the employee.email check) to normalize both sides with
.trim().toLowerCase() before comparing (e.g., normalize employee.email and
process.env.FIRST_ADMIN_EMAIL) so emails match regardless of case/whitespace,
and add a short comment in .env.example next to FIRST_ADMIN_EMAIL noting it must
match the first-provisioned admin email (case/whitespace are ignored) and that
it only applies during initial provisioning.
In `@backend/src/datasource.ts`:
- Around line 22-24: Add a new baseline migration file named with an earlier
timestamp (e.g., 1776082215807-InitialSchema.ts) that runs before the existing
1776082215808-InitialSchema.ts and creates the missing tables (at minimum the
"user" table) referenced by your entities; implement proper up() to CREATE TABLE
"user" (and any other entity tables used by later migrations) and down() to DROP
them, so that with the current TypeORM config (migrationsRun: true and the
migrations array in datasource.ts) the ALTER in 1776082215808-InitialSchema.ts
will succeed on clean DBs. Ensure the migration class name and filename are
unique and the SQL/schema matches your entity definitions.
In `@backend/src/user/user.controller.ts`:
- Around line 68-81: The update allows an admin to demote themselves or remove
the last admin; modify the updateUserRole flow to prevent that: change the
updateUserRole signature (and update the controller call in updateRole) to
accept the acting user id (e.g., currentUserId from request) so the service can
detect self-demotion, then inside userService.updateUserRole check (1) if id ===
currentUserId and the new role is not UserRole.Admin then reject with a
Forbidden/BadRequest, and (2) before applying a demotion of any admin,
query/count current admins (e.g., via userRepository.count({ role:
UserRole.Admin })) and reject any change that would result in zero admins; keep
throwing UnknownUserException when the target user is missing and use
appropriate HTTP error for forbidden actions.
In `@backend/src/user/user.service.ts`:
- Around line 69-97: upsertByGoogleWorkspaceToken currently does a
check-then-act (calls getUserByEmployeeId then userRepository.save) which races
and can throw a unique-constraint QueryFailedError; change it to either (A)
perform an atomic upsert via TypeORM's upsert([], { conflictPaths:
['employee_id'] }) on the User entity instead of save, or (B) wrap the save in a
try/catch that detects the DB unique-violation (e.g., Postgres 23505 /
QueryFailedError) and on conflict re-read the existing row with
getUserByEmployeeId and return that user; also extract the googleUid extraction
logic used in upsertByGoogleWorkspaceToken into a small shared helper and reuse
it from getUserByGoogleWorkspaceUid to avoid duplication.
In `@backend/test/auth.e2e-spec.ts`:
- Around line 62-64: The afterEach hook currently calls await app.close()
unguarded; if buildApp()/beforeAll failed and app is undefined this masks the
original error. Modify the afterEach to first check that the test app is
initialized and has a close method (e.g. if (app && typeof app.close ===
'function') await app.close()) so it only calls close when safe; reference the
afterEach hook and the app variable created by buildApp()/beforeAll to locate
where to apply this guard.
In `@frontend/src/components/AdminRoute.tsx`:
- Around line 15-17: AdminRoute currently returns <Navigate to='/login' />
without using replace or passing the original location, so unauthenticated users
create an extra history entry and Login.tsx cannot redirect back; update the
AdminRoute component to read the current location via useLocation() and return
<Navigate to='/login' replace state={{ from: location.pathname }} /> (mirror the
behavior used by ProtectedRoute) so the login flow can redirect back to the
attempted /admin route.
---
Nitpick comments:
In `@backend/src/admin/admin.module.ts`:
- Line 11: Remove the redundant RolesGuard entry from the AdminModule providers
array: in admin.module.ts, delete RolesGuard from the providers list (leaving
AdminService) since RolesGuard is already provided in UserModule and applied via
`@UseGuards`(AuthGuard, RolesGuard) at controller level; this keeps DI behavior
unchanged while avoiding duplicate registration.
In `@backend/src/auth/auth.controller.ts`:
- Around line 22-32: The inline type import for UserRole in toUserResponseDto
makes the signature harder to read; replace the inline import by adding a
top-level type import (e.g. import type { UserRole, User } from
'@/user/user.entity') and then change the parameter type to either Pick<User,
'id' | 'name' | 'role'> or use UserRole explicitly, keeping the function name
toUserResponseDto and return type UserResponseDto unchanged; update the
file-level imports and the function signature accordingly so types are
consistent and easier to scan.
- Around line 41-81: login and getMe each call
userService.upsertByGoogleWorkspaceToken(req.firebaseUser) and duplicate the
same error mapping and return logic; extract that shared logic into a single
private helper (e.g., resolveUserFromFirebase or handleUpsertAndMapErrors) that
calls userService.upsertByGoogleWorkspaceToken, maps the discriminated errors
('no-google-identity' -> BadRequestException, other -> NotFoundException) and
returns toUserResponseDto(result.user); then have both login and getMe simply
call the new helper with req.firebaseUser to avoid drift and centralize error
handling.
In `@backend/src/auth/auth.guard.ts`:
- Around line 12-16: Hardcoded domain "@profiq.com" in the auth guard causes
FE/BE drift and auth failures; extract this domain into a centralized backend
config (e.g., add a DOMAIN or ALLOWED_EMAIL_DOMAIN constant in
backend/src/config/configuration.ts) and import that constant into auth.guard.ts
and employee.service.ts (referencing the token check in AuthGuard and any domain
logic in EmployeeService) so both use the same source of truth. Also update
AuthService.verifyToken() to throw an error for missing/invalid tokens instead
of returning { email: undefined }, and change the guard (AuthGuard) to catch
that error and rethrow a NestJS UnauthorizedException (401) rather than
returning false, ensuring authentication failures produce 401s not 403s.
In `@backend/src/db/migrations/1776082215808-InitialSchema.ts`:
- Around line 1-16: Rename the migration file to match the class name to avoid
confusion: change the filename from 1776082215808-InitialSchema.ts to
1776082215808-AddRoleToUser.ts and ensure the exported class name
AddRoleToUser1776082215808 remains unchanged so tooling and humans can match
file ↔ class (no code changes needed inside the file).
In `@backend/src/main.ts`:
- Line 23: The global ValidationPipe is instantiated with defaults which allow
unknown properties and do not transform payloads; update the instantiation where
app.useGlobalPipes(new ValidationPipe()) is called to enable security-focused
options: set whitelist: true, forbidNonWhitelisted: true, and transform: true so
DTOs (e.g., UpdateRoleRequest) are transformed and extraneous fields are
stripped/forbidden and validation like `@IsEnum`(UserRole) operates on coerced
values; keep the same ValidationPipe usage but pass the options object to
enforce these rules globally.
In `@backend/src/user/user.service.spec.ts`:
- Around line 29-105: Add comprehensive unit tests for
UserService.upsertByGoogleWorkspaceToken covering the FIRST_ADMIN_EMAIL branch:
write tests for (1) token missing google.com identity returns { error:
'no-google-identity' }, (2) existing user in repository is returned unchanged
(preserves stored role), (3) when employeeService cannot find user in directory
return { error: 'not-in-directory' }, (4) new user whose email matches
process.env.FIRST_ADMIN_EMAIL is saved with role UserRole.Admin, (5) new user
whose email does not match is saved with role UserRole.User, and (6) when
process.env.FIRST_ADMIN_EMAIL is unset no escalation to Admin occurs; use the
existing mocks (mockRepository, mockEmployeeService) and methods
(mockRepository.findOne, mockRepository.save,
mockEmployeeService.getByWorkspaceId) and assert returned values and that save
is called with expected role.
In `@backend/src/user/user.service.ts`:
- Around line 56-97: Create a private helper (e.g., private
getGoogleUidFromToken(token: { uid: string; firebase?: { identities?:
Record<string, unknown> } }): string | undefined) that encapsulates the cast and
first-element lookup currently duplicated in getUserByGoogleWorkspaceUid and
upsertByGoogleWorkspaceToken (i.e., return
(token.firebase?.identities?.['google.com'] as string[] | undefined)?.[0]).
Replace the duplicated logic in getUserByGoogleWorkspaceUid to call this helper
and return null when it yields undefined, and in upsertByGoogleWorkspaceToken
call the helper and return { error: 'no-google-identity' } when it yields
undefined; preserve all other behavior (use existing
employeeService.getEmployee, role assignment, and repository.save).
In `@frontend/src/components/AdminRoute.tsx`:
- Around line 18-27: The Access Denied block in the AdminRoute component (the
conditional that checks role !== 'admin') should provide a recovery path and
semantic landmark: wrap the returned JSX in a <main role="main"> landmark and
add a visible "Go home" link or button that navigates to the safe route (e.g.,
"/") — ensure the control uses a semantic element (anchor or button) with an
accessible label/aria-label and keyboard-focusable styling; update AdminRoute's
return for non-admins to include these changes so screen-reader users and
accidental visitors can recover.
In `@frontend/src/components/navigation-menu-reference.tsx`:
- Line 49: Replace the hard-coded role string check (role === 'admin') with a
comparison against the shared UserRole constant/enum from your auth context;
import UserRole from "@/lib/contexts" and use the appropriate member (e.g.,
UserRole.Admin or UserRole.ADMIN) in the conditional where the variable role is
checked to prevent drift if the role string changes.
In `@frontend/src/components/ProtectedRoute.tsx`:
- Around line 20-24: The current redirect drops query and hash because
ProtectedRoute sets state={{ from: location.pathname }}; update the Navigate
state to include the full location (e.g., state={{ from: location.pathname +
location.search + location.hash }} or simply state={{ from: location }}) so
post-login redirects restore query params and fragment; change the state passed
to Navigate in ProtectedRoute.tsx accordingly.
In `@frontend/src/lib/contexts.tsx`:
- Around line 9-35: The UserRole string union duplicates the backend UserRole
enum which will drift if roles change; replace the local UserRole declaration by
importing the shared/generated role type or constants and update usages (e.g.,
the UserRole type and any references inside AuthContextType) to use that single
source of truth (or wire up a codegen step that emits a frontend type from the
backend enum) so frontend and backend stay in sync.
In `@frontend/src/lib/providers/auth/AuthProvider.tsx`:
- Around line 24-26: toUser currently just returns its FirebaseUser input,
providing no real mapping between FirebaseUser and your app-level User; either
remove toUser and use FirebaseUser directly everywhere, or make User a narrower
type (e.g., only uid, email, displayName, photoURL) and implement toUser to
project fbUser -> { uid: fbUser.uid, email: fbUser.email, displayName:
fbUser.displayName, ... } so call sites receive the concrete User shape; update
any usages that assume structural identity to use the new User properties or the
original FirebaseUser as appropriate and adjust types for toUser, User, and
FirebaseUser accordingly.
In `@frontend/src/lib/providers/auth/domain.ts`:
- Around line 3-8: DOMAIN is hardcoded and parsing uses split('@')[1], causing
FE/BE drift and incorrect parsing for addresses with multiple '@'; change DOMAIN
to read from the environment (e.g., import.meta.env.VITE_ALLOWED_EMAIL_DOMAIN
with a sensible fallback) and update checkDomain to compute the domain using
email.slice(email.lastIndexOf('@') + 1) (handle undefined/empty email) and
compare lowercased/trimmed values against DOMAIN so frontend matches backend
semantics; update references to DOMAIN and the checkDomain(UserInfo) function
accordingly.
In `@frontend/src/routes/Login.tsx`:
- Around line 16-21: getRedirectFrom currently accepts any string for the "from"
field (loginStateSchema) which could allow absolute URLs like "//evil.com" to be
treated as safe redirects; tighten the schema or add a one-line guard in
getRedirectFrom to only accept internal paths (e.g., require strings that start
with a single "/" and do not start with "//" and do not contain "://"); update
loginStateSchema (or validate inside getRedirectFrom) to enforce that constraint
and fall back to "/" when the value fails validation so navigate never receives
an external URL.
In `@frontend/src/services/auth/auth.ts`:
- Around line 16-38: Extract a small helper (e.g., handleApiResponse or
unwrapApiResponse) that accepts an APIResponse<DbUser> returned from
APIClient.fetch and centralizes the success check: if result.success return
result.data, otherwise throw createError(result.status_code,
result.error?.message ?? 'Unknown error'); then update loginToBackend and getMe
to call this helper after their client.fetch calls instead of duplicating the
success/error logic so you remove copy/paste and guard against missing
error.message from APIResponse.
🪄 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
Run ID: db01a925-ff29-4e99-929a-6dfce0f5d800
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
.env.examplebackend/src/admin/admin.controller.tsbackend/src/admin/admin.module.tsbackend/src/auth/auth.controller.spec.tsbackend/src/auth/auth.controller.tsbackend/src/auth/auth.guard.tsbackend/src/auth/auth.module.tsbackend/src/auth/roles.decorator.tsbackend/src/auth/roles.guard.spec.tsbackend/src/auth/roles.guard.tsbackend/src/datasource.tsbackend/src/db/migrations/1776082215808-InitialSchema.tsbackend/src/employee/employee.module.tsbackend/src/loans/loans.controller.spec.tsbackend/src/loans/loans.service.spec.tsbackend/src/main.tsbackend/src/user/dto/update_role.tsbackend/src/user/dto/user_response.dto.tsbackend/src/user/user.controller.tsbackend/src/user/user.entity.tsbackend/src/user/user.module.tsbackend/src/user/user.service.spec.tsbackend/src/user/user.service.tsbackend/test/auth.e2e-spec.tsbackend/test/user.e2e-spec.tsfrontend/src/App.tsxfrontend/src/components/AdminRoute.tsxfrontend/src/components/ProtectedRoute.component-spec.tsxfrontend/src/components/ProtectedRoute.tsxfrontend/src/components/navigation-menu-reference.tsxfrontend/src/lib/contexts.tsxfrontend/src/lib/providers/auth/AuthProvider.tsxfrontend/src/lib/providers/auth/AuthProvider.unit-spec.tsxfrontend/src/lib/providers/auth/domain.tsfrontend/src/routes/Login.component-spec.tsxfrontend/src/routes/Login.tsxfrontend/src/services/auth/auth.ts
| GOOGLE_CLIENT_EMAIL= | ||
| GOOGLE_PRIVATE_KEY= | ||
| VITE_API_URL=http://localhost:3000 | ||
| FIRST_ADMIN_EMAIL= |
There was a problem hiding this comment.
Admin bootstrap compared case-sensitively — risk of silent misconfiguration.
The consumer in backend/src/user/user.service.ts (lines 69‑97) does employee.email === process.env.FIRST_ADMIN_EMAIL with no trim or case-folding. Email local-parts are case-insensitive for all practical providers (and Google normalizes), so a value like Admin@profiq.com in .env will never promote admin@profiq.com to Admin, and the misconfiguration is silent (user is created as a regular user and the env value is ignored on subsequent runs because the user already exists).
Recommend normalizing both sides at the comparison site (toLowerCase().trim()) and/or documenting the expected format here in .env.example. Also consider that once the first admin is created, this env var becomes dead weight — worth a short comment near the variable documenting that it only applies to the first-ever provisioning of that user.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 15-15: [UnorderedKey] The FIRST_ADMIN_EMAIL key should go before the GOOGLE_CLIENT_EMAIL key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example at line 15, The FIRST_ADMIN_EMAIL check currently compares
employee.email === process.env.FIRST_ADMIN_EMAIL case-sensitively which can
silently fail; update the comparison in backend/src/user/user.service.ts (the
block that promotes the first admin around the employee.email check) to
normalize both sides with .trim().toLowerCase() before comparing (e.g.,
normalize employee.email and process.env.FIRST_ADMIN_EMAIL) so emails match
regardless of case/whitespace, and add a short comment in .env.example next to
FIRST_ADMIN_EMAIL noting it must match the first-provisioned admin email
(case/whitespace are ignored) and that it only applies during initial
provisioning.
| synchronize: false, | ||
| migrations: [__dirname + '/db/migrations/**/*{.js,.ts}'], | ||
| migrationsRun: true, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f . backend/src/db/migrations
echo '---'
rg -n "CREATE TABLE|createTable" backend/src/db/migrationsRepository: profiq/items-management-app
Length of output: 131
🏁 Script executed:
cat -n backend/src/db/migrations/1776082215808-InitialSchema.tsRepository: profiq/items-management-app
Length of output: 750
🏁 Script executed:
cat -n backend/src/datasource.tsRepository: profiq/items-management-app
Length of output: 1141
🏁 Script executed:
find backend/src -name "*migration*" -o -name "*schema*" | head -20Repository: profiq/items-management-app
Length of output: 96
🏁 Script executed:
rg -l "CREATE TABLE" backend/src --type tsRepository: profiq/items-management-app
Length of output: 53
Add baseline schema migration that creates the user table before AddRoleToUser runs.
With synchronize: false + migrationsRun: true, the single migration 1776082215808-InitialSchema.ts attempts to alter the user table on fresh databases, but that table is never created. The migration only executes ALTER TABLE "user" ADD COLUMN "role" — it will fail immediately on clean SQLite DBs (dev, CI, e2e) because the user table does not exist.
Create a baseline schema migration (e.g., 1776082215807-InitialSchema.ts) that runs before the current one and creates all required tables (at minimum user and any others referenced by entities).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/datasource.ts` around lines 22 - 24, Add a new baseline migration
file named with an earlier timestamp (e.g., 1776082215807-InitialSchema.ts) that
runs before the existing 1776082215808-InitialSchema.ts and creates the missing
tables (at minimum the "user" table) referenced by your entities; implement
proper up() to CREATE TABLE "user" (and any other entity tables used by later
migrations) and down() to DROP them, so that with the current TypeORM config
(migrationsRun: true and the migrations array in datasource.ts) the ALTER in
1776082215808-InitialSchema.ts will succeed on clean DBs. Ensure the migration
class name and filename are unique and the SQL/schema matches your entity
definitions.
| @Patch(':id/role') | ||
| @UseGuards(RolesGuard) | ||
| @Roles(UserRole.Admin) | ||
| @ApiOkResponse({ type: User }) | ||
| async updateRole( | ||
| @Param('id', ParseIntPipe) id: number, | ||
| @Body() body: UpdateRoleRequest | ||
| ): Promise<User> { | ||
| const user = await this.userService.updateUserRole(id, body.role); | ||
| if (!user) { | ||
| throw new UnknownUserException(); | ||
| } | ||
| return user; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'useGlobalPipes|ValidationPipe' backend/src/main.ts
rg -nP '@IsEnum|UpdateRoleRequest' backend/src/user/dto/update_role.tsRepository: profiq/items-management-app
Length of output: 225
🏁 Script executed:
rg -A 20 'updateUserRole' backend/src/user/user.service.tsRepository: profiq/items-management-app
Length of output: 555
Implement safeguards against admin self-demotion and last-admin lockout.
The updateUserRole method has no checks to prevent an admin from demoting themselves or removing the last admin from the system. Add validation in the service to reject self-demotion and reject any role change that would leave zero admins.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/user/user.controller.ts` around lines 68 - 81, The update allows
an admin to demote themselves or remove the last admin; modify the
updateUserRole flow to prevent that: change the updateUserRole signature (and
update the controller call in updateRole) to accept the acting user id (e.g.,
currentUserId from request) so the service can detect self-demotion, then inside
userService.updateUserRole check (1) if id === currentUserId and the new role is
not UserRole.Admin then reject with a Forbidden/BadRequest, and (2) before
applying a demotion of any admin, query/count current admins (e.g., via
userRepository.count({ role: UserRole.Admin })) and reject any change that would
result in zero admins; keep throwing UnknownUserException when the target user
is missing and use appropriate HTTP error for forbidden actions.
| async upsertByGoogleWorkspaceToken(token: { | ||
| uid: string; | ||
| firebase?: { identities?: Record<string, unknown> }; | ||
| }): Promise<UpsertResult> { | ||
| const googleUid = ( | ||
| token.firebase?.identities?.['google.com'] as string[] | undefined | ||
| )?.[0]; | ||
| if (!googleUid) { | ||
| return { error: 'no-google-identity' }; | ||
| } | ||
| const existing = await this.getUserByEmployeeId(googleUid); | ||
| if (existing) { | ||
| return { user: existing }; | ||
| } | ||
| const employee = await this.employeeService.getEmployee(googleUid); | ||
| if (!employee) { | ||
| return { error: 'not-in-directory' }; | ||
| } | ||
| const user = new User(); | ||
| user.employee_id = googleUid; | ||
| user.name = employee.name; | ||
| user.role = | ||
| process.env.FIRST_ADMIN_EMAIL && | ||
| employee.email === process.env.FIRST_ADMIN_EMAIL | ||
| ? UserRole.Admin | ||
| : UserRole.User; | ||
| await this.userRepository.save(user); | ||
| return { user }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TypeORM 0.3.28 Repository.upsert conflictPaths return type
💡 Result:
In TypeORM 0.3.28, the Repository.upsert method, including when using the conflictPaths option, returns Promise<InsertResult>.
Citations:
- 1: https://cdn.jsdelivr.net/npm/typeorm@0.3.28/repository/UpsertOptions.d.ts
- 2: https://github.com/typeorm/typeorm/blob/master/src/repository/Repository.ts
- 3: https://typeorm.io/docs/working-with-entity-manager/repository-api/
- 4: https://typeorm.io/docs/working-with-entity-manager/repository-api
- 5: https://orkhan.gitbook.io/typeorm/docs/docs/working-with-entity-manager/6-repository-api
🏁 Script executed:
# Find the user entity file
fd -e ts "user.entity" backend/Repository: profiq/items-management-app
Length of output: 102
🏁 Script executed:
# Check the package.json for TypeORM version
cat backend/package.json | grep -i typeorm -A 2 -B 2Repository: profiq/items-management-app
Length of output: 1046
🏁 Script executed:
# Look at the actual user.service.ts code around the target lines
cat -n backend/src/user/user.service.ts | sed -n '60,110p'Repository: profiq/items-management-app
Length of output: 1878
🏁 Script executed:
# Search for unique constraints in user entity
rg "unique|constraint" backend/src/user/ -t ts -A 2 -B 2Repository: profiq/items-management-app
Length of output: 310
🏁 Script executed:
# Check TypeORM 0.3.28 Repository for upsert method signature and conflictPaths support
cat backend/node_modules/typeorm/repository/Repository.d.ts 2>/dev/null | grep -A 20 "upsert(" || echo "Node modules not available, will use web search"Repository: profiq/items-management-app
Length of output: 118
🌐 Web query:
TypeORM 0.3.28 Repository upsert conflictPaths method signature documentation
💡 Result:
In TypeORM 0.3.28, the conflictPaths property is part of the UpsertOptions interface used in the Repository.upsert method. Method usage example from official docs: await repository.upsert( [{ externalId: "abc123", firstName: "Rizzrak" }], { conflictPaths: ["externalId"] } ); Type definition from typeorm@0.3.28 UpsertOptions.d.ts: export interface UpsertOptions extends InsertOrUpdateOptions { conflictPaths: string[] | { [P in keyof Entity]?: true }; // ... other properties } conflictPaths specifies the columns/fields for conflict detection in the upsert operation (e.g., ON CONFLICT clause in PostgreSQL). It supports either an array of property path strings (e.g., ["externalId"] or ["user.id"] for relations) or a typed object selecting entity keys (e.g., { externalId: true }), added in earlier versions via PR #9365 and included in 0.3.28 (released Dec 2025). This signature has remained stable since its introduction, with no breaking changes noted in the 0.3.28 changelog.
Citations:
- 1: https://cdn.jsdelivr.net/npm/typeorm@0.3.28/repository/UpsertOptions.d.ts
- 2: typeorm/typeorm@b282428
- 3: feature: upsert options conflictPaths as Entity keys typeorm/typeorm#9365
- 4: https://typeorm.io/docs/working-with-entity-manager/repository-api
- 5: https://orkhan.gitbook.io/typeorm/docs/docs/working-with-entity-manager/6-repository-api
🏁 Script executed:
# Verify QueryFailedError or similar is what would be thrown on unique violation
rg "QueryFailedError|UniqueConstraintFailed" backend/ -t ts -A 2 -B 2 || echo "No existing error handling found"Repository: profiq/items-management-app
Length of output: 103
🏁 Script executed:
# Check for database migrations that might define the unique constraint
find backend/src/db/migrations -name "*.ts" -type f | head -5 && \
cat backend/src/db/migrations/*.ts 2>/dev/null | grep -i "unique\|employee_id" -B 3 -A 3 | head -40Repository: profiq/items-management-app
Length of output: 127
🏁 Script executed:
# Verify the actual entity decorator usage
cat -n backend/src/user/user.entity.ts | head -50Repository: profiq/items-management-app
Length of output: 810
🌐 Web query:
TypeORM sqlite3 unique constraint violation error handling save() method
💡 Result:
To handle unique constraint violation errors from TypeORM's save method with SQLite3, catch the QueryFailedError and check the error code. The save method throws QueryFailedError when a database constraint is violated during insert or update operations. For SQLite unique constraint violations, the error has code: 'SQLITE_CONSTRAINT' and errno: 19. Example handling code: typescript import { QueryFailedError } from 'typeorm'; try { await repository.save(entity); } catch (error) { if (error instanceof QueryFailedError) { if (error.code === 'SQLITE_CONSTRAINT') { // Handle unique constraint violation console.log('Unique constraint violation:', error.message); // Optionally check driverError for more details throw new Error('Duplicate entry - unique constraint violated'); } } throw error; // Re-throw if not the expected error } Key points: - QueryFailedError wraps the driver-specific error (driverError property contains SQLite error details). - SQLite returns 'SQLITE_CONSTRAINT' for UNIQUE, PRIMARY KEY, CHECK, and FOREIGN KEY violations. - Distinguish unique violations by inspecting error.message (contains "UNIQUE constraint failed") or driverError details. - Common causes: Attempting to insert duplicate unique/primary key values, or save bugs with composite uniques/relations where it tries INSERT instead of UPDATE. For prevention: - Use findOne before save to check existence manually if save behavior is unreliable with composites. - Define @Unique decorators properly on entities. - Consider QueryBuilder with onConflict for upsert (PostgreSQL-specific; SQLite limited support). This approach works across TypeORM versions as shown in multiple GitHub issues and Stack Overflow examples.
Citations:
- 1: .save() is throwing duplicate key value violates unique constraint typeorm/typeorm#4122
- 2: What is the right/recommended way for error handling? typeorm/typeorm#5057
- 3: .save not working with two unique constraints typeorm/typeorm#7598
- 4: https://stackoverflow.com/questions/60591696/with-typeorm-sqlite-constraint-foreign-key-constraint-failed-when-adding-a-c
- 5: QueryFailedError: SQLITE_CONSTRAINT: FOREIGN KEY constraint failed typeorm/typeorm#1803
- 6:
SQLITE_CONSTRAINT: FOREIGN KEY constraint failedwhen adding a column to an entity typeorm/typeorm#5641 - 7: https://github.com/typeorm/typeorm/blob/master/src/error/QueryFailedError.ts
- 8: https://orkhan.gitbook.io/typeorm/docs/docs/working-with-entity-manager/6-repository-api
- 9: https://typeorm.io/docs/getting-started/
- 10: https://typeorm.io/docs/drivers/sqlite
Race on concurrent first logins can 500 on a unique-constraint violation.
getUserByEmployeeId → userRepository.save(new User) is a check-then-act without a transaction or unique-violation handler. Two near-simultaneous first logins for the same employee (e.g. multiple tabs opening during onboarding, or a retry after a slow popup) can both observe existing === undefined and both call save; the second hits the unique index on employee_id and surfaces as a QueryFailedError (500) to the client instead of the intended { user } response.
Two reasonable fixes:
- Catch the unique-violation error after
saveand re-read the existing row:
🛡️ Proposed fix
const employee = await this.employeeService.getEmployee(googleUid);
if (!employee) {
return { error: 'not-in-directory' };
}
const user = new User();
user.employee_id = googleUid;
user.name = employee.name;
user.role =
process.env.FIRST_ADMIN_EMAIL &&
employee.email === process.env.FIRST_ADMIN_EMAIL
? UserRole.Admin
: UserRole.User;
- await this.userRepository.save(user);
- return { user };
+ try {
+ await this.userRepository.save(user);
+ return { user };
+ } catch (err) {
+ // Concurrent first login may have inserted the row first.
+ const raced = await this.getUserByEmployeeId(googleUid);
+ if (raced) {
+ return { user: raced };
+ }
+ throw err;
+ }- Or use TypeORM 0.3.28's
upsert([], { conflictPaths: ['employee_id'] })for atomic conflict resolution.
Minor: googleUid extraction (lines 73–75) duplicates the logic from getUserByGoogleWorkspaceUid (lines 60–66). Consider a shared helper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/user/user.service.ts` around lines 69 - 97,
upsertByGoogleWorkspaceToken currently does a check-then-act (calls
getUserByEmployeeId then userRepository.save) which races and can throw a
unique-constraint QueryFailedError; change it to either (A) perform an atomic
upsert via TypeORM's upsert([], { conflictPaths: ['employee_id'] }) on the User
entity instead of save, or (B) wrap the save in a try/catch that detects the DB
unique-violation (e.g., Postgres 23505 / QueryFailedError) and on conflict
re-read the existing row with getUserByEmployeeId and return that user; also
extract the googleUid extraction logic used in upsertByGoogleWorkspaceToken into
a small shared helper and reuse it from getUserByGoogleWorkspaceUid to avoid
duplication.
| afterEach(async () => { | ||
| await app.close(); | ||
| }); |
There was a problem hiding this comment.
Minor: guard afterEach against an un-initialised app.
If a beforeAll/buildApp call ever fails, app will be undefined and afterEach throws Cannot read properties of undefined (reading 'close'), masking the real failure.
🛡️ Suggested guard
afterEach(async () => {
- await app.close();
+ if (app) {
+ await app.close();
+ }
});📝 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.
| afterEach(async () => { | |
| await app.close(); | |
| }); | |
| afterEach(async () => { | |
| if (app) { | |
| await app.close(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/test/auth.e2e-spec.ts` around lines 62 - 64, The afterEach hook
currently calls await app.close() unguarded; if buildApp()/beforeAll failed and
app is undefined this masks the original error. Modify the afterEach to first
check that the test app is initialized and has a close method (e.g. if (app &&
typeof app.close === 'function') await app.close()) so it only calls close when
safe; reference the afterEach hook and the app variable created by
buildApp()/beforeAll to locate where to apply this guard.
| if (!user) { | ||
| return <Navigate to='/login' />; | ||
| } |
There was a problem hiding this comment.
Inconsistent redirect — missing replace and from state.
Unlike ProtectedRoute, this <Navigate> doesn't pass replace nor state={{ from: location.pathname }}. As a result, an unauthenticated user hitting /admin gets an extra history entry and — per Login.tsx which reads location.state.from — won't be routed back to /admin after signing in.
🛠️ Suggested fix
-import { Navigate, Outlet } from 'react-router';
+import { Navigate, Outlet, useLocation } from 'react-router';
@@
- const { user, role, loading } = useAuth();
+ const { user, role, loading } = useAuth();
+ const location = useLocation();
@@
- if (!user) {
- return <Navigate to='/login' />;
- }
+ if (!user) {
+ return (
+ <Navigate to='/login' state={{ from: location.pathname }} replace />
+ );
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/AdminRoute.tsx` around lines 15 - 17, AdminRoute
currently returns <Navigate to='/login' /> without using replace or passing the
original location, so unauthenticated users create an extra history entry and
Login.tsx cannot redirect back; update the AdminRoute component to read the
current location via useLocation() and return <Navigate to='/login' replace
state={{ from: location.pathname }} /> (mirror the behavior used by
ProtectedRoute) so the login flow can redirect back to the attempted /admin
route.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores