Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 31 additions & 14 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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}`**

Expand Down Expand Up @@ -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).

Expand All @@ -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
Expand Down Expand Up @@ -339,16 +356,16 @@ 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.

| 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 `<uid>_<noteId>` 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. |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand Down Expand Up @@ -61,7 +59,6 @@ object RatingCalculator {
sum = sum,
count = count,
average = average,
pointsDelta = delta,
updatedExistingRating = updatedExistingRating
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -83,14 +82,12 @@ 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"
const val AVERAGE_RATING_FIELD = "avgRating"
const val RATING_FIELD = "rating"
const val USER_ID_FIELD = "uid"
const val NOTE_ID_FIELD = "noteId"
const val POINTS_FIELD = "points"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand All @@ -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
Expand All @@ -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)
}

Expand All @@ -74,7 +71,6 @@ class RatingCalculatorTest {

assertEquals(6L, totals.sum)
assertEquals(2L, totals.count)
assertEquals(-4L, totals.pointsDelta)
}

@Test
Expand All @@ -88,7 +84,6 @@ class RatingCalculatorTest {

assertEquals(7L, totals.sum)
assertEquals(2L, totals.count)
assertEquals(0L, totals.pointsDelta)
}

@Test
Expand Down
5 changes: 2 additions & 3 deletions app/src/test/java/duygu/yilmaz/campusnote/testing/Fixtures.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading