From cdfdc6ff4f61a7495cd70609c0d9098ff10b55a4 Mon Sep 17 00:00:00 2001 From: Mathijs van Veluw Date: Sun, 17 Nov 2024 21:33:23 +0100 Subject: [PATCH] Fix Org Import duplicate collections (#5200) This fixes an issue with collections be duplicated same as was an issue with folders. Also made some optimizations by using HashSet where possible and device the Vec/Hash capacity. And instead of passing objects only use the UUID which was the only value we needed. Also found an issue with importing a personal export via the Org import where folders are used. Since Org's do not use folder we needed to clear those out, same as Bitwarden does. Fixes #5193 Signed-off-by: BlackDex --- src/api/core/ciphers.rs | 10 ++++---- src/api/core/organizations.rs | 39 ++++++++++++++++------------- src/api/core/two_factor/duo_oidc.rs | 5 +--- 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/api/core/ciphers.rs b/src/api/core/ciphers.rs index 57d069b8..aa390e5e 100644 --- a/src/api/core/ciphers.rs +++ b/src/api/core/ciphers.rs @@ -222,7 +222,7 @@ pub struct CipherData { // Id is optional as it is included only in bulk share pub id: Option, // Folder id is not included in import - folder_id: Option, + pub folder_id: Option, // TODO: Some of these might appear all the time, no need for Option #[serde(alias = "organizationID")] pub organization_id: Option, @@ -585,11 +585,11 @@ async fn post_ciphers_import( Cipher::validate_cipher_data(&data.ciphers)?; // Read and create the folders - let existing_folders: Vec = - Folder::find_by_user(&headers.user.uuid, &mut conn).await.into_iter().map(|f| f.uuid).collect(); + let existing_folders: HashSet> = + Folder::find_by_user(&headers.user.uuid, &mut conn).await.into_iter().map(|f| Some(f.uuid)).collect(); let mut folders: Vec = Vec::with_capacity(data.folders.len()); for folder in data.folders.into_iter() { - let folder_uuid = if folder.id.is_some() && existing_folders.contains(folder.id.as_ref().unwrap()) { + let folder_uuid = if existing_folders.contains(&folder.id) { folder.id.unwrap() } else { let mut new_folder = Folder::new(headers.user.uuid.clone(), folder.name); @@ -601,8 +601,8 @@ async fn post_ciphers_import( } // Read the relations between folders and ciphers + // Ciphers can only be in one folder at the same time let mut relations_map = HashMap::with_capacity(data.folder_relationships.len()); - for relation in data.folder_relationships { relations_map.insert(relation.key, relation.value); } diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 7ee6a089..2b51a144 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -11,7 +11,6 @@ use crate::{ }, auth::{decode_invite, AdminHeaders, ClientVersion, Headers, ManagerHeaders, ManagerHeadersLoose, OwnerHeaders}, db::{models::*, DbConn}, - error::Error, mail, util::{convert_json_key_lcase_first, NumberOrString}, CONFIG, @@ -127,6 +126,7 @@ struct NewCollectionData { name: String, groups: Vec, users: Vec, + id: Option, external_id: Option, } @@ -1598,40 +1598,43 @@ async fn post_org_import( // TODO: See if we can optimize the whole cipher adding/importing and prevent duplicate code and checks. Cipher::validate_cipher_data(&data.ciphers)?; - let mut collections = Vec::new(); + let existing_collections: HashSet> = + Collection::find_by_organization(&org_id, &mut conn).await.into_iter().map(|c| (Some(c.uuid))).collect(); + let mut collections: Vec = Vec::with_capacity(data.collections.len()); for coll in data.collections { - let collection = Collection::new(org_id.clone(), coll.name, coll.external_id); - if collection.save(&mut conn).await.is_err() { - collections.push(Err(Error::new("Failed to create Collection", "Failed to create Collection"))); + let collection_uuid = if existing_collections.contains(&coll.id) { + coll.id.unwrap() } else { - collections.push(Ok(collection)); - } + let new_collection = Collection::new(org_id.clone(), coll.name, coll.external_id); + new_collection.save(&mut conn).await?; + new_collection.uuid + }; + + collections.push(collection_uuid); } // Read the relations between collections and ciphers - let mut relations = Vec::new(); + // Ciphers can be in multiple collections at the same time + let mut relations = Vec::with_capacity(data.collection_relationships.len()); for relation in data.collection_relationships { relations.push((relation.key, relation.value)); } let headers: Headers = headers.into(); - let mut ciphers = Vec::new(); - for cipher_data in data.ciphers { + let mut ciphers: Vec = Vec::with_capacity(data.ciphers.len()); + for mut cipher_data in data.ciphers { + // Always clear folder_id's via an organization import + cipher_data.folder_id = None; let mut cipher = Cipher::new(cipher_data.r#type, cipher_data.name.clone()); update_cipher_from_data(&mut cipher, cipher_data, &headers, None, &mut conn, &nt, UpdateType::None).await.ok(); - ciphers.push(cipher); + ciphers.push(cipher.uuid); } // Assign the collections for (cipher_index, coll_index) in relations { - let cipher_id = &ciphers[cipher_index].uuid; - let coll = &collections[coll_index]; - let coll_id = match coll { - Ok(coll) => coll.uuid.as_str(), - Err(_) => err!("Failed to assign to collection"), - }; - + let cipher_id = &ciphers[cipher_index]; + let coll_id = &collections[coll_index]; CollectionCipher::save(cipher_id, coll_id, &mut conn).await?; } diff --git a/src/api/core/two_factor/duo_oidc.rs b/src/api/core/two_factor/duo_oidc.rs index d252df91..eb7fb329 100644 --- a/src/api/core/two_factor/duo_oidc.rs +++ b/src/api/core/two_factor/duo_oidc.rs @@ -211,10 +211,7 @@ impl DuoClient { nonce, }; - let token = match self.encode_duo_jwt(jwt_payload) { - Ok(token) => token, - Err(e) => return Err(e), - }; + let token = self.encode_duo_jwt(jwt_payload)?; let authz_endpoint = format!("https://{}/oauth/v1/authorize", self.api_host); let mut auth_url = match Url::parse(authz_endpoint.as_str()) {