@@ -8,9 +8,92 @@ import {
88 autoOrgAdminGrantReason ,
99} from './auto-org-admin-grant.js' ;
1010
11+ // ---------------------------------------------------------------------------
12+ // [#4640] The double speaks the ENGINE's signatures — or it proves nothing.
13+ //
14+ // The previous stub implemented `delete(object, id)`, a signature ObjectQL has
15+ // never had. The module called `ql.delete(object, id, ctx)`; the stub happily
16+ // deleted the row, every revoke test above went green, and in production the
17+ // id landed in the option-bag slot where `rejectUnknownEngineOptions` reads its
18+ // character indices as unknown keys and throws — straight into a swallowing
19+ // `catch`. So for this module's entire life NOTHING was ever revoked: demoted
20+ // admins kept `organization_admin`, hence tenant admin.
21+ //
22+ // A double looser than the real thing is not a weaker test — it is a test of a
23+ // different program. This one therefore mirrors the engine's entry-point
24+ // contract (`packages/objectql/src/engine.ts`) on both axes that matter:
25+ //
26+ // 1. ARITY AND ARGUMENT ROLES, which is where this bug lived:
27+ // find(object, query: EngineQueryOptions, options?: EngineReadOptions)
28+ // insert(object, data, options?) ← context in the 3rd arg
29+ // delete(object, options?) ← context in the 2nd arg
30+ // 2. `rejectUnknownEngineOptions`'s rule that an option key the engine does
31+ // not execute is an ERROR — never something to quietly ignore. A
32+ // positional argument in the bag slot fails this the same way it fails in
33+ // the engine, so the same drift is loud here next time.
34+ // ---------------------------------------------------------------------------
35+
36+ /** Mirrors `ENGINE_FIND_OPTION_KEYS` in `packages/objectql/src/engine.ts`. */
37+ const FIND_QUERY_KEYS = new Set ( [
38+ 'context' , 'where' , 'fields' , 'orderBy' , 'limit' , 'offset' , 'search' , 'searchFields' , 'expand' ,
39+ ] ) ;
40+ /** Mirrors `ENGINE_DELETE_OPTION_KEYS` — note `where`, and note NO id argument. */
41+ const DELETE_OPTION_KEYS = new Set ( [ 'context' , 'where' , 'multi' ] ) ;
42+ /** The trailing read/write options bag (`EngineReadOptions` and friends). */
43+ const TRAILING_OPTION_KEYS = new Set ( [ 'context' ] ) ;
44+
45+ /**
46+ * The engine's own unknown-key rule, applied to a double.
47+ *
48+ * Rejecting a non-object bag is the half that catches a positional argument:
49+ * `Object.entries('ups_1')` yields `'0'/'1'/'2'…`, which is exactly how the
50+ * real engine reports a mis-shaped call — the message just reads better here.
51+ */
52+ function assertOptionBag (
53+ operation : string ,
54+ object : string ,
55+ bag : unknown ,
56+ legal : ReadonlySet < string > ,
57+ ) : void {
58+ if ( bag === undefined || bag === null ) return ;
59+ if ( typeof bag !== 'object' || Array . isArray ( bag ) ) {
60+ throw new Error (
61+ `${ operation } ${ object } ${ typeof bag } +
62+ `(${ String ( bag ) } +
63+ `e.g. delete(object, { where: { id }, context }).` ,
64+ ) ;
65+ }
66+ const unknown = Object . entries ( bag as Record < string , unknown > )
67+ . filter ( ( [ k , v ] ) => v != null && ! legal . has ( k ) )
68+ . map ( ( [ k ] ) => k ) ;
69+ if ( unknown . length > 0 ) {
70+ throw new Error (
71+ `${ operation } ${ object } ${ unknown . length > 1 ? 's' : '' } +
72+ `${ unknown . map ( ( k ) => `'${ k } ) . join ( ', ' ) } +
73+ `call would succeed with the option silently ignored (#4371). ` +
74+ `Legal keys for ${ operation } ${ [ ...legal ] . sort ( ) . join ( ', ' ) } ,
75+ ) ;
76+ }
77+ }
78+
1179/**
12- * Tiny in-memory ObjectQL stub: just enough surface for the reconciler
13- * (find / insert / delete) with isSystem context passthrough.
80+ * This module's writes must run as the system (better-auth's identity tables
81+ * refuse user-context writes — ADR-0092 D2). Dropping the context was the
82+ * *other* casualty of the three-arg delete, so the double checks for it too.
83+ */
84+ function assertSystemContext ( operation : string , object : string , context : any ) : void {
85+ if ( ! context || context . isSystem !== true ) {
86+ throw new Error (
87+ `${ operation } ${ object } +
88+ `(got ${ JSON . stringify ( context ) ?? 'undefined' } +
89+ `system writes; a dropped context is how a call shape silently loses its privileges.` ,
90+ ) ;
91+ }
92+ }
93+
94+ /**
95+ * Tiny in-memory ObjectQL double: just enough surface for the reconciler
96+ * (find / insert / delete), with the engine's call shapes ENFORCED.
1497 */
1598function makeStub ( seed : {
1699 sys_permission_set ?: any [ ] ;
@@ -22,6 +105,8 @@ function makeStub(seed: {
22105 sys_member : seed . sys_member ?? [ ] ,
23106 sys_user_permission_set : seed . sys_user_permission_set ?? [ ] ,
24107 } ;
108+ /** Every delete the module issued, as the engine received it. */
109+ const deleteCalls : Array < { object : string ; options : any } > = [ ] ;
25110
26111 const matches = ( row : any , where : any ) => {
27112 for ( const [ k , v ] of Object . entries ( where ?? { } ) ) {
@@ -37,19 +122,47 @@ function makeStub(seed: {
37122
38123 return {
39124 tables,
40- async find ( object : string , args : any ) {
41- const rows = tables [ object ] ?? [ ] ;
42- return rows . filter ( ( r ) => matches ( r , args ?. where ) ) ;
125+ deleteCalls,
126+ // find(object, query, options) — `where`/`limit` in the query, execution
127+ // context in either bag (`options.context` wins, as in the engine).
128+ async find ( object : string , query ?: any , options ?: any ) {
129+ assertOptionBag ( 'find' , object , query , FIND_QUERY_KEYS ) ;
130+ assertOptionBag ( 'find' , object , options , TRAILING_OPTION_KEYS ) ;
131+ assertSystemContext ( 'find' , object , options ?. context ?? query ?. context ) ;
132+ const rows = ( tables [ object ] ?? [ ] ) . filter ( ( r ) => matches ( r , query ?. where ) ) ;
133+ return typeof query ?. limit === 'number' ? rows . slice ( 0 , query . limit ) : rows ;
43134 } ,
44- async insert ( object : string , data : any ) {
135+ // insert(object, data, options) — context in the TRAILING bag.
136+ async insert ( object : string , data : any , options ?: any ) {
137+ assertOptionBag ( 'insert' , object , options , TRAILING_OPTION_KEYS ) ;
138+ assertSystemContext ( 'insert' , object , options ?. context ) ;
139+ if ( ! data || typeof data !== 'object' || Array . isArray ( data ) ) {
140+ throw new Error ( `insert('${ object } ) ;
141+ }
45142 const id = data . id ?? `${ object } ${ tables [ object ] . length + 1 } ;
46143 const row = { ...data , id } ;
47144 tables [ object ] = [ ...( tables [ object ] ?? [ ] ) , row ] ;
48145 return row ;
49146 } ,
50- async delete ( object : string , id : string ) {
51- tables [ object ] = ( tables [ object ] ?? [ ] ) . filter ( ( r ) => r . id !== id ) ;
52- return true ;
147+ // delete(object, options) — TWO arguments. The row is named by
148+ // `where.id`; there is no positional id and no third argument.
149+ async delete ( object : string , options ?: any ) {
150+ assertOptionBag ( 'delete' , object , options , DELETE_OPTION_KEYS ) ;
151+ assertSystemContext ( 'delete' , object , options ?. context ) ;
152+ deleteCalls . push ( { object, options } ) ;
153+ const where = options ?. where ;
154+ const id = where && typeof where === 'object' ? ( where as any ) . id : undefined ;
155+ const scalarId = typeof id === 'string' || typeof id === 'number' ? id : undefined ;
156+ if ( scalarId === undefined && options ?. multi !== true ) {
157+ // The engine's own refusal — an unscoped delete never runs by accident.
158+ throw new Error ( 'Delete requires an ID or options.multi=true' ) ;
159+ }
160+ const before = tables [ object ] ?? [ ] ;
161+ tables [ object ] =
162+ scalarId !== undefined
163+ ? before . filter ( ( r ) => r . id !== scalarId )
164+ : before . filter ( ( r ) => ! matches ( r , where ) ) ;
165+ return before . length - tables [ object ] . length ;
53166 } ,
54167 } ;
55168}
@@ -369,3 +482,97 @@ describe('[#4586] the auto-grant records its provenance', () => {
369482 expect ( row . reason ) . toContain ( 'mem_7' ) ;
370483 } ) ;
371484} ) ;
485+
486+ // ---------------------------------------------------------------------------
487+ // [#4640] The revoke channel, pinned at the call SHAPE.
488+ //
489+ // Every `revoked` assertion in this file was already green while production
490+ // revoked nothing, because the double implemented the wrong signature. The
491+ // tests below pin the two things that green-ness depended on and nobody was
492+ // checking: the exact call the module hands the engine, and the double's
493+ // refusal to accept anything else.
494+ // ---------------------------------------------------------------------------
495+ describe ( '[#4640] revoke speaks the engine\'s delete signature' , ( ) => {
496+ const seedDemoted = ( ) =>
497+ makeStub ( {
498+ sys_permission_set : [ ORG_ADMIN_SET , ORG_ADMIN_NO_BYPASS_SET ] ,
499+ sys_member : [ { id : 'm1' , user_id : 'u1' , organization_id : 'o1' , role : 'member' } ] ,
500+ sys_user_permission_set : [
501+ { id : 'ups1' , user_id : 'u1' , organization_id : 'o1' , permission_set_id : 'ps_org_admin' } ,
502+ ] ,
503+ } ) ;
504+
505+ it ( 'names the row by `where.id` in a TWO-argument call carrying the system context' , async ( ) => {
506+ const stub = seedDemoted ( ) ;
507+ const res = await reconcileOrgAdminGrant ( stub , 'u1' , 'o1' , WALLED ) ;
508+
509+ expect ( res . action ) . toBe ( 'revoked' ) ;
510+ expect ( stub . deleteCalls ) . toHaveLength ( 1 ) ;
511+ const [ call ] = stub . deleteCalls ;
512+ expect ( call . object ) . toBe ( 'sys_user_permission_set' ) ;
513+ // The whole bug in one assertion: the id belongs INSIDE the option bag.
514+ expect ( call . options ) . toEqual ( { where : { id : 'ups1' } , context : { isSystem : true } } ) ;
515+ } ) ;
516+
517+ it ( 'the double refuses the three-argument call the module used to make' , async ( ) => {
518+ // The drift guard. If a future edit reverts the call shape — or loosens
519+ // this double back toward `delete(object, id)` — this is what goes red
520+ // instead of the whole feature going silently inert.
521+ const stub = seedDemoted ( ) ;
522+ await expect (
523+ ( stub as any ) . delete ( 'sys_user_permission_set' , 'ups1' , { context : { isSystem : true } } ) ,
524+ ) . rejects . toThrow ( / t a k e s a n O P T I O N B A G / ) ;
525+ expect ( stub . tables . sys_user_permission_set ) . toHaveLength ( 1 ) ;
526+ } ) ;
527+
528+ it ( 'a delete the datastore rejects is REPORTED — never a silent no-op' , async ( ) => {
529+ // The other half of why this survived: the wrapper's `catch {}` turned a
530+ // throwing revoke into `false` and told nobody. The capability is still in
531+ // force, so that has to reach an operator.
532+ const stub = seedDemoted ( ) ;
533+ stub . delete = async ( ) => {
534+ throw new Error ( 'driver exploded' ) ;
535+ } ;
536+ const warnings : Array < { msg : string ; meta ?: any } > = [ ] ;
537+ const logger = { warn : ( msg : string , meta ?: any ) => warnings . push ( { msg, meta } ) } ;
538+
539+ const res = await reconcileOrgAdminGrant ( stub , 'u1' , 'o1' , { ...WALLED , logger } ) ;
540+
541+ expect ( res ) . toEqual ( { action : 'skipped' , reason : 'delete_failed' } ) ;
542+ // The grant row is still there — the state the warning is about.
543+ expect ( stub . tables . sys_user_permission_set ) . toHaveLength ( 1 ) ;
544+ expect ( warnings . map ( ( w ) => w . msg ) ) . toEqual ( [
545+ '[security] org-admin grant revoke FAILED — capability still in force' ,
546+ '[security] org-admin capability could NOT be revoked — grant rows remain' ,
547+ ] ) ;
548+ expect ( warnings [ 0 ] . meta . error ) . toBe ( 'driver exploded' ) ;
549+ } ) ;
550+
551+ it ( '"nothing to revoke" stays distinguishable from "revoke failed"' , async ( ) => {
552+ // `noop` and `skipped/delete_failed` are different facts about the
553+ // platform's state; collapsing them is how the failure hid.
554+ const stub = makeStub ( {
555+ sys_permission_set : [ ORG_ADMIN_SET , ORG_ADMIN_NO_BYPASS_SET ] ,
556+ sys_member : [ { id : 'm1' , user_id : 'u1' , organization_id : 'o1' , role : 'member' } ] ,
557+ sys_user_permission_set : [ ] ,
558+ } ) ;
559+ const res = await reconcileOrgAdminGrant ( stub , 'u1' , 'o1' , WALLED ) ;
560+ expect ( res ) . toEqual ( { action : 'noop' } ) ;
561+ expect ( stub . deleteCalls ) . toHaveLength ( 0 ) ;
562+ } ) ;
563+
564+ it ( 'membership removal revokes through the same channel' , async ( ) => {
565+ // The `sys_member` delete path: no membership row at all, grant still there.
566+ const stub = makeStub ( {
567+ sys_permission_set : [ ORG_ADMIN_SET , ORG_ADMIN_NO_BYPASS_SET ] ,
568+ sys_member : [ ] ,
569+ sys_user_permission_set : [
570+ { id : 'ups1' , user_id : 'u1' , organization_id : 'o1' , permission_set_id : 'ps_org_admin' } ,
571+ ] ,
572+ } ) ;
573+ const res = await reconcileOrgAdminGrant ( stub , 'u1' , 'o1' , WALLED ) ;
574+ expect ( res . action ) . toBe ( 'revoked' ) ;
575+ expect ( stub . tables . sys_user_permission_set ) . toHaveLength ( 0 ) ;
576+ expect ( stub . deleteCalls [ 0 ] . options . where ) . toEqual ( { id : 'ups1' } ) ;
577+ } ) ;
578+ } ) ;
0 commit comments