Repository navigation
refactor(auth): complete PBAC implementation with platform user support - #37
Merged
Merged
Conversation
pramodnarayana
marked this pull request as draft
January 30, 2026 05:17
Contributor
📝 WalkthroughWalkthroughSwaps IdentityModule to async registration with DI-backed DB/email provisioning, migrates role-based auth to permissions (PBAC) with session enrichment during login, adds password-reset support and frontend Forgot/Reset pages+tests, and updates frontend types, routes, and permission checks. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client (Browser)
participant Web as Web App (LoginPage)
participant API as AuthService (API)
participant Identity as IdentityModule / Auth Provider
participant Permission as PermissionProvider
participant DB as Database
Browser->>Web: submit credentials
Web->>API: POST /auth/login
API->>Identity: validate credentials / create session
API->>DB: fetch tenant/context (if needed)
API->>Permission: getPermissions(userId, tenant?)
Permission-->>API: permissions[]
API->>API: merge platform + tenant permissions into session.user.permissions
API-->>Web: return enriched session (user.permissions[])
Web-->>Browser: redirect based on permissions (AppRoutes)
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 |
pramodnarayana
marked this pull request as ready for review
January 30, 2026 05:18
pramodnarayana
marked this pull request as draft
January 30, 2026 05:18
![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: 12
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/auth/auth.service.ts (1)
84-110:⚠️ Potential issue | 🟡 MinorPrevent duplicate platform permissions on the error path.
If
permissionProvider.getPermissionsthrows,'*'or'admin_dashboard:view'can be appended twice. A small guard avoids duplicates.♻️ Suggested fix (dedupe platform permissions)
- if (user.systemRole === 'platform_admin') { - permissions.push('*'); - } else if (user.systemRole === 'platform_user') { - // Platform Users get access to the admin dashboard (but not everything) - permissions.push('admin_dashboard:view'); - } + if (user.systemRole === 'platform_admin') { + permissions.push('*'); + } else if (user.systemRole === 'platform_user') { + // Platform Users get access to the admin dashboard (but not everything) + permissions.push('admin_dashboard:view'); + } @@ - if (user.systemRole === 'platform_admin') { - permissions.push('*'); - } else if (user.systemRole === 'platform_user') { - permissions.push('admin_dashboard:view'); - } + if (user.systemRole === 'platform_admin' && !permissions.includes('*')) { + permissions.push('*'); + } else if ( + user.systemRole === 'platform_user' && + !permissions.includes('admin_dashboard:view') + ) { + permissions.push('admin_dashboard:view'); + }
🤖 Fix all issues with AI agents
In `@apps/api/src/app.module.ts`:
- Around line 25-26: The module default betterAuthUrl now falls back to
"http://localhost:3000/api/auth" but the Docker env var BETTER_AUTH_URL and the
test value in invitations.e2e-spec.ts are inconsistent; update the
Docker-compose BETTER_AUTH_URL to "http://localhost:3000/api/auth" (or
explicitly set the intended 3001 URL if your Docker networking requires that
port) and update the test's BETTER_AUTH_URL value to include the trailing
"/auth" so the integration test uses the full auth endpoint; ensure references
to BETTER_AUTH_URL and betterAuthUrl remain consistent across config, docker
env, and tests.
In `@apps/api/src/modules/identity/auth/auth.controller.ts`:
- Around line 206-209: The current BetterAuth catch‑all handler logs the full
request URL (this.logger.log(`BetterAuth Request: ${req.method} ${req.url}`)),
which can expose OAuth/magic‑link tokens; change the logging to avoid query
strings and lower verbosity by logging only req.path (or construct a URL without
req.query) and use a debug/verbose log level (e.g., this.logger.debug) instead
of logger.log/info; update the betterAuth method to emit a sanitized message
referencing req.method and req.path and ensure no query or raw url is included.
In `@apps/api/src/modules/identity/auth/auth.service.ts`:
- Around line 27-44: The login method currently calls
getEnrichedSession(result.session.token) and will let any thrown error abort the
login; update auth.service.ts so login wraps the enrichment call in a try/catch:
call getEnrichedSession inside try, on success merge as before into the returned
AuthResult (preserving result and overwriting user with enriched.user), and on
any error or if enriched is falsy, log the error (or debug) and return the
original result fallback; ensure you reference the existing login function,
getEnrichedSession, result.session.token and AuthResult shape when applying the
change.
In `@apps/api/vitest.config.mts`:
- Around line 3-4: The import of the Node.js core module 'url' is missing the
`node:` prefix; update the import that brings in fileURLToPath so it uses
`node:url` (i.e., change the module specifier for the fileURLToPath import) to
match the existing `node:path` usage for consistent core module imports.
In `@apps/web/src/App.tsx`:
- Around line 25-26: Replace the hard-coded route paths with the centralized
AppRoutes constants: change the Route entries using path="/forgot-password" and
path="/reset-password" to use AppRoutes.FORGOT_PASSWORD and
AppRoutes.RESET_PASSWORD respectively, keep the element props as
ForgotPasswordPage and ResetPasswordPage, and add the AppRoutes import (e.g.,
import { AppRoutes } from '...') at the top of the file so the routes use the
shared constants.
In `@apps/web/src/lib/auth/AuthProvider.tsx`:
- Around line 53-56: The AuthUser type currently marks permissions as optional
but backend getEnrichedSession() always provides it; update the AuthUser type
definition (the AuthUser interface/type) to make permissions: string[] required,
then remove defensive fallbacks where you used (sessionUser.permissions || []) —
e.g., update the logic in AuthProvider.tsx that checks isPlatformUser to use
sessionUser.permissions.includes('*') directly; ensure any other usages of
AuthUser across the frontend are updated to satisfy the new required property.
In `@apps/web/src/pages/auth/ForgotPasswordPage.tsx`:
- Around line 20-23: Replace hardcoded auth path strings with the centralized
route constant: change the redirectTo value in the
authClient.requestPasswordReset call (and the other hardcoded occurrences at the
other spots in this file) to use AppRoutes.ResetPassword (or the appropriate
AppRoutes auth reset constant used across the app), and add the AppRoutes import
at the top of the file; specifically update the redirectTo argument in the
authClient.requestPasswordReset call and any Link/href/navigation uses around
lines 61-62 and 97-99 to reference the AppRoutes constant instead of the literal
'/reset-password'.
- Around line 25-33: The error handling in the ForgotPasswordPage.tsx block that
checks res.error and throws "Email Not Found" leaks account existence; update
the handler in the code that processes the password-reset response (the if
(res.error) branch) to treat "not found" as a success case or return a generic
response instead of throwing a specific "Email Not Found" error—i.e.,
remove/replace the toLowerCase().includes('not found') branch so that both
not-found and success paths yield the same generic message or behavior to
prevent account enumeration.
In `@apps/web/src/pages/auth/LoginPage.tsx`:
- Around line 65-68: The assignment uses unnecessary parentheses around data;
change the declaration const rawUser = (data).user; to remove the parentheses so
it reads const rawUser = data.user; — keep the rest of the logic (the PBAC check
using hasPermission(rawUser?.permissions || [], Resources.ADMIN_DASHBOARD,
Actions.VIEW) and the fallback assignment to AppRoutes.ADMIN.ROOT or
AppRoutes.TENANT.ROOT) unchanged.
In `@apps/web/src/pages/auth/ResetPasswordPage.tsx`:
- Around line 1-6: Import the AppRoutes constant and replace the hardcoded login
path used in the navigation call within ResetPasswordPage with
AppRoutes.AUTH.LOGIN; specifically add an import for AppRoutes and update the
navigate(...) invocation (the one currently navigating to the login route after
reset or cancel) to use AppRoutes.AUTH.LOGIN so routing is consistent with
LoginPage and PBAC refactor.
In `@packages/identity/src/adapters/better-auth.adapter.ts`:
- Line 49: The console.log calls in Better Auth Adapter (e.g., the
initialization log "Better Auth Adapter Initializing with Password Reset
Enabled" and the similar log at line ~136) should be gated behind an
environment/debug flag or replaced with a structured logger; update the code in
better-auth.adapter.ts to check a NODE_ENV/DEBUG-like flag (or use the existing
logger instance if one exists) before emitting these messages, and change the
prints to use the structured logger (e.g., logger.debug/info) so initialization
details and route lists are not unconditionally written in production.
- Around line 67-76: The reset link is constructed with the raw token which may
break query strings or HTML attributes; inside sendResetPassword (and where
resetUrl is formed using validateFrontendUrl(this.config.frontendUrl)),
URL‑encode the token (e.g. via encodeURIComponent(token)) before interpolating
it into resetUrl and into the HTML/text payload passed to
this.emailService.sendEmail so both the href and visible link use the encoded
token.
pramodnarayana
marked this pull request as ready for review
January 30, 2026 08:25
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/App.tsx (1)
32-35: 🧹 Nitpick | 🔵 TrivialConsider using AppRoutes constants for consistency with wildcard routes.
The public auth routes correctly use
AppRoutesconstants, but the tenant and admin wildcard routes still use hardcoded strings. For consistency:♻️ Suggested refactor for consistency
{/* Tenant Routes */} - <Route path="/dashboard/*" element={<TenantRoutes />} /> + <Route path={`${AppRoutes.TENANT.ROOT}/*`} element={<TenantRoutes />} /> {/* Admin Routes */} - <Route path="/admin/*" element={<AdminRoutes />} /> + <Route path={`${AppRoutes.ADMIN.ROOT}/*`} element={<AdminRoutes />} />apps/api/src/modules/identity/auth/auth.service.ts (1)
107-118:⚠️ Potential issue | 🟡 MinorPotential duplicate permissions in error fallback path.
If
permissionProvider.getPermissions()throws after platform-level permissions were already added (Lines 91-97), the catch block adds them again (Lines 113-117), resulting in duplicate entries like['*', '*']or['admin_dashboard:view', 'admin_dashboard:view'].Consider deduplicating or checking before adding in the fallback:
🐛 Proposed fix
} catch (error) { this.logger.error( `Failed to fetch permissions for user ${user.id} in org ${organizationId}`, error, ); // Fallback: If platform admin, ensure they still have access - if (user.systemRole === 'platform_admin') { + if (user.systemRole === 'platform_admin' && !permissions.includes('*')) { permissions.push('*'); - } else if (user.systemRole === 'platform_user') { + } else if (user.systemRole === 'platform_user' && !permissions.includes('admin_dashboard:view')) { permissions.push('admin_dashboard:view'); } }Alternatively, deduplicate at the end:
return { ... user: { ...user, permissions: [...new Set(permissions)], }, };
🤖 Fix all issues with AI agents
In `@apps/api/src/app.module.ts`:
- Around line 26-28: The constructor parameters typed as `any` (`db` and
`emailService`) disable TypeScript checks; replace `any` with the actual
injected types used by your providers (e.g., use PrismaClient or your
DatabaseService type for `db`, and the concrete EmailService/MailerService
interface/class used by your DI container for `emailService`), import those
types at the top, update the constructor signature in the AppModule (or the
class containing `db` and `emailService`) to use them, and then remove the
now-unneeded eslint-disable comments that were suppressing type checks.
- Around line 20-47: The test mock for IdentityModule only implements register;
add a registerAsync property to the exported IdentityModule mock that mirrors
register (e.g., registerAsync: vi.fn(() => ({ module: class IdentityModuleMock
{}, providers: [], exports: [] }))) so code that calls
IdentityModule.registerAsync uses the mock; ensure the mock exports both
register and registerAsync functions returning the same mock module shape and
update any existing imports that reference IdentityModule if needed.
In `@packages/identity/src/identity.module.ts`:
- Around line 153-157: Multiple provider factories (USER_PROVIDER,
TENANT_PROVIDER, PERMISSION_PROVIDER) redundantly check identityOptions.db even
though AUTH_PROVIDER already validates db and email; to DRY this up, move the db
null-check into the central IDENTITY_OPTIONS factory (or perform a single
validation prior to provider creation) so subsequent factories can assume
identityOptions.db exists, and remove the repeated checks inside USER_PROVIDER,
TENANT_PROVIDER, and PERMISSION_PROVIDER while keeping defensive comments if you
want to retain safety.
- Around line 113-121: The registerAsync signature uses loose any[] types for
imports and inject; update it to use Nest types for better safety: change
imports to ModuleMetadata['imports'] and change inject to (string | symbol |
Provider | Type<any>)[] (import Provider, ModuleMetadata, and Type from
`@nestjs/common`), keeping the existing useFactory: () =>
Promise<IdentityModuleOptions> | IdentityModuleOptions and return type
DynamicModule so registerAsync, useFactory, and IdentityModuleOptions are the
referenced symbols to locate and apply this type tightening.
- Around line 126-130: Replace the magic string "IDENTITY_OPTIONS" with a named
exported constant (e.g., IDENTITY_OPTIONS) declared in your constants file and
referenced by the provider; update the provider object that currently uses
provide: "IDENTITY_OPTIONS" and any inject: ["IDENTITY_OPTIONS"] usages to use
the imported constant instead; ensure you export the constant from constants.ts
and import it into identity.module.ts (and any other modules) so all tokens
consistently reference the same identifier to avoid collisions and make
refactors safe.
Comment on lines
+26
to
+28
| db: any, | ||
|
|
||
| emailService: any, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding type annotations instead of any for injected dependencies.
Using any for db and emailService parameters disables TypeScript's type checking. Consider importing the proper types for better IDE support and compile-time safety.
♻️ Proposed fix
+import { NodePgDatabase } from 'drizzle-orm/node-postgres';
+import * as schema from './db/schema';
+import { IEmailProvider } from '@nexiom/identity';
...
useFactory: (
configService: ConfigService,
-
- db: any,
-
- emailService: any,
+ db: NodePgDatabase<typeof schema>,
+ emailService: IEmailProvider,
) => ({This removes the need for the eslint-disable comments on lines 42 and 44.
📝 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
| db: any, | |
| emailService: any, | |
| import { NodePgDatabase } from 'drizzle-orm/node-postgres'; | |
| import * as schema from './db/schema'; | |
| import { IEmailProvider } from '@nexiom/identity'; | |
| db: NodePgDatabase<typeof schema>, | |
| emailService: IEmailProvider, |
🤖 Prompt for AI Agents
In `@apps/api/src/app.module.ts` around lines 26 - 28, The constructor parameters
typed as `any` (`db` and `emailService`) disable TypeScript checks; replace
`any` with the actual injected types used by your providers (e.g., use
PrismaClient or your DatabaseService type for `db`, and the concrete
EmailService/MailerService interface/class used by your DI container for
`emailService`), import those types at the top, update the constructor signature
in the AppModule (or the class containing `db` and `emailService`) to use them,
and then remove the now-unneeded eslint-disable comments that were suppressing
type checks.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/api/src/app.module.ts`:
- Around line 42-44: The explicit cast db: db as any hides a schema mismatch
between ./db/schema and the identity module; replace the any by
aligning/exporting a shared schema type: export the expected schema
interface/type from the identity module (or add a shared Schema type), import
that type into apps/api/src/app.module.ts, and change the provider to db: db as
IdentitySchemaType (or adapt the local ./db/schema to implement that interface).
Update the identity module’s exported types (or create a shared types package)
so consumers can import the exact schema type instead of using any and remove
the as any cast.
Comment on lines
+42
to
+44
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| db: db as any, | ||
| email: emailService, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type cast to any masks schema type mismatch.
The db: db as any cast is necessary because the app's schema (./db/schema) differs from the identity module's expected schema type. While functional, this hides potential incompatibilities at compile time.
Consider defining a shared schema interface or exporting the identity module's schema types so consumers can extend or align with them, eliminating the need for as any.
🤖 Prompt for AI Agents
In `@apps/api/src/app.module.ts` around lines 42 - 44, The explicit cast db: db as
any hides a schema mismatch between ./db/schema and the identity module; replace
the any by aligning/exporting a shared schema type: export the expected schema
interface/type from the identity module (or add a shared Schema type), import
that type into apps/api/src/app.module.ts, and change the provider to db: db as
IdentitySchemaType (or adapt the local ./db/schema to implement that interface).
Update the identity module’s exported types (or create a shared types package)
so consumers can import the exact schema type instead of using any and remove
the as any cast.
This was referenced Jan 30, 2026
This was referenced Feb 6, 2026
This was referenced Feb 16, 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
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.