diff --git a/README.md b/README.md index 51c9c44..3d0b03c 100644 --- a/README.md +++ b/README.md @@ -98,6 +98,11 @@ flowchart TD by a `limit(1)` query for the user's own notes rather than a `hasUploadedNote` flag on the profile. A stored flag drifted: it was set on upload and never cleared on delete, so uploading once and deleting granted permanent access. +- **Points are derived too, and that is what made them safe.** A user's point total is + the sum of `ratingSum` over their own notes, computed where it is displayed. It used to + live in `users.points`, written by whoever cast the vote — which forced the rules to + let one user write another user's document, and left the owner able to write their own. + Deleting the field deleted both holes; nothing read it anyway. - **Rating uses a transaction.** `runTransaction` re-reads the note inside the transaction, so two people rating the same note at once cannot lose one of the votes. - **File content lives in a subcollection.** `notes/{id}/content/file` is separate from @@ -148,11 +153,11 @@ Cloud Firestore, four collections: | `id` | string | matches the Firebase Auth UID | | `email` | string | | | `department` | string | | -| `points` | number | earned from ratings on your notes | | `createdAt` | number | | -Older user documents may still carry a `hasUploadedNote` boolean. It is no longer read -or written — the contribution gate is derived from the user's notes instead. +Older user documents may still carry `hasUploadedNote` and `points`. Neither is read or +written any more: the contribution gate is derived from the user's notes, and so is the +point total. Both were stored once and both drifted from the truth — see Key decisions. **`notes/{noteId}`** @@ -182,8 +187,22 @@ firebase deploy --only firestore:rules ``` The rules require authentication everywhere, restrict note edits and deletes to the -uploader, and pin each rating document to its author's UID. Note that the delete rule -is the *only* ownership check on deletion — the app code does not verify it. +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. + +They also verify the scoring rather than trusting it. A vote writes the note's totals and +the rating document in one atomic commit, and the rule for the note uses `getAfter()` to +read the rating being written in that same commit, recompute the expected `ratingSum` and +`ratingCount` from it, and reject anything that does not match. So the totals cannot move +without a real vote behind them, a changed vote cannot be counted twice, and rating your +own note is refused by the server rather than only by the client. + +The 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. They are covered by their own test suite; see [Rules tests](#rules-tests). @@ -193,12 +212,10 @@ 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. -**The points system is client-authoritative.** -Rating a note updates the note's totals *and* the uploader's `points` directly from the -client, so the security rules have to permit one user to modify another user's points. -Values are constrained by type and range, but a malicious client could still award -itself points. The correct fix is to move the calculation into a Cloud Function so the -client only writes its vote — which again needs the Blaze plan. +**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 +[Security rules](#security-rules)) — but the arithmetic itself still runs on a device. **Email addresses are never verified.** Registration only checks that the address ends in `@ogr.akdeniz.edu.tr`; no confirmation mail is sent, so anyone can register with a @@ -339,7 +356,7 @@ enforces — who may delete a note, who may write to whose `points` — was prev unverified. That is the layer an attacker actually meets: the Android client can be replaced, the rules cannot. -32 tests in [`firestore-tests/rules.test.js`](firestore-tests/rules.test.js) run +38 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. @@ -347,8 +364,8 @@ 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 the narrow exception that lets a rater raise *another* user's `points` — including that it cannot carry a second field along, go negative, or be a non-integer. | -| `notes` | `uploaderUid` cannot be forged on create; metadata edits are the uploader's alone; a rater may touch only `ratingSum`, `ratingCount` and `avgRating`; delete is owner-only. | +| `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. | | `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/RatingCalculation.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingCalculation.kt index a084f26..cf6b8b6 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 @@ -5,8 +5,6 @@ data class RatingTotals( val sum: Long, val count: Long, val average: Double, - /** Not sahibinin puanına eklenecek fark; oy düşürüldüyse negatif olur. */ - val pointsDelta: Long, /** Kullanıcı ilk kez mi oy verdi, yoksa eski oyunu mu değiştirdi. */ val updatedExistingRating: Boolean ) @@ -61,7 +59,6 @@ object RatingCalculator { sum = sum, count = count, average = average, - pointsDelta = delta, updatedExistingRating = updatedExistingRating ) } diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/model/UserProfile.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/model/UserProfile.kt index e8754b2..84d0a5d 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/model/UserProfile.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/model/UserProfile.kt @@ -4,7 +4,6 @@ data class UserProfile( val id: String = "", val email: String = "", val department: String = "", - val points: Long = 0L, val createdAt: Long = 0L ) { companion object { 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 bcf216e..9282ce0 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 @@ -9,6 +9,17 @@ class FirebaseRatingRepository( private val firestore: FirebaseFirestore = FirebaseFirestore.getInstance() ) : RatingRepository { + /** + * Oy, notun toplamları ve oy dokümanı aynı transaction'da yazılıyor — güvenlik + * kurallarının ikisini birbirine bağlayabilmesi buna dayanıyor. Kural, notun yeni + * `ratingSum`'ını aynı commit'te yazılan oy dokümanından hesaplıyor, yani ikisi + * ayrılırsa yazma reddedilir. + * + * Not sahibinin puanı burada güncellenmiyor. Puan artık `users` dokümanında + * saklanmıyor; profil ekranı kullanıcının kendi notlarının `ratingSum` toplamından + * türetiyor. Saklandığı sürece kurallar bir kullanıcının başka bir kullanıcının + * dokümanına yazmasına izin vermek zorundaydı ve o izin kapatılamıyordu. + */ override suspend fun submitRating( noteId: String, raterUid: String, @@ -30,12 +41,6 @@ class FirebaseRatingRepository( if (ownerUid == raterUid) throw OwnNoteRatingException() val ratingSnapshot = transaction.get(ratingReference) - val ownerReference = ownerUid - .takeIf { it.isNotBlank() } - ?.let { firestore.collection(USERS_COLLECTION).document(it) } - val ownerSnapshot = ownerReference?.let { reference -> - transaction.get(reference) - } val totals = RatingCalculator.recalculate( currentSum = noteSnapshot.getLong(RATING_SUM_FIELD) ?: 0L, @@ -65,12 +70,6 @@ class FirebaseRatingRepository( ) ) - if (ownerReference != null && ownerSnapshot?.exists() == true) { - val currentPoints = ownerSnapshot.getLong(POINTS_FIELD) ?: 0L - val newPoints = (currentPoints + totals.pointsDelta).coerceAtLeast(0L) - transaction.update(ownerReference, POINTS_FIELD, newPoints) - } - RatingResult( average = totals.average, count = totals.count, @@ -83,7 +82,6 @@ class FirebaseRatingRepository( private companion object { const val NOTES_COLLECTION = "notes" const val RATINGS_COLLECTION = "ratings" - const val USERS_COLLECTION = "users" const val UPLOADER_UID_FIELD = "uploaderUid" const val RATING_SUM_FIELD = "ratingSum" const val RATING_COUNT_FIELD = "ratingCount" @@ -91,6 +89,5 @@ class FirebaseRatingRepository( const val RATING_FIELD = "rating" const val USER_ID_FIELD = "uid" const val NOTE_ID_FIELD = "noteId" - const val POINTS_FIELD = "points" } } diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseUserRepository.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseUserRepository.kt index d24ca8e..3f350ea 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseUserRepository.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseUserRepository.kt @@ -27,7 +27,6 @@ class FirebaseUserRepository( id = document.getString("id") ?: document.id, email = document.getString("email") ?: "", department = document.getString("department") ?: "", - points = document.getLong("points") ?: 0L, createdAt = document.getLong("createdAt") ?: 0L ) } 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 fbd000e..e114acb 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 @@ -28,7 +28,6 @@ class RatingCalculatorTest { assertEquals(4L, totals.sum) assertEquals(1L, totals.count) assertEquals(4.0, totals.average, DELTA) - assertEquals(4L, totals.pointsDelta) assertFalse(totals.updatedExistingRating) } @@ -44,7 +43,6 @@ class RatingCalculatorTest { assertEquals(12L, totals.sum) assertEquals(3L, totals.count) assertEquals(4.0, totals.average, DELTA) - assertEquals(3L, totals.pointsDelta) } @Test @@ -59,7 +57,6 @@ class RatingCalculatorTest { assertEquals(10L, totals.sum) assertEquals(2L, totals.count) assertEquals(5.0, totals.average, DELTA) - assertEquals(1L, totals.pointsDelta) assertTrue(totals.updatedExistingRating) } @@ -74,7 +71,6 @@ class RatingCalculatorTest { assertEquals(6L, totals.sum) assertEquals(2L, totals.count) - assertEquals(-4L, totals.pointsDelta) } @Test @@ -88,7 +84,6 @@ class RatingCalculatorTest { assertEquals(7L, totals.sum) assertEquals(2L, totals.count) - assertEquals(0L, totals.pointsDelta) } @Test 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 82726e4..ea488e5 100644 --- a/app/src/test/java/duygu/yilmaz/campusnote/testing/Fixtures.kt +++ b/app/src/test/java/duygu/yilmaz/campusnote/testing/Fixtures.kt @@ -18,9 +18,8 @@ fun authenticatedUser( fun userProfile( uid: String = "uid-1", email: String = "ogrenci@ogr.akdeniz.edu.tr", - department: String = "Bilgisayar Mühendisliği", - points: Long = 0L -) = UserProfile(id = uid, email = email, department = department, points = points) + department: String = "Bilgisayar Mühendisliği" +) = UserProfile(id = uid, email = email, department = department) fun post( id: String = "note-1", diff --git a/app/src/test/java/duygu/yilmaz/campusnote/ui/auth/RegisterViewModelTest.kt b/app/src/test/java/duygu/yilmaz/campusnote/ui/auth/RegisterViewModelTest.kt index c11e547..9e7ecc2 100644 --- a/app/src/test/java/duygu/yilmaz/campusnote/ui/auth/RegisterViewModelTest.kt +++ b/app/src/test/java/duygu/yilmaz/campusnote/ui/auth/RegisterViewModelTest.kt @@ -51,7 +51,6 @@ class RegisterViewModelTest { val saved = userRepository.savedUsers.single() assertEquals("duygu@ogr.akdeniz.edu.tr", saved.email) assertEquals("Bilgisayar Mühendisliği", saved.department) - assertEquals(0L, saved.points) } @Test diff --git a/firestore-tests/rules.test.js b/firestore-tests/rules.test.js index 0639295..659636a 100644 --- a/firestore-tests/rules.test.js +++ b/firestore-tests/rules.test.js @@ -55,14 +55,12 @@ beforeEach(async () => { id: OWNER, email: "owner@ogr.akdeniz.edu.tr", department: "Bilgisayar Mühendisliği", - points: 10, createdAt: 1, }); await setDoc(doc(db, "users", RATER), { id: RATER, email: "rater@ogr.akdeniz.edu.tr", department: "Bilgisayar Mühendisliği", - points: 0, createdAt: 1, }); await setDoc(doc(db, "notes", NOTE), { @@ -84,6 +82,31 @@ beforeEach(async () => { }); }); +/** + * 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 + * dokümanından doğruluyor, o yüzden ikisini ayırmak testin anlamını kaybettirir. + * + * @param db oy veren bağlam + * @param uid oy dokümanının sahibi olarak yazılacak uid — kurala göre çağıranınki olmalı + * @param totals notun yazılacak yeni değerleri; `extra` ile fazladan alan sızdırılır + */ +function vote(db, uid, noteId, rating, { sum, count, extra = {} }) { + const batch = writeBatch(db); + batch.update(doc(db, "notes", noteId), { + ratingSum: sum, + ratingCount: count, + avgRating: count === 0 ? 0 : sum / count, + ...extra, + }); + batch.set(doc(db, "ratings", `${uid}_${noteId}`), { + uid, + noteId, + rating, + }); + return batch.commit(); +} + describe("signed-out access", () => { // Every collection matches on isSignedIn() first, so this is the one guard // that has to hold everywhere: an unauthenticated client reads nothing. @@ -114,7 +137,6 @@ describe("users", () => { id: RATER, email: "rater@ogr.akdeniz.edu.tr", department: "Bilgisayar Mühendisliği", - points: 0, createdAt: 1, }), ); @@ -122,44 +144,36 @@ describe("users", () => { test("a user cannot create a profile under someone else's uid", async () => { await testEnv.clearFirestore(); - await assertFails( - setDoc(doc(rater, "users", OWNER), { id: OWNER, points: 0 }), - ); + await assertFails(setDoc(doc(rater, "users", OWNER), { id: OWNER })); }); test("a create whose id field disagrees with the document id is rejected", async () => { await testEnv.clearFirestore(); - await assertFails( - setDoc(doc(rater, "users", RATER), { id: OWNER, points: 0 }), - ); + await assertFails(setDoc(doc(rater, "users", RATER), { id: OWNER })); }); - test("a user may edit their own profile freely", async () => { - await assertSucceeds( + test("nobody may update a profile, not even its owner", async () => { + // The app only ever creates this document. Leaving update open is what forced + // the old rule to permit writing `points` — the field that made self-awarding + // a one-line request. + await assertFails( updateDoc(doc(rater, "users", RATER), { department: "Matematik" }), ); - }); - - test("rating another user's note may raise only their points", async () => { - // The client-side rating flow needs this: it writes the owner's new total. - await assertSucceeds( - updateDoc(doc(rater, "users", OWNER), { points: 14 }), + await assertFails( + updateDoc(doc(owner, "users", OWNER), { department: "Matematik" }), ); }); - test("a points write cannot smuggle another field along with it", async () => { + test("a user cannot award themselves points", async () => { + // This was the hole: `allow update: if isOwner(userId)` let a user write any + // field of their own document, so `points: 999999` needed no rating at all. await assertFails( - updateDoc(doc(rater, "users", OWNER), { - points: 14, - email: "attacker@example.com", - }), + updateDoc(doc(rater, "users", RATER), { points: 999999 }), ); }); - test("points must be a non-negative integer", async () => { - await assertFails(updateDoc(doc(rater, "users", OWNER), { points: -1 })); - await assertFails(updateDoc(doc(rater, "users", OWNER), { points: 4.5 })); - await assertFails(updateDoc(doc(rater, "users", OWNER), { points: "9" })); + test("a user cannot write points onto anyone else either", async () => { + await assertFails(updateDoc(doc(rater, "users", OWNER), { points: 14 })); }); test("a stranger cannot rewrite another profile's department", async () => { @@ -184,6 +198,8 @@ describe("notes", () => { title: "Note", uploaderUid: RATER, department: "Bilgisayar Mühendisliği", + ratingSum: 0, + ratingCount: 0, }), ); await assertFails( @@ -191,6 +207,20 @@ describe("notes", () => { title: "Note", uploaderUid: OWNER, department: "Bilgisayar Mühendisliği", + ratingSum: 0, + ratingCount: 0, + }), + ); + }); + + 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, }), ); }); @@ -207,41 +237,65 @@ describe("notes", () => { ); }); - test("a rater may update only the three rating fields", async () => { - await assertSucceeds( - updateDoc(doc(rater, "notes", NOTE), { - ratingSum: 4, - ratingCount: 1, - avgRating: 4, + test("the uploader cannot raise their own note's score while editing it", async () => { + // The shortest route to the top of the leaderboard used to be editing your + // own note, since ownership alone authorised the whole document. + await assertFails( + updateDoc(doc(owner, "notes", NOTE), { + title: "Güncellenmiş başlık", + ratingSum: 500, }), ); }); - test("a rating update cannot carry a metadata change with it", async () => { - await assertFails( - updateDoc(doc(rater, "notes", NOTE), { - ratingSum: 4, - ratingCount: 1, - avgRating: 4, - title: "Ele geçirildi", - }), - ); + test("a vote and the totals it implies are accepted together", async () => { + await assertSucceeds(vote(rater, RATER, NOTE, 4, { sum: 4, count: 1 })); }); - test("rating totals cannot go negative", async () => { + test("totals cannot move without a vote to justify them", async () => { + // This is the whole point of the getAfter() check: the note update is only + // authorised by a rating document written in the same commit. await assertFails( updateDoc(doc(rater, "notes", NOTE), { - ratingSum: -1, - ratingCount: 1, - avgRating: 0, + ratingSum: 500, + ratingCount: 100, + avgRating: 5, }), ); + }); + + test("the totals must match the vote, not exceed it", async () => { + // Every value here stays inside the range checks, so the only thing that can + // reject them is the arithmetic: a 1-star vote claiming a sum of 5, and a + // 5-star vote claiming a second voter that does not exist. + await assertFails(vote(rater, RATER, NOTE, 1, { sum: 5, count: 1 })); + await assertFails(vote(rater, RATER, NOTE, 5, { sum: 5, count: 2 })); + await assertFails(vote(rater, RATER, NOTE, 3, { sum: 3, count: 0 })); + }); + + test("changing a vote moves the sum by the difference and leaves the count", async () => { + await assertSucceeds(vote(rater, RATER, NOTE, 2, { sum: 2, count: 1 })); + await assertSucceeds(vote(rater, RATER, NOTE, 5, { sum: 5, count: 1 })); + }); + + test("a changed vote cannot be counted twice", async () => { + await assertSucceeds(vote(rater, RATER, NOTE, 2, { sum: 2, count: 1 })); + await assertFails(vote(rater, RATER, NOTE, 5, { sum: 5, count: 2 })); + }); + + test("nobody can rate their own note", async () => { + await assertFails(vote(owner, OWNER, NOTE, 5, { sum: 5, count: 1 })); + }); + + test("a vote cannot be laundered through someone else's rating document", async () => { + // Writing the totals while pointing getAfter at a rating the caller does not + // own: the rating rule rejects the forged document, so the batch fails whole. + await assertFails(vote(rater, OWNER, NOTE, 5, { sum: 5, count: 1 })); + }); + + test("a rating update cannot carry a metadata change with it", async () => { await assertFails( - updateDoc(doc(rater, "notes", NOTE), { - ratingSum: 4, - ratingCount: -1, - avgRating: 4, - }), + vote(rater, RATER, NOTE, 4, { sum: 4, count: 1, extra: { title: "Ele geçirildi" } }), ); }); @@ -264,6 +318,8 @@ describe("note content", () => { title: "Note", uploaderUid: RATER, department: "Bilgisayar Mühendisliği", + ratingSum: 0, + ratingCount: 0, }); batch.set(doc(rater, "notes", "batched", "content", "file"), { fileData: "ZmFrZQ==", diff --git a/firestore.rules b/firestore.rules index 144ec2e..4266594 100644 --- a/firestore.rules +++ b/firestore.rules @@ -7,18 +7,11 @@ rules_version = '2'; // // firebase deploy --only firestore:rules // -// ÖNEMLİ — bilinen zayıflık: puanlama işlemi tamamen istemcide yapılıyor. -// RatingRepository bir oy verirken not sahibinin `points` alanını ve notun -// `ratingSum`/`ratingCount`/`avgRating` alanlarını doğrudan güncelliyor. -// Bu yüzden aşağıdaki kurallar, oturum açmış bir kullanıcının başka bir -// kullanıcının puanını ve bir notun ortalamasını değiştirmesine izin vermek -// zorunda. Değerler tür ve aralık olarak sınırlandırıldı ama kötü niyetli bir -// istemci hâlâ kendine puan yazabilir. -// -// Bunun doğru çözümü puanlamayı bir Cloud Function'a taşımaktır (istemci -// yalnızca oyunu yazar, toplamları sunucu hesaplar). Cloud Functions -// faturalandırmalı Blaze planı gerektirdiği için proje ücretsiz planda -// kaldığı sürece bu risk kabul edilmiş durumda. +// Puanlama hâlâ istemcide hesaplanıyor — Cloud Functions faturalandırmalı Blaze +// planı istiyor, proje ücretsiz planda. Ama "istemci hesaplıyor" ile "istemciye +// 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. service cloud.firestore { match /databases/{database}/documents { @@ -44,14 +37,14 @@ service cloud.firestore { allow create: if isOwner(userId) && request.resource.data.id == userId; - allow update: if isOwner(userId) - // Puanlamanın not sahibinin puanını artırması için gereken izin. - // Yalnızca `points` alanı değişebilir ve negatif olamaz. - || ( - changedFields().hasOnly(['points']) - && request.resource.data.points is int - && request.resource.data.points >= 0 - ); + // Uygulama bu dokümanı yalnızca oluşturuyor, hiç güncellemiyor + // (`saveUser` tek çağrısı kayıt akışında). Açık bırakmanın bedeli vardı: + // `points` burada saklandığı sürece kurallar hem sahibinin kendi puanını + // yazmasına hem de bir kullanıcının başkasının puanını değiştirmesine izin + // vermek zorundaydı. Puan artık saklanmıyor, notların `ratingSum` + // toplamından türetiliyor; dolayısıyla güncellemeye hiç gerek kalmadı. + // İleride profil düzenleme eklenirse kendi dar kuralını da getirmeli. + allow update: if false; // Hesap silme uygulamada yok; kaza ile veri kaybını engelle. allow delete: if false; @@ -60,21 +53,68 @@ service cloud.firestore { match /notes/{noteId} { allow read: if isSignedIn(); - // Not oluşturan kişi kendini başkası olarak gösteremez. + // 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. allow create: if isSignedIn() - && request.resource.data.uploaderUid == request.auth.uid; + && request.resource.data.uploaderUid == request.auth.uid + && request.resource.data.ratingSum == 0 + && request.resource.data.ratingCount == 0; allow update: if isSignedIn() && ( - // Sahibi metadata'yı düzenleyebilir. - resource.data.uploaderUid == request.auth.uid - // Oy veren yalnızca puan alanlarını değiştirebilir. - || ( - changedFields().hasOnly(['ratingSum', 'ratingCount', 'avgRating']) - && request.resource.data.ratingSum >= 0 - && request.resource.data.ratingCount >= 0 - ) + 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. + function isMetadataEditByOwner() { + return resource.data.uploaderUid == request.auth.uid + && !changedFields().hasAny([ + 'ratingSum', 'ratingCount', 'avgRating', 'uploaderUid' + ]); + } + + function ratingPath(noteId) { + return /databases/$(database)/documents/ratings/$(request.auth.uid + '_' + noteId); + } + + /** + * Oyu veren kişinin yazabileceği tek değişiklik. + * + * `getAfter` aynı commit'in *sonrasındaki* oy dokümanını okur; oy dokümanı + * yazılmadan notun toplamı değiştirilemez. Beklenen toplamlar + * RatingCalculator ile birebir aynı hesaplanıyor — tabanları dahil, çünkü + * istemci de aynı tabanları uyguluyor ve eşitlik tutmak zorunda. + * + * `math.max` kurallarda yok, o yüzden tabanlar üçlü işleçle yazıldı. + */ + function isRatingWrite(noteId) { + let path = ratingPath(noteId); + let votedBefore = exists(path); + let previous = votedBefore ? get(path).data.rating : 0; + let submitted = getAfter(path).data; + let rawSum = resource.data.ratingSum + submitted.rating - previous; + + return changedFields().hasOnly(['ratingSum', 'ratingCount', 'avgRating']) + // Kendi notuna oy vermek burada yasaklanıyor; eskiden yalnızca + // istemci kodunda kontrol ediliyordu. + && resource.data.uploaderUid != request.auth.uid + && submitted.noteId == noteId + && request.resource.data.ratingSum == (rawSum < 0 ? 0 : rawSum) + // Oy değiştirildiğinde sayı artmaz. + && request.resource.data.ratingCount == ( + 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 // engel bu kural. (NoteRepository.deleteNote doğrudan siliyor, arayüz // sadece kendi notlarında silme düğmesini gösteriyor.) @@ -106,7 +146,12 @@ service cloud.firestore { && ratingId == request.auth.uid + '_' + request.resource.data.noteId && request.resource.data.rating is int && request.resource.data.rating >= 1 - && request.resource.data.rating <= 5; + && request.resource.data.rating <= 5 + // Kendi notuna oy veren bir kayıt hiç oluşmasın: + // notun toplamını değiştirmese de liderlik tablosunun + // oy sayısını şişirirdi. + && get(/databases/$(database)/documents/notes/$(request.resource.data.noteId)) + .data.uploaderUid != request.auth.uid; allow delete: if false; }