fix: allow event owner to write participant check-in fields (#753) - #759
Conversation
checkInParticipant() runs as the organizer, but the participants update rule only permitted the participant themselves to write status/buddyPreference/updatedAt, so the transaction's participant write was denied and free-RSVP check-in always failed. Owner updates are now allowed for checkInStatus/checkedInAt/checkedInBy only, with checkInStatus validated to checked-in/checked-out. Callers updating anything else as the owner stay denied.
|
📝 WalkthroughWalkthroughThe participant update rule now supports participant attendance updates and event-owner check-in updates. Tests cover allowed owner writes and denied writes for restricted fields and non-owner users. ChangesParticipant Check-In Authorization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/firestore.rules.test.ts (1)
347-390: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTest rejection of invalid check-in statuses.
The rule restricts
checkInStatustochecked-inorchecked-out, but these tests do not verify that an owner write such ascheckInStatus: 'attending'fails. Add this negative case to preserve the authorization contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/firestore.rules.test.ts` around lines 347 - 390, Add a negative Firestore rules test alongside “Event owner marks participant checked-in -> allowed” that uses the event owner context to write an invalid checkInStatus such as “attending” and asserts the operation fails. Reuse the existing event and participant seeding setup and preserve the valid check-in and non-check-in field tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@firestore.rules`:
- Around line 349-351: Update the check-in rule condition surrounding
isEventOwner so checkedInBy, when changed, must equal request.auth.uid and
checkedInAt, when changed, must equal request.time. Require these fields to
exist in request.resource.data so affected updates cannot delete them, while
preserving the existing allowed-field and checkInStatus validation.
In `@tests/firestore.rules.test.ts`:
- Around line 347-389: Update the tests around the allowed and denied
participant writes to retain assertSucceeds/assertFails while adding explicit
Jest assertions that SonarCloud recognizes, such as asserting the successful
write result or verifying the persisted document afterward. Apply this to each
affected test without changing the Firestore rule scenarios.
---
Nitpick comments:
In `@tests/firestore.rules.test.ts`:
- Around line 347-390: Add a negative Firestore rules test alongside “Event
owner marks participant checked-in -> allowed” that uses the event owner context
to write an invalid checkInStatus such as “attending” and asserts the operation
fails. Reuse the existing event and participant seeding setup and preserve the
valid check-in and non-check-in field tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 880f15ec-8b3a-4066-8344-2a34edb6e992
📒 Files selected for processing (2)
firestore.rulestests/firestore.rules.test.ts
| isEventOwner(database, eventId) && | ||
| request.resource.data.diff(resource.data).affectedKeys().hasOnly(['checkInStatus', 'checkedInAt', 'checkedInBy']) && | ||
| (!('checkInStatus' in request.resource.data) || request.resource.data.checkInStatus in ['checked-in', 'checked-out']) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Bind check-in audit fields to the authenticated owner.
Line 350 restricts field names, but it does not validate checkedInBy or checkedInAt. An event owner can record another user as the actor or store an arbitrary timestamp. Require checkedInBy == request.auth.uid and require changed checkedInAt values to equal request.time. Reject deletion of these fields when they are affected.
#!/bin/bash
set -euo pipefail
# Inspect the check-in producer and all call sites to confirm that organizerId
# always represents the authenticated Firestore user.
rg -n -C 8 '\bcheckInParticipant\s*\(' app/src --glob '*.js'
# Inspect every participant check-in rule test and check-in field use.
rg -n -C 6 'checkInStatus|checkedInAt|checkedInBy' firestore.rules tests/firestore.rules.test.ts app/src/lib/checkInService.js🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@firestore.rules` around lines 349 - 351, Update the check-in rule condition
surrounding isEventOwner so checkedInBy, when changed, must equal
request.auth.uid and checkedInAt, when changed, must equal request.time. Require
these fields to exist in request.resource.data so affected updates cannot delete
them, while preserving the existing allowed-field and checkInStatus validation.
| test('Event owner marks participant checked-in -> allowed', async () => { | ||
| await seedDocument('events/event1', { title: 'Tech Fest', ownerId: 'clubOwner1' }); | ||
| await seedDocument('events/event1/participants/student1', { status: 'attending' }); | ||
| await assertSucceeds( | ||
| setDoc( | ||
| doc(getFirestoreContext('clubOwner1'), 'events/event1/participants/student1'), | ||
| { | ||
| checkInStatus: 'checked-in', | ||
| checkedInAt: serverTimestamp(), | ||
| checkedInBy: 'clubOwner1', | ||
| }, | ||
| { merge: true }, | ||
| ), | ||
| ); | ||
| }); | ||
|
|
||
| test('Event owner writes non-check-in field on participant -> denied', async () => { | ||
| await seedDocument('events/event1', { title: 'Tech Fest', ownerId: 'clubOwner1' }); | ||
| await seedDocument('events/event1/participants/student1', { status: 'attending' }); | ||
| await assertFails( | ||
| setDoc( | ||
| doc(getFirestoreContext('clubOwner1'), 'events/event1/participants/student1'), | ||
| { status: 'cancelled' }, | ||
| { merge: true }, | ||
| ), | ||
| ); | ||
| }); | ||
|
|
||
| test('Non-owner organizer writes check-in fields on participant -> denied', async () => { | ||
| await seedDocument('events/event1', { title: 'Tech Fest', ownerId: 'clubOwner1' }); | ||
| await seedDocument('events/event1/participants/student1', { status: 'attending' }); | ||
| await assertFails( | ||
| setDoc( | ||
| doc(getFirestoreContext('otherUser'), 'events/event1/participants/student1'), | ||
| { | ||
| checkInStatus: 'checked-in', | ||
| checkedInAt: serverTimestamp(), | ||
| checkedInBy: 'otherUser', | ||
| }, | ||
| { merge: true }, | ||
| ), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the SonarCloud assertion failures.
SonarCloud reports a missing assertion for each new test. Keep assertSucceeds and assertFails for rule evaluation, but add an explicit Jest assertion on the allowed write result or persisted document, or configure SonarCloud to recognize these helpers. The current analysis check fails.
#!/bin/bash
set -euo pipefail
rg -n -C 4 'assertSucceeds|assertFails|expect\(' tests/firestore.rules.test.ts🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 363-363: Add at least one assertion to this test case.
[failure] 375-375: Add at least one assertion to this test case.
[failure] 347-347: Add at least one assertion to this test case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/firestore.rules.test.ts` around lines 347 - 389, Update the tests
around the allowed and denied participant writes to retain
assertSucceeds/assertFails while adding explicit Jest assertions that SonarCloud
recognizes, such as asserting the successful write result or verifying the
persisted document afterward. Apply this to each affected test without changing
the Firestore rule scenarios.
1 participant
Closes #753
Problem
checkInParticipant()runs as the organizer and writescheckInStatus/checkedInAt/checkedInByonevents/{eventId}/participants/{participantId}, but the Firestore update rule only permitted theparticipant themselvesto writestatus/buddyPreference/updatedAt. The denied write fails the atomic transaction, so free-RSVP check-in always returnedUnable to complete check-in.Fix
Extended the participants
allow updaterule: the event owner (isEventOwner) may now write only the three check-in fields, withcheckInStatusvalidated to['checked-in','checked-out']. All other owner writes to participant docs remain denied, and participant-only self-updates are unchanged.Verification
npm run test:rules: 49 passed (was 46 on baseline; the 4 remaining failures are pre-existing on main, unrelated to this change).Summary by CodeRabbit
Bug Fixes
Tests