Fix some external_id issues

- Do not update `externalId` on group updates
   Groups are only updated via the web-vault currently, and those do not
   send the `externalId` value, and thus we need to prevent updating it.
 - Refactored some other ExternalId functions
 - Prevent empty `externalId` on `Collections`
 - Return `externalId` for users

Fixes #3685

src/api/core/organizations.rs

@@ -6,8 +6,7 @@ use serde_json::Value;
 use crate::{
     api::{
         core::{log_event, CipherSyncData, CipherSyncType},
-        ApiResult, EmptyResult, JsonResult, JsonUpcase, JsonUpcaseVec, JsonVec, Notify, NumberOrString, PasswordData,
-        UpdateType,
+        EmptyResult, JsonResult, JsonUpcase, JsonUpcaseVec, JsonVec, Notify, NumberOrString, PasswordData, UpdateType,
     },
     auth::{decode_invite, AdminHeaders, Headers, ManagerHeaders, ManagerHeadersLoose, OwnerHeaders},
     db::{models::*, DbConn},
@@ -468,7 +467,11 @@ async fn post_organization_collection_update(
     }
 
     collection.name = data.Name;
-    collection.external_id = data.ExternalId;
+    collection.external_id = match data.ExternalId {
+        Some(external_id) if !external_id.trim().is_empty() => Some(external_id),
+        _ => None,
+    };
+
     collection.save(&mut conn).await?;
 
     log_event(
@@ -2222,29 +2225,22 @@ struct GroupRequest {
 }
 
 impl GroupRequest {
-    pub fn to_group(&self, organizations_uuid: &str) -> ApiResult<Group> {
-        match self.AccessAll {
-            Some(access_all_value) => Ok(Group::new(
-                organizations_uuid.to_owned(),
-                self.Name.clone(),
-                access_all_value,
-                self.ExternalId.clone(),
-            )),
-            _ => err!("Could not convert GroupRequest to Group, because AccessAll has no value!"),
-        }
+    pub fn to_group(&self, organizations_uuid: &str) -> Group {
+        Group::new(
+            String::from(organizations_uuid),
+            self.Name.clone(),
+            self.AccessAll.unwrap_or(false),
+            self.ExternalId.clone(),
+        )
     }
 
-    pub fn update_group(&self, mut group: Group) -> ApiResult<Group> {
-        match self.AccessAll {
-            Some(access_all_value) => {
-                group.name = self.Name.clone();
-                group.access_all = access_all_value;
-                group.set_external_id(self.ExternalId.clone());
+    pub fn update_group(&self, mut group: Group) -> Group {
+        group.name = self.Name.clone();
+        group.access_all = self.AccessAll.unwrap_or(false);
+        // Group Updates do not support changing the external_id
+        // These input fields are in a disabled state, and can only be updated/added via ldap_import
 
-                Ok(group)
-            }
-            _ => err!("Could not update group, because AccessAll has no value!"),
-        }
+        group
     }
 }
 
@@ -2305,7 +2301,7 @@ async fn post_groups(
     }
 
     let group_request = data.into_inner().data;
-    let group = group_request.to_group(org_id)?;
+    let group = group_request.to_group(org_id);
 
     log_event(
         EventType::GroupCreated as i32,
@@ -2339,7 +2335,7 @@ async fn put_group(
     };
 
     let group_request = data.into_inner().data;
-    let updated_group = group_request.update_group(group)?;
+    let updated_group = group_request.update_group(group);
 
     CollectionGroup::delete_all_by_group(group_id, &mut conn).await?;
     GroupUser::delete_all_by_group(group_id, &mut conn).await?;

src/db/models/group.rs

@@ -94,18 +94,11 @@ impl Group {
     }
 
     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
-        match external_id {
-            Some(external_id) => {
-                if external_id.is_empty() {
-                    self.external_id = None;
-                } else {
-                    self.external_id = Some(external_id)
-                }
-            }
-            None => self.external_id = None,
-        }
+        // Check if external_id is empty. We do not want to have empty strings in the database
+        self.external_id = match external_id {
+            Some(external_id) if !external_id.trim().is_empty() => Some(external_id),
+            _ => None,
+        };
     }
 }
 

src/db/models/organization.rs

@@ -434,6 +434,7 @@ impl UserOrganization {
             "UserId": self.user_uuid,
             "Name": user.name,
             "Email": user.email,
+            "ExternalId": user.external_id,
             "Groups": groups,
             "Collections": collections,
 
@@ -441,7 +442,7 @@ impl UserOrganization {
             "Type": self.atype,
             "AccessAll": self.access_all,
             "TwoFactorEnabled": twofactor_enabled,
-            "ResetPasswordEnrolled":self.reset_password_key.is_some(),
+            "ResetPasswordEnrolled": self.reset_password_key.is_some(),
 
             "Object": "organizationUserUserDetails",
         })