diff --git a/src/api/core/ciphers.rs b/src/api/core/ciphers.rs index aecbe28a..1c89aed7 100644 --- a/src/api/core/ciphers.rs +++ b/src/api/core/ciphers.rs @@ -1924,11 +1924,21 @@ impl CipherSyncData { // Generate a HashMap with the collections_uuid as key and the CollectionGroup record let user_collections_groups: HashMap = if CONFIG.org_groups_enabled() { - CollectionGroup::find_by_user(user_id, conn) - .await - .into_iter() - .map(|collection_group| (collection_group.collections_uuid.clone(), collection_group)) - .collect() + CollectionGroup::find_by_user(user_id, conn).await.into_iter().fold( + HashMap::new(), + |mut combined_permissions, cg| { + combined_permissions + .entry(cg.collections_uuid.clone()) + .and_modify(|existing| { + // Combine permissions: take the most permissive settings. + existing.read_only &= cg.read_only; // false if ANY group allows write + existing.hide_passwords &= cg.hide_passwords; // false if ANY group allows password view + existing.manage |= cg.manage; // true if ANY group allows manage + }) + .or_insert(cg); + combined_permissions + }, + ) } else { HashMap::new() }; diff --git a/src/db/models/cipher.rs b/src/db/models/cipher.rs index 5e8971c8..f8d60505 100644 --- a/src/db/models/cipher.rs +++ b/src/db/models/cipher.rs @@ -609,22 +609,23 @@ impl Cipher { let mut rows: Vec<(bool, bool, bool)> = Vec::new(); if let Some(collections) = cipher_sync_data.cipher_collections.get(&self.uuid) { for collection in collections { - //User permissions + // User permissions if let Some(cu) = cipher_sync_data.user_collections.get(collection) { rows.push((cu.read_only, cu.hide_passwords, cu.manage)); - } - - //Group permissions - if let Some(cg) = cipher_sync_data.user_collections_groups.get(collection) { + // Group permissions + } else if let Some(cg) = cipher_sync_data.user_collections_groups.get(collection) { rows.push((cg.read_only, cg.hide_passwords, cg.manage)); } } } rows } else { - let mut access_flags = self.get_user_collections_access_flags(user_uuid, conn).await; - access_flags.append(&mut self.get_group_collections_access_flags(user_uuid, conn).await); - access_flags + let user_permissions = self.get_user_collections_access_flags(user_uuid, conn).await; + if !user_permissions.is_empty() { + user_permissions + } else { + self.get_group_collections_access_flags(user_uuid, conn).await + } }; if rows.is_empty() { @@ -633,6 +634,9 @@ impl Cipher { } // A cipher can be in multiple collections with inconsistent access flags. + // Also, user permission overrule group permissions + // and only user permissions are returned by the code above. + // // For example, a cipher could be in one collection where the user has // read-only access, but also in another collection where the user has // read/write access. For a flag to be in effect for a cipher, upstream @@ -641,13 +645,15 @@ impl Cipher { // and `hide_passwords` columns. This could ideally be done as part of the // query, but Diesel doesn't support a min() or bool_and() function on // booleans and this behavior isn't portable anyway. + // + // The only exception is for the `manage` flag, that needs a boolean OR! let mut read_only = true; let mut hide_passwords = true; let mut manage = false; for (ro, hp, mn) in rows.iter() { read_only &= ro; hide_passwords &= hp; - manage &= mn; + manage |= mn; } Some((read_only, hide_passwords, manage)) diff --git a/src/db/models/collection.rs b/src/db/models/collection.rs index 4f192a25..c14c5946 100644 --- a/src/db/models/collection.rs +++ b/src/db/models/collection.rs @@ -97,13 +97,13 @@ impl Collection { ( cu.read_only, cu.hide_passwords, - cu.manage || (is_manager && !cu.read_only && !cu.hide_passwords), + is_manager && (cu.manage || (!cu.read_only && !cu.hide_passwords)), ) } else if let Some(cg) = cipher_sync_data.user_collections_groups.get(&self.uuid) { ( cg.read_only, cg.hide_passwords, - cg.manage || (is_manager && !cg.read_only && !cg.hide_passwords), + is_manager && (cg.manage || (!cg.read_only && !cg.hide_passwords)), ) } else { (false, false, false) @@ -114,7 +114,9 @@ impl Collection { } else { match Membership::find_confirmed_by_user_and_org(user_uuid, &self.org_uuid, conn).await { Some(m) if m.has_full_access() => (false, false, m.atype >= MembershipType::Manager), - Some(_) if self.is_manageable_by_user(user_uuid, conn).await => (false, false, true), + Some(m) if m.atype == MembershipType::Manager && self.is_manageable_by_user(user_uuid, conn).await => { + (false, false, true) + } Some(m) => { let is_manager = m.atype == MembershipType::Manager; let read_only = !self.is_writable_by_user(user_uuid, conn).await;