Merge pull request 'fix(security): enforce ownership on medication/appointment write paths (#12)' (#13) from fix/12-write-path-ownership into main

Reviewed-on: #13

backend/src/handlers/appointments.rs

@@ -142,31 +142,98 @@ pub async fn get_appointment(
 
 pub async fn update_appointment(
     State(state): State<AppState>,
-    Extension(_claims): Extension<Claims>,
+    Extension(claims): Extension<Claims>,
     Path(id): Path<String>,
     Json(req): Json<UpdateAppointmentRequest>,
-) -> Result<Json<AppointmentResponse>, StatusCode> {
+) -> Result<Json<AppointmentResponse>, (StatusCode, Json<serde_json::Value>)> {
     let database = state.db.get_database();
     let repo = AppointmentRepository::new(database.collection("appointments"));
 
+    // Authorization: owner-only (recipients are read-only). 404 on mismatch to
+    // avoid leaking existence.
+    let existing = match repo.find_by_appointment_id(&id).await {
+        Ok(Some(a)) => a,
+        Ok(None) => {
+            return Err((
+                StatusCode::NOT_FOUND,
+                Json(serde_json::json!({ "error": "not found" })),
+            ));
+        }
+        Err(e) => {
+            tracing::error!("update_appointment lookup failed: {}", e);
+            return Err((
+                StatusCode::INTERNAL_SERVER_ERROR,
+                Json(serde_json::json!({ "error": "database error" })),
+            ));
+        }
+    };
+    if existing.user_id != claims.sub {
+        return Err((
+            StatusCode::NOT_FOUND,
+            Json(serde_json::json!({ "error": "not found" })),
+        ));
+    }
+
     match repo.update_by_appointment_id(&id, req).await {
         Ok(Some(appt)) => Ok(Json(appt.into())),
-        Ok(None) => Err(StatusCode::NOT_FOUND),
-        Err(_) => Err(StatusCode::INTERNAL_SERVER_ERROR),
+        Ok(None) => Err((
+            StatusCode::NOT_FOUND,
+            Json(serde_json::json!({ "error": "not found" })),
+        )),
+        Err(e) => {
+            tracing::error!("update_appointment failed: {}", e);
+            Err((
+                StatusCode::INTERNAL_SERVER_ERROR,
+                Json(serde_json::json!({ "error": "database error" })),
+            ))
+        }
     }
 }
 
 pub async fn delete_appointment(
     State(state): State<AppState>,
-    Extension(_claims): Extension<Claims>,
+    Extension(claims): Extension<Claims>,
     Path(id): Path<String>,
-) -> Result<StatusCode, StatusCode> {
+) -> Result<StatusCode, (StatusCode, Json<serde_json::Value>)> {
     let database = state.db.get_database();
     let repo = AppointmentRepository::new(database.collection("appointments"));
 
+    // Authorization: owner-only.
+    let existing = match repo.find_by_appointment_id(&id).await {
+        Ok(Some(a)) => a,
+        Ok(None) => {
+            return Err((
+                StatusCode::NOT_FOUND,
+                Json(serde_json::json!({ "error": "not found" })),
+            ));
+        }
+        Err(e) => {
+            tracing::error!("delete_appointment lookup failed: {}", e);
+            return Err((
+                StatusCode::INTERNAL_SERVER_ERROR,
+                Json(serde_json::json!({ "error": "database error" })),
+            ));
+        }
+    };
+    if existing.user_id != claims.sub {
+        return Err((
+            StatusCode::NOT_FOUND,
+            Json(serde_json::json!({ "error": "not found" })),
+        ));
+    }
+
     match repo.delete_by_appointment_id(&id).await {
         Ok(true) => Ok(StatusCode::NO_CONTENT),
-        Ok(false) => Err(StatusCode::NOT_FOUND),
-        Err(_) => Err(StatusCode::INTERNAL_SERVER_ERROR),
+        Ok(false) => Err((
+            StatusCode::NOT_FOUND,
+            Json(serde_json::json!({ "error": "not found" })),
+        )),
+        Err(e) => {
+            tracing::error!("delete_appointment failed: {}", e);
+            Err((
+                StatusCode::INTERNAL_SERVER_ERROR,
+                Json(serde_json::json!({ "error": "database error" })),
+            ))
+        }
     }
 }

backend/src/handlers/medications.rs

@@ -155,34 +155,102 @@ pub async fn get_medication(
 
 pub async fn update_medication(
     State(state): State<AppState>,
-    Extension(_claims): Extension<Claims>,
+    Extension(claims): Extension<Claims>,
     Path(id): Path<String>,
     Json(req): Json<UpdateMedicationRequest>,
-) -> Result<Json<MedicationResponse>, StatusCode> {
+) -> Result<Json<MedicationResponse>, (StatusCode, Json<serde_json::Value>)> {
     let database = state.db.get_database();
     let repo = MedicationRepository::new(database.collection("medications"));
 
-    // Look up by medication_id (UUID) — the path param is not a Mongo ObjectId.
+    // Authorization: look up first, confirm the caller owns the medication
+    // (writes are owner-only — recipients are read-only per the ADR). Use 404
+    // (not 403) on a mismatch to avoid leaking the existence of other users'
+    // records.
+    let existing = match repo.find_by_medication_id(&id).await {
+        Ok(Some(m)) => m,
+        Ok(None) => {
+            return Err((
+                StatusCode::NOT_FOUND,
+                Json(serde_json::json!({ "error": "not found" })),
+            ));
+        }
+        Err(e) => {
+            tracing::error!("update_medication lookup failed: {}", e);
+            return Err((
+                StatusCode::INTERNAL_SERVER_ERROR,
+                Json(serde_json::json!({ "error": "database error" })),
+            ));
+        }
+    };
+    if existing.user_id != claims.sub {
+        return Err((
+            StatusCode::NOT_FOUND,
+            Json(serde_json::json!({ "error": "not found" })),
+        ));
+    }
+
     match repo.update_by_medication_id(&id, req).await {
         Ok(Some(medication)) => Ok(Json(medication.into())),
-        Ok(None) => Err(StatusCode::NOT_FOUND),
-        Err(_) => Err(StatusCode::INTERNAL_SERVER_ERROR),
+        Ok(None) => Err((
+            StatusCode::NOT_FOUND,
+            Json(serde_json::json!({ "error": "not found" })),
+        )),
+        Err(e) => {
+            tracing::error!("update_medication failed: {}", e);
+            Err((
+                StatusCode::INTERNAL_SERVER_ERROR,
+                Json(serde_json::json!({ "error": "database error" })),
+            ))
+        }
     }
 }
 
 pub async fn delete_medication(
     State(state): State<AppState>,
-    Extension(_claims): Extension<Claims>,
+    Extension(claims): Extension<Claims>,
     Path(id): Path<String>,
-) -> Result<StatusCode, StatusCode> {
+) -> Result<StatusCode, (StatusCode, Json<serde_json::Value>)> {
     let database = state.db.get_database();
     let repo = MedicationRepository::new(database.collection("medications"));
 
-    // Look up by medication_id (UUID), not Mongo _id.
+    // Authorization: owner-only (see update_medication). Look up first to
+    // avoid leaking existence via the deleted_count.
+    let existing = match repo.find_by_medication_id(&id).await {
+        Ok(Some(m)) => m,
+        Ok(None) => {
+            return Err((
+                StatusCode::NOT_FOUND,
+                Json(serde_json::json!({ "error": "not found" })),
+            ));
+        }
+        Err(e) => {
+            tracing::error!("delete_medication lookup failed: {}", e);
+            return Err((
+                StatusCode::INTERNAL_SERVER_ERROR,
+                Json(serde_json::json!({ "error": "database error" })),
+            ));
+        }
+    };
+    if existing.user_id != claims.sub {
+        return Err((
+            StatusCode::NOT_FOUND,
+            Json(serde_json::json!({ "error": "not found" })),
+        ));
+    }
+
     match repo.delete_by_medication_id(&id).await {
         Ok(true) => Ok(StatusCode::NO_CONTENT),
-        Ok(false) => Err(StatusCode::NOT_FOUND),
-        Err(_) => Err(StatusCode::INTERNAL_SERVER_ERROR),
+        Ok(false) => Err((
+            StatusCode::NOT_FOUND,
+            Json(serde_json::json!({ "error": "not found" })),
+        )),
+        Err(e) => {
+            tracing::error!("delete_medication failed: {}", e);
+            Err((
+                StatusCode::INTERNAL_SERVER_ERROR,
+                Json(serde_json::json!({ "error": "database error" })),
+            ))
+        }
     }
 }
 
@@ -191,8 +259,34 @@ pub async fn log_dose(
     Extension(claims): Extension<Claims>,
     Path(id): Path<String>,
     Json(req): Json<LogDoseRequest>,
-) -> Result<(StatusCode, Json<MedicationDose>), StatusCode> {
+) -> Result<(StatusCode, Json<MedicationDose>), (StatusCode, Json<serde_json::Value>)> {
     let database = state.db.get_database();
+    let repo = MedicationRepository::new(database.collection("medications"));
+
+    // Authorization: confirm the caller owns the medication before logging a
+    // dose against it (otherwise a user could skew another's adherence stats).
+    let med = match repo.find_by_medication_id(&id).await {
+        Ok(Some(m)) => m,
+        Ok(None) => {
+            return Err((
+                StatusCode::NOT_FOUND,
+                Json(serde_json::json!({ "error": "not found" })),
+            ));
+        }
+        Err(e) => {
+            tracing::error!("log_dose lookup failed: {}", e);
+            return Err((
+                StatusCode::INTERNAL_SERVER_ERROR,
+                Json(serde_json::json!({ "error": "database error" })),
+            ));
+        }
+    };
+    if med.user_id != claims.sub {
+        return Err((
+            StatusCode::NOT_FOUND,
+            Json(serde_json::json!({ "error": "not found" })),
+        ));
+    }
 
     let now = SystemTime::now();
 
@@ -210,7 +304,13 @@ pub async fn log_dose(
         .collection::<crate::models::medication::MedicationDose>("medication_doses")
         .insert_one(&dose, None)
         .await
-        .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
+        .map_err(|e| {
+            tracing::error!("log_dose insert failed: {}", e);
+            (
+                StatusCode::INTERNAL_SERVER_ERROR,
+                Json(serde_json::json!({ "error": "database error" })),
+            )
+        })?;
 
     // Populate the generated _id so the caller gets the persisted dose back.
     dose.id = result.inserted_id.as_object_id();
@@ -220,9 +320,10 @@ pub async fn log_dose(
 
 pub async fn get_adherence(
     State(state): State<AppState>,
-    Extension(_claims): Extension<Claims>,
+    Extension(claims): Extension<Claims>,
     Path(id): Path<String>,
-) -> Result<Json<crate::models::medication::AdherenceStats>, StatusCode> {
+) -> Result<Json<crate::models::medication::AdherenceStats>, (StatusCode, Json<serde_json::Value>)>
+{
     use mongodb::bson::{doc, DateTime};
 
     const PERIOD_DAYS: i64 = 30;
@@ -231,6 +332,28 @@ pub async fn get_adherence(
     let doses: mongodb::Collection<MedicationDose> = database.collection("medication_doses");
     let med_repo = MedicationRepository::new(database.collection("medications"));
 
+    // Authorization: owner-only. Look up the medication and confirm ownership
+    // before computing/returning adherence (which leaks dose history).
+    let medication = match med_repo.find_by_medication_id(&id).await {
+        Ok(Some(m)) => {
+            if m.user_id != claims.sub {
+                return Err((
+                    StatusCode::NOT_FOUND,
+                    Json(serde_json::json!({ "error": "not found" })),
+                ));
+            }
+            Some(m)
+        }
+        Ok(None) => None,
+        Err(e) => {
+            tracing::error!("get_adherence lookup failed: {}", e);
+            return Err((
+                StatusCode::INTERNAL_SERVER_ERROR,
+                Json(serde_json::json!({ "error": "database error" })),
+            ));
+        }
+    };
+
     // Look at doses logged in the last PERIOD_DAYS days for this medication.
     let since = DateTime::from_system_time(
         SystemTime::now() - std::time::Duration::from_secs(PERIOD_DAYS as u64 * 86400),
@@ -243,19 +366,25 @@ pub async fn get_adherence(
     let total_logged = doses
         .count_documents(filter.clone(), None)
         .await
-        .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
+        .map_err(|e| {
+            tracing::error!("get_adherence count failed: {}", e);
+            (
+                StatusCode::INTERNAL_SERVER_ERROR,
+                Json(serde_json::json!({ "error": "database error" })),
+            )
+        })?;
 
     let taken_filter = doc! { "$and": [filter, doc! { "taken": true }] };
     let taken = doses
         .count_documents(taken_filter, None)
         .await
-        .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
-
-    // Fetch the medication to check for a dose schedule.
-    let medication = med_repo
-        .find_by_medication_id(&id)
-        .await
-        .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
+        .map_err(|e| {
+            tracing::error!("get_adherence taken count failed: {}", e);
+            (
+                StatusCode::INTERNAL_SERVER_ERROR,
+                Json(serde_json::json!({ "error": "database error" })),
+            )
+        })?;
 
     let (scheduled_doses, total_doses, missed_doses, rate) =
         match medication.and_then(|m| m.dose_schedule) {

backend/tests/ownership_tests.rs

@@ -0,0 +1,258 @@
+//! Write/delete path ownership tests (#12).
+//!
+//! Verifies the ownership checks added to fix the IDOR: user A creates a
+//! medication/appointment; user B cannot update, delete, log-dose-against, or
+//! read adherence for it. All cross-user attempts must 404 (we use 404, not
+//! 403, to avoid leaking the existence of other users' records).
+//!
+//! Requires a live MongoDB; skips gracefully otherwise.
+
+mod common;
+
+use serde_json::{json, Value};
+
+macro_rules! require_app {
+    ($app:expr) => {
+        match $app {
+            Some(x) => x,
+            None => {
+                eprintln!("[integration] skipped (MongoDB unavailable)");
+                return;
+            }
+        }
+    };
+}
+
+fn unique_email() -> String {
+    format!("test_{}@example.com", uuid::Uuid::new_v4())
+}
+
+/// Register a user and return the access token.
+async fn register(app: &axum::Router, email: &str) -> String {
+    let (status, body) = common::send_json(
+        app,
+        "POST",
+        "/api/auth/register",
+        Some(json!({ "email": email, "username": "tester", "password": "supersecret" })),
+        None,
+    )
+    .await;
+    assert_eq!(status, 201, "register failed, body: {body}");
+    body["token"].as_str().unwrap().to_string()
+}
+
+/// Create a medication as `token` and return its medication_id.
+async fn create_medication(app: &axum::Router, token: &str) -> String {
+    let (status, body) = common::send_json(
+        app,
+        "POST",
+        "/api/medications",
+        Some(json!({
+            "profile_id": "default",
+            "encrypted_data": { "data": "b3duZXItYmxvYg==", "iv": "aXZ2aXZ2aXZ2aXZ2" },
+        })),
+        Some(token),
+    )
+    .await;
+    assert!(
+        status == 200 || status == 201,
+        "create_medication failed: {status} {body}"
+    );
+    body["medication_id"].as_str().unwrap().to_string()
+}
+
+/// Create an appointment as `token` and return its appointment_id.
+async fn create_appointment(app: &axum::Router, token: &str) -> String {
+    let (status, body) = common::send_json(
+        app,
+        "POST",
+        "/api/appointments",
+        Some(json!({
+            "profile_id": "default",
+            "encrypted_data": { "data": "YXBwdC1ibG9i", "iv": "aXZ2aXZ2aXZ2aXZ2" },
+            "status": "upcoming",
+        })),
+        Some(token),
+    )
+    .await;
+    assert!(
+        status == 200 || status == 201,
+        "create_appointment failed: {status} {body}"
+    );
+    body["appointment_id"].as_str().unwrap().to_string()
+}
+
+#[tokio::test]
+async fn other_user_cannot_update_or_delete_medication() {
+    let (app, db_name) = require_app!(common::app_for_test().await);
+    let token_a = register(&app, &unique_email()).await;
+    let token_b = register(&app, &unique_email()).await;
+    let med_id = create_medication(&app, &token_a).await;
+
+    // B cannot UPDATE A's medication.
+    let (status, _body) = common::send_json(
+        &app,
+        "POST",
+        &format!("/api/medications/{med_id}"),
+        Some(json!({
+            "encrypted_data": { "data": "aGFja2Vk", "iv": "aXZ2aXZ2aXZ2aXZ2" },
+        })),
+        Some(&token_b),
+    )
+    .await;
+    assert_eq!(status, 404, "B must not update A's medication");
+
+    // B cannot DELETE A's medication.
+    let (status, _) = common::send_json(
+        &app,
+        "POST",
+        &format!("/api/medications/{med_id}/delete"),
+        None,
+        Some(&token_b),
+    )
+    .await;
+    assert_eq!(status, 404, "B must not delete A's medication");
+
+    // A still sees it intact.
+    let (status, _body) = common::send_json(
+        &app,
+        "GET",
+        &format!("/api/medications/{med_id}"),
+        None,
+        Some(&token_a),
+    )
+    .await;
+    assert_eq!(status, 200, "A's medication must be untouched");
+
+    common::drop_test_db(&db_name).await;
+}
+
+#[tokio::test]
+async fn other_user_cannot_log_dose_or_read_adherence_for_others_medication() {
+    let (app, db_name) = require_app!(common::app_for_test().await);
+    let token_a = register(&app, &unique_email()).await;
+    let token_b = register(&app, &unique_email()).await;
+    let med_id = create_medication(&app, &token_a).await;
+
+    // B cannot LOG A DOSE against A's medication (would skew A's adherence).
+    let (status, _body) = common::send_json(
+        &app,
+        "POST",
+        &format!("/api/medications/{med_id}/log"),
+        Some(json!({ "taken": true })),
+        Some(&token_b),
+    )
+    .await;
+    assert_eq!(status, 404, "B must not log a dose against A's medication");
+
+    // B cannot READ A's adherence (leaks dose history).
+    let (status, _body) = common::send_json(
+        &app,
+        "GET",
+        &format!("/api/medications/{med_id}/adherence"),
+        None,
+        Some(&token_b),
+    )
+    .await;
+    assert_eq!(status, 404, "B must not read A's adherence");
+
+    // A can both (sanity).
+    let (status, _body) = common::send_json(
+        &app,
+        "POST",
+        &format!("/api/medications/{med_id}/log"),
+        Some(json!({ "taken": true })),
+        Some(&token_a),
+    )
+    .await;
+    assert_eq!(status, 201, "A should be able to log a dose: {_body}");
+    let (status, _body) = common::send_json(
+        &app,
+        "GET",
+        &format!("/api/medications/{med_id}/adherence"),
+        None,
+        Some(&token_a),
+    )
+    .await;
+    assert_eq!(status, 200, "A should be able to read adherence");
+
+    common::drop_test_db(&db_name).await;
+}
+
+#[tokio::test]
+async fn other_user_cannot_update_or_delete_appointment() {
+    let (app, db_name) = require_app!(common::app_for_test().await);
+    let token_a = register(&app, &unique_email()).await;
+    let token_b = register(&app, &unique_email()).await;
+    let appt_id = create_appointment(&app, &token_a).await;
+
+    // B cannot UPDATE A's appointment.
+    let (status, _body) = common::send_json(
+        &app,
+        "POST",
+        &format!("/api/appointments/{appt_id}"),
+        Some(json!({
+            "encrypted_data": { "data": "aGFja2Vk", "iv": "aXZ2aXZ2aXZ2aXZ2" },
+            "status": "cancelled",
+        })),
+        Some(&token_b),
+    )
+    .await;
+    assert_eq!(status, 404, "B must not update A's appointment");
+
+    // B cannot DELETE A's appointment.
+    let (status, _) = common::send_json(
+        &app,
+        "POST",
+        &format!("/api/appointments/{appt_id}/delete"),
+        None,
+        Some(&token_b),
+    )
+    .await;
+    assert_eq!(status, 404, "B must not delete A's appointment");
+
+    // A still sees it.
+    let (status, _body): (u16, Value) = common::send_json(
+        &app,
+        "GET",
+        &format!("/api/appointments/{appt_id}"),
+        None,
+        Some(&token_a),
+    )
+    .await;
+    assert_eq!(status, 200, "A's appointment must be untouched");
+
+    common::drop_test_db(&db_name).await;
+}
+
+#[tokio::test]
+async fn owner_can_update_and_delete_own_medication() {
+    // Sanity: the new checks must not block the legitimate owner.
+    let (app, db_name) = require_app!(common::app_for_test().await);
+    let token = register(&app, &unique_email()).await;
+    let med_id = create_medication(&app, &token).await;
+
+    let (status, _body) = common::send_json(
+        &app,
+        "POST",
+        &format!("/api/medications/{med_id}"),
+        Some(json!({
+            "encrypted_data": { "data": "dXBkYXRlZA==", "iv": "aXZ2aXZ2aXZ2aXZ2" },
+        })),
+        Some(&token),
+    )
+    .await;
+    assert_eq!(status, 200, "owner should update: {_body}");
+
+    let (status, _) = common::send_json(
+        &app,
+        "POST",
+        &format!("/api/medications/{med_id}/delete"),
+        None,
+        Some(&token),
+    )
+    .await;
+    assert_eq!(status, 204, "owner should delete");
+
+    common::drop_test_db(&db_name).await;
+}