Merge pull request dani-garcia#3804 from BlackDex/fix-3777

Fix External ID not set during DC Sync

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;

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;

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;

src/api/core/public.rs

@@ -56,26 +56,43 @@ 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 {
             // If user is not part of the organization
             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)

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 {

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()

src/db/schemas/mysql/schema.rs

@@ -228,6 +228,7 @@ table! {
         status -> Integer,
         atype -> Integer,
         reset_password_key -> Nullable<Text>,
+        external_id -> Nullable<Text>,
     }
 }
 

src/db/schemas/postgresql/schema.rs

@@ -228,6 +228,7 @@ table! {
         status -> Integer,
         atype -> Integer,
         reset_password_key -> Nullable<Text>,
+        external_id -> Nullable<Text>,
     }
 }
 

src/db/schemas/sqlite/schema.rs

@@ -228,6 +228,7 @@ table! {
         status -> Integer,
         atype -> Integer,
         reset_password_key -> Nullable<Text>,
+        external_id -> Nullable<Text>,
     }
 }