commit 6822e445bbcbe36dbfba0feb0809324625bf9a60
parent 18fbc1ccf66d9242e515107374130baf04e3e769
Author: Daniel García <dani-garcia@users.noreply.github.com>
Date: Sat, 21 Oct 2023 17:58:51 +0200
Merge pull request #3804 from BlackDex/fix-3777
Fix External ID not set during DC Sync
Diffstat:
12 files changed, 75 insertions(+), 40 deletions(-)
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
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
@@ -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
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
@@ -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
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
@@ -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
@@ -56,16 +56,34 @@ async fn ldap_import(data: JsonUpcase<OrgImportData>, 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<OrgImportData>, 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<OrgImportData>, 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<OrgImportData>, 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<OrgImportData>, token: PublicToken, mut co
// Generate a HashSet to quickly verify if a member is listed or not.
let sync_members: HashSet<String> = 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
@@ -31,6 +31,7 @@ db_object! {
pub status: i32,
pub atype: i32,
pub reset_password_key: Option<String>,
+ pub external_id: Option<String>,
}
#[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<String>) -> 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::<UserOrganizationDb>(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<Self> {
+ db_run! {conn: {
+ users_organizations::table
+ .filter(
+ users_organizations::external_id.eq(ext_id)
+ .and(users_organizations::org_uuid.eq(org_uuid))
+ )
+ .first::<UserOrganizationDb>(conn).ok().from_db()
+ }}
+ }
}
impl OrganizationApiKey {
diff --git a/src/db/models/user.rs b/src/db/models/user.rs
@@ -51,7 +51,7 @@ db_object! {
pub avatar_color: Option<String>,
- pub external_id: Option<String>,
+ pub external_id: Option<String>, // 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<String>) {
- //Check if external id is empty. We don't want to have
- //empty strings in the database
- let mut ext_id: Option<String> = 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<Self> {
- db_run! {conn: {
- users::table.filter(users::external_id.eq(id)).first::<UserDb>(conn).ok().from_db()
- }}
- }
-
pub async fn get_all(conn: &mut DbConn) -> Vec<Self> {
db_run! {conn: {
users::table.load::<UserDb>(conn).expect("Error loading users").from_db()
diff --git 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<Text>,
+ external_id -> Nullable<Text>,
}
}
diff --git 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<Text>,
+ external_id -> Nullable<Text>,
}
}
diff --git 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<Text>,
+ external_id -> Nullable<Text>,
}
}
