From 63c77f16b776bafdcad1239fa706329d6bb94941 Mon Sep 17 00:00:00 2001 From: Duygu <84078855+duuyguyiilmaz@users.noreply.github.com> Date: Tue, 1 Sep 2026 18:30:42 +0300 Subject: [PATCH 1/6] fix: stop storing an average the rules cannot verify, and pin note creation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings, and one of them is answered by deletion rather than by another check. **The average could be faked during a legitimate vote.** The rules recompute `ratingSum` and `ratingCount` from the rating document written in the same commit and reject anything that disagrees. `avgRating` got no such treatment — only "is a number, is between 0 and 5" — because rules do integer division, so the exact comparison was not available to write. That gap let a modified client cast a real 1-star vote and set the displayed average to 5.0 in the same batch. The fix is to stop storing it. An average is derived from two numbers the rules already pin exactly, so keeping a third, unverifiable copy of that information bought nothing and cost a forgery. Screens now show `ratingSum` — the running total, which is what the leaderboard already ranked on. `RatingCalculator` no longer computes an average, `RatingResult` no longer carries one, and the note detail view's `tvAvgRating` is now `tvRatingSum`, because a name that says "average" over a total is the next bug. Old documents keep the field; nothing reads it, and the create rule refuses to write it again. **Note creation accepted almost anything.** The rule checked `uploaderUid` and the two score fields and stopped. A client talking to Firestore directly — which is the only client that matters here, since nobody is obliged to use the Android app — could file a note into another department's feed, put someone else's address in `uploaderEmail`, backdate `createdAt` to sit at the top of a feed that orders on it, or add fields nobody expected. The form validation in the app was never a security boundary; it only ever described what the app itself sends. The shape is now fixed with `hasOnly` + `hasAll`, so a missing field is refused as firmly as an invented one, and each field is checked for type and size. Identity comes from the session instead of the request: `uploaderUid` is the caller's uid, `uploaderEmail` is the token's email, `createdAt` is `request.time`, and `department` must equal the department on the caller's own profile. Editing a note can no longer move any of those either. One behaviour change worth stating: a profile whose department is blank can no longer upload at all, where it used to write into a "Bilinmiyor" bucket. Registration cannot complete without choosing a department, so this only reaches corrupted data — and no feed ever showed those notes. Tests: rules 38 -> 47, one refusal test per check, so relaxing any single check turns exactly one test red. 55 green in the rules suite including the backfill, 128 green on the JVM side, lint clean. Verified against the live project with the new rules deployed: a note uploads, appears in the feed, and the logcat carries no PERMISSION_DENIED. Co-Authored-By: Claude Opus 5 --- README.md | 36 +++- .../yilmaz/campusnote/data/model/Post.kt | 1 - .../data/model/RatingCalculation.kt | 3 - .../campusnote/data/model/RatingResult.kt | 1 - .../data/repository/FirebaseNoteRepository.kt | 6 +- .../repository/FirebaseRatingRepository.kt | 5 +- .../campusnote/ui/common/PostAdapter.kt | 4 +- .../ui/notedetail/NoteDetailFragment.kt | 2 +- .../ui/notedetail/NoteDetailViewModel.kt | 1 - .../main/res/layout/fragment_note_detail.xml | 4 +- app/src/main/res/layout/item_post.xml | 2 +- .../data/model/RatingCalculatorTest.kt | 24 --- .../campusnote/testing/FakeRepositories.kt | 2 +- .../ui/notedetail/NoteDetailViewModelTest.kt | 4 +- firestore-tests/rules.test.js | 190 +++++++++++++----- firestore.rules | 98 +++++++-- 16 files changed, 267 insertions(+), 116 deletions(-) diff --git a/README.md b/README.md index e852692..44c4006 100644 --- a/README.md +++ b/README.md @@ -46,7 +46,9 @@ full email — see - **Note upload** as PDF or image, with automatic image downscaling and compression - **Note rating** on a 1–5 scale; you cannot rate your own notes, and re-rating replaces your previous vote instead of adding a second one — both enforced by the security - rules, not just the app + rules, not just the app. A note's score is the running total of its votes, not an + average: the total is the one figure the rules can verify exactly (see + [Security rules](#security-rules)) - **Leaderboard** ranking your department's top 50 notes by total score, with gold/silver/bronze placings - **Points and rewards** — your total is the score your own notes have earned; discounts unlock at 100 points - **Note management** — edit or delete your own notes from your profile @@ -183,7 +185,7 @@ point total. Both were stored once and both drifted from the truth — see Key d | `department` | string | the feed and the leaderboard both filter on this | | `uploaderUid`, `uploaderEmail` | string | | | `fileName`, `fileType`, `fileSize` | string / string / number | `fileType` is `pdf`, `image` or empty | -| `ratingSum`, `ratingCount`, `avgRating` | number | denormalised so the feed needs no aggregation | +| `ratingSum`, `ratingCount` | number | denormalised so the feed needs no aggregation; `ratingSum` is the score shown on every screen | | `createdAt`, `updatedAt` | timestamp | | **`notes/{noteId}/content/file`** — one field, `fileData`, holding the base64 file content. @@ -206,6 +208,16 @@ The rules require authentication everywhere, restrict note edits and deletes to 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. +Creating a note is pinned to an exact shape. `hasOnly` plus `hasAll` fix the field set, +so a missing field is refused as firmly as an invented one, and each field is then +checked for type and size. Identity is taken from the session rather than the request: +`uploaderUid` must be the caller's own uid, `uploaderEmail` must match the token's email, +`createdAt` must be `request.time`, and `department` must equal the department on the +caller's own profile — a note cannot be filed into someone else's feed. Editing a note +cannot move any of those either. Until this landed the rule checked only `uploaderUid` +and the two score fields; everything else was open to a client that skipped the Android +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 @@ -216,9 +228,16 @@ own note is refused by the server rather than only by the client. The expected totals are the same arithmetic as [`RatingCalculator`](app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingCalculation.kt), floors included — the client and the rule have to agree exactly, so the rule is written -as that function's twin. `avgRating` is checked only for range: rules do integer -division, so an exact comparison is not available, and it is a derived display field -whose inputs are already pinned. +as that function's twin. + +**There is no average any more.** A stored `avgRating` was the one score field the rules +could not actually verify: rules do integer division, so it could only be range-checked +as "a number between 0 and 5", which left a legitimate vote free to carry a fabricated +average onto the screen — a note could show 5.0 on a single 1-star rating. Rather than +guard a field that could not be pinned, the field is gone. Screens show `ratingSum`, the +running total, which the rule already recomputes from the vote and matches exactly. Old +documents still carry `avgRating`; nothing reads it, and the create rule refuses to write +it again. They are covered by their own test suite; see [Rules tests](#rules-tests). @@ -322,7 +341,7 @@ emulator on API 24+. ./gradlew testDebugUnitTest ``` -129 JVM tests, no emulator and no network. They cover the layers where a bug would be +128 JVM tests, no emulator and no network. They cover the layers where a bug would be invisible rather than loud: the scoring arithmetic, the decision logic in every ViewModel, the list diffing that decides which rows get redrawn, and — for the feature the app is built around — what the screen actually shows. @@ -394,7 +413,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. -38 tests in [`firestore-tests/rules.test.js`](firestore-tests/rules.test.js) run +47 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. @@ -403,7 +422,8 @@ it down again; nothing touches the real project, and no billing account is invol |---|---| | `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, and that nobody updates a profile afterwards — including the two shapes of the old hole: awarding yourself points, and writing points onto someone else. | -| `notes` | `uploaderUid` cannot be forged and a note cannot be born with a score; the uploader may edit metadata but not the totals; 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; 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. | +| `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 `uploaderEmail`, 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 replace and delete, and a test that deliberately asserts the *open* create rule, so the gap documented in `firestore.rules` cannot be closed by accident and go unnoticed. | | `ratings` | The `_` document id, which is what stops one user voting as another, plus the 1–5 integer range and the ban on deleting votes. | | `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. | diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/model/Post.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/model/Post.kt index 49af88a..c8ebe7a 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/model/Post.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/model/Post.kt @@ -13,7 +13,6 @@ data class Post( val department: String, val timeMills: Long, val uploaderUid: String = "", - val avgRating: Double = 0.0, val ratingCount: Long = 0L, val ratingSum: Long = 0L, val course: String = "", diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingCalculation.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingCalculation.kt index cf6b8b6..31cd6b6 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingCalculation.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingCalculation.kt @@ -4,7 +4,6 @@ package duygu.yilmaz.campusnote.data.model data class RatingTotals( val sum: Long, val count: Long, - val average: Double, /** Kullanıcı ilk kez mi oy verdi, yoksa eski oyunu mu değiştirdi. */ val updatedExistingRating: Boolean ) @@ -53,12 +52,10 @@ object RatingCalculator { // Toplam hiçbir koşulda negatife düşmemeli; eski veriler bozuksa taban 0. val sum = (currentSum + delta).coerceAtLeast(0L) - val average = if (count == 0L) 0.0 else sum.toDouble() / count return RatingTotals( sum = sum, count = count, - average = average, updatedExistingRating = updatedExistingRating ) } diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingResult.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingResult.kt index d2c6688..3d953ee 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingResult.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingResult.kt @@ -1,7 +1,6 @@ package duygu.yilmaz.campusnote.data.model data class RatingResult( - val average: Double, val count: Long, val sum: Long, val updatedExistingRating: Boolean diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseNoteRepository.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseNoteRepository.kt index 54d8c58..80ae58d 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseNoteRepository.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseNoteRepository.kt @@ -41,7 +41,6 @@ class FirebaseNoteRepository( "createdAt" to FieldValue.serverTimestamp(), "ratingSum" to 0L, "ratingCount" to 0L, - "avgRating" to 0.0, "fileName" to draft.fileName, "fileType" to draft.fileType, "fileSize" to draft.fileSize @@ -233,9 +232,8 @@ class FirebaseNoteRepository( department = document.getString("department") ?: "", timeMills = createdAt, uploaderUid = document.getString("uploaderUid") ?: "", - avgRating = document.getDouble("avgRating") - ?: document.getLong("avgRating")?.toDouble() - ?: 0.0, + // Eski notlar hâlâ bir `avgRating` alanı taşıyor; okunmuyor. Ortalama + // artık ne saklanıyor ne de gösteriliyor — bkz. firestore.rules. ratingCount = document.getLong("ratingCount") ?: document.getDouble("ratingCount")?.toLong() ?: 0L, diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseRatingRepository.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseRatingRepository.kt index 9282ce0..d9878b7 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseRatingRepository.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseRatingRepository.kt @@ -57,8 +57,7 @@ class FirebaseRatingRepository( noteReference, mapOf( RATING_SUM_FIELD to totals.sum, - RATING_COUNT_FIELD to totals.count, - AVERAGE_RATING_FIELD to totals.average + RATING_COUNT_FIELD to totals.count ) ) transaction.set( @@ -71,7 +70,6 @@ class FirebaseRatingRepository( ) RatingResult( - average = totals.average, count = totals.count, sum = totals.sum, updatedExistingRating = totals.updatedExistingRating @@ -85,7 +83,6 @@ class FirebaseRatingRepository( const val UPLOADER_UID_FIELD = "uploaderUid" const val RATING_SUM_FIELD = "ratingSum" const val RATING_COUNT_FIELD = "ratingCount" - const val AVERAGE_RATING_FIELD = "avgRating" const val RATING_FIELD = "rating" const val USER_ID_FIELD = "uid" const val NOTE_ID_FIELD = "noteId" diff --git a/app/src/main/java/duygu/yilmaz/campusnote/ui/common/PostAdapter.kt b/app/src/main/java/duygu/yilmaz/campusnote/ui/common/PostAdapter.kt index 39fa619..a3e0b5f 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/ui/common/PostAdapter.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/ui/common/PostAdapter.kt @@ -46,7 +46,7 @@ class PostAdapter( tvDesc.text = post.desc tvUploader.text = post.uploaderName tvDept.text = post.department.take(3).uppercase() - tvRating.text = String.format("%.1f", post.avgRating) + tvRating.text = post.ratingSum.toString() tvRatingCount.text = root.context.getString(R.string.vote_count, post.ratingCount) root.setOnClickListener { @@ -88,7 +88,7 @@ class PostAdapter( oldItem.id == newItem.id // Post bir data class; `==` bütün alanları karşılaştırıyor, dolayısıyla - // nota yeni bir oy geldiğinde avgRating farkı buradan yakalanıyor. + // nota yeni bir oy geldiğinde ratingSum farkı buradan yakalanıyor. override fun areContentsTheSame(oldItem: Post, newItem: Post) = oldItem == newItem } diff --git a/app/src/main/java/duygu/yilmaz/campusnote/ui/notedetail/NoteDetailFragment.kt b/app/src/main/java/duygu/yilmaz/campusnote/ui/notedetail/NoteDetailFragment.kt index 6fb7884..ff1806a 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/ui/notedetail/NoteDetailFragment.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/ui/notedetail/NoteDetailFragment.kt @@ -120,7 +120,7 @@ class NoteDetailFragment : Fragment() { tvTag.text = post.tag.uppercase() } - tvAvgRating.text = String.format("%.1f", post.avgRating) + tvRatingSum.text = post.ratingSum.toString() tvRatingCount.text = getString(R.string.rating_count, post.ratingCount) } diff --git a/app/src/main/java/duygu/yilmaz/campusnote/ui/notedetail/NoteDetailViewModel.kt b/app/src/main/java/duygu/yilmaz/campusnote/ui/notedetail/NoteDetailViewModel.kt index 8e520d9..2e547b3 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/ui/notedetail/NoteDetailViewModel.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/ui/notedetail/NoteDetailViewModel.kt @@ -137,7 +137,6 @@ class NoteDetailViewModel( _noteState.value = NoteDetailUiState.Content( current.copy( - avgRating = result.average, ratingCount = result.count, ratingSum = result.sum ) diff --git a/app/src/main/res/layout/fragment_note_detail.xml b/app/src/main/res/layout/fragment_note_detail.xml index 88b8d62..755ca2c 100644 --- a/app/src/main/res/layout/fragment_note_detail.xml +++ b/app/src/main/res/layout/fragment_note_detail.xml @@ -285,14 +285,14 @@ app:tint="@color/soft_orange_light"/> + tools:text="12"/> diff --git a/app/src/test/java/duygu/yilmaz/campusnote/data/model/RatingCalculatorTest.kt b/app/src/test/java/duygu/yilmaz/campusnote/data/model/RatingCalculatorTest.kt index e114acb..19178fe 100644 --- a/app/src/test/java/duygu/yilmaz/campusnote/data/model/RatingCalculatorTest.kt +++ b/app/src/test/java/duygu/yilmaz/campusnote/data/model/RatingCalculatorTest.kt @@ -27,7 +27,6 @@ class RatingCalculatorTest { assertEquals(4L, totals.sum) assertEquals(1L, totals.count) - assertEquals(4.0, totals.average, DELTA) assertFalse(totals.updatedExistingRating) } @@ -42,7 +41,6 @@ class RatingCalculatorTest { assertEquals(12L, totals.sum) assertEquals(3L, totals.count) - assertEquals(4.0, totals.average, DELTA) } @Test @@ -56,7 +54,6 @@ class RatingCalculatorTest { assertEquals(10L, totals.sum) assertEquals(2L, totals.count) - assertEquals(5.0, totals.average, DELTA) assertTrue(totals.updatedExistingRating) } @@ -98,7 +95,6 @@ class RatingCalculatorTest { ) assertEquals(0L, totals.sum) - assertEquals(0.0, totals.average, DELTA) } @Test @@ -112,21 +108,6 @@ class RatingCalculatorTest { assertEquals(1L, totals.count) assertEquals(8L, totals.sum) - assertEquals(8.0, totals.average, DELTA) - } - - @Test - fun `ortalama tam bolunmeyen degerlerde de dogru hesaplanir`() { - val totals = RatingCalculator.recalculate( - currentSum = 9L, - currentCount = 2L, - previousRating = null, - newRating = 4 - ) - - assertEquals(13L, totals.sum) - assertEquals(3L, totals.count) - assertEquals(13.0 / 3.0, totals.average, DELTA) } @Test @@ -150,9 +131,4 @@ class RatingCalculatorTest { RatingCalculator.recalculate(0L, 0L, null, newRating = -3) } } - - private companion object { - /** Kayan nokta karşılaştırması için tolerans. */ - const val DELTA = 1e-9 - } } diff --git a/app/src/test/java/duygu/yilmaz/campusnote/testing/FakeRepositories.kt b/app/src/test/java/duygu/yilmaz/campusnote/testing/FakeRepositories.kt index 9d26262..706f97f 100644 --- a/app/src/test/java/duygu/yilmaz/campusnote/testing/FakeRepositories.kt +++ b/app/src/test/java/duygu/yilmaz/campusnote/testing/FakeRepositories.kt @@ -195,7 +195,7 @@ class FakeNoteRepository : NoteRepository { class FakeRatingRepository : RatingRepository { - var result = RatingResult(average = 4.0, count = 2L, sum = 8L, updatedExistingRating = false) + var result = RatingResult(count = 2L, sum = 8L, updatedExistingRating = false) var error: Exception? = null val submissions = mutableListOf() diff --git a/app/src/test/java/duygu/yilmaz/campusnote/ui/notedetail/NoteDetailViewModelTest.kt b/app/src/test/java/duygu/yilmaz/campusnote/ui/notedetail/NoteDetailViewModelTest.kt index f00fd1c..ec78cad 100644 --- a/app/src/test/java/duygu/yilmaz/campusnote/ui/notedetail/NoteDetailViewModelTest.kt +++ b/app/src/test/java/duygu/yilmaz/campusnote/ui/notedetail/NoteDetailViewModelTest.kt @@ -187,11 +187,10 @@ class NoteDetailViewModelTest { @Test fun `basarili oy ekrandaki ortalamayi da gunceller`() = runTest(mainDispatcherRule.testDispatcher) { - // Ekran, yeniden yüklemeye gerek kalmadan yeni ortalamayı göstermeli. + // Ekran, yeniden yüklemeye gerek kalmadan yeni toplamı göstermeli. authRepository.setUser(authenticatedUser(uid = "uid-1")) noteRepository.note = post(id = "note-1", uploaderUid = "uid-2") ratingRepository.result = RatingResult( - average = 4.5, count = 4L, sum = 18L, updatedExistingRating = false @@ -210,7 +209,6 @@ class NoteDetailViewModelTest { ) val post = (viewModel.noteState.value as NoteDetailUiState.Content).post - assertEquals(4.5, post.avgRating, 1e-9) assertEquals(4L, post.ratingCount) assertEquals(18L, post.ratingSum) } diff --git a/firestore-tests/rules.test.js b/firestore-tests/rules.test.js index 659636a..437a9e6 100644 --- a/firestore-tests/rules.test.js +++ b/firestore-tests/rules.test.js @@ -8,6 +8,7 @@ import { deleteDoc, doc, getDoc, + serverTimestamp, setDoc, updateDoc, writeBatch, @@ -19,6 +20,9 @@ import { afterAll, beforeAll, beforeEach, describe, test } from "vitest"; const OWNER = "owner-uid"; const RATER = "rater-uid"; const NOTE = "note-1"; +const OWNER_EMAIL = "owner@ogr.akdeniz.edu.tr"; +const RATER_EMAIL = "rater@ogr.akdeniz.edu.tr"; +const DEPARTMENT = "Bilgisayar Mühendisliği"; let testEnv; let owner; @@ -35,8 +39,10 @@ beforeAll(async () => { }, }); - owner = testEnv.authenticatedContext(OWNER).firestore(); - rater = testEnv.authenticatedContext(RATER).firestore(); + // The note-create rule compares uploaderEmail against the session's token, so + // the contexts have to carry an email claim the way a real sign-in does. + owner = testEnv.authenticatedContext(OWNER, { email: OWNER_EMAIL }).firestore(); + rater = testEnv.authenticatedContext(RATER, { email: RATER_EMAIL }).firestore(); anonymous = testEnv.unauthenticatedContext().firestore(); }); @@ -53,28 +59,19 @@ beforeEach(async () => { const db = context.firestore(); await setDoc(doc(db, "users", OWNER), { id: OWNER, - email: "owner@ogr.akdeniz.edu.tr", - department: "Bilgisayar Mühendisliği", + email: OWNER_EMAIL, + department: DEPARTMENT, createdAt: 1, }); await setDoc(doc(db, "users", RATER), { id: RATER, - email: "rater@ogr.akdeniz.edu.tr", - department: "Bilgisayar Mühendisliği", + email: RATER_EMAIL, + department: DEPARTMENT, createdAt: 1, }); await setDoc(doc(db, "notes", NOTE), { - title: "Ders notu", - desc: "", - authorEmail: "owner@ogr.akdeniz.edu.tr", - department: "Bilgisayar Mühendisliği", - timeMills: 1, - uploaderUid: OWNER, - avgRating: 0, - ratingCount: 0, - ratingSum: 0, - fileName: "notes.pdf", - fileType: "pdf", + ...noteFields({ uploaderUid: OWNER, uploaderEmail: OWNER_EMAIL }), + createdAt: 1, }); await setDoc(doc(db, "notes", NOTE, "content", "file"), { fileData: "ZmFrZQ==", @@ -82,6 +79,36 @@ beforeEach(async () => { }); }); +/** + * A note document in exactly the shape the create rule now demands. + * + * The rule pins the field set with hasOnly + hasAll, so tests can no longer write + * an approximation: a missing field is as fatal as a forged one. Keeping the shape + * in one place is what lets each test override the single field it is about and + * stay readable. + * + * `createdAt` defaults to a server timestamp because the rule requires + * request.time — a client-chosen date is one of the things being rejected. + */ +function noteFields(overrides = {}) { + return { + course: "Veri Yapıları", + title: "Ders notu", + description: "", + tag: "", + department: DEPARTMENT, + uploaderUid: RATER, + uploaderEmail: RATER_EMAIL, + createdAt: serverTimestamp(), + ratingSum: 0, + ratingCount: 0, + fileName: "notes.pdf", + fileType: "pdf", + fileSize: 1024, + ...overrides, + }; +} + /** * Bir oyu uygulamanın yazdığı gibi yazar: notun toplamları ve oy dokümanı tek * atomik commit'te. Kural, notun yeni toplamlarını aynı commit'teki oy @@ -96,7 +123,6 @@ function vote(db, uid, noteId, rating, { sum, count, extra = {} }) { batch.update(doc(db, "notes", noteId), { ratingSum: sum, ratingCount: count, - avgRating: count === 0 ? 0 : sum / count, ...extra, }); batch.set(doc(db, "ratings", `${uid}_${noteId}`), { @@ -194,34 +220,111 @@ describe("notes", () => { test("uploaderUid must be the caller's own uid", async () => { await assertSucceeds( - setDoc(doc(rater, "notes", "own-note"), { - title: "Note", - uploaderUid: RATER, - department: "Bilgisayar Mühendisliği", - ratingSum: 0, - ratingCount: 0, - }), + setDoc(doc(rater, "notes", "own-note"), noteFields()), ); await assertFails( - setDoc(doc(rater, "notes", "forged-note"), { - title: "Note", - uploaderUid: OWNER, - department: "Bilgisayar Mühendisliği", - ratingSum: 0, - ratingCount: 0, - }), + setDoc( + doc(rater, "notes", "forged-note"), + noteFields({ uploaderUid: OWNER, uploaderEmail: OWNER_EMAIL }), + ), + ); + }); + + /** + * The create rule used to check only uploaderUid and the two score fields, which + * left everything else — the department a note lands in, the email shown beside + * it, and any field at all that nobody expected — writable by a client that skips + * the Android UI. The form validation in the app is not a security boundary: an + * attacker was never obliged to use the app. + */ + describe("note creation is pinned to an exact shape", () => { + test("an unexpected field is refused", async () => { + await assertFails( + setDoc( + doc(rater, "notes", "extra-field"), + noteFields({ isFeatured: true }), + ), + ); + }); + + test("a missing field is refused", async () => { + const { fileSize, ...withoutFileSize } = noteFields(); + await assertFails(setDoc(doc(rater, "notes", "missing-field"), withoutFileSize)); + }); + + /** + * The average is the field this change removed. It can no longer be verified + * against anything — rules do integer division, so a stored avgRating could + * only ever be range-checked, which let a legitimate vote carry a fabricated + * average to the screen. The value shown is now ratingSum itself, and the old + * field is refused so it cannot creep back. + */ + test("avgRating cannot be written back", async () => { + await assertFails( + setDoc(doc(rater, "notes", "with-average"), noteFields({ avgRating: 5 })), + ); + }); + + test("a note cannot be filed under another department", async () => { + await assertFails( + setDoc( + doc(rater, "notes", "wrong-dept"), + noteFields({ department: "Makine Mühendisliği" }), + ), + ); + }); + + test("uploaderEmail cannot be impersonated", async () => { + await assertFails( + setDoc( + doc(rater, "notes", "forged-email"), + noteFields({ uploaderEmail: OWNER_EMAIL }), + ), + ); + }); + + test("createdAt must be the server's clock, not the client's", async () => { + // A client-chosen date would let a note pin itself to the top of a feed that + // orders on createdAt, or hide itself at the bottom. + await assertFails( + setDoc(doc(rater, "notes", "backdated"), noteFields({ createdAt: 1 })), + ); + }); + + test("a field of the wrong type is refused", async () => { + await assertFails( + setDoc(doc(rater, "notes", "bad-sum"), noteFields({ ratingSum: "0" })), + ); + await assertFails( + setDoc(doc(rater, "notes", "bad-type"), noteFields({ fileType: "exe" })), + ); + }); + + test("an empty title or an oversized one is refused", async () => { + await assertFails( + setDoc(doc(rater, "notes", "no-title"), noteFields({ title: "" })), + ); + await assertFails( + setDoc( + doc(rater, "notes", "huge-title"), + noteFields({ title: "a".repeat(201) }), + ), + ); + }); + }); + + test("the uploader cannot move their note to another department while editing", async () => { + await assertFails( + updateDoc(doc(owner, "notes", NOTE), { department: "Makine Mühendisliği" }), ); }); test("a note cannot be born with a score", async () => { await assertFails( - setDoc(doc(rater, "notes", "head-start"), { - title: "Note", - uploaderUid: RATER, - department: "Bilgisayar Mühendisliği", - ratingSum: 500, - ratingCount: 100, - }), + setDoc( + doc(rater, "notes", "head-start"), + noteFields({ ratingSum: 500, ratingCount: 100 }), + ), ); }); @@ -259,7 +362,6 @@ describe("notes", () => { updateDoc(doc(rater, "notes", NOTE), { ratingSum: 500, ratingCount: 100, - avgRating: 5, }), ); }); @@ -314,13 +416,7 @@ describe("note content", () => { test("upload writes the note and its content in one batch", async () => { const batch = writeBatch(rater); - batch.set(doc(rater, "notes", "batched"), { - title: "Note", - uploaderUid: RATER, - department: "Bilgisayar Mühendisliği", - ratingSum: 0, - ratingCount: 0, - }); + batch.set(doc(rater, "notes", "batched"), noteFields()); batch.set(doc(rater, "notes", "batched", "content", "file"), { fileData: "ZmFrZQ==", }); diff --git a/firestore.rules b/firestore.rules index 4266594..415bb6a 100644 --- a/firestore.rules +++ b/firestore.rules @@ -12,6 +12,13 @@ rules_version = '2'; // güveniliyor" aynı şey değil: notun yeni `ratingSum`/`ratingCount` değerleri // aşağıda, aynı commit'te yazılan oy dokümanından yeniden hesaplanıp // karşılaştırılıyor. İstemci sonucu üretiyor, sunucu doğruluyor. +// +// Ortalama puan artık hiç saklanmıyor. Saklandığı sürece kurallar onu tam olarak +// doğrulayamıyordu — kural dili tam sayı bölmesi yaptığı için `avgRating` yalnızca +// 0–5 aralığında bir sayı olmakla sınırlanabiliyordu, yani geçerli bir oyla +// birlikte uydurma bir ortalama yazılabiliyordu. Alan kaldırıldı: gösterilen değer +// artık `ratingSum`'ın kendisi, o da oy dokümanından birebir doğrulanıyor. +// Doğrulanamayan bir alanı tutmaktansa tutmamak. service cloud.firestore { match /databases/{database}/documents { @@ -53,24 +60,95 @@ service cloud.firestore { match /notes/{noteId} { allow read: if isSignedIn(); - // Not oluşturan kişi kendini başkası olarak gösteremez ve puanı sıfırdan - // başlatmak zorunda: aksi halde nota doğrudan yüksek puanla doğulurdu. + /** + * Not oluşturma. + * + * Buradaki kontroller uygulamadaki form doğrulamasının kopyası değil, onun + * yerine geçeni: Android arayüzü saldırganın kullanmak zorunda olduğu tek yol + * değil, Firestore'a doğrudan bağlanan bir istemci hangi alanı isterse yazar. + * Eskiden yalnızca `uploaderUid` ile puan alanlarına bakılıyordu; geri kalan + * her şey — bölüm adı, yükleyenin e-postası, hiç beklenmeyen alanlar — + * serbestti. + * + * Alan kümesi `hasOnly` + `hasAll` ile birebir sabitleniyor: eksik alan da + * fazlası da reddediliyor. Bu, artık yazılmayan `avgRating`'i de kapatıyor — + * ortalama saklanmıyor, dolayısıyla uydurulamıyor da. + */ allow create: if isSignedIn() + && hasExactNoteFields() + && hasValidNoteTypes() + // Kimlik istemciden değil, oturumdan geliyor. && request.resource.data.uploaderUid == request.auth.uid + && request.resource.data.uploaderEmail == request.auth.token.email + // Not, yazarın kendi bölümüne düşmek zorunda: aksi halde biri + // başka bölümün feed'ine içerik sokabilirdi. Bölümü boş kalmış + // eski bir profil bu yüzden hiç yükleyemez — uygulamadaki + // `UNKNOWN_DEPARTMENT` yedeği artık yazma yolunda geçmiyor. + // Kayıt bölüm seçmeden tamamlanamadığı için bu yalnızca bozuk + // veriyi kapsıyor, ve o notu zaten hiçbir feed göstermiyordu. + && request.resource.data.department == ownDepartment() + // Sunucu saati; istemci notu geçmişe ya da geleceğe tarihleyemez. + && request.resource.data.createdAt == request.time + // Puanı sıfırdan başlatmak zorunda: aksi halde nota doğrudan + // yüksek puanla doğulurdu. && request.resource.data.ratingSum == 0 && request.resource.data.ratingCount == 0; + function noteFields() { + return [ + 'course', 'title', 'description', 'tag', 'department', + 'uploaderUid', 'uploaderEmail', 'createdAt', + 'ratingSum', 'ratingCount', + 'fileName', 'fileType', 'fileSize' + ]; + } + + function hasExactNoteFields() { + return request.resource.data.keys().hasOnly(noteFields()) + && request.resource.data.keys().hasAll(noteFields()); + } + + /** + * Tip ve boyut kontrolleri. Uzunluk sınırları arayüzdekilerle aynı olmak + * zorunda değil — buradaki iş bir dokümanın feed'i bozacak kadar büyümesini + * ya da sayı beklenen yere metin yazılmasını engellemek. + */ + function hasValidNoteTypes() { + 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.department is string + && data.uploaderUid is string + && data.uploaderEmail is string + && data.createdAt is timestamp + && data.ratingSum is int + && data.ratingCount is int + && data.fileName is string && data.fileName.size() <= 260 + && data.fileType in ['pdf', 'image', ''] + && data.fileSize is int && data.fileSize >= 0; + } + + /** Yazarın kendi profilindeki bölüm; istemcinin gönderdiği değil. */ + function ownDepartment() { + return get(/databases/$(database)/documents/users/$(request.auth.uid)) + .data.department; + } + allow update: if isSignedIn() && ( isMetadataEditByOwner() || isRatingWrite(noteId) ); - // Sahibi metadata'yı düzenleyebilir ama puan alanlarına ve sahiplik - // bilgisine dokunamaz — kendi notunu öne çıkarmanın en kısa yolu buydu. + // 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. function isMetadataEditByOwner() { return resource.data.uploaderUid == request.auth.uid && !changedFields().hasAny([ - 'ratingSum', 'ratingCount', 'avgRating', 'uploaderUid' + 'ratingSum', 'ratingCount', 'avgRating', + 'uploaderUid', 'uploaderEmail', 'department', 'createdAt' ]); } @@ -95,7 +173,7 @@ service cloud.firestore { let submitted = getAfter(path).data; let rawSum = resource.data.ratingSum + submitted.rating - previous; - return changedFields().hasOnly(['ratingSum', 'ratingCount', 'avgRating']) + return changedFields().hasOnly(['ratingSum', 'ratingCount']) // Kendi notuna oy vermek burada yasaklanıyor; eskiden yalnızca // istemci kodunda kontrol ediliyordu. && resource.data.uploaderUid != request.auth.uid @@ -106,13 +184,7 @@ service cloud.firestore { votedBefore ? (resource.data.ratingCount < 1 ? 1 : resource.data.ratingCount) : resource.data.ratingCount + 1 - ) - // Ortalama türetilmiş bir gösterim alanı; asıl doğrulama toplam ve - // sayıda. Kurallarda tam sayı bölmesi yapıldığı için burada - // birebir eşitlik yerine aralık kontrolü var. - && request.resource.data.avgRating is number - && request.resource.data.avgRating >= 0 - && request.resource.data.avgRating <= 5; + ); } // Uygulama kodu silme öncesi sahiplik kontrolü YAPMIYOR; tek gerçek From 9881d4c7497265e0ea513ce620875a45f4d07e21 Mon Sep 17 00:00:00 2001 From: Duygu <84078855+duuyguyiilmaz@users.noreply.github.com> Date: Tue, 1 Sep 2026 18:43:26 +0300 Subject: [PATCH 2/6] fix: scope reads to the caller and stop storing uploaders' addresses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every read rule was `if isSignedIn()`. That made three things true at once: any signed-in student could pull every profile in the university, every note in every department, and every attached file. The app only ever asked for its own department, but that is a property of the app, and the app is not the only client anyone can write. **Profiles are now readable only by their owner.** The old comment claimed the leaderboard and note detail needed to read other people's profiles. They do not — all four `getUser` calls in the app pass the caller's own uid, and those screens take the uploader's name from the note itself. The open rule was granting an access nobody had ever used, and what it granted was every student's address and department in a single query. **Notes and their file subcollections are scoped to the caller's department.** Rules are evaluated against each document a query returns and the whole query fails if any is denied, so the feed's `where department == …` filter stops being a courtesy and becomes the condition for the query to run at all. The file subcollection is checked against its parent note's department, or the boundary could be walked around through the file path while the note stayed unreadable. **The uploader's email is gone from the note.** Three screens rendered `email.substringBefore("@")`, which kept the address off the screen but not out of the document — masking at render time is a rendering choice, not a privacy boundary, and every student in the department could read the raw field. Notes now carry `uploaderName`, the rule requires it to equal the local part of the caller's own token email, and writing `uploaderEmail` is refused outright. `Post` and `LeaderboardEntry` no longer hold an address to leak, so the masking is structural rather than a getter someone can route around. `strip-uploader-email.js` converts existing notes, following the same contract as the createdAt backfill: dry run by default, `--apply` to write, safe to re-run. It sets the name and deletes the address in one update so no note is left carrying both, and `toPost()` still derives the name from a legacy `uploaderEmail` when it finds one, so the app works before and after the run. Tests: rules 47 -> 55, adding the reads that must fail — another department's note, an unfiltered collection query, a query filtered to someone else's department, another department's file, someone else's profile — plus the refusal to write an address onto a note. 70 green in the firestore-tests suite with the new migration's own 9, 126 green on the JVM side, lint clean. Co-Authored-By: Claude Opus 5 --- README.md | 62 ++++++-- .../campusnote/data/model/LeaderboardEntry.kt | 7 +- .../yilmaz/campusnote/data/model/Post.kt | 7 +- .../data/repository/FirebaseNoteRepository.kt | 29 ++-- .../data/repository/NoteRepository.kt | 4 +- .../campusnote/ui/profile/ProfileViewModel.kt | 3 +- .../campusnote/ui/upload/UploadViewModel.kt | 3 +- .../campusnote/data/model/UploaderNameTest.kt | 48 ++---- .../campusnote/testing/FakeRepositories.kt | 10 +- .../yilmaz/campusnote/testing/Fixtures.kt | 6 +- .../leaderboard/LeaderboardViewModelTest.kt | 2 +- .../ui/profile/ProfileViewModelTest.kt | 4 +- .../ui/upload/UploadViewModelTest.kt | 13 +- firestore-tests/package.json | 3 +- firestore-tests/rules.test.js | 105 ++++++++++-- firestore-tests/strip-uploader-email.js | 149 ++++++++++++++++++ firestore-tests/strip-uploader-email.test.js | 142 +++++++++++++++++ firestore.rules | 66 ++++++-- 18 files changed, 560 insertions(+), 103 deletions(-) create mode 100644 firestore-tests/strip-uploader-email.js create mode 100644 firestore-tests/strip-uploader-email.test.js diff --git a/README.md b/README.md index 44c4006..310ee15 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,9 @@ The leaderboard screenshot shows live Cloud Firestore data read through a real-t snapshot listener; the accounts in it are test data. It was captured before the ranking was scoped to the viewer's own department, so the list it shows is wider than what the app now returns. Uploaders appear as the local part of their address rather than the -full email — see +full email — and since the change described under [Security rules](#security-rules) that +is no longer a rendering choice: the address is never written to the note at all, only +the name, derived by [`UploaderName.kt`](app/src/main/java/duygu/yilmaz/campusnote/data/model/UploaderName.kt). --- @@ -183,7 +185,7 @@ point total. Both were stored once and both drifted from the truth — see Key d |---|---|---| | `title`, `description`, `course`, `tag` | string | | | `department` | string | the feed and the leaderboard both filter on this | -| `uploaderUid`, `uploaderEmail` | string | | +| `uploaderUid`, `uploaderName` | string | `uploaderName` is the part before the `@`, and the only identity a note carries; the address itself is never written (see [Security rules](#security-rules)) | | `fileName`, `fileType`, `fileSize` | string / string / number | `fileType` is `pdf`, `image` or empty | | `ratingSum`, `ratingCount` | number | denormalised so the feed needs no aggregation; `ratingSum` is the score shown on every screen | | `createdAt`, `updatedAt` | timestamp | | @@ -208,10 +210,30 @@ The rules require authentication everywhere, restrict note edits and deletes to 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, +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; because rules are evaluated against each document a query returns and the whole +query fails if one is denied, the feed's `where department == …` filter is now the +condition for the query to run rather than a courtesy. 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. + +**The uploader's address is no longer stored.** Three screens showed +`email.substringBefore("@")`, which kept the address off the screen but not out of the +document — masking at render time is not a privacy boundary. Notes now carry +`uploaderName` and nothing else identifying, the rule requires it to equal the local part +of the caller's own token email, and writing an `uploaderEmail` field is refused. Existing +notes are converted by a migration; see [Data migrations](#data-migrations). + Creating a note is pinned to an exact shape. `hasOnly` plus `hasAll` fix the field set, so a missing field is refused as firmly as an invented one, and each field is then checked for type and size. Identity is taken from the session rather than the request: -`uploaderUid` must be the caller's own uid, `uploaderEmail` must match the token's email, +`uploaderUid` must be the caller's own uid, `uploaderName` must be the token email's own +local part, `createdAt` must be `request.time`, and `department` must equal the department on the caller's own profile — a note cannot be filed into someone else's feed. Editing a note cannot move any of those either. Until this landed the rule checked only `uploaderUid` @@ -341,7 +363,7 @@ emulator on API 24+. ./gradlew testDebugUnitTest ``` -128 JVM tests, no emulator and no network. They cover the layers where a bug would be +126 JVM tests, no emulator and no network. They cover the layers where a bug would be invisible rather than loud: the scoring arithmetic, the decision logic in every ViewModel, the list diffing that decides which rows get redrawn, and — for the feature the app is built around — what the screen actually shows. @@ -413,7 +435,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. -47 tests in [`firestore-tests/rules.test.js`](firestore-tests/rules.test.js) run +55 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. @@ -421,10 +443,11 @@ 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, and that nobody updates a profile afterwards — including the two shapes of the old hole: awarding yourself points, and writing points onto someone else. | +| `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. | +| `notes` (reads) | A note in the reader's own department is readable and one in another department is not; an unfiltered query over the collection is refused, a query filtered to the reader's own department is allowed, and one filtered to another department is refused. The last three are what make the scoping real rather than advisory. | | `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. | -| `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 `uploaderEmail`, 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 replace and delete, and a test that deliberately asserts the *open* create rule, so the gap documented in `firestore.rules` cannot be closed by accident and go unnoticed. | +| `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 replace and delete, that a file on another department's note cannot be read — otherwise the boundary is walked around through the file path — and a test that deliberately asserts the *open* create rule, so the gap documented in `firestore.rules` cannot be closed by accident and go unnoticed. | | `ratings` | The `_` document id, which is what stops one user voting as another, plus the 1–5 integer range and the ban on deleting votes. | | `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. | @@ -468,6 +491,27 @@ against the emulator with legacy-shaped documents, including the assertion that most: before the migration an ordered query over two notes returns one of them, and after it returns both. +### Removing the uploader's email + +```bash +cd firestore-tests +npm run strip:uploader-email -- --project # reports only +npm run strip:uploader-email -- --project --apply # writes +``` + +Notes written before the privacy change carry a full `uploaderEmail`. The screens only +ever rendered the part before the `@`, so nothing on display changes — but the address was +in the document, readable by every student in the department. This replaces it with +`uploaderName` and deletes the address in the same update, so no note is ever left +carrying both. Same contract as the backfill above: application-default credentials, a dry +run unless `--apply` is passed, and safe to re-run after a partial failure. `toPost()` +still derives the name from a legacy `uploaderEmail` when it finds one, so the app works +before and after. + +[`strip-uploader-email.test.js`](firestore-tests/strip-uploader-email.test.js) runs it +against the emulator, including the assertion the change exists for: afterwards no +document in the collection has an `uploaderEmail` field at all. + ### Continuous integration [`.github/workflows/android.yml`](.github/workflows/android.yml) runs on every push to @@ -509,7 +553,7 @@ app/src/test/java/duygu/yilmaz/campusnote/ diff tests, and FeedScreenTest firestore-tests/ emulator-backed tests for firestore.rules, - plus the createdAt backfill and its own test + plus two data migrations and their own tests ``` --- diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/model/LeaderboardEntry.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/model/LeaderboardEntry.kt index 061f523..d41cbf2 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/model/LeaderboardEntry.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/model/LeaderboardEntry.kt @@ -3,11 +3,8 @@ package duygu.yilmaz.campusnote.data.model data class LeaderboardEntry( val docId: String, val title: String, - val uploaderEmail: String, + val uploaderName: String, val department: String, val ratingCount: Long, val ratingSum: Long = 0L -) { - /** Notu yükleyen kişinin gösterilecek adı; tam e-posta arayüze çıkmaz. */ - val uploaderName: String get() = uploaderEmail.uploaderName() -} +) diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/model/Post.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/model/Post.kt index c8ebe7a..2057733 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/model/Post.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/model/Post.kt @@ -9,7 +9,7 @@ data class Post( val id: String, val title: String, val desc: String, - val authorEmail: String, + val uploaderName: String, val department: String, val timeMills: Long, val uploaderUid: String = "", @@ -20,7 +20,4 @@ data class Post( val fileName: String = "", val fileType: String = "", val fileSize: Long = 0L -) { - /** Notu yükleyen kişinin gösterilecek adı; tam e-posta arayüze çıkmaz. */ - val uploaderName: String get() = authorEmail.uploaderName() -} +) diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseNoteRepository.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseNoteRepository.kt index 80ae58d..ac98e00 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseNoteRepository.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseNoteRepository.kt @@ -9,6 +9,7 @@ import duygu.yilmaz.campusnote.data.model.LeaderboardEntry import duygu.yilmaz.campusnote.data.model.NoteDraft import duygu.yilmaz.campusnote.data.model.NoteUpdate import duygu.yilmaz.campusnote.data.model.Post +import duygu.yilmaz.campusnote.data.model.uploaderName import kotlinx.coroutines.channels.awaitClose import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.callbackFlow @@ -26,7 +27,7 @@ class FirebaseNoteRepository( override suspend fun createNote( draft: NoteDraft, uploaderUid: String, - uploaderEmail: String, + uploaderName: String, department: String ) { val noteReference = firestore.collection(NOTES_COLLECTION).document() @@ -37,7 +38,7 @@ class FirebaseNoteRepository( "tag" to draft.tag, "department" to department, "uploaderUid" to uploaderUid, - "uploaderEmail" to uploaderEmail, + "uploaderName" to uploaderName, "createdAt" to FieldValue.serverTimestamp(), "ratingSum" to 0L, "ratingCount" to 0L, @@ -92,12 +93,12 @@ class FirebaseNoteRepository( override fun observeNotesByUploader( uploaderUid: String, - defaultUploaderEmail: String + defaultUploaderName: String ): Flow> = observePosts( query = firestore.collection(NOTES_COLLECTION) .whereEqualTo(UPLOADER_UID_FIELD, uploaderUid), - defaultUploaderEmail = defaultUploaderEmail + defaultUploaderName = defaultUploaderName ) /** @@ -192,7 +193,7 @@ class FirebaseNoteRepository( */ private fun observePosts( query: Query, - defaultUploaderEmail: String = "" + defaultUploaderName: String = "" ): Flow> = callbackFlow { val listener = query.addSnapshotListener { snapshot, exception -> if (exception != null) { @@ -201,7 +202,7 @@ class FirebaseNoteRepository( } val posts = snapshot?.documents - ?.mapNotNull { document -> toPost(document, defaultUploaderEmail) } + ?.mapNotNull { document -> toPost(document, defaultUploaderName) } ?.sortedByDescending { it.timeMills } .orEmpty() @@ -211,9 +212,14 @@ class FirebaseNoteRepository( awaitClose { listener.remove() } } + /** + * Migration'dan önce yazılmış notlar hâlâ tam `uploaderEmail` taşıyor; okunurken + * adı ondan türetiyoruz. Yeni notlarda alan hiç yok — bkz. + * `firestore-tests/strip-uploader-email.js`. + */ private fun toPost( document: DocumentSnapshot, - defaultUploaderEmail: String = "" + defaultUploaderName: String = "" ): Post? { val title = document.getString("title") ?: return null val createdAt = try { @@ -228,7 +234,7 @@ class FirebaseNoteRepository( id = document.id, title = title, desc = document.getString("description") ?: "", - authorEmail = document.getString("uploaderEmail") ?: defaultUploaderEmail, + uploaderName = document.displayName() ?: defaultUploaderName, department = document.getString("department") ?: "", timeMills = createdAt, uploaderUid = document.getString("uploaderUid") ?: "", @@ -246,6 +252,11 @@ class FirebaseNoteRepository( ) } + /** Yeni şekil önce; eski notlarda ad e-postadan türetiliyor. */ + private fun DocumentSnapshot.displayName(): String? = + getString("uploaderName") + ?: getString("uploaderEmail")?.uploaderName() + private fun DocumentReference.fileDocument(): DocumentReference = collection(CONTENT_COLLECTION).document(FILE_DOCUMENT) @@ -255,7 +266,7 @@ class FirebaseNoteRepository( return LeaderboardEntry( docId = document.id, title = title, - uploaderEmail = document.getString("uploaderEmail") ?: "", + uploaderName = document.displayName() ?: "", department = document.getString("department") ?: "", ratingCount = document.getLong("ratingCount") ?: 0L, ratingSum = document.getLong("ratingSum") ?: 0L diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/NoteRepository.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/NoteRepository.kt index 1d7b791..71f6588 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/NoteRepository.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/NoteRepository.kt @@ -11,7 +11,7 @@ interface NoteRepository { suspend fun createNote( draft: NoteDraft, uploaderUid: String, - uploaderEmail: String, + uploaderName: String, department: String ) @@ -28,7 +28,7 @@ interface NoteRepository { fun observeNotesByUploader( uploaderUid: String, - defaultUploaderEmail: String + defaultUploaderName: String ): Flow> /** diff --git a/app/src/main/java/duygu/yilmaz/campusnote/ui/profile/ProfileViewModel.kt b/app/src/main/java/duygu/yilmaz/campusnote/ui/profile/ProfileViewModel.kt index 2f4c6cd..6779ccd 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/ui/profile/ProfileViewModel.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/ui/profile/ProfileViewModel.kt @@ -4,6 +4,7 @@ import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope +import duygu.yilmaz.campusnote.data.model.uploaderName import duygu.yilmaz.campusnote.data.repository.AuthRepository import duygu.yilmaz.campusnote.data.repository.FirebaseAuthRepository import duygu.yilmaz.campusnote.data.repository.FirebaseNoteRepository @@ -53,7 +54,7 @@ class ProfileViewModel( noteRepository.observeNotesByUploader( uploaderUid = user.uid, - defaultUploaderEmail = user.email + defaultUploaderName = user.email.uploaderName() ) .catch { throwable -> _uiState.value = ProfileUiState.Error( diff --git a/app/src/main/java/duygu/yilmaz/campusnote/ui/upload/UploadViewModel.kt b/app/src/main/java/duygu/yilmaz/campusnote/ui/upload/UploadViewModel.kt index ab2c38b..637ec75 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/ui/upload/UploadViewModel.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/ui/upload/UploadViewModel.kt @@ -6,6 +6,7 @@ import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import duygu.yilmaz.campusnote.data.model.NoteDraft import duygu.yilmaz.campusnote.data.model.UserProfile +import duygu.yilmaz.campusnote.data.model.uploaderName import duygu.yilmaz.campusnote.data.repository.AuthRepository import duygu.yilmaz.campusnote.data.repository.FirebaseAuthRepository import duygu.yilmaz.campusnote.data.repository.FirebaseNoteRepository @@ -51,7 +52,7 @@ class UploadViewModel( noteRepository.createNote( draft = draft, uploaderUid = user.uid, - uploaderEmail = user.email.ifBlank { profile.email }, + uploaderName = user.email.ifBlank { profile.email }.uploaderName(), department = profile.department .ifBlank { UserProfile.UNKNOWN_DEPARTMENT } ) diff --git a/app/src/test/java/duygu/yilmaz/campusnote/data/model/UploaderNameTest.kt b/app/src/test/java/duygu/yilmaz/campusnote/data/model/UploaderNameTest.kt index ebc8a30..d92b2a9 100644 --- a/app/src/test/java/duygu/yilmaz/campusnote/data/model/UploaderNameTest.kt +++ b/app/src/test/java/duygu/yilmaz/campusnote/data/model/UploaderNameTest.kt @@ -4,47 +4,25 @@ import org.junit.Assert.assertEquals import org.junit.Test /** - * Yükleyen adının maskelenmesi. + * Yükleyen adının e-postadan türetilmesi. * - * Bu maskeleme bir gizlilik kararı: kimse başka bir öğrencinin tam okul adresini - * görmemeli. Kural üç ekranda birden kullanıldığı için ([Post], [LeaderboardEntry] - * ve not detayı) burada tek yerden sabitleniyor — biri sessizce tam adresi - * göstermeye dönerse test düşer. + * Bu eskiden bir *gösterim* kararıydı: adres nota tam hâliyle yazılıyor, ekranda + * kırpılıyordu. Kırpmak adresi ekrandan saklıyordu, dokümandan değil — aynı + * bölümdeki herkes ham alanı okuyabiliyordu. + * + * Artık türetme yazma anında yapılıyor: nota yalnızca bu fonksiyonun sonucu + * yazılıyor, adres hiç gitmiyor. Fonksiyon iki yerde yaşıyor — yükleme yolunda ve + * migration'dan önce yazılmış notları okurken — ve ikisinin aynı cevabı vermesi + * gerekiyor, çünkü kural adı oturumdaki e-postanın aynı şekilde bölünmüş hâliyle + * karşılaştırıyor. */ class UploaderNameTest { @Test - fun `okul adresinin sadece kullanici adi kismi gosterilir`() { + fun `okul adresinin sadece kullanici adi kismi alinir`() { assertEquals("duygu.yilmaz", "duygu.yilmaz@ogr.akdeniz.edu.tr".uploaderName()) } - @Test - fun `notun yukleyen adi tam adresi sizdirmaz`() { - val post = Post( - id = "note-1", - title = "Veri Yapıları Özeti", - desc = "", - authorEmail = "duygu.yilmaz@ogr.akdeniz.edu.tr", - department = "Bilgisayar Mühendisliği", - timeMills = 0L - ) - - assertEquals("duygu.yilmaz", post.uploaderName) - } - - @Test - fun `liderlik tablosu da ayni adi gosterir`() { - val entry = LeaderboardEntry( - docId = "note-1", - title = "Veri Yapıları Özeti", - uploaderEmail = "duygu.yilmaz@ogr.akdeniz.edu.tr", - department = "Bilgisayar Mühendisliği", - ratingCount = 3L - ) - - assertEquals("duygu.yilmaz", entry.uploaderName) - } - @Test fun `adres bos kalmis eski kayitlarda bos ad doner`() { // Firestore'da `uploaderEmail` boş kalmış notlar var; çökmemeli. @@ -56,6 +34,10 @@ class UploaderNameTest { assertEquals("duygu", "duygu".uploaderName()) } + /** + * Kuraldaki `email.split('@')[0]` ile aynı davranış: ilk parça alınır. İkisi + * ayrışırsa yükleme sunucuda reddedilir, o yüzden burada sabitleniyor. + */ @Test fun `birden fazla at isaretinde ilk parca alinir`() { assertEquals("duygu", "duygu@ogr@akdeniz.edu.tr".uploaderName()) diff --git a/app/src/test/java/duygu/yilmaz/campusnote/testing/FakeRepositories.kt b/app/src/test/java/duygu/yilmaz/campusnote/testing/FakeRepositories.kt index 706f97f..95d35ef 100644 --- a/app/src/test/java/duygu/yilmaz/campusnote/testing/FakeRepositories.kt +++ b/app/src/test/java/duygu/yilmaz/campusnote/testing/FakeRepositories.kt @@ -124,11 +124,11 @@ class FakeNoteRepository : NoteRepository { override suspend fun createNote( draft: NoteDraft, uploaderUid: String, - uploaderEmail: String, + uploaderName: String, department: String ) { createNoteError?.let { throw it } - createdNotes += CreatedNote(draft, uploaderUid, uploaderEmail, department) + createdNotes += CreatedNote(draft, uploaderUid, uploaderName, department) } override suspend fun getNoteFile(noteId: String): String? { @@ -148,9 +148,9 @@ class FakeNoteRepository : NoteRepository { override fun observeNotesByUploader( uploaderUid: String, - defaultUploaderEmail: String + defaultUploaderName: String ): Flow> { - observedUploader = uploaderUid to defaultUploaderEmail + observedUploader = uploaderUid to defaultUploaderName return uploaderNotes } @@ -182,7 +182,7 @@ class FakeNoteRepository : NoteRepository { data class CreatedNote( val draft: NoteDraft, val uploaderUid: String, - val uploaderEmail: String, + val uploaderName: String, val department: String ) diff --git a/app/src/test/java/duygu/yilmaz/campusnote/testing/Fixtures.kt b/app/src/test/java/duygu/yilmaz/campusnote/testing/Fixtures.kt index ea488e5..290c702 100644 --- a/app/src/test/java/duygu/yilmaz/campusnote/testing/Fixtures.kt +++ b/app/src/test/java/duygu/yilmaz/campusnote/testing/Fixtures.kt @@ -25,7 +25,7 @@ fun post( id: String = "note-1", title: String = "Veri Yapıları Özeti", uploaderUid: String = "uid-2", - authorEmail: String = "baskasi@ogr.akdeniz.edu.tr", + uploaderName: String = "baskasi", department: String = "Bilgisayar Mühendisliği", ratingSum: Long = 0L, fileType: String = "" @@ -33,7 +33,7 @@ fun post( id = id, title = title, desc = "", - authorEmail = authorEmail, + uploaderName = uploaderName, department = department, timeMills = 0L, uploaderUid = uploaderUid, @@ -49,7 +49,7 @@ fun leaderboardEntry( ) = LeaderboardEntry( docId = docId, title = title, - uploaderEmail = "baskasi@ogr.akdeniz.edu.tr", + uploaderName = "baskasi", department = "Bilgisayar Mühendisliği", ratingCount = ratingCount, ratingSum = ratingSum diff --git a/app/src/test/java/duygu/yilmaz/campusnote/ui/leaderboard/LeaderboardViewModelTest.kt b/app/src/test/java/duygu/yilmaz/campusnote/ui/leaderboard/LeaderboardViewModelTest.kt index 4a1a2be..225bf3b 100644 --- a/app/src/test/java/duygu/yilmaz/campusnote/ui/leaderboard/LeaderboardViewModelTest.kt +++ b/app/src/test/java/duygu/yilmaz/campusnote/ui/leaderboard/LeaderboardViewModelTest.kt @@ -44,7 +44,7 @@ class LeaderboardViewModelTest { private fun entry(id: String, ratingSum: Long) = LeaderboardEntry( docId = id, title = "Not $id", - uploaderEmail = "ogrenci@ogr.akdeniz.edu.tr", + uploaderName = "ogrenci", department = "Bilgisayar Mühendisliği", ratingCount = 3L, ratingSum = ratingSum diff --git a/app/src/test/java/duygu/yilmaz/campusnote/ui/profile/ProfileViewModelTest.kt b/app/src/test/java/duygu/yilmaz/campusnote/ui/profile/ProfileViewModelTest.kt index 12e2ede..31695d3 100644 --- a/app/src/test/java/duygu/yilmaz/campusnote/ui/profile/ProfileViewModelTest.kt +++ b/app/src/test/java/duygu/yilmaz/campusnote/ui/profile/ProfileViewModelTest.kt @@ -79,7 +79,9 @@ class ProfileViewModelTest { assertEquals(2, state.posts.size) assertEquals("duygu@ogr.akdeniz.edu.tr", state.email) assertEquals("Bilgisayar Mühendisliği", state.department) - assertEquals("uid-1" to "duygu@ogr.akdeniz.edu.tr", noteRepository.observedUploader) + // Profil ekranı kendi e-postasını tam gösterir, ama notlara yedek ad + // olarak yine kırpılmış hâli geçer. + assertEquals("uid-1" to "duygu", noteRepository.observedUploader) } @Test diff --git a/app/src/test/java/duygu/yilmaz/campusnote/ui/upload/UploadViewModelTest.kt b/app/src/test/java/duygu/yilmaz/campusnote/ui/upload/UploadViewModelTest.kt index 9427f93..36d329d 100644 --- a/app/src/test/java/duygu/yilmaz/campusnote/ui/upload/UploadViewModelTest.kt +++ b/app/src/test/java/duygu/yilmaz/campusnote/ui/upload/UploadViewModelTest.kt @@ -21,7 +21,7 @@ import java.io.IOException /** * [UploadViewModel] birim testleri. * - * Yükleme, katkı kapısını açan tek eylem. Bu yüzden nota yazılan bölüm ve e-posta + * Yükleme, katkı kapısını açan tek eylem. Bu yüzden nota yazılan bölüm ve ad * bilgisinin doğruluğu kritik: yanlış bölüm yazılan not, sahibinin feed'ini açmadığı * gibi başka bir bölümün akışına da düşer. */ @@ -73,12 +73,14 @@ class UploadViewModelTest { val created = noteRepository.createdNotes.single() assertEquals(draft, created.draft) assertEquals("uid-1", created.uploaderUid) - assertEquals("duygu@ogr.akdeniz.edu.tr", created.uploaderEmail) + // Nota adresin kendisi değil, `@` öncesi kısmı yazılır — kural da + // oturumdaki e-postayı aynı şekilde bölüp karşılaştırıyor. + assertEquals("duygu", created.uploaderName) assertEquals("Bilgisayar Mühendisliği", created.department) } @Test - fun `oturumda e-posta yoksa profildeki adres kullanilir`() = + fun `oturumda e-posta yoksa profildeki adresten ad turetilir`() = runTest(mainDispatcherRule.testDispatcher) { authRepository.setUser(authenticatedUser(uid = "uid-1", email = "")) userRepository.profile = userProfile(email = "yedek@ogr.akdeniz.edu.tr") @@ -87,10 +89,7 @@ class UploadViewModelTest { viewModel.uploadNote(noteDraft()) advanceUntilIdle() - assertEquals( - "yedek@ogr.akdeniz.edu.tr", - noteRepository.createdNotes.single().uploaderEmail - ) + assertEquals("yedek", noteRepository.createdNotes.single().uploaderName) } @Test diff --git a/firestore-tests/package.json b/firestore-tests/package.json index a7a2ffc..4ac620f 100644 --- a/firestore-tests/package.json +++ b/firestore-tests/package.json @@ -6,7 +6,8 @@ "type": "module", "scripts": { "test": "firebase emulators:exec --config ../firebase.json --only firestore --project campusnote-rules-test \"vitest run\"", - "backfill:created-at": "node backfill-created-at.js" + "backfill:created-at": "node backfill-created-at.js", + "strip:uploader-email": "node strip-uploader-email.js" }, "devDependencies": { "@firebase/rules-unit-testing": "^4.0.1", diff --git a/firestore-tests/rules.test.js b/firestore-tests/rules.test.js index 437a9e6..ba504ea 100644 --- a/firestore-tests/rules.test.js +++ b/firestore-tests/rules.test.js @@ -5,12 +5,16 @@ import { initializeTestEnvironment, } from "@firebase/rules-unit-testing"; import { + collection, deleteDoc, doc, getDoc, + getDocs, + query, serverTimestamp, setDoc, updateDoc, + where, writeBatch, } from "firebase/firestore"; import { afterAll, beforeAll, beforeEach, describe, test } from "vitest"; @@ -22,6 +26,9 @@ const RATER = "rater-uid"; const NOTE = "note-1"; const OWNER_EMAIL = "owner@ogr.akdeniz.edu.tr"; const RATER_EMAIL = "rater@ogr.akdeniz.edu.tr"; +// The rule derives the only permitted name from the session's own address. +const OWNER_NAME = "owner"; +const RATER_NAME = "rater"; const DEPARTMENT = "Bilgisayar Mühendisliği"; let testEnv; @@ -70,7 +77,7 @@ beforeEach(async () => { createdAt: 1, }); await setDoc(doc(db, "notes", NOTE), { - ...noteFields({ uploaderUid: OWNER, uploaderEmail: OWNER_EMAIL }), + ...noteFields({ uploaderUid: OWNER, uploaderName: OWNER_NAME }), createdAt: 1, }); await setDoc(doc(db, "notes", NOTE, "content", "file"), { @@ -98,7 +105,7 @@ function noteFields(overrides = {}) { tag: "", department: DEPARTMENT, uploaderUid: RATER, - uploaderEmail: RATER_EMAIL, + uploaderName: RATER_NAME, createdAt: serverTimestamp(), ratingSum: 0, ratingCount: 0, @@ -151,9 +158,18 @@ describe("signed-out access", () => { }); describe("users", () => { - test("a signed-in user may read any profile", async () => { - // The leaderboard and note detail screens show other people's profiles. - await assertSucceeds(getDoc(doc(rater, "users", OWNER))); + test("a user may read their own profile", async () => { + await assertSucceeds(getDoc(doc(rater, "users", RATER))); + }); + + /** + * Every getUser call in the app passes the caller's own uid — the leaderboard and + * note detail take the uploader's name from the note, not from a profile. The + * open read rule was an permission nobody used, and it handed any signed-in + * student every address and department in the university in one query. + */ + test("a user cannot read someone else's profile", async () => { + await assertFails(getDoc(doc(rater, "users", OWNER))); }); test("registration writes a profile whose id matches the caller", async () => { @@ -214,10 +230,46 @@ describe("users", () => { }); describe("notes", () => { - test("any signed-in user may read notes", async () => { + test("a note in the reader's own department can be read", async () => { await assertSucceeds(getDoc(doc(rater, "notes", NOTE))); }); + /** + * The app filters every feed and leaderboard query by department, but that was a + * rendering decision, not a boundary: a client talking to Firestore directly could + * drop the filter and pull every note in the university. Rules are evaluated + * against each document a query returns and the whole query fails if any is + * denied, so the filter is now the condition for the query to run at all. + */ + test("a note in another department cannot be read", async () => { + await testEnv.withSecurityRulesDisabled(async (context) => { + await setDoc(doc(context.firestore(), "notes", "other-dept"), { + ...noteFields({ department: "Makine Mühendisliği" }), + createdAt: 1, + }); + }); + + await assertFails(getDoc(doc(rater, "notes", "other-dept"))); + }); + + test("a query without the department filter is refused", async () => { + await assertFails(getDocs(collection(rater, "notes"))); + }); + + test("a query filtered to the reader's own department is allowed", async () => { + await assertSucceeds( + getDocs(query(collection(rater, "notes"), where("department", "==", DEPARTMENT))), + ); + }); + + test("a query filtered to another department is refused", async () => { + await assertFails( + getDocs( + query(collection(rater, "notes"), where("department", "==", "Makine Mühendisliği")), + ), + ); + }); + test("uploaderUid must be the caller's own uid", async () => { await assertSucceeds( setDoc(doc(rater, "notes", "own-note"), noteFields()), @@ -225,7 +277,7 @@ describe("notes", () => { await assertFails( setDoc( doc(rater, "notes", "forged-note"), - noteFields({ uploaderUid: OWNER, uploaderEmail: OWNER_EMAIL }), + noteFields({ uploaderUid: OWNER, uploaderName: OWNER_NAME }), ), ); }); @@ -274,11 +326,25 @@ describe("notes", () => { ); }); - test("uploaderEmail cannot be impersonated", async () => { + test("uploaderName cannot be impersonated", async () => { await assertFails( setDoc( - doc(rater, "notes", "forged-email"), - noteFields({ uploaderEmail: OWNER_EMAIL }), + doc(rater, "notes", "forged-name"), + noteFields({ uploaderName: OWNER_NAME }), + ), + ); + }); + + /** + * The address itself is no longer stored anywhere on a note. Masking it at + * render time hid it from the screen, not from the document — every student in + * the department could read the raw field. + */ + test("an email address cannot be written onto a note", async () => { + await assertFails( + setDoc( + doc(rater, "notes", "with-email"), + noteFields({ uploaderEmail: RATER_EMAIL }), ), ); }); @@ -410,10 +476,27 @@ describe("notes", () => { }); describe("note content", () => { - test("any signed-in user may read the attached file", async () => { + test("a file on a note in the reader's own department can be read", async () => { await assertSucceeds(getDoc(doc(rater, "notes", NOTE, "content", "file"))); }); + // Without this the department boundary could be walked around through the file + // path: the note stays unreadable while its contents do not. + test("a file on another department's note cannot be read", async () => { + await testEnv.withSecurityRulesDisabled(async (context) => { + const db = context.firestore(); + await setDoc(doc(db, "notes", "other-dept"), { + ...noteFields({ department: "Makine Mühendisliği" }), + createdAt: 1, + }); + await setDoc(doc(db, "notes", "other-dept", "content", "file"), { + fileData: "ZmFrZQ==", + }); + }); + + await assertFails(getDoc(doc(rater, "notes", "other-dept", "content", "file"))); + }); + test("upload writes the note and its content in one batch", async () => { const batch = writeBatch(rater); batch.set(doc(rater, "notes", "batched"), noteFields()); diff --git a/firestore-tests/strip-uploader-email.js b/firestore-tests/strip-uploader-email.js new file mode 100644 index 0000000..d41c324 --- /dev/null +++ b/firestore-tests/strip-uploader-email.js @@ -0,0 +1,149 @@ +/** + * Replaces every note's `uploaderEmail` with a `uploaderName` holding only the part + * the app ever displayed. + * + * Deliberately has no `#!` line, for the same reason as backfill-created-at.js: the + * repo normalises to CRLF on Windows checkouts and esbuild does not strip a shebang + * ending in `\r`, which broke the test that imports the file — on Windows only. + * + * Why this exists: the three screens that show an uploader all rendered + * `email.substringBefore("@")`, so the address itself was never on screen. It was + * still in the document, and any signed-in student could read it — masking in the UI + * is not a privacy boundary, it is a rendering choice. Scoping note reads to a + * department narrows who can see it; deleting the field is what actually removes it. + * + * The script reports by default and only writes when told to: + * + * node strip-uploader-email.js --project # dry run, changes nothing + * node strip-uploader-email.js --project --apply # writes + * + * Against production it needs application-default credentials: + * + * gcloud auth application-default login + * + * Against the emulator, set FIRESTORE_EMULATOR_HOST instead; no credentials are used. + */ + +import { resolve } from "node:path"; +import { fileURLToPath } from "node:url"; +import { initializeApp } from "firebase-admin/app"; +import { FieldValue, Firestore } from "firebase-admin/firestore"; + +const NOTES = "notes"; +const EMAIL_FIELD = "uploaderEmail"; +const NAME_FIELD = "uploaderName"; +// Firestore accepts at most 500 writes per batch. +const BATCH_LIMIT = 450; + +/** + * The same derivation the app used at render time: everything before the first `@`. + * + * @returns the name to store, or null when the document needs no write. + */ +export function displayNameFor(data) { + const existing = data?.[NAME_FIELD]; + const email = data?.[EMAIL_FIELD]; + + // Already migrated, and no address left behind. + if (typeof existing === "string" && existing.length > 0 && email === undefined) { + return null; + } + + if (typeof email === "string" && email.length > 0) { + return email.split("@")[0]; + } + + // No address to derive from. An existing name is kept; otherwise the note keeps + // whatever it has rather than gaining an empty field it never had. + if (typeof existing === "string") return null; + + return ""; +} + +export function reasonFor(data) { + const hasEmail = typeof data?.[EMAIL_FIELD] === "string"; + const hasName = typeof data?.[NAME_FIELD] === "string"; + + if (hasEmail && hasName) return "both"; + if (hasEmail) return "email"; + if (hasName) return "name"; + return "neither"; +} + +export async function strip(firestore, { apply = false, log = console.log } = {}) { + const snapshot = await firestore.collection(NOTES).get(); + + const counts = { both: 0, email: 0, name: 0, neither: 0 }; + const pending = []; + + for (const doc of snapshot.docs) { + const data = doc.data(); + counts[reasonFor(data)] += 1; + + const replacement = displayNameFor(data); + if (replacement !== null) { + pending.push({ ref: doc.ref, value: replacement, id: doc.id }); + } + } + + log(`notes scanned: ${snapshot.size}`); + log(` carrying an email: ${counts.email}`); + log(` carrying email and name: ${counts.both}`); + log(` already migrated: ${counts.name}`); + log(` neither field: ${counts.neither}`); + log(`needs a write: ${pending.length}`); + + if (!apply) { + log("dry run — nothing was written. Re-run with --apply to write."); + return { scanned: snapshot.size, counts, written: 0 }; + } + + let written = 0; + for (let i = 0; i < pending.length; i += BATCH_LIMIT) { + const batch = firestore.batch(); + for (const item of pending.slice(i, i + BATCH_LIMIT)) { + // One update per document: the name arrives and the address leaves together, + // so no note is ever readable with both, and a partial run leaves the rest + // untouched rather than half-converted. + batch.update(item.ref, { + [NAME_FIELD]: item.value, + [EMAIL_FIELD]: FieldValue.delete(), + }); + } + await batch.commit(); + written += Math.min(BATCH_LIMIT, pending.length - i); + log(`written: ${written}/${pending.length}`); + } + + return { scanned: snapshot.size, counts, written }; +} + +/** Only runs when the file is executed directly, so the tests can import it. */ +const runDirectly = + process.argv[1] && resolve(process.argv[1]) === fileURLToPath(import.meta.url); + +if (runDirectly) { + const args = process.argv.slice(2); + const apply = args.includes("--apply"); + const projectIndex = args.indexOf("--project"); + const projectId = + projectIndex >= 0 ? args[projectIndex + 1] : process.env.GOOGLE_CLOUD_PROJECT; + + if (!projectId) { + console.error("usage: node strip-uploader-email.js --project [--apply]"); + process.exit(2); + } + + initializeApp({ projectId }); + const firestore = new Firestore({ projectId }); + + strip(firestore, { apply }) + .then((result) => { + console.log(apply ? `done — ${result.written} note(s) updated.` : "done."); + process.exit(0); + }) + .catch((error) => { + console.error(error); + process.exit(1); + }); +} diff --git a/firestore-tests/strip-uploader-email.test.js b/firestore-tests/strip-uploader-email.test.js new file mode 100644 index 0000000..e308839 --- /dev/null +++ b/firestore-tests/strip-uploader-email.test.js @@ -0,0 +1,142 @@ +import { initializeApp, deleteApp } from "firebase-admin/app"; +import { Firestore } from "firebase-admin/firestore"; +import { afterAll, beforeAll, beforeEach, describe, expect, test } from "vitest"; +import { displayNameFor, reasonFor, strip } from "./strip-uploader-email.js"; + +// The emulator is already running (npm test wraps the suite in emulators:exec), and +// the admin SDK talks to it as long as this is set before it connects. +process.env.FIRESTORE_EMULATOR_HOST ??= "127.0.0.1:8080"; + +const PROJECT = "campusnote-rules-test"; + +let app; +let firestore; + +/** Swallows the script's progress output so the test run stays readable. */ +const quiet = () => {}; + +beforeAll(() => { + app = initializeApp({ projectId: PROJECT }, "strip-email-test"); + firestore = new Firestore({ projectId: PROJECT }); +}); + +afterAll(async () => { + await deleteApp(app); +}); + +beforeEach(async () => { + const existing = await firestore.collection("notes").get(); + await Promise.all(existing.docs.map((doc) => doc.ref.delete())); +}); + +const note = (extra) => ({ + title: "Ders notu", + department: "Bilgisayar Mühendisliği", + uploaderUid: "uid-1", + ...extra, +}); + +describe("classifying a note", () => { + test("a legacy note yields the name in front of the @", () => { + const data = note({ uploaderEmail: "duygu@ogr.akdeniz.edu.tr" }); + + expect(reasonFor(data)).toBe("email"); + expect(displayNameFor(data)).toBe("duygu"); + }); + + test("an already migrated note needs no write", () => { + const data = note({ uploaderName: "duygu" }); + + expect(reasonFor(data)).toBe("name"); + expect(displayNameFor(data)).toBeNull(); + }); + + /** + * A half-migrated note — the name arrived but the address is still there — is the + * shape a partial run leaves behind, and it is exactly the one that must still be + * rewritten. Treating it as "done" would leave the address in place forever. + */ + test("a note carrying both is still rewritten", () => { + const data = note({ uploaderName: "duygu", uploaderEmail: "duygu@ogr.akdeniz.edu.tr" }); + + expect(reasonFor(data)).toBe("both"); + expect(displayNameFor(data)).toBe("duygu"); + }); + + test("a note with neither field gets an empty name rather than being skipped", () => { + const data = note({}); + + expect(reasonFor(data)).toBe("neither"); + expect(displayNameFor(data)).toBe(""); + }); +}); + +describe("running against Firestore", () => { + test("a dry run reports without writing", async () => { + await firestore + .collection("notes") + .doc("legacy") + .set(note({ uploaderEmail: "duygu@ogr.akdeniz.edu.tr" })); + + const result = await strip(firestore, { log: quiet }); + + expect(result.written).toBe(0); + const after = await firestore.collection("notes").doc("legacy").get(); + expect(after.data().uploaderEmail).toBe("duygu@ogr.akdeniz.edu.tr"); + expect(after.data().uploaderName).toBeUndefined(); + }); + + /** + * The assertion that matters: after the migration the address is not merely + * hidden from the UI, it is absent from the document. + */ + test("applying replaces the address with the name", async () => { + await firestore + .collection("notes") + .doc("legacy") + .set(note({ uploaderEmail: "duygu@ogr.akdeniz.edu.tr" })); + + const result = await strip(firestore, { apply: true, log: quiet }); + + expect(result.written).toBe(1); + const after = await firestore.collection("notes").doc("legacy").get(); + expect(after.data().uploaderName).toBe("duygu"); + expect(after.data().uploaderEmail).toBeUndefined(); + }); + + test("re-running after a partial failure is safe", async () => { + await firestore + .collection("notes") + .doc("done") + .set(note({ uploaderName: "duygu" })); + await firestore + .collection("notes") + .doc("pending") + .set(note({ uploaderEmail: "asya@ogr.akdeniz.edu.tr" })); + + const first = await strip(firestore, { apply: true, log: quiet }); + expect(first.written).toBe(1); + + const second = await strip(firestore, { apply: true, log: quiet }); + expect(second.written).toBe(0); + + const pending = await firestore.collection("notes").doc("pending").get(); + expect(pending.data().uploaderName).toBe("asya"); + expect(pending.data().uploaderEmail).toBeUndefined(); + }); + + test("no note is left carrying an address", async () => { + await firestore.collection("notes").doc("a").set(note({ uploaderEmail: "a@x.tr" })); + await firestore.collection("notes").doc("b").set(note({ uploaderName: "b" })); + await firestore + .collection("notes") + .doc("c") + .set(note({ uploaderName: "c", uploaderEmail: "c@x.tr" })); + + await strip(firestore, { apply: true, log: quiet }); + + const all = await firestore.collection("notes").get(); + const withEmail = all.docs.filter((doc) => doc.data().uploaderEmail !== undefined); + expect(withEmail).toHaveLength(0); + }); +}); diff --git a/firestore.rules b/firestore.rules index 415bb6a..786c5f0 100644 --- a/firestore.rules +++ b/firestore.rules @@ -37,8 +37,18 @@ service cloud.firestore { } match /users/{userId} { - // Liderlik tablosu ve profil ekranı diğer kullanıcıları da okuyabilmeli. - allow read: if isSignedIn(); + /** + * Profil yalnızca sahibine açık. + * + * Buradaki eski yorum "liderlik tablosu ve profil ekranı diğer kullanıcıları + * da okuyabilmeli" diyordu; doğru değildi. Uygulamadaki dört `getUser` + * çağrısının dördü de oturumdaki kullanıcının kendi uid'ini geçiyor — sıralama + * ve not detayı yükleyenin adını notun kendi alanından alıyor, profilden + * değil. Herkese açık okuma, kimsenin ihtiyaç duymadığı bir izindi: oturum + * açmış biri bütün öğrencilerin e-postasını ve bölümünü tek sorguda + * indirebiliyordu. + */ + allow read: if isOwner(userId); // Kayıt sırasında kullanıcı yalnızca kendi dokümanını oluşturabilir. allow create: if isOwner(userId) @@ -58,7 +68,18 @@ service cloud.firestore { } match /notes/{noteId} { - allow read: if isSignedIn(); + /** + * Notlar yalnızca kendi bölümüne açık. + * + * Arayüzün her sorguyu `department`'a filtrelemesi bir sınır değildi: + * Firestore'a doğrudan bağlanan bir istemci filtreyi kaldırıp bütün + * üniversitenin notlarını indirebiliyordu. + * + * Sorgular için kural her dönen dokümana ayrı ayrı uygulanır ve biri bile + * düşerse sorgunun tamamı reddedilir — yani feed'in `where department == X` + * filtresi artık isteğe bağlı bir nezaket değil, sorgunun geçme koşulu. + */ + allow read: if isSignedIn() && resource.data.department == ownDepartment(); /** * Not oluşturma. @@ -79,7 +100,10 @@ service cloud.firestore { && hasValidNoteTypes() // Kimlik istemciden değil, oturumdan geliyor. && request.resource.data.uploaderUid == request.auth.uid - && request.resource.data.uploaderEmail == request.auth.token.email + // Nota e-posta hiç yazılmıyor: gösterilen ad doğrudan + // saklanıyor. Arayüzde kırpmak dokümandaki adresi gizlemiyordu + // — aynı bölümdeki herkes ham `uploaderEmail`'i okuyabiliyordu. + && request.resource.data.uploaderName == ownDisplayName() // Not, yazarın kendi bölümüne düşmek zorunda: aksi halde biri // başka bölümün feed'ine içerik sokabilirdi. Bölümü boş kalmış // eski bir profil bu yüzden hiç yükleyemez — uygulamadaki @@ -97,7 +121,7 @@ service cloud.firestore { function noteFields() { return [ 'course', 'title', 'description', 'tag', 'department', - 'uploaderUid', 'uploaderEmail', 'createdAt', + 'uploaderUid', 'uploaderName', 'createdAt', 'ratingSum', 'ratingCount', 'fileName', 'fileType', 'fileSize' ]; @@ -121,7 +145,7 @@ service cloud.firestore { && data.tag is string && data.tag.size() <= 50 && data.department is string && data.uploaderUid is string - && data.uploaderEmail is string + && data.uploaderName is string && data.uploaderName.size() <= 64 && data.createdAt is timestamp && data.ratingSum is int && data.ratingCount is int @@ -130,12 +154,33 @@ service cloud.firestore { && data.fileSize is int && data.fileSize >= 0; } - /** Yazarın kendi profilindeki bölüm; istemcinin gönderdiği değil. */ + /** + * Çağıranın kendi profilindeki bölüm; istemcinin gönderdiği değil. + * + * Profili olmayan bir hesapta `get()` null döner ve erişim değerlendirme + * hatasına düşer — yani kapalı kalır, ki doğrusu bu. + */ function ownDepartment() { return get(/databases/$(database)/documents/users/$(request.auth.uid)) .data.department; } + /** + * Oturumdaki e-postanın `@` öncesi kısmı — nota yazılabilecek tek ad. + * + * Adın oturumdan türetilmesi, birinin başkasının adıyla not paylaşmasını + * engelliyor; e-postanın kendisi hiçbir yere yazılmıyor. + */ + function ownDisplayName() { + return request.auth.token.email.split('@')[0]; + } + + /** Dosya içeriği kuralı için üst notun bölümü. */ + function noteDepartment(noteId) { + return get(/databases/$(database)/documents/notes/$(noteId)) + .data.department; + } + allow update: if isSignedIn() && ( isMetadataEditByOwner() || isRatingWrite(noteId) @@ -148,7 +193,8 @@ service cloud.firestore { return resource.data.uploaderUid == request.auth.uid && !changedFields().hasAny([ 'ratingSum', 'ratingCount', 'avgRating', - 'uploaderUid', 'uploaderEmail', 'department', 'createdAt' + 'uploaderUid', 'uploaderName', 'uploaderEmail', + 'department', 'createdAt' ]); } @@ -195,7 +241,9 @@ service cloud.firestore { // Dosya içeriği ayrı dokümanda: feed sorguları base64 veriyi indirmesin. match /content/{contentId} { - allow read: if isSignedIn(); + // Dosya, bağlı olduğu notla aynı kapsamda: notu okuyamayan içeriğini de + // okuyamamalı, aksi halde bölüm sınırı dosya yolundan atlanırdı. + allow read: if isSignedIn() && noteDepartment(noteId) == ownDepartment(); // Not ile içerik aynı batch'te yazılıyor. Batch içindeki her yazma // ayrı değerlendirilir ve get() henüz commit edilmemiş üst dokümanı From ebd347ace9af7685bae2d654c469bbb6b5e40707 Mon Sep 17 00:00:00 2001 From: Duygu <84078855+duuyguyiilmaz@users.noreply.github.com> Date: Tue, 1 Sep 2026 18:54:52 +0300 Subject: [PATCH 3/6] fix: let the department read rule cover the app's own-notes query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit scoped note reads to the caller's department and nothing else. Deployed against the live project, the feed opened empty: the contribution gate and the profile screen both query `where uploaderUid == me`, carrying no department filter, and the rule rejected them. The reason is a property of Firestore I had wrong. Rules on a list operation are evaluated against the *query*, not against the documents it would return — Firestore will not run a query unless its filters guarantee the rule holds. That these particular notes were the caller's own, in the caller's own department, is not something it works out. So the rule needs a branch matching each query the app actually builds, and the second branch — `uploaderUid == request.auth.uid` — is what the profile and the gate depend on. It is not slack: a query with neither filter still satisfies no branch and is still refused. Two tests now cover it, and they are the ones that would have caught this before the deploy: the own-notes query succeeds without a department filter, and the same query aimed at another uid fails. The cross-department fixture had to change too — it was written as the reader's own note, so the new branch let it through, which the suite caught immediately. Verified on the device with the corrected rules deployed: the feed lists the department's notes again, the profile shows its own, and an upload lands with `uploaderName` and no PERMISSION_DENIED anywhere in the log. Co-Authored-By: Claude Opus 5 --- README.md | 14 +++++++++----- firestore-tests/rules.test.js | 36 +++++++++++++++++++++++++++++++++-- firestore.rules | 20 +++++++++++++++---- 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 310ee15..af88c98 100644 --- a/README.md +++ b/README.md @@ -215,9 +215,13 @@ signed-in student could pull every profile, every note and every attached file i university. 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; because rules are evaluated against each document a query returns and the whole -query fails if one is denied, the feed's `where department == …` filter is now the -condition for the query to run rather than a courtesy. Profiles went further and are +profile, or the note is the caller's own. Firestore evaluates a list against the *query* +rather than the documents it would return, so an unfiltered query is refused before it +runs and the feed's `where department == …` filter becomes the condition for running +rather than a courtesy. The second branch is not slack: the contribution gate and the +profile screen query `where uploaderUid == me` with no department filter, and a +single-branch rule rejected them outright — the feed opened empty in production, which is +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. @@ -435,7 +439,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. -55 tests in [`firestore-tests/rules.test.js`](firestore-tests/rules.test.js) run +57 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. @@ -444,7 +448,7 @@ it down again; nothing touches the real project, and no billing account is invol |---|---| | `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. | -| `notes` (reads) | A note in the reader's own department is readable and one in another department is not; an unfiltered query over the collection is refused, a query filtered to the reader's own department is allowed, and one filtered to another department is refused. The last three are what make the scoping real rather than advisory. | +| `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. | | `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 replace and delete, that a file on another department's note cannot be read — otherwise the boundary is walked around through the file path — and a test that deliberately asserts the *open* create rule, so the gap documented in `firestore.rules` cannot be closed by accident and go unnoticed. | diff --git a/firestore-tests/rules.test.js b/firestore-tests/rules.test.js index ba504ea..d31fc6b 100644 --- a/firestore-tests/rules.test.js +++ b/firestore-tests/rules.test.js @@ -244,7 +244,13 @@ describe("notes", () => { test("a note in another department cannot be read", async () => { await testEnv.withSecurityRulesDisabled(async (context) => { await setDoc(doc(context.firestore(), "notes", "other-dept"), { - ...noteFields({ department: "Makine Mühendisliği" }), + // Başkasının notu olmalı: kendi notun bölüm değişse de okunabilir kalıyor, + // o yüzden aynı fixture bu testi ölçmezdi. + ...noteFields({ + department: "Makine Mühendisliği", + uploaderUid: OWNER, + uploaderName: OWNER_NAME, + }), createdAt: 1, }); }); @@ -262,6 +268,26 @@ describe("notes", () => { ); }); + /** + * The query the contribution gate and the profile screen actually build. It + * carries no department filter, and a rule with only the department branch + * rejected it outright — Firestore evaluates a list against the query, not the + * documents it would return, so "these happen to be my own notes in my own + * department" is not something it can work out. The feed opened empty in + * production before this branch existed. + */ + test("a query for the reader's own notes is allowed without a department filter", async () => { + await assertSucceeds( + getDocs(query(collection(rater, "notes"), where("uploaderUid", "==", RATER))), + ); + }); + + test("a query for someone else's notes is refused", async () => { + await assertFails( + getDocs(query(collection(rater, "notes"), where("uploaderUid", "==", OWNER))), + ); + }); + test("a query filtered to another department is refused", async () => { await assertFails( getDocs( @@ -486,7 +512,13 @@ describe("note content", () => { await testEnv.withSecurityRulesDisabled(async (context) => { const db = context.firestore(); await setDoc(doc(db, "notes", "other-dept"), { - ...noteFields({ department: "Makine Mühendisliği" }), + // Başkasının notu olmalı: kendi notun bölüm değişse de okunabilir kalıyor, + // o yüzden aynı fixture bu testi ölçmezdi. + ...noteFields({ + department: "Makine Mühendisliği", + uploaderUid: OWNER, + uploaderName: OWNER_NAME, + }), createdAt: 1, }); await setDoc(doc(db, "notes", "other-dept", "content", "file"), { diff --git a/firestore.rules b/firestore.rules index 786c5f0..f1f439d 100644 --- a/firestore.rules +++ b/firestore.rules @@ -75,11 +75,23 @@ service cloud.firestore { * Firestore'a doğrudan bağlanan bir istemci filtreyi kaldırıp bütün * üniversitenin notlarını indirebiliyordu. * - * Sorgular için kural her dönen dokümana ayrı ayrı uygulanır ve biri bile - * düşerse sorgunun tamamı reddedilir — yani feed'in `where department == X` - * filtresi artık isteğe bağlı bir nezaket değil, sorgunun geçme koşulu. + * Firestore liste sorgularında kuralı dönen belgelere değil *sorgunun kendisine* + * uyguluyor: sorgu, sonuçların kuralı sağlayacağını garanti eden bir filtre + * taşımıyorsa hiç çalıştırılmadan reddediliyor. Yani feed'in + * `where department == X` filtresi artık isteğe bağlı bir nezaket değil, + * sorgunun geçme koşulu. + * + * `uploaderUid` dalı bu yüzden var, gevşeklikten değil: katkı kapısı ve profil + * ekranı `where uploaderUid == ben` sorguluyor ve bölüm filtresi taşımıyor. + * Tek dallı bir kural o sorguları — kullanıcının kendi notları olmasına + * rağmen — reddediyordu; canlıda feed boş açıldı, kapı hiç değerlendirilemedi. + * İki dal, uygulamanın gerçekten kurduğu iki sorguya karşılık geliyor; + * filtresiz bir sorgu ikisini de sağlamadığı için hâlâ reddediliyor. */ - allow read: if isSignedIn() && resource.data.department == ownDepartment(); + allow read: if isSignedIn() && ( + resource.data.department == ownDepartment() + || resource.data.uploaderUid == request.auth.uid + ); /** * Not oluşturma. From 363765138b65c4c40da7fb8c920c5251b23ff95f Mon Sep 17 00:00:00 2001 From: Duygu <84078855+duuyguyiilmaz@users.noreply.github.com> Date: Tue, 1 Sep 2026 19:06:01 +0300 Subject: [PATCH 4/6] fix: bind content creation to the note's owner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `allow create: if isSignedIn()` on notes/{id}/content let any signed-in user hang a content document off anybody's note. Overwriting an existing file was already owner-only, so the hole was narrow but real: a note that had no file yet would take one from a stranger, and the file document is what the detail screen downloads and shows. The rules documented this as unavoidable — a batch cannot `get()` its own uncommitted parent note, so ownership could not be checked at create time. The first half is true and the conclusion is not. `getAfter()` reads the state the commit will leave behind, which is exactly what the rating rule already uses to tie a note's totals to the vote written beside it. The same call reads the parent note's `uploaderUid` from inside the upload batch. Create is now owner-only and the document shape is pinned to a single `fileData` string, so a content document cannot carry fields nobody expects either. Because `getAfter()` returns the current note when the write does not touch it, the rule also covers attaching a file to an existing note of your own, which is the other path the app can take. The test that asserted the gap is replaced by four that assert the closure: a stranger cannot attach content, the owner can — both to a batched note and to an existing one — a document carrying anything but `fileData` is refused, and a batch pairing a legitimate note of the caller's own with a file smuggled onto someone else's fails as a whole. Rules suite 57 -> 61, 75 green overall. Deployed and verified on the device: a note-plus-file upload still commits, and the log carries no PERMISSION_DENIED. Co-Authored-By: Claude Opus 5 --- README.md | 11 ++++++-- firestore-tests/rules.test.js | 50 +++++++++++++++++++++++++++++++---- firestore.rules | 26 +++++++++++++++--- 3 files changed, 76 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index af88c98..7796be2 100644 --- a/README.md +++ b/README.md @@ -226,6 +226,13 @@ readable only by their owner: all four `getUser` calls in the app pass the calle uid, and the leaderboard takes the uploader's name from the note, so the open rule was granting an access nobody needed. +**Attaching a file is owner-only too.** Creating a content document used to be open +to any signed-in user, and the rules said why: a batch cannot `get()` its own +uncommitted parent note, so ownership could not be checked. The `get()` half was true and +the conclusion was not — `getAfter()` reads the state the commit will leave behind, and +the rating rule was already using it for exactly this. Overwriting an existing file was +blocked, but a note that had no file yet would take one from anybody. + **The uploader's address is no longer stored.** Three screens showed `email.substringBefore("@")`, which kept the address off the screen but not out of the document — masking at render time is not a privacy boundary. Notes now carry @@ -439,7 +446,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. -57 tests in [`firestore-tests/rules.test.js`](firestore-tests/rules.test.js) run +61 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. @@ -451,7 +458,7 @@ it down again; nothing touches the real project, and no billing account is invol | `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. | | `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 replace and delete, that a file on another department's note cannot be read — otherwise the boundary is walked around through the file path — and a test that deliberately asserts the *open* create rule, so the gap documented in `firestore.rules` cannot be closed by accident and go unnoticed. | +| `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 and the ban on deleting votes. | | `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. | diff --git a/firestore-tests/rules.test.js b/firestore-tests/rules.test.js index d31fc6b..2abdf5f 100644 --- a/firestore-tests/rules.test.js +++ b/firestore-tests/rules.test.js @@ -556,16 +556,56 @@ describe("note content", () => { await assertFails(deleteDoc(doc(rater, "notes", NOTE, "content", "file"))); }); - test("content creation is open to any signed-in user — a known gap", async () => { - // Documented in firestore.rules: a batch cannot get() its own uncommitted - // parent note, so create cannot verify ownership. This test pins the gap in - // place so that closing it later is a deliberate, visible change. - await assertSucceeds( + /** + * This used to be asserted the other way round, as a gap the rules documented and + * accepted: a batch supposedly could not verify the parent note's owner because + * `get()` does not see an uncommitted document. The `get()` half was right and the + * conclusion wrong — `getAfter()` reads the post-commit state and the rating rule + * was already relying on it. Any signed-in user could hang a content document off + * somebody else's note; overwriting an existing file was blocked, but a note that + * had no file yet would happily take one. + */ + test("someone else cannot attach content to a note they do not own", async () => { + await assertFails( setDoc(doc(rater, "notes", NOTE, "content", "extra"), { fileData: "ZmFrZQ==", }), ); }); + + test("the uploader may attach a file to their own existing note", async () => { + // getAfter() returns the current note when the write does not touch it, so the + // rule covers adding a file later as well as the batched upload. + await assertSucceeds( + setDoc(doc(owner, "notes", NOTE, "content", "extra"), { + fileData: "ZmFrZQ==", + }), + ); + }); + + test("a content document carrying anything but fileData is refused", async () => { + await assertFails( + setDoc(doc(owner, "notes", NOTE, "content", "extra"), { + fileData: "ZmFrZQ==", + uploaderUid: RATER, + }), + ); + await assertFails( + setDoc(doc(owner, "notes", NOTE, "content", "extra"), { fileData: 42 }), + ); + }); + + test("a batch cannot smuggle content onto someone else's note", async () => { + // The batch writes a legitimate note of the caller's own *and* a file on the + // owner's note. Each write is evaluated separately, so the second must fail and + // take the whole commit with it. + const batch = writeBatch(rater); + batch.set(doc(rater, "notes", "smuggle"), noteFields()); + batch.set(doc(rater, "notes", NOTE, "content", "file"), { + fileData: "a290dQ==", + }); + await assertFails(batch.commit()); + }); }); describe("ratings", () => { diff --git a/firestore.rules b/firestore.rules index f1f439d..e7295ca 100644 --- a/firestore.rules +++ b/firestore.rules @@ -257,10 +257,28 @@ service cloud.firestore { // okuyamamalı, aksi halde bölüm sınırı dosya yolundan atlanırdı. allow read: if isSignedIn() && noteDepartment(noteId) == ownDepartment(); - // Not ile içerik aynı batch'te yazılıyor. Batch içindeki her yazma - // ayrı değerlendirilir ve get() henüz commit edilmemiş üst dokümanı - // göremez — bu yüzden create'te sahiplik doğrulanamaz. - allow create: if isSignedIn(); + /** + * İçerik yalnızca notun sahibi tarafından oluşturulabilir. + * + * Burada eskiden `if isSignedIn()` yazıyordu ve yanındaki yorum bunu + * "get() henüz commit edilmemiş üst dokümanı göremez" diye açıklıyordu. + * Doğru olan kısmı `get()` ile ilgiliydi, sonucu değil: [getAfter] aynı + * commit'in *sonrasındaki* hâli okuyor, yani not ile içeriğin birlikte + * yazıldığı batch'te bile üst dokümanın sahibi görülebiliyor. Aynı çağrı + * oy kuralında zaten kullanılıyordu. + * + * Açık hâliyle oturum açmış herkes başkasının notuna içerik dokümanı + * yazabiliyordu: not sahibinin dosyasının üzerine yazmak `update` ile + * engelliyken, dosyası olmayan bir nota istediğini iliştirmek serbestti. + * + * Notu değiştirmeyen bir yazmada `getAfter` mevcut hâli döndürüyor, bu + * yüzden kural kendi eski notuna sonradan dosya eklemeyi de kapsıyor. + */ + allow create: if isSignedIn() + && getAfter(/databases/$(database)/documents/notes/$(noteId)) + .data.uploaderUid == request.auth.uid + && request.resource.data.keys().hasOnly(['fileData']) + && request.resource.data.fileData is string; allow update, delete: if isSignedIn() && get(/databases/$(database)/documents/notes/$(noteId)) From 55da2ec5a4dba60402dc44dd9bf4f2c6acd6504f Mon Sep 17 00:00:00 2001 From: Duygu <84078855+duuyguyiilmaz@users.noreply.github.com> Date: Tue, 1 Sep 2026 19:14:19 +0300 Subject: [PATCH 5/6] fix: delete a note's votes with the note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ratings` is a top-level collection keyed `_`, not a subcollection of the note, so `deleteNote` — which removed the note and its file — never touched the votes cast on it. They stayed forever, invisible to every screen, holding document ids nobody could reuse. The delete rule made it worse by being airtight. `allow delete: if false` was right on its own terms: removing a vote does not lower the note's total, but it does hand the voter a fresh first-time vote, which the rule counts anew and the tally climbs. Free deletion was a way to pad a score. But it also meant no client could ever clean up after itself. `existsAfter()` ties the two together. A vote may be deleted only when the same commit removes its note, and only by that note's owner — which is precisely the write `deleteNote` now issues: query the votes for the note, then delete them, the file and the note in one batch. A vote deletion sent on its own is still refused, because the note is still there afterwards. The batch caps this at Firestore's 500 writes, so a note with more than ~498 votes cannot be deleted this way. Not a number a department feed reaches, and it fails loudly rather than half-deleting. Votes orphaned before this change stay out of reach — the rule calls `get()` on a note that no longer exists, which is an evaluation error and fails closed — so `prune-orphan-ratings.js` covers them, following the same contract as the other two migrations. It compares the two collections in memory rather than issuing a `get()` per vote, and leaves a rating with no `noteId` alone rather than deleting on a guess. Rules suite 61 -> 65, plus the script's own 6: a vote cannot go alone, not even for the owner; the owner may remove it with the note; a stranger cannot reach it by deleting a note they do not own; and an already-orphaned vote stays refused. 85 green in firestore-tests, 126 on the JVM side, lint clean. Deployed and verified on the device: deleting a note from the profile succeeds with no PERMISSION_DENIED. Co-Authored-By: Claude Opus 5 --- README.md | 35 ++++- .../data/repository/FirebaseNoteRepository.kt | 26 ++++ firestore-tests/package.json | 3 +- firestore-tests/prune-orphan-ratings.js | 137 ++++++++++++++++++ firestore-tests/prune-orphan-ratings.test.js | 114 +++++++++++++++ firestore-tests/rules.test.js | 61 +++++++- firestore.rules | 29 +++- 7 files changed, 395 insertions(+), 10 deletions(-) create mode 100644 firestore-tests/prune-orphan-ratings.js create mode 100644 firestore-tests/prune-orphan-ratings.test.js diff --git a/README.md b/README.md index 7796be2..09a964c 100644 --- a/README.md +++ b/README.md @@ -226,6 +226,14 @@ readable only by their owner: all four `getUser` calls in the app pass the calle uid, and the leaderboard takes the uploader's name from the note, so the open rule was granting an access nobody needed. +**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 +the documents piled up unreachable — the rule was a flat `if false`, and rightly so on its +own terms: a deleted vote does not lower the note's total but does hand the voter a fresh +first-time vote, so free deletion was a way to pad a score. `existsAfter()` ties the two +together instead. A vote may be deleted only when the same commit removes its note, and +only by that note's owner, which is exactly what `deleteNote` now writes. + **Attaching a file is owner-only too.** Creating a content document used to be open to any signed-in user, and the rules said why: a batch cannot `get()` its own uncommitted parent note, so ownership could not be checked. The `get()` half was true and @@ -446,7 +454,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. -61 tests in [`firestore-tests/rules.test.js`](firestore-tests/rules.test.js) run +65 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. @@ -459,7 +467,7 @@ it down again; nothing touches the real project, and no billing account is invol | `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. | | `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 and the ban on deleting votes. | +| `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. | | `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 @@ -523,6 +531,27 @@ before and after. against the emulator, including the assertion the change exists for: afterwards no document in the collection has an `uploaderEmail` field at all. +### Clearing orphaned votes + +```bash +cd firestore-tests +npm run prune:orphan-ratings -- --project # reports only +npm run prune:orphan-ratings -- --project --apply # deletes +``` + +Votes cast on notes deleted before the change above are still in the `ratings` +collection, and no client can reach them: the delete rule calls `get()` on the note, and +a note that is gone turns that into an evaluation error, which fails closed. This is the +only thing that can clear them. It reads both collections once and compares in memory +rather than issuing a `get()` per vote, since the collection it is aimed at is one that +has been accumulating. A rating carrying no `noteId` is reported and left alone — +deleting on a guess is worse than saying so. + +[`prune-orphan-ratings.test.js`](firestore-tests/prune-orphan-ratings.test.js) runs it +against the emulator: the orphan goes, the live vote stays, and the note id is read from +the field rather than split out of the document id, which would be ambiguous whenever a +uid contains an underscore. + ### Continuous integration [`.github/workflows/android.yml`](.github/workflows/android.yml) runs on every push to @@ -564,7 +593,7 @@ app/src/test/java/duygu/yilmaz/campusnote/ diff tests, and FeedScreenTest firestore-tests/ emulator-backed tests for firestore.rules, - plus two data migrations and their own tests + plus three data migrations and their own tests ``` --- diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseNoteRepository.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseNoteRepository.kt index ac98e00..b78a18f 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseNoteRepository.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseNoteRepository.kt @@ -135,10 +135,34 @@ class FirebaseNoteRepository( } /** Notu ve içerik dokümanını birlikte siler — Firestore alt koleksiyonları kendiliğinden silmez. */ + /** + * Notu, dosyasını ve ona verilmiş oyları tek commit'te siler. + * + * Oylar eskiden kalıyordu: `ratings` ayrı bir üst düzey koleksiyon, notun alt + * koleksiyonu değil, dolayısıyla notu silmek onlara dokunmuyordu. Geride kalan + * doküman kimseye görünmüyor ama sonsuza kadar duruyor ve `_` + * kimliğini işgal ediyor. + * + * Aynı batch'te olmaları şart, süslemeden değil: güvenlik kuralı bir oyun + * silinmesine ancak notu da aynı commit'te siliniyorsa izin veriyor + * (`existsAfter`). Ayrı ayrı gönderilseler ikisi de reddedilirdi — ve reddin + * kendisi doğru, çünkü tek başına silinen bir oy, oy verenin ikinci kez + * oylayıp sayıyı şişirmesine kapı açardı. + * + * Sınır: Firestore bir batch'te en çok 500 yazma kabul ediyor, yani ~498'den + * fazla oy almış bir not bu yolla silinemez. Bölüm ölçeğinde ulaşılacak bir + * sayı değil; aşılırsa silme hata verir, sessizce yarım kalmaz. + */ override suspend fun deleteNote(noteId: String) { val noteReference = firestore.collection(NOTES_COLLECTION).document(noteId) + val ratings = firestore.collection(RATINGS_COLLECTION) + .whereEqualTo(NOTE_ID_FIELD, noteId) + .get() + .await() + firestore.runBatch { batch -> + ratings.documents.forEach { batch.delete(it.reference) } batch.delete(noteReference.fileDocument()) batch.delete(noteReference) }.await() @@ -275,6 +299,8 @@ class FirebaseNoteRepository( private companion object { const val NOTES_COLLECTION = "notes" + const val RATINGS_COLLECTION = "ratings" + const val NOTE_ID_FIELD = "noteId" const val CONTENT_COLLECTION = "content" const val FILE_DOCUMENT = "file" const val FILE_DATA_FIELD = "fileData" diff --git a/firestore-tests/package.json b/firestore-tests/package.json index 4ac620f..57d6a88 100644 --- a/firestore-tests/package.json +++ b/firestore-tests/package.json @@ -7,7 +7,8 @@ "scripts": { "test": "firebase emulators:exec --config ../firebase.json --only firestore --project campusnote-rules-test \"vitest run\"", "backfill:created-at": "node backfill-created-at.js", - "strip:uploader-email": "node strip-uploader-email.js" + "strip:uploader-email": "node strip-uploader-email.js", + "prune:orphan-ratings": "node prune-orphan-ratings.js" }, "devDependencies": { "@firebase/rules-unit-testing": "^4.0.1", diff --git a/firestore-tests/prune-orphan-ratings.js b/firestore-tests/prune-orphan-ratings.js new file mode 100644 index 0000000..b679908 --- /dev/null +++ b/firestore-tests/prune-orphan-ratings.js @@ -0,0 +1,137 @@ +/** + * Deletes rating documents whose note no longer exists. + * + * Deliberately has no `#!` line, for the same reason as the other two scripts: the + * repo normalises to CRLF on Windows checkouts and esbuild does not strip a shebang + * ending in `\r`, which broke the test that imports the file — on Windows only. + * + * Why this exists: `ratings` is a top-level collection keyed `_`, not a + * subcollection of the note, so deleting a note never touched the votes cast on it. + * `deleteNote` now removes them in the same commit and the rules only permit a vote + * to be deleted alongside its note, so no new orphans appear — but the ones already + * written cannot be reached by any client: the rule calls `get()` on the note, and a + * note that is gone makes that an evaluation error, which fails closed. + * + * The script reports by default and only writes when told to: + * + * node prune-orphan-ratings.js --project # dry run, changes nothing + * node prune-orphan-ratings.js --project --apply # deletes + * + * Against production it needs application-default credentials: + * + * gcloud auth application-default login + * + * Against the emulator, set FIRESTORE_EMULATOR_HOST instead; no credentials are used. + */ + +import { resolve } from "node:path"; +import { fileURLToPath } from "node:url"; +import { initializeApp } from "firebase-admin/app"; +import { Firestore } from "firebase-admin/firestore"; + +const NOTES = "notes"; +const RATINGS = "ratings"; +const NOTE_ID_FIELD = "noteId"; +// Firestore accepts at most 500 writes per batch. +const BATCH_LIMIT = 450; + +/** + * @returns the note id a rating claims, or null when the document cannot say. + * + * The field is the source of truth rather than the document id: the id is + * `_` and a uid may itself contain an underscore, so splitting it is + * ambiguous in exactly the case that matters. + */ +export function noteIdOf(data) { + const noteId = data?.[NOTE_ID_FIELD]; + return typeof noteId === "string" && noteId.length > 0 ? noteId : null; +} + +export async function prune(firestore, { apply = false, log = console.log } = {}) { + const [ratings, notes] = await Promise.all([ + firestore.collection(RATINGS).get(), + firestore.collection(NOTES).get(), + ]); + + // One pass over the notes rather than a get() per rating: the whole point is to + // run cheaply over a collection that has been accumulating. + const liveNotes = new Set(notes.docs.map((doc) => doc.id)); + + const counts = { live: 0, orphan: 0, unreadable: 0 }; + const pending = []; + + for (const doc of ratings.docs) { + const noteId = noteIdOf(doc.data()); + + if (noteId === null) { + // No note id to check against. Left alone: deleting on a guess is worse than + // reporting it and letting someone look. + counts.unreadable += 1; + continue; + } + + if (liveNotes.has(noteId)) { + counts.live += 1; + } else { + counts.orphan += 1; + pending.push(doc.ref); + } + } + + log(`ratings scanned: ${ratings.size}`); + log(` note still exists: ${counts.live}`); + log(` orphaned: ${counts.orphan}`); + log(` no noteId field: ${counts.unreadable}`); + + if (counts.unreadable > 0) { + log(` ${counts.unreadable} rating(s) carry no noteId and are left untouched.`); + } + + if (!apply) { + log("dry run — nothing was deleted. Re-run with --apply to delete."); + return { scanned: ratings.size, counts, deleted: 0 }; + } + + let deleted = 0; + for (let i = 0; i < pending.length; i += BATCH_LIMIT) { + const batch = firestore.batch(); + for (const ref of pending.slice(i, i + BATCH_LIMIT)) { + batch.delete(ref); + } + await batch.commit(); + deleted += Math.min(BATCH_LIMIT, pending.length - i); + log(`deleted: ${deleted}/${pending.length}`); + } + + return { scanned: ratings.size, counts, deleted }; +} + +/** Only runs when the file is executed directly, so the tests can import it. */ +const runDirectly = + process.argv[1] && resolve(process.argv[1]) === fileURLToPath(import.meta.url); + +if (runDirectly) { + const args = process.argv.slice(2); + const apply = args.includes("--apply"); + const projectIndex = args.indexOf("--project"); + const projectId = + projectIndex >= 0 ? args[projectIndex + 1] : process.env.GOOGLE_CLOUD_PROJECT; + + if (!projectId) { + console.error("usage: node prune-orphan-ratings.js --project [--apply]"); + process.exit(2); + } + + initializeApp({ projectId }); + const firestore = new Firestore({ projectId }); + + prune(firestore, { apply }) + .then((result) => { + console.log(apply ? `done — ${result.deleted} rating(s) deleted.` : "done."); + process.exit(0); + }) + .catch((error) => { + console.error(error); + process.exit(1); + }); +} diff --git a/firestore-tests/prune-orphan-ratings.test.js b/firestore-tests/prune-orphan-ratings.test.js new file mode 100644 index 0000000..22dbbf0 --- /dev/null +++ b/firestore-tests/prune-orphan-ratings.test.js @@ -0,0 +1,114 @@ +import { initializeApp, deleteApp } from "firebase-admin/app"; +import { Firestore } from "firebase-admin/firestore"; +import { afterAll, beforeAll, beforeEach, describe, expect, test } from "vitest"; +import { noteIdOf, prune } from "./prune-orphan-ratings.js"; + +// The emulator is already running (npm test wraps the suite in emulators:exec), and +// the admin SDK talks to it as long as this is set before it connects. +process.env.FIRESTORE_EMULATOR_HOST ??= "127.0.0.1:8080"; + +const PROJECT = "campusnote-rules-test"; + +let app; +let firestore; + +/** Swallows the script's progress output so the test run stays readable. */ +const quiet = () => {}; + +beforeAll(() => { + app = initializeApp({ projectId: PROJECT }, "prune-ratings-test"); + firestore = new Firestore({ projectId: PROJECT }); +}); + +afterAll(async () => { + await deleteApp(app); +}); + +beforeEach(async () => { + for (const name of ["notes", "ratings"]) { + const existing = await firestore.collection(name).get(); + await Promise.all(existing.docs.map((doc) => doc.ref.delete())); + } +}); + +const seedNote = (id) => + firestore.collection("notes").doc(id).set({ + title: "Ders notu", + department: "Bilgisayar Mühendisliği", + uploaderUid: "uid-1", + }); + +const seedRating = (uid, noteId, extra = {}) => + firestore + .collection("ratings") + .doc(`${uid}_${noteId}`) + .set({ uid, noteId, rating: 4, ...extra }); + +describe("reading a rating's note id", () => { + test("the field is used, not the document id", () => { + // The id is `_` and a uid may itself contain an underscore, so + // splitting it is ambiguous in exactly the case that matters. + expect(noteIdOf({ uid: "a_b", noteId: "note-1", rating: 4 })).toBe("note-1"); + }); + + test("a rating with no usable noteId reports none", () => { + expect(noteIdOf({ uid: "a", rating: 4 })).toBeNull(); + expect(noteIdOf({ uid: "a", noteId: "", rating: 4 })).toBeNull(); + expect(noteIdOf({ uid: "a", noteId: 7, rating: 4 })).toBeNull(); + }); +}); + +describe("running against Firestore", () => { + test("a dry run reports without deleting", async () => { + await seedRating("uid-2", "gone"); + + const result = await prune(firestore, { log: quiet }); + + expect(result.counts.orphan).toBe(1); + expect(result.deleted).toBe(0); + const after = await firestore.collection("ratings").get(); + expect(after.size).toBe(1); + }); + + /** + * The assertion the script exists for: a vote whose note was deleted before the + * repository started removing them is unreachable by any client — the rule calls + * `get()` on a note that is gone — so this is the only thing that can clear it. + */ + test("applying deletes the orphan and keeps the live vote", async () => { + await seedNote("note-1"); + await seedRating("uid-2", "note-1"); + await seedRating("uid-3", "gone"); + + const result = await prune(firestore, { apply: true, log: quiet }); + + expect(result.counts).toMatchObject({ live: 1, orphan: 1 }); + expect(result.deleted).toBe(1); + + const remaining = await firestore.collection("ratings").get(); + expect(remaining.docs.map((doc) => doc.id)).toEqual(["uid-2_note-1"]); + }); + + test("a rating with no noteId is reported and left alone", async () => { + await firestore.collection("ratings").doc("broken").set({ uid: "uid-2", rating: 4 }); + + const result = await prune(firestore, { apply: true, log: quiet }); + + expect(result.counts.unreadable).toBe(1); + expect(result.deleted).toBe(0); + const after = await firestore.collection("ratings").doc("broken").get(); + expect(after.exists).toBe(true); + }); + + test("re-running finds nothing left to do", async () => { + await seedNote("note-1"); + await seedRating("uid-2", "note-1"); + await seedRating("uid-3", "gone"); + + await prune(firestore, { apply: true, log: quiet }); + const second = await prune(firestore, { apply: true, log: quiet }); + + expect(second.counts.orphan).toBe(0); + expect(second.deleted).toBe(0); + }); +}); diff --git a/firestore-tests/rules.test.js b/firestore-tests/rules.test.js index 2abdf5f..1b42c20 100644 --- a/firestore-tests/rules.test.js +++ b/firestore-tests/rules.test.js @@ -661,15 +661,66 @@ describe("ratings", () => { } }); - test("votes cannot be deleted", 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", `${RATER}_${NOTE}`), - validVote, - ); + 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 + * afresh and the tally would climb. Free deletion was the short path to a padded + * score, which is why the rule used to be a flat `if false`. + */ + test("a vote cannot be deleted on its own", async () => { + await seedVote(); await assertFails(deleteDoc(doc(rater, "ratings", `${RATER}_${NOTE}`))); }); + + test("the note's owner cannot delete a vote while the note stays", async () => { + await seedVote(); + await assertFails(deleteDoc(doc(owner, "ratings", `${RATER}_${NOTE}`))); + }); + + /** + * The pairing the repository now performs. `ratings` is a top-level collection + * rather than a subcollection of the note, so deleting a note never touched its + * votes and the documents accumulated with nothing able to reach them. + */ + test("the owner may delete a vote in the same commit as its note", async () => { + await seedVote(); + + const batch = writeBatch(owner); + batch.delete(doc(owner, "ratings", `${RATER}_${NOTE}`)); + 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(); + + const batch = writeBatch(rater); + batch.delete(doc(rater, "ratings", `${RATER}_${NOTE}`)); + batch.delete(doc(rater, "notes", NOTE)); + await assertFails(batch.commit()); + }); + + /** + * The gap the rule deliberately leaves: once a note is gone, `get()` on it fails + * and the delete is refused. Votes orphaned before this change are cleaned up by + * `prune-orphan-ratings.js`, not by a client. + */ + test("a vote whose note is already gone cannot be deleted by a client", async () => { + await seedVote(); + await testEnv.withSecurityRulesDisabled(async (context) => { + await deleteDoc(doc(context.firestore(), "notes", NOTE)); + }); + + await assertFails(deleteDoc(doc(owner, "ratings", `${RATER}_${NOTE}`))); + }); }); describe("unmatched paths", () => { diff --git a/firestore.rules b/firestore.rules index e7295ca..570656d 100644 --- a/firestore.rules +++ b/firestore.rules @@ -303,7 +303,34 @@ service cloud.firestore { && get(/databases/$(database)/documents/notes/$(request.resource.data.noteId)) .data.uploaderUid != request.auth.uid; - allow delete: if false; + /** + * Bir oy yalnızca ait olduğu notla birlikte silinebilir. + * + * Kural eskiden `if false` idi ve tek başına doğruydu: silinen bir oy notun + * toplamını düşürmüyor, ama oy verene ikinci kez oy verme hakkı kazandırıyor + * — kural onu ilk oy sayıp sayıyı bir daha artırıyor. Yani serbest silme, + * puan şişirmenin kısa yoluydu. + * + * Kapalı hâlin bedeli sahipsiz dokümanlardı: `ratings` notun alt koleksiyonu + * değil ayrı bir üst düzey koleksiyon, dolayısıyla not silinince oyları + * kalıyor ve hiçbir istemci onları temizleyemiyordu. + * + * `existsAfter` ikisini birbirine bağlıyor: silme ancak commit'ten sonra not + * ortada yoksa geçiyor. Tek başına gönderilen bir oy silme hâlâ reddediliyor, + * çünkü not yerinde duruyor. `get()` batch öncesi hâli okuduğu için notun + * sahibi de aynı commit içinde doğrulanabiliyor — oyu silebilecek tek kişi + * notu silebilecek kişiyle aynı. + * + * Notu zaten silinmiş eski oylar bu yolla temizlenemez: `get()` olmayan notta + * değerlendirme hatasına düşer, yani kapalı kalır. Onlar için + * `firestore-tests/prune-orphan-ratings.js` var. + */ + allow delete: if isSignedIn() + && !existsAfter( + /databases/$(database)/documents/notes/$(resource.data.noteId) + ) + && get(/databases/$(database)/documents/notes/$(resource.data.noteId)) + .data.uploaderUid == request.auth.uid; } // Yukarıda eşleşmeyen her yol reddedilir. Yeni bir koleksiyon eklendiğinde From b048d53401583ac84ea2e4e11cbab1ddc7909227 Mon Sep 17 00:00:00 2001 From: Duygu <84078855+duuyguyiilmaz@users.noreply.github.com> Date: Tue, 1 Sep 2026 19:20:32 +0300 Subject: [PATCH 6/6] docs: correct the claims the code does not support, drop what nothing uses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six things a reviewer flagged, five of which were the documentation describing a project that no longer exists. **`app/README.md` is deleted.** It was the original course submission: a report mechanism that was never built, a `points` field on users and an `avgRating` on notes that have both since been removed, and a `Reports` collection that never existed in Firestore. It contradicted the root README on nearly every point. Two READMEs where one is stale is worse than one, and nothing linked to it. **The architecture claim was overstated.** "Firebase types stay behind the repositories and never reach the `ui` package" is not true twice over: every ViewModel defaults its constructor to a concrete `FirebaseXRepository`, and `AuthErrorMessages` matches on `FirebaseAuthException.errorCode` — deliberately, since that code is the only stable description of an auth failure. The separation is real and worth having; it is just not absolute, and the README now says which two places cross the line and why, rather than claiming a purity the code does not have. **KSP is removed.** The plugin was applied in both build files and pinned in the version catalog, and no `ksp(...)` dependency existed anywhere — an annotation processor configured to process nothing, costing a plugin resolution on every build and implying a code-generation step a reader would go looking for. **Three limitations are now written down** rather than left for a reviewer to discover: nothing runs on a device in CI, no test executes a real Firestore query (the fakes stop at the repository interfaces — the rules suite is the other half of that gap), and a green rules suite says the file is correct, not that the Firebase project is running it. The last one is the sharpest: the repo cannot detect the drift, so deploying is part of merging a rules change. **`.claude/` is ignored.** Per-machine tool permissions, not project config. Not changed: the point that repository tests use fakes was already documented under Tests, and it was accurate. Build, 126 JVM tests and lint all green without the KSP plugin. Co-Authored-By: Claude Opus 5 --- .gitignore | 3 ++ README.md | 40 +++++++++++++++++-- app/README.md | 82 --------------------------------------- app/build.gradle.kts | 1 - build.gradle.kts | 1 - gradle/libs.versions.toml | 2 - 6 files changed, 40 insertions(+), 89 deletions(-) delete mode 100644 app/README.md diff --git a/.gitignore b/.gitignore index efd401c..4188822 100644 --- a/.gitignore +++ b/.gitignore @@ -26,3 +26,6 @@ ui-debug.log # Firebase client config — every developer supplies their own. # The committed template is app/google-services.json.example. /app/google-services.json + +# Local Claude Code settings — per-machine tool permissions, not project config. +/.claude diff --git a/README.md b/README.md index 09a964c..efa6ff3 100644 --- a/README.md +++ b/README.md @@ -59,9 +59,21 @@ the name, derived by ## Architecture -The app follows MVVM with a repository layer. Firebase types stay behind the -repositories and never reach the `ui` package, and every Firebase call is a `suspend` -function bridged with `.await()`, so none of them block the main thread. +The app follows MVVM with a repository layer. Every Firebase call is a `suspend` +function bridged with `.await()`, so none of them block the main thread, and Firebase +data types — `DocumentSnapshot`, `Query`, `Timestamp` — stop at the repositories; the +`ui` package only ever sees the plain data classes in `data/model`. + +Two places do reach across that line, and calling the separation absolute would be +overstating it. Each ViewModel defaults its constructor to the Firebase-backed +implementation (`AuthRepository = FirebaseAuthRepository()`), which is what lets the +Activity create it with no wiring — the interface is still what the body depends on, and +what every test substitutes, but the default names a concrete Firebase class. And +[`AuthErrorMessages`](app/src/main/java/duygu/yilmaz/campusnote/ui/auth/AuthErrorMessages.kt) +matches on `FirebaseAuthException.errorCode` to pick an error message, because that code +is the only stable description of what went wrong; the alternative, translating every +failure into an app-specific type at the repository, is more indirection than one screen's +error text is worth. Both are deliberate; neither is invisible. ```mermaid flowchart TD @@ -288,6 +300,28 @@ They are covered by their own test suite; see [Rules tests](#rules-tests). Honest list of things a reviewer would spot, and why they are the way they are. +**Nothing runs on a device in CI.** Every test here is JVM-only — Robolectric gives the +screen tests a real fragment and real view inflation, but no emulator, no `androidTest` +source set, no instrumentation run. So the things only a device settles — the file picker +returning a URI the app cannot decode, an OEM keyboard, a configuration change mid-upload +— are covered by running the app by hand and nothing else. The trade was deliberate: +`testDebugUnitTest` finishes in under a minute and runs on every push, and an emulator job +that takes ten would have been switched off. + +**No test executes a real Firestore query.** The ViewModel tests substitute fakes of the +repository *interfaces*, so the `whereEqualTo` / `orderBy` / `limit` calls that make up +the actual queries never run — see [Tests](#tests). A test can prove the leaderboard asks +for the right department; it cannot prove the query built from it filters on one. The +security rules are the other half of that gap and they *are* tested, against the emulator, +which is where a wrong filter would surface as a denial. + +**A green rules suite is not a deployed rule.** `firestore.rules` is tested against the +emulator on every push, and that says the file is correct — not that the Firebase project +is running it. The two drift the moment someone edits the rules in the Console, or forgets +`firebase deploy --only firestore:rules` after a change here. Nothing in this repo can +detect that; treat the deploy step as part of merging a rules change, not as an +afterthought. + **Rating totals are still computed on the client.** A Cloud Function would compute them server-side, and that needs the Blaze plan. The rules make up most of the difference — they recompute the expected totals from the vote and reject anything else (see diff --git a/app/README.md b/app/README.md deleted file mode 100644 index e26b9cb..0000000 --- a/app/README.md +++ /dev/null @@ -1,82 +0,0 @@ -CampusNote -CampusNote is a mobile application designed to support academic collaboration among university students. -The application encourages students to share academic notes within their departments and access notes uploaded by other students. - -The system is based on contribution fairness: students must upload at least one academic note in order to access other shared notes. - -The main purpose of the application is to promote: - -Academic collaboration -Structured note sharing -Quality-based ranking within departments -Target Users -Akdeniz University undergraduate students. - -The application is designed specifically for university students who want to share and access course notes within their department. - -Core Features (MVP) -University e-mail login -Department selection -Notes feed with department-based filtering -Note upload (PDF or image format) -Note rating system -Leaderboard / points system -Report mechanism for inappropriate or empty uploads -Database & Network Operations -As required by the course guidelines, the project includes database and network connection operations. - -The application uses Firebase services to manage authentication and real-time database operations. - -Database Structure (Cloud Firestore) -The application stores data using Firebase Cloud Firestore, a NoSQL cloud database. - -Users -id (Firebase UID) -email -department -points -Notes -id -title -description -uploaderUid -uploaderEmail -department -createdAt -avgRating -Ratings -id -noteId -userId -value -Reports -id -noteId -userId -reason -createdAt -Network Operations -The application performs several network-based operations including: - -User authentication -Uploading notes -Fetching notes from the database -Submitting ratings -Reporting inappropriate content -Real-time note listing using Firestore listeners -Technical Stack -Mobile Platform -Kotlin (Android) - -Authentication -Firebase Authentication - -Database -Firebase Cloud Firestore - -File Storage -Cloud-based storage for uploaded notes (planned feature) - -Purpose of the Project -CampusNote was developed as part of a Mobile Programming course project. -The project demonstrates the implementation of mobile UI design, database integration, and network communication using modern Android development tools. \ No newline at end of file diff --git a/app/build.gradle.kts b/app/build.gradle.kts index c7994ab..b7e2627 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -1,7 +1,6 @@ plugins { alias(libs.plugins.android.application) alias(libs.plugins.kotlin.android) - alias(libs.plugins.ksp) alias(libs.plugins.google.services) } diff --git a/build.gradle.kts b/build.gradle.kts index 3b9ceb2..aca0aa1 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -1,6 +1,5 @@ plugins { alias(libs.plugins.android.application) apply false alias(libs.plugins.kotlin.android) apply false - alias(libs.plugins.ksp) apply false alias(libs.plugins.google.services) apply false } \ No newline at end of file diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 68b4f79..cb49817 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -1,6 +1,5 @@ [versions] agp = "8.13.2" -ksp = "2.0.21-1.0.27" kotlin = "2.0.21" googleServices = "4.4.2" @@ -57,5 +56,4 @@ androidx-fragment-testing-manifest = { group = "androidx.fragment", name = "frag [plugins] android-application = { id = "com.android.application", version.ref = "agp" } kotlin-android = { id = "org.jetbrains.kotlin.android", version.ref = "kotlin" } -ksp = { id = "com.google.devtools.ksp", version.ref = "ksp" } google-services = { id = "com.google.gms.google-services", version.ref = "googleServices" }