Decide admin status from Firestore, not from the token claim

firestore.rules

@@ -12,28 +12,41 @@ service cloud.firestore {
       return isAuthenticated() && request.auth.uid == userId;
     }
 
-    // Admin and banned status are mirrored to Auth custom claims by the
-    // onAdminStateWritten trigger (functions/src/users.ts). Rules trust a
-    // POSITIVE token claim (cheap, no Firestore read) and fall back to
-    // the admin/state doc when the token doesn't carry the claim — covers
-    // freshly issued tokens that haven't picked up a recent change yet.
+    // Admin status is decided by the admin/state document, never by the token.
     //
-    // CALLABLES still read admin/state directly (via getNotificationActor)
-    // because custom claims have up-to-1h propagation lag, and we don't
-    // want a banned user to keep mutating until their token refreshes.
+    // This used to trust a POSITIVE `admin` custom claim on its own, as a cheap
+    // path with no Firestore read, falling back to the document only when the
+    // token didn't carry the claim. That was unsound in one direction, and the
+    // direction matters:
+    //
+    //   isNotBanned() trusts a positive `banned` claim → a STALE claim keeps
+    //     someone banned slightly too long. Fails safe.
+    //   isAdmin() trusting a positive `admin` claim → a STALE claim keeps
+    //     someone an admin for the rest of that token's life. Fails open.
+    //
+    // onAdminStateWritten clears the claim on demotion but nothing revokes the
+    // already-issued ID token, and rules only verify a JWT's signature and
+    // expiry — not that the claim inside it is still true, nor even that the
+    // Auth user still exists. So a demoted admin kept full admin power for up
+    // to an hour, and `role` being writable with it made that permanent: write
+    // {role:'admin'} back to your own admin/state on the stale claim, and the
+    // trigger re-issues the claim for real.
+    //
+    // Reading the document is what the callables have always done
+    // (getNotificationActor), so this makes the two layers agree. Cost is one
+    // document read wherever isAdmin() is actually reached; it sits behind a
+    // short-circuit on every hot path (see the `|| isAdmin()` shapes below).
+    //
+    // exists() must come first so this returns FALSE rather than raising an
+    // evaluation error for a user with no admin/state doc — an error would
+    // propagate through `isAdmin() || (own record)` and lock ordinary users out
+    // of their own reports and feedback. That is #150, pinned by a test.
     function isAdmin() {
       let adminDoc = /databases/$(database)/documents/users/$(request.auth.uid)/admin/state;
-      return isAuthenticated() && (
-        // Guard the claim access: a token without the `admin` claim (i.e. every
-        // non-admin) must yield false here, not a failed expression — same
-        // missing-custom-claim footgun fixed for isNotBanned in #149.
-        ('admin' in request.auth.token && request.auth.token.admin == true) ||
-        (
-          exists(adminDoc) &&
-          'role' in get(adminDoc).data &&
-          get(adminDoc).data.role == 'admin'
-        )
-      );
+      return isAuthenticated() &&
+        exists(adminDoc) &&
+        'role' in get(adminDoc).data &&
+        get(adminDoc).data.role == 'admin';
     }
 
     function isNotBanned() {
@@ -85,6 +98,16 @@ service cloud.firestore {
         hasSafePublicLocation(request.resource.data);
     }
 
+    // Which keys may EXIST on an admin/state document. `role` stays in this
+    // list deliberately: it is written by the Admin SDK (console, or
+    // functions/scripts/migrate-admin-state.js), which bypasses rules, so a
+    // real document legitimately carries it. Dropping it here would not stop
+    // a client writing role — it would stop a client banning anyone whose
+    // document already has one, because request.resource.data on an update is
+    // the whole POST-WRITE document, not the changed keys. That is the same
+    // trap hasSafePublicLocation fell into; see the audit note on it.
+    //
+    // Whether a client may write `role` is enforced per-operation below.
     function isAllowedAdminStateWrite() {
       let allowed = ['role', 'banned', 'bannedReason', 'bannedAt'];
       return request.resource.data.keys().hasOnly(allowed);
@@ -198,8 +221,20 @@ service cloud.firestore {
 
       match /admin/state {
         allow read: if isOwner(userId) || isAdmin();
-        allow create: if isAdmin() && isAllowedAdminStateWrite();
-        allow update: if isAdmin() && isAllowedAdminStateWrite();
+        // No client write may introduce or change `role`, not even an admin's.
+        // Promotion is an owner operation through the console or the Admin SDK,
+        // which bypass these rules; the app has never written role (the only
+        // caller, blockUserByAdmin in src/services/admin.ts, writes the three
+        // ban fields). Keeping it out of reach means one compromised admin
+        // session cannot mint more admins, and — with isAdmin() now reading
+        // this document rather than the token — closes the loop where a demoted
+        // admin re-promoted themselves on a stale claim.
+        allow create: if isAdmin() &&
+          isAllowedAdminStateWrite() &&
+          !('role' in request.resource.data);
+        allow update: if isAdmin() &&
+          isAllowedAdminStateWrite() &&
+          !request.resource.data.diff(resource.data).affectedKeys().hasAny(['role']);
         allow delete: if false;
       }
     }

functions/src/users.ts

@@ -218,14 +218,29 @@ export const onUserUpdated = onDocumentWritten(
   }
 );
 
-// Sync admin/banned to Auth custom claims so firestore.rules can short-
-// circuit cheap token-claim checks (admin == true, banned == true) ahead
-// of the existing get(/admin/state) reads. Custom claims are eventually
-// consistent — they only land on the user's NEXT id token, up to 1h
-// later — so callables continue to read Firestore directly via
-// getNotificationActor for time-critical authorization. Rules combine
-// both: positive token claim trusted, negative/missing falls back to
-// Firestore.
+// Sync admin/banned to Auth custom claims. Custom claims are eventually
+// consistent — they only land on the user's NEXT id token, up to 1h later —
+// so callables read Firestore directly via getNotificationActor for
+// time-critical authorization.
+//
+// What rules do with each claim is NOT symmetric, and the asymmetry is the
+// point:
+//
+//   `banned`  is trusted positively by isNotBanned(), as a cheap path ahead
+//             of the get(/admin/state) fallback. A stale banned claim keeps
+//             someone banned slightly too long, which fails safe.
+//   `admin`   is NOT trusted by isAdmin() any more. Nothing revokes an
+//             already-issued id token when this trigger clears the claim on
+//             demotion, and rules cannot tell a stale claim from a live one,
+//             so trusting it positively meant a demoted admin kept admin
+//             power for the rest of that token's life. isAdmin() reads this
+//             document instead. The claim is still set here — it costs
+//             nothing and other backends may want it — but do not
+//             reintroduce a rule that grants anything on the claim alone.
+//
+// Revoking refresh tokens here would not have fixed it: revocation stops the
+// client getting a NEW id token, but Firestore rules validate only a JWT's
+// signature and expiry, so the current one keeps working until it expires.
 export const onAdminStateWritten = onDocumentWritten(
   "users/{userId}/admin/state",
   async (event) => {

tests/rules/users.test.ts

@@ -133,7 +133,6 @@ describe("users/{uid}/admin/state", () => {
     const db = adminUser(env, ADMIN).firestore();
     await assertSucceeds(
       setDoc(doc(db, `users/${BOB}/admin/state`), {
-        role: "user",
         banned: true,
         bannedReason: "spam",
         bannedAt: new Date(),
@@ -147,6 +146,42 @@ describe("users/{uid}/admin/state", () => {
     );
   });
 
+  it("refuses to let even a real admin write role from the client", async () => {
+    // Promotion is an owner operation via the console or the Admin SDK, both
+    // of which bypass rules. No client path has ever needed it, so keeping it
+    // unreachable costs nothing and means one compromised admin session cannot
+    // mint more admins.
+    const db = adminUser(env, ADMIN).firestore();
+    await assertFails(
+      setDoc(doc(db, `users/${BOB}/admin/state`), { role: "admin" })
+    );
+    await assertFails(
+      setDoc(doc(db, `users/${BOB}/admin/state`), {
+        role: "user",
+        banned: true,
+      })
+    );
+  });
+
+  it("still lets an admin ban a user whose doc already carries a role", async () => {
+    // The reason `role` stays in isAllowedAdminStateWrite's key allowlist.
+    // request.resource.data on an update is the whole post-write document, so
+    // removing role from hasOnly would not have blocked writing role — it
+    // would have blocked banning anyone the Admin SDK had given one.
+    await env.withSecurityRulesDisabled(async (ctx) => {
+      await setDoc(doc(ctx.firestore(), `users/${BOB}/admin/state`), {
+        role: "user",
+      });
+    });
+    const db = adminUser(env, ADMIN).firestore();
+    await assertSucceeds(
+      updateDoc(doc(db, `users/${BOB}/admin/state`), {
+        banned: true,
+        bannedReason: "spam",
+      })
+    );
+  });
+
   it("lets a user read their own admin state but not someone else's", async () => {
     await assertSucceeds(
       getDoc(doc(plainUser(env, ALICE).firestore(), `users/${ALICE}/admin/state`))
@@ -189,3 +224,60 @@ describe("users/{uid}/admin/state", () => {
     );
   });
 });
+
+describe("an admin custom claim is not sufficient on its own", () => {
+  // isAdmin() used to return true on a positive `admin` claim alone. Nothing
+  // revokes an already-issued ID token on demotion, and rules only check a
+  // JWT's signature and expiry — not whether the claim inside it is still
+  // true. These tests pin that the admin/state document is now the authority.
+
+  it("grants nothing to a token claiming admin with no admin/state doc", async () => {
+    // ALICE is seeded with a user doc but no admin/state — the shape of a
+    // demoted admin whose doc was deleted, or a forged/stale token.
+    const db = adminUser(env, ALICE).firestore();
+    await assertFails(
+      setDoc(doc(db, `users/${BOB}/admin/state`), { banned: true })
+    );
+    await assertFails(getDoc(doc(db, `users/${BOB}/admin/state`)));
+  });
+
+  it("does not let a demoted admin re-promote themselves on the stale claim", async () => {
+    // The exploit, end to end. The owner demotes ALICE in the console; her ID
+    // token keeps saying admin:true for up to an hour. Previously she could
+    // write role:'admin' back to her own admin/state, and
+    // onAdminStateWritten would re-issue the claim for real — making the
+    // demotion permanently reversible by the person demoted.
+    await env.withSecurityRulesDisabled(async (ctx) => {
+      await setDoc(doc(ctx.firestore(), `users/${ALICE}/admin/state`), {
+        role: "user",
+      });
+    });
+    const db = adminUser(env, ALICE).firestore();
+    await assertFails(
+      updateDoc(doc(db, `users/${ALICE}/admin/state`), { role: "admin" })
+    );
+    await assertFails(
+      setDoc(
+        doc(db, `users/${ALICE}/admin/state`),
+        { role: "admin" },
+        { merge: true }
+      )
+    );
+    // And the rest of what the stale claim used to buy is gone too.
+    await assertFails(
+      setDoc(doc(db, `users/${BOB}/admin/state`), { banned: true })
+    );
+  });
+
+  it("recognises a real admin whose token carries no claim at all", async () => {
+    // The other direction, which is why the document is read rather than
+    // ANDed with the claim: a freshly promoted admin must not have to wait
+    // out their old token. ADMIN holds role:'admin' in Firestore (seeded in
+    // beforeEach) and plainUser gives a token with no custom claims.
+    const db = plainUser(env, ADMIN).firestore();
+    await assertSucceeds(
+      setDoc(doc(db, `users/${BOB}/admin/state`), { banned: true })
+    );
+    await assertSucceeds(getDoc(doc(db, `users/${BOB}/admin/state`)));
+  });
+});