fix(rules): lock Watchlists reads to playlist members - #138
Draft
larabail wants to merge 5 commits into
Draft
Conversation
Any authenticated account could read every document in Watchlists,
including playlists it had never been invited to. Each one carries its
access code, so enumerating the collection also handed over the codes
needed to join the lists it turned up.
Reads now require membership, and the non-member update branch goes with
them. That branch existed so an older client could join a list by writing
itself into `Users`; joinPlaylist replaced it by checking the code on the
server, and once reads are closed it is only an open door.
Membership is accepted from either `memberUids` or `Users`, which is not
redundancy. Firestore allows a query only when the query's own filters
prove every document it can return is readable, and nothing can filter on
`Users`: arrayContains matches whole elements and the role is part of the
element. A rule reading only `Users` therefore refuses the
`where("memberUids", arrayContains: uid)` call the app makes to find its
own lists. A rule reading only `memberUids` instead locks members out of
any playlist that syncPlaylistMembers, being a write trigger, has never
touched. Accepting both allows the query and still covers those
documents. Backfilling `memberUids` and dropping the `Users` arm is the
tidier end state, but it needs a migration over live data and is left
undone here.
The edit-a-playlist dialogue found its document by downloading the whole
collection and matching on name and access code, so the new rule would
have broken it. It writes by id now, which it already held: the old
search also edited the wrong list when two shared a name and code, and
silently saved nothing when the globals it matched against went stale.
Restricting the read breaks any build that still finds its lists by
reading the collection, everywhere at once, because a rules deploy has no
staged rollout. Merging does not do that: nothing in CI deploys this
file, whose only firebase deploy is `--only functions`. It takes effect
when someone runs `firebase deploy --only firestore:rules` by hand, and
that is the step to hold until Play Console shows the pre-3.18 clients
are gone.
Take master's wording for the friend-writes gap and this branch's numbering: master reworded it while this branch renumbered it after removing the playlist read gap that this change closes. Keep both new imports in the rules suite, and keep this branch's version name over master's build number, at 3.18.7 because the calendar and people-score fixes already claim 3.18.5 and 3.18.6.
Correct what this rule says about its own deployment. The comment was written when nothing in CI deployed firestore.rules, so it told the reader to hold a manual `firebase deploy` until the old clients were gone. The release now deploys the rules and the indexes alongside the functions, so the merge itself is what reaches every phone and the merge is what has to be held. Take 3.18.7 over master's build number, since 3.18.5 and 3.18.6 are taken by the calendar and people-score fixes.
Keep 3.18.7 over master's build number; the people-score fix took 3.18.6.
Master reverted this change in #137 because it was merged before the client adoption it is gated on. The revert nets out against the original merge, so this branch keeps the restriction and takes 3.18.10 over master's build number.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Caution
Do not merge until production is on 3.18.x. This is #110, restored after
it was merged early and reverted in #137. Nothing about the change was wrong;
the timing was.
Merging is the deploy. The release runs
firebase deploy --only functions,firestore:rules,firestore:indexeson anymerge touching one of them, so squashing this restricts playlist reads on the
live project within one release run. There is no manual step left to hold.
The gate, concretely: Play Console must show the pre-3.18 clients are
gone. When #110 was merged, the most recent promotion to the Play production
track had shipped 3.17.4 -- a client that finds its playlists by reading
the whole
Watchlistscollection, which is exactly what this rule refuses.Every production user would have lost access at once, since a rules deploy
has no staged rollout.
The condition to check before merging:
just internal testing.
tail gone.
Whether adoption is high enough is a human call, not mine.
Restores #110. Original description follows.
Closes #41.
What this changes
match /Watchlists/{listId}carriedallow read: if signedIn(), so anyauthenticated account could read every playlist in the collection, including
ones it had never been invited to. Each playlist document carries its own
access code, so enumerating the collection also handed over the codes needed to
join the lists it turned up. This was KNOWN GAP #1 in
firestore.rules.Reads now require membership. The non-member
allow updatebranch goes withthem: it existed so an older client could join a list by writing itself into
Users, whichjoinPlaylistreplaced by verifying the code on the server, andonce reads are closed it is only an open door. Gap #1 is removed from the
KNOWN GAPS block and the remaining two are renumbered.
Why this way
Membership is accepted from either
memberUidsorUsers, and that is notredundancy. This is the part worth reviewing.
Firestore allows a query only when the query's own filters prove every document
it could return is readable — rules are not filters. Nothing can filter on
Users, becausearrayContainsmatches whole elements and the role is part ofthe element (
{uid: "Owner"}). So the obvious rule the issue describes,allow read: if playlistMember(resource.data)readingUsers, refuses thewhere("memberUids", arrayContains: uid)query the app makes to find its ownlists. I confirmed this against the emulator rather than assuming it:
getmemberUidsquerygetUsersonlymemberUidsonlyReading only
memberUidsfixes the query but introduces a different problem:syncPlaylistMembersis anonDocumentWrittentrigger, so a playlist notwritten since it was deployed has no
memberUidsat all, and its own memberslose access. Accepting either field allows the query and still covers those
documents, while refusing a non-member on both. There is a regression test for
each half.
Backfilling
memberUidsacross the collection and dropping theUsersarm isthe tidier end state. It needs a migration over live data, so it is
deliberately left undone here rather than bundled into a change that is
already gated on adoption.
The
'memberUids' in dataguard is load-bearing: testing a missing field inrules is an error, not a false.
One client path had to change.
lib/popups/list_edit_popup.dartfound thedocument to save by downloading the entire
Watchlistscollection and matchingon name and access code, so the new rule would have broken editing a playlist.
It writes by document id now — which it already had. The old search was also
wrong on its own terms: it edited every list sharing a name and code, and
silently saved nothing when the module-level globals it matched against had
gone stale. Both now have tests.
Every other client path was checked and already addresses playlists by id or
through the
memberUidsquery:playlist_service.dart,grant_access_dialogue.dart,list_add_popup.dart,list_result.dart,movie_add_popup.dart,tv_add_popup.dart, and joining viaplaylist_join.dart.How it was tested
firestore-tests/— the suite is the failing-before test for this fix. Againstthe unchanged rules, four of the new cases fail:
With the change, 89 pass and none fail. New cases cover: a member reading their
own list; a non-member refused; the access code not leaking to someone who
guesses the id; a playlist with no
memberUidsyet still readable by itsmembers and still refused to others; the
memberUidsquery returning only thecaller's lists; an unfiltered collection read refused; joining by direct write
refused; and an Owner still able to grant access.
test/popups/list_edit_popup_test.dart— both new cases fail against the olddialogue and pass with it: editing the list it was opened with when another
shares its name and access code, and saving when the globals have gone stale.
flutter analyze— clean, no issues.flutter test— 908 passing.firestore-tests— 89 passing (was 83).Note that CI does not run
firestore-tests/— no workflow references it, sothis suite only runs when someone runs it locally. That is not this PR's to fix,
but it is worth knowing when reviewing a rules change.
Not tested on a device: the rules were exercised against the Firestore emulator,
which is what that suite is for.
Checklist
master; no commits made directly onmasterversion:inpubspec.yamlbumped to match what this changes —3.18.1→3.18.2(PATCH,fix); verified withtool/check_version_bump.pyflutter analyzeis cleanflutter testpasses, and new behaviour has tests covering it.arbfile changed andgen-l10nwas not neededfirestore-tests/syncPlaylistMembersentry now saysmemberUidsis what makes a member-only read queryableCo-authored-bytrailer