Repository navigation
Refactor/identity strict layer enforcement - #28
Merged
pramodnarayana merged 13 commits intoJan 25, 2026
Merged
Conversation
- Decouple SystemAdminController from Drizzle - Implement IUserProvider/ITenantProvider methods - Rewrite controller tests with strict mocking - Correct Jest coverage exclusions
Contributor
📝 WalkthroughWalkthroughRefactors Identity to replace direct Drizzle/DB usage with injected provider tokens (AUTH_PROVIDER, USER_PROVIDER, TENANT_PROVIDER), expands provider interfaces/adapters with paginated CRUD/count APIs, removes legacy TenantsService and its tests, updates controller tests to use provider mocks, adjusts module wiring, and adds design/refactor docs and coverage path updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Guard as Guard (Auth/Platform)
participant Controller as Controller
participant AuthProv as AuthProvider
participant UserProv as UserProvider
participant TenantProv as TenantProvider
participant Adapter as DrizzleAdapter/DB
Client->>Guard: HTTP request (headers)
Guard-->>Controller: allow (or bypassed in tests)
Controller->>AuthProv: getSessionFromHeaders / verify
AuthProv-->>Controller: session / user
Controller->>UserProv: findAll/create/update/delete (params)
UserProv->>Adapter: SQL queries / transactions
Adapter-->>UserProv: record or { data, total }
Controller->>TenantProv: findAll/createTenant/findBySlug/update/delete
TenantProv->>Adapter: tenant SQL operations / transactions
Adapter-->>TenantProv: tenant record or { data, total }
Controller-->>Client: HTTP response ({ data, total } or item)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/modules/identity/users/users.controller.ts (1)
46-60: Consider returning pagination metadata for future extensibility.The method correctly unwraps
result.data, but discardsresult.total. If the API needs pagination support later, you'll need to change the response shape. Consider whether returning the full envelope{ data, total }now would be more future-proof.async findAll(`@Req`() req: Request & { user: { organizationId?: string } }) { const tenantId = req.user?.organizationId; if (!tenantId) { return []; } const result = await this.userProvider.findAll({ tenantId }); - return result.data; + return result; // Or result.data if flat array is intentional }If the flat array return is intentional for API compatibility, this is fine as-is.
🤖 Fix all issues with AI agents
In `@apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts`:
- Around line 12-58: Add unit tests in system-admin.controller.spec.ts covering
the SystemAdminController.createSystemInvitation endpoint: (1) an unauthorized
case where mockAuthProvider.getSessionFromHeaders is mocked to return
null/undefined and you assert the controller call rejects/throws or returns the
expected unauthorized error and that mockAuthProvider.createInvitation is not
called; (2) a success case where mockAuthProvider.getSessionFromHeaders returns
a valid session object, mockAuthProvider.createInvitation returns a fake
invitation object, invoke controller.createSystemInvitation with mockHeaders and
a sample DTO, then assert getSessionFromHeaders and createInvitation were called
with the correct args and the controller returns the invitation. Use the
existing mockAuthProvider, mockHeaders and reference
SystemAdminController.createSystemInvitation,
AUTH_PROVIDER.getSessionFromHeaders and AUTH_PROVIDER.createInvitation when
implementing these tests.
In `@apps/api/src/modules/identity/system-admin/system-admin.controller.ts`:
- Around line 280-294: Tighten the comment explaining the admin-count check
around user.systemRole === 'platform_admin' and this.userProvider.count so it's
explicit that count() includes the target user and therefore the guard is
intentionally using adminCount <= 1; update the inline comment above the if
(adminCount <= 1) throw new BadRequestException(...) to state: count includes
the current/target user, so a count of 1 means the user is the last admin (0 is
inconsistent) and thus deletion must be prevented—no logic change required, just
clarify intent.
- Around line 138-160: Remove the inline debug/comment block between the call to
this.userProvider.create and the subsequent systemRole handling; then eliminate
the two-step create+update by adding systemRole to the CreateUserInput type (and
propagate that addition to the provider/adapters so they accept/persist
systemRole) and pass systemRole directly in the single create call (e.g.,
this.userProvider.create({ ...input, systemRole: input.systemRole })); if
provider cannot be changed immediately, keep the update but remove comments and
add a TODO with a short note to extend CreateUserInput later.
In `@docs/refactor/modules/identity/task/01c_strict_layer_enforcement.md`:
- Around line 27-31: The documented method signatures for IUserProvider.findAll
and ITenantProvider.findAll require page and limit but the actual interfaces
declare them as optional; update the doc to match the real signatures: change
IUserProvider.findAll to use options?: { page?: number; limit?: number; search?:
string; tenantId?: string } and change ITenantProvider.findAll to options?: {
page?: number; limit?: number } (i.e., mark page/limit optional and include
tenantId in the IUserProvider signature) so the documentation aligns with the
IUserProvider and ITenantProvider interfaces.
In `@packages/identity/src/adapters/drizzle-tenant.adapter.ts`:
- Around line 59-76: createTenant currently inserts an organization and will
bubble up a raw DB constraint error (23505) when slug already exists; wrap the
insert in a try/catch (similar to the existing create method) to detect Postgres
unique-violation (code "23505") for schema.organization.slug and either retry
with a new slug or throw a clear, user-friendly error indicating the slug is
taken; ensure you reference the same db.insert(schema.organization).values(...)
call, preserve uuidv4() and createdAt usage, and still return
this.mapTenant(org) on success.
- Around line 97-109: The delete method currently silently succeeds when the
organization id doesn't exist; to make behavior consistent with update, first
check for existence of the organization inside the same transaction (e.g.,
select from schema.organization where schema.organization.id = id) and if no row
is found throw the same "Tenant not found" error, otherwise proceed with the
current deletes (members, invitations, organization) using this.db.transaction;
alternatively, if idempotent delete is desired, add a clear comment/docstring on
the delete method explaining that deletion of non‑existent tenants is a no-op.
In `@packages/identity/src/adapters/drizzle-user.adapter.ts`:
- Around line 97-106: The code builds a local filters array with the search
condition but only uses it in the tenant-scoped branch while the non-tenant
branch recreates an equivalent globalFilters array; consolidate by creating a
single baseFilters (or reuse filters) containing the ilike(schema.user.email,
`%${options.search}%`) when options?.search is present, then in the tenant case
push eq(schema.member.organizationId, options.tenantId) into that same array and
in the global case reuse baseFilters as globalFilters so you avoid duplicating
the search filter creation; update references to filters/globalFilters
accordingly (functions/variables: filters, globalFilters, options?.tenantId,
ilike, schema.user.email, schema.member.organizationId).
Comment on lines
138
to
160
| // Adapt input to provider requirement (provider handles ID generation and timestamps) | ||
| const user = await this.userProvider.create({ | ||
| ...input, | ||
| // Default systemRole handled by provider or we pass explicitly? | ||
| // create signature: (input: CreateUserInput) -> email, password?, firstName?, lastName?, role? | ||
| // Does CreateUserInput support systemRole? | ||
| // Let's check CreateUserInput interface. | ||
| // It supports role, but not systemRole explicitly in interface file I saw? | ||
| // Wait, let's verify CreateUserInput. | ||
| }); | ||
| // Ah, DrizzleUserAdapter delegates to authProvider which delegates to BetterAuth. | ||
| // BetterAuth input usually has role. | ||
| // If 'systemRole' is not in CreateUserInput, we might need to update user AFTER create | ||
| // OR update CreateUserInput. | ||
| // For now, assume provider handles it or update immediately. | ||
| // Let's assume we update immediately if provider doesn't support generic fields in create. | ||
| if (input.systemRole) { | ||
| await this.userProvider.update(user.id, { systemRole: input.systemRole }); | ||
| return this.userProvider.findById(user.id); | ||
| } | ||
|
|
||
| return user; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug comments and consider consolidating the create flow.
Lines 141-153 contain extensive inline comments expressing uncertainty about the interface. These should be removed before merging. Additionally, the two-step create + update pattern for systemRole is suboptimal.
♻️ Suggested cleanup
// Adapt input to provider requirement (provider handles ID generation and timestamps)
- const user = await this.userProvider.create({
- ...input,
- // Default systemRole handled by provider or we pass explicitly?
- // create signature: (input: CreateUserInput) -> email, password?, firstName?, lastName?, role?
- // Does CreateUserInput support systemRole?
- // Let's check CreateUserInput interface.
- // It supports role, but not systemRole explicitly in interface file I saw?
- // Wait, let's verify CreateUserInput.
- });
- // Ah, DrizzleUserAdapter delegates to authProvider which delegates to BetterAuth.
- // BetterAuth input usually has role.
- // If 'systemRole' is not in CreateUserInput, we might need to update user AFTER create
- // OR update CreateUserInput.
- // For now, assume provider handles it or update immediately.
- // Let's assume we update immediately if provider doesn't support generic fields in create.
+ const user = await this.userProvider.create(input);
+
if (input.systemRole) {
await this.userProvider.update(user.id, { systemRole: input.systemRole });
return this.userProvider.findById(user.id);
}
return user;Consider extending CreateUserInput to include systemRole to eliminate the extra round-trip.
📝 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
| // Adapt input to provider requirement (provider handles ID generation and timestamps) | |
| const user = await this.userProvider.create({ | |
| ...input, | |
| // Default systemRole handled by provider or we pass explicitly? | |
| // create signature: (input: CreateUserInput) -> email, password?, firstName?, lastName?, role? | |
| // Does CreateUserInput support systemRole? | |
| // Let's check CreateUserInput interface. | |
| // It supports role, but not systemRole explicitly in interface file I saw? | |
| // Wait, let's verify CreateUserInput. | |
| }); | |
| // Ah, DrizzleUserAdapter delegates to authProvider which delegates to BetterAuth. | |
| // BetterAuth input usually has role. | |
| // If 'systemRole' is not in CreateUserInput, we might need to update user AFTER create | |
| // OR update CreateUserInput. | |
| // For now, assume provider handles it or update immediately. | |
| // Let's assume we update immediately if provider doesn't support generic fields in create. | |
| if (input.systemRole) { | |
| await this.userProvider.update(user.id, { systemRole: input.systemRole }); | |
| return this.userProvider.findById(user.id); | |
| } | |
| return user; | |
| } | |
| // Adapt input to provider requirement (provider handles ID generation and timestamps) | |
| const user = await this.userProvider.create(input); | |
| if (input.systemRole) { | |
| await this.userProvider.update(user.id, { systemRole: input.systemRole }); | |
| return this.userProvider.findById(user.id); | |
| } | |
| return user; |
🤖 Prompt for AI Agents
In `@apps/api/src/modules/identity/system-admin/system-admin.controller.ts` around
lines 138 - 160, Remove the inline debug/comment block between the call to
this.userProvider.create and the subsequent systemRole handling; then eliminate
the two-step create+update by adding systemRole to the CreateUserInput type (and
propagate that addition to the provider/adapters so they accept/persist
systemRole) and pass systemRole directly in the single create call (e.g.,
this.userProvider.create({ ...input, systemRole: input.systemRole })); if
provider cannot be changed immediately, keep the update but remove comments and
add a TODO with a short note to extend CreateUserInput later.
Comment on lines
+27
to
+31
| - [ ] **Modify `IUserProvider`**: Add methods to support Admin Console requirements. | ||
| - `findAll(options?: { page: number; limit: number; search?: string }): Promise<{ data: User[]; total: number }>` | ||
| - `count(): Promise<number>` | ||
| - [ ] **Modify `ITenantProvider`**: Add methods for Admin listing. | ||
| - `findAll(options?: { page: number; limit: number }): Promise<{ data: Tenant[]; total: number }>` |
Contributor
There was a problem hiding this comment.
Interface signature mismatch with implementation.
The documented signature shows page and limit as required parameters:
findAll(options?: { page: number; limit: number; search?: string })
However, the actual IUserProvider interface (from the relevant code snippets) defines them as optional:
findAll(options?: { page?: number; limit?: number; search?: string; tenantId?: string })Update the documentation to reflect the actual interface to avoid confusion.
🤖 Prompt for AI Agents
In `@docs/refactor/modules/identity/task/01c_strict_layer_enforcement.md` around
lines 27 - 31, The documented method signatures for IUserProvider.findAll and
ITenantProvider.findAll require page and limit but the actual interfaces declare
them as optional; update the doc to match the real signatures: change
IUserProvider.findAll to use options?: { page?: number; limit?: number; search?:
string; tenantId?: string } and change ITenantProvider.findAll to options?: {
page?: number; limit?: number } (i.e., mark page/limit optional and include
tenantId in the IUserProvider signature) so the documentation aligns with the
IUserProvider and ITenantProvider interfaces.
- SystemAdminController: Add createSystemInvitation tests, clarify deleteUser logic, add TODO - DrizzleTenantAdapter: Handle slug unique errors, check existence on delete - DrizzleUserAdapter: Consolidate findAll filters - UsersController: Return full pagination metadata
pramodnarayana
marked this pull request as draft
January 24, 2026 11:29
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/modules/identity/users/users.controller.ts (1)
46-59: Return a consistent paginated shape for all branches.
Returning[]whentenantIdis missing produces a different response type than the{ data, total }envelope, which can break clients and tests that expect a stable shape. Return an empty envelope instead (and update the two “no organizationId/user” tests to match).🛠️ Proposed fix
- return []; + return { data: [], total: 0 };
pramodnarayana
marked this pull request as ready for review
January 24, 2026 11:34
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/identity/src/adapters/drizzle-tenant.adapter.ts`:
- Around line 86-103: The update method in drizzle-tenant.adapter.ts (async
update) needs to handle Postgres unique-constraint violations when changing
schema.organization.slug: wrap the DB update/returning call in a try/catch,
detect Postgres error code "23505" (unique violation) or the DB client's
unique-constraint error shape, and throw a clear domain error like "Tenant slug
already exists" instead of propagating the raw DB error; keep existing behavior
for other errors and continue to return mapTenant(updated) on success.
In `@packages/identity/src/adapters/drizzle-user.adapter.ts`:
- Around line 97-103: The code builds a filters array (e.g., in
drizzle-user.adapter.ts) and then calls and(...filters) which can be invoked
with zero args; explicitly guard this by only passing a WHERE clause when
filters.length > 0 — replace direct and(...filters) usage with a conditional
like filters.length ? and(...filters) : undefined (or omit the where call)
wherever filters are used (e.g., the blocks that construct the query using
filters and ilike(schema.user.email, `%${options.search}%`)), so empty filters
produce no WHERE clause without relying on implicit behavior.
- DrizzleTenantAdapter: Handle unique slug violation in update - DrizzleUserAdapter: Guard and(...filters) calls
pramodnarayana
marked this pull request as draft
January 24, 2026 11:48
pramodnarayana
marked this pull request as ready for review
January 24, 2026 11:49
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/identity/src/adapters/drizzle-tenant.adapter.ts`:
- Around line 181-197: Make the filter intent explicit by replacing the
ambiguous where(and(...filters)) usage with a conditional: only call
.where(and(...filters)) when filters.length > 0, otherwise omit the .where call
(or pass a true/no-op condition). Update the query building around filters,
and(...filters), and the two places used (the select query and the count query
against schema.organization) so both data and count use the same explicit
conditional application of filters.
In `@packages/identity/src/adapters/drizzle-user.adapter.ts`:
- Around line 93-96: The pagination calculation uses page, limit and offset and
doesn't guard against zero/negative values, so clamp page to at least 1 and
limit to a sensible minimum (e.g., 1) before computing offset to avoid
negative/invalid offsets; update the block that defines page, limit, and offset
in drizzle-user.adapter.ts (variables page, limit, offset) to coerce/validate
inputs (e.g., page = Math.max(1, Number(options?.page) || 1); limit =
Math.max(1, Number(options?.limit) || 10); offset = (page - 1) * limit) and
optionally enforce an upper bound on limit if needed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
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/interfaces/user-provider.interface.ts (1)
3-10: Prevent privilege escalation viasystemRoleon CreateUserInput.If this input is used in self‑signup flows, callers could set elevated system roles unless explicitly stripped or validated. Consider separating admin-only input or enforcing an allowlist upstream.
🤖 Fix all issues with AI agents
In `@docs/draft/identity_module_design_doc.md`:
- Around line 11-16: Fix the spelling typo in the Identity Module
responsibilities list: change the phrase "Credental Management" to "Credential
Management" in the documentation where the Identity Module (`@nexiom/identity`,
labeled "Identity Module" / "Security Kernel") enumerates its responsibilities
so it reads "Credential Management: How do we talk to external apps? (Layer 5
Support)".
In `@docs/refactor/modules/identity/task/01d_retire_legacy_tenants_service.md`:
- Around line 1-48: Add a rollback strategy to the TenantsService removal task
by documenting a concrete fallback plan: specify a feature-flag approach to
toggle between TenantsService and ITenantProvider (TENANT_PROVIDER) during
rollout, a Git rollback/playbook to revert the commit that deletes
TenantsService and changes to tenants.controller/TenantsController and
auth.service/AuthService, and monitoring/alerting for failures in
DrizzleTenantAdapter methods (provisionTenantForUser and findAllForUser) and
tenant operations; include steps to re-enable the legacy TenantsService export
in tenants.module if rollback is needed and list tests/commands to verify the
rollback (e.g., run unit/integration tests and smoke checks).
- Around line 32-35: The review warns that only provisionTenantForUser and
findAllForUser are being checked but the adapter must implement the full public
API of TenantsService; audit TenantsService's public methods, then update
DrizzleTenantAdapter and ITenantProvider to implement/forward each method (not
just provisionTenantForUser and findAllForUser), e.g.,
createTenant/createOrganization/deleteTenant/getTenantById/listTenants/etc.
Ensure method signatures match TenantsService, add missing implementations or
throw explicit NotImplemented errors, run TypeScript compile/type checks to
catch mismatches, and add unit tests that exercise all public TenantsService
methods to prevent regressions.
- Around line 37-48: Extend the verification plan to include searches for any
remaining imports, references, or documentation and to validate runtime and CI:
run repository-wide searches for "TenantsService", "tenants.service",
"DRIZZLE_DB", and "tenants.module" to catch import statements and comments;
check docs/README, API docs, and migration files for references to
TenantsService; run type-check/build and CI pipelines (pnpm build / pnpm test
all) and run integration/e2e tests to catch runtime failures; ensure no tests
import TenantsService by grepping tests and update or remove any fixtures or
mocks referencing TenantsService or tenants.service; and finally confirm exports
removed from the TenantsModule (tenants.module.ts) do not break DI or consumers.
- Around line 20-30: Add a preliminary "consumer discovery" step: search the
repository for all references to TenantsService and its methods (e.g.,
TenantsService, tenantService.*, TenantsService.findAll, etc.), collate every
consumer file (not just
apps/api/src/modules/identity/tenants/tenants.controller.ts and
apps/api/src/modules/identity/auth/auth.service.ts), and add them to the
refactor plan; for each discovered consumer, replace TenantsService injection
with ITenantProvider (token TENANT_PROVIDER) and update calls to use the
provider API (e.g., tenantProvider.findAllForUser) then run unit/integration
tests and update typings if any provider method signatures differ.
In `@packages/identity/src/adapters/drizzle-tenant.adapter.ts`:
- Around line 172-179: The findAll method computes offset from options?.page and
options?.limit but does not validate negative or zero values; ensure page and
limit are sanitized inside findAll (e.g., use Math.max or explicit checks on
options?.page and options?.limit) so page < 1 or limit < 1 are clamped to
sensible defaults (page = 1, limit = 10) and compute offset = (page - 1) * limit
with offset guaranteed >= 0; update the logic around the page/limit/offset
variables in findAll to enforce these constraints and use the sanitized values
in the subsequent query.
In `@packages/identity/src/interfaces/user-provider.interface.ts`:
- Around line 28-44: The findAll signature doesn't support filtering by
systemRole while count does, causing pagination mismatches; update the
UserProvider interface by adding an optional systemRole?: string field to the
findAll(options?: { page?: number; limit?: number; search?: string; tenantId?:
string; systemRole?: string; }) parameter and ensure any implementing adapters
(methods named findAll in adapter classes) accept and forward this option to
their queries so list results and count use the same filters.
Comment on lines
+11
to
+16
| The **Identity Module** (`@nexiom/identity`) is the "Security Kernel" of the Nexiom Platform. It is NOT just a user table; it is the **Authority** for: | ||
|
|
||
| 1. **Authentication:** Who are you? (Users/Machines) | ||
| 2. **Multitenancy:** Which data silo do you own? (Tenant Resolution) | ||
| 3. **Authorization:** What can you click? (CBAC: Capability-Based Access Control) | ||
| 4. **Credental Management:** How do we talk to external apps? (Layer 5 Support) |
Contributor
There was a problem hiding this comment.
Fix typo: “Credental” → “Credential”.
Minor spelling issue in the responsibilities list.
✏️ Proposed fix
-4. **Credental Management:** How do we talk to external apps? (Layer 5 Support)
+4. **Credential Management:** How do we talk to external apps? (Layer 5 Support)📝 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
| The **Identity Module** (`@nexiom/identity`) is the "Security Kernel" of the Nexiom Platform. It is NOT just a user table; it is the **Authority** for: | |
| 1. **Authentication:** Who are you? (Users/Machines) | |
| 2. **Multitenancy:** Which data silo do you own? (Tenant Resolution) | |
| 3. **Authorization:** What can you click? (CBAC: Capability-Based Access Control) | |
| 4. **Credental Management:** How do we talk to external apps? (Layer 5 Support) | |
| The **Identity Module** (`@nexiom/identity`) is the "Security Kernel" of the Nexiom Platform. It is NOT just a user table; it is the **Authority** for: | |
| 1. **Authentication:** Who are you? (Users/Machines) | |
| 2. **Multitenancy:** Which data silo do you own? (Tenant Resolution) | |
| 3. **Authorization:** What can you click? (CBAC: Capability-Based Access Control) | |
| 4. **Credential Management:** How do we talk to external apps? (Layer 5 Support) |
🧰 Tools
🪛 LanguageTool
[grammar] ~15-~15: Ensure spelling is correct
Context: ... (CBAC: Capability-Based Access Control) 4. Credental Management: How do we talk to externa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In `@docs/draft/identity_module_design_doc.md` around lines 11 - 16, Fix the
spelling typo in the Identity Module responsibilities list: change the phrase
"Credental Management" to "Credential Management" in the documentation where the
Identity Module (`@nexiom/identity`, labeled "Identity Module" / "Security
Kernel") enumerates its responsibilities so it reads "Credential Management: How
do we talk to external apps? (Layer 5 Support)".
Comment on lines
+1
to
+48
| # Task 01d: Retire Legacy TenantsService | ||
|
|
||
| **Priority:** HIGH | ||
| **Estimated Time:** 2 hours | ||
| **Assignee:** Coder | ||
| **Status:** Ready to Start | ||
|
|
||
| --- | ||
|
|
||
| ## Objective | ||
|
|
||
| The `TenantsService` class in `apps/api/src/modules/identity/tenants/tenants.service.ts` is a legacy artifact that violates the Strict Layer Enforcement by injecting `DRIZZLE_DB` directly. | ||
|
|
||
| It duplicates logic (e.g., `provisionTenantForUser`) that is now correctly implemented in the `DrizzleTenantAdapter`. | ||
|
|
||
| **Goal:** Remove `TenantsService` entirely and refactor its consumers to use `ITenantProvider`. | ||
|
|
||
| --- | ||
|
|
||
| ## Implementation Steps | ||
|
|
||
| ### 1. Refactor Consumers | ||
|
|
||
| - [ ] **Target:** `apps/api/src/modules/identity/tenants/tenants.controller.ts` | ||
| - Inject `ITenantProvider` (token: `TENANT_PROVIDER`) instead of `TenantsService`. | ||
| - Update methods to call provider methods directly. | ||
| - [ ] **Target:** `apps/api/src/modules/identity/auth/auth.service.ts` | ||
| - Inject `ITenantProvider` instead of `TenantsService`. | ||
| - Update `getEnrichedSession` to use `tenantProvider.findAllForUser`. | ||
|
|
||
| ### 2. Verify Adapter Capabilities | ||
|
|
||
| - [ ] Ensure `DrizzleTenantAdapter` implements `provisionTenantForUser` correctly (it generates a slug and creates the org/member). | ||
| - [ ] Ensure `findAllForUser` is implemented. | ||
|
|
||
| ### 3. Cleanup | ||
|
|
||
| - [ ] **Delete:** `apps/api/src/modules/identity/tenants/tenants.service.ts` | ||
| - [ ] **Delete:** `apps/api/src/modules/identity/tenants/tenants.service.spec.ts` | ||
| - [ ] **Remove Export:** Update `apps/api/src/modules/identity/tenants/tenants.module.ts` to stop exporting `TenantsService`. | ||
|
|
||
| --- | ||
|
|
||
| ## Verification Plan | ||
|
|
||
| - [ ] `grep -r "TenantsService" apps/api` should return 0 results (except maybe in migration files if any). | ||
| - [ ] `pnpm test apps/api` should pass. | ||
| - [ ] `grep -r "DRIZZLE_DB" apps/api/src/modules/identity` should return 0 results. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a rollback strategy.
While the documentation outlines the migration path well, there's no mention of a rollback plan if issues are discovered post-deployment. For a high-priority refactoring that touches multiple modules, consider documenting:
- Feature flag approach (if applicable) to toggle between old and new implementations during transition
- Git rollback procedure if critical issues emerge
- Monitoring/alerting to watch for tenant operation failures post-migration
🤖 Prompt for AI Agents
In `@docs/refactor/modules/identity/task/01d_retire_legacy_tenants_service.md`
around lines 1 - 48, Add a rollback strategy to the TenantsService removal task
by documenting a concrete fallback plan: specify a feature-flag approach to
toggle between TenantsService and ITenantProvider (TENANT_PROVIDER) during
rollout, a Git rollback/playbook to revert the commit that deletes
TenantsService and changes to tenants.controller/TenantsController and
auth.service/AuthService, and monitoring/alerting for failures in
DrizzleTenantAdapter methods (provisionTenantForUser and findAllForUser) and
tenant operations; include steps to re-enable the legacy TenantsService export
in tenants.module if rollback is needed and list tests/commands to verify the
rollback (e.g., run unit/integration tests and smoke checks).
Comment on lines
+20
to
+30
| ## Implementation Steps | ||
|
|
||
| ### 1. Refactor Consumers | ||
|
|
||
| - [ ] **Target:** `apps/api/src/modules/identity/tenants/tenants.controller.ts` | ||
| - Inject `ITenantProvider` (token: `TENANT_PROVIDER`) instead of `TenantsService`. | ||
| - Update methods to call provider methods directly. | ||
| - [ ] **Target:** `apps/api/src/modules/identity/auth/auth.service.ts` | ||
| - Inject `ITenantProvider` instead of `TenantsService`. | ||
| - Update `getEnrichedSession` to use `tenantProvider.findAllForUser`. | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add a consumer discovery step before refactoring.
The implementation steps list only two specific consumers (tenants.controller.ts and auth.service.ts), but there may be additional consumers throughout the codebase. Based on the AI summary, this refactoring impacts multiple modules.
📝 Suggested addition: consumer discovery step
Add a preliminary step before refactoring:
## Implementation Steps
+
+### 0. Discover All Consumers
+
+- [ ] Run `grep -r "TenantsService" apps/api/src --include="*.ts" --exclude="*.spec.ts"` to identify all consumers.
+- [ ] Document each consumer and its usage patterns.
+- [ ] Verify that all identified usages can be replaced with ITenantProvider methods.
### 1. Refactor ConsumersThis ensures no consumers are missed during the migration.
🤖 Prompt for AI Agents
In `@docs/refactor/modules/identity/task/01d_retire_legacy_tenants_service.md`
around lines 20 - 30, Add a preliminary "consumer discovery" step: search the
repository for all references to TenantsService and its methods (e.g.,
TenantsService, tenantService.*, TenantsService.findAll, etc.), collate every
consumer file (not just
apps/api/src/modules/identity/tenants/tenants.controller.ts and
apps/api/src/modules/identity/auth/auth.service.ts), and add them to the
refactor plan; for each discovered consumer, replace TenantsService injection
with ITenantProvider (token TENANT_PROVIDER) and update calls to use the
provider API (e.g., tenantProvider.findAllForUser) then run unit/integration
tests and update typings if any provider method signatures differ.
Comment on lines
+32
to
+35
|
|
||
| - [ ] Ensure `DrizzleTenantAdapter` implements `provisionTenantForUser` correctly (it generates a slug and creates the org/member). | ||
| - [ ] Ensure `findAllForUser` is implemented. | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Verify complete method coverage, not just specific methods.
The verification only checks two specific methods (provisionTenantForUser and findAllForUser), but TenantsService likely exposes additional methods. Before deletion, ensure that ITenantProvider and DrizzleTenantAdapter cover ALL public methods from TenantsService that are actively used.
📝 Suggested comprehensive verification
### 2. Verify Adapter Capabilities
+- [ ] List all public methods in TenantsService.
+- [ ] Verify each method has an equivalent in ITenantProvider/DrizzleTenantAdapter.
+- [ ] Document any methods that are truly unused and can be safely dropped.
- [ ] Ensure `DrizzleTenantAdapter` implements `provisionTenantForUser` correctly (it generates a slug and creates the org/member).
- [ ] Ensure `findAllForUser` is implemented.🤖 Prompt for AI Agents
In `@docs/refactor/modules/identity/task/01d_retire_legacy_tenants_service.md`
around lines 32 - 35, The review warns that only provisionTenantForUser and
findAllForUser are being checked but the adapter must implement the full public
API of TenantsService; audit TenantsService's public methods, then update
DrizzleTenantAdapter and ITenantProvider to implement/forward each method (not
just provisionTenantForUser and findAllForUser), e.g.,
createTenant/createOrganization/deleteTenant/getTenantById/listTenants/etc.
Ensure method signatures match TenantsService, add missing implementations or
throw explicit NotImplemented errors, run TypeScript compile/type checks to
catch mismatches, and add unit tests that exercise all public TenantsService
methods to prevent regressions.
Comment on lines
+37
to
+48
|
|
||
| - [ ] **Delete:** `apps/api/src/modules/identity/tenants/tenants.service.ts` | ||
| - [ ] **Delete:** `apps/api/src/modules/identity/tenants/tenants.service.spec.ts` | ||
| - [ ] **Remove Export:** Update `apps/api/src/modules/identity/tenants/tenants.module.ts` to stop exporting `TenantsService`. | ||
|
|
||
| --- | ||
|
|
||
| ## Verification Plan | ||
|
|
||
| - [ ] `grep -r "TenantsService" apps/api` should return 0 results (except maybe in migration files if any). | ||
| - [ ] `pnpm test apps/api` should pass. | ||
| - [ ] `grep -r "DRIZZLE_DB" apps/api/src/modules/identity` should return 0 results. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen the verification plan with additional checks.
The current verification plan is a good start but could be more comprehensive to ensure a clean migration.
📋 Suggested verification improvements
## Verification Plan
-- [ ] `grep -r "TenantsService" apps/api` should return 0 results (except maybe in migration files if any).
+- [ ] `grep -r "TenantsService" apps/api/src --include="*.ts"` should return 0 results.
+- [ ] `grep -r "from.*tenants\.service" apps/api/src` should return 0 results (verify imports removed).
+- [ ] Check documentation: `grep -r "TenantsService" docs/` and update any references.
- [ ] `pnpm test apps/api` should pass.
+- [ ] Verify test coverage: ensure all critical tenant operations have tests using ITenantProvider.
- [ ] `grep -r "DRIZZLE_DB" apps/api/src/modules/identity` should return 0 results.
+- [ ] Manual smoke test: verify tenant creation, user-tenant association, and tenant listing work as expected.These additional checks help catch:
- Import statements that might have been missed
- Documentation that references the old service
- Test coverage gaps
- Runtime issues not caught by unit tests
📝 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
| - [ ] **Delete:** `apps/api/src/modules/identity/tenants/tenants.service.ts` | |
| - [ ] **Delete:** `apps/api/src/modules/identity/tenants/tenants.service.spec.ts` | |
| - [ ] **Remove Export:** Update `apps/api/src/modules/identity/tenants/tenants.module.ts` to stop exporting `TenantsService`. | |
| --- | |
| ## Verification Plan | |
| - [ ] `grep -r "TenantsService" apps/api` should return 0 results (except maybe in migration files if any). | |
| - [ ] `pnpm test apps/api` should pass. | |
| - [ ] `grep -r "DRIZZLE_DB" apps/api/src/modules/identity` should return 0 results. | |
| - [ ] **Delete:** `apps/api/src/modules/identity/tenants/tenants.service.ts` | |
| - [ ] **Delete:** `apps/api/src/modules/identity/tenants/tenants.service.spec.ts` | |
| - [ ] **Remove Export:** Update `apps/api/src/modules/identity/tenants/tenants.module.ts` to stop exporting `TenantsService`. | |
| --- | |
| ## Verification Plan | |
| - [ ] `grep -r "TenantsService" apps/api/src --include="*.ts"` should return 0 results. | |
| - [ ] `grep -r "from.*tenants\.service" apps/api/src` should return 0 results (verify imports removed). | |
| - [ ] Check documentation: `grep -r "TenantsService" docs/` and update any references. | |
| - [ ] `pnpm test apps/api` should pass. | |
| - [ ] Verify test coverage: ensure all critical tenant operations have tests using ITenantProvider. | |
| - [ ] `grep -r "DRIZZLE_DB" apps/api/src/modules/identity` should return 0 results. | |
| - [ ] Manual smoke test: verify tenant creation, user-tenant association, and tenant listing work as expected. |
🤖 Prompt for AI Agents
In `@docs/refactor/modules/identity/task/01d_retire_legacy_tenants_service.md`
around lines 37 - 48, Extend the verification plan to include searches for any
remaining imports, references, or documentation and to validate runtime and CI:
run repository-wide searches for "TenantsService", "tenants.service",
"DRIZZLE_DB", and "tenants.module" to catch import statements and comments;
check docs/README, API docs, and migration files for references to
TenantsService; run type-check/build and CI pipelines (pnpm build / pnpm test
all) and run integration/e2e tests to catch runtime failures; ensure no tests
import TenantsService by grepping tests and update or remove any fixtures or
mocks referencing TenantsService or tenants.service; and finally confirm exports
removed from the TenantsModule (tenants.module.ts) do not break DI or consumers.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/identity/src/adapters/drizzle-user.adapter.ts`:
- Around line 97-165: The filter construction in findAll (filters array built
from options.search, options.systemRole, options.tenantId and use of
schema.user/schema.member) is duplicated for the dataQuery and the count query;
extract that logic into a helper (e.g., buildUserFilters(options) or
buildUserQueryParts) that returns the where clause (and a flag indicating
whether a member join is required) so both the dataQuery and count query reuse
the same filters and join decision; update findAll to call this helper to build
the where condition and to decide whether to innerJoin schema.member (used in
the tenant-scoped branch) so mapUser, dataQuery, and countResult logic remain
unchanged but no filter construction is duplicated.
pramodnarayana
marked this pull request as draft
January 25, 2026 06:46
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/identity/src/adapters/drizzle-user.adapter.ts`:
- Around line 24-28: The block that handles input.systemRole should return the
updated user directly from the existing update method instead of calling
findById afterwards; replace the pattern that calls await this.update(user.id, {
systemRole: input.systemRole }); then awaits this.findById(user.id) with a
single return of the update result from this.update(user.id, { systemRole:
input.systemRole }) to avoid redundant DB lookup and the risk of silently
falling back to the stale user when findById returns null; update the branch in
the adapter where input.systemRole is handled (referencing user.id,
input.systemRole, this.update and this.findById) so it returns the update result
immediately.
pramodnarayana
marked this pull request as ready for review
January 25, 2026 07:13
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/identity/src/adapters/drizzle-user.adapter.ts`:
- Around line 18-28: The create method can leave a user without systemRole if
authProvider.createUser succeeds but this.update fails; wrap the post-create
update in a try/catch: call const user = await
this.authProvider.createUser(input) as now, then if (input.systemRole) attempt
await this.update(user.id, { systemRole: input.systemRole }) inside try, and on
failure attempt a compensating cleanup by calling this.delete(user.id) (or
equivalent teardown) while preserving and rethrowing the original update error;
alternatively, if authProvider.createUser supports custom fields, prefer passing
systemRole through to authProvider.createUser to make it atomic (refer to
create, authProvider.createUser, update, and delete).
- Around line 102-104: page and limit are being parsed with Number(), which
allows floats and can produce fractional offsets; coerce them to integers before
computing offset by converting the parsed values with an integer truncation
(e.g., Math.floor or parseInt) and then apply Math.max(1, ...) as you already do
for page/limit; compute offset = (pageInt - 1) * limitInt and optionally enforce
a sane maxLimit cap on limitInt to avoid huge queries. Use the symbols page,
limit, and offset in drizzle-user.adapter.ts (convert Number(options?.page) and
Number(options?.limit) to integer-truncated variants).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@packages/identity/src/adapters/drizzle-tenant.adapter.ts`:
- Around line 59-84: createTenant may insert an empty/whitespace slug which
breaks mapTenant/findBySlug because mapTenant falls back to id when slug is
falsy; before inserting in createTenant, trim the incoming input.slug and
validate it is non-empty (reject/throw a descriptive error like "Tenant slug is
required" or "Invalid slug") and normalize it (e.g., lower-case) as your slug
policy dictates, then persist the normalized value; apply the same
trimming/validation logic in the update method when a slug is provided so both
createTenant and update follow the same rule and prevent storing
empty/whitespace slugs.
- Around line 113-136: Replace the separate existence select in the delete(id:
string) method with a single DELETE ... RETURNING on schema.organization: remove
the initial tx.select([...]) check, perform
tx.delete(schema.organization).where(eq(schema.organization.id, id)).returning({
id: schema.organization.id }), and if the returned array is empty throw the
"Tenant not found" error; keep the subsequent deletes of schema.member and
schema.invitation (or run them before the organization delete if FK constraints
require it) so the entire operation remains inside the same transaction.
- Around line 172-203: The pagination ordering in findAll is only sorting by
createdAt which can produce unstable paging; update the query that builds data
to add a deterministic tie-breaker by also ordering on id. Specifically, modify
the orderBy call that currently uses desc(schema.organization.createdAt) to
include a secondary sort desc(schema.organization.id) (or asc depending on
desired tie-break direction) so both the data query in findAll and any other
paginated query use the same stable ordering; ensure schema.organization.id is
selected so the mapper (mapTenant) still receives id.
In `@packages/identity/src/adapters/drizzle-user.adapter.ts`:
- Around line 18-35: The compensating delete in create(input: CreateUserInput)
should be best‑effort so a delete failure doesn't replace the original update
error: inside the catch block for update(user.id, { systemRole }), capture the
original error, attempt await this.delete(user.id) inside its own try/catch (log
or ignore any delete error), and then rethrow the original error (the one from
update). Keep references to authProvider.createUser, update, delete,
input.systemRole and ensure the original error is the one propagated to callers.
- Around line 101-110: The findAll method currently allows unbounded limit
values; add a defensive cap (e.g., const MAX_LIMIT = 100) and clamp the computed
limit using Math.min(limit, MAX_LIMIT) before calculating offset and running the
query in drizzle-user.adapter.ts (function findAll) and apply the same clamp to
the analogous method in the tenant adapter; ensure the cap value is a named
constant (MAX_LIMIT) so it’s easy to adjust and the rest of the logic (offset,
query) uses the clamped limit.
Comment on lines
+59
to
+84
| async createTenant(input: { | ||
| name: string; | ||
| slug: string; | ||
| logo?: string | null; | ||
| }): Promise<TenantInterface> { | ||
| try { | ||
| const [org] = await this.db | ||
| .insert(schema.organization) | ||
| .values({ | ||
| id: uuidv4(), | ||
| name: input.name, | ||
| slug: input.slug, | ||
| logo: input.logo, | ||
| createdAt: new Date(), | ||
| status: "active", | ||
| }) | ||
| .returning(); | ||
| return this.mapTenant(org); | ||
| } catch (error: any) { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call | ||
| if (error.code === "23505" && error.detail?.includes("slug")) { | ||
| throw new Error("Tenant slug already exists"); | ||
| } | ||
| throw error; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against empty/whitespace slugs to avoid inconsistent reads.
mapTenant falls back to id when slug is falsy, so storing an empty string can make findBySlug unusable for that tenant. Consider trimming/validating the slug (and reusing the same rule in update when slug is provided).
💡 Proposed fix
async createTenant(input: {
name: string;
slug: string;
logo?: string | null;
}): Promise<TenantInterface> {
+ const slug = input.slug.trim();
+ if (!slug) {
+ throw new Error("Tenant slug is required");
+ }
try {
const [org] = await this.db
.insert(schema.organization)
.values({
id: uuidv4(),
name: input.name,
- slug: input.slug,
+ slug,
logo: input.logo,
createdAt: new Date(),
status: "active",
})
.returning();🤖 Prompt for AI Agents
In `@packages/identity/src/adapters/drizzle-tenant.adapter.ts` around lines 59 -
84, createTenant may insert an empty/whitespace slug which breaks
mapTenant/findBySlug because mapTenant falls back to id when slug is falsy;
before inserting in createTenant, trim the incoming input.slug and validate it
is non-empty (reject/throw a descriptive error like "Tenant slug is required" or
"Invalid slug") and normalize it (e.g., lower-case) as your slug policy
dictates, then persist the normalized value; apply the same trimming/validation
logic in the update method when a slug is provided so both createTenant and
update follow the same rule and prevent storing empty/whitespace slugs.
Comment on lines
+101
to
+110
| async findAll(options?: { | ||
| page?: number; | ||
| limit?: number; | ||
| search?: string; | ||
| tenantId?: string; | ||
| systemRole?: string; | ||
| }): Promise<{ data: UserInterface[]; total: number }> { | ||
| const page = Math.max(1, Math.floor(Number(options?.page) || 1)); | ||
| const limit = Math.max(1, Math.floor(Number(options?.limit) || 10)); | ||
| const offset = (page - 1) * limit; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the actual file and the findAll method
echo "=== Checking the drizzle-user.adapter.ts file ==="
cat -n packages/identity/src/adapters/drizzle-user.adapter.ts | sed -n '95,120p'
# Search for where findAll is called (upstream usage)
echo -e "\n=== Searching for findAll calls ==="
rg -n "findAll" packages/identity apps/api --type=ts -A2 -B2
# Search for limit/page validation patterns
echo -e "\n=== Searching for limit/page validation ==="
rg -n "limit|page" packages/identity apps/api --type=ts | grep -i "valid\|max\|cap\|constrain" -A2 -B2
# Check for MAX_LIMIT constants
echo -e "\n=== Checking for existing MAX_LIMIT constants ==="
rg -n "MAX_LIMIT|MAX.*LIMIT" packages/identity apps/api --type=tsRepository: pramodnarayana/nexiom
Length of output: 22146
🏁 Script executed:
#!/bin/bash
# Find where MAX_PAGE_SIZE is defined
echo "=== Searching for MAX_PAGE_SIZE definition ==="
rg -n "MAX_PAGE_SIZE\|const.*MAX" apps/api/src/modules/identity/system-admin --type=ts -B2 -A2
# Get more context from system-admin controller around the limit assignment
echo -e "\n=== System-admin controller listUsers method ==="
sed -n '190,215p' apps/api/src/modules/identity/system-admin/system-admin.controller.ts
echo -e "\n=== System-admin controller listTenants method ==="
sed -n '282,310p' apps/api/src/modules/identity/system-admin/system-admin.controller.ts
# Check if there are other findAll calls that might bypass the cap
echo -e "\n=== All findAll calls in codebase ==="
rg -n "\.findAll\(" --type=ts | grep -v "spec\|test"Repository: pramodnarayana/nexiom
Length of output: 1943
Add a limit cap to the findAll method to prevent unbounded queries.
The adapter accepts potentially unlimited limit values. While the system-admin controller applies a cap (100) upstream, the adapter should enforce its own defensive limit. Both the user and tenant adapters have this exposure.
♻️ Proposed change
const page = Math.max(1, Math.floor(Number(options?.page) || 1));
- const limit = Math.max(1, Math.floor(Number(options?.limit) || 10));
+ const MAX_LIMIT = 100;
+ const limit = Math.min(
+ MAX_LIMIT,
+ Math.max(1, Math.floor(Number(options?.limit) || 10)),
+ );📝 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
| async findAll(options?: { | |
| page?: number; | |
| limit?: number; | |
| search?: string; | |
| tenantId?: string; | |
| systemRole?: string; | |
| }): Promise<{ data: UserInterface[]; total: number }> { | |
| const page = Math.max(1, Math.floor(Number(options?.page) || 1)); | |
| const limit = Math.max(1, Math.floor(Number(options?.limit) || 10)); | |
| const offset = (page - 1) * limit; | |
| async findAll(options?: { | |
| page?: number; | |
| limit?: number; | |
| search?: string; | |
| tenantId?: string; | |
| systemRole?: string; | |
| }): Promise<{ data: UserInterface[]; total: number }> { | |
| const page = Math.max(1, Math.floor(Number(options?.page) || 1)); | |
| const MAX_LIMIT = 100; | |
| const limit = Math.min( | |
| MAX_LIMIT, | |
| Math.max(1, Math.floor(Number(options?.limit) || 10)), | |
| ); | |
| const offset = (page - 1) * limit; |
🤖 Prompt for AI Agents
In `@packages/identity/src/adapters/drizzle-user.adapter.ts` around lines 101 -
110, The findAll method currently allows unbounded limit values; add a defensive
cap (e.g., const MAX_LIMIT = 100) and clamp the computed limit using
Math.min(limit, MAX_LIMIT) before calculating offset and running the query in
drizzle-user.adapter.ts (function findAll) and apply the same clamp to the
analogous method in the tenant adapter; ensure the cap value is a named constant
(MAX_LIMIT) so it’s easy to adjust and the rest of the logic (offset, query)
uses the clamped limit.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/identity/src/adapters/drizzle-tenant.adapter.ts`:
- Around line 92-118: The update method has a TOCTOU race and allows
empty-string slugs; modify TenantAdapter.update so instead of relying on the
pre-check via findBySlug alone, perform the update inside a try/catch around the
db.update(schema.organization) call (as done in createTenant) and translate
unique constraint DB errors into the same "Tenant with this slug already exists"
error to make slug uniqueness atomic, and also ensure you don't write an empty
slug by treating input.slug === "" as invalid or by omitting slug from the
update payload (i.e., only include slug when input.slug is a non-empty string);
keep the existing metadata handling and return this.mapTenant(updated) as
before.
…ion in tenant adapter
This was referenced Jan 27, 2026
This was referenced Feb 4, 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.
Summary by CodeRabbit
Refactor
Tests
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.