Email notifications, password reset, role upgrade requests, and security hardening#6
Conversation
There was a problem hiding this comment.
Pull request overview
Adds email-backed user notifications and account/role workflows across backend + frontend, integrating Resend for transactional email delivery and introducing password reset + role upgrade request flows.
Changes:
- Add Resend-based email service and send notifications for welcome emails, password resets, shares, and role request lifecycle events.
- Implement password reset (token model + auth endpoints + frontend pages) and role upgrade requests (DB model + API + admin/user UI).
- Change default user role to
VIEWER(Prisma default + registration behavior + test update).
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/services/core/roleRequest.service.ts | New frontend client wrapper for role request CRUD/review endpoints. |
| frontend/src/services/core/index.ts | Exposes roleRequestService and RoleRequest type via core services barrel. |
| frontend/src/services/core/api.client.ts | Adds auth endpoints for forgot/reset password and role-request endpoint constants; extends authApi. |
| frontend/src/components/auth/RoleRequestDialog.tsx | Adds user-facing dialog to submit role upgrade requests and view request history. |
| frontend/src/components/auth/ResetPasswordPage.tsx | Adds reset-password UI consuming token from query string and calling authApi.resetPassword. |
| frontend/src/components/auth/LoginPage.tsx | Adds “Forgot password?” link entry point to the new reset flow. |
| frontend/src/components/auth/ForgotPasswordPage.tsx | Adds forgot-password UI to request reset email via authApi.forgotPassword. |
| frontend/src/components/admin/RoleRequestManagement.tsx | Adds admin UI for filtering, approving, rejecting, and adding review notes to role requests. |
| frontend/src/components/admin/index.ts | Exports RoleRequestManagement. |
| frontend/src/components/admin/AdminPanel.tsx | Adds “Role Requests” tab to the admin panel. |
| frontend/src/App.tsx | Adds routes for forgot/reset password (unauthenticated) and a drawer action to open the role request dialog. |
| docker-compose.prod.yml | Adds Resend + APP_URL environment variables for backend email functionality. |
| backend/src/services/sharing.service.ts | Triggers share notification email on share creation. |
| backend/src/services/roleRequest.service.ts | Implements role request creation/listing/review logic and triggers notification emails. |
| backend/src/services/email.service.ts | New Resend-backed email sending and HTML templates for all notification types. |
| backend/src/services/auth.service.ts | Sends welcome email on registration; implements password reset token issuance + reset logic. |
| backend/src/routes/roleRequest.routes.ts | Adds authenticated role request endpoints (user submit/history; admin list/review). |
| backend/src/routes/index.ts | Registers /role-requests routes behind authenticate. |
| backend/src/routes/auth.routes.ts | Adds public, rate-limited /auth/forgot-password and /auth/reset-password endpoints. |
| backend/src/config/index.ts | Adds Resend/APP_URL configuration fields sourced from env. |
| backend/src/tests/services/auth.service.test.ts | Updates registration expectation to default VIEWER role. |
| backend/prisma/schema.prisma | Adds PasswordResetToken + RoleRequest models/enums; changes user default role to VIEWER. |
| backend/prisma/migrations/20260401120000_add_password_reset_and_role_requests/migration.sql | Creates new tables/enums and updates default role at the DB level. |
| backend/package.json | Adds resend dependency. |
| backend/package-lock.json | Locks resend and transitive dependencies. |
| backend/.env.example | Documents new Resend + APP_URL environment variables. |
Files not reviewed (1)
- backend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sendShareNotificationEmail( | ||
| targetUser.email, | ||
| owner.email, | ||
| resourceType, | ||
| resourceId | ||
| ); | ||
|
|
||
| return this.mapToSharedResource(share); | ||
| } | ||
|
|
There was a problem hiding this comment.
sendShareNotificationEmail expects a human-readable resourceName, but shareResource is passing resourceId. This makes the email content misleading (it will show an opaque UUID instead of a name/title). Consider either (a) fetching the resource’s display name based on resourceType and passing it, or (b) renaming the email parameter/template to explicitly use resourceId (and adjust wording accordingly).
| sendShareNotificationEmail( | |
| targetUser.email, | |
| owner.email, | |
| resourceType, | |
| resourceId | |
| ); | |
| return this.mapToSharedResource(share); | |
| } | |
| const resourceName = await this.getResourceDisplayName(resourceType, resourceId); | |
| sendShareNotificationEmail( | |
| targetUser.email, | |
| owner.email, | |
| resourceType, | |
| resourceName | |
| ); | |
| return this.mapToSharedResource(share); | |
| } | |
| private async getResourceDisplayName(resourceType: ResourceType, resourceId: string): Promise<string> { | |
| switch (resourceType) { | |
| case 'MODEL': { | |
| const model = await prisma.model.findUnique({ | |
| where: { id: resourceId }, | |
| select: { name: true }, | |
| }); | |
| return model?.name ?? resourceId; | |
| } | |
| case 'METAMODEL': { | |
| const metamodel = await prisma.metamodel.findUnique({ | |
| where: { id: resourceId }, | |
| select: { name: true }, | |
| }); | |
| return metamodel?.name ?? resourceId; | |
| } | |
| case 'DIAGRAM': { | |
| const diagram = await prisma.diagram.findUnique({ | |
| where: { id: resourceId }, | |
| select: { name: true }, | |
| }); | |
| return diagram?.name ?? resourceId; | |
| } | |
| case 'CODEGEN_PROJECT': { | |
| const project = await prisma.codegenProject.findUnique({ | |
| where: { id: resourceId }, | |
| select: { name: true }, | |
| }); | |
| return project?.name ?? resourceId; | |
| } | |
| default: | |
| // Fallback: if we don't have a specific lookup, use the ID | |
| return resourceId; | |
| } | |
| } |
| const status = req.query.status as string | undefined; | ||
| const filters = status ? { status: status as any } : undefined; | ||
|
|
||
| const requests = await roleRequestService.getAllRequests(filters); |
There was a problem hiding this comment.
status is taken directly from the query string and cast to any for Prisma filtering. If a client sends an invalid value, Prisma will throw and this endpoint will 500. Validate status against the allowed RoleRequestStatus values and return 400 for invalid inputs (or ignore the filter).
| const token = crypto.randomBytes(32).toString('hex'); | ||
| const expiresAt = new Date(Date.now() + 60 * 60 * 1000); // 1 hour | ||
|
|
||
| await prisma.passwordResetToken.create({ | ||
| data: { | ||
| token, | ||
| userId: user.id, | ||
| expiresAt, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Password reset tokens are being stored in plaintext in the DB. If the DB is leaked, active tokens can be used to take over accounts. Prefer storing a hash of the token (e.g., SHA-256) and emailing the raw token; on reset, hash the provided token and compare. Also consider deleting/invalidating prior unused tokens for the user when issuing a new one to limit token sprawl.
| /** | ||
| * Request password reset - generates token and sends email | ||
| * Always succeeds (no information leakage about whether email exists) | ||
| */ | ||
| async requestPasswordReset(email: string): Promise<void> { | ||
| const user = await prisma.user.findUnique({ | ||
| where: { email: email.toLowerCase() }, | ||
| }); | ||
|
|
||
| if (!user) { | ||
| return; // Silent - don't reveal if email exists | ||
| } | ||
|
|
||
| const token = crypto.randomBytes(32).toString('hex'); | ||
| const expiresAt = new Date(Date.now() + 60 * 60 * 1000); // 1 hour | ||
|
|
||
| await prisma.passwordResetToken.create({ | ||
| data: { | ||
| token, | ||
| userId: user.id, | ||
| expiresAt, | ||
| }, | ||
| }); | ||
|
|
||
| sendPasswordResetEmail(user.email, token); | ||
| } | ||
|
|
||
| /** | ||
| * Reset password using token | ||
| */ | ||
| async resetPassword(token: string, newPassword: string): Promise<void> { | ||
| const passwordValidation = this.validatePassword(newPassword); | ||
| if (!passwordValidation.valid) { | ||
| throw new Error(passwordValidation.message); | ||
| } | ||
|
|
||
| const resetToken = await prisma.passwordResetToken.findUnique({ | ||
| where: { token }, | ||
| include: { user: true }, | ||
| }); | ||
|
|
||
| if (!resetToken) { | ||
| throw new Error('Invalid or expired reset token'); | ||
| } | ||
|
|
||
| if (resetToken.usedAt) { | ||
| throw new Error('This reset token has already been used'); | ||
| } | ||
|
|
||
| if (resetToken.expiresAt < new Date()) { | ||
| throw new Error('Invalid or expired reset token'); | ||
| } | ||
|
|
||
| const hashedPassword = await bcrypt.hash(newPassword, SALT_ROUNDS); | ||
|
|
||
| await prisma.$transaction([ | ||
| prisma.user.update({ | ||
| where: { id: resetToken.userId }, | ||
| data: { password: hashedPassword }, | ||
| }), | ||
| prisma.passwordResetToken.update({ | ||
| where: { id: resetToken.id }, | ||
| data: { usedAt: new Date() }, | ||
| }), | ||
| ]); | ||
| } |
There was a problem hiding this comment.
New password reset behavior was added in AuthService (requestPasswordReset / resetPassword) but the existing auth service test suite wasn’t extended to cover it. Add unit tests for: unknown email (no token created), token creation with expiry, reset success path (password updated + token marked used), already-used token, and expired/invalid token.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| export async function sendRoleRequestSubmittedEmail( | ||
| adminEmails: string[], | ||
| requesterEmail: string, | ||
| requestedRole: string, | ||
| reason: string | ||
| ): Promise<void> { | ||
| for (const adminEmail of adminEmails) { | ||
| await sendEmail( | ||
| adminEmail, | ||
| 'New Role Request - SpatialDSL Studio', | ||
| `<h2>New Role Upgrade Request</h2> | ||
| <p><strong>${requesterEmail}</strong> has requested a role upgrade to <strong>${requestedRole}</strong>.</p> | ||
| <p><strong>Reason:</strong> ${reason}</p> | ||
| <p>Review this request in the <a href="${appUrl}/admin">Admin Panel</a>.</p>` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| export async function sendRoleRequestReviewedEmail( | ||
| userEmail: string, | ||
| requestedRole: string, | ||
| approved: boolean, | ||
| reviewNote?: string | ||
| ): Promise<void> { | ||
| const status = approved ? 'approved' : 'rejected'; | ||
| const noteHtml = reviewNote ? `<p><strong>Note from admin:</strong> ${reviewNote}</p>` : ''; | ||
| await sendEmail( | ||
| userEmail, | ||
| `Role Request ${approved ? 'Approved' : 'Rejected'} - SpatialDSL Studio`, | ||
| `<h2>Role Request ${approved ? 'Approved' : 'Rejected'}</h2> | ||
| <p>Your request for the <strong>${requestedRole}</strong> role has been <strong>${status}</strong>.</p> | ||
| ${noteHtml} | ||
| <p>${approved ? 'Your role has been updated. Log in to access your new permissions.' : 'You can submit a new request with additional information if needed.'}</p> |
There was a problem hiding this comment.
Several email templates interpolate user-controlled strings (e.g., reason, reviewNote, and potentially resource names) directly into HTML. This allows HTML injection in email clients. Escape/encode interpolated values (or generate text-only emails) before including them in the HTML body.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| const currentRoleIndex = user ? ROLE_HIERARCHY.indexOf(user.role) : 0; | ||
| const availableRoles = ROLE_HIERARCHY.filter((_, i) => i > currentRoleIndex && i < ROLE_HIERARCHY.length - 1); // Exclude ADMIN | ||
|
|
||
| useEffect(() => { | ||
| if (open) { | ||
| setError(null); | ||
| setSuccess(false); | ||
| setReason(''); | ||
| loadMyRequests(); | ||
| if (availableRoles.length > 0) { | ||
| setRequestedRole(availableRoles[0]); | ||
| } | ||
| } | ||
| }, [open]); |
There was a problem hiding this comment.
useEffect depends on availableRoles and loadMyRequests, but the dependency array only includes open. With CRA’s hooks linting this will raise react-hooks/exhaustive-deps warnings (and can fail react-scripts build in CI). Consider restructuring so the effect includes the needed dependencies (e.g., memoize loadMyRequests and compute availableRoles inside the effect) or explicitly justify/disable the lint rule for this effect.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| useEffect(() => { | ||
| loadRequests(); | ||
| }, [statusFilter]); |
There was a problem hiding this comment.
This effect calls loadRequests but doesn’t include it in the dependency array. That can trigger react-hooks/exhaustive-deps warnings (and potentially fail builds in CI with CRA). Consider memoizing loadRequests with useCallback and adding it to the dependency list, or move the async logic directly into the effect.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| {mode === 'login' && ( | ||
| <Box sx={{ textAlign: 'right', mt: 1 }}> | ||
| <Link | ||
| href="/forgot-password" | ||
| variant="body2" | ||
| sx={{ fontWeight: 500, textDecoration: 'none', '&:hover': { textDecoration: 'underline' } }} | ||
| > | ||
| Forgot password? | ||
| </Link> | ||
| </Box> |
There was a problem hiding this comment.
The "Forgot password?" link uses an href, which triggers a full page navigation. In deployments without server-side SPA route fallback this can 404, and even when it works it reloads the app unnecessarily. Prefer client-side navigation via react-router (e.g., MUI Link with component={RouterLink} or onClick={() => navigate('/forgot-password')}).
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| model User { | ||
| id String @id @default(uuid()) | ||
| email String @unique | ||
| password String // Hashed with bcrypt | ||
| role UserRole @default(DSL_DESIGNER) | ||
| role UserRole @default(VIEWER) | ||
| isSuspended Boolean @default(false) | ||
| lastLogin DateTime? | ||
|
|
||
| createdAt DateTime @default(now()) | ||
| updatedAt DateTime @updatedAt | ||
|
|
||
| // Relations - user owns all their data | ||
| ePackages EPackage[] | ||
| metamodels Metamodel[] | ||
| models Model[] | ||
| diagrams Diagram[] | ||
| transformationRules TransformationRule[] | ||
| codegenProjects CodeGenerationProject[] | ||
| testCases TestCase[] | ||
| storedFiles StoredFile[] | ||
|
|
||
| // Sharing relations | ||
| ownedShares SharedResource[] @relation("ShareOwner") | ||
| receivedShares SharedResource[] @relation("ShareRecipient") | ||
|
|
||
|
|
||
| // Password reset & role request relations | ||
| passwordResetTokens PasswordResetToken[] | ||
| roleRequests RoleRequest[] | ||
| reviewedRoleRequests RoleRequest[] @relation("RoleRequestReviewer") | ||
|
|
There was a problem hiding this comment.
This PR is titled/described as "email notification", but it also introduces a password reset flow, role request workflow (DB models, routes, UI), and changes the default user role to VIEWER. If that scope expansion is intentional, please update the PR title/description to reflect these significant changes so reviewers know what to focus on.
| model PasswordResetToken { | ||
| id String @id @default(uuid()) | ||
| token String @unique | ||
| userId String | ||
| user User @relation(fields: [userId], references: [id], onDelete: Cascade) | ||
| expiresAt DateTime | ||
| usedAt DateTime? | ||
| createdAt DateTime @default(now()) | ||
| @@map("password_reset_tokens") |
There was a problem hiding this comment.
If you move to hashed password reset tokens (recommended), the schema will need to store the hash rather than the raw token. As written, token is a unique plaintext value; that couples the DB to the insecure approach and makes it hard to rotate to hashing later without migration.
|
@copilot apply changes based on the comments in this thread |
Applied all changes from the review thread in commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Adds email-backed user notifications and account/role workflows across backend and frontend, integrating Resend for transactional email delivery and introducing password reset and role upgrade request flows.
Changes Made
Email Service
te***@example.com) to prevent PII leakagereason,reviewNote, resource names) are HTML-escaped before interpolation in email templates to prevent HTML injectionAuthentication
/forgot-password,/reset-password)Role Upgrade Requests
statusquery parameter on the admin list endpoint is validated against allowedRoleRequestStatusvalues (returns 400 for invalid inputs)Sharing
Schema
PasswordResetToken.tokenrenamed totokenHashto make clear that a hash is storedVIEWERFrontend
useNavigatefor client-side SPA navigation instead of a full-pagehrefuseCallbackanduseMemoused inRoleRequestDialogandRoleRequestManagementto satisfyreact-hooks/exhaustive-depsTesting
requestPasswordResetandresetPasswordcovering: unknown email, token creation and invalidation, SHA-256 hashing, reset success, already-used token, expired token, and invalid password