diff --git a/backend/src/handlers/appointments.rs b/backend/src/handlers/appointments.rs index 7939fc4..e95fbde 100644 --- a/backend/src/handlers/appointments.rs +++ b/backend/src/handlers/appointments.rs @@ -142,31 +142,98 @@ pub async fn get_appointment( pub async fn update_appointment( State(state): State, - Extension(_claims): Extension, + Extension(claims): Extension, Path(id): Path, Json(req): Json, -) -> Result, StatusCode> { +) -> Result, (StatusCode, Json)> { 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, - Extension(_claims): Extension, + Extension(claims): Extension, Path(id): Path, -) -> Result { +) -> Result)> { 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" })), + )) + } } } diff --git a/backend/src/handlers/medications.rs b/backend/src/handlers/medications.rs index 2488c9a..c6bf0c5 100644 --- a/backend/src/handlers/medications.rs +++ b/backend/src/handlers/medications.rs @@ -155,34 +155,102 @@ pub async fn get_medication( pub async fn update_medication( State(state): State, - Extension(_claims): Extension, + Extension(claims): Extension, Path(id): Path, Json(req): Json, -) -> Result, StatusCode> { +) -> Result, (StatusCode, Json)> { 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, - Extension(_claims): Extension, + Extension(claims): Extension, Path(id): Path, -) -> Result { +) -> Result)> { 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, Path(id): Path, Json(req): Json, -) -> Result<(StatusCode, Json), StatusCode> { +) -> Result<(StatusCode, Json), (StatusCode, Json)> { 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::("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, - Extension(_claims): Extension, + Extension(claims): Extension, Path(id): Path, -) -> Result, StatusCode> { +) -> Result, (StatusCode, Json)> +{ use mongodb::bson::{doc, DateTime}; const PERIOD_DAYS: i64 = 30; @@ -231,6 +332,28 @@ pub async fn get_adherence( let doses: mongodb::Collection = 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) { diff --git a/backend/tests/ownership_tests.rs b/backend/tests/ownership_tests.rs new file mode 100644 index 0000000..b5cc3b9 --- /dev/null +++ b/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; +}