From 1cc0b97a72ff2bc19f249128f80bd895d843aabb Mon Sep 17 00:00:00 2001
From: Ajay Ghanwat <ghanwat.ajay@workroot.in>
Date: Tue, 4 Aug 2026 20:52:18 +0530
Subject: [PATCH] fix(authz): replace vacuous concurrency test, validate effect
 in memory store

The fix-round-1 test "a concurrent write is not lost to another
method's failure" was vacuous: a single-process Promise.all cannot
reliably land a bare write inside another method's open transaction,
so it passed against both the fixed and the (previously) defective
grant() implementation. The shared-connection rollback hazard it was
meant to catch is real (confirmed separately by forcing the
transaction open before the write), but this specific test could
never reach that state and gave false assurance either way.

Replaced it with "a rejected write leaves unrelated state intact",
which asserts a grant() call with an invalid effect is refused without
disturbing the subject's existing roles/grants, plus a NOTE
documenting that the rollback hazard is now prevented structurally (no
transactions) rather than by a dedicated concurrency test.

memoryPermissionStore.grant() had no effect validation, so it failed
the new test; added a guard mirroring the db adapter's CHECK
constraint so both adapters agree on rejecting anything other than
"allow"/"deny".

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 packages/authz/src/store.ts              |  8 ++++++++
 packages/authz/test/store-conformance.ts | 24 ++++++++++++++++--------
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/packages/authz/src/store.ts b/packages/authz/src/store.ts
index 89c1fe7e..11480bab 100644
--- a/packages/authz/src/store.ts
+++ b/packages/authz/src/store.ts
@@ -77,6 +77,14 @@ export function memoryPermissionStore(): PermissionStore {
       if (at !== -1) roles.splice(at, 1);
     },
     async grant(subjectId, permission, effect, scope) {
+      // The db adapter enforces this via a CHECK constraint; the memory
+      // adapter must agree, or a bad effect would silently vanish from both
+      // the grant and deny buckets on read instead of being refused up front.
+      if (effect !== "allow" && effect !== "deny") {
+        throw new TypeError(
+          `WRN-AUTHZ-EFFECT: effect must be "allow" or "deny", received ${JSON.stringify(effect)}`,
+        );
+      }
       const key = scopeKey(scope);
       const at = grants.findIndex(
         (g) => g.subjectId === subjectId && g.scope === key && g.permission === permission,
diff --git a/packages/authz/test/store-conformance.ts b/packages/authz/test/store-conformance.ts
index a9154d8e..cb82f99f 100644
--- a/packages/authz/test/store-conformance.ts
+++ b/packages/authz/test/store-conformance.ts
@@ -154,17 +154,25 @@ export function runStoreConformance(name: string, makeStore: () => Promise<Permi
       expect(assignments.denies).toEqual(["post:delete"]);
     });
 
-    test("a concurrent write is not lost to another method's failure", async () => {
-      // A store that wraps one method in a transaction on a shared connection
-      // will roll back this unrelated write and still resolve successfully.
+    test("a rejected write leaves unrelated state intact", async () => {
       await store.assignRole("victim", "admin");
-      await Promise.all([
-        store.revokeRole("victim", "admin"),
-        store.grant("other", "post:read", "allow").catch(() => undefined),
-      ]);
-      expect((await store.assignmentsFor("victim")).roles).toEqual([]);
+      await store.grant("victim", "post:read", "allow");
+      // An invalid effect must be refused without disturbing anything else.
+      await expect(store.grant("victim", "post:write", "bogus" as never)).rejects.toThrow();
+      const assignments = await store.assignmentsFor("victim");
+      expect(assignments.roles).toEqual(["admin"]);
+      expect(assignments.grants).toEqual(["post:read"]);
     });
 
+    // NOTE: the shared-connection rollback hazard - where one method's open
+    // transaction sweeps in a concurrent bare write from another method and
+    // discards it, so a revoke resolves successfully while the role survives -
+    // is prevented STRUCTURALLY, by the store using no transactions at all.
+    // It is deliberately not covered here: reproducing it needs the bare write
+    // to land inside the open transaction, which a single-process Promise.all
+    // does not reliably arrange, so any such test would pass against the
+    // defective implementation and give false assurance.
+
     test("listSubjects with no scope returns global assignees only", async () => {
       await store.assignRole("g1", "viewer");
       await store.assignRole("s1", "editor", { tenantId: "t1" });