Skip to content

Commit d5d8874

Browse files
committed
fix(objectql): read a referenced user's ban state from the existence probe's own read
The ban check issued a find on sys_user of its own on every write naming a user, which the plugin-audit read-cost pins (a create pays zero reads on the reference targets) caught. The probe now selects the target's declared ban columns beside the id (referenceRow, which referenceExists delegates to), so the check costs no read and keeps the probe's elevation. Claude-Session: https://claude.ai/code/session_01Utp6CZavYYLh5k5r6jhgCF Co-authored-by: Claude <noreply@anthropic.com>
1 parent ed85181 commit d5d8874

2 files changed

Lines changed: 40 additions & 56 deletions

File tree

‎packages/objectql/src/engine-referential-integrity.ts‎

Lines changed: 16 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ export interface ReferentialIntegrityHost {
6969
find: ObjectQL['find'];
7070
masterDetailHeaderExists: ObjectQL['masterDetailHeaderExists'];
7171
referenceExists: ObjectQL['referenceExists'];
72+
referenceRow: ObjectQL['referenceRow'];
7273
}
7374

7475
/**
@@ -188,7 +189,7 @@ function isEmptyReferenceValue(v: unknown): boolean {
188189
* (see {@link masterDetailHeadersCallerReads}).
189190
*/
190191
export async function assertReferencesResolve(
191-
this: Pick<ReferentialIntegrityHost, 'find' | 'logger' | 'masterDetailHeaderExists' | 'referenceExists'>,
192+
this: Pick<ReferentialIntegrityHost, 'masterDetailHeaderExists' | 'referenceExists' | 'referenceRow'>,
192193
schema: any,
193194
data: Record<string, unknown> | null | undefined,
194195
supplied: Record<string, unknown> | null | undefined,
@@ -204,9 +205,6 @@ export async function assertReferencesResolve(
204205
if (!fields || !data) return;
205206

206207
const refused: Array<{ name: string; def: any; value: unknown; target: string; code?: 'reference_user_banned' }> = [];
207-
// [#30205] The user ids that resolved, by the fields that named them: a
208-
// banned user is a row that exists, so the existence probe passes them.
209-
const users: Array<{ name: string; def: any; value: unknown; target: string }> = [];
210208
for (const name of Object.keys(fields)) {
211209
// [#4441, narrowing kept by #4743] A `readonly` field is never the
212210
// caller's to answer for — BY CONSTRUCTION, not by exemption.
@@ -263,19 +261,18 @@ export async function assertReferencesResolve(
263261
if (v === null || v === undefined || v === '') continue;
264262
if (typeof v === 'object') continue;
265263
// [#22519] A master-detail header must also be one the caller can read.
264+
if (def?.type !== 'master_detail' && target === BAN_CHECKED_USER_OBJECT) {
265+
// [#30205] One probe answers both: the row's existence and its ban.
266+
const row = await this.referenceRow(target, v, context, USER_BAN_FIELDS);
267+
if (row === false) refused.push({ name, def, value: v, target });
268+
// adr0112-ok: D6 — a field-addressed FieldErrorCode; this entry becomes `{ field, code }` below
269+
else if (row !== null && isUserBanInForce(row)) refused.push({ name, def, value: v, target, code: 'reference_user_banned' });
270+
continue;
271+
}
266272
const resolved = def?.type === 'master_detail'
267273
? await this.masterDetailHeaderExists(target, v, context)
268274
: await this.referenceExists(target, v, context);
269275
if (resolved === false) refused.push({ name, def, value: v, target });
270-
else if (resolved === true && target === BAN_CHECKED_USER_OBJECT) users.push({ name, def, value: v, target });
271-
}
272-
}
273-
if (users.length > 0) {
274-
const banned = await bannedUserIds.call(this, users.map((u) => u.value), context);
275-
for (const u of users) {
276-
if (!banned.has(String(u.value))) continue;
277-
// adr0112-ok: D6 — a field-addressed FieldErrorCode; this entry becomes `{ field, code }` below
278-
refused.push({ ...u, code: 'reference_user_banned' });
279276
}
280277
}
281278
if (refused.length === 0) return;
@@ -300,12 +297,6 @@ export async function assertReferencesResolve(
300297

301298
/**
302299
* [#30205] The user object whose rows a reference check also asks about a ban.
303-
*/
304-
const BAN_CHECKED_USER_OBJECT = 'sys_user';
305-
306-
/**
307-
* [#30205] Which of `ids` name a user whose ban is in force
308-
* (`isUserBanInForce`, @objectstack/core).
309300
*
310301
* Banning a user leaves the row in place, so the existence probe resolves a
311302
* banned user like any other, and a write could hand new work to someone who
@@ -316,37 +307,13 @@ const BAN_CHECKED_USER_OBJECT = 'sys_user';
316307
* the banned user stays editable, and is handed over by the transfer
317308
* instead), never a system write the reference check exempts.
318309
*
319-
* Read the way the existence probe reads — `sudo()`-shaped, the caller's
320-
* context spread under `isSystem: true` — because a ban is a fact about the
321-
* user, not about what the caller may read: a caller with no read grant on
322-
* the user object is refused the same way. One `find` for the whole write.
323-
* A read that fails refuses nothing, and says so once at `warn`: like the
324-
* probe, a check that cannot run must not invent a rejection.
310+
* The ban columns ride the existence probe's own read (`referenceRow`), so the
311+
* check costs no read of its own and is read the way the probe reads:
312+
* elevated inside the caller's tenant, because a ban is a fact about the
313+
* user, not about what the caller may read. A probe that cannot run refuses
314+
* nothing, as before.
325315
*/
326-
async function bannedUserIds(
327-
this: Pick<ReferentialIntegrityHost, 'find' | 'logger'>,
328-
ids: ReadonlyArray<unknown>,
329-
context: ExecutionContext | undefined,
330-
): Promise<ReadonlySet<string>> {
331-
const asked = [...new Set(ids.map((id) => String(id)))];
332-
try {
333-
const rows = await this.find(BAN_CHECKED_USER_OBJECT, {
334-
where: { id: asked.length === 1 ? asked[0] : { $in: asked } },
335-
fields: [...USER_BAN_FIELDS],
336-
context: { ...(context ?? {}), isSystem: true } as ExecutionContext,
337-
} as EngineQueryOptions) as Array<Record<string, unknown>>;
338-
const banned = new Set<string>();
339-
for (const row of Array.isArray(rows) ? rows : []) {
340-
if (row?.id != null && isUserBanInForce(row)) banned.add(String(row.id));
341-
}
342-
return banned;
343-
} catch (err) {
344-
this.logger?.warn?.('the ban-state read of the referenced users failed — the write is not refused for a ban', {
345-
object: BAN_CHECKED_USER_OBJECT, error: err,
346-
});
347-
return new Set();
348-
}
349-
}
316+
const BAN_CHECKED_USER_OBJECT = 'sys_user';
350317

351318
/**
352319
* [#29062] The rows a write lands on, as stored before it, read on demand by

‎packages/objectql/src/engine.ts‎

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3137,13 +3137,11 @@ export class ObjectQL implements IObjectQLEngine {
31373137
storedRows?: StoredReferenceRows,
31383138
): Promise<void> {
31393139
// [#25522 cut 10] The body moved to `engine-referential-integrity.ts`, its docblock with it (see `ReferentialIntegrityHost`).
3140-
// [#30205] `find` and `logger` serve the ban-state read of referenced users.
3141-
const readLogger = () => this.logger;
3140+
// [#30205] `referenceRow` answers a referenced user's ban state from the existence probe's own read.
31423141
const host: ThisParameterType<typeof assertReferencesResolve> = {
3143-
get logger() { return readLogger(); },
3144-
find: (...a) => this.find(...a),
31453142
masterDetailHeaderExists: (...a) => this.masterDetailHeaderExists(...a),
31463143
referenceExists: (...a) => this.referenceExists(...a),
3144+
referenceRow: (...a) => this.referenceRow(...a),
31473145
};
31483146
return assertReferencesResolve.call(host, schema, data, supplied, context, msgCtx, storedRows);
31493147
}
@@ -3170,15 +3168,34 @@ export class ObjectQL implements IObjectQLEngine {
31703168
* now refuses.
31713169
*/
31723170
private async referenceExists(target: string, id: unknown, context?: ExecutionContext): Promise<boolean | null> {
3171+
const row = await this.referenceRow(target, id, context, []);
3172+
return row === null ? null : row !== false;
3173+
}
3174+
3175+
/**
3176+
* [#30205] {@link referenceExists}'s probe, answering the row it found:
3177+
* `id` plus whichever of `extraFields` the target declares, `false` when the
3178+
* probe ran and found nothing, `null` when it could not run. Same read, same
3179+
* elevation, so a check that needs a column of the target (a user's ban
3180+
* state) costs no read of its own.
3181+
*/
3182+
private async referenceRow(
3183+
target: string,
3184+
id: unknown,
3185+
context: ExecutionContext | undefined,
3186+
extraFields: readonly string[],
3187+
): Promise<Record<string, unknown> | false | null> {
31733188
try {
31743189
const resolved = this.resolveObjectName(target);
3175-
if (!this._registry.getObject(resolved)) return null;
3190+
const schema = this._registry.getObject(resolved);
3191+
if (!schema) return null;
3192+
const declared = (schema as { fields?: Record<string, unknown> }).fields ?? {};
31763193
const row = await this.findOne(resolved, {
31773194
where: { id },
3178-
fields: ['id'],
3195+
fields: ['id', ...extraFields.filter((f) => f !== 'id' && f in declared)],
31793196
context: ObjectQL.referenceCheckContext(context),
31803197
} as any);
3181-
return !!row;
3198+
return row ? row as Record<string, unknown> : false;
31823199
} catch {
31833200
return null;
31843201
}

0 commit comments

Comments
 (0)