From 787a645575a0ec8c00a637bbc8872ea54156ee23 Mon Sep 17 00:00:00 2001 From: BlackDex Date: Sat, 16 Nov 2024 15:13:19 +0100 Subject: [PATCH] Fix Org Import duplicate collections 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 57d069b8e0..aa390e5ede 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 7ee6a0895a..2b51a14412 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 d252df9196..eb7fb3296a 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()) {