diff --git a/services/headless-lms/migrations/20260703090000_add-unique-index-users-upstream-id.down.sql b/services/headless-lms/migrations/20260703090000_add-unique-index-users-upstream-id.down.sql new file mode 100644 index 000000000000..db8355535aba --- /dev/null +++ b/services/headless-lms/migrations/20260703090000_add-unique-index-users-upstream-id.down.sql @@ -0,0 +1 @@ +DROP INDEX IF EXISTS users_upstream_id_active_uniq_idx; diff --git a/services/headless-lms/migrations/20260703090000_add-unique-index-users-upstream-id.up.sql b/services/headless-lms/migrations/20260703090000_add-unique-index-users-upstream-id.up.sql new file mode 100644 index 000000000000..43c1bb0fcabc --- /dev/null +++ b/services/headless-lms/migrations/20260703090000_add-unique-index-users-upstream-id.up.sql @@ -0,0 +1,18 @@ +-- Prevent concurrent user creations (e.g. parallel TMC-server create-user requests during +-- password migration) from inserting duplicate users for the same TMC account. Partial so that +-- soft-deleted users don't block re-creating an account with the same upstream_id. +-- +-- In production, create this index with CONCURRENTLY before applying this migration (a plain +-- CREATE INDEX blocks writes to users while it builds; CONCURRENTLY cannot run inside the +-- migration transaction): +-- CREATE UNIQUE INDEX CONCURRENTLY users_upstream_id_active_uniq_idx ON users (upstream_id) +-- WHERE upstream_id IS NOT NULL AND deleted_at IS NULL; +-- +-- If this fails, duplicate active users with the same upstream_id already exist and must be +-- merged manually first: +-- SELECT upstream_id, array_agg(id) FROM users +-- WHERE upstream_id IS NOT NULL AND deleted_at IS NULL +-- GROUP BY upstream_id HAVING count(*) > 1; +CREATE UNIQUE INDEX IF NOT EXISTS users_upstream_id_active_uniq_idx ON users (upstream_id) +WHERE upstream_id IS NOT NULL + AND deleted_at IS NULL; diff --git a/services/headless-lms/models/src/error.rs b/services/headless-lms/models/src/error.rs index 0ceb9bda0b25..038a946be385 100644 --- a/services/headless-lms/models/src/error.rs +++ b/services/headless-lms/models/src/error.rs @@ -257,6 +257,14 @@ impl From for ModelError { err.to_string(), Some(err.into()), ), + "users_upstream_id_active_uniq_idx" => ModelError::new( + ModelErrorType::DatabaseConstraint { + constraint: constraint.to_string(), + description: "A user with this upstream id already exists.", + }, + err.to_string(), + Some(err.into()), + ), "unique_chatbot_names_within_course" => ModelError::new( ModelErrorType::DatabaseConstraint { constraint: constraint.to_string(), diff --git a/services/headless-lms/server/src/domain/authorization.rs b/services/headless-lms/server/src/domain/authorization.rs index 5f25ca98457f..bfca03484a6b 100644 --- a/services/headless-lms/server/src/domain/authorization.rs +++ b/services/headless-lms/server/src/domain/authorization.rs @@ -942,24 +942,40 @@ pub async fn get_or_create_user_from_tmc_mooc_fi_response( let user = match models::users::find_by_upstream_id(conn, upstream_id).await? { Some(existing_user) => existing_user, None => { - models::users::insert_with_upstream_id_and_moocfi_id( + let inserted = models::users::insert_with_upstream_id_and_moocfi_id( conn, &email, - // convert empty names to None - if user_field.first_name.trim().is_empty() { - None - } else { - Some(user_field.first_name.as_str()) - }, - if user_field.last_name.trim().is_empty() { - None - } else { - Some(user_field.last_name.as_str()) - }, + // convert missing/empty names to None + user_field + .first_name + .as_deref() + .filter(|s| !s.trim().is_empty()), + user_field + .last_name + .as_deref() + .filter(|s| !s.trim().is_empty()), upstream_id, id, ) - .await? + .await; + match inserted { + Ok(user) => user, + // A concurrent request can create the user between the find and the insert + // (the insert runs in a savepoint, so the connection stays usable). The unique + // index on upstream_id rejects the loser; return the winner's row instead. + Err(insert_error) + if matches!( + insert_error.error_type(), + models::ModelErrorType::DatabaseConstraint { constraint, .. } + if constraint == "users_upstream_id_active_uniq_idx" + ) => + { + models::users::find_by_upstream_id(conn, upstream_id) + .await? + .ok_or(insert_error)? + } + Err(insert_error) => return Err(insert_error.into()), + } } }; Ok(user) diff --git a/services/headless-lms/utils/src/services/tmc.rs b/services/headless-lms/utils/src/services/tmc.rs index 396b854fc0c5..98d11ac9785a 100644 --- a/services/headless-lms/utils/src/services/tmc.rs +++ b/services/headless-lms/utils/src/services/tmc.rs @@ -51,14 +51,22 @@ pub struct TMCUser { pub email: String, pub administrator: bool, pub courses_mooc_fi_user_id: Option, + #[serde(default)] pub user_field: TMCUserField, } -#[derive(Debug, Serialize, Deserialize)] +/// User fields are optional data on the TMC side: a user who never filled in their profile (or a +/// TMC instance without the field definitions) serializes them as null or omits them entirely, so +/// deserialization must not require them. +#[derive(Debug, Default, Serialize, Deserialize)] pub struct TMCUserField { - pub first_name: String, - pub last_name: String, - pub organizational_id: String, + #[serde(default)] + pub first_name: Option, + #[serde(default)] + pub last_name: Option, + #[serde(default)] + pub organizational_id: Option, + #[serde(default)] pub course_announcements: bool, }