Require confirmed membership for user access checks, not for admin views (#7763)

src/api/core/ciphers.rs

@@ -2180,8 +2180,15 @@ impl CipherSyncData {
             }
         }
 
+        // Generate a HashMap with the Organization UUID as key and the Membership record
+        let members: HashMap<OrganizationId, Membership> = Membership::find_confirmed_by_user(user_id, conn)
+            .await
+            .into_iter()
+            .map(|m| (m.org_uuid.clone(), m))
+            .collect();
+
         // Generate a list of Cipher UUID's containing a Vec with one or more Attachment records
-        let orgs = Membership::get_orgs_by_user(user_id, conn).await;
+        let orgs: Vec<OrganizationId> = members.keys().cloned().collect();
         let attachments = Attachment::find_all_by_user_and_orgs(user_id, &orgs, conn).await;
         let mut cipher_attachments: HashMap<CipherId, Vec<Attachment>> = HashMap::with_capacity(attachments.len());
         for attachment in attachments {
@@ -2196,13 +2203,6 @@ impl CipherSyncData {
             cipher_collections.entry(cipher).or_default().push(collection);
         }
 
-        // Generate a HashMap with the Organization UUID as key and the Membership record
-        let members: HashMap<OrganizationId, Membership> = Membership::find_confirmed_by_user(user_id, conn)
-            .await
-            .into_iter()
-            .map(|m| (m.org_uuid.clone(), m))
-            .collect();
-
         // Generate a HashMap with the User_Collections UUID as key and the CollectionUser record
         let user_collections: HashMap<CollectionId, CollectionUser> = CollectionUser::find_by_user(user_id, conn)
             .await

src/api/core/organizations.rs

@@ -415,8 +415,9 @@ async fn get_org_collections_details(org_id: OrganizationId, headers: ManagerHea
     let col_users = CollectionUser::find_by_organization_swap_user_uuid_with_member_uuid(&org_id, &conn).await;
     // Generate a HashMap to get the correct MembershipType per user to determine the manage permission
     // We use the uuid instead of the user_uuid here, since that is what is used in CollectionUser
+    // This lists other members for admins, so it must not depend on the membership status
     let membership_type: HashMap<MembershipId, i32> =
-        Membership::find_confirmed_by_org(&org_id, &conn).await.into_iter().map(|m| (m.uuid, m.atype)).collect();
+        Membership::find_by_org(&org_id, &conn).await.into_iter().map(|m| (m.uuid, m.atype)).collect();
 
     // check if current user has full access to the organization (either directly or via any group)
     let has_full_access_to_org = member.has_full_access()
@@ -818,11 +819,9 @@ async fn get_org_collection_detail(
 
             // Generate a HashMap to get the correct MembershipType per user to determine the manage permission
             // We use the uuid instead of the user_uuid here, since that is what is used in CollectionUser
-            let membership_type: HashMap<MembershipId, i32> = Membership::find_confirmed_by_org(&org_id, &conn)
+            // This lists other members for admins, so it must not depend on the membership status
-                .await
+            let membership_type: HashMap<MembershipId, i32> =
-                .into_iter()
+                Membership::find_by_org(&org_id, &conn).await.into_iter().map(|m| (m.uuid, m.atype)).collect();
-                .map(|m| (m.uuid, m.atype))
-                .collect();
 
             let users: Vec<Value> =
                 CollectionUser::find_by_org_and_coll_swap_user_uuid_with_member_uuid(&org_id, &collection.uuid, &conn)
@@ -960,8 +959,9 @@ async fn get_members(
 
     let mut users_json = Vec::new();
     for u in Membership::find_by_org(&org_id, &conn).await {
+        // The user can be a manager instead of an admin, but we've checked above that they have full access
         users_json.push(
-            u.to_json_user_details(
+            u.to_json_details_for_admin(
                 data.include_collections.unwrap_or(false),
                 data.include_groups.unwrap_or(false),
                 &conn,
@@ -1516,7 +1516,9 @@ async fn get_user(
     // In this case, when groups are requested we also need to include collections.
     // Else these will not be shown in the interface, and could lead to missing collections when saved.
     let include_groups = data.include_groups.unwrap_or(false);
-    Ok(Json(user.to_json_user_details(data.include_collections.unwrap_or(include_groups), include_groups, &conn).await))
+    Ok(Json(
+        user.to_json_details_for_admin(data.include_collections.unwrap_or(include_groups), include_groups, &conn).await,
+    ))
 }
 
 #[derive(Deserialize)]
@@ -1835,6 +1837,10 @@ async fn post_org_import(
     if org_id != headers.membership.org_uuid {
         err!("Organization not found", "Organization id's do not match");
     }
+    // OrgMemberHeaders also allows invited and accepted members, which are not allowed to import
+    if headers.membership.status != MembershipStatus::Confirmed as i32 {
+        err!("You need to be a Member of the Organization to call this endpoint")
+    }
     let data: ImportData = data.into_inner();
 
     // Validate the import before continuing

src/db/models/cipher.rs

@@ -1003,6 +1003,7 @@ impl Cipher {
                             .eq(ciphers_collections::collection_uuid)
                             .and(collections_groups::groups_uuid.eq(groups::uuid))),
                     )
+                    .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32))
                     .filter(
                         users_organizations::access_all
                             .eq(true) // User has access all
@@ -1034,6 +1035,7 @@ impl Cipher {
                             .eq(ciphers_collections::collection_uuid)
                             .and(users_collections::user_uuid.eq(user_uuid.clone()))),
                     )
+                    .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32))
                     .filter(
                         users_organizations::access_all
                             .eq(true) // User has access all
@@ -1078,6 +1080,7 @@ impl Cipher {
                             .eq(ciphers_collections::collection_uuid)
                             .and(collections_groups::groups_uuid.eq(groups::uuid))),
                     )
+                    .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32))
                     .filter(
                         users_organizations::access_all
                             .eq(true) // User has access all
@@ -1110,6 +1113,7 @@ impl Cipher {
                             .eq(ciphers_collections::collection_uuid)
                             .and(users_collections::user_uuid.eq(user_uuid.clone()))),
                     )
+                    .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32))
                     .filter(
                         users_organizations::access_all
                             .eq(true) // User has access all
@@ -1161,6 +1165,7 @@ impl Cipher {
                 .or_filter(users_organizations::atype.le(MembershipType::Admin as i32)) // User is admin or owner
                 .or_filter(groups::access_all.eq(true)) //Access via group
                 .or_filter(collections_groups::collections_uuid.is_not_null()) //Access via group
+                .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32))
                 .select(ciphers_collections::all_columns)
                 .distinct()
                 .load::<(CipherId, CollectionId)>(conn)

src/db/models/collection.rs

@@ -367,6 +367,7 @@ impl Collection {
                             .and(collections_groups::collections_uuid.eq(collections::uuid))),
                     )
                     .filter(collections::uuid.eq(uuid))
+                    .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32))
                     .filter(
                         users_collections::collection_uuid
                             .eq(uuid)
@@ -406,6 +407,7 @@ impl Collection {
                             .and(users_organizations::user_uuid.eq(user_uuid))),
                     )
                     .filter(collections::uuid.eq(uuid))
+                    .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32))
                     .filter(users_collections::collection_uuid.eq(uuid).or(
                         // Directly accessed collection
                         users_organizations::access_all.eq(true).or(
@@ -430,7 +432,8 @@ impl Collection {
                     .inner_join(
                         users_organizations::table.on(collections::org_uuid
                             .eq(users_organizations::org_uuid)
-                            .and(users_organizations::user_uuid.eq(user_uuid.clone()))),
+                            .and(users_organizations::user_uuid.eq(user_uuid.clone()))
+                            .and(users_organizations::status.eq(MembershipStatus::Confirmed as i32))),
                     )
                     .left_join(
                         users_collections::table.on(users_collections::collection_uuid
@@ -476,7 +479,8 @@ impl Collection {
                     .inner_join(
                         users_organizations::table.on(collections::org_uuid
                             .eq(users_organizations::org_uuid)
-                            .and(users_organizations::user_uuid.eq(user_uuid.clone()))),
+                            .and(users_organizations::user_uuid.eq(user_uuid.clone()))
+                            .and(users_organizations::status.eq(MembershipStatus::Confirmed as i32))),
                     )
                     .left_join(
                         users_collections::table.on(users_collections::collection_uuid
@@ -527,6 +531,7 @@ impl Collection {
                         .and(collections_groups::collections_uuid.eq(collections::uuid))),
                 )
                 .filter(collections::uuid.eq(&self.uuid))
+                .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32))
                 .filter(
                     users_collections::collection_uuid
                         .eq(&self.uuid)
@@ -586,6 +591,7 @@ impl Collection {
                         .and(collections_groups::collections_uuid.eq(collections::uuid))),
                 )
                 .filter(collections::uuid.eq(&uuid))
+                .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32))
                 .filter(
                     users_collections::collection_uuid
                         .eq(&uuid)
@@ -650,6 +656,7 @@ impl Collection {
                         .and(collections_groups::collections_uuid.eq(collections::uuid))),
                 )
                 .filter(collections::org_uuid.eq(&org_uuid))
+                .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32))
                 .filter(
                     // Manage permission on a collection assigned directly or via a group.
                     users_collections::manage.eq(true).or(collections_groups::manage.eq(true)),
@@ -819,10 +826,18 @@ impl CollectionUser {
         .await
     }
 
+    // Only returns the collections of organizations the user is a confirmed member of
     pub async fn find_by_user(user_uuid: &UserId, conn: &DbConn) -> Vec<Self> {
         conn.run(move |conn| {
             users_collections::table
+                .inner_join(collections::table.on(collections::uuid.eq(users_collections::collection_uuid)))
+                .inner_join(
+                    users_organizations::table.on(users_organizations::org_uuid
+                        .eq(collections::org_uuid)
+                        .and(users_organizations::user_uuid.eq(users_collections::user_uuid))),
+                )
                 .filter(users_collections::user_uuid.eq(user_uuid))
+                .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32))
                 .select(users_collections::all_columns)
                 .load::<Self>(conn)
                 .expect("Error loading users_collections")

src/db/models/group.rs

@@ -249,6 +249,7 @@ impl Group {
                         .and(groups::organizations_uuid.eq(users_organizations::org_uuid))),
                 )
                 .filter(users_organizations::user_uuid.eq(user_uuid))
+                .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32))
                 .filter(groups::access_all.eq(true))
                 .select(groups::organizations_uuid)
                 .distinct()
@@ -387,6 +388,7 @@ impl CollectionGroup {
                         .and(collections::org_uuid.eq(groups::organizations_uuid))),
                 )
                 .filter(users_organizations::user_uuid.eq(user_uuid))
+                .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32))
                 .select(collections_groups::all_columns)
                 .load::<Self>(conn)
                 .expect("Error loading user collection groups")

src/db/models/organization.rs

@@ -1,7 +1,4 @@
-use std::{
+use std::cmp::Ordering;
-    cmp::Ordering,
-    collections::{HashMap, HashSet},
-};
 
 use chrono::{NaiveDateTime, Utc};
 use derive_more::{AsRef, Deref, Display, From};
@@ -24,8 +21,8 @@ use crate::{
 use macros::UuidFromParam;
 
 use super::{
-    Cipher, CipherId, Collection, CollectionGroup, CollectionId, CollectionUser, Group, GroupId, GroupUser, OrgPolicy,
+    Cipher, CipherId, Collection, CollectionId, CollectionUser, Group, GroupId, GroupUser, OrgPolicy, OrgPolicyType,
-    OrgPolicyType, TwoFactor, User, UserId,
+    TwoFactor, User, UserId,
 };
 
 #[derive(Identifiable, Queryable, Insertable, AsChangeset)]
@@ -536,7 +533,13 @@ impl Membership {
         })
     }
 
-    pub async fn to_json_user_details(&self, include_collections: bool, include_groups: bool, conn: &DbConn) -> Value {
+    // Used by admins to view other members, so nothing here may depend on this member's status
+    pub async fn to_json_details_for_admin(
+        &self,
+        include_collections: bool,
+        include_groups: bool,
+        conn: &DbConn,
+    ) -> Value {
         let user = User::find_by_uuid(&self.user_uuid, conn).await.unwrap();
 
         // Because BitWarden want the status to be -1 for revoked users we need to catch that here.
@@ -559,52 +562,23 @@ impl Membership {
 
         // Check if a user is in a group which has access to all collections
         // If that is the case, we should not return individual collections!
+        // This is used by admins to view other members, so it must not depend on the membership status
         let full_access_group =
-            CONFIG.org_groups_enabled() && Group::is_in_full_access_group(&self.user_uuid, &self.org_uuid, conn).await;
+            CONFIG.org_groups_enabled() && GroupUser::has_full_access_by_member(&self.org_uuid, &self.uuid, conn).await;
 
         // If collections are to be included, only include them if the user does not have full access via a group or defined to the user it self
+        // Only the collections assigned directly are returned, the ones assigned via a group are returned via a special group endpoint
         let collections: Vec<Value> = if include_collections && !(full_access_group || self.access_all) {
-            // Get all collections for the user here already to prevent more queries
+            CollectionUser::find_by_organization_and_user_uuid(&self.org_uuid, &self.user_uuid, conn)
-            let cu: HashMap<CollectionId, CollectionUser> =
-                CollectionUser::find_by_organization_and_user_uuid(&self.org_uuid, &self.user_uuid, conn)
-                    .await
-                    .into_iter()
-                    .map(|cu| (cu.collection_uuid.clone(), cu))
-                    .collect();
-
-            // Get all collection groups for this user to prevent there inclusion
-            let cg: HashSet<CollectionId> = CollectionGroup::find_by_user(&self.user_uuid, conn)
                 .await
                 .into_iter()
-                .map(|cg| cg.collections_uuid)
+                .map(|cu| {
-                .collect();
+                    json!({
-
+                        "id": cu.collection_uuid,
-            Collection::find_by_organization_and_user_uuid(&self.org_uuid, &self.user_uuid, conn)
+                        "readOnly": cu.read_only,
-                .await
+                        "hidePasswords": cu.hide_passwords,
-                .into_iter()
+                        "manage": cu.manage || (self.atype == MembershipType::Manager && !cu.read_only && !cu.hide_passwords),
-                .filter_map(|c| {
+                    })
-                    let (read_only, hide_passwords, manage) = if self.has_full_access() {
-                        (false, false, self.atype >= MembershipType::Manager)
-                    } else if let Some(cu) = cu.get(&c.uuid) {
-                        (
-                            cu.read_only,
-                            cu.hide_passwords,
-                            cu.manage || (self.atype == MembershipType::Manager && !cu.read_only && !cu.hide_passwords),
-                        )
-                    // If previous checks failed it might be that this user has access via a group, but we should not return those elements here
-                    // Those are returned via a special group endpoint
-                    } else if cg.contains(&c.uuid) {
-                        return None;
-                    } else {
-                        (true, true, false)
-                    };
-
-                    Some(json!({
-                        "id": c.uuid,
-                        "readOnly": read_only,
-                        "hidePasswords": hide_passwords,
-                        "manage": manage,
-                    }))
                 })
                 .collect()
         } else {
@@ -1032,17 +1006,6 @@ impl Membership {
         .await
     }
 
-    pub async fn get_orgs_by_user(user_uuid: &UserId, conn: &DbConn) -> Vec<OrganizationId> {
-        conn.run(move |conn| {
-            users_organizations::table
-                .filter(users_organizations::user_uuid.eq(user_uuid))
-                .select(users_organizations::org_uuid)
-                .load::<OrganizationId>(conn)
-                .unwrap_or_default()
-        })
-        .await
-    }
-
     pub async fn find_by_user_and_policy(user_uuid: &UserId, policy_type: OrgPolicyType, conn: &DbConn) -> Vec<Self> {
         conn.run(move |conn| {
             users_organizations::table
@@ -1126,6 +1089,7 @@ impl Membership {
                         .and(ciphers::organization_uuid.eq(users_organizations::org_uuid.nullable()))),
                 )
                 .filter(users_organizations::user_uuid.eq(user_uuid))
+                .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32))
                 .filter(
                     users_organizations::atype.eq_any(vec![MembershipType::Owner as i32, MembershipType::Admin as i32]),
                 )