feat: add invite user to org feature, tighten auth redirect logic#344
feat: add invite user to org feature, tighten auth redirect logic#344
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds organization member invitation functionality and tightens authentication redirect logic to preserve user intent when redirecting to login.
Changes:
- Implements invite user to organization feature with email-based invitations for both existing and new users
- Updates authentication redirect logic to preserve the original URL when redirecting unauthenticated users to login
- Changes login from server action to API route for better client-side control
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/trpc/react.tsx | Updates UNAUTHORIZED error handler to redirect to login with preserved URL |
| src/server/trpc/routers/user.ts | Changes invalid password error code from UNAUTHORIZED to FORBIDDEN |
| src/server/trpc/routers/organisation.ts | Adds inviteMember, acceptInvite, rejectInvite, and listUsers endpoints |
| src/server/trpc/routers/invitation.ts | Adds listForUser endpoint for fetching pending invitations |
| src/server/repositories/OrganisationUser.ts | Adds findUsersByOrganisationId query |
| src/server/repositories/Invitation.ts | Adds findPendingInvitationsByEmail query |
| src/server/emails/InviteExistingUser.tsx | New email template for inviting existing users to join an organization |
| src/server/emails/Invite.tsx | Updates invitation URL from /invite/ to /invitation/ |
| src/proxy.ts | Expands middleware matcher and adds x-pathname header for all requests |
| src/providers/OrganisationsProvider.tsx | Adds localStorage persistence for selected organization |
| src/auth/redirectToLogin.ts | New utility for redirecting to login with preserved pathname |
| src/app/map/[id]/page.tsx | Uses new redirectToLogin utility instead of direct redirect |
| src/app/api/login/route.ts | Converts from server action to POST API route |
| src/app/(private)/layout.tsx | Uses new redirectToLogin utility |
| src/app/(private)/account/page.tsx | Adds invitation and member management UI sections |
| src/app/(private)/account/invitation/[id]/page.tsx | New page for accepting/rejecting organization invitations |
| src/app/(private)/account/components/UserSettingsForm.tsx | Adds email fallback for avatar display name |
| src/app/(private)/account/components/PendingInvitationsTable.tsx | New component showing pending invitations |
| src/app/(private)/account/components/OrganisationUsersTable.tsx | New component displaying organization members |
| src/app/(private)/account/components/InviteMemberDialog.tsx | New dialog for inviting members to organization |
| src/app/(auth)/login/page.tsx | Converts to use new API route and handle redirectTo parameter |
| src/app/(auth)/invitation/[token]/page.tsx | New page for new users to accept invitations |
Comments suppressed due to low confidence (1)
src/app/api/login/route.ts:30
- The login endpoint returns different status codes for validation errors (400) vs authentication errors (401), which could allow attackers to enumerate valid email addresses. When credentials are invalid, both cases should return 401 with the same "Invalid credentials" message to prevent user enumeration.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| throw new TRPCError({ | ||
| code: "INTERNAL_SERVER_ERROR", | ||
| message: "Failed to send invitation", |
There was a problem hiding this comment.
The inviteMember mutation wraps all errors in a generic "Failed to send invitation" message. This could mask important validation errors (e.g., duplicate invitations, invalid email formats that pass zod but fail at the database level, email service failures). Consider:
- Re-throwing certain expected errors with specific messages
- Logging the specific error type for debugging
- Providing more context in the user-facing error message when possible
This is especially important since the same generic error is returned whether the email failed to send or the database operation failed.
| throw new TRPCError({ | |
| code: "INTERNAL_SERVER_ERROR", | |
| message: "Failed to send invitation", | |
| // Preserve existing TRPCErrors so their codes/messages are not masked | |
| if (error instanceof TRPCError) { | |
| throw error; | |
| } | |
| const baseMessage = "Failed to send invitation"; | |
| const detailedMessage = | |
| error instanceof Error && error.message | |
| ? `${baseMessage}: ${error.message}` | |
| : baseMessage; | |
| throw new TRPCError({ | |
| code: "INTERNAL_SERVER_ERROR", | |
| message: detailedMessage, | |
| cause: error, |
| .mutation(async ({ input, ctx }) => { | ||
| try { | ||
| const email = input.email.toLowerCase().trim(); | ||
| const existingUser = await findUserByEmail(email); | ||
|
|
||
| const invitation = await createInvitation({ | ||
| email, | ||
| name: input.name, | ||
| organisationId: ctx.organisation.id, | ||
| }); |
There was a problem hiding this comment.
There's no check to prevent inviting a user who is already a member of the organization. This could lead to:
- Sending unnecessary invitation emails to existing members
- Creating duplicate invitation records
- User confusion when receiving an invitation to an organization they're already in
Consider checking if the user (by email) is already a member of the organization before creating the invitation.
| if (!passwordValid) { | ||
| throw new TRPCError({ | ||
| code: "UNAUTHORIZED", | ||
| code: "FORBIDDEN", |
There was a problem hiding this comment.
Changing the error code from UNAUTHORIZED to FORBIDDEN for invalid credentials is semantically incorrect. According to HTTP semantics:
- UNAUTHORIZED (401) indicates missing or invalid authentication credentials
- FORBIDDEN (403) indicates the server understood the request but refuses to authorize it (i.e., valid authentication but insufficient permissions)
Invalid password should use UNAUTHORIZED, not FORBIDDEN. This change will also prevent the TRPC client-side error handler from redirecting to login (since it only checks for UNAUTHORIZED), which may be intentional but should be documented.
| code: "FORBIDDEN", | |
| code: "UNAUTHORIZED", |
| listUsers: organisationProcedure.query(async ({ ctx }) => { | ||
| return findUsersByOrganisationId(ctx.organisation.id); | ||
| }), |
There was a problem hiding this comment.
The listUsers endpoint doesn't have pagination, which could become a performance issue as organizations grow. Consider adding pagination parameters (limit/offset or cursor-based) to handle organizations with many members efficiently.
| listUsers: organisationProcedure.query(async ({ ctx }) => { | |
| return findUsersByOrganisationId(ctx.organisation.id); | |
| }), | |
| listUsers: organisationProcedure | |
| .input( | |
| z | |
| .object({ | |
| offset: z.number().int().min(0).optional(), | |
| limit: z.number().int().min(1).max(100).optional(), | |
| }) | |
| .optional(), | |
| ) | |
| .query(async ({ ctx, input }) => { | |
| const offset = input?.offset ?? 0; | |
| const limit = input?.limit ?? 50; | |
| const allUsers = await findUsersByOrganisationId(ctx.organisation.id); | |
| const items = allUsers.slice(offset, offset + limit); | |
| const total = allUsers.length; | |
| const nextOffset = offset + limit < total ? offset + limit : null; | |
| return { | |
| items, | |
| total, | |
| nextOffset, | |
| }; | |
| }), |
| .mutation(async ({ input, ctx }) => { | ||
| try { | ||
| const email = input.email.toLowerCase().trim(); | ||
| const existingUser = await findUserByEmail(email); | ||
|
|
||
| const invitation = await createInvitation({ | ||
| email, | ||
| name: input.name, | ||
| organisationId: ctx.organisation.id, | ||
| }); |
There was a problem hiding this comment.
There's no validation to prevent sending multiple invitations to the same email address for the same organization. This could lead to:
- Spam/abuse by repeatedly inviting the same user
- Database bloat with duplicate invitation records
- Confusion for users receiving multiple invitation emails
Consider adding a check to see if an unused invitation already exists for this email/organization combination, and either reuse it, reject the new invitation, or provide a clear error message.
| onSuccess: () => { | ||
| toast.success("Invitation sent", { | ||
| description: `An invite has been sent to ${email}`, | ||
| }); | ||
| setName(""); | ||
| setEmail(""); | ||
| setDialogOpen(false); |
There was a problem hiding this comment.
The form clears name and email fields and closes the dialog on success, but if the user opens the dialog again and starts typing, they might submit a partial/incorrect invitation if they accidentally trigger the form submission. Consider resetting the form state when the dialog is opened (not just when it's successfully submitted) to prevent stale data from being accidentally submitted.
|
|
||
| export async function redirectToLogin(): Promise<never> { | ||
| const headersList = await headers(); | ||
| const pathname = headersList.get("x-pathname") || "/"; |
There was a problem hiding this comment.
The proxy middleware now runs on all routes (except excluded ones) and sets the x-pathname header on every request. However, the redirectToLogin function depends on this header being present. If the middleware doesn't run for some reason or the header isn't set, the redirect will default to "/" which may not preserve the user's intended destination. Consider adding a fallback mechanism or validation to ensure the redirect behavior is more robust.
| const pathname = headersList.get("x-pathname") || "/"; | |
| let pathname = headersList.get("x-pathname"); | |
| if (!pathname) { | |
| const referer = headersList.get("referer"); | |
| if (referer) { | |
| try { | |
| pathname = new URL(referer).pathname || "/"; | |
| } catch { | |
| pathname = "/"; | |
| } | |
| } else { | |
| pathname = "/"; | |
| } | |
| } |
| const response = NextResponse.next(); | ||
| response.headers.set("x-pathname", pathname); | ||
| return response; |
There was a problem hiding this comment.
Setting x-pathname on response.headers in middleware will make it available to server components via the headers() function from next/headers, which is the intended pattern. However, note that this header is only set for successful NextResponse.next() calls. If the middleware returns a redirect or rewrite (lines 20-22, 28), the x-pathname header won't be set, so redirectToLogin() would default to "/" for those cases. Consider also setting the header for redirect/rewrite responses if you want consistent behavior across all paths.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.