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

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,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();
+                // Only revoke a user if it is not the last confirmed owner
-                user_org.save(&mut conn).await?;
+                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 {
+            let restored = user_org.restore();
-                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_external_id_and_org(ext_id, &org_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());
-                        let mut group_user = GroupUser::new(group_uuid.clone(), user_org.uuid.clone());
+                    group_user.save(&mut conn).await?;
-                        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) =
+            if let Some(ref user_external_id) = user_org.external_id {
-                User::find_by_uuid(&user_org.user_uuid, &mut conn).await.map(|u| u.external_id)
+                if !sync_members.contains(user_external_id) {
-            {
-                if user_external_id.is_some() && !sync_members.contains(&user_external_id.unwrap()) {
                     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>,
     }
 }