diff --git a/README.md b/README.md index efa6ff3..15458b0 100644 --- a/README.md +++ b/README.md @@ -183,9 +183,9 @@ Cloud Firestore, four collections: | Field | Type | Notes | |---|---|---| | `id` | string | matches the Firebase Auth UID | -| `email` | string | | -| `department` | string | | -| `createdAt` | number | | +| `email` | string | must equal the session's own address, and end in `@ogr.akdeniz.edu.tr` | +| `department` | string | non-empty; `ownDepartment()` reads it to decide what the account may read | +| `createdAt` | timestamp | `request.time`; profiles written before this carry epoch millis and are still read | Older user documents may still carry `hasUploadedNote` and `points`. Neither is read or written any more: the contribution gate is derived from the user's notes, and so is the @@ -218,13 +218,25 @@ Firebase Console, so they can be reviewed and versioned. Deploy with: firebase deploy --only firestore:rules ``` +**Registration is restricted on the server, not only on the form.** The create rule used +to check that you were writing your own document and that its `id` field agreed with the +document id, and nothing else — so the `@ogr.akdeniz.edu.tr` requirement lived only in +`RegisterActivity`. Firebase Auth accepts any address, so anyone could sign up with a +personal one, skip the Android screen entirely and write themselves a profile carrying any +`email`, any `department`, any `createdAt` and any extra fields they liked. Since reads are +now scoped by `department` and `ownDepartment()` reads it from this very document, that +forged profile opened a department's notes. The rule now pins the field set, takes the +address from `request.auth.token.email` rather than from the request body, checks the +domain itself, requires a non-empty department, and requires `createdAt` to be +`request.time`. + The rules require authentication everywhere, restrict note edits and deletes to the uploader, and pin each rating document to its author's UID. The delete rule is the *only* ownership check on deletion — the app code does not verify it. **Reads are scoped, not merely filtered.** Every read used to be `if isSignedIn()`, so any -signed-in student could pull every profile, every note and every attached file in the -university. That the app only ever asked for its own department was a property of the app, +signed-in student could pull every profile, every note, every attached file and every vote +in the university — the votes being a record of who rated what, and how. That the app only ever asked for its own department was a property of the app, and the app is not the only client anyone can write. Notes and their file subcollections are now readable only when their `department` matches the department on the caller's own profile, or the note is the caller's own. Firestore evaluates a list against the *query* @@ -236,7 +248,8 @@ single-branch rule rejected them outright — the feed opened empty in productio how the branch was found. Profiles went further and are readable only by their owner: all four `getUser` calls in the app pass the caller's own uid, and the leaderboard takes the uploader's name from the note, so the open rule was -granting an access nobody needed. +granting an access nobody needed. Votes are the same shape of fix — readable by the voter, +and by the owner of the note being rated, which are exactly the two reads the app makes. **A vote can only be deleted with its note.** `ratings` is a top-level collection rather than a subcollection, so deleting a note never touched the votes cast on it and @@ -272,11 +285,22 @@ and the two score fields; everything else was open to a client that skipped the UI, which an attacker was never obliged to use. They also verify the scoring rather than trusting it. A vote writes the note's totals and -the rating document in one atomic commit, and the rule for the note uses `getAfter()` to -read the rating being written in that same commit, recompute the expected `ratingSum` and -`ratingCount` from it, and reject anything that does not match. So the totals cannot move -without a real vote behind them, a changed vote cannot be counted twice, and rating your -own note is refused by the server rather than only by the client. +the rating document in one atomic commit, and each side of that commit checks the other +with `getAfter()`: the note's rule recomputes the expected `ratingSum` and `ratingCount` +from the rating being written beside it, and the rating's rule recomputes the same two +numbers and requires the note to actually carry them. So the totals cannot move without a +real vote behind them, a vote cannot be written without the totals that follow from it, a +changed vote cannot be counted twice, and rating your own note is refused by the server +rather than only by the client. + +The second direction was missing until recently, and its absence was the worst hole in +this file. Only "totals moved, so show me the vote" was enforced; "a vote was written, so +the totals must follow" was not, which left a rating document writable entirely on its +own — and the test suite asserted that as correct behaviour, so CI stayed green over it. +It was not untidiness but unbounded inflation, four points a lap from one voter: write the +vote alone as 1, raise it to 5 with the totals (`sum += 4`, count unchanged because the +rule sees a changed vote), write it alone as 1 again, repeat. The lone write is now +refused, which breaks the loop at step one. The expected totals are the same arithmetic as [`RatingCalculator`](app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingCalculation.kt), @@ -423,7 +447,7 @@ the app is built around — what the screen actually shows. | Suite | What it pins down | |---|---| -| `RatingCalculatorTest` | First-time votes, changed votes, unchanged votes, average recalculation, and two inconsistent-data guards — the total flooring at zero, and the vote count flooring at one so the average never divides by zero. | +| `RatingCalculatorTest` | First-time votes, changed votes, unchanged votes, and two inconsistent-data guards: the total flooring at zero, and the vote count flooring at one. Both floors are mirrored in `firestore.rules`, so the client and the server have to agree on them exactly. | | `FeedViewModelTest` | The contribution gate: locked without an upload, unlocked with one, and locked for a missing profile or a blank department — and that the department query is never even built while locked. Also the paging: the window each page asks for, when there is more to fetch, that the last page stops asking, and that the list stays on screen while the next page loads. | | `UploadViewModelTest` | The uid, email and department written onto a note, including the `UNKNOWN_DEPARTMENT` fallback that keeps a blank department out of `whereEqualTo` queries. | | `NoteDetailViewModelTest` | Metadata and file content loading as separate states, no file read for a note without one, and the rating rules (own note, missing session, deleted note). | @@ -488,7 +512,7 @@ enforces — who may delete a note, whether a score can move without a vote behi invisible to them. That is the layer an attacker actually meets: the Android client can be replaced, the rules cannot. -65 tests in [`firestore-tests/rules.test.js`](firestore-tests/rules.test.js) run +87 tests in [`firestore-tests/rules.test.js`](firestore-tests/rules.test.js) run [`firestore.rules`](firestore.rules) against the local Firestore emulator through `@firebase/rules-unit-testing`. `npm test` starts the emulator, runs the suite and shuts it down again; nothing touches the real project, and no billing account is involved. @@ -496,12 +520,12 @@ it down again; nothing touches the real project, and no billing account is invol | Group | What it pins down | |---|---| | `signed-out access` | Every collection is closed to an unauthenticated client — the one guard shared by all four rule blocks. | -| `users` | Self-registration only, the document id matching the `id` field, that nobody updates a profile afterwards — including the two shapes of the old hole, awarding yourself points and writing points onto someone else — and that a profile is readable only by its owner. | +| `users` | Self-registration only, the document id matching the `id` field, that a personal address cannot register at all and the profile's `email` must be the session's own — removing those two clauses turns exactly those two tests red — that a blank department, a client-chosen `createdAt` and any unexpected or missing field are refused, that nobody updates a profile afterwards — including the two shapes of the old hole, awarding yourself points and writing points onto someone else — and that a profile is readable only by its owner. | | `notes` (reads) | A note in the reader's own department is readable and another department's note is not; an unfiltered query is refused, one filtered to the reader's own department is allowed, and one filtered to another department is refused. Two more cover the query the app actually builds for a profile: `where uploaderUid == me` is allowed without a department filter, and the same query for someone else's uid is refused. | -| `notes` | `uploaderUid` cannot be forged and a note cannot be born with a score; the uploader may edit metadata but not the totals, the department, or the identity fields; a vote and the totals it implies are accepted only together, must match the arithmetic exactly, cannot be counted twice, and cannot be cast on your own note; delete is owner-only. | +| `notes` | `uploaderUid` cannot be forged and a note cannot be born with a score; an edit may touch only the metadata fields and each is type- and size-checked, so a field nobody expects cannot be introduced and `updatedAt` cannot be backdated; a vote and the totals it implies are accepted only together, must match the arithmetic exactly, cannot be counted twice, and cannot be cast on your own note; delete is owner-only. | | `note creation is pinned to an exact shape` | One test per way a hand-rolled request used to get through: an unexpected field, a missing one, a resurrected `avgRating`, another department, a forged `uploaderName`, an email address written onto a note, a client-chosen `createdAt`, a wrong type, and an empty or oversized title. Each asserts the refusal, so relaxing any single check turns exactly one test red. | -| `note content` | The batched note-plus-file upload, owner-only create, replace and delete, that a file on another department's note cannot be read — otherwise the boundary is walked around through the file path — that a content document carrying anything but `fileData` is refused, and that a batch cannot pair a legitimate note of its own with a file smuggled onto someone else's. | -| `ratings` | The `_` document id, which is what stops one user voting as another, plus the 1–5 integer range. Deletion has four: a vote cannot be removed on its own, not even by the note's owner; the owner may remove it in the same commit that deletes the note; a stranger cannot reach it by deleting a note they do not own; and a vote whose note is already gone stays refused, which is the gap the prune script covers. | +| `note content` | The batched note-plus-file upload, owner-only create, replace and delete, that replacing a file must keep the same shape — create pinned it and update did not, so writing the document twice skipped the check — that a file on another department's note cannot be read — otherwise the boundary is walked around through the file path — that a content document carrying anything but `fileData` is refused, and that a batch cannot pair a legitimate note of its own with a file smuggled onto someone else's. | +| `ratings` | The `_` document id, which is what stops one user voting as another, plus the 1–5 integer range. Six cover who may read a vote: your own is readable, someone else's is not, the whole collection cannot be pulled, and of the two queries the app makes — your own votes, and the votes on your own note when deleting it — each is allowed for the person entitled to it and refused for anyone else. Six cover the link back to the note: a vote cannot be written or changed without the totals that follow from it, the totals must match the vote from this side too, the inflation loop above is refused at its first move, and — because the app writes through `runTransaction` rather than a batch — the transaction the repository actually issues is exercised in both directions. Removing that one clause turns exactly those three red and leaves the other 86 green. Deletion has four: a vote cannot be removed on its own, not even by the note's owner; the owner may remove it in the same commit that deletes the note; a stranger cannot reach it by deleting a note they do not own; and a vote whose note is already gone stays refused, which is the gap the prune script covers. | | `unmatched paths` | A path no `match` block covers is denied, so adding a collection to the app without adding a rule fails closed rather than open. | Fixtures are seeded with `withSecurityRulesDisabled`, so a broken rule surfaces as a @@ -513,9 +537,11 @@ everything would otherwise pass a suite made only of `assertFails`. ## Data migrations -The feed and the leaderboard currently download every note in the department and sort in -the client. Paginating them means ordering in the query instead — and Firestore's -ordering has two behaviours that turn old data into damage a user can see: +### Normalising `createdAt` + +The feed and the leaderboard used to download every note in the department and sort in the +client. Paginating them meant ordering in the query instead — and Firestore's ordering has +two behaviours that turn old data into damage a user can see: - **A document that lacks the ordered field is excluded from the query.** Not sorted last: absent. A note written before `createdAt` existed would vanish from the feed @@ -523,8 +549,8 @@ ordering has two behaviours that turn old data into damage a user can see: - **Values sort by type before value, and numbers come before timestamps.** A note whose `createdAt` is epoch millis would land below every timestamped note whatever its date. -`toPost()` still reads both shapes, which is the evidence that both exist. So the data is -normalised before the ordering changes, not after: +`toPost()` still reads both shapes, which is the evidence that both exist. The data was +normalised before the ordering changed, not after: ```bash cd firestore-tests @@ -586,7 +612,9 @@ against the emulator: the orphan goes, the live vote stays, and the note id is r the field rather than split out of the document id, which would be ambiguous whenever a uid contains an underscore. -### Continuous integration +--- + +## Continuous integration [`.github/workflows/android.yml`](.github/workflows/android.yml) runs on every push to `main` and on every pull request. Two jobs run in parallel: unit tests, a debug build and @@ -597,6 +625,12 @@ Because `google-services.json` is not in the repository, CI copies the example f place before building. The placeholder credentials are enough — the Google Services plugin only parses the file at build time and never contacts Firebase. +That is also the limit of what CI can tell you about the security rules: it runs +`firestore.rules` against the emulator, which proves the file is correct and says nothing +about whether the Firebase project is running it. Nothing here holds credentials for the +live project, so nothing here can check. Deploying is part of merging a rules change — see +[Known limitations](#known-limitations). + --- ## Project structure diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseUserRepository.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseUserRepository.kt index 3f350ea..f512627 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseUserRepository.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseUserRepository.kt @@ -1,5 +1,7 @@ package duygu.yilmaz.campusnote.data.repository +import com.google.firebase.firestore.DocumentSnapshot +import com.google.firebase.firestore.FieldValue import com.google.firebase.firestore.FirebaseFirestore import duygu.yilmaz.campusnote.data.model.UserProfile import kotlinx.coroutines.tasks.await @@ -8,10 +10,23 @@ class FirebaseUserRepository( private val firestore: FirebaseFirestore = FirebaseFirestore.getInstance() ) : UserRepository { + /** + * Profili alan alan yazıyor, veri sınıfını olduğu gibi değil: `createdAt` sunucu + * saatinden geliyor ve [FieldValue.serverTimestamp] bir `Long` alanına sığmıyor. + * Güvenlik kuralı da `request.time` bekliyor — kayıt tarihi istemcinin + * söylediği şey olmamalı. + */ override suspend fun saveUser(user: UserProfile) { firestore.collection(USERS_COLLECTION) .document(user.id) - .set(user) + .set( + mapOf( + "id" to user.id, + "email" to user.email, + "department" to user.department, + "createdAt" to FieldValue.serverTimestamp() + ) + ) .await() } @@ -27,10 +42,25 @@ class FirebaseUserRepository( id = document.getString("id") ?: document.id, email = document.getString("email") ?: "", department = document.getString("department") ?: "", - createdAt = document.getLong("createdAt") ?: 0L + createdAt = document.createdAtMillis() ) } + /** + * Kayıt tarihi artık sunucu saatinden gelen bir `Timestamp`; sunucu saatine + * geçmeden önce yazılmış profiller hâlâ epoch millis taşıyor. + * + * `getTimestamp()` alan sayı olduğunda null dönmüyor, [RuntimeException] + * fırlatıyor — yani `?:` ile yedeklemek yetmiyor, çağrının kendisi sarılmak + * zorunda. Aynı tuzak `FirebaseNoteRepository.toPost()` içinde de var ve orada + * da böyle çözülmüş; ikisi ayrışırsa biri eski veride çöker. + */ + private fun DocumentSnapshot.createdAtMillis(): Long = try { + getTimestamp("createdAt")?.toDate()?.time ?: getLong("createdAt") ?: 0L + } catch (_: Exception) { + getLong("createdAt") ?: 0L + } + private companion object { const val USERS_COLLECTION = "users" } diff --git a/app/src/main/java/duygu/yilmaz/campusnote/ui/auth/RegisterViewModel.kt b/app/src/main/java/duygu/yilmaz/campusnote/ui/auth/RegisterViewModel.kt index 3a4c922..3662ba4 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/ui/auth/RegisterViewModel.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/ui/auth/RegisterViewModel.kt @@ -41,8 +41,8 @@ class RegisterViewModel( UserProfile( id = authenticatedUser.uid, email = email, - department = department, - createdAt = System.currentTimeMillis() + department = department + // createdAt sunucuda yazılıyor; bkz. FirebaseUserRepository. ) ) AuthUiState.Success diff --git a/firestore-tests/rules.test.js b/firestore-tests/rules.test.js index 1b42c20..57b06f2 100644 --- a/firestore-tests/rules.test.js +++ b/firestore-tests/rules.test.js @@ -13,6 +13,7 @@ import { query, serverTimestamp, setDoc, + runTransaction, updateDoc, where, writeBatch, @@ -125,17 +126,24 @@ function noteFields(overrides = {}) { * @param uid oy dokümanının sahibi olarak yazılacak uid — kurala göre çağıranınki olmalı * @param totals notun yazılacak yeni değerleri; `extra` ile fazladan alan sızdırılır */ -function vote(db, uid, noteId, rating, { sum, count, extra = {} }) { +function vote( + db, + uid, + noteId, + rating, + { sum, count, extra = {}, id = `${uid}_${noteId}`, voteOverrides = {} }, +) { const batch = writeBatch(db); batch.update(doc(db, "notes", noteId), { ratingSum: sum, ratingCount: count, ...extra, }); - batch.set(doc(db, "ratings", `${uid}_${noteId}`), { + batch.set(doc(db, "ratings", id), { uid, noteId, rating, + ...voteOverrides, }); return batch.commit(); } @@ -172,26 +180,91 @@ describe("users", () => { await assertFails(getDoc(doc(rater, "users", OWNER))); }); + /** A profile in exactly the shape the create rule now demands. */ + const profileFields = (overrides = {}) => ({ + id: RATER, + email: RATER_EMAIL, + department: DEPARTMENT, + createdAt: serverTimestamp(), + ...overrides, + }); + test("registration writes a profile whose id matches the caller", async () => { await testEnv.clearFirestore(); - await assertSucceeds( - setDoc(doc(rater, "users", RATER), { - id: RATER, - email: "rater@ogr.akdeniz.edu.tr", - department: "Bilgisayar Mühendisliği", - createdAt: 1, - }), - ); + await assertSucceeds(setDoc(doc(rater, "users", RATER), profileFields())); }); test("a user cannot create a profile under someone else's uid", async () => { await testEnv.clearFirestore(); - await assertFails(setDoc(doc(rater, "users", OWNER), { id: OWNER })); + await assertFails( + setDoc(doc(rater, "users", OWNER), profileFields({ id: OWNER })), + ); }); test("a create whose id field disagrees with the document id is rejected", async () => { await testEnv.clearFirestore(); - await assertFails(setDoc(doc(rater, "users", RATER), { id: OWNER })); + await assertFails( + setDoc(doc(rater, "users", RATER), profileFields({ id: OWNER })), + ); + }); + + /** + * The university restriction used to live only in RegisterActivity. Firebase Auth + * accepts any address, so anyone could sign up with a personal one, skip the + * Android screen and write themselves a profile — and since reads are scoped by + * `department`, which is read from this very document, that profile opened a + * department's notes. + */ + test("a profile cannot be created from a non-university account", async () => { + await testEnv.clearFirestore(); + const outsider = testEnv + .authenticatedContext("outsider-uid", { email: "someone@gmail.com" }) + .firestore(); + + await assertFails( + setDoc( + doc(outsider, "users", "outsider-uid"), + profileFields({ id: "outsider-uid", email: "someone@gmail.com" }), + ), + ); + }); + + test("the profile's email must be the session's own", async () => { + await testEnv.clearFirestore(); + // Claiming a university address while signed in as someone else was the other + // half of the same forgery. + await assertFails( + setDoc( + doc(rater, "users", RATER), + profileFields({ email: "rektor@ogr.akdeniz.edu.tr" }), + ), + ); + }); + + test("a profile carrying an unexpected or missing field is rejected", async () => { + await testEnv.clearFirestore(); + await assertFails( + setDoc(doc(rater, "users", RATER), profileFields({ points: 999999 })), + ); + + const { department, ...withoutDepartment } = profileFields(); + await assertFails(setDoc(doc(rater, "users", RATER), withoutDepartment)); + }); + + test("a blank department is rejected", async () => { + await testEnv.clearFirestore(); + // `ownDepartment()` reads this field to decide what the account may read, so an + // empty one is not a cosmetic problem. + await assertFails( + setDoc(doc(rater, "users", RATER), profileFields({ department: "" })), + ); + }); + + test("createdAt must be the server's clock", async () => { + await testEnv.clearFirestore(); + await assertFails( + setDoc(doc(rater, "users", RATER), profileFields({ createdAt: 1 })), + ); }); test("nobody may update a profile, not even its owner", async () => { @@ -422,7 +495,56 @@ describe("notes", () => { test("the uploader may edit their note's metadata", async () => { await assertSucceeds( - updateDoc(doc(owner, "notes", NOTE), { title: "Güncellenmiş başlık" }), + updateDoc(doc(owner, "notes", NOTE), { + title: "Güncellenmiş başlık", + updatedAt: serverTimestamp(), + }), + ); + }); + + /** + * The check used to be a blacklist — it named the fields that must not move and + * let everything else through. A field nobody thought of, including one added + * next year, was writable by default, and the metadata that *was* allowed had no + * type or size bound at all. + */ + test("an edit cannot introduce a field nobody expects", async () => { + await assertFails( + updateDoc(doc(owner, "notes", NOTE), { + title: "Güncellenmiş başlık", + updatedAt: serverTimestamp(), + isFeatured: true, + }), + ); + }); + + test("an edit cannot put the wrong type or size in the metadata", async () => { + await assertFails( + updateDoc(doc(owner, "notes", NOTE), { + title: 42, + updatedAt: serverTimestamp(), + }), + ); + await assertFails( + updateDoc(doc(owner, "notes", NOTE), { + title: "a".repeat(201), + updatedAt: serverTimestamp(), + }), + ); + await assertFails( + updateDoc(doc(owner, "notes", NOTE), { + title: "", + updatedAt: serverTimestamp(), + }), + ); + }); + + test("an edit cannot backdate updatedAt", async () => { + await assertFails( + updateDoc(doc(owner, "notes", NOTE), { + title: "Güncellenmiş başlık", + updatedAt: 1, + }), ); }); @@ -547,6 +669,22 @@ describe("note content", () => { await assertSucceeds(deleteDoc(doc(owner, "notes", NOTE, "content", "file"))); }); + /** + * Create pinned the shape and update did not, so the whole check could be skipped + * by writing the document twice — a valid one first, anything at all second. + */ + test("replacing a file must keep the same shape", async () => { + await assertFails( + updateDoc(doc(owner, "notes", NOTE, "content", "file"), { + fileData: "eWVuaQ==", + uploaderUid: RATER, + }), + ); + await assertFails( + updateDoc(doc(owner, "notes", NOTE, "content", "file"), { fileData: 42 }), + ); + }); + test("someone else cannot overwrite or delete the file", async () => { await assertFails( setDoc(doc(rater, "notes", NOTE, "content", "file"), { @@ -611,64 +749,205 @@ describe("note content", () => { describe("ratings", () => { const validVote = { uid: RATER, noteId: NOTE, rating: 4 }; - test("a vote is written under _", async () => { + /** Seeds a vote without going through the rules, so a broken rule shows up as a + * failed assertion rather than a failed setup. */ + const seedVote = async (vote = validVote, id = `${RATER}_${NOTE}`) => { + await testEnv.withSecurityRulesDisabled(async (context) => { + await setDoc(doc(context.firestore(), "ratings", id), vote); + }); + }; + + + test("a user may read their own vote", async () => { + await seedVote(); + await assertSucceeds(getDoc(doc(rater, "ratings", `${RATER}_${NOTE}`))); + }); + + /** + * The open read rule exposed every vote in the database to every signed-in user: + * who rated what, and how. The app never needed that — the voter reads their own + * document inside the rating transaction, and the note's owner reads the votes on + * their own note when deleting it. + */ + test("a user cannot read someone else's vote", async () => { + await seedVote({ uid: OWNER, noteId: NOTE, rating: 4 }, `${OWNER}_${NOTE}`); + await assertFails(getDoc(doc(rater, "ratings", `${OWNER}_${NOTE}`))); + }); + + test("the whole ratings collection cannot be pulled", async () => { + await assertFails(getDocs(collection(rater, "ratings"))); + }); + + test("a user may query their own votes", async () => { await assertSucceeds( - setDoc(doc(rater, "ratings", `${RATER}_${NOTE}`), validVote), + getDocs(query(collection(rater, "ratings"), where("uid", "==", RATER))), ); }); - test("re-voting replaces the existing document", async () => { + test("the note's owner may query the votes on their note", async () => { + // This is the read `deleteNote` performs before removing a note. await assertSucceeds( + getDocs(query(collection(owner, "ratings"), where("noteId", "==", NOTE))), + ); + }); + + test("a stranger cannot query the votes on someone else's note", async () => { + await assertFails( + getDocs(query(collection(rater, "ratings"), where("noteId", "==", NOTE))), + ); + }); + + test("a vote is written under _", async () => { + await assertSucceeds(vote(rater, RATER, NOTE, 4, { sum: 4, count: 1 })); + }); + + test("re-voting replaces the existing document", async () => { + await assertSucceeds(vote(rater, RATER, NOTE, 4, { sum: 4, count: 1 })); + // Changing 4 to 2 moves the sum by the difference and leaves the count. + await assertSucceeds(vote(rater, RATER, NOTE, 2, { sum: 2, count: 1 })); + }); + + /** + * The other half of the link, and the one that was missing. + * + * The note's own rule says "if the totals move, a vote must justify them". It + * never said the reverse — "if a vote is written, the totals must move with it" — + * so a rating document could be written entirely on its own, and the test here + * asserted that as correct behaviour. + * + * That was not a tidiness problem. It was unbounded score inflation from a single + * voter, four points a lap: + * + * 1. write the vote alone as 1 (totals untouched) + * 2. raise it to 5 with the totals (sum += 4, count unchanged) + * 3. write it alone as 1 again (totals untouched) + * 4. go to 2 + * + * Step 1 is what is now refused, which breaks the loop at its first move. + */ + test("a vote cannot be written without the totals that follow from it", async () => { + await assertFails( setDoc(doc(rater, "ratings", `${RATER}_${NOTE}`), validVote), ); - await assertSucceeds( + }); + + test("an existing vote cannot be changed without the totals following", async () => { + await assertSucceeds(vote(rater, RATER, NOTE, 5, { sum: 5, count: 1 })); + + // The lone downgrade that made the loop above possible. + await assertFails( + setDoc(doc(rater, "ratings", `${RATER}_${NOTE}`), { + ...validVote, + rating: 1, + }), + ); + }); + + test("the inflation loop does not survive its first lap", async () => { + await assertSucceeds(vote(rater, RATER, NOTE, 5, { sum: 5, count: 1 })); + + // Every remaining step of the loop, each refused on its own: dropping the vote + // silently, and then claiming the difference as if it had been dropped. + await assertFails( setDoc(doc(rater, "ratings", `${RATER}_${NOTE}`), { ...validVote, - rating: 2, + rating: 1, + }), + ); + await assertFails(vote(rater, RATER, NOTE, 5, { sum: 9, count: 1 })); + }); + + /** + * The app does not use a batch here — `FirebaseRatingRepository.submitRating` + * wraps the two writes in `runTransaction`, so it can re-read the note and + * compute the new totals from what it finds. Rules treat a transaction's writes + * as one commit the same way they treat a batch's, and `getAfter()` sees across + * it, but that is the mechanism the app actually ships and nothing here exercised + * it. This does, with the arithmetic the repository performs. + */ + test("the vote the app writes, through a transaction, is accepted", async () => { + await assertSucceeds( + runTransaction(rater, async (transaction) => { + const noteRef = doc(rater, "notes", NOTE); + const note = await transaction.get(noteRef); + + transaction.update(noteRef, { + ratingSum: note.data().ratingSum + 4, + ratingCount: note.data().ratingCount + 1, + }); + transaction.set(doc(rater, "ratings", `${RATER}_${NOTE}`), { + uid: RATER, + noteId: NOTE, + rating: 4, + }); + }), + ); + }); + + test("a transaction cannot write the vote without the totals either", async () => { + await assertFails( + runTransaction(rater, async (transaction) => { + transaction.set(doc(rater, "ratings", `${RATER}_${NOTE}`), { + uid: RATER, + noteId: NOTE, + rating: 4, + }); }), ); }); + test("the totals must match the vote from this side too", async () => { + // Written as a batch, so the note rule is satisfied by the vote beside it and + // the arithmetic is the only thing left to reject. + await assertFails(vote(rater, RATER, NOTE, 1, { sum: 5, count: 1 })); + await assertFails(vote(rater, RATER, NOTE, 4, { sum: 4, count: 2 })); + }); + test("a user cannot vote in someone else's name", async () => { // The uid field and the document id are checked separately, so both // halves of the forgery have to be rejected on their own. await assertFails( - setDoc(doc(rater, "ratings", `${OWNER}_${NOTE}`), { - ...validVote, - uid: OWNER, + vote(rater, RATER, NOTE, 4, { + sum: 4, + count: 1, + id: `${OWNER}_${NOTE}`, + voteOverrides: { uid: OWNER }, }), ); await assertFails( - setDoc(doc(rater, "ratings", `${OWNER}_${NOTE}`), validVote), + vote(rater, RATER, NOTE, 4, { sum: 4, count: 1, id: `${OWNER}_${NOTE}` }), ); }); test("the document id must match the uid and note it claims", async () => { await assertFails( - setDoc(doc(rater, "ratings", `${RATER}_other-note`), validVote), + vote(rater, RATER, NOTE, 4, { + sum: 4, + count: 1, + id: `${RATER}_other-note`, + }), + ); + await assertFails( + vote(rater, RATER, NOTE, 4, { sum: 4, count: 1, id: "arbitrary-id" }), ); - await assertFails(setDoc(doc(rater, "ratings", "arbitrary-id"), validVote)); }); test("the rating must be an integer between 1 and 5", async () => { - for (const rating of [0, 6, -1, 3.5, "4"]) { - await assertFails( - setDoc(doc(rater, "ratings", `${RATER}_${NOTE}`), { - ...validVote, - rating, - }), - ); + // Each case carries the totals the rule would compute *if* the rating were + // valid, so the range check is the only thing left that can reject it. The + // string case is the exception: it fails on the arithmetic as well, since a + // string cannot be added to a sum. + for (const [rating, sum] of [ + [0, 0], + [6, 6], + [-1, 0], + [3.5, 3.5], + ["4", 4], + ]) { + await assertFails(vote(rater, RATER, NOTE, rating, { sum, count: 1 })); } }); - /** Seeds a vote without going through the rules, so a broken rule shows up as a - * failed assertion rather than a failed setup. */ - const seedVote = async (vote = validVote, id = `${RATER}_${NOTE}`) => { - await testEnv.withSecurityRulesDisabled(async (context) => { - await setDoc(doc(context.firestore(), "ratings", id), vote); - }); - }; - /** * A vote on its own must stay put. Deleting one does not lower the note's total, * but it does hand the voter a second first-time vote — the rule would count it @@ -699,6 +978,37 @@ describe("ratings", () => { await assertSucceeds(batch.commit()); }); + /** + * The realistic delete: a note with many votes, all removed in the commit that + * removes it. Rules cap the document access calls one request may make, and each + * rating delete here evaluates `existsAfter()` and `get()` on the note. If those + * are counted per rating rather than cached per path, a note with a dozen votes + * becomes undeletable — which is exactly the size of note that would have them. + */ + test("a note with many votes can still be deleted in one commit", async () => { + const voters = Array.from({ length: 12 }, (_, i) => `voter-${i}`); + + await testEnv.withSecurityRulesDisabled(async (context) => { + const db = context.firestore(); + for (const uid of voters) { + await setDoc(doc(db, "ratings", `${uid}_${NOTE}`), { + uid, + noteId: NOTE, + rating: 4, + }); + } + }); + + const batch = writeBatch(owner); + for (const uid of voters) { + batch.delete(doc(owner, "ratings", `${uid}_${NOTE}`)); + } + batch.delete(doc(owner, "notes", NOTE, "content", "file")); + batch.delete(doc(owner, "notes", NOTE)); + + await assertSucceeds(batch.commit()); + }); + test("a stranger cannot delete votes by deleting a note they do not own", async () => { await seedVote(); diff --git a/firestore.rules b/firestore.rules index 570656d..1a226fd 100644 --- a/firestore.rules +++ b/firestore.rules @@ -50,9 +50,55 @@ service cloud.firestore { */ allow read: if isOwner(userId); - // Kayıt sırasında kullanıcı yalnızca kendi dokümanını oluşturabilir. + /** + * Kayıt. + * + * Buradaki tek kontrol "kendi belgeni yazıyorsun ve `id` alanı belge kimliğiyle + * aynı" idi. Geri kalan her şey serbestti, ve bunun en ağır sonucu şuydu: + * `@ogr.akdeniz.edu.tr` kısıtı yalnızca `RegisterActivity`'de, yani istemcide + * yaşıyordu. Firebase Auth'ta e-posta/parola açık olduğu için herhangi biri + * gmail adresiyle hesap açıp Firestore'a doğrudan bağlanarak kendine profil + * yazabiliyordu — üstelik profildeki `email` alanına oturumdakinden başka bir + * adres, `department` alanına da listede olmayan bir bölüm koyarak. + * + * Okuma kapsamı `department`'a bağlandığından beri bu bir erişim sorunu: + * `ownDepartment()` tam da bu belgeden okuyor, yani doğrulanmamış bir profil + * istediği bölümün bütün notlarını açıyordu. + * + * Adres oturumun kendisinden alınıyor — istemcinin yazdığı değerden değil — + * ve alan adı burada, sunucuda kontrol ediliyor. + */ allow create: if isOwner(userId) - && request.resource.data.id == userId; + && hasExactProfileFields() + && request.resource.data.id == userId + && request.resource.data.email == request.auth.token.email + && isUniversityAddress(request.auth.token.email) + // Bölüm listesinin 144 girdisini kurala gömmek onu her müfredat + // değişikliğinde bozardı; buradaki iş boş ya da devasa bir değerin + // `ownDepartment()` üzerinden erişim kapsamı olmasını engellemek. + && request.resource.data.department is string + && request.resource.data.department.size() > 0 + && request.resource.data.department.size() <= 120 + // Sunucu saati; kayıt tarihi istemciden gelmiyor. + && request.resource.data.createdAt == request.time; + + function profileFields() { + return ['id', 'email', 'department', 'createdAt']; + } + + function hasExactProfileFields() { + return request.resource.data.keys().hasOnly(profileFields()) + && request.resource.data.keys().hasAll(profileFields()); + } + + /** + * `matches` bütün dizeyi eşleştiriyor. Nokta karakter sınıfıyla yazıldı + * (`[.]`), kaçış yerine: kural dilinde ters bölü iki kez kaçmak zorunda ve + * sessizce yanlış yazılması kolay. + */ + function isUniversityAddress(email) { + return email is string && email.matches('.*@ogr[.]akdeniz[.]edu[.]tr'); + } // Uygulama bu dokümanı yalnızca oluşturuyor, hiç güncellemiyor // (`saveUser` tek çağrısı kayıt akışında). Açık bırakmanın bedeli vardı: @@ -198,16 +244,36 @@ service cloud.firestore { || isRatingWrite(noteId) ); - // Sahibi metadata'yı düzenleyebilir ama puan alanlarına, sahiplik bilgisine - // ve bölüme dokunamaz — kendi notunu öne çıkarmanın en kısa yolu buydu. - // `avgRating` listede: alan artık yazılmıyor, düzenlemeyle geri gelmesin. + /** + * Sahibi yalnızca metadata'yı düzenleyebilir. + * + * Bu kontrol eskiden kara listeydi: dokunulmaması gereken alanlar sayılıyor, + * geri kalan her şey serbest bırakılıyordu. Sayılmayan bir alan — bugün var + * olmayan ama yarın eklenecek olan da dahil — sessizce yazılabilir demekti; + * üstelik `title`'a sayı, `description`'a megabaytlık metin koymak da mümkündü. + * + * Artık beyaz liste: değişebilecek alan kümesi `hasOnly` ile sabit ve her biri + * oluşturmadakiyle aynı tip/boyut kontrolünden geçiyor. Yeni bir alan eklemek + * kuralı da güncellemeyi gerektiriyor, ki doğrusu bu. + */ function isMetadataEditByOwner() { return resource.data.uploaderUid == request.auth.uid - && !changedFields().hasAny([ - 'ratingSum', 'ratingCount', 'avgRating', - 'uploaderUid', 'uploaderName', 'uploaderEmail', - 'department', 'createdAt' - ]); + && changedFields().hasOnly(editableNoteFields()) + && hasValidEditedMetadata(); + } + + /** `updatedAt` yalnızca düzenlemede yazılıyor, oluşturmada yok. */ + function editableNoteFields() { + return ['course', 'title', 'description', 'tag', 'updatedAt']; + } + + function hasValidEditedMetadata() { + let data = request.resource.data; + return data.course is string && data.course.size() > 0 && data.course.size() <= 100 + && data.title is string && data.title.size() > 0 && data.title.size() <= 200 + && data.description is string && data.description.size() <= 2000 + && data.tag is string && data.tag.size() <= 50 + && data.updatedAt == request.time; } function ratingPath(noteId) { @@ -280,14 +346,41 @@ service cloud.firestore { && request.resource.data.keys().hasOnly(['fileData']) && request.resource.data.fileData is string; - allow update, delete: if isSignedIn() + // Güncelleme oluşturmayla aynı şemayı geçmek zorunda: create'te alan kümesi + // sabitlenip update'te serbest bırakılırsa, kontrol yalnızca ilk yazmayı + // bağlar ve ikincisinde atlanır. + allow update: if isSignedIn() + && get(/databases/$(database)/documents/notes/$(noteId)) + .data.uploaderUid == request.auth.uid + && request.resource.data.keys().hasOnly(['fileData']) + && request.resource.data.fileData is string; + + allow delete: if isSignedIn() && get(/databases/$(database)/documents/notes/$(noteId)) .data.uploaderUid == request.auth.uid; } } match /ratings/{ratingId} { - allow read: if isSignedIn(); + /** + * Oylar herkese açık değil. + * + * Kural `isSignedIn()` iken oturum açmış herkes bütün koleksiyonu okuyabiliyor + * ve her satır bir kullanıcının hangi nota kaç verdiğini söylüyordu — uid, not + * kimliği ve puan. Uygulamanın buna ihtiyacı yoktu; yalnızca iki okuma var: + * oy verenin transaction içinde kendi oyunu okuması, ve not sahibinin notu + * silerken o notun oylarını `where noteId == ...` ile bulması. + * + * İki dal tam olarak o iki okumaya karşılık geliyor. Firestore liste + * sorgusunu kurala göre değerlendirdiği için `where uid == ben` ya da + * `where noteId == kendi notum` taşımayan bir sorgu geçmiyor; koleksiyonun + * tamamını çekmek artık mümkün değil. + */ + allow read: if isSignedIn() && ( + resource.data.uid == request.auth.uid + || get(/databases/$(database)/documents/notes/$(resource.data.noteId)) + .data.uploaderUid == request.auth.uid + ); // Doküman kimliği "_" biçiminde; bu kalıbı zorunlu // kılmak bir kullanıcının başkasının oyunu yazmasını engelliyor. @@ -301,7 +394,48 @@ service cloud.firestore { // notun toplamını değiştirmese de liderlik tablosunun // oy sayısını şişirirdi. && get(/databases/$(database)/documents/notes/$(request.resource.data.noteId)) - .data.uploaderUid != request.auth.uid; + .data.uploaderUid != request.auth.uid + && totalsFollowTheVote(request.resource.data.noteId); + + function notePath(noteId) { + return /databases/$(database)/documents/notes/$(noteId); + } + + /** + * Oyun karşılığı toplam aynı commit'te yazılmış olmalı. + * + * Bağ eskiden tek yönlüydü: notun kuralı "toplam değişiyorsa yanında oy + * bulunsun" diyordu, ama oyun kuralı "oy yazılıyorsa toplam da doğru + * değişsin" demiyordu. Tek başına oy belgesi yazmak serbestti ve bu, + * sınırsız puan şişirmenin yoluydu: + * + * 1. oy belgesini tek başına 1 yap (toplam değişmez) + * 2. oyu 5'e çıkar + toplamı güncelle (fark 4 eklenir, sayı artmaz) + * 3. oy belgesini tek başına 1 yap (toplam değişmez) + * 4. 2. adıma dön + * + * Her turda not 4 puan kazanıyor ve tek bir oy verenden geliyordu. + * + * `getAfter` notun commit sonrası hâlini okuyor; beklenen değerler notun + * kendi kuralındaki aritmetiğin aynısı — iki taraf da RatingCalculator ile + * birebir aynı olmak zorunda, tabanları dahil. + */ + function totalsFollowTheVote(noteId) { + let before = get(notePath(noteId)).data; + let after = getAfter(notePath(noteId)).data; + let votedBefore = exists(/databases/$(database)/documents/ratings/$(ratingId)); + let previous = votedBefore + ? get(/databases/$(database)/documents/ratings/$(ratingId)).data.rating + : 0; + let rawSum = before.ratingSum + request.resource.data.rating - previous; + + return after.ratingSum == (rawSum < 0 ? 0 : rawSum) + && after.ratingCount == ( + votedBefore + ? (before.ratingCount < 1 ? 1 : before.ratingCount) + : before.ratingCount + 1 + ); + } /** * Bir oy yalnızca ait olduğu notla birlikte silinebilir.