Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/learnings.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@ Durable rules worth keeping across sessions.
- Idempotency hashing that runs in callable code must be async and Web Crypto-safe; a `require('node:crypto')` fallback can fail under ESM/browser bundling even when the code works in unit tests.
- When a callable looks like an auth or App Check problem, verify the initialized functions region before chasing browser state; a region mismatch can produce misleading unauthenticated failures.
- **Stale compiled functions binary is the first thing to check when `FirebaseError: internal` appears in E2E but unit tests pass.** The emulator runs `functions/lib/`, not `functions/src/`. If source was changed (e.g. `enforceAppCheck`) but `pnpm --filter @bantayog/functions build` was not re-run, the emulator silently enforces the old setting. Fix: rebuild before running `firebase emulators:exec`.
- `createTestEnv()` in this repo expects Firestore, Database, and Storage emulators. If you only boot Firestore, rules tests that initialize the shared harness fail before they reach assertions.
- When a payload schema is `strict()` and a field is truly transitional, strip that field before validation instead of widening the schema and accidentally allowing unrelated junk.
- If a transitional field is part of the contract, model it explicitly in the schema instead of stripping it in the trigger. Stripping is only the fallback when the field is truly out-of-band.
- Ops-facing document schemas should use ops-specific enums, not the broader public report enum, or the rules/tests will drift silently.
- `z.string().uuid()` trips `@typescript-eslint/no-deprecated` under the current lint config. Use `z.uuid()` in shared validators.
- Collection query tests can fail on a rule that is really written for per-document access. If the rule uses `resource.data` in a way that doesn’t support `list`, switch the test to `getDoc` or rewrite the rule intentionally.

## Firestore

Expand Down
13 changes: 13 additions & 0 deletions docs/progress.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,19 @@

## Current

### Phase 5 PRE-B - Schema + Rules Foundation (2026-04-24)

- Status: DONE locally - schema amendments, rules additions, and inbox materialization updates landed
- Scope:
- shared validators: coordination, responders, users, reports, and export updates
- Firestore rules: field mode sessions, report notes, report messages, command channel map-key lookup, report sharing events, and shared report reads
- inbox trigger: `report_ops.reportType`, optional `locationGeohash`, and explicit `exactLocation` payload support derived from the inbox item
- Verification:
- `pnpm --filter @bantayog/shared-validators exec vitest run src/coordination.test.ts src/responders.test.ts src/users.test.ts src/reports.test.ts` - PASS
- `firebase emulators:exec --only firestore,database,storage "pnpm --filter @bantayog/functions exec vitest run src/__tests__/rules/field-mode-sessions.rules.test.ts src/__tests__/rules/report-notes.rules.test.ts src/__tests__/rules/report-messages.rules.test.ts src/__tests__/rules/coordination.rules.test.ts src/__tests__/rules/report-sharing.rules.test.ts"` - PASS
- `firebase emulators:exec --only firestore "pnpm --filter @bantayog/functions exec vitest run src/__tests__/triggers/process-inbox-item.test.ts"` - PASS
- `npx turbo run lint typecheck` - PASS

### Refactor Audit 2026-04-23 — Implementation Continuation

**Branch:** `refactor/audit-2026-04-23-continuation`
Expand Down
2 changes: 2 additions & 0 deletions functions/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,13 @@
"file-type": "^22.0.1",
"firebase-admin": "^13.8.0",
"firebase-functions": "^7.2.5",
"ngeohash": "^0.6.3",
"sharp": "^0.34.5",
"zod": "^4.3.6"
},
"devDependencies": {
"@firebase/rules-unit-testing": "^5.0.0",
"@types/ngeohash": "^0.6.8",
"@types/node": "^20.12.0",
"firebase": "^12.0.0",
"firebase-functions-test": "^3.3.0",
Expand Down
149 changes: 140 additions & 9 deletions functions/src/__tests__/rules/coordination.rules.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { assertFails, assertSucceeds } from '@firebase/rules-unit-testing'
import { collection, doc, getDocs, setDoc, addDoc } from 'firebase/firestore'
import { collection, doc, getDoc, getDocs, setDoc, addDoc } from 'firebase/firestore'
import { afterAll, beforeAll, describe, it } from 'vitest'
import { authed, createTestEnv } from '../helpers/rules-harness.js'
import { seedActiveAccount, staffClaims, ts } from '../helpers/seed-factories.js'
Expand All @@ -13,6 +13,11 @@ beforeAll(async () => {
role: 'municipal_admin',
municipalityId: 'daet',
})
await seedActiveAccount(env, {
uid: 'other-admin',
role: 'municipal_admin',
municipalityId: 'mercedes',
})
})

afterAll(async () => {
Expand Down Expand Up @@ -134,20 +139,146 @@ describe('coordination collections rules', () => {
})

it('muni admin can read own municipality requests', async () => {
const unauthed = env.unauthenticatedContext().firestore()
await setDoc(doc(unauthed, 'agency_assistance_requests/req-1'), {
requestedByMunicipality: 'daet',
targetAgencyId: 'bfp-daet',
dispatchId: 'd-1',
requestType: 'BFP',
requestedAt: ts,
await env.withSecurityRulesDisabled(async (ctx) => {
await setDoc(doc(ctx.firestore(), 'agency_assistance_requests', 'req-1'), {
requestedByMunicipality: 'daet',
targetAgencyId: 'bfp-daet',
dispatchId: 'd-1',
requestType: 'BFP',
requestedAt: ts,
})
})
const db = authed(
env,
'daet-admin',
staffClaims({ role: 'municipal_admin', municipalityId: 'daet' }),
)
await assertSucceeds(getDoc(doc(db, 'agency_assistance_requests', 'req-1')))
})
})

describe('command_channel_threads/messages participant lookup', () => {
beforeAll(async () => {
await env.withSecurityRulesDisabled(async (ctx) => {
await setDoc(doc(ctx.firestore(), 'command_channel_threads', 'thread-1'), {
threadId: 'thread-1',
reportId: 'report-1',
threadType: 'agency_assistance',
subject: 'Need help',
participantUids: { 'daet-admin': true },
createdBy: 'daet-admin',
createdAt: ts,
updatedAt: ts,
schemaVersion: 1,
})

await setDoc(doc(ctx.firestore(), 'command_channel_messages', 'msg-1'), {
threadId: 'thread-1',
authorUid: 'daet-admin',
authorRole: 'municipal_admin',
body: 'hello',
createdAt: ts,
schemaVersion: 1,
})
})
})

it('allows participant to read thread', async () => {
const db = authed(
env,
'daet-admin',
staffClaims({ role: 'municipal_admin', municipalityId: 'daet' }),
)
await assertSucceeds(getDocs(collection(db, 'agency_assistance_requests')))
await assertSucceeds(getDoc(doc(db, 'command_channel_threads', 'thread-1')))
})

it('denies non-participant from reading thread', async () => {
const db = authed(
env,
'other-admin',
staffClaims({ role: 'municipal_admin', municipalityId: 'mercedes' }),
)
await assertFails(getDoc(doc(db, 'command_channel_threads', 'thread-1')))
})

it('allows participant to read message through parent thread lookup', async () => {
const db = authed(
env,
'daet-admin',
staffClaims({ role: 'municipal_admin', municipalityId: 'daet' }),
)
await assertSucceeds(getDoc(doc(db, 'command_channel_messages', 'msg-1')))
})

it('denies non-participant from reading message', async () => {
const db = authed(
env,
'other-admin',
staffClaims({ role: 'municipal_admin', municipalityId: 'mercedes' }),
)
await assertFails(getDoc(doc(db, 'command_channel_messages', 'msg-1')))
})
})
Comment on lines +213 to +274
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider adding unauthenticated read denial tests.

The participant lookup tests cover authorized/unauthorized participants but don't explicitly test unauthenticated access denial. While isActivePrivileged() implicitly requires authentication, explicit tests would strengthen coverage.

📝 Suggested test addition
it('denies unauthenticated reads on threads', async () => {
  const db = unauthed(env)
  await assertFails(getDoc(doc(db, 'command_channel_threads', 'thread-1')))
})

it('denies unauthenticated reads on messages', async () => {
  const db = unauthed(env)
  await assertFails(getDoc(doc(db, 'command_channel_messages', 'msg-1')))
})

Note: Requires importing unauthed from ../helpers/rules-harness.js.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@functions/src/__tests__/rules/coordination.rules.test.ts` around lines 213 -
274, Tests for participant lookup lack explicit unauthenticated access checks;
add two tests that use unauthed(env) to assert reads are denied on
'command_channel_threads' ('thread-1') and 'command_channel_messages' ('msg-1').
Import the unauthed helper from ../helpers/rules-harness.js if not already
present, then add an it('denies unauthenticated reads on threads', ...) and an
it('denies unauthenticated reads on messages', ...) that call
assertFails(getDoc(doc(db, 'command_channel_threads', 'thread-1'))) and
assertFails(getDoc(doc(db, 'command_channel_messages', 'msg-1'))) respectively
to strengthen coverage of isActivePrivileged() enforcement.

})

describe('command_channel_threads/messages — participant map key lookup', () => {
beforeAll(async () => {
await env.withSecurityRulesDisabled(async (ctx) => {
await setDoc(doc(ctx.firestore(), 'command_channel_threads', 'thread-1'), {
threadId: 'thread-1',
reportId: 'report-1',
threadType: 'agency_assistance',
subject: 'Need help',
participantUids: { 'daet-admin': true },
createdBy: 'daet-admin',
createdAt: ts,
updatedAt: ts,
schemaVersion: 1,
})

await setDoc(doc(ctx.firestore(), 'command_channel_messages', 'msg-1'), {
threadId: 'thread-1',
message: 'hello',
sentBy: 'daet-admin',
sentAt: ts,
schemaVersion: 1,
})
Comment on lines +292 to +298
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Message document fields inconsistent with schema.

The msg-2 document uses { message, sentBy, sentAt } fields which don't match commandChannelMessageDocSchema from packages/shared-validators/src/coordination.ts that expects { authorUid, authorRole, body, createdAt } (as used in msg-1).

While Firestore rules don't enforce schema, using inconsistent test data may:

  1. Mask future regressions if rules are updated to validate fields
  2. Confuse readers about the expected document structure
💡 Suggested alignment with schema
       await setDoc(doc(ctx.firestore(), 'command_channel_messages', 'msg-2'), {
         threadId: 'thread-2',
-        message: 'hello',
-        sentBy: 'daet-admin',
-        sentAt: ts,
+        authorUid: 'daet-admin',
+        authorRole: 'municipal_admin',
+        body: 'hello',
+        createdAt: ts,
         schemaVersion: 1,
       })
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@functions/src/__tests__/rules/coordination.rules.test.ts` around lines 292 -
298, The test inserts a `msg-2` document with fields {message, sentBy, sentAt}
that are inconsistent with the expected commandChannelMessageDocSchema used
elsewhere (fields like authorUid, authorRole, body, createdAt as in `msg-1`);
update the setDoc call that writes `doc(ctx.firestore(),
'command_channel_messages', 'msg-2')` in the coordination.rules.test to use the
same schema fields (authorUid, authorRole, body, createdAt and schemaVersion)
and values consistent with `msg-1` so both test messages follow the
`commandChannelMessageDocSchema`.

})
})
Comment thread
coderabbitai[bot] marked this conversation as resolved.

it('allows participant to read thread', async () => {
const db = authed(
env,
'daet-admin',
staffClaims({ role: 'municipal_admin', municipalityId: 'daet' }),
)
await assertSucceeds(getDoc(doc(db, 'command_channel_threads', 'thread-1')))
})

it('denies non-participant from reading thread', async () => {
const db = authed(
env,
'other-admin',
staffClaims({ role: 'municipal_admin', municipalityId: 'mercedes' }),
)
await assertFails(getDoc(doc(db, 'command_channel_threads', 'thread-1')))
})

it('allows participant to read a message when parent thread participantUids contains uid', async () => {
const db = authed(
env,
'daet-admin',
staffClaims({ role: 'municipal_admin', municipalityId: 'daet' }),
)
await assertSucceeds(getDoc(doc(db, 'command_channel_messages', 'msg-1')))
})

it('denies non-participant from reading a message', async () => {
const db = authed(
env,
'other-admin',
staffClaims({ role: 'municipal_admin', municipalityId: 'mercedes' }),
)
await assertFails(getDoc(doc(db, 'command_channel_messages', 'msg-1')))
})
})
83 changes: 83 additions & 0 deletions functions/src/__tests__/rules/field-mode-sessions.rules.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import { assertFails, assertSucceeds } from '@firebase/rules-unit-testing'
import { doc, getDoc, setDoc } from 'firebase/firestore'
import { afterAll, beforeAll, describe, it } from 'vitest'
import { authed, createTestEnv, unauthed } from '../helpers/rules-harness.js'
import { seedActiveAccount, staffClaims, ts } from '../helpers/seed-factories.js'

let env: Awaited<ReturnType<typeof createTestEnv>>

const sessionData = {
uid: 'daet-admin',
municipalityId: 'daet',
enteredAt: ts,
expiresAt: ts + 43200000,
isActive: true,
schemaVersion: 1,
}

beforeAll(async () => {
env = await createTestEnv('field-mode-sessions-rules-test')
await seedActiveAccount(env, {
uid: 'daet-admin',
role: 'municipal_admin',
municipalityId: 'daet',
})
await seedActiveAccount(env, {
uid: 'other-admin',
role: 'municipal_admin',
municipalityId: 'mercedes',
})
await seedActiveAccount(env, { uid: 'superadmin', role: 'provincial_superadmin' })

await env.withSecurityRulesDisabled(async (ctx) => {
await setDoc(doc(ctx.firestore(), 'field_mode_sessions', 'daet-admin'), sessionData)
})
})

afterAll(async () => {
await env.cleanup()
})

describe('field_mode_sessions rules', () => {
it('allows owner to read their own session', async () => {
const db = authed(
env,
'daet-admin',
staffClaims({ role: 'municipal_admin', municipalityId: 'daet' }),
)
await assertSucceeds(getDoc(doc(db, 'field_mode_sessions', 'daet-admin')))
})

it('allows owner to write their own session', async () => {
const db = authed(
env,
'daet-admin',
staffClaims({ role: 'municipal_admin', municipalityId: 'daet' }),
)
await assertSucceeds(setDoc(doc(db, 'field_mode_sessions', 'daet-admin'), sessionData))
})

it('denies other user reading another user session', async () => {
const db = authed(
env,
'other-admin',
staffClaims({ role: 'municipal_admin', municipalityId: 'mercedes' }),
)
await assertFails(getDoc(doc(db, 'field_mode_sessions', 'daet-admin')))
})

it('denies unauthenticated reads', async () => {
const db = unauthed(env)
await assertFails(getDoc(doc(db, 'field_mode_sessions', 'daet-admin')))
})

it('denies superadmin writes to field_mode_sessions', async () => {
const db = authed(env, 'superadmin', staffClaims({ role: 'provincial_superadmin' }))
await assertFails(setDoc(doc(db, 'field_mode_sessions', 'daet-admin'), sessionData))
})

it('allows superadmin reads', async () => {
const db = authed(env, 'superadmin', staffClaims({ role: 'provincial_superadmin' }))
await assertSucceeds(getDoc(doc(db, 'field_mode_sessions', 'daet-admin')))
})
})
86 changes: 86 additions & 0 deletions functions/src/__tests__/rules/report-messages.rules.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import { assertFails, assertSucceeds } from '@firebase/rules-unit-testing'
import { addDoc, collection, doc, getDoc, setDoc } from 'firebase/firestore'
import { afterAll, beforeAll, describe, it } from 'vitest'
import { authed, createTestEnv } from '../helpers/rules-harness.js'
import { seedActiveAccount, seedReport, staffClaims, ts } from '../helpers/seed-factories.js'

let env: Awaited<ReturnType<typeof createTestEnv>>

beforeAll(async () => {
env = await createTestEnv('report-messages-rules-test')
await seedActiveAccount(env, {
uid: 'daet-admin',
role: 'municipal_admin',
municipalityId: 'daet',
})
await seedActiveAccount(env, {
uid: 'bfp-admin',
role: 'agency_admin',
municipalityId: 'daet',
agencyId: 'bfp-daet',
})
await seedReport(env, 'report-1', {
municipalityId: 'daet',
opsOverrides: { municipalityId: 'daet', agencyIds: ['bfp-daet'] },
})
await env.withSecurityRulesDisabled(async (ctx) => {
await setDoc(doc(ctx.firestore(), 'reports', 'report-1', 'messages', 'msg-1'), {
authorUid: 'daet-admin',
body: 'Seed message',
createdAt: ts,
schemaVersion: 1,
})
})
})

afterAll(async () => {
await env.cleanup()
})

describe('reports/messages rules', () => {
it('allows muni admin to read a message', async () => {
const db = authed(
env,
'daet-admin',
staffClaims({ role: 'municipal_admin', municipalityId: 'daet' }),
)
await assertSucceeds(getDoc(doc(db, 'reports', 'report-1', 'messages', 'msg-1')))
})

it('allows agency admin to read a message when report_ops agencyIds includes their agency', async () => {
const db = authed(
env,
'bfp-admin',
staffClaims({ role: 'agency_admin', municipalityId: 'daet', agencyId: 'bfp-daet' }),
)
await assertSucceeds(getDoc(doc(db, 'reports', 'report-1', 'messages', 'msg-1')))
})
Comment on lines +50 to +57
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider adding agency admin read denial test.

The test verifies an agency admin with matching agencyId can read, but doesn't verify that an agency admin with a different agencyId is denied. This negative case would strengthen confidence in the agency scoping logic.

📝 Suggested test addition
it('denies agency admin read when their agency is not in report_ops.agencyIds', async () => {
  await seedActiveAccount(env, {
    uid: 'pnp-admin',
    role: 'agency_admin',
    municipalityId: 'daet',
    agencyId: 'pnp-daet', // Different agency
  })
  const db = authed(
    env,
    'pnp-admin',
    staffClaims({ role: 'agency_admin', municipalityId: 'daet', agencyId: 'pnp-daet' }),
  )
  // report-1 only has bfp-daet in agencyIds
  await assertFails(getDoc(doc(db, 'reports', 'report-1', 'messages', 'msg-1')))
})
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@functions/src/__tests__/rules/report-messages.rules.test.ts` around lines 50
- 57, Add a negative test that verifies an agency_admin with a different
agencyId cannot read a message: create a seeded account using seedActiveAccount
with uid (e.g., 'pnp-admin'), role 'agency_admin', municipalityId 'daet' and
agencyId 'pnp-daet', build an authenticated DB using authed with staffClaims({
role: 'agency_admin', municipalityId: 'daet', agencyId: 'pnp-daet' }) and assert
the read is denied by calling assertFails(getDoc(doc(db, 'reports', 'report-1',
'messages', 'msg-1'))); name the test something like "denies agency admin read
when their agency is not in report_ops.agencyIds" to mirror the existing
positive test and ensure it uses the same report id ('report-1') which only
includes 'bfp-daet' in its agencyIds.

🧹 Nitpick | 🔵 Trivial

Missing agency admin write test.

The test verifies agency admin can read messages but doesn't test that agency admin can create messages when their agency is in report_ops.agencyIds. This is a valid code path per the rules at firestore.rules.template:108-117.

📝 Suggested test addition
it('allows agency admin to write a message when report_ops agencyIds includes their agency', async () => {
  const db = authed(
    env,
    'bfp-admin',
    staffClaims({ role: 'agency_admin', municipalityId: 'daet', agencyId: 'bfp-daet' }),
  )
  await assertSucceeds(
    addDoc(collection(db, 'reports', 'report-1', 'messages'), {
      authorUid: 'bfp-admin',
      body: 'Agency response.',
      createdAt: ts,
      schemaVersion: 1,
    }),
  )
})
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@functions/src/__tests__/rules/report-messages.rules.test.ts` around lines 50
- 57, Add a new test in report-messages.rules.test.ts that asserts an
agency_admin can create a message when their agency is listed in
report_ops.agencyIds: use authed(env, 'bfp-admin', staffClaims({ role:
'agency_admin', municipalityId: 'daet', agencyId: 'bfp-daet' })), then call
assertSucceeds(addDoc(collection(db, 'reports', 'report-1', 'messages'), {
authorUid: 'bfp-admin', body: 'Agency response.', createdAt: ts, schemaVersion:
1 })) to mirror the read test and validate the write path governed by the
Firestore rules around report_ops.agencyIds.


it('allows muni admin to write a message', async () => {
const db = authed(
env,
'daet-admin',
staffClaims({ role: 'municipal_admin', municipalityId: 'daet' }),
)
await assertSucceeds(
addDoc(collection(db, 'reports', 'report-1', 'messages'), {
authorUid: 'daet-admin',
body: 'En route.',
createdAt: ts,
schemaVersion: 1,
}),
)
})

it('denies citizen writes to messages', async () => {
const db = authed(env, 'citizen-1', staffClaims({ role: 'citizen' }))
await assertFails(
addDoc(collection(db, 'reports', 'report-1', 'messages'), {
authorUid: 'citizen-1',
body: 'hi',
createdAt: ts,
schemaVersion: 1,
}),
)
})
Comment thread
coderabbitai[bot] marked this conversation as resolved.
})
Loading
Loading