Repository navigation
feat(identity): implement secure user management with self-deletion p… - #44
Merged
Merged
Conversation
…rotection - Add View/Edit functionality for tenant users - Implement enterprise-grade self-deletion prevention (UI + API) - Fix user role display in tenant dashboard - Add comprehensive test coverage for user deletion endpoint - Enhance cleanup script with auto-env loading - Update test to match new fancy name generator Coverage: 89.75% statements | 82.05% branch Tests: 173 passed | 1 skipped
Contributor
📝 WalkthroughWalkthroughAdds tenant auto-provisioning during signup (swallowed errors), tenant-scoped memberRole and an atomic delete-if-not-last-admin operation, frontend admin/user route and page additions, Tabs UI, axios apiClient, a fancy tenant-name generator and dependency, multiple DB utility scripts, validation/DTO updates, and corresponding tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/User
participant AuthCtrl as Auth Controller
participant AuthSvc as Auth Service
participant UserProvider as User Provider
participant TenantProvider as Tenant Provider
participant DB as Database
Client->>AuthCtrl: POST /signup
AuthCtrl->>AuthSvc: createUser(input)
AuthSvc->>UserProvider: createUser(input)
UserProvider->>DB: INSERT user
DB-->>UserProvider: user
UserProvider-->>AuthSvc: user
AuthSvc->>TenantProvider: provisionTenantForUser(user.id)
alt provisioning succeeds
TenantProvider->>DB: INSERT org & member
DB-->>TenantProvider: success
TenantProvider-->>AuthSvc: ok
else provisioning fails
TenantProvider-->>AuthSvc: error
AuthSvc->>AuthSvc: log error (caught)
end
AuthSvc-->>AuthCtrl: user
AuthCtrl-->>Client: signup complete (user)
sequenceDiagram
participant Admin as Admin Client
participant UsersCtrl as Users Controller
participant UserProvider as User Adapter
participant DB as Database
Admin->>UsersCtrl: DELETE /users/{id}
UsersCtrl->>UsersCtrl: validate org context & not self
UsersCtrl->>UserProvider: deleteIfNotLastAdmin(userId, tenantId)
UserProvider->>DB: transaction: lock org, check membership, count admins
alt allowed
DB-->>UserProvider: ok
UserProvider->>DB: DELETE membership
DB-->>UserProvider: deleted
UserProvider-->>UsersCtrl: true
UsersCtrl-->>Admin: { success: true }
else blocked or not member
DB-->>UserProvider: blocked/not found
UserProvider-->>UsersCtrl: false
UsersCtrl-->>Admin: 4xx / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 25
🤖 Fix all issues with AI agents
In `@apps/api/src/modules/identity/auth/auth.service.spec.ts`:
- Around line 238-271: The two tests that exercise service.createUser (the
"should delegate to authProvider.createUser and auto-provision tenant" and
"should swallow error if auto-provisioning fails" it blocks) need to be moved
out of the hasSystemPermission describe and placed inside their own
describe('createUser', () => { ... }) block; locate the tests by the use of
service.createUser, mockAuthProvider.createUser, and (mockTenantProvider as
any).provisionTenantForUser and wrap those two it() cases in a new
describe('createUser') so they are topically grouped and no longer nested under
hasSystemPermission.
In `@apps/api/src/modules/identity/users/users.controller.ts`:
- Around line 68-70: Remove the debug console.log calls in users.controller.ts
that print '--- findAll result ---' and JSON.stringify(result.data.slice(0, 1),
null, 2); specifically delete those two lines in the method that returns result
(the findAll handler), as they leak user data to logs; if you need retained
visibility, replace with a proper logger call at debug level (e.g.,
processLogger.debug) and ensure you never log raw user objects or sensitive
fields when calling logger.
- Around line 139-143: Before calling this.userProvider.delete(id) add a
validation that prevents removing the last organization admin and avoids an
unintended global delete of a user who belongs to multiple orgs: fetch the
target orgId and the user's membership/roles (via the controller's
membership/organization services or methods), call a helper like
OrganizationService.countAdmins(orgId) or similar to ensure there is at least
one other admin before proceeding, and if the user is the last admin return a
4xx error; if the request is intended to remove the user only from the current
org (not globally) call the membership removal method (e.g.
MembershipService.removeMember(orgId, id)) instead of userProvider.delete(id).
Apply the same check to the system-admin deletion endpoint
(system-admin.controller.ts) to prevent deleting the last org admin there as
well.
In `@apps/api/src/modules/identity/users/users.validation.ts`:
- Around line 57-66: Create a shared constant for the role enum and its message,
and use it in both CreateUserSchema and InviteUserSchema to remove duplication;
specifically, extract the z.enum([...], { message: 'Role must be admin, editor,
viewer, or user' }) into a reusable symbol (e.g., RoleEnum or RoleEnumSchema)
and replace the inline enum in InviteUserSchema and the one in CreateUserSchema
with that shared constant, keeping the .default('user') call where needed and
ensuring InviteUser extends createZodDto(InviteUserSchema) continues to work.
In `@apps/web/src/modules/identity/pages/admin/users/UserEdit.tsx`:
- Around line 52-55: Use ES6 property shorthand for the useOne and update calls:
replace explicit object properties like resource: resource and id: id with
shorthand resource and id in the useOne(...) call and in the update(...)
invocation (and any similar objects around lines where update is used, e.g., the
block referencing values and mutationOptions) so the hooks read more concisely
while preserving the same identifiers.
In `@apps/web/src/modules/identity/pages/admin/users/UserShow.tsx`:
- Around line 23-25: The call to useShow<any> is using an explicit key-value
pair for resource; simplify by using ES6 property shorthand so the object passed
to useShow uses { resource } instead of { resource: resource }—update the
invocation where useShow is called (const start = useShow<any>({ resource }))
ensuring the symbol useShow and the variable resource are used with shorthand.
In `@apps/web/src/modules/identity/pages/TenantSettingsPage.tsx`:
- Around line 85-90: The users query in TenantSettingsPage uses queryKey
['users', orgId] but calls apiClient.get('/users') which ignores orgId and can
leak cross-tenant data; update the request in the useQuery queryFn to call a
tenant-scoped endpoint or include orgId as a query param (e.g.,
apiClient.get(`/tenants/${orgId}/users`) or
apiClient.get(`/users?orgId=${orgId}`)) so the network request matches the
queryKey and is properly scoped to orgId; ensure the symbol names involved are
useQuery, queryKey, orgId, and apiClient.get in TenantSettingsPage.tsx.
- Around line 68-77: Replace the semantic <label> elements that have no
associated form controls in TenantSettingsPage.tsx with non-label elements or
mark them as non-interactive so the a11y linter stops flagging them:
specifically change the three <label className="text-sm font-medium"> instances
that precede the display blocks for org.name, org.slug and org.id to a <div> or
<span> (or add aria-hidden="true") with the same classes so the visible headings
remain but are not treated as form labels by tools like Biome.
- Around line 45-55: The tenant lookup in the useQuery's queryFn (in
TenantSettingsPage) incorrectly calls apiClient.get(`/tenants?limit=1`) which
will usually miss the target org; change the call to fetch the tenant by ID
(e.g. call apiClient.get(`/tenants/${orgId}`) or the backend-supported GET
/tenants/:id equivalent) and return the single tenant object directly instead of
finding it in a list; update the queryFn to use orgId and handle 404/undefined
responses consistently with the component's error handling.
In `@apps/web/src/modules/identity/pages/UserProfilePage.tsx`:
- Around line 34-40: The <label> elements in UserProfilePage used for
display-only text (surrounding user.id and roles) should be replaced with
semantic non-form elements (e.g., <span> or <p>) to fix accessibility; also
guard against user.roles being undefined before calling .join() in the Roles
display (use a nullish/default array or conditional render so you show 'Member'
when roles is missing or empty). Update the JSX that renders user.id and
user.roles (in the UserProfilePage component) to use spans/paragraphs with the
same classes and to compute rolesDisplay = (user.roles && user.roles.length) ?
user.roles.join(', ') : 'Member' (or equivalent) before rendering.
In `@apps/web/src/modules/identity/users/UserList.tsx`:
- Line 37: Update the UserTableItem interface to include memberRole?: string so
the type reflects the identity schema change (add memberRole to the interface
definition), then remove the ad-hoc type assertion in UserList.tsx and change
the mapping to use user.memberRole || user.role (i.e., reference memberRole
directly from UserTableItem instead of casting).
- Line 32: Remove the debug console.log in the UserList component (the line
logging 'UserList Raw Data:' and data?.data) before merging; locate it in
UserList.tsx within the UserList component or render function and delete the
console.log (or replace with a conditional debug/logger that respects
environment) so no raw data is printed to the browser console in production.
In `@apps/web/src/shared/components/ui/tabs.tsx`:
- Around line 34-47: The TabsList component is missing the ARIA role required
for tab containers; update the React.forwardRef component TabsList to add
role="tablist" on the root div (ensuring it remains on the element that renders
the tab triggers) so screen readers recognize it as a tablist; keep the existing
props spread ({...props}) so overrides still work but default the role to
"tablist" on the div rendered by TabsList.
- Around line 53-80: TabsTrigger lacks keyboard navigation per WAI-ARIA tabs
pattern; add an onKeyDown handler inside the TabsTrigger component that handles
ArrowRight/ArrowLeft/Home/End (and optionally ArrowUp/ArrowDown) to move focus
and change tabs via context.setActiveTab; use the TabsContext (which should
expose the ordered list of tab values and a method to focus a tab or return
refs) to compute next/previous indices, call context.setActiveTab(newValue) and
programmatically focus the corresponding button element (use the forwarded ref
or a ref map on the context), and ensure aria-selected/data-state update; update
TabsTrigger to merge this onKeyDown with any provided onKeyDown prop so existing
handlers are preserved.
In `@apps/web/src/shared/lib/api-client.ts`:
- Line 3: The const apiURL (import.meta.env.VITE_API_URL) must be validated at
runtime to avoid axios falling back to relative URLs; update the module that
declares apiURL to check whether import.meta.env.VITE_API_URL is present and
either provide a safe default or throw in development (e.g., when
import.meta.env.DEV) — locate the apiURL declaration and surrounding axios
client creation/usage (look for apiURL and any axios.create or baseURL
references) and add a guard that logs/throws a clear error when missing and
otherwise sets baseURL to the validated value.
In `@packages/identity/src/utils/name-generator.ts`:
- Around line 1-15: The import animals is unused in generateFancyTenantName
because uniqueNamesGenerator is called with length: 2 and dictionaries:
[adjectives, colors]; remove the animals import and any reference to it (the
symbol animals) so only the needed imports remain (uniqueNamesGenerator,
adjectives, colors) and leave generateFancyTenantName unchanged otherwise.
In `@scripts/check-permissions.ts`:
- Around line 53-59: The script currently auto-runs seedRbac(db) when missing
permissions are detected; change this so seeding only occurs after explicit user
consent by either adding a CLI flag (e.g., --fix or --yes) parsed at startup or
prompting interactively before calling seedRbac(db). Locate the block
referencing the missing variable check and the seedRbac(db) call in
check-permissions.ts and: add argument parsing (or read process.argv) to detect
a --fix flag, or use a readline prompt to request confirmation, and only invoke
seedRbac(db) when the flag is present or the user types a clear affirmative
(e.g., "yes"); ensure the non-fix path still logs "Permissions look correct. No
action taken." and exit without modifying the DB.
In `@scripts/debug-member-role.ts`:
- Around line 30-50: The script uses an N+1 pattern by querying memberships per
user and roles per membership inside the loops (see variables users, members,
m.roleId and the db.select calls against schema.member and schema.role); change
this to two batched queries: first fetch all memberships for all user IDs (WHERE
IN on schema.member.userId) and group them by userId, then fetch all roles for
the distinct roleIds from those memberships (WHERE IN on schema.role.id) and
build a roleId->role map, then iterate users and their grouped memberships using
the pre-fetched maps to avoid per-item queries.
In `@scripts/fix-orphaned-users.ts`:
- Around line 15-16: The script silently exits when DATABASE_URL is missing;
before calling process.exit(1) update the dbUrl check to emit a clear error
(e.g., console.error or processLogger.error) that includes the name of the
missing env var (process.env.DATABASE_URL) and a short hint, then call
process.exit(1); modify the block using the dbUrl variable (the const dbUrl and
subsequent if (!dbUrl) process.exit(1)) to log the message first so failures are
visible when running scripts/fix-orphaned-users.ts.
- Line 5: The file imports "and" from 'drizzle-orm' but never uses it; update
the import statement that currently reads something like "import { eq, and }
from 'drizzle-orm';" to remove "and" so only the used symbol(s) (e.g., "eq")
remain, then run a quick local lint/type check to ensure no other references to
"and" exist.
- Around line 25-57: The organization creation and member insert inside the
users loop can leave a dangling organization if the member insert fails; wrap
the org and member inserts in a single transaction (use the existing
db.transaction/transactional API) so both inserts are committed or rolled back
together—specifically, perform the db.insert(schema.organization) and
db.insert(schema.member) for the given u inside one transaction callback
(referencing the users loop, the orgId/ orgName variables, and the two insert
calls) and move logging accordingly so failures rollback the org creation.
- Around line 23-27: The loop issues an N+1 by querying schema.member for each
user (users / for (const u of users) /
db.select().from(schema.member).where(eq(schema.member.userId, u.id))); replace
this with a single batched query (e.g., join users to members or select members
WHERE member.userId IN (...)) using a JOIN via
db.select().from(schema.user).leftJoin(schema.member, eq(schema.member.userId,
schema.user.id)) or a members-by-userId map built from
db.select().from(schema.member).where(sql`member.userId IN (${userIds})`),
ensure you import sql from drizzle-orm if using the IN pattern, then use that
map to check memberships instead of per-user queries.
In `@scripts/force-delete-user.ts`:
- Around line 34-52: The script currently hard-codes targetEmail and may not
always close the DB pool; replace the hard-coded targetEmail with a parameter
(e.g., read from process.argv or an env var and validate it) before calling
adapter.findByEmail, and ensure pool.end() is always invoked by wrapping the
main flow in a try/finally (or registering a process exit handler) so
adapter.findByEmail, adapter.delete, and any early exits (process.exit) still
guarantee pool.end() runs; also validate the provided email and bail with a
non-destructive message if missing or invalid.
In `@scripts/test-users-api.ts`:
- Around line 27-28: Replace the hardcoded tenantId string with code that reads
a CLI argument (e.g., process.argv or a small arg parser) so the script accepts
tenantId at runtime; locate the tenantId constant in scripts/test-users-api.ts,
parse the argument (with a sensible fallback or an error when missing), and
assign that parsed value to the tenantId variable so callers can run the script
with a provided tenant ID.
- Around line 14-15: The check for DATABASE_URL exits silently; update the guard
around dbUrl (const dbUrl = process.env.DATABASE_URL) to emit a clear error
before exiting—use console.error or a thrown Error with a message like "Missing
DATABASE_URL environment variable" and then call process.exit(1) (or rethrow) so
callers see why the script failed; ensure the message references DATABASE_URL
and the dbUrl variable so it’s obvious in logs.
Comment on lines
52
to
55
| const { data: userResult, isLoading: userLoading } = useOne({ | ||
| resource: "admin/users", | ||
| resource: resource, | ||
| id: id, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider ES6 property shorthand for consistency.
Both useOne and update calls can use property shorthand.
✨ Optional cleanup
const { data: userResult, isLoading: userLoading } = useOne({
- resource: resource,
+ resource,
id: id,
}); update(
{
- resource: resource,
+ resource,
id: id as string,
values: values,
},Also applies to: 77-83
🤖 Prompt for AI Agents
In `@apps/web/src/modules/identity/pages/admin/users/UserEdit.tsx` around lines 52
- 55, Use ES6 property shorthand for the useOne and update calls: replace
explicit object properties like resource: resource and id: id with shorthand
resource and id in the useOne(...) call and in the update(...) invocation (and
any similar objects around lines where update is used, e.g., the block
referencing values and mutationOptions) so the hooks read more concisely while
preserving the same identifiers.
Comment on lines
+23
to
+27
| const users = await db.select().from(schema.user); | ||
|
|
||
| for (const u of users) { | ||
| // Check memberships | ||
| const members = await db.select().from(schema.member).where(eq(schema.member.userId, u.id)); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
N+1 query pattern: Consider batching membership lookups.
Currently, this issues a separate query for each user to check memberships. For large user sets, this will be inefficient.
♻️ Proposed optimization using a JOIN
- const users = await db.select().from(schema.user);
-
- for (const u of users) {
- // Check memberships
- const members = await db.select().from(schema.member).where(eq(schema.member.userId, u.id));
+ // Get all users with their membership count in a single query
+ const usersWithMemberships = await db
+ .select({
+ user: schema.user,
+ hasMembership: sql<boolean>`EXISTS (SELECT 1 FROM ${schema.member} WHERE ${schema.member.userId} = ${schema.user.id})`
+ })
+ .from(schema.user);
+
+ for (const { user: u, hasMembership } of usersWithMemberships) {
+ if (hasMembership) continue;Note: You'll need to import sql from drizzle-orm.
🤖 Prompt for AI Agents
In `@scripts/fix-orphaned-users.ts` around lines 23 - 27, The loop issues an N+1
by querying schema.member for each user (users / for (const u of users) /
db.select().from(schema.member).where(eq(schema.member.userId, u.id))); replace
this with a single batched query (e.g., join users to members or select members
WHERE member.userId IN (...)) using a JOIN via
db.select().from(schema.user).leftJoin(schema.member, eq(schema.member.userId,
schema.user.id)) or a members-by-userId map built from
db.select().from(schema.member).where(sql`member.userId IN (${userIds})`),
ensure you import sql from drizzle-orm if using the IN pattern, then use that
map to check memberships instead of per-user queries.
Comment on lines
+25
to
+57
| for (const u of users) { | ||
| // Check memberships | ||
| const members = await db.select().from(schema.member).where(eq(schema.member.userId, u.id)); | ||
|
|
||
| if (members.length === 0) { | ||
| console.log(`\nFound Orphaned User: ${u.email} (${u.name || 'No Name'})`); | ||
|
|
||
| // 1. Create Organization | ||
| const orgName = u.name ? `${u.name}'s Organization` : `Organization for ${u.email.split('@')[0]}`; | ||
| const orgId = uuidv4(); | ||
| console.log(` > Creating Org: ${orgName}`); | ||
|
|
||
| await db.insert(schema.organization).values({ | ||
| id: orgId, | ||
| name: orgName, | ||
| slug: uuidv4(), // Temporary slug | ||
| status: 'active', | ||
| createdAt: new Date(), | ||
| updatedAt: new Date() | ||
| }); | ||
|
|
||
| // 2. Add Member as Admin | ||
| console.log(` > Assigning 'admin' role...`); | ||
| await db.insert(schema.member).values({ | ||
| id: uuidv4(), | ||
| userId: u.id, | ||
| organizationId: orgId, | ||
| roleId: 'admin', // Ensure this matches the ID in 'role' table | ||
| createdAt: new Date() | ||
| }); | ||
|
|
||
| console.log(` > [DONE] User is now Admin of their new/default org.`); | ||
| } |
Contributor
There was a problem hiding this comment.
Wrap organization and member inserts in a transaction.
If the member insert fails after the organization is created, the database will be left in an inconsistent state with an orphan organization.
🐛 Proposed fix
if (members.length === 0) {
console.log(`\nFound Orphaned User: ${u.email} (${u.name || 'No Name'})`);
- // 1. Create Organization
const orgName = u.name ? `${u.name}'s Organization` : `Organization for ${u.email.split('@')[0]}`;
const orgId = uuidv4();
- console.log(` > Creating Org: ${orgName}`);
- await db.insert(schema.organization).values({
- id: orgId,
- name: orgName,
- slug: uuidv4(), // Temporary slug
- status: 'active',
- createdAt: new Date(),
- updatedAt: new Date()
- });
-
- // 2. Add Member as Admin
- console.log(` > Assigning 'admin' role...`);
- await db.insert(schema.member).values({
- id: uuidv4(),
- userId: u.id,
- organizationId: orgId,
- roleId: 'admin', // Ensure this matches the ID in 'role' table
- createdAt: new Date()
- });
+ await db.transaction(async (tx) => {
+ console.log(` > Creating Org: ${orgName}`);
+ await tx.insert(schema.organization).values({
+ id: orgId,
+ name: orgName,
+ slug: uuidv4(),
+ status: 'active',
+ createdAt: new Date(),
+ updatedAt: new Date()
+ });
+
+ console.log(` > Assigning 'admin' role...`);
+ await tx.insert(schema.member).values({
+ id: uuidv4(),
+ userId: u.id,
+ organizationId: orgId,
+ roleId: 'admin',
+ createdAt: new Date()
+ });
+ });
console.log(` > [DONE] User is now Admin of their new/default org.`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const u of users) { | |
| // Check memberships | |
| const members = await db.select().from(schema.member).where(eq(schema.member.userId, u.id)); | |
| if (members.length === 0) { | |
| console.log(`\nFound Orphaned User: ${u.email} (${u.name || 'No Name'})`); | |
| // 1. Create Organization | |
| const orgName = u.name ? `${u.name}'s Organization` : `Organization for ${u.email.split('@')[0]}`; | |
| const orgId = uuidv4(); | |
| console.log(` > Creating Org: ${orgName}`); | |
| await db.insert(schema.organization).values({ | |
| id: orgId, | |
| name: orgName, | |
| slug: uuidv4(), // Temporary slug | |
| status: 'active', | |
| createdAt: new Date(), | |
| updatedAt: new Date() | |
| }); | |
| // 2. Add Member as Admin | |
| console.log(` > Assigning 'admin' role...`); | |
| await db.insert(schema.member).values({ | |
| id: uuidv4(), | |
| userId: u.id, | |
| organizationId: orgId, | |
| roleId: 'admin', // Ensure this matches the ID in 'role' table | |
| createdAt: new Date() | |
| }); | |
| console.log(` > [DONE] User is now Admin of their new/default org.`); | |
| } | |
| for (const u of users) { | |
| // Check memberships | |
| const members = await db.select().from(schema.member).where(eq(schema.member.userId, u.id)); | |
| if (members.length === 0) { | |
| console.log(`\nFound Orphaned User: ${u.email} (${u.name || 'No Name'})`); | |
| const orgName = u.name ? `${u.name}'s Organization` : `Organization for ${u.email.split('@')[0]}`; | |
| const orgId = uuidv4(); | |
| await db.transaction(async (tx) => { | |
| console.log(` > Creating Org: ${orgName}`); | |
| await tx.insert(schema.organization).values({ | |
| id: orgId, | |
| name: orgName, | |
| slug: uuidv4(), | |
| status: 'active', | |
| createdAt: new Date(), | |
| updatedAt: new Date() | |
| }); | |
| console.log(` > Assigning 'admin' role...`); | |
| await tx.insert(schema.member).values({ | |
| id: uuidv4(), | |
| userId: u.id, | |
| organizationId: orgId, | |
| roleId: 'admin', | |
| createdAt: new Date() | |
| }); | |
| }); | |
| console.log(` > [DONE] User is now Admin of their new/default org.`); | |
| } |
🤖 Prompt for AI Agents
In `@scripts/fix-orphaned-users.ts` around lines 25 - 57, The organization
creation and member insert inside the users loop can leave a dangling
organization if the member insert fails; wrap the org and member inserts in a
single transaction (use the existing db.transaction/transactional API) so both
inserts are committed or rolled back together—specifically, perform the
db.insert(schema.organization) and db.insert(schema.member) for the given u
inside one transaction callback (referencing the users loop, the orgId/ orgName
variables, and the two insert calls) and move logging accordingly so failures
rollback the org creation.
Comment on lines
+34
to
+52
| const targetEmail = "pramod.narayana+tenant1@gmail.com"; | ||
|
|
||
| console.log(`Looking for user: ${targetEmail}`); | ||
| const user = await adapter.findByEmail(targetEmail); | ||
|
|
||
| if (!user) { | ||
| console.log("User not found. They might have already been deleted."); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| console.log(`Found user: ${user.id}. Deleting...`); | ||
|
|
||
| // We can use the adapter's delete, but we need to make sure we clean up the member relation explicitly if the adapter doesn't (it seemed to have cascading logic in previous view, let's verify or plain use raw queries if unsafe) | ||
| // Checking adapter code from memory: it did have transaction with manual deletes for member, invitation, session, account. | ||
|
|
||
| await adapter.delete(user.id); | ||
| console.log("User successfully deleted."); | ||
|
|
||
| await pool.end(); |
Contributor
There was a problem hiding this comment.
Avoid hard-coded email; parameterize and always close the DB pool.
Hard-coding a real email is a privacy/compliance risk and makes the script risky to run. Also, ensure the pool is closed on all exit paths.
🔧 Proposed fix
- const adapter = new DrizzleUserAdapter(db, mockAuthProvider);
- const targetEmail = "pramod.narayana+tenant1@gmail.com";
+ const adapter = new DrizzleUserAdapter(db, mockAuthProvider);
+ const targetEmail = process.argv[2] || process.env.TARGET_EMAIL;
+ if (!targetEmail) {
+ console.error("Usage: TARGET_EMAIL=<email> ts-node scripts/force-delete-user.ts");
+ process.exit(1);
+ }
- console.log(`Looking for user: ${targetEmail}`);
- const user = await adapter.findByEmail(targetEmail);
-
- if (!user) {
- console.log("User not found. They might have already been deleted.");
- process.exit(0);
- }
-
- console.log(`Found user: ${user.id}. Deleting...`);
- await adapter.delete(user.id);
- console.log("User successfully deleted.");
-
- await pool.end();
+ try {
+ console.log(`Looking for user: ${targetEmail}`);
+ const user = await adapter.findByEmail(targetEmail);
+
+ if (!user) {
+ console.log("User not found. They might have already been deleted.");
+ return;
+ }
+
+ console.log(`Found user: ${user.id}. Deleting...`);
+ await adapter.delete(user.id);
+ console.log("User successfully deleted.");
+ } finally {
+ await pool.end();
+ }🤖 Prompt for AI Agents
In `@scripts/force-delete-user.ts` around lines 34 - 52, The script currently
hard-codes targetEmail and may not always close the DB pool; replace the
hard-coded targetEmail with a parameter (e.g., read from process.argv or an env
var and validate it) before calling adapter.findByEmail, and ensure pool.end()
is always invoked by wrapping the main flow in a try/finally (or registering a
process exit handler) so adapter.findByEmail, adapter.delete, and any early
exits (process.exit) still guarantee pool.end() runs; also validate the provided
email and bail with a non-destructive message if missing or invalid.
Comment on lines
+14
to
+15
| const dbUrl = process.env.DATABASE_URL; | ||
| if (!dbUrl) process.exit(1); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Silent exit without error message hinders debugging.
When DATABASE_URL is missing, the script exits silently with code 1, making it unclear why it failed.
🔧 Add error message before exit
const dbUrl = process.env.DATABASE_URL;
- if (!dbUrl) process.exit(1);
+ if (!dbUrl) {
+ console.error('Error: DATABASE_URL is not set. Check apps/api/.env');
+ process.exit(1);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const dbUrl = process.env.DATABASE_URL; | |
| if (!dbUrl) process.exit(1); | |
| const dbUrl = process.env.DATABASE_URL; | |
| if (!dbUrl) { | |
| console.error('Error: DATABASE_URL is not set. Check apps/api/.env'); | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
In `@scripts/test-users-api.ts` around lines 14 - 15, The check for DATABASE_URL
exits silently; update the guard around dbUrl (const dbUrl =
process.env.DATABASE_URL) to emit a clear error before exiting—use console.error
or a thrown Error with a message like "Missing DATABASE_URL environment
variable" and then call process.exit(1) (or rethrow) so callers see why the
script failed; ensure the message references DATABASE_URL and the dbUrl variable
so it’s obvious in logs.
Comment on lines
+27
to
+28
| // Org ID for tenant1 from previous debug script: 02b8e986-81d0-4a8d-b2db-ad29e93999d3 | ||
| const tenantId = '02b8e986-81d0-4a8d-b2db-ad29e93999d3'; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded tenant ID limits script reusability.
Consider accepting the tenant ID as a command-line argument for broader utility.
🔧 Accept tenantId from CLI argument
- // Org ID for tenant1 from previous debug script: 02b8e986-81d0-4a8d-b2db-ad29e93999d3
- const tenantId = '02b8e986-81d0-4a8d-b2db-ad29e93999d3';
+ const tenantId = process.argv[2];
+ if (!tenantId) {
+ console.error('Usage: npx ts-node scripts/test-users-api.ts <tenantId>');
+ process.exit(1);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Org ID for tenant1 from previous debug script: 02b8e986-81d0-4a8d-b2db-ad29e93999d3 | |
| const tenantId = '02b8e986-81d0-4a8d-b2db-ad29e93999d3'; | |
| const tenantId = process.argv[2]; | |
| if (!tenantId) { | |
| console.error('Usage: npx ts-node scripts/test-users-api.ts <tenantId>'); | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
In `@scripts/test-users-api.ts` around lines 27 - 28, Replace the hardcoded
tenantId string with code that reads a CLI argument (e.g., process.argv or a
small arg parser) so the script accepts tenantId at runtime; locate the tenantId
constant in scripts/test-users-api.ts, parse the argument (with a sensible
fallback or an error when missing), and assign that parsed value to the tenantId
variable so callers can run the script with a provided tenant ID.
pramodnarayana
marked this pull request as draft
February 3, 2026 06:52
pramodnarayana
marked this pull request as ready for review
February 3, 2026 06:52
… type safety improvements Security Enhancements: - Remove debug console.log statements to prevent data leakage - Fix tenant data scoping with proper API endpoints (/tenants/:id, /tenants/:id/users) - Add last-admin deletion protection with ROLES constant - Add runtime validation for VITE_API_URL environment variable Accessibility Improvements: - Replace semantic <label> elements with <div> for display-only text - Add ARIA role="tablist" to TabsList component - Implement WAI-ARIA keyboard navigation (Arrow keys, Home, End) in TabsTrigger Type Safety & Code Quality: - Add memberRole property to UserTableItem interface - Add null safety for user.roles.join() with fallback to 'Member' - Extract shared ROLES constant to eliminate duplication - Apply ES6 property shorthand syntax (resource, id) - Remove unused animals import from name-generator Test Improvements: - Reorganize test structure (move createUser tests to dedicated block) - Add 2 comprehensive test cases for last-admin protection - All tests passing: 175 passed | 1 skipped Coverage: 82.5% branch, 89.97% statements (above 80% threshold)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/modules/identity/pages/admin/users/UserShow.tsx (1)
23-50:⚠️ Potential issue | 🟡 MinorNormalize
resourceonce to avoid hook/invite mismatches.
resourcePathis trimmed for the invite URL, butuseShow/useCanstill use the raw value. Ifresourcehas leading/trailing slashes, hooks and invite calls can point at different resources.Suggested fix
- const start = useShow<any>({ - resource, - }); + const normalizedResource = resource.replace(/^\/+|\/+$/g, ""); + const start = useShow<any>({ + resource: normalizedResource, + }); // useShow gives us 'queryResult'. We want strict permission check for buttons. const { data: canManage } = useCan({ - resource: resource, + resource: normalizedResource, action: "edit", }); @@ - const resourcePath = resource.replace(/^\/+|\/+$/g, ""); + const resourcePath = normalizedResource;apps/web/src/modules/identity/pages/admin/users/UserEdit.tsx (1)
52-110:⚠️ Potential issue | 🟡 MinorNormalize
resourceonce and reuse for useOne/update/invite.
Invite URL trims slashes butuseOne/updateuse the rawresource, which can diverge if callers pass/admin/usersor similar.Suggested fix
+ const normalizedResource = resource.replace(/^\/+|\/+$/g, ""); const { data: userResult, isLoading: userLoading } = useOne({ - resource, + resource: normalizedResource, id, }); @@ - resource: resource, + resource: normalizedResource, id: id as string, values: values, }, @@ - const resourcePath = resource.replace(/^\/+|\/+$/g, ""); + const resourcePath = normalizedResource;
🤖 Fix all issues with AI agents
In `@apps/api/src/modules/identity/auth/auth.service.spec.ts`:
- Around line 240-259: The test dynamically assigns (mockTenantProvider as
any).provisionTenantForUser which bypasses types—declare provisionTenantForUser
on the mockTenantProvider mock up front instead: add a jest/vi mock function
property named provisionTenantForUser to the mockTenantProvider definition and
return the desired resolved value there, then remove the dynamic assignment and
eslint-disable comments in the createUser test; update the error-case test to
use the same mock property (provisionTenantForUser) so all references use the
typed mockTenantProvider.provisionTenantForUser instead of casting to any.
In `@apps/api/src/modules/identity/users/users.controller.ts`:
- Around line 137-151: The current last-admin protection in users.controller.ts
(targetMembership, userTenants check, this.userProvider.findAll + ROLES.ADMIN
count and BadRequestException) is vulnerable to TOCTOU race conditions; move the
admin-check and deletion into a single atomic operation inside the data layer by
adding a transactional provider method (e.g., userProvider.deleteIfNotLastAdmin
or a DB-level constraint) that verifies the user’s memberRole and the total
admin count and performs the delete in one transaction, and call that new
provider method from the controller instead of performing the separate
findAll/count and delete.
In `@apps/api/src/modules/identity/users/users.validation.ts`:
- Around line 7-19: RoleEnum is duplicated from ROLES; change RoleEnum to be
derived from the single source of truth ROLES by building the enum options from
Object.values(ROLES) (e.g., z.enum(Object.values(ROLES) as [/* tuple cast */]) )
and keep the same error message; update the RoleEnum declaration (symbol:
RoleEnum) to use Object.values(ROLES) so any change to ROLES (symbol: ROLES)
automatically updates the validation.
In `@apps/web/src/modules/identity/pages/TenantSettingsPage.tsx`:
- Around line 101-131: The component TenantSettingsPage currently only checks
isLoading and renders the users table, so when the useQuery call fails (users
undefined and isLoading false) the UI shows an empty table with no feedback;
update the query destructure to include isError (and optionally error) from the
useQuery hook, and add a conditional branch (e.g., if isError) that renders a
clear error message (using error.message if available) instead of the
table—locate the useQuery invocation and the JSX that references isLoading and
users to implement this error-state handling.
In `@apps/web/src/shared/components/ui/tabs.tsx`:
- Around line 133-152: TabsContent currently returns a panel without ARIA id
relationships; update TabsContent to render an id and aria-labelledby that link
the panel to its trigger (e.g., id={`tabpanel-${value}`} and
aria-labelledby={`tab-${value}`}) and coordinate IDs via TabsContext—either use
existing context helpers or extend TabsContext with functions like
getTabId/getPanelId (or a mapping) so TabsTrigger can use the matching
`tab-${value}` id and TabsContent uses `tabpanel-${value}`; ensure IDs are
unique per value and preserve forwardRef and other props.
…nd accessibility Mock Type Safety: - Add typed provisionTenantForUser to mockTenantProvider definition - Remove dynamic any casting and eslint-disable comments in auth.service.spec.ts DRY Principle: - Derive RoleEnum from ROLES using Object.values() for single source of truth - Ensure role validation automatically updates when ROLES changes Error State Handling & UI Improvements: - Add isError and error to TenantSettingsPage useQuery - Display clear error messages with error.message fallback - Refactor nested ternary to independent if statements for readability Complete ARIA Relationships: - Add getTabId/getPanelId helpers to TabsContext - Implement id and aria-labelledby on TabsContent - Link TabsTrigger with TabsContent via aria-controls TOCTOU Race Condition Fix (Critical): - Add deleteIfNotLastAdmin method to IUserProvider interface - Implement atomic transaction in DrizzleUserAdapter with role table joins - Replace vulnerable multi-step check+delete with single atomic operation - Update users.controller.ts to use atomic deleteIfNotLastAdmin - Update all controller tests to use atomic operation expectations - Prevents race condition where two concurrent requests could delete last admin All tests passing: 175 passed | 1 skipped Coverage: API 89.97%, Identity 91.74%
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@apps/api/src/modules/identity/users/users.controller.spec.ts`:
- Around line 257-276: The test "should successfully delete admin if there are
other admins" is a duplicate of the prior case; remove the redundant it block or
modify it to exercise a different path: either (A) delete the duplicate test
entirely, or (B) change the mock behavior of userProvider.deleteIfNotLastAdmin
and assertions to cover the alternate flow (e.g., mockResolvedValue(false) and
assert that controller.remove throws the expected Forbidden/BadRequest error or
returns { success: false } depending on controller.remove's behavior). Locate
the duplicate by the test name and references to controller.remove and
userProvider.deleteIfNotLastAdmin when applying the change.
In `@apps/api/src/modules/identity/users/users.controller.ts`:
- Around line 129-137: Wrap the atomic call to
this.userProvider.deleteIfNotLastAdmin(id, tenantId) in a try-catch, catch the
provider-specific error (e.g., inspect error.name or message for "not a member"
/ a custom NotMemberError), and translate it into an appropriate HTTP exception
(for example throw new NotFoundException('User not found in organization') or
new ForbiddenException(...)); keep the existing deleted-check path for the
last-admin case and rethrow or throw a generic
BadRequestException/InternalServerError for unexpected errors so they don't
surface as 500 with ambiguous messages.
In `@apps/web/src/modules/identity/pages/TenantSettingsPage.tsx`:
- Line 80: UsersListTab's props are typed as { orgId: string } while
GeneralSettingsTab uses Readonly<{ orgId: string }>, causing an inconsistency;
update UsersListTab's signature to use Readonly<{ orgId: string }> (matching
GeneralSettingsTab) so both components share the same immutable prop type, and
adjust any references to UsersListTab if needed to satisfy the new type.
- Around line 116-140: In TenantSettingsPage update the table header elements
(<th>) used where users is rendered to include scope="col" so screen readers can
associate header cells with their data cells; locate the table in the
TenantSettingsPage component where users?.map(...) is used and add scope="col"
to the three header <th> elements for "Name", "Email", and "Role" (you may also
add a caption element above the table for additional accessibility if desired).
- Around line 45-54: The useQuery call in TenantSettingsPage currently only
checks isLoading and org, which conflates API errors and missing orgs; update
the destructuring to include isError and error from useQuery (the existing
queryKey/queryFn block) and add a branch that returns an informative error UI
when isError is true (e.g., render "Error loading organization" and include
error.message or a friendly fallback), while keeping the existing "Organization
not found" when org is null and preserving the isLoading handling in the
component.
In `@apps/web/src/shared/components/ui/tabs.tsx`:
- Around line 25-26: The getTabId and getPanelId helpers create static IDs
(`tab-${value}` / `tabpanel-${value}`) which can collide across multiple Tabs
instances; update the Tabs component to generate a unique prefix (e.g., via
React's useId() or another instance-unique token) and prepend it to these
helpers (modify getTabId and getPanelId to include the prefix) so each Tabs
instance emits distinct ARIA IDs (keep references to getTabId, getPanelId, and
the Tabs component when implementing).
In `@packages/identity/src/adapters/drizzle-user.adapter.ts`:
- Around line 242-261: The role-comparison is case-sensitive: change the
hardcoded checks that compare userRole and schema.role.name against "admin" to
match the database value "Admin" so the admin-deletion-prevention in the block
around userRole, adminCountResult and the where clause (eq(schema.role.name,
...)) actually runs; update both occurrences and optionally extract the role
string into a shared constant (e.g., ADMIN_ROLE_NAME) used by the userRole check
and the tx.where eq(schema.role.name, ...) to avoid future mismatches.
…nique tab ids Controller Tests: - Remove duplicate test case in users.controller.spec.ts Controller Error Handling: - Wrap atomic delete in try-catch to translate provider errors to correct HTTP exceptions (e.g., NotFoundException) TenantSettingsPage: - Use Readonly props for consistency - Add scope='col' to table headers for accessibility - Improve organization query error handling Tabs Component: - Use React.useId() to ensure unique ARIA IDs across multiple instances Drizzle User Adapter: - Fix case-sensitive role comparison by using 'Admin' (database value) instead of 'admin' - Extract ADMIN_ROLE_NAME constant
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/api/src/modules/identity/users/users.controller.ts`:
- Around line 158-161: The controller currently throws a BadRequestException
with the raw error.message (in the delete handler inside UsersController), which
can leak internals; change the exception to a generic client-facing message like
"Failed to delete user" and stop propagating error.message to the response, and
instead log the full error details internally (e.g., this.logger.error or a
monitored error sink) before throwing the generic BadRequestException in the
UsersController delete method.
In `@apps/web/src/shared/components/ui/tabs.tsx`:
- Around line 60-132: The TabsTrigger currently leaves every tab in the natural
tab order and includes disabled tabs in keyboard navigation; update TabsTrigger
to implement roving tabindex and skip disabled tabs: compute tabIndex={isActive
? 0 : -1} on the button (so only the active tab is tabbable), and in
handleKeyDown build the tabs array by querying for role="tab" and then filtering
out disabled buttons (e.g. .filter(t => !t.disabled)) before computing
currentIndex/nextIndex; use the filtered array to compute nextTab, focus/click
it, and call context.setActiveTab(value) in the onClick handler as already
present so state stays in sync.
In `@packages/identity/src/adapters/drizzle-user.adapter.ts`:
- Around line 213-216: Update the JSDoc for the method currently documented as
"Atomically delete a user" to accurately describe its behavior: it removes a
user's membership from an organization while preventing removal of the last
admin. Change the description for the method (the function that performs the
membership deletion in this file—e.g., removeUserFromOrganization /
deleteUserMembership) to something like "Atomically remove a user's membership
from an organization, ensuring the user is not the last admin" and mention that
the user account itself is not deleted (the implementation deletes only the
membership record around the membership deletion block).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/api/src/modules/identity/users/users.controller.ts`:
- Around line 10-13: The delete endpoint in UsersController currently throws
BadRequestException for all unhandled errors; update the generic failure path to
throw InternalServerErrorException instead while preserving BadRequestException
for the "last-admin" and "self-delete" validation checks. Specifically, in the
controller's delete handler (the delete method in UsersController) change the
catch / generic error throw from new BadRequestException(...) to new
InternalServerErrorException(...), and add InternalServerErrorException to the
imports from '@nestjs/common'. Keep existing BadRequestException usage for the
validation branches.
In `@apps/web/src/shared/components/ui/tabs.tsx`:
- Around line 160-172: Add keyboard focusability to the tabpanel by adding
tabIndex={0} to the div with role="tabpanel" (the element using
context.getPanelId(value) and aria-labelledby={context.getTabId(value)} in
tabs.tsx); update the JSX for the tabpanel div so it includes tabIndex={0}
alongside the existing props and className to ensure users can focus the panel
after activating a tab for proper WAI-ARIA keyboard accessibility.
- Around line 36-41: The CN call in the Tabs component is passed an unnecessary
empty string: replace the call cn("", className) inside the TabsContext.Provider
render (the div with ref={ref} and className handling) with a call that omits
the empty string (e.g., cn(className) or simply use className directly) so only
meaningful class tokens are passed; update the div that uses ref, className and
{...props} accordingly.
In `@packages/identity/src/adapters/drizzle-user.adapter.ts`:
- Around line 213-278: The deleteIfNotLastAdmin transaction suffers write-skew:
two concurrent calls can both observe adminCount > 1 and both delete, removing
the last admins; fix by taking a strong lock scoped to the tenant inside
deleteIfNotLastAdmin before counting and deleting (e.g. acquire a row-level FOR
UPDATE on the organization row or use a transaction-scoped advisory lock keyed
by tenantId, or run the transaction at SERIALIZABLE and retry on serialization
failure); ensure the lock/acquire happens inside the same tx before computing
adminCount and performing tx.delete so only one concurrent transaction can
proceed to delete for that tenant.
…e locking, strict types
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/identity/src/adapters/drizzle-user.adapter.ts (1)
111-143: 🧹 Nitpick | 🔵 TrivialTenant-scoped list is richer, but avoid duplicate tenant filters.
Nice to exposememberRole. SincebuildUserFiltersalready injects the tenant condition, the explicitorganizationIdpredicate here (and in the count query) is redundant—consider relying on the filters array to keep a single source of truth.♻️ Suggested simplification
- .where( - and( - eq(schema.member.organizationId, options.tenantId), - ...(filters.length ? filters : []), - ), - ) + .where(and(...filters))- .where( - and( - eq(schema.member.organizationId, options.tenantId), - ...(filters.length ? filters : []), - ), - ); + .where(and(...filters));
🤖 Fix all issues with AI agents
In `@packages/identity/src/adapters/drizzle-user.adapter.spec.ts`:
- Around line 327-332: The test mock returns a role with roleName "admin", but
deleteIfNotLastAdmin expects the canonical "Admin", so update the mock in
tx.select.mockReturnValueOnce to return [{ roleName: "Admin" }] to exercise the
admin-count branch; ensure the change targets the mock used in the
deleteIfNotLastAdmin test so the code path that checks for "Admin" is executed.
- Around line 335-341: The mock creates a thenable (has a then property) which
Biome flags; replace the thenable with an async mock by removing the then
property and using tx.select.mockResolvedValueOnce([{ count: 2 }]) (or the same
mockResolvedValueOnce pattern used at line 331) so the select call resolves to
the expected array instead of returning an object with a then property.
This was referenced Feb 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…rotection
Coverage: 89.75% statements | 82.05% branch
Tests: 173 passed | 1 skipped
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation