Merged
Conversation
📝 WalkthroughWalkthroughImplemented role-based access control by adding a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AdminController as Admin Controller
participant VerifyAdmin as verifyAdmin Middleware
participant JwtService
participant SendInviteUseCase
participant AdminRepository
participant EmailService
Client->>AdminController: POST /admin/invites
AdminController->>VerifyAdmin: Extract & verify role
VerifyAdmin->>JwtService: verifyToken(bearer_token)
JwtService-->>VerifyAdmin: { data: { role: "admin", ... } }
alt Role is admin
VerifyAdmin-->>AdminController: ✓ Proceed
AdminController->>SendInviteUseCase: execute({ invite_token })
SendInviteUseCase->>AdminRepository: getInviteByToken(invite_token)
AdminRepository-->>SendInviteUseCase: invite { email, organization_id, token, expires_at }
alt Invite found
SendInviteUseCase->>EmailService: send(email, token, expires_at)
EmailService-->>SendInviteUseCase: ✓ Sent
SendInviteUseCase-->>AdminController: { invite_id, email, expires_at }
AdminController-->>Client: 200 OK
else Invite not found
SendInviteUseCase-->>AdminController: throw InviteNotFound
AdminController-->>Client: 404 Not Found
end
else Role is not admin
VerifyAdmin-->>Client: 403 Forbidden
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/use-cases/auth/sso-refresh-token/index.ts (1)
34-40:⚠️ Potential issue | 🟠 MajorGuard
refresh_claims.rolebefore minting a new access token.On Line 38,
refresh_claims.roleis used directly. If that claim is missing, you still mint a new token with an invalid auth context. Fail fast beforecreateToken(...).🔧 Proposed fix
if (session.email !== refresh_claims.email) throw new InvalidSessionError() + if (!refresh_claims.role) throw new InvalidTokenError() const { token: access_token, claims: access_claims } = this.jwtService.createToken( refresh_claims.id, refresh_claims.email, refresh_claims.role, Duration.minutes(15) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/use-cases/auth/sso-refresh-token/index.ts` around lines 34 - 40, Check for the presence of refresh_claims.role and fail fast before calling this.jwtService.createToken: validate that refresh_claims.role is defined and valid (e.g., non-empty, matches expected enum) and throw/return an appropriate error (or reject) if missing; then call createToken(refresh_claims.id, refresh_claims.email, refresh_claims.role, Duration.minutes(15)) only after the role check to avoid minting tokens with an invalid auth context.src/core/use-cases/auth/login-user/index.ts (1)
27-33:⚠️ Potential issue | 🟡 MinorRemove unnecessary optional chaining on
organization.roleat line 59.The schema and migration guarantee
roleisNOT NULLwithDEFAULT 'member'. Lines 31 and 39 are safe. However, line 59 violates the return type contract by using optional chaining:role: organization?.rolecould returnundefined, butLoginUserUseCaseReturndeclaresrole: string(non-optional).Change line 59 from:
role: organization?.roleto:
role: organization.role🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/use-cases/auth/login-user/index.ts` around lines 27 - 33, The return object uses optional chaining for organization.role which can produce undefined and violates the declared non-optional LoginUserUseCaseReturn.role; update the return to use the guaranteed non-null property (replace organization?.role with organization.role) in the function that assembles the response after jwtService.createToken (refer to organization, role, and LoginUserUseCaseReturn in this module) so the returned type matches the contract.
🧹 Nitpick comments (14)
src/core/domain/exceptions/invites.ts (1)
3-7: Consider making the status immutable for this specific error.The constructor parameter
public status = 404allows callers to override the status (e.g.,new InviteNotFound(500)), which would be semantically incorrect for a "not found" error. Consider hardcoding the status:♻️ Proposed fix
export class InviteNotFound extends ControllerError { - constructor(public status = 404) { + constructor() { super('Invite not found.') + this.status = 404 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/domain/exceptions/invites.ts` around lines 3 - 7, The InviteNotFound error currently accepts a mutable status via the constructor (constructor(public status = 404)), allowing callers to override a 404; change InviteNotFound so the status is fixed to 404 — remove the status constructor parameter and set a readonly or private status property to 404 (or call super with appropriate args if ControllerError accepts status), ensuring InviteNotFound always represents a 404 and cannot be instantiated with a different status.src/core/use-cases/email/send-invite/index.ts (1)
15-19: Consider validating invite state before sending email.The use case checks if the invite exists but doesn't validate whether it's still valid for sending. Consider checking for:
invite.used_at- already used invites shouldn't receive resend emailsinvite.cancelled_at- cancelled invites shouldn't be emailedinvite.expires_at < now- expired invites might warrant different handling♻️ Proposed validation
const invite = await this.adminRepository.getInviteByToken(invite_token) if (!invite) { throw new InviteNotFound() } + if (invite.used_at) { + throw new InviteAlreadyUsedError() + } + + if (invite.cancelled_at) { + throw new InviteCancelledError() + } + await this.resendRepository.sendInviteEmail({You may need to create the additional exception classes or reuse existing ones from
@/core/domain/exceptions/admin.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/use-cases/email/send-invite/index.ts` around lines 15 - 19, The handler currently only checks existence via this.adminRepository.getInviteByToken(invite_token) and throws InviteNotFound; add validation of invite state before sending by checking invite.used_at, invite.cancelled_at, and invite.expires_at versus Date.now() in the send-invite use case (method using getInviteByToken and the local invite variable) and throw appropriate domain exceptions (e.g., InviteAlreadyUsed, InviteCancelled, InviteExpired or reuse equivalents from `@/core/domain/exceptions/admin`) instead of proceeding to send; ensure the new exceptions are imported and used where the current InviteNotFound is thrown so already used, cancelled, or expired invites are rejected.src/core/use-cases/admin/regenerate-token-and-resend-email/index.ts (1)
26-28: Consider a more precise error type for this edge case.The token existence is already validated at lines 12-16. If
regenerateAndResendTokenreturns null after that validation, it indicates an unexpected failure (e.g., race condition, database error) rather than "token doesn't exist." The current error type is semantically misleading.That said, this defensive check is reasonable for robustness. If you keep it, consider a more appropriate error like
InternalErrororRegenerationFailedError.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/use-cases/admin/regenerate-token-and-resend-email/index.ts` around lines 26 - 28, The defensive check throwing InviteTokenDoesNotExistError when refreshedToken is falsy is semantically incorrect because existence was already validated; update the error to a more precise type (e.g., RegenerationFailedError or a generic InternalError) and ensure regenerateAndResendToken callers can handle this as an unexpected failure; specifically replace the throw of InviteTokenDoesNotExistError with throwing a new RegenerationFailedError (or InternalError) and, if RegenerationFailedError does not exist, add it to the error types and use it in the refreshedToken null branch in the regenerateAndResendToken handling code.src/core/use-cases/auth/login-user/types.ts (1)
15-15: Consider using a stricter type forrole.Using
stringloses type safety. Prisma generates aUserRoleEnumtype that could be used here, or define a union type:♻️ Proposed fix
+type UserRole = 'member' | 'admin' + export interface LoginUserUseCaseReturn { session_id: string access_token: string refresh_token: string access_token_expires_at: Date refresh_token_expires_at: Date organization: { name: string email: string - role: string + role: UserRole } }Alternatively, import the Prisma-generated enum type if available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/use-cases/auth/login-user/types.ts` at line 15, The `role: string` property is too permissive—replace it with a stricter enum type by importing the Prisma-generated user role type (e.g., UserRole or UserRoleEnum from `@prisma/client`) or define a literal union (e.g., 'admin'|'user'|...) and use that in place of `role` in the type that declares `role` in this file; update any related imports and usages to use the new enum/union type.src/shared/infra/auth/jwt/types.ts (1)
7-8: Prefer a constrained role type for JWT claims.
role: stringon Line 7 is permissive. Consider using a shared role union/enum (e.g.,admin | member) to prevent accidental invalid claim values at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/infra/auth/jwt/types.ts` around lines 7 - 8, The JWT claim currently uses an unconstrained property `role: string`; change it to a constrained type—either import and use your shared enum/union (e.g., `UserRole` or `Role`) or replace it with a literal union like `role: 'admin' | 'member'` on the JWT claim type in src/shared/infra/auth/jwt/types.ts; update the type declaration that contains `role` and add the corresponding import from the shared types module if using a shared enum so the compiler enforces valid role values.src/shared/infra/auth/jwt/index.ts (2)
25-25: Use shorthand property syntax.Since the property name matches the variable name, use the shorthand syntax for consistency.
✨ Suggested fix
- role: role, + role,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/infra/auth/jwt/index.ts` at line 25, Change the object property assignment that currently uses "role: role" to use JavaScript shorthand property syntax ("role") where the variable and property names match; locate the object literal in src/shared/infra/auth/jwt/index.ts (the object that includes the "role: role" entry) and replace that explicit key-value pair with the shorthand property "role".
35-43: Consider validatingroleclaim inverifyToken.The method validates
idandrole. Sinceroleis now a required claim for authorization decisions (used inverifyAdmin), consider adding validation to ensure tokens without aroleclaim are rejected early.🛡️ Suggested fix
verifyToken(token: string): UserClaims { const decoded = jwt.verify(token, this.secretKey) as UserClaims - if (!decoded.id || !decoded.email) { + if (!decoded.id || !decoded.email || !decoded.role) { throw new Error('Invalid token claims') } return decoded }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/infra/auth/jwt/index.ts` around lines 35 - 43, The verifyToken method currently checks id and email but misses validating the role claim, so update verifyToken (in src/shared/infra/auth/jwt/index.ts) to assert decoded.role exists (and optionally is one of the expected role strings) and throw an Error('Invalid token claims') or a more specific error when role is missing/invalid; ensure this aligns with verifyAdmin's expectations so tokens lacking a role are rejected early.src/adapters/inbound/http/middlewares/verify-jwt.ts (2)
18-18: Incomplete user data population.The middleware sets only
subonrequest.user, butUserClaimsincludesglobal.d.ts,request.usersupportsemail?: string | null. Consider populating available fields for downstream handlers.✨ Suggested fix
- request.user = { sub: data.sub } + request.user = { sub: data.sub, email: data.email }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/inbound/http/middlewares/verify-jwt.ts` at line 18, The middleware verify-jwt.ts currently sets only request.user = { sub: data.sub }; update it to populate all available claim fields from the decoded token (e.g., email, name, roles or any other properties present on data) so downstream handlers receive the full UserClaims shape defined in global.d.ts; for example, set request.user to include sub plus email: data.email ?? null and copy any other standard/expected claims from data, ensuring the object matches the UserClaims type.
20-20: Replaceconsole.logwith structured logging.Using
console.logfor error logging is not recommended for production. Consider using a proper logger with appropriate log levels.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/inbound/http/middlewares/verify-jwt.ts` at line 20, Replace the plain console.log in verify-jwt.ts with the project's structured logger: import or use the existing logger (e.g., logger or processLogger) and call logger.error('Error verifying JWT', { error: err, token: tokenValueIfAvailable, route: req?.path }) so the error is recorded with context and proper log level; ensure the log call passes the error object (not just stringifies it) and remove the console.log('Error verifying JWT:', err) statement.src/adapters/inbound/http/middlewares/verify-admin.ts (3)
18-18: Consider using a constant or enum for the role check.The hardcoded string
'admin'is fragile. Consider using a constant or theUserRoleEnum(referenced inglobal.d.ts) for consistency and maintainability.✨ Example using a constant
// In a constants file or at the top of this file const ADMIN_ROLE = 'admin' // Then use: if (data.role !== ADMIN_ROLE) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/inbound/http/middlewares/verify-admin.ts` at line 18, Replace the hardcoded role literal in the verify-admin middleware (the `if (data.role !== 'admin')` check) with a single source-of-truth constant or enum: import and use the existing `UserRoleEnum` (or define `ADMIN_ROLE = 'admin'`) and compare `data.role` against that symbol (e.g., `UserRoleEnum.Admin` or `ADMIN_ROLE`) so the check becomes consistent and maintainable; update any nearby imports in `verify-admin.ts` to reference the chosen constant/enum.
29-29: Replaceconsole.logwith structured logging.Same as in
verifyJWT- use a proper logger with appropriate log levels for production code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/inbound/http/middlewares/verify-admin.ts` at line 29, Replace the console.log call in the verify-admin middleware with the project’s structured logger (the same logger used in verifyJWT) and log at error level; in the verifyAdmin middleware (or the exported middleware function in verify-admin.ts) call processLogger.error (or the shared logger instance) with a clear message like "Error verifying admin token" and include the err object and any request context (e.g., req.path or admin identifier) instead of using console.log so production logs are structured and searchable.
24-27: Inconsistent request property population between middlewares.
verifyAdminsetsrequest.profilewithrole, whileverifyJWTsetsrequest.userwithsub. If routes need both authentication state and role information, handlers may need to check different properties. Consider also settingrequest.userhere for consistency.✨ Suggested enhancement
request.profile = { ...request.profile, role: data.role } + request.user = { sub: data.sub, email: data.email }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/inbound/http/middlewares/verify-admin.ts` around lines 24 - 27, verifyAdmin currently attaches role to request.profile (request.profile.role) while authentication middleware verifyJWT populates request.user (request.user.sub), causing inconsistent access patterns; update verifyAdmin to also set request.user (e.g., copy or merge user-identifying fields and role into request.user) so route handlers can rely on a consistent property, and ensure verifyAdmin returns/continues with both request.profile and request.user populated (refer to verifyAdmin and verifyJWT, and request.profile/request.user symbols).src/core/use-cases/auth/change-password/index.ts (1)
64-71: Unusedsessionvariable and inconsistent return value.The
sessionis created but never used, and the method returnsnulldespite being namedResetPasswordAndLoginUseCase. If this is a "login" use case, consider returning the tokens (access_token,refresh_token) for client use.💡 Suggested return with tokens
- const session = await this.authRepository.createSession({ + await this.authRepository.createSession({ id: refresh_claims.jti, email: organization.email, refresh_token, expires_at: new Date(refresh_claims.exp * 1000) }) - return null + return { access_token, refresh_token } }Note: This would also require updating the
Promise<null>return type to match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/use-cases/auth/change-password/index.ts` around lines 64 - 71, The created session returned from authRepository.createSession is unused and the method returns null despite being a "ResetPasswordAndLoginUseCase"; change the method (likely the execute method on ResetPasswordAndLoginUseCase) to return the generated tokens rather than null: use the existing refresh_token and access_token variables and return an object containing access_token and refresh_token (and update the method's Promise<null> return type to the appropriate Promise<{access_token: string; refresh_token: string;}>) or, if a session object must be surfaced, return both session and tokens; alternatively remove creation of session if truly unnecessary. Ensure you reference authRepository.createSession and the refresh_claims/refresh_token/access_token variables when making the change.src/adapters/inbound/http/controllers/admin/get-invite-by-token/index.ts (1)
18-18: Consider clarifying the route path.The path
/admin/invites/:token/tokenhas a redundant/tokensuffix. Consider simplifying to/admin/invites/:tokenor/admin/invites/by-token/:tokenfor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/inbound/http/controllers/admin/get-invite-by-token/index.ts` at line 18, The route path in the `@Route` decorator currently uses a redundant '/admin/invites/:token/token'; update the decorator in the get-invite-by-token controller to a clearer path such as '/admin/invites/:token' or '/admin/invites/by-token/:token' and ensure any references are updated accordingly (route registration, OpenAPI/Swagger metadata, client calls, and tests) so they match the new path; specifically locate the `@Route`('GET', '/admin/invites/:token/token', ...) declaration and replace the path string and then search for usages of '/admin/invites/:token/token' to update them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/adapters/inbound/http/middlewares/verify-admin.ts`:
- Around line 28-33: The catch block in the verify-admin middleware currently
returns 403 for all errors; change it to return 401 for JWT/authentication
failures and reserve 403 only for role/authorization mismatches. Specifically,
in the catch branch of the verify-admin middleware (the try/catch around token
verification), replace reply.status(403) with reply.status(401) for errors
coming from JWT verification (e.g. err.name === 'TokenExpiredError' || err.name
=== 'JsonWebTokenError' or by checking instanceof jwt.JsonWebTokenError after
importing jsonwebtoken), and ensure any later checks that detect a role mismatch
(e.g. where you compare user.role or isAdmin) continue to return
reply.status(403).
---
Outside diff comments:
In `@src/core/use-cases/auth/login-user/index.ts`:
- Around line 27-33: The return object uses optional chaining for
organization.role which can produce undefined and violates the declared
non-optional LoginUserUseCaseReturn.role; update the return to use the
guaranteed non-null property (replace organization?.role with organization.role)
in the function that assembles the response after jwtService.createToken (refer
to organization, role, and LoginUserUseCaseReturn in this module) so the
returned type matches the contract.
In `@src/core/use-cases/auth/sso-refresh-token/index.ts`:
- Around line 34-40: Check for the presence of refresh_claims.role and fail fast
before calling this.jwtService.createToken: validate that refresh_claims.role is
defined and valid (e.g., non-empty, matches expected enum) and throw/return an
appropriate error (or reject) if missing; then call
createToken(refresh_claims.id, refresh_claims.email, refresh_claims.role,
Duration.minutes(15)) only after the role check to avoid minting tokens with an
invalid auth context.
---
Nitpick comments:
In `@src/adapters/inbound/http/controllers/admin/get-invite-by-token/index.ts`:
- Line 18: The route path in the `@Route` decorator currently uses a redundant
'/admin/invites/:token/token'; update the decorator in the get-invite-by-token
controller to a clearer path such as '/admin/invites/:token' or
'/admin/invites/by-token/:token' and ensure any references are updated
accordingly (route registration, OpenAPI/Swagger metadata, client calls, and
tests) so they match the new path; specifically locate the `@Route`('GET',
'/admin/invites/:token/token', ...) declaration and replace the path string and
then search for usages of '/admin/invites/:token/token' to update them.
In `@src/adapters/inbound/http/middlewares/verify-admin.ts`:
- Line 18: Replace the hardcoded role literal in the verify-admin middleware
(the `if (data.role !== 'admin')` check) with a single source-of-truth constant
or enum: import and use the existing `UserRoleEnum` (or define `ADMIN_ROLE =
'admin'`) and compare `data.role` against that symbol (e.g.,
`UserRoleEnum.Admin` or `ADMIN_ROLE`) so the check becomes consistent and
maintainable; update any nearby imports in `verify-admin.ts` to reference the
chosen constant/enum.
- Line 29: Replace the console.log call in the verify-admin middleware with the
project’s structured logger (the same logger used in verifyJWT) and log at error
level; in the verifyAdmin middleware (or the exported middleware function in
verify-admin.ts) call processLogger.error (or the shared logger instance) with a
clear message like "Error verifying admin token" and include the err object and
any request context (e.g., req.path or admin identifier) instead of using
console.log so production logs are structured and searchable.
- Around line 24-27: verifyAdmin currently attaches role to request.profile
(request.profile.role) while authentication middleware verifyJWT populates
request.user (request.user.sub), causing inconsistent access patterns; update
verifyAdmin to also set request.user (e.g., copy or merge user-identifying
fields and role into request.user) so route handlers can rely on a consistent
property, and ensure verifyAdmin returns/continues with both request.profile and
request.user populated (refer to verifyAdmin and verifyJWT, and
request.profile/request.user symbols).
In `@src/adapters/inbound/http/middlewares/verify-jwt.ts`:
- Line 18: The middleware verify-jwt.ts currently sets only request.user = {
sub: data.sub }; update it to populate all available claim fields from the
decoded token (e.g., email, name, roles or any other properties present on data)
so downstream handlers receive the full UserClaims shape defined in global.d.ts;
for example, set request.user to include sub plus email: data.email ?? null and
copy any other standard/expected claims from data, ensuring the object matches
the UserClaims type.
- Line 20: Replace the plain console.log in verify-jwt.ts with the project's
structured logger: import or use the existing logger (e.g., logger or
processLogger) and call logger.error('Error verifying JWT', { error: err, token:
tokenValueIfAvailable, route: req?.path }) so the error is recorded with context
and proper log level; ensure the log call passes the error object (not just
stringifies it) and remove the console.log('Error verifying JWT:', err)
statement.
In `@src/core/domain/exceptions/invites.ts`:
- Around line 3-7: The InviteNotFound error currently accepts a mutable status
via the constructor (constructor(public status = 404)), allowing callers to
override a 404; change InviteNotFound so the status is fixed to 404 — remove the
status constructor parameter and set a readonly or private status property to
404 (or call super with appropriate args if ControllerError accepts status),
ensuring InviteNotFound always represents a 404 and cannot be instantiated with
a different status.
In `@src/core/use-cases/admin/regenerate-token-and-resend-email/index.ts`:
- Around line 26-28: The defensive check throwing InviteTokenDoesNotExistError
when refreshedToken is falsy is semantically incorrect because existence was
already validated; update the error to a more precise type (e.g.,
RegenerationFailedError or a generic InternalError) and ensure
regenerateAndResendToken callers can handle this as an unexpected failure;
specifically replace the throw of InviteTokenDoesNotExistError with throwing a
new RegenerationFailedError (or InternalError) and, if RegenerationFailedError
does not exist, add it to the error types and use it in the refreshedToken null
branch in the regenerateAndResendToken handling code.
In `@src/core/use-cases/auth/change-password/index.ts`:
- Around line 64-71: The created session returned from
authRepository.createSession is unused and the method returns null despite being
a "ResetPasswordAndLoginUseCase"; change the method (likely the execute method
on ResetPasswordAndLoginUseCase) to return the generated tokens rather than
null: use the existing refresh_token and access_token variables and return an
object containing access_token and refresh_token (and update the method's
Promise<null> return type to the appropriate Promise<{access_token: string;
refresh_token: string;}>) or, if a session object must be surfaced, return both
session and tokens; alternatively remove creation of session if truly
unnecessary. Ensure you reference authRepository.createSession and the
refresh_claims/refresh_token/access_token variables when making the change.
In `@src/core/use-cases/auth/login-user/types.ts`:
- Line 15: The `role: string` property is too permissive—replace it with a
stricter enum type by importing the Prisma-generated user role type (e.g.,
UserRole or UserRoleEnum from `@prisma/client`) or define a literal union (e.g.,
'admin'|'user'|...) and use that in place of `role` in the type that declares
`role` in this file; update any related imports and usages to use the new
enum/union type.
In `@src/core/use-cases/email/send-invite/index.ts`:
- Around line 15-19: The handler currently only checks existence via
this.adminRepository.getInviteByToken(invite_token) and throws InviteNotFound;
add validation of invite state before sending by checking invite.used_at,
invite.cancelled_at, and invite.expires_at versus Date.now() in the send-invite
use case (method using getInviteByToken and the local invite variable) and throw
appropriate domain exceptions (e.g., InviteAlreadyUsed, InviteCancelled,
InviteExpired or reuse equivalents from `@/core/domain/exceptions/admin`) instead
of proceeding to send; ensure the new exceptions are imported and used where the
current InviteNotFound is thrown so already used, cancelled, or expired invites
are rejected.
In `@src/shared/infra/auth/jwt/index.ts`:
- Line 25: Change the object property assignment that currently uses "role:
role" to use JavaScript shorthand property syntax ("role") where the variable
and property names match; locate the object literal in
src/shared/infra/auth/jwt/index.ts (the object that includes the "role: role"
entry) and replace that explicit key-value pair with the shorthand property
"role".
- Around line 35-43: The verifyToken method currently checks id and email but
misses validating the role claim, so update verifyToken (in
src/shared/infra/auth/jwt/index.ts) to assert decoded.role exists (and
optionally is one of the expected role strings) and throw an Error('Invalid
token claims') or a more specific error when role is missing/invalid; ensure
this aligns with verifyAdmin's expectations so tokens lacking a role are
rejected early.
In `@src/shared/infra/auth/jwt/types.ts`:
- Around line 7-8: The JWT claim currently uses an unconstrained property `role:
string`; change it to a constrained type—either import and use your shared
enum/union (e.g., `UserRole` or `Role`) or replace it with a literal union like
`role: 'admin' | 'member'` on the JWT claim type in
src/shared/infra/auth/jwt/types.ts; update the type declaration that contains
`role` and add the corresponding import from the shared types module if using a
shared enum so the compiler enforces valid role values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4452cf44-4933-4bac-a77d-40280ef81f02
📒 Files selected for processing (22)
package.jsonprisma/schema/account.prismaprisma/schema/migrations/20260326150100_add_role_on_organization/migration.sqlsrc/adapters/inbound/http/controllers/admin/cancel-pending-invite/index.tssrc/adapters/inbound/http/controllers/admin/create-and-send-invite/index.tssrc/adapters/inbound/http/controllers/admin/get-invite-by-token/index.tssrc/adapters/inbound/http/controllers/admin/list-all-invites/index.tssrc/adapters/inbound/http/controllers/admin/regenerate-token-and-resend-email/index.tssrc/adapters/inbound/http/controllers/admin/use-invite-token/index.tssrc/adapters/inbound/http/controllers/admin/validate-invite-token/index.tssrc/adapters/inbound/http/middlewares/verify-admin.tssrc/adapters/inbound/http/middlewares/verify-jwt.tssrc/core/domain/exceptions/invites.tssrc/core/use-cases/admin/regenerate-token-and-resend-email/index.tssrc/core/use-cases/auth/change-password/index.tssrc/core/use-cases/auth/login-user/index.tssrc/core/use-cases/auth/login-user/types.tssrc/core/use-cases/auth/sso-refresh-token/index.tssrc/core/use-cases/email/send-invite/index.tssrc/core/use-cases/email/send-invite/types.tssrc/shared/infra/auth/jwt/index.tssrc/shared/infra/auth/jwt/types.ts
Comment on lines
+28
to
+33
| } catch (err) { | ||
| console.log('Error verifying admin token:', err) | ||
| return reply.status(403).send({ | ||
| message: 'Forbidden', | ||
| }) | ||
| } |
There was a problem hiding this comment.
JWT verification errors should return 401, not 403.
The catch block returns 403 (Forbidden) for all errors, including JWT verification failures (invalid signature, expired token). These are authentication failures and should return 401 (Unauthorized). Only role mismatches should return 403.
🐛 Suggested fix
} catch (err) {
console.log('Error verifying admin token:', err)
- return reply.status(403).send({
- message: 'Forbidden',
+ return reply.status(401).send({
+ message: 'Unauthorized',
})
}📝 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
| } catch (err) { | |
| console.log('Error verifying admin token:', err) | |
| return reply.status(403).send({ | |
| message: 'Forbidden', | |
| }) | |
| } | |
| } catch (err) { | |
| console.log('Error verifying admin token:', err) | |
| return reply.status(401).send({ | |
| message: 'Unauthorized', | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/adapters/inbound/http/middlewares/verify-admin.ts` around lines 28 - 33,
The catch block in the verify-admin middleware currently returns 403 for all
errors; change it to return 401 for JWT/authentication failures and reserve 403
only for role/authorization mismatches. Specifically, in the catch branch of the
verify-admin middleware (the try/catch around token verification), replace
reply.status(403) with reply.status(401) for errors coming from JWT verification
(e.g. err.name === 'TokenExpiredError' || err.name === 'JsonWebTokenError' or by
checking instanceof jwt.JsonWebTokenError after importing jsonwebtoken), and
ensure any later checks that detect a role mismatch (e.g. where you compare
user.role or isAdmin) continue to return reply.status(403).
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
2 participants
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
Bug Fixes
Chores