Repository navigation
feat(auth): JWT authentication, RBAC administration and email delivery - #3
Merged
Merged
Conversation
feat(data-modeling): add Lucid models, migrations and seeders for the EER schema
This branch needs three packages that the scaffold never installed. Bouncer and limiter are the authorization and rate limiting libraries the README names explicitly for this branch, and neither ships with the base @adonisjs/auth or core packages. A JWT signing library is needed as well, since the installed @adonisjs/auth only ships session, basic auth and access tokens guards, no JWT guard exists in the package itself, so the custom guard this branch writes needs a real implementation to sign and verify tokens against. jose was chosen over jsonwebtoken for that last piece since it is pure JavaScript with no native bindings, ships native ESM and TypeScript types, and is the library the Node ecosystem has largely converged on for JWT and JOSE operations going forward.
node ace configure @adonisjs/bouncer and node ace configure @adonisjs/limiter both need their provider registered in adonisrc.ts before the IoC container can resolve ctx.bouncer or the limiter service anywhere else in the app, the same wiring every other package in this file already goes through. The bouncer commands entry and the indexPolicies hook are added for the same reason, so that ace can discover policy classes and expose the make:policy generator. Running the packages' own configure command rather than hand writing this file avoids guessing at an API shape from memory, which is exactly the kind of mistake that broke typecheck on the previous branch.
ctx.bouncer only exists on the request context once InitializeBouncerMiddleware has run, the same pattern initialize_auth_middleware already follows for ctx.auth. It is placed after silent_auth_middleware in the router stack since bouncer needs to read the resolved user off ctx.auth when it builds the Bouncer instance for the request, and that resolution has to happen first.
This middleware builds a Bouncer instance for each request, resolved from ctx.auth.user, and attaches it to the context as ctx.bouncer so downstream middleware, controllers and policies can call bouncer.authorize or bouncer.allows without constructing their own instance. It also augments the HttpContext type so ctx.bouncer is typed against the abilities and policies actually registered in the app, rather than an untyped any. Generated by the package's own configure command rather than written by hand, since this class needs to match the exact Bouncer constructor signature of the installed version.
Abilities registered here can be referenced by name from a policy or from Edge templates, and the file needs to exist before any policy class can import from it. The placeholder editUser ability that ships with the package's own scaffold is left in place for now rather than deleted, since this branch does not yet define resource specific abilities, those belong to the categories, products, customers and orders branches once their own authorization rules are known.
LIMITER_STORE was already declared in the typed env schema back in feature/core-architecture, set to database in every env file, but nothing had actually configured the limiter package against that setting until now. The database store persists rate limit counters in the rate_limits table this branch also adds, which is the right choice for this project since it already runs a shared PostgreSQL instance in every environment and avoids introducing Redis as a new piece of infrastructure just for throttling a handful of auth routes. A memory store is kept alongside it since the limiter package ships it by default and it costs nothing to keep as a documented alternative, useful for a future test suite that wants throttling behavior without touching the database.
This is the named throttle middleware the register, login and forgot-password routes apply later in this branch, matching the THROTTLE_AUTH_MAX and THROTTLE_AUTH_WINDOW values already present in the env schema. The limit itself is wired directly to those env values here rather than hardcoded, so tuning the throttle in a given environment only ever requires an env change, never a code change.
The limiter package's database store needs a table to persist request counters and their expiry against a key, generated directly by the package's own configure command so its column names and types match exactly what the store implementation expects to read and write.
…UTH_WINDOW The previous commit on this file claimed the throttle was already wired to the env driven auth limits, but it had actually only committed the package's generated stub unchanged, a global limiter hardcoded to ten requests per minute under the name global. This commit corrects that: the exported throttle is renamed to authThrottle and reads its request count and window straight from the typed env service instead of a literal, matching what register, login and forgot-password are supposed to apply later in this branch. Caught while reviewing the previous commit's own message against the file it actually produced, before any route wired the throttle in and made the mismatch harder to notice.
HASH_DRIVER has been set to argon in every env file since feature/core-architecture, and the README's tech stack table names Argon2 explicitly as the password hashing algorithm for this project, but the argon2 npm package itself, the native binding the hash service's argon driver wraps, was never installed. Without it, config/hash.ts could only ever resolve to the scrypt driver the --kit=api scaffold shipped by default, regardless of what HASH_DRIVER said. The package ships prebuilt binaries for linux, darwin, freebsd and win32 across common architectures, verified directly by hashing and verifying a password with it before wiring it into config/hash.ts, so no local compilation step is required in most environments including this one.
The hash config inherited from the --kit=api scaffold hardcoded scrypt as the only hasher and never read HASH_DRIVER at all, so setting that variable in any env file had no effect. This branch introduces password based login through AuthService, so the actual hasher the project uses stops being a detail and starts being something the README specifies directly, Argon2, the algorithm OWASP currently recommends for password storage. The default is now read from the typed env service instead of a literal, and the argon entry uses the id variant with a 64 MiB memory cost, three iterations and four way parallelism, values chosen to sit well above argon2's own minimum recommended memory cost without making login latency noticeable on ordinary hardware. Bcrypt is kept configured as a documented fallback since HASH_DRIVER's schema in start/env.ts already allows it, rather than deleting a working driver the project might still want later.
The generated config read its default store straight from
env.get('LIMITER_STORE'), but that variable is typed as
'database' | 'redis' in start/env.ts, matching the README's
documented env reference table, while only a database store is
actually configured below. Running npm run typecheck against the
file as generated failed since 'redis' is not assignable to the
narrower type the configured stores record actually allows.
The default is hardcoded to 'database' instead, the same way
config/database.ts hardcodes its own connection rather than deriving
it from a variable that has no wired up alternative. Redis stays
undocumented as an option here until the project actually provisions
a Redis instance, at which point both the store and this default
would need to be added together.
The generated middleware included a guarded block sharing bouncer helpers with Edge templates whenever ctx.view was present. This project has no Edge or view provider registered anywhere, it is an API only service per the README's own architecture, so ctx.view never exists on the HttpContext type here and the guard could not narrow it to anything usable, which failed npm run typecheck with ctx.view being of type unknown. Removing the branch rather than typing around it keeps the middleware honest about what this project actually is. If a view layer is ever added later, this is the file where that sharing call would come back.
InitializeBouncerMiddleware imports #generated/policies, but that file only starts existing once node ace codegen runs after the bouncer package's indexPolicies hook is registered in adonisrc.ts. Without it, npm run typecheck failed with a missing module error on that import. The registry is currently empty since this branch does not define any policy classes yet, categories, products, customers and orders each own their own policy in their respective feature branches.
Running the rate_limits migration against the live test database regenerated this file automatically, the same way it did in feature/data-modeling. It now includes the RateLimitSchema class alongside the twelve tables from the core and auth schemas.
The custom JWT guard needs to live somewhere, and app/auth is where that guard and any future auth specific helpers belong, distinct from app/middleware and app/services which already have their own aliases. Without this entry in the imports map, config/auth.ts could not resolve #auth/jwt_guard through Node's subpath imports, the same mechanism every other first party import in this project already relies on instead of relative paths.
The installed @adonisjs/auth version ships session, basic auth and
access tokens guards, no JWT guard exists in the package itself, yet
the README's own code samples call ctx.auth.use('jwt') and read a
payload property off it, meaning the project expects a guard
registered under that name inside the normal auth guard system rather
than a bespoke authentication mechanism bolted on separately.
JwtGuard implements the package's own GuardContract interface, the
same contract SessionGuard and AccessTokensGuard satisfy, so the rest
of the app can treat it identically to a built in guard: authenticate,
check, getUserOrFail and authenticateAsClient for Japa's api client
test helpers. It is deliberately narrow in scope, verifying the
token's signature and expiry with jose and resolving the subject claim
to a real User is all it does. It does not check the jti blacklist or
the user's enabled and accountLocked flags, since the README assigns
that check to AuthMiddleware specifically, layered on top of
authenticate() rather than duplicated inside the guard.
JWT_EXPIRY is documented as either a bare number of seconds or a time
span like 7d, and jose's own parser only accepts the latter form for
strings, so a purely numeric value is converted to a number before
being handed to setExpirationTime. Signing is exposed as a standalone
signJwt function rather than a private guard method, since
AuthService's login and register flows need to produce a token
directly, not through a guard instance tied to an existing request.
feature/data-modeling left a session guard configured here purely as a placeholder so the app would boot without the tokens guard the old scaffold's User model could no longer support. This project authenticates every request statelessly through a JWT in the Authorization header, never through a cookie backed session, so the session guard was never going to be more than scaffolding. The default guard is now jwt, backed by the JwtGuard class this same branch adds, which is what AuthMiddleware, RoleMiddleware and every protected route from here on will actually authenticate against.
The README only names three of the six schemas this controller needs (registerSchema, loginSchema, resetPasswordSchema) and leaves activate and forgot-password unspecified. Both of those turned out to need their own compiled schema anyway, an opaque token string for activate and an email lookup for forgot-password, since the hard rule against inline VineJS schemas in controllers applies to every action, not only the three the README happened to name. The password complexity rule (lowercase, uppercase, digit, on top of VineJS's own minLength(8)) is a judgment call the README leaves open by only asking for "a complexity regex" without specifying one. It is shared between registerSchema and resetPasswordSchema so a password set through either path meets the same bar, and deliberately left out of loginSchema, which only checks whatever is already stored and should fail with a credentials error rather than a validation error when the input is merely short. Field length caps on firstName, lastName and email mirror the users table's VARCHAR limits from the README's Auth Model section, so a too-long value is rejected as a 422 before it ever reaches a query that would otherwise truncate or error against the database column.
JwtGuard proves a token is validly signed and not expired, nothing
more, by design (see app/auth/jwt_guard.ts). Revocation is a separate
concern: once logout writes a token's jti into blacklisted_tokens,
that token's signature is still perfectly valid until it naturally
expires, so something has to reject it anyway. This middleware is
that something, and it runs on every route that declares
middleware.auth(), which is also the only place JwtGuard is ever
invoked, so there is no path into the app that skips the check.
The jti is read from ctx.auth.use('jwt').payload after
authenticateUsing() succeeds, then looked up against
blacklisted_tokens with a single findBy. A hit returns 401 directly
from the middleware rather than throwing, matching the README's own
code sample for this file, and the response body is built out by hand
to match the ApiResponse envelope (including timestamp and path, which
the README's illustrative snippet omits for brevity but which every
other error path in this app, including the global exception handler,
includes) since this early return never reaches that handler.
The stub from feature/data-modeling passed every request through
unconditionally so route files could reference middleware.role()
before this branch existed. This replaces that stub with the real
check: authenticate, load the roles relation, and compare role names
against the options.roles argument passed at the route declaration
site, returning 403 in the ApiResponse shape when none match.
The explicit user.load('roles') call is not optional. The README's
troubleshooting section calls out, by name, that auth.authenticate()
never preloads relations and that skipping this load is the most
common reason RoleMiddleware ends up rejecting a user who does hold
the right role, since user.roles would silently be an empty array
rather than throwing. Calling auth.authenticate() again here, after
AuthMiddleware already called it earlier in the same request, is safe
and cheap: JwtGuard tracks authenticationAttempted internally, so a
second call in the same request returns the already-resolved user
instead of re-verifying the token from scratch.
This is the only place in the app that touches User, ActivationToken,
BlacklistedToken or PasswordResetToken for anything auth-related,
per the hard rule against direct model access in controllers.
register, activate, login, logout, forgotPassword, resetPassword and
me each map to one method, matching the README's task list exactly,
plus a refresh method the task list does not enumerate by name but
the Endpoints table requires ("Issue new JWT (if not blacklisted)").
Password hashing never happens here; User's @beforeSave hook does
that on every create and save, so register and resetPassword just
assign the plain password and let the model handle it.
Two decisions the README leaves open are worth calling out. First,
register and forgotPassword both return their raw token (activation
token and password reset token respectively) directly in the service
result rather than only persisting it, because there is no mail
provider configured on this branch and the README documents exactly
this placeholder for forgotPassword; register needed the same
treatment for the register -> activate -> login functional test flow
to be possible at all. Second, login and the credentials branch of
authentication raise the same E_INVALID_CREDENTIALS whether the email
does not exist or the password is wrong, so the response never
confirms which case occurred; forgotPassword does not extend that same
protection to email enumeration and uses findByOrFail instead, a
deliberate tradeoff toward explicit, testable behavior over that
particular hardening within this tutorial's scope.
Business exceptions (account not activated, account locked, token
already used, token expired, invalid reset token) are defined with
createError from @adonisjs/core/exceptions rather than thrown as raw
Error, each carrying its own status and code so the global exception
handler's generic "has a numeric status" branch formats them correctly
without any auth-specific handling added to that file.
Every action follows the same shape: validateUsing the matching schema from auth_validator.ts, call the corresponding AuthService method, wrap the result with respond(). None of the account lifecycle logic (password verification, token validation, role assignment) lives here, it all lives in AuthService, which is what the hard rule against business logic in controllers actually means in practice for this branch. The class is decorated with @Inject() since its constructor takes AuthService as a dependency, resolved automatically by the IoC container. logout and refresh both read from ctx.auth.use('jwt') rather than calling ctx.auth.authenticate() again, since middleware.auth() on those routes has already authenticated the request and rejected a blacklisted jti before the action runs; re-authenticating here would just be redundant work against an already-cached guard instance. logout additionally needs the raw bearer token string, which JwtGuard does not expose after authentication (it only exposes the resolved user and the decoded payload), so it is pulled directly off the Authorization header here and passed to the service, which needs it for the NOT NULL token column on blacklisted_tokens.
… stub
Every route under /api/v1/auth is declared here with its access rule
applied at declaration time, never added retroactively: register,
login and forgot-password carry .use(authThrottle) since they are the
three endpoints an attacker would script against (credential
stuffing, mass account creation, reset-token flooding), while
activate and reset-password are deliberately left unthrottled because
an unguessable token is already their real defense, and
logout/refresh/me are protected by middleware.auth() instead, which
already limits them to a valid, non-blacklisted bearer token. Method,
path and access all match the README's Endpoints table for this
branch exactly.
The two /api/v1/_stub routes exist only because the README's own test
checklist calls for a functional smoke test proving AuthMiddleware and
RoleMiddleware actually work end to end (401 with no token, 200 with a
valid USER token, 403 for USER and 200 for ADMIN behind
role('ADMIN')), and no real protected resource route exists yet on
this branch to hit; categories, products, customers and orders are all
later branches. Both routes are commented as temporary scaffolding and
should be deleted once feature/categories adds its first ADMIN-guarded
route, at which point that route's own functional test supersedes what
these stubs exist to prove.
These files are generated automatically from the controllers and routes present in the app, and each carries its own header warning against hand edits. Adding auth_controller.ts and the /api/v1/auth routes (plus the two temporary middleware smoke-test stubs) changed what the app actually exposes, so the checked-in snapshots were out of date the moment those files were added. Running node ace codegen regenerated the client-side route registry (used by the Tuyau API client) and the server-side controllers and routes type declarations to match. No source file changed here, only the generated reflection of what already exists elsewhere in the tree.
…tests There was no shared way for a functional test to obtain a JWT for one of the seeded ADMIN or USER accounts, which would have meant duplicating a login request and credential lookup in every spec file that needs an authenticated request. This helper reads TEST_ADMIN_EMAIL, TEST_ADMIN_PASSWORD, TEST_USER_EMAIL and TEST_USER_PASSWORD from the typed env service and logs in through the real POST /api/v1/auth/login endpoint, so callers exercise the same credential check and token issuance path a real client goes through instead of forging a token directly. The file also exports ensureRole and ensureSeededUser. While writing these tests it became clear that tests/unit/database/migrations.spec.ts rolls every migration down and back up as part of its own coverage, which drops all seed data as a real side effect since that test does not wrap the migrator in a rollback-able transaction. When the unit and functional suites run together in one process, any auth test running after that migration test would otherwise find the roles table and the seeded accounts empty. These two helpers make each test group that needs that reference data create it for itself inside its own rolled-back transaction, so this branch's tests do not depend on seed data planted by an unrelated file earlier in the run.
…and logout Before this commit AuthService had no test coverage at all, even though it owns the entire account lifecycle other branches will build on top of. There was nothing proving that register actually attaches the default USER role and produces a usable activation token rather than a dangling one, that activate really flips enabled and marks the token consumed so it cannot be replayed, or that login enforces its three failure paths, wrong password, locked account, inactive account, in the specific order the service documents. Getting the ordering wrong would matter in practice: a locked-but-inactive account should read back as not activated rather than locked, so the response never tells a caller more than it needs to know about why an account cannot sign in. These tests hit the real test database inside a rolled back global transaction, following the same pattern used by the model specs from feature/data-modeling, instead of mocking the Lucid models AuthService depends on. The behavior under test only means something once the role pivot row, the activation token row and the blacklist row are actually read back from the database, so asserting against a mock would only restate the implementation rather than verify it. The login tests also decode the returned JWT to confirm the jti claim used later by the blacklist check is present and correctly tied to the user's id, and the logout test confirms the raw token and jti actually land in blacklisted_tokens with the right foreign key.
AuthMiddleware's reason for existing beyond calling auth.authenticate() is rejecting a request whose token has already been logged out, and that logic had no test pinned directly to it. The functional revoked-token scenario in tests/functional/auth.spec.ts does exercise this path end to end through a real HTTP request, but a failure there could just as easily come from the login or logout endpoints as from the middleware's own blacklist query, so it would not point at the actual cause. This test isolates AuthMiddleware's handle method against a minimal fake HttpContext exposing only the auth and response surface it actually reads, so a regression in the query against blacklisted_tokens fails here specifically instead of only showing up as a vague functional failure somewhere downstream. The blacklist row itself is real and written to the test database inside a rolled back global transaction, since AuthMiddleware queries the BlacklistedToken model directly and there is nothing meaningful to mock at that layer. Two cases are covered, a jti that is present in blacklisted_tokens gets an unauthorized response and next is never called, and a jti that is not present lets the request continue through to next as normal, which is the behavior every authenticated route in the application depends on.
…nd rate limiting This branch wires together a JWT guard, two middleware classes and an eight endpoint controller, but none of it had been exercised through a real HTTP request before this commit. Unit coverage on AuthService and AuthMiddleware proves each piece works in isolation, it does not prove register's activation token actually unlocks activate through the HTTP layer, that a token really becomes unusable on the very next request after logout rather than only in a direct service call, or that the role middleware genuinely tells an ADMIN token apart from a USER token using the seeded accounts rather than a hand built fixture. The full lifecycle test walks register, activate, login, a profile fetch, logout and a final rejected profile fetch as eight sequential requests against the running app, and the middleware smoke test hits the two temporary stub routes to confirm AuthMiddleware and RoleMiddleware actually gate access rather than merely compiling. The rate limiting test needed particular care. Register, login and forgot-password all share a single named limiter keyed by client IP, so any of them running earlier in the same process, including the login calls the other two groups in this file make to obtain tokens, consumes from that same bucket. Rather than trying to guarantee this test runs before every other request that touches those three routes, it clears the rate_limits rows for that limiter's key prefix immediately before firing its own burst of requests, so it gets a clean window regardless of what already ran and in what order. It then sends exactly THROTTLE_AUTH_MAX successful requests followed by one more, and asserts only that last one comes back as 429, which is the actual contract callers depend on rather than an assumption about how many requests came before it in this run.
…as the role A role still assigned to at least one user cannot be deleted, and must fail with 409 rather than either silently succeeding, leaving that user's role_user row pointing at a deleted role, or letting the database's own FK constraint surface as an unhandled 500. Nothing in this branch proved the guard actually runs before the delete, or that a role really is removable once it is no longer assigned to anyone. These tests hit the real test database inside a rolled back global transaction, the same pattern as auth_service.spec.ts, since the rule only means anything once a real pivot row exists to be checked against.
…role has it Mirrors RolesService's rule for roles still assigned to a user: a permission still attached to at least one role cannot be deleted. Without this guard, deleting a permission a role still grants would either violate the role_permission FK constraint as an unhandled 500 or, worse, silently leave that role pointing at a permission row that no longer exists. These tests hit the real test database inside a rolled back global transaction, the same pattern as roles_service.spec.ts, since the rule only means anything once a real role_permission pivot row exists to be checked against.
Two groups. The first proves the full CRUD lifecycle over real
requests, list, detail, update, assign a role, revoke it, delete, and
that every response matches the ApiResponse envelope, not just the
service layer in isolation. The second, and more important one, proves
something no test in this branch had touched yet: that assigning or
revoking a role through the admin API actually changes what a user's
own already-issued JWT is authorized to do on the very next request,
without any re-login.
Getting that wrong in either direction is dangerous. A stale check
that keeps trusting a revoked ADMIN role would let a demoted account
keep acting as an admin until their token expires on its own; one that
never picks up a newly granted role would force every promoted user to
log out and back in before the promotion takes effect. RoleMiddleware
calls user.load('roles') fresh on every request rather than caching
anything from the token itself, and this test is what actually proves
that design choice behaves the way its own code comments claim.
The unit spec for RolesService already proves its 409 guard throws the right error at the service layer, but nothing proved the HTTP layer actually turns it into a 409 response in the ApiResponse envelope rather than falling through to a generic 500, or that the full create, read, update, assign a permission, revoke it, delete cycle a real ADMIN client would perform actually works end to end over real requests rather than only through direct service calls. The second test in this file walks the same 409 scenario through the HTTP layer specifically: create a role, assign it to a user, attempt to delete it and confirm 409, revoke the assignment, then confirm the same delete now succeeds, proving the guard is not just correct in isolation but wired all the way through the controller.
Mirrors roles.spec.ts's reasoning for the permission side: the unit spec already proves PermissionsService.remove's 409 guard throws the right error, but nothing proved the HTTP layer surfaces it correctly or that the create, list, update, delete cycle works end to end. Deletion is confirmed by re-listing rather than a detail route fetch, since the README does not define a GET /api/v1/permissions/:id endpoint and this test respects that rather than assuming one should exist.
…ware
The README requires every user, role and permission administration
route to return 401 without a token and 403 with a non-ADMIN token,
but the CRUD tests in users.spec.ts, roles.spec.ts and
permissions.spec.ts only ever exercise a handful of routes with a
valid ADMIN token, they say nothing about the other sixteen. A route
added later that forgets middleware.role({ roles: ['ADMIN'] }), or
gets it applied in the wrong order relative to middleware.auth(),
would pass every other test in this branch while quietly granting a
regular USER account admin-level access.
This file exercises all seventeen administration routes twice, once
with no token and once with a seeded USER token, table-driven rather
than one test per route, so adding a new admin route later only means
adding one entry to the list instead of writing a new test. The ids in
each URL are placeholders since both checks happen in middleware
before any controller action reads the database, a route rejecting
with 401 or 403 never depends on whether that id actually exists.
…ec.ts
This file's group.setup() only ensured the ADMIN role existed (via
ensureSeededUser('admin')), but its second test registers a fresh
account through the public /api/v1/auth/register endpoint to have a
real user to assign the role under test to. AuthService.register looks
up the USER role by name and throws when it is missing, which the
exception handler turns into a 404, not the 201 this test expected.
Running the full suite exposed this immediately: users.spec.ts already
ensures USER in its own setup, this file never did.
Also casts response.body() at the two POST /api/v1/roles call sites.
That route resolves to a union of every action registered on the same
literal path (create and list), which TypeScript cannot narrow to the
specific shape a given response actually has, unlike the dynamic
:id routes elsewhere in this file that fall back to a loose type on
their own.
…llide POST and GET both resolve to /api/v1/permissions and /api/v1/roles, so response.body() is typed against the union of every action registered on that literal path, and TypeScript cannot narrow .data to the shape a specific call actually returns. npm run typecheck failed on every property access following one of these calls. A small local body() helper casts once at the call site instead of scattering the same inline cast across ten different assertions.
…ests npm run lint flagged formatting-only issues in these three files after the roles.spec.ts and permissions.spec.ts fixes, caught during the same verification pass. No assertions or test behavior changed.
…e same way APP_KEY was declared as Env.schema.secret() on develop before this branch, wrapping the value in a Secret object that redacts itself from logs, console output and error traces. Feature/core-architecture had already weakened it to a plain string() schema, losing that protection for the key backing session and cookie encryption; this branch's code review caught it while auditing everything touching start/env.ts. JWT_SECRET, new in this branch, signs and verifies every issued token and deserves the identical protection: a debugging log statement or a crash dump that happens to serialize env state should never print either secret in plaintext. app/auth/jwt_guard.ts is updated in the next commit to call .release() on the value, the only way to get the raw string back out of a Secret.
JWT_SECRET is now Env.schema.secret(), so env.get() returns a Secret<string> rather than a plain string. TextEncoder.encode has no awareness of Secret the way config/encryption.ts's aes256gcm driver does for APP_KEY, it needs the raw string, so this call site needs an explicit .release() to unwrap it, matching the corresponding env schema change in start/env.ts.
The mapping from a Lucid ModelPaginatorContract to this project's PageResponse<T> envelope had been copy-pasted verbatim into users_service.ts, roles_service.ts and permissions_service.ts, each carrying a comment acknowledging the duplication and predicting the categories, products, customers and orders services would repeat it a fourth and fifth time. The code review flagged this from three independent angles. page_response.ts is where PageResponse<T> itself is defined and its docstring already documented this exact mapping as the pattern every service was expected to reimplement, so exporting the function from here instead gives every current and future list endpoint one implementation to fix instead of a growing set of hand-synced copies. The three services are updated in the following commits to import it.
…nt reload Three fixes to the same file, caught together during code review. toPageResponse now imports from app/helpers/page_response.ts instead of a locally duplicated copy. Email uniqueness checks in update() were case sensitive, since none of the auth or user validators normalize casing, meaning Test@Example.com and test@example.com could coexist as two distinct rows against a column that is only unique per exact string; the email is now lowercased before comparison and before being persisted. assignRole and revokeRole each reloaded the roles relation from the database a second time purely to refresh the in-memory array after an attach or detach whose effect was already fully known, an avoidable round trip since attach only adds the one role already held in memory and detach only removes it; both now mutate the loaded array in place instead. user.roles is Lucid's opaque ManyToMany<typeof Role> at the type level, array-like at runtime but not reassignable, so revokeRole splices the detached role out by index rather than filtering into a new array, which the type checker rejects as an assignment.
…undant reload Same three fixes as the previous commit to users_service.ts, applied to the role side. toPageResponse now imports from the shared helper. create() and update() never checked roleName uniqueness before this, even though roles.role_name carries a UNIQUE constraint at the database level; a duplicate name fell through to a raw Postgres constraint error with no .status property, which app/exceptions/handler.ts's generic branch turns into an unhandled 500 instead of a clean 409, the exact gap UsersService.update already guards against for email. assignPermission and revokePermission each reloaded the permissions relation a second time after an attach or detach whose effect was already known in memory; both now mutate the loaded array in place, splicing by index on the revoke side since role.permissions is Lucid's opaque, non-reassignable ManyToMany type.
Same pagination mapping duplication as users_service.ts and roles_service.ts, now importing toPageResponse from app/helpers/page_response.ts instead of a third locally duplicated copy. Permissions has no uniqueness constraint in the schema (unlike users.email and roles.role_name), so create() needed no equivalent pre-check fix here.
Every list endpoint's index action coerced page and limit with a bare Number(request.input(...)), duplicated identically across three controllers, flagged from two independent code review angles. Beyond the duplication, the coercion itself was unsafe: a non-numeric value produced NaN, which Lucid's paginate() forwarded unchecked into the SQL LIMIT/OFFSET clause, and limit had no upper bound at all, letting a client request limit=999999999 and receive an entire table in one response, exactly what the project's own no-unbounded-queries rule exists to prevent. page and limit are both optional and validated as positive numbers, with limit additionally capped at 100. The three admin controllers are updated in the following commits to validate through this schema instead of coercing the raw query string themselves.
Replaces the bare Number(request.input('page'/'limit', ...)) coercion
with request.validateUsing(paginationSchema), closing the NaN and
unbounded-limit gaps that helper fixes for the whole admin surface.
Same fix as UsersController: request.validateUsing(paginationSchema) replaces the unchecked Number() coercion on page and limit.
Same fix as UsersController and RolesController: request.validateUsing (paginationSchema) replaces the unchecked Number() coercion on page and limit.
The { success: false, message, timestamp, path } shape was hand-built
independently at five call sites: three branches of the global
exception handler, and once each in AuthMiddleware and RoleMiddleware.
respond() already exists in this file as the success-side counterpart;
fail() is the error-side equivalent, so both halves of the ApiResponse
contract live in one place instead of the success path being
centralized while the error path stays scattered. The five call sites
are updated in the following commits.
All three branches of the exception handler now call fail() from
app/helpers/api_response.ts instead of hand-building the same error
envelope three times. Also simplifies a redundant condition: the HTTP
exception branch cast error to { status?: number } and then checked
both the cast field's truthiness and typeof httpError.status ===
'number', two clauses expressing the same guard, since the cast type
already declares status as number | undefined. A single typeof check
covers both "present" and "numeric" in one clause.
…count Uses fail() from app/helpers/api_response.ts instead of hand-building the error envelope inline, matching the exception handler's own fix. More importantly, this closes a real gap the code review caught: this middleware only ever checked the jti blacklist, never enabled or accountLocked, so a JWT issued before an ADMIN disabled or locked that account via PUT /api/v1/users/:id would keep authenticating successfully against every protected route until the token's own natural expiry. AuthService only checks those flags at login and refresh time, not on every request, so nothing else in the request pipeline was catching this. The check runs immediately after authenticateUsing() resolves the user, before the blacklist lookup, since there is no reason to query blacklisted_tokens for a request that is going to be rejected anyway.
Same dedup as the exception handler and AuthMiddleware: the 403 response now calls fail() from app/helpers/api_response.ts instead of hand-building the same envelope shape a fifth time.
… mail errors Three fixes to the account lifecycle, caught during code review. register() never checked email uniqueness before User.create(), unlike UsersService.update which already does for the same column; a duplicate registration fell through to a raw Postgres unique constraint violation with no .status property, which the exception handler's generic branch turns into an unhandled 500 instead of a clean 409. None of register, login or forgotPassword normalized email casing either, so Ed@Example.com and ed@example.com could register as two distinct accounts, and logging in with different casing than what was registered would fail as invalid credentials even with the correct password; every lookup and write now lowercases the email first. mail.send() in both register() and forgotPassword() was previously unguarded. Both the user row and the activation or reset token are already committed to the database by the time mail is sent, so an SMTP failure (Mailhog unreachable, for example) must not turn an otherwise- successful request into a 500 the client would retry straight into the new E_EMAIL_TAKEN conflict, unable to activate an account they cannot obtain a fresh token for. Both sends are now wrapped in try/catch and logged rather than left to propagate.
…ector None of registerSchema, loginSchema or resetPasswordSchema bounded password length. config/hash.ts hashes with Argon2id, whose cost scales with input size, and register and forgot-password are public, unauthenticated endpoints; a client could submit a multi-megabyte password field (config/bodyparser.ts allows large JSON bodies) and force expensive hashing work per request, a cost-amplification vector authThrottle's request-count limit alone does not prevent since it caps how often a client can hit the endpoint, not how expensive each hit can be. maxLength(128) is added to all three password fields, including login's, which intentionally still accepts any password shorter than that cap without a complexity check, since login validates a credential against whatever is already stored and rejecting a short password before it reaches AuthService.login would turn a wrong password into a confusing 422 instead of the expected 400 E_INVALID_CREDENTIALS.
registerSchema only bounds firstName and lastName by length, per the README's Auth Model column widths, not by character set, so a value like <img src=x onerror=alert(1)> passes validation. ActivationMail and PasswordResetMail interpolate firstName directly into their .html() bodies, so an unescaped value would render live in whatever HTML capable client opens the mail, Mailhog in every environment this project runs in, a stored injection vector reachable from unauthenticated registration input. The following two commits apply this helper at both call sites.
Closes the injection vector app/mails/escape_html.ts was added for. The plain text body needs no change, there is no markup there to inject into.
Same fix as ActivationMail, closing the same injection vector in the other mail class that interpolates firstName into an HTML body.
… check AuthMiddleware.handle now reads user.enabled and user.accountLocked immediately after authenticateUsing() resolves the user, but this file's fake HttpContext never actually returned a user from its authenticateUsing mock, only undefined, which the middleware's new check threw against. The mock now accepts and returns a real user created in the test database. A third test is added specifically for the new behavior: a request carrying a valid, non-blacklisted jti for an account with accountLocked set is still rejected, since neither of the two existing tests exercised that branch, only the jti blacklist check the file's own docstring says it exists to pin down.
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
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
This branch implements the full account lifecycle described in the README's feature/auth section: a custom JWT guard since the installed @adonisjs/auth ships no JWT support natively, Argon2 password hashing, rate limiting on the auth routes most likely to be abused, and register, activate, login, logout, refresh, forgot-password and reset-password all working end to end against a real Postgres and Mailhog instance. Registration and password reset now send real emails through SMTP rather than returning the raw token in the HTTP response, since a token a client can read is a token anyone reading that response can also use.
Two things extend beyond the README's original feature/auth scope, both added after discussion mid-branch and documented directly in the README itself. The first is a full ADMIN-only administration surface for users, roles and permissions, list, detail, update, delete, and assigning or revoking a role on a user or a permission on a role, since nothing in the original plan had any way to promote an account to ADMIN or manage the authorization model at all beyond direct database access. The second is the email delivery itself, replacing the original placeholder of echoing the activation and reset tokens in the API response.
A multi-angle code review after the bulk of the branch was implemented surfaced a substantial list of real findings, including a case-sensitive email column with no normalization anywhere in the auth flow, unbounded pagination limits on every admin list endpoint, a JWT that kept authenticating successfully after an ADMIN disabled or locked the account it belonged to, an unescaped HTML injection vector in the two mail templates, duplicate registration and role creation falling through to raw 500s instead of clean conflict responses, an Argon2 cost-amplification vector from unbounded password length, and a regression from develop where APP_KEY had lost its Secret redaction. All of these are fixed on this branch; the review's remaining lower-priority architectural findings, such as Bouncer sitting fully wired but unused alongside RoleMiddleware's own role check, are noted but left for a later branch to reconcile deliberately rather than restructured under this PR's time budget.
Task checklist
Every task in the README's feature/auth section is complete, including the extended administration and email delivery scope. JWT guard, Bouncer and Limiter are installed and configured; config/hash.ts uses Argon2 as the default hasher, driven by HASH_DRIVER. AuthMiddleware authenticates and rejects a blacklisted or revoked jti, and additionally rejects a disabled or locked account on every request, not just at login. RoleMiddleware preloads roles and checks them against the route's required roles. AuthService covers register, activate, login, logout, refresh, forgot password and reset password, with email delivery through Mailhog in every environment. Rate limiting is applied to register, login and forgot-password. The administration endpoints for users, roles and permissions are fully built with their own validators, services and controllers, all ADMIN only, with the same pagination and conflict-handling conventions as the rest of the API. Every listed unit and functional test, including the auth lifecycle, the middleware smoke test, the rate limiting test, and the full administration CRUD and authorization matrix, is written and passing.
Test plan
npm run typecheck, npm run lint and npm test all pass locally against a live PostgreSQL and Mailhog stack, 41 tests passing with zero failures, verified twice: once against an incrementally built local database, and once against a database reset to a single fresh migration batch to match what a clean CI run will see. Registration and password reset were also manually verified end to end against a running dev server, confirming the activation and reset emails actually land in Mailhog with the correct token, before any automated test was written against that flow.