From e5ce98c874947a406036bc0b6aa3759cfe33d05e Mon Sep 17 00:00:00 2001 From: Valentin Tolmer Date: Fri, 14 Apr 2023 11:40:35 +0200 Subject: [PATCH] server: Improve the error message in case of duplicate emails --- docs/migration_guides/v0.5.md | 58 ++++++++++++++++++++ server/src/domain/sql_migrations.rs | 83 ++++++++++++++++++++++++----- server/src/domain/sql_tables.rs | 11 ++-- 3 files changed, 136 insertions(+), 16 deletions(-) create mode 100644 docs/migration_guides/v0.5.md diff --git a/docs/migration_guides/v0.5.md b/docs/migration_guides/v0.5.md new file mode 100644 index 0000000..759d2f5 --- /dev/null +++ b/docs/migration_guides/v0.5.md @@ -0,0 +1,58 @@ +# Migration from 0.4 to 0.5 + +Welcome! If you're here, it's probably that the migration from 0.4.x to 0.5 +didn't go smoothly for you. Don't worry, we can fix that. + +## Multiple users with the same email + +This is the most common case. You can see in the LLDAP logs that there are +several users with the same email, and they are listed. + +This is not allowed anymore in v0.5, to prevent a user from setting their email +to someone else's email and gaining access to systems that identify by email. + +The problem is that you currently have several users with the same email, so the +constraint cannot be enforced. + +### Step 1: Take a note of the users with duplicate emails + +In the LLDAP logs when you tried to start v0.5+, you'll see some warnings with +the list of users with the same emails. Take note of them. + +### Step 2: Downgrade to v0.4.3 + +If using docker, switch to the `lldap/lldap:v0.4.3` image. Alternatively, grab +the binaries at https://github.com/lldap/lldap/releases/tag/v0.4.3. + +This downgrade is safe and supported. + +### Step 3: Remove duplicate emails + +Restart LLDAP with the v0.4.3 version, and using your notes from step 1, change +the email of users with duplicate emails to make sure that each email is unique. + +### Step 4: Upgrade again + +You can now revert to the initial version. + +## Multiple users/groups with the same UUID + +This should be extremely rare. In this case, you'll need to find which users +have the same UUID, revert to v0.4.3 to be able to apply the changes, and delete +one of the duplicates. + +## FAQ + +### What if I want several users to be controlled by the same email? + +You can use plus codes to set "the same" email to several users, while ensuring +that they can't identify as each other. For instance: + + - Admin: `admin@example.com` + - Read-only admin: `admin+readonly@example.com` + - Jellyfin admin: `admin+jellyfin@example.com` + +### I'm upgrading to a higher version than v0.5. + +This guide is still relevant: you can use whatever later version in place of +v0.5. You'll still need to revert to v0.4.3 to apply the changes. diff --git a/server/src/domain/sql_migrations.rs b/server/src/domain/sql_migrations.rs index 6b6f749..e619ff7 100644 --- a/server/src/domain/sql_migrations.rs +++ b/server/src/domain/sql_migrations.rs @@ -3,9 +3,12 @@ use crate::domain::{ types::{GroupId, UserId, Uuid}, }; use anyhow::Context; +use itertools::Itertools; use sea_orm::{ - sea_query::{self, ColumnDef, Expr, ForeignKey, ForeignKeyAction, Index, Query, Table, Value}, - ConnectionTrait, FromQueryResult, Iden, Statement, TransactionTrait, + sea_query::{ + self, all, ColumnDef, Expr, ForeignKey, ForeignKeyAction, Func, Index, Query, Table, Value, + }, + ConnectionTrait, FromQueryResult, Iden, Order, Statement, TransactionTrait, }; use serde::{Deserialize, Serialize}; use tracing::{info, instrument, warn}; @@ -462,20 +465,73 @@ async fn migrate_to_v3(pool: &DbConnection) -> anyhow::Result<()> { async fn migrate_to_v4(pool: &DbConnection) -> anyhow::Result<()> { let builder = pool.get_database_backend(); // Make emails and UUIDs unique. + if let Err(e) = pool + .execute( + builder.build( + Index::create() + .if_not_exists() + .name("unique-user-email") + .table(Users::Table) + .col(Users::Email) + .unique(), + ), + ) + .await + .context( + r#"while enforcing unicity on emails (2 users have the same email). + +See https://github.com/lldap/lldap/blob/main/docs/migration_guides/v0.5.md for details. + +"#, + ) + { + warn!("Found several users with the same email:"); + for (email, users) in &pool + .query_all( + builder.build( + Query::select() + .from(Users::Table) + .columns([Users::Email, Users::UserId]) + .order_by_columns([(Users::Email, Order::Asc), (Users::UserId, Order::Asc)]) + .and_where( + Expr::col(Users::Email).in_subquery( + Query::select() + .from(Users::Table) + .column(Users::Email) + .group_by_col(Users::Email) + .cond_having(all![Expr::gt( + Expr::expr(Func::count(Expr::col(Users::Email))), + 1 + )]) + .take(), + ), + ), + ), + ) + .await + .expect("Could not check duplicate users") + .into_iter() + .map(|row| { + ( + row.try_get::("", &Users::UserId.to_string()) + .unwrap(), + row.try_get::("", &Users::Email.to_string()) + .unwrap(), + ) + }) + .group_by(|(_user, email)| email.to_owned()) + { + warn!("Email: {email}"); + for (user, _email) in users { + warn!(" User: {}", user.as_str()); + } + } + return Err(e); + } pool.execute( builder.build( Index::create() - .name("unique-user-email") - .table(Users::Table) - .col(Users::Email) - .unique(), - ), - ) - .await - .context("while enforcing unicity on emails (2 users have the same email)")?; - pool.execute( - builder.build( - Index::create() + .if_not_exists() .name("unique-user-uuid") .table(Users::Table) .col(Users::Uuid) @@ -487,6 +543,7 @@ async fn migrate_to_v4(pool: &DbConnection) -> anyhow::Result<()> { pool.execute( builder.build( Index::create() + .if_not_exists() .name("unique-group-uuid") .table(Groups::Table) .col(Groups::Uuid) diff --git a/server/src/domain/sql_tables.rs b/server/src/domain/sql_tables.rs index 2afce3f..cc0d37b 100644 --- a/server/src/domain/sql_tables.rs +++ b/server/src/domain/sql_tables.rs @@ -46,6 +46,7 @@ mod tests { use super::*; use chrono::prelude::*; use sea_orm::{ConnectionTrait, Database, DbBackend, FromQueryResult}; + use tracing::error; async fn get_in_memory_db() -> DbConnection { let mut sql_opt = sea_orm::ConnectOptions::new("sqlite::memory:".to_owned()); @@ -208,6 +209,7 @@ mod tests { #[tokio::test] async fn test_migration_to_v4() { + crate::infra::logging::init_for_tests(); let sql_pool = get_in_memory_db().await; upgrade_to_v1(&sql_pool).await.unwrap(); migrate_from_version(&sql_pool, SchemaVersion(1), SchemaVersion(3)) @@ -227,9 +229,12 @@ mod tests { )) .await .unwrap(); - migrate_from_version(&sql_pool, SchemaVersion(3), SchemaVersion(4)) - .await - .expect_err("migration should fail"); + error!( + "{}", + migrate_from_version(&sql_pool, SchemaVersion(3), SchemaVersion(4)) + .await + .expect_err("migration should fail") + ); assert_eq!( sql_migrations::JustSchemaVersion::find_by_statement(raw_statement( r#"SELECT version FROM metadata"#