From 25865efd799d0321428e1dff1489b834e1206021 Mon Sep 17 00:00:00 2001 From: Richy Date: Fri, 25 Jul 2025 20:58:41 +0200 Subject: [PATCH] fix: resolve group permission conflicts with multiple groups (#6017) * fix: resolve group permission conflicts with multiple groups When a user belonged to multiple groups with different permissions for the same collection, only the permissions from one group were applied instead of combining them properly. This caused users to see incorrect access levels when initially viewing collection items. The fix combines permissions from all user groups by taking the most permissive settings: - read_only: false if ANY group allows write access - hide_passwords: false if ANY group allows password viewing - manage: true if ANY group allows management This ensures users immediately see the correct permissions when opening collection entries, matching the behavior after editing and saving. * Update src/api/core/ciphers.rs Co-authored-by: Mathijs van Veluw * fix: format * fix: restrict collection manage permissions to managers only Prevent users from getting logged out when they have manage permissions by only allowing manage permissions for MembershipType::Manager and higher roles. * refactor: cipher permission logic to prioritize user access Updated permission checks to return user collection permissions if available, otherwise fallback to group permissions. Clarified comments to indicate user permissions overrule group permissions and corrected the logic for the 'manage' flag to use boolean OR instead of AND. --------- Co-authored-by: Mathijs van Veluw --- src/api/core/ciphers.rs | 20 +++++++++++++++----- src/db/models/cipher.rs | 24 +++++++++++++++--------- src/db/models/collection.rs | 8 +++++--- 3 files changed, 35 insertions(+), 17 deletions(-) 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;