From 18d66474e01ce931cdf9c84b6ebc6f91e996ec18 Mon Sep 17 00:00:00 2001 From: BlackDex Date: Sat, 2 Sep 2023 23:57:43 +0200 Subject: [PATCH] Fix External ID not set during DC Sync While working on the fix I realised the location where the `external_id` is stored was wrong. It was stored in the `users` table, but it actually should have been stored in the `users_organizations` table. This will move the column to the right table. It will not move the values of the `external_id` column, because if there are more organizations, there is no way to really know which organization it is linked to. Setups using the Directory Connector can clear the sync cache, and sync again, that will store all the `external_id` values at the right location. Also changed the function to revoke,restore an org-user and set_external_id to return a boolean. It will state if the value has been changed or not, and if not, we can prevent a `save` call to the database. The `users` table is not changed to remove the `external_id` column, thi to prevent issue when users want to revert back to an earlier version for some reason. We can do this after a few minor release i think. Fixes #3777 --- .../down.sql | 0 .../up.sql | 2 + .../down.sql | 0 .../up.sql | 2 + .../down.sql | 0 .../up.sql | 2 + src/api/core/public.rs | 48 ++++++++++++------- src/db/models/organization.rs | 36 ++++++++++++-- src/db/models/user.rs | 22 +-------- src/db/schemas/mysql/schema.rs | 1 + src/db/schemas/postgresql/schema.rs | 1 + src/db/schemas/sqlite/schema.rs | 1 + 12 files changed, 75 insertions(+), 40 deletions(-) create mode 100644 migrations/mysql/2023-09-02-212336_move_user_external_id/down.sql create mode 100644 migrations/mysql/2023-09-02-212336_move_user_external_id/up.sql create mode 100644 migrations/postgresql/2023-09-02-212336_move_user_external_id/down.sql create mode 100644 migrations/postgresql/2023-09-02-212336_move_user_external_id/up.sql create mode 100644 migrations/sqlite/2023-09-02-212336_move_user_external_id/down.sql create mode 100644 migrations/sqlite/2023-09-02-212336_move_user_external_id/up.sql diff --git a/migrations/mysql/2023-09-02-212336_move_user_external_id/down.sql b/migrations/mysql/2023-09-02-212336_move_user_external_id/down.sql new file mode 100644 index 00000000..e69de29b diff --git a/migrations/mysql/2023-09-02-212336_move_user_external_id/up.sql b/migrations/mysql/2023-09-02-212336_move_user_external_id/up.sql new file mode 100644 index 00000000..5adacf95 --- /dev/null +++ b/migrations/mysql/2023-09-02-212336_move_user_external_id/up.sql @@ -0,0 +1,2 @@ +ALTER TABLE users_organizations +ADD COLUMN external_id TEXT; diff --git a/migrations/postgresql/2023-09-02-212336_move_user_external_id/down.sql b/migrations/postgresql/2023-09-02-212336_move_user_external_id/down.sql new file mode 100644 index 00000000..e69de29b diff --git a/migrations/postgresql/2023-09-02-212336_move_user_external_id/up.sql b/migrations/postgresql/2023-09-02-212336_move_user_external_id/up.sql new file mode 100644 index 00000000..5adacf95 --- /dev/null +++ b/migrations/postgresql/2023-09-02-212336_move_user_external_id/up.sql @@ -0,0 +1,2 @@ +ALTER TABLE users_organizations +ADD COLUMN external_id TEXT; diff --git a/migrations/sqlite/2023-09-02-212336_move_user_external_id/down.sql b/migrations/sqlite/2023-09-02-212336_move_user_external_id/down.sql new file mode 100644 index 00000000..e69de29b diff --git a/migrations/sqlite/2023-09-02-212336_move_user_external_id/up.sql b/migrations/sqlite/2023-09-02-212336_move_user_external_id/up.sql new file mode 100644 index 00000000..07b451a1 --- /dev/null +++ b/migrations/sqlite/2023-09-02-212336_move_user_external_id/up.sql @@ -0,0 +1,2 @@ +-- Add the external_id to the users_organizations table +ALTER TABLE "users_organizations" ADD COLUMN "external_id" TEXT; diff --git a/src/api/core/public.rs b/src/api/core/public.rs index ab30635c..74f79a3e 100644 --- a/src/api/core/public.rs +++ b/src/api/core/public.rs @@ -56,16 +56,34 @@ async fn ldap_import(data: JsonUpcase, token: PublicToken, mut co if let Some(mut user_org) = UserOrganization::find_by_email_and_org(&user_data.Email, &org_id, &mut conn).await { - user_org.revoke(); - user_org.save(&mut conn).await?; - } + // Only revoke a user if it is not the last confirmed owner + let revoked = if user_org.atype == UserOrgType::Owner + && user_org.status == UserOrgStatus::Confirmed as i32 + { + if UserOrganization::count_confirmed_by_org_and_type(&org_id, UserOrgType::Owner, &mut conn).await + <= 1 + { + warn!("Can't revoke the last owner"); + false + } else { + user_org.revoke() + } + } else { + user_org.revoke() + }; + let ext_modified = user_org.set_external_id(Some(user_data.ExternalId.clone())); + if revoked || ext_modified { + user_org.save(&mut conn).await?; + } + } // If user is part of the organization, restore it } else if let Some(mut user_org) = UserOrganization::find_by_email_and_org(&user_data.Email, &org_id, &mut conn).await { - if user_org.status < UserOrgStatus::Revoked as i32 { - user_org.restore(); + let restored = user_org.restore(); + let ext_modified = user_org.set_external_id(Some(user_data.ExternalId.clone())); + if restored || ext_modified { user_org.save(&mut conn).await?; } } else { @@ -73,9 +91,8 @@ async fn ldap_import(data: JsonUpcase, token: PublicToken, mut co let user = match User::find_by_mail(&user_data.Email, &mut conn).await { Some(user) => user, // exists in vaultwarden None => { - // doesn't exist in vaultwarden + // User does not exist yet let mut new_user = User::new(user_data.Email.clone()); - new_user.set_external_id(Some(user_data.ExternalId.clone())); new_user.save(&mut conn).await?; if !CONFIG.mail_enabled() { @@ -92,6 +109,7 @@ async fn ldap_import(data: JsonUpcase, token: PublicToken, mut co }; let mut new_org_user = UserOrganization::new(user.uuid.clone(), org_id.clone()); + new_org_user.set_external_id(Some(user_data.ExternalId.clone())); new_org_user.access_all = false; new_org_user.atype = UserOrgType::User as i32; new_org_user.status = user_org_status; @@ -132,12 +150,10 @@ async fn ldap_import(data: JsonUpcase, token: PublicToken, mut co GroupUser::delete_all_by_group(&group_uuid, &mut conn).await?; for ext_id in &group_data.MemberExternalIds { - if let Some(user) = User::find_by_external_id(ext_id, &mut conn).await { - if let Some(user_org) = UserOrganization::find_by_user_and_org(&user.uuid, &org_id, &mut conn).await - { - let mut group_user = GroupUser::new(group_uuid.clone(), user_org.uuid.clone()); - group_user.save(&mut conn).await?; - } + if let Some(user_org) = UserOrganization::find_by_external_id_and_org(ext_id, &org_id, &mut conn).await + { + let mut group_user = GroupUser::new(group_uuid.clone(), user_org.uuid.clone()); + group_user.save(&mut conn).await?; } } } @@ -150,10 +166,8 @@ async fn ldap_import(data: JsonUpcase, token: PublicToken, mut co // Generate a HashSet to quickly verify if a member is listed or not. let sync_members: HashSet = data.Members.into_iter().map(|m| m.ExternalId).collect(); for user_org in UserOrganization::find_by_org(&org_id, &mut conn).await { - if let Some(user_external_id) = - User::find_by_uuid(&user_org.user_uuid, &mut conn).await.map(|u| u.external_id) - { - if user_external_id.is_some() && !sync_members.contains(&user_external_id.unwrap()) { + if let Some(ref user_external_id) = user_org.external_id { + if !sync_members.contains(user_external_id) { if user_org.atype == UserOrgType::Owner && user_org.status == UserOrgStatus::Confirmed as i32 { // Removing owner, check that there is at least one other confirmed owner if UserOrganization::count_confirmed_by_org_and_type(&org_id, UserOrgType::Owner, &mut conn) diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index 3db7e12d..534dbce8 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -31,6 +31,7 @@ db_object! { pub status: i32, pub atype: i32, pub reset_password_key: Option, + pub external_id: Option, } #[derive(Identifiable, Queryable, Insertable, AsChangeset)] @@ -208,19 +209,37 @@ impl UserOrganization { status: UserOrgStatus::Accepted as i32, atype: UserOrgType::User as i32, reset_password_key: None, + external_id: None, } } - pub fn restore(&mut self) { + pub fn restore(&mut self) -> bool { if self.status < UserOrgStatus::Accepted as i32 { self.status += ACTIVATE_REVOKE_DIFF; + return true; } + false } - pub fn revoke(&mut self) { + pub fn revoke(&mut self) -> bool { if self.status > UserOrgStatus::Revoked as i32 { self.status -= ACTIVATE_REVOKE_DIFF; + return true; } + false + } + + pub fn set_external_id(&mut self, external_id: Option) -> bool { + //Check if external id is empty. We don't want to have + //empty strings in the database + if self.external_id != external_id { + self.external_id = match external_id { + Some(external_id) if !external_id.is_empty() => Some(external_id), + _ => None, + }; + return true; + } + false } } @@ -434,7 +453,7 @@ impl UserOrganization { "UserId": self.user_uuid, "Name": user.name, "Email": user.email, - "ExternalId": user.external_id, + "ExternalId": self.external_id, "Groups": groups, "Collections": collections, @@ -778,6 +797,17 @@ impl UserOrganization { .load::(conn).expect("Error loading user organizations").from_db() }} } + + pub async fn find_by_external_id_and_org(ext_id: &str, org_uuid: &str, conn: &mut DbConn) -> Option { + db_run! {conn: { + users_organizations::table + .filter( + users_organizations::external_id.eq(ext_id) + .and(users_organizations::org_uuid.eq(org_uuid)) + ) + .first::(conn).ok().from_db() + }} + } } impl OrganizationApiKey { diff --git a/src/db/models/user.rs b/src/db/models/user.rs index 596d2d75..1475d637 100644 --- a/src/db/models/user.rs +++ b/src/db/models/user.rs @@ -51,7 +51,7 @@ db_object! { pub avatar_color: Option, - pub external_id: Option, + pub external_id: Option, // Todo: Needs to be removed in the future, this is not used anymore. } #[derive(Identifiable, Queryable, Insertable)] @@ -129,7 +129,7 @@ impl User { avatar_color: None, - external_id: None, + external_id: None, // Todo: Needs to be removed in the future, this is not used anymore. } } @@ -154,18 +154,6 @@ impl User { matches!(self.api_key, Some(ref api_key) if crate::crypto::ct_eq(api_key, key)) } - pub fn set_external_id(&mut self, external_id: Option) { - //Check if external id is empty. We don't want to have - //empty strings in the database - let mut ext_id: Option = None; - if let Some(external_id) = external_id { - if !external_id.is_empty() { - ext_id = Some(external_id); - } - } - self.external_id = ext_id; - } - /// Set the password hash generated /// And resets the security_stamp. Based upon the allow_next_route the security_stamp will be different. /// @@ -392,12 +380,6 @@ impl User { }} } - pub async fn find_by_external_id(id: &str, conn: &mut DbConn) -> Option { - db_run! {conn: { - users::table.filter(users::external_id.eq(id)).first::(conn).ok().from_db() - }} - } - pub async fn get_all(conn: &mut DbConn) -> Vec { db_run! {conn: { users::table.load::(conn).expect("Error loading users").from_db() diff --git a/src/db/schemas/mysql/schema.rs b/src/db/schemas/mysql/schema.rs index 65036a08..f1a001fd 100644 --- a/src/db/schemas/mysql/schema.rs +++ b/src/db/schemas/mysql/schema.rs @@ -228,6 +228,7 @@ table! { status -> Integer, atype -> Integer, reset_password_key -> Nullable, + external_id -> Nullable, } } diff --git a/src/db/schemas/postgresql/schema.rs b/src/db/schemas/postgresql/schema.rs index 9d2fb55e..64786fb9 100644 --- a/src/db/schemas/postgresql/schema.rs +++ b/src/db/schemas/postgresql/schema.rs @@ -228,6 +228,7 @@ table! { status -> Integer, atype -> Integer, reset_password_key -> Nullable, + external_id -> Nullable, } } diff --git a/src/db/schemas/sqlite/schema.rs b/src/db/schemas/sqlite/schema.rs index 9d2fb55e..64786fb9 100644 --- a/src/db/schemas/sqlite/schema.rs +++ b/src/db/schemas/sqlite/schema.rs @@ -228,6 +228,7 @@ table! { status -> Integer, atype -> Integer, reset_password_key -> Nullable, + external_id -> Nullable, } }