Feat/improvement for v 0 2 - #1
Merged
Merged
Conversation
…across controllers and services Every hono `Context` call inside the package consuming the `AppVariables` now
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The check only matched @donilite/wrap/testing's testContext() test double (a plain object whose req.json/query/formData are own-enumerable properties). Against a real Hono Context, HonoRequest.json()/query() live on the prototype, so Object.keys(c.req) never contains them and every @ValidateDTO-decorated call (BaseService.create/update) threw "must receive the Hono context" on real HTTP requests — masked in tests because they only ever went through testContext(). Now checks `instanceof Context` first (stable across Hono versions, unlike sniffing method names on the request object), falling back to the duck-typed check so the test double keeps working.
createAuth() was a rigid, JWT+cookie-specific function — extending it for any other auth paradigm meant copying it wholesale. AuthController is now the real source of truth: an abstract class that only mandates authenticate()/revoke(); the guard shape (authMiddleware) is identical across paradigms so it's implemented once via Hono's own createMiddleware() helper. Authorization is a generic guard(predicate) over the resolved identity — roles, permissions, scopes, tenant checks are all equally first-class, and the framework never assumes RBAC. Concrete presets are free to constructor-inject a repository/service, so auth can participate in the same entity/event/transaction machinery as the rest of the app instead of being stuck stateless. JwtCookieAuthController is the reference preset: today's bearer/cookie behavior, with requireRoles() as its own convenience built on guard() rather than a framework mandate. createAuth() becomes a thin deprecated shim delegating to it. decorators/access.ts's @can now reads c.get("identity") — the one canonical context key — instead of the JWT-flavored "jwtPayload". The identity shape is typed through the registry, the same declaration-merging mechanism already used for schema/variables/roles: `declare module "@donilite/wrap" { interface WrapRegistry { identity: MyShape } }`. AppVariables merges identity in automatically so c.get("identity") is typed everywhere without every consumer redeclaring it.
…us offline-first sync BaseController and BaseService forced a CRUD service/repository generic even on features that don't need one (health checks, root routes, orchestration services). RouterController now holds the route-scanning/ middleware-assembly logic on its own — BaseController<Service> extends it, adding only the service field, so a controller can extend RouterController directly with no fake service. WrapService is the equivalent split for services without a repository. RouterController also gains register() — the same child-controller composition primitive as Wrap (mount at the child's own @controller basePath, optionally prefixed), so controllers can compose children under themselves and it works from registerCustomRoutes() too. register()'s middlewares option scopes a middleware to the whole mount (including the mount's own bare path): it's applied on the PARENT app before .route() attaches the child, which is what makes it actually guard the child's routes regardless of when the child registered them on itself. joinPath/mountController are the shared helpers behind both Wrap.register() and this. BaseRepository gains findChangedSince()/applyBatch() for offline-first sync (mobile app stores locally, syncs to backend without going through the DB directly): cursor pull ordered by updatedAt, including soft-deleted rows as tombstones (BaseRow's $onUpdate already bumps updatedAt on any set() call, including the soft-delete's { deletedAt } update); batch push with last-write-wins conflict resolution — a client change older than the server's current updatedAt is reported back as a conflict and skipped rather than applied. Deletes in applyBatch go through the same soft-delete path as reads so they surface as tombstones, not the repository's hard delete(). BaseService forwards both, same pattern as findById/findAll.
Path params like /:id never showed up in the generated OpenAPI spec —
they were only ever added from an explicit route.params option that
nothing in the codebase actually set. Parameters are now derived from
the route path itself (:name and Hono's :name{regex} constrained
syntax), with any explicit route.params entry only overriding
type/description. normalizePath's :id -> {id} conversion is fixed the
same way — it previously mangled regex-constrained params into
{id{[0-9]+}} instead of {id}.
"Does this route need auth" detection moves from sniffing middleware
function names (mw.name === "authMiddleware", breaks on bound/wrapped
functions) to a WRAP_AUTH_MIDDLEWARE tag set once by AuthController.
Security schemes in the generated spec now come from the registered
AuthController's openApiSecurityScheme() instead of a hardcoded
bearer+cookie pair, falling back to that pair when no AuthController is
registered. SwaggerGenerator takes an AuthController instance (not the
class) to match Wrap.with()'s ergonomics — AuthControllerClass is a
narrow structural type covering just the static hook, since typeof
AuthController would also encode the constructor's argument list,
which varies per preset and broke assignability for no reason.
setupSwaggerUI/setupSwagger forward UI options to @hono/swagger-ui, so
an app can enable withCredentials/persistAuthorization for cookie-
session "Try it out" support.
Every generated project hand-wired the same boilerplate in its entrypoint: new Hono(), cors/secureHeaders/requestId/bodyLimit, app.onError(errorHandler()), app.notFound(...), setupSwagger(...) — and still had to import and mount every feature's Hono sub-app manually. Wrap owns all of that: constructing it wires the standard middleware stack and error/404 handling; .with() registers a global middleware or the app's AuthController (made available to .swagger() without being auto-applied globally); .register() mounts a controller at its own @controller basePath (optionally with scoped middlewares, see the previous commit); .swagger() wires OpenAPI + UI, deferring to the registered AuthController for security schemes and defaulting to credentialed/persisted "Try it out". .get/.post/.put/.patch/.delete/.use/.request are thin passthroughs to the underlying Hono app — ad-hoc routes, path-scoped middleware and direct testing (app.request(...)) don't need the escape hatch. .raw stays for what genuinely needs the raw Hono instance (realtime websocket upgrade, a custom Bun.serve topology).
src/index.ts now composes the app through Wrap instead of hand-rolling
Hono: cors config in, auth registered via .with(), the admin guard via
.use("/admin/*", ...), swagger via .swagger(), realtime wired through
the raw escape hatch (it needs the underlying Hono instance and the
Bun.serve server handle directly) and .get() for the ergonomic routes.
IndexController is the single controller registered on Wrap; it
composes ExampleController as a child via register() in its own
constructor — parent-child controller composition, not a flat list of
independently Wrap-registered controllers. ExampleController's
@controller basePath ("/api/examples") stays the one place that path
is declared. example.app.ts is gone — register() replaces what it did
by hand.
middleware/auth.ts constructs a JwtCookieAuthController instead of
calling the deprecated createAuth(); web.factory.ts's webFactory is
now typed with the framework-merged AppVariables instead of the app's
own narrower Variables, since every controller's Hono app needs to
agree with what AuthController/Wrap expect (Variables stays what the
app registers into WrapRegistry — the two are different consumers of
the same declaration).
…ation None of tonight's new surface had automated coverage: Wrap itself, AuthController/JwtCookieAuthController, the sync methods, and the swagger fixes were only exercised by throwaway verification scripts during development. The existing suite also bypassed the real composition path — example.controller.test.ts builds new ExampleController().getApp() directly, never going through Wrap. wrap.test.ts boots a real Wrap, registers IndexController (which registers ExampleController as its own child) and drives requests through app.raw/app.request — the actual src/index.ts wiring, not a bypass — plus register()'s middlewares option guarding a whole mount including its bare path. auth.test.ts exercises JwtCookieAuthController end-to-end over real HTTP (bearer, sliding cookie sessions, requireRoles, revoke, wrong- secret rejection) rather than unit-testing pieces in isolation. example.repository.sync.test.ts covers findChangedSince pagination and cursor exclusivity, and applyBatch's conflict-vs-applied outcomes including the soft-delete tombstone path. swagger.test.ts covers the path-param derivation bug fix (plain and regex-constrained), AuthController-driven security schemes, and that only routes actually tagged by AuthController middleware get a security block.
… greater compatibility
Reviewer's GuideIntroduces a new Wrap composition root and RouterController abstraction, refactors auth into a reusable AuthController/JwtCookieAuthController model with identity-aware context and OpenAPI integration, adds offline-first sync support to BaseRepository/BaseService, enhances Swagger generation and templates to use the new architecture, and adds tests for sync, auth, swagger, and Wrap wiring. Sequence diagram for auth middleware and Swagger security integrationsequenceDiagram
actor Dev
participant Wrap
participant JwtCookieAuthController as Auth
participant SwaggerGenerator as Swagger
participant Hono as App
Dev->>Wrap: new Wrap(options)
Dev->>Auth: new JwtCookieAuthController(options)
Dev->>Wrap: with(auth)
Wrap->>Wrap: store AuthController instance
Dev->>Wrap: swagger(config, uiOptions)
Wrap->>Swagger: new SwaggerGenerator(config, auth)
Swagger->>Auth: openApiSecurityScheme()
Auth-->>Swagger: securitySchemes
Swagger->>App: setupSwaggerUI(app, path, uiOptions)
%% Request-time auth flow
actor User
participant Route as ProtectedRoute
User->>App: HTTP request to guarded path
App->>Auth: authMiddleware
Auth->>Auth: authenticate(c)
alt unauthenticated
Auth-->>App: 401 Unauthorized
else authenticated
Auth->>App: c.set("identity", identity)
App->>Route: handler(c)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
AuthController.withand related wiring, relying oninstanceof AuthControllerto detect auth plugins could be brittle if multiple copies of the library or different module contexts are loaded; consider using a branded symbol or static marker on the class instead ofinstanceoffor more robust detection. - The new
BaseRepository.applyBatchprocesses the batch sequentially without any transactional guardrails; if you expect batches to be atomic, consider wrapping the loop in a transaction (or making it pluggable) so that partial failures don’t leave the server-side state only partially applied.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AuthController.with` and related wiring, relying on `instanceof AuthController` to detect auth plugins could be brittle if multiple copies of the library or different module contexts are loaded; consider using a branded symbol or static marker on the class instead of `instanceof` for more robust detection.
- The new `BaseRepository.applyBatch` processes the batch sequentially without any transactional guardrails; if you expect batches to be atomic, consider wrapping the loop in a transaction (or making it pluggable) so that partial failures don’t leave the server-side state only partially applied.
## Individual Comments
### Comment 1
<location path="packages/create-wrap/template/src/middleware/auth.ts" line_range="17" />
<code_context>
});
-export const { authMiddleware, setupCookieSession, clearCookieSession } = auth;
+export const authMiddleware = auth;
+export const setupCookieSession = auth.setupCookieSession.bind(auth);
+export const clearCookieSession = auth.clearCookieSession.bind(auth);
</code_context>
<issue_to_address>
**issue (bug_risk):** Exporting `authMiddleware` as the controller instance will break middleware usage
`auth` is a `JwtCookieAuthController` instance, not a `MiddlewareHandler`. Exporting `authMiddleware = auth` changes the public API: any code doing `app.use('/admin/*', authMiddleware)` will now receive an object instead of a function and fail at runtime.
To preserve the previous behavior, export the controller’s middleware function instead:
```ts
export const authMiddleware = auth.authMiddleware;
```
and keep `auth` as the controller instance for `app.with(auth)` and chaining.
</issue_to_address>
### Comment 2
<location path="packages/wrap/src/decorators/validation.ts" line_range="42-50" />
<code_context>
+ // stable check — but it doesn't match the plain-object test double
+ // from `@donilite/wrap/testing`'s `testContext()`, so that shape is
+ // still accepted via the duck-typed fallback below.
+ const c: Context<{ Variables: AppVariables }> | undefined = args.find(
(arg) =>
- arg &&
- typeof arg === "object" &&
- "req" in arg &&
- typeof (arg as any).json === "function",
+ arg instanceof Context ||
+ (arg &&
+ typeof arg === "object" &&
+ "req" in arg &&
+ arg.req &&
+ typeof arg.req === "object" &&
+ Object.keys(arg.req).includes(provider)),
);
</code_context>
<issue_to_address>
**issue (bug_risk):** The new context detection logic may miss valid test contexts and is brittle
The new fallback relies on `Object.keys(arg.req).includes(provider)`, but real Hono `Context` instances expose methods like `json`/`formData` on the prototype, not as own enumerable properties. That means the detection effectively depends only on the `instanceof Context` branch, and third-party or custom test doubles may be skipped if they use non-enumerable properties or different method names.
To keep the fast-path while preserving the previous duck-typing behaviour, consider checking for a callable `req[provider]` instead of enumerating keys:
```ts
const c = args.find((arg) =>
arg instanceof Context ||
(arg && typeof arg === 'object' &&
'req' in arg && arg.req && typeof arg.req === 'object' &&
typeof (arg.req as any)[provider] === 'function')
) as Context<{ Variables: AppVariables }> | undefined;
```
This keeps the detection robust for both real contexts and compatible mocks.
</issue_to_address>
### Comment 3
<location path="packages/wrap/src/base.repository.ts" line_range="552-554" />
<code_context>
+ options?: { limit?: number },
+ ): Promise<SyncPage<InferEntity<Tb>>> {
+ const updatedAtColumn = this.requireColumn("updatedAt");
+ const cursorDate = typeof cursor === "string" ? new Date(cursor) : cursor;
+ const limit = options?.limit ?? 200;
+
+ const rows = (await this.db
+ .select()
+ .from(this.table as PgTable)
+ .where(gt(updatedAtColumn, cursorDate))
+ .orderBy(asc(updatedAtColumn))
+ .limit(limit)) as InferEntity<Tb>[];
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Lack of validation for invalid cursor dates can lead to surprising sync behaviour
`new Date(cursor)` may yield `Invalid Date` for malformed strings, which is then passed into `gt(updatedAtColumn, cursorDate)`. Depending on the driver, this could throw or produce incorrect result sets (e.g., all or no rows). Consider validating the parsed date and failing fast, for example by checking `Number.isNaN(cursorDate.getTime())` and throwing a clear error when the cursor is invalid to avoid inconsistent sync windows.
```suggestion
const updatedAtColumn = this.requireColumn("updatedAt");
const cursorDate =
typeof cursor === "string" ? new Date(cursor) : cursor;
if (Number.isNaN(cursorDate.getTime())) {
throw new Error(
`Invalid cursor date passed to findChangedSince: ${String(cursor)}`,
);
}
const limit = options?.limit ?? 200;
```
</issue_to_address>
### Comment 4
<location path="packages/create-wrap/template/tests/auth.test.ts" line_range="46-23" />
<code_context>
+ return { token, cookie: cookie.split(";")[0] ?? "" };
+}
+
+describe("JwtCookieAuthController", () => {
+ it("rejects requests without a token", async () => {
+ const auth = new JwtCookieAuthController({ secret: SECRET });
+ const res = await buildApp(auth).request("/private");
+ expect(res.status).toBe(401);
+ });
+
+ it("accepts a bearer token and exposes the identity", async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for the generic guard() API with a non-role-based predicate
Since `guard()` is the core authorization primitive (with `requireRoles` as a thin wrapper), please add a test that wires up `auth.guard(identity => identity.userId === "some-id")` to a route and asserts:
- A request whose `userId` does not match gets a 403
- A request with the matching `userId` succeeds
This will exercise the generic guard path (including `c.get("identity")`, the default 403, and `WRAP_AUTH_MIDDLEWARE` tagging) so we catch regressions even if `requireRoles` is refactored.
Suggested implementation:
```typescript
describe("JwtCookieAuthController", () => {
it("rejects requests without a token", async () => {
const auth = new JwtCookieAuthController({ secret: SECRET });
const res = await buildApp(auth).request("/private");
expect(res.status).toBe(401);
});
it("accepts a bearer token and exposes the identity", async () => {
const auth = new JwtCookieAuthController({ secret: SECRET });
const { token } = await loginAs(auth, "u1", UserRoles.ADMIN);
const res = await buildApp(auth).request("/private", {
headers: { Authorization: `Bearer ${token}` },
});
expect(res.status).toBe(200);
const body = (await res.json()) as {
identity: { userId: string; role: string };
};
expect(body.identity.userId).toBe("u1");
expect(body.identity.role).toBe(UserRoles.ADMIN);
});
it("applies generic guard() predicates and returns 403 by default", async () => {
const auth = new JwtCookieAuthController({ secret: SECRET });
// Predicate only allows a specific userId
const predicate = (identity: { userId: string }) => identity.userId === "allowed-user";
const app = buildAppWithGuard(auth, predicate);
// First, a user that does NOT match the predicate should be rejected with 403
const { token: otherToken } = await loginAs(auth, "other-user", UserRoles.ADMIN);
const forbiddenResponse = await app.request("/guarded", {
headers: { Authorization: `Bearer ${otherToken}` },
});
expect(forbiddenResponse.status).toBe(403);
// Then, a user that matches the predicate should be allowed
const { token: allowedToken } = await loginAs(auth, "allowed-user", UserRoles.ADMIN);
const okResponse = await app.request("/guarded", {
headers: { Authorization: `Bearer ${allowedToken}` },
});
expect(okResponse.status).toBe(200);
});
```
To fully implement `buildAppWithGuard` so this test exercises the generic `guard()` path (including `c.get("identity")`, the default 403, and `WRAP_AUTH_MIDDLEWARE` tagging), you should:
1. **Define `buildAppWithGuard` in this test file** (near `buildApp` / `loginAs`) roughly as:
- Create the app using the same framework used by `buildApp` (e.g. `new Hono()`).
- Apply your auth middleware (`WRAP_AUTH_MIDDLEWARE`) in the same way `buildApp` does.
- Register a route `/guarded` with `auth.guard(predicate)` before the handler, e.g.:
```ts
function buildAppWithGuard(
auth: JwtCookieAuthController,
predicate: (identity: { userId: string }) => boolean,
) {
const app = new Hono();
// This should mirror how buildApp wires the auth middleware,
// including any WRAP_AUTH_MIDDLEWARE tagging.
app.use("*", auth.wrap());
app.get(
"/guarded",
auth.guard(predicate),
(c) => {
const identity = c.get("identity");
return c.json({ ok: true, identity });
},
);
return app;
}
```
Adjust `Hono`, `auth.wrap()`, or middleware wiring to match how `buildApp` is currently implemented in your codebase.
2. **Ensure the default 403 behavior is exercised**:
- The `auth.guard` implementation should *not* call `next()` when the predicate fails and should set a 403 status (or delegate to a shared `forbidden` helper). The first part of the test (`other-user`) is asserting that.
- The route handler should only be reached when `auth.guard` calls `next()` for an allowed identity (`allowed-user`).
3. **Reuse existing imports/conventions**:
- Import `Hono` or the equivalent app type only if this test file isn’t already importing it.
- Use the same `WRAP_AUTH_MIDDLEWARE` tagging mechanism as in `buildApp` so that the test truly covers that path.
These adjustments will ensure the new test validates the generic `guard()` authorization primitive independently of role-based helpers like `requireRoles`.
</issue_to_address>
### Comment 5
<location path="packages/create-wrap/template/tests/auth.test.ts" line_range="69-80" />
<code_context>
+ expect(body.identity.role).toBe(UserRoles.ADMIN);
+ });
+
+ it("accepts a session cookie and slides (refreshes) it", async () => {
+ const auth = new JwtCookieAuthController({ secret: SECRET });
+ const { cookie } = await loginAs(auth, "u2", UserRoles.USER);
+ expect(cookie).toContain("session=");
+
+ const res = await buildApp(auth).request("/private", {
+ headers: { Cookie: cookie },
+ });
+ expect(res.status).toBe(200);
+ // authMiddleware re-signs and refreshes the cookie on every request.
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting that identity is set on the context for cookie-based auth as well
To fully verify cookie-based auth from the app’s perspective, also assert that `identity` is populated on the cookie-based `/private` request. For example, parse the JSON body and check that `body.identity.userId` and `body.identity.role` match the logged-in user, mirroring the bearer-token test and exercising `setupCookieSession`’s `c.set("identity", ...)` in this flow.
```suggestion
it("accepts a session cookie and slides (refreshes) it", async () => {
const auth = new JwtCookieAuthController({ secret: SECRET });
const { cookie } = await loginAs(auth, "u2", UserRoles.USER);
expect(cookie).toContain("session=");
const res = await buildApp(auth).request("/private", {
headers: { Cookie: cookie },
});
expect(res.status).toBe(200);
const body = (await res.json()) as {
identity: { userId: string; role: string };
};
expect(body.identity.userId).toBe("u2");
expect(body.identity.role).toBe(UserRoles.USER);
// authMiddleware re-signs and refreshes the cookie on every request.
expect(res.headers.get("set-cookie")).toContain("session=");
});
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| app.post("/logout", (c) => { | ||
| auth.revoke(c); | ||
| return c.json({ ok: true }); | ||
| }); |
There was a problem hiding this comment.
suggestion (testing): Add a test for the generic guard() API with a non-role-based predicate
Since guard() is the core authorization primitive (with requireRoles as a thin wrapper), please add a test that wires up auth.guard(identity => identity.userId === "some-id") to a route and asserts:
- A request whose
userIddoes not match gets a 403 - A request with the matching
userIdsucceeds
This will exercise the generic guard path (including c.get("identity"), the default 403, and WRAP_AUTH_MIDDLEWARE tagging) so we catch regressions even if requireRoles is refactored.
Suggested implementation:
describe("JwtCookieAuthController", () => {
it("rejects requests without a token", async () => {
const auth = new JwtCookieAuthController({ secret: SECRET });
const res = await buildApp(auth).request("/private");
expect(res.status).toBe(401);
});
it("accepts a bearer token and exposes the identity", async () => {
const auth = new JwtCookieAuthController({ secret: SECRET });
const { token } = await loginAs(auth, "u1", UserRoles.ADMIN);
const res = await buildApp(auth).request("/private", {
headers: { Authorization: `Bearer ${token}` },
});
expect(res.status).toBe(200);
const body = (await res.json()) as {
identity: { userId: string; role: string };
};
expect(body.identity.userId).toBe("u1");
expect(body.identity.role).toBe(UserRoles.ADMIN);
});
it("applies generic guard() predicates and returns 403 by default", async () => {
const auth = new JwtCookieAuthController({ secret: SECRET });
// Predicate only allows a specific userId
const predicate = (identity: { userId: string }) => identity.userId === "allowed-user";
const app = buildAppWithGuard(auth, predicate);
// First, a user that does NOT match the predicate should be rejected with 403
const { token: otherToken } = await loginAs(auth, "other-user", UserRoles.ADMIN);
const forbiddenResponse = await app.request("/guarded", {
headers: { Authorization: `Bearer ${otherToken}` },
});
expect(forbiddenResponse.status).toBe(403);
// Then, a user that matches the predicate should be allowed
const { token: allowedToken } = await loginAs(auth, "allowed-user", UserRoles.ADMIN);
const okResponse = await app.request("/guarded", {
headers: { Authorization: `Bearer ${allowedToken}` },
});
expect(okResponse.status).toBe(200);
});To fully implement buildAppWithGuard so this test exercises the generic guard() path (including c.get("identity"), the default 403, and WRAP_AUTH_MIDDLEWARE tagging), you should:
-
Define
buildAppWithGuardin this test file (nearbuildApp/loginAs) roughly as:- Create the app using the same framework used by
buildApp(e.g.new Hono()). - Apply your auth middleware (
WRAP_AUTH_MIDDLEWARE) in the same waybuildAppdoes. - Register a route
/guardedwithauth.guard(predicate)before the handler, e.g.:
function buildAppWithGuard( auth: JwtCookieAuthController, predicate: (identity: { userId: string }) => boolean, ) { const app = new Hono(); // This should mirror how buildApp wires the auth middleware, // including any WRAP_AUTH_MIDDLEWARE tagging. app.use("*", auth.wrap()); app.get( "/guarded", auth.guard(predicate), (c) => { const identity = c.get("identity"); return c.json({ ok: true, identity }); }, ); return app; }
Adjust
Hono,auth.wrap(), or middleware wiring to match howbuildAppis currently implemented in your codebase. - Create the app using the same framework used by
-
Ensure the default 403 behavior is exercised:
- The
auth.guardimplementation should not callnext()when the predicate fails and should set a 403 status (or delegate to a sharedforbiddenhelper). The first part of the test (other-user) is asserting that. - The route handler should only be reached when
auth.guardcallsnext()for an allowed identity (allowed-user).
- The
-
Reuse existing imports/conventions:
- Import
Honoor the equivalent app type only if this test file isn’t already importing it. - Use the same
WRAP_AUTH_MIDDLEWAREtagging mechanism as inbuildAppso that the test truly covers that path.
- Import
These adjustments will ensure the new test validates the generic guard() authorization primitive independently of role-based helpers like requireRoles.
Comment on lines
+69
to
+80
| it("accepts a session cookie and slides (refreshes) it", async () => { | ||
| const auth = new JwtCookieAuthController({ secret: SECRET }); | ||
| const { cookie } = await loginAs(auth, "u2", UserRoles.USER); | ||
| expect(cookie).toContain("session="); | ||
|
|
||
| const res = await buildApp(auth).request("/private", { | ||
| headers: { Cookie: cookie }, | ||
| }); | ||
| expect(res.status).toBe(200); | ||
| // authMiddleware re-signs and refreshes the cookie on every request. | ||
| expect(res.headers.get("set-cookie")).toContain("session="); | ||
| }); |
There was a problem hiding this comment.
suggestion (testing): Consider asserting that identity is set on the context for cookie-based auth as well
To fully verify cookie-based auth from the app’s perspective, also assert that identity is populated on the cookie-based /private request. For example, parse the JSON body and check that body.identity.userId and body.identity.role match the logged-in user, mirroring the bearer-token test and exercising setupCookieSession’s c.set("identity", ...) in this flow.
Suggested change
| it("accepts a session cookie and slides (refreshes) it", async () => { | |
| const auth = new JwtCookieAuthController({ secret: SECRET }); | |
| const { cookie } = await loginAs(auth, "u2", UserRoles.USER); | |
| expect(cookie).toContain("session="); | |
| const res = await buildApp(auth).request("/private", { | |
| headers: { Cookie: cookie }, | |
| }); | |
| expect(res.status).toBe(200); | |
| // authMiddleware re-signs and refreshes the cookie on every request. | |
| expect(res.headers.get("set-cookie")).toContain("session="); | |
| }); | |
| it("accepts a session cookie and slides (refreshes) it", async () => { | |
| const auth = new JwtCookieAuthController({ secret: SECRET }); | |
| const { cookie } = await loginAs(auth, "u2", UserRoles.USER); | |
| expect(cookie).toContain("session="); | |
| const res = await buildApp(auth).request("/private", { | |
| headers: { Cookie: cookie }, | |
| }); | |
| expect(res.status).toBe(200); | |
| const body = (await res.json()) as { | |
| identity: { userId: string; role: string }; | |
| }; | |
| expect(body.identity.userId).toBe("u2"); | |
| expect(body.identity.role).toBe(UserRoles.USER); | |
| // authMiddleware re-signs and refreshes the cookie on every request. | |
| expect(res.headers.get("set-cookie")).toContain("session="); | |
| }); |
…eware
export const authMiddleware = auth; exported the JwtCookieAuthController
instance itself instead of auth.authMiddleware — any code doing
app.use("/admin/*", authMiddleware) would receive an object instead of
a function and fail at runtime. Regression from the AuthController
rewrite, caught by Sourcery's review on PR #1.
…umeration Object.keys(arg.req).includes(provider) only matches when the property is own-enumerable. testContext()'s req methods happen to be, but a custom test double using getters, Object.defineProperty, or a differently-shaped mock wouldn't be detected. Checking typeof arg.req[provider] === "function" verifies the same thing (the double can actually produce a body) without depending on how the property happens to be defined. Flagged by Sourcery's review on PR #1.
new Date(cursor) silently produces an Invalid Date on a malformed string, which would then feed a comparison the driver may throw on or silently mis-evaluate — findChangedSince and applyBatch both now fail fast with a clear error instead. applyBatch also processed its changes as separate, independent statements: an exception partway through (e.g. a constraint violation on change N) left every change already applied in the batch committed, with the rest never running — a partially-applied sync batch. The whole batch now runs inside withTransaction(), so a failure rolls back everything applied so far in that call; conflicts don't throw, so they still land in the same commit as whatever else in the batch succeeded. Both flagged by Sourcery's review on PR #1. Co-Auth
…allback chain Lets an app mix strategies instead of picking exactly one: e.g. cookie sessions for browser requests, falling back to a legacy/API-key controller for clients that can't store cookies. AuthController.combine(...controllers) returns a controller whose authenticate() tries each delegate in turn and returns the first non-null identity; revoke() runs on every delegate (best-effort, same no-op-when-inapplicable contract every preset's own revoke() already needs); openApiSecurityScheme() merges every delegate's schemes. Because combining only needs the public AuthController contract, the delegates can come from entirely separate packages — this is meant to be the seam a community of auth-paradigm packages hangs off of. Merging security schemes from a dynamic set of delegates is exactly why openApiSecurityScheme() moves from static to an instance method: a static method has no way to reach a specific combined instance's list of wrapped controllers, only the class. SwaggerGenerator simplifies along with it — it now just holds the AuthController instance directly instead of extracting and re-typing its constructor (the AuthControllerClass structural type this replaces existed solely to work around the static method). Also swaps Wrap.with()'s `instanceof AuthController` for a branded `isAuthController()` check (Sourcery flagged this on PR #1): instanceof breaks if two copies of this module ever end up loaded (mismatched nested dependency versions, separate bundles) since each copy's class reference is a different object. The brand uses Symbol.for (the global symbol registry) instead of a module-scoped Symbol() for the same reason — WRAP_AUTH_MIDDLEWARE (swagger's route-tagging symbol) moves to Symbol.for too, for consistency and the same resilience.
auth.test.ts previously only exercised requireRoles (the role-based convenience) and never the generic guard() primitive it's built on — added a route guarded by an arbitrary userId predicate and a test asserting the 403/200 split independent of roles, plus identity assertions on the cookie-session request (was only asserted for bearer). Both gaps flagged by Sourcery's review on PR #1. auth.combine.test.ts covers the new combine() feature end-to-end: a hand-rolled LegacyHeaderAuthController (deliberately not part of the framework, standing in for a third-party paradigm package) combined with JwtCookieAuthController — authenticating via whichever delegate resolves an identity, falling back correctly when the first can't, rejecting when neither can, revoke() reaching every delegate, merged security schemes, and the empty-call guard.
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 Sourcery
Introduce a new Wrap composition root with configurable auth integration and Swagger generation, refactor controllers/services to support non-CRUD routing, and add offline-first sync support plus tests.
New Features:
Bug Fixes:
Enhancements:
Tests: