mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-19 05:26:47 +00:00
refactor(server): plan 4 — unify the trip-access predicate's Kysely copies
TripsRepository.findForViewer/listForUser/activeTrip each hand-wrote the identical trip-access boolean (t.user_id = ? OR m.user_id IS NOT NULL) through the Kysely expression builder. Task 7's security review (M1) evaluated a shared helper and set it aside for time, falling back to the TRIPREPO-041 cross-method parity test as a regression guard instead. Extracted the boolean into one generic helper, tripAccessExpr (_shared/trip-access.ts), following the reservation-visibility.ts publicReservationExpr precedent: fully generic over <DB, TB> since the predicate has no correlated subquery, so it composes across TripSelectKyselyDB and ActiveTripKyselyDB without needing a narrower fixed-alias shape. The LEFT JOIN itself stays hand-written at each call site — tripSelectQuery's own docstring already explains why a shared join helper can't be typed across these differently-joined outer queries. feeds.service.ts and memories-access.service.ts no longer duplicate this predicate at all (Task 2's facade-inline sweep already converted both onto TripsRepository.findAccessible/getFeedTokenIfReachable/ listReachableActiveTrips, which read through the canonical MikroORM QueryBuilder form, accessibleTripsQuery) — the remaining duplication this task closes is the three Kysely copies inside this file. TRIPREPO-041 (64/64 in the file) stays green as the unification's own regression guard.
This commit is contained in:
@@ -2,6 +2,7 @@ import type { Trips } from '../entities/Trips.entity';
|
||||
import { coalesceParam, currentTimestamp, nowDateOffset } from '../dialect/sql-functions';
|
||||
import type { AssertRowKeys } from './_shared/rows';
|
||||
import { TrekRepository } from './_shared/trek-repository';
|
||||
import { tripAccessExpr } from './_shared/trip-access';
|
||||
|
||||
/** What a trip-scoped request learns about the trip once access is verified. */
|
||||
export interface TripAccess {
|
||||
@@ -405,7 +406,7 @@ export class TripsRepository extends TrekRepository<Trips> {
|
||||
const row = await this.tripSelectQuery(user_id)
|
||||
.leftJoin('trip_members as m', (join) => join.onRef('m.trip_id', '=', 't.id').on('m.user_id', '=', user_id))
|
||||
.where('t.id', '=', trip_id)
|
||||
.where((eb) => eb.or([eb('t.user_id', '=', user_id), eb('m.user_id', 'is not', null)]))
|
||||
.where((eb) => tripAccessExpr(eb, 't.user_id', 'm.user_id', user_id))
|
||||
.executeTakeFirst();
|
||||
return row as TripSelectRow | undefined;
|
||||
}
|
||||
@@ -419,7 +420,7 @@ export class TripsRepository extends TrekRepository<Trips> {
|
||||
async listForUser(user_id: number, archived: number | null): Promise<TripSelectRow[]> {
|
||||
let query = this.tripSelectQuery(user_id)
|
||||
.leftJoin('trip_members as m', (join) => join.onRef('m.trip_id', '=', 't.id').on('m.user_id', '=', user_id))
|
||||
.where((eb) => eb.or([eb('t.user_id', '=', user_id), eb('m.user_id', 'is not', null)]));
|
||||
.where((eb) => tripAccessExpr(eb, 't.user_id', 'm.user_id', user_id));
|
||||
if (archived !== null) query = query.where('t.is_archived', '=', archived);
|
||||
const rows = await query.orderBy('t.created_at', 'desc').execute();
|
||||
return rows as TripSelectRow[];
|
||||
@@ -459,7 +460,7 @@ export class TripsRepository extends TrekRepository<Trips> {
|
||||
.end()
|
||||
.as('relevance'),
|
||||
])
|
||||
.where((eb) => eb.or([eb('t.user_id', '=', user_id), eb('m.user_id', 'is not', null)]))
|
||||
.where((eb) => tripAccessExpr(eb, 't.user_id', 'm.user_id', user_id))
|
||||
.where('t.is_archived', '=', 0)
|
||||
.orderBy((eb) =>
|
||||
eb.case()
|
||||
|
||||
@@ -0,0 +1,41 @@
|
||||
import type { ExpressionBuilder, ExpressionWrapper, ReferenceExpression, SqlBool } from 'kysely';
|
||||
|
||||
/**
|
||||
* The trip-access predicate — "can `userId` see this trip?" — as ONE Kysely
|
||||
* boolean expression, generic over the caller's own `<DB, TB>` (the same
|
||||
* fully-generic shape `reservation-visibility.ts`'s `publicReservationExpr`
|
||||
* uses, for the identical reason: this predicate has no correlated
|
||||
* subquery, so nothing forces the narrower fixed-alias shape
|
||||
* `publicStayExists` needs).
|
||||
*
|
||||
* Byte-for-byte the SAME boolean `Trips.repository.ts`'s MikroORM
|
||||
* QueryBuilder helper `accessibleTripsQuery` renders (`findAccessible`'s own
|
||||
* canonical shape, per Plan 4 Task 8a): `t.user_id = ? OR m.user_id IS NOT
|
||||
* NULL`, where `m` is a `LEFT JOIN trip_members ON m.trip_id = t.id AND
|
||||
* m.user_id = ?` the caller has already built. This file does not (and
|
||||
* cannot, per `tripSelectQuery`'s own docstring on why a shared join helper
|
||||
* can't be typed across differently-joined outer queries) unify the JOIN
|
||||
* itself — only the WHERE boolean that follows it, which is the part that
|
||||
* was actually duplicated three times by hand in this program (Task 7
|
||||
* security review M1 / TRIPREPO-041): `TripsRepository.findForViewer`,
|
||||
* `.listForUser` and `.activeTrip` each re-typed the identical
|
||||
* `eb.or([eb('t.user_id', '=', user_id), eb('m.user_id', 'is not', null)])`
|
||||
* independently. Proven identical to the MikroORM form and across all three
|
||||
* Kysely call sites by `TRIPREPO-041`'s cross-method access-parity test,
|
||||
* which stays green as this refactor's own regression guard (its own
|
||||
* comment already said a future unification pass should).
|
||||
*
|
||||
* `tripUserIdRef`/`memberUserIdRef` are the caller's own already-aliased
|
||||
* column references (`'t.user_id'`, `'m.user_id'` at every known call site
|
||||
* today) — passed in, not hard-coded, so this helper stays usable if a
|
||||
* future caller joins under different aliases, the same reasoning
|
||||
* `publicReservationExpr`'s own `ref` parameter gives.
|
||||
*/
|
||||
export function tripAccessExpr<DB, TB extends keyof DB>(
|
||||
eb: ExpressionBuilder<DB, TB>,
|
||||
tripUserIdRef: ReferenceExpression<DB, TB>,
|
||||
memberUserIdRef: ReferenceExpression<DB, TB>,
|
||||
userId: number,
|
||||
): ExpressionWrapper<DB, TB, SqlBool> {
|
||||
return eb.or([eb(tripUserIdRef, '=', userId), eb(memberUserIdRef, 'is not', null)]);
|
||||
}
|
||||
@@ -564,18 +564,27 @@ describe('TripsRepository.insertTripCopy (TP37)', () => {
|
||||
|
||||
// ── Task 7 security review M1, absorbed here (Task 8 touches the same file) ──
|
||||
//
|
||||
// The trip access predicate (`t.user_id = ? OR EXISTS a trip_members row`) is
|
||||
// The trip access predicate (`t.user_id = ? OR EXISTS a trip_members row`) was
|
||||
// written out FOUR times in this file: once as the shared QB helper
|
||||
// `accessibleTripsQuery` (`findAccessible`/`listAccessibleIds`), and three
|
||||
// more times by hand in the Kysely methods (`findForViewer`, `listForUser`,
|
||||
// `activeTrip`) — a different builder API that cannot share the QB helper.
|
||||
// A single Kysely `.$call()` helper for the latter three was evaluated and
|
||||
// set aside for this task (real typing risk across three different `DB`
|
||||
// shapes on a security-sensitive predicate, under this task's own time
|
||||
// budget) in favor of the review's own stated fallback: a cross-method
|
||||
// parity test proving all five agree on who can see a trip. A future task
|
||||
// unifying the three Kysely copies should keep this test green as its own
|
||||
// regression guard.
|
||||
// `activeTrip`) — a different builder API that cannot share the QB helper. A
|
||||
// single Kysely `.$call()` helper for the latter three was evaluated and set
|
||||
// aside by Task 7 (real typing risk across three different `DB` shapes on a
|
||||
// security-sensitive predicate, under that task's own time budget) in favor
|
||||
// of the review's own stated fallback: this cross-method parity test proving
|
||||
// all five agree on who can see a trip.
|
||||
//
|
||||
// Plan 4 Task 8a unified the three Kysely copies onto ONE shared boolean
|
||||
// expression, `tripAccessExpr` (`_shared/trip-access.ts`) — fully generic
|
||||
// over `<DB, TB>` (no correlated subquery forces the narrower fixed-alias
|
||||
// shape `reservation-visibility.ts`'s `publicStayExists` needs, so it
|
||||
// composes across `TripSelectKyselyDB` and `ActiveTripKyselyDB` without a
|
||||
// `.$call()` helper). The LEFT JOIN itself stays hand-written at each call
|
||||
// site (unchanged, still three copies) — `tripSelectQuery`'s own docstring
|
||||
// records why a shared join helper can't be typed across these
|
||||
// differently-joined outer queries. This test stays green as the
|
||||
// unification's own regression guard, exactly as it always was.
|
||||
describe('Cross-method access parity (Task 7 security review M1, absorbed)', () => {
|
||||
it('TRIPREPO-041: owner, member, stranger and an admin with no membership get the identical accessible/inaccessible verdict from all five access-checking methods', async () => {
|
||||
const { user: owner } = createUser(testDb);
|
||||
|
||||
Reference in New Issue
Block a user