Fix cross-tenant and revoke authorization bypasses from security audit

Addresses the confirmed High/Medium findings of the granular-collection-
permissions security audit:

- H-1: bind direct (non-sync) cipher access to a confirmed membership in the
  cipher's organization, and harden the user/group collection access-flag
  queries to require org consistency plus confirmed status. Revoked/invited
  members can no longer reach ciphers via stale assignment rows.
- H-2: validate group ids against the org before mutating group membership in
  edit_member, and reject cross-org group<->membership links in GroupUser::save
  and Group::is_in_full_access_group.
- H-3: fully pre-validate collections/groups/users in bulk-access and collection
  create before any mutation, and reject cross-org collection<->group links in
  CollectionGroup::save.
- M-1: require Manage Users for the full member list and Manage Users/Groups for
  group details (new ManageUsersOrGroupsHeaders guard).
- M-2: validate the whole bulk-access request before the destructive
  delete/replace so an invalid element can't leave partial state behind.
- M-3: bounds-validate import collection relationships before writing and
  propagate cipher-save errors instead of discarding them.

src/api/core/organizations.rs

@@ -13,8 +13,8 @@ use crate::{
     },
     auth::{
         AdminHeaders, CollectionDeleteHeaders, CollectionReadHeaders, Headers, ManageGroupsHeaders,
-        ManagePoliciesHeaders, ManageUsersHeaders, ManagerHeaders, ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders,
+        ManagePoliciesHeaders, ManageUsersHeaders, ManageUsersOrGroupsHeaders, ManagerHeaders, ManagerHeadersLoose,
-        decode_invite,
+        OrgMemberHeaders, OwnerHeaders, decode_invite,
     },
     db::{
         DbConn,
@@ -548,6 +548,20 @@ async fn post_organization_collections(
     let data: FullCollectionData = data.into_inner();
     data.validate(&org_id, &conn).await?;
 
+    // Security (audit H-3): validate every referenced group and user against this organization
+    // *before* creating the collection or any assignment, so a foreign-tenant group can't be
+    // attached to the new collection and no partial state is left behind on rejection.
+    for group in &data.groups {
+        if Group::find_by_uuid_and_org(&group.id, &org_id, &conn).await.is_none() {
+            err!("Group not found in this organization")
+        }
+    }
+    for user in &data.users {
+        if Membership::find_by_uuid_and_org(&user.id, &org_id, &conn).await.is_none() {
+            err!("User is not part of organization")
+        }
+    }
+
     let collection = Collection::new(org_id.clone(), data.name, data.external_id);
     collection.save(&conn).await?;
 
@@ -622,8 +636,25 @@ async fn post_bulk_access_collections(
         err!("You don't have permission to modify collection access")
     }
 
-    for col_id in data.collection_ids {
+    // Security (audit H-3) and atomicity (audit M-2): validate the whole request against this
-        let Some(collection) = Collection::find_by_uuid_and_org(&col_id, &org_id, &conn).await else {
+    // organization *before* mutating anything. Every collection must exist in the org and be
+    // manageable by the caller, and every referenced group and user must belong to the org. Only
+    // once the entire request is known-valid do we begin the destructive delete/replace of
+    // assignments, so a foreign-tenant group can never be linked and a later invalid element can no
+    // longer leave earlier collections with their assignments already wiped.
+    for group in &data.groups {
+        if Group::find_by_uuid_and_org(&group.id, &org_id, &conn).await.is_none() {
+            err!("Group not found in this organization")
+        }
+    }
+    for user in &data.users {
+        if Membership::find_by_uuid_and_org(&user.id, &org_id, &conn).await.is_none() {
+            err!("User is not part of organization")
+        }
+    }
+    let mut collections = Vec::with_capacity(data.collection_ids.len());
+    for col_id in &data.collection_ids {
+        let Some(collection) = Collection::find_by_uuid_and_org(col_id, &org_id, &conn).await else {
             err!("Collection not found")
         };
 
@@ -631,6 +662,12 @@ async fn post_bulk_access_collections(
             err!("Collection not found", "The current user isn't a manager for this collection")
         }
 
+        collections.push(collection);
+    }
+
+    for collection in collections {
+        let col_id = &collection.uuid;
+
         // update collection modification date
         collection.save(&conn).await?;
 
@@ -645,14 +682,14 @@ async fn post_bulk_access_collections(
         )
         .await;
 
-        CollectionGroup::delete_all_by_collection(&col_id, &org_id, &conn).await?;
+        CollectionGroup::delete_all_by_collection(col_id, &org_id, &conn).await?;
         for group in &data.groups {
             CollectionGroup::new(col_id.clone(), group.id.clone(), group.read_only, group.hide_passwords, group.manage)
                 .save(&org_id, &conn)
                 .await?;
         }
 
-        CollectionUser::delete_all_by_collection(&col_id, &conn).await?;
+        CollectionUser::delete_all_by_collection(col_id, &conn).await?;
         for user in &data.users {
             let Some(member) = Membership::find_by_uuid_and_org(&user.id, &org_id, &conn).await else {
                 err!("User is not part of organization")
@@ -662,7 +699,7 @@ async fn post_bulk_access_collections(
                 continue;
             }
 
-            CollectionUser::save(&member.user_uuid, &col_id, user.read_only, user.hide_passwords, user.manage, &conn)
+            CollectionUser::save(&member.user_uuid, col_id, user.read_only, user.hide_passwords, user.manage, &conn)
                 .await?;
         }
     }
@@ -1024,10 +1061,14 @@ struct GetOrgUserData {
 async fn get_members(
     data: GetOrgUserData,
     org_id: OrganizationId,
-    headers: ManagerHeadersLoose,
+    // Security (audit M-1): the full member list exposes each member's PII, 2FA/enrollment status,
+    // permission flags and (optionally) collection/group assignments. Reading it requires the
+    // 'Manage Users' permission (or Admin/Owner), matching Bitwarden. Members who only need to
+    // reference other users (e.g. the collection dialog) use the member-readable mini-details.
+    headers: ManageUsersHeaders,
     conn: DbConn,
 ) -> JsonResult {
-    if org_id != headers.membership.org_uuid {
+    if org_id != headers.org_id {
         err!("Organization not found", "Organization id's do not match");
     }
     let mut users_json = Vec::new();
@@ -1812,6 +1853,16 @@ async fn edit_member(
         };
 
     if caller_can_manage_groups {
+        // Security (audit H-2): validate that every requested group belongs to this organization
+        // *before* mutating any group membership. Otherwise a caller could link the member to a
+        // group of a foreign tenant (e.g. an access-all group), which the direct cipher-access
+        // checks would then honor. Fail closed on the whole request if any group is foreign.
+        for group_id in data.groups.iter().flatten() {
+            if Group::find_by_uuid_and_org(group_id, &org_id, &conn).await.is_none() {
+                err!("Group not found in this organization")
+            }
+        }
+
         if caller_can_manage_collections {
             // Caller may grant/revoke collection access via groups: full replace.
             GroupUser::delete_all_by_member(&member_to_edit.uuid, &conn).await?;
@@ -2046,6 +2097,22 @@ 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)?;
 
+    // Robustness/DoS (audit M-3): validate every collection<->cipher relationship index against the
+    // import payload *before* creating any collection or cipher. `key` indexes into `ciphers` and
+    // `value` into `collections`; an out-of-range index would otherwise cause an out-of-bounds panic
+    // when the relations are applied below — a 500 (or a process abort under panic="abort") that
+    // happens after rows have already been written, leaving partial state behind.
+    let import_cipher_count = data.ciphers.len();
+    let import_collection_count = data.collections.len();
+    for relation in &data.collection_relationships {
+        if relation.key >= import_cipher_count || relation.value >= import_collection_count {
+            err!(
+                "Invalid collection relationship",
+                "A collection relationship references a non-existent cipher or collection"
+            )
+        }
+    }
+
     let existing_collections: HashSet<Option<CollectionId>> =
         Collection::find_by_organization(&org_id, &conn).await.into_iter().map(|c| Some(c.uuid)).collect();
     let mut collections: Vec<CollectionId> = Vec::with_capacity(data.collections.len());
@@ -2095,6 +2162,9 @@ async fn post_org_import(
         // 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());
+        // Propagate cipher-save failures instead of silently discarding them (audit M-3): a
+        // discarded error would still push the cipher id and let a relationship reference a cipher
+        // that was never persisted. This matches Bitwarden's all-or-nothing import semantics.
         update_cipher_from_data(
             &mut cipher,
             cipher_data,
@@ -2104,15 +2174,16 @@ async fn post_org_import(
             &nt,
             UpdateType::None,
         )
-        .await
+        .await?;
-        .ok();
         ciphers.push(cipher.uuid);
     }
 
-    // Assign the collections
+    // Assign the collections. Indices were bounds-validated above, but use `.get()` here as well so
+    // any future drift fails closed with an error instead of panicking.
     for (cipher_index, col_index) in relations {
-        let cipher_id = &ciphers[cipher_index];
+        let (Some(cipher_id), Some(col_id)) = (ciphers.get(cipher_index), collections.get(col_index)) else {
-        let col_id = &collections[col_index];
+            err!("Invalid collection relationship", "A collection relationship references a non-existent cipher or collection")
+        };
         CollectionCipher::save(cipher_id, col_id, &conn).await?;
     }
 
@@ -2696,15 +2767,7 @@ async fn restore_member_impl(
     Ok(())
 }
 
-async fn get_groups_data(
+async fn get_groups_data(details: bool, org_id: OrganizationId, conn: DbConn) -> JsonResult {
-    details: bool,
-    org_id: OrganizationId,
-    headers: ManagerHeadersLoose,
-    conn: DbConn,
-) -> JsonResult {
-    if org_id != headers.membership.org_uuid {
-        err!("Organization not found", "Organization id's do not match");
-    }
     let groups: Vec<Value> = if CONFIG.org_groups_enabled() {
         let groups = Group::find_by_organization(&org_id, &conn).await;
         let mut groups_json = Vec::with_capacity(groups.len());
@@ -2732,14 +2795,25 @@ async fn get_groups_data(
     })))
 }
 
+// The plain group list (id, name, externalId) stays member-readable: the web vault needs it to
+// render group names, and it exposes no access mappings.
 #[get("/organizations/<org_id>/groups")]
 async fn get_groups(org_id: OrganizationId, headers: ManagerHeadersLoose, conn: DbConn) -> JsonResult {
-    get_groups_data(false, org_id, headers, conn).await
+    if org_id != headers.membership.org_uuid {
+        err!("Organization not found", "Organization id's do not match");
+    }
+    get_groups_data(false, org_id, conn).await
 }
 
+// Security (audit M-1): group *details* expose accessAll, external IDs and collection mappings, so
+// reading them requires the 'Manage Users' or 'Manage Groups' permission (or Admin/Owner), matching
+// Bitwarden's ReadAll/ReadAllWithAccess authorization.
 #[get("/organizations/<org_id>/groups/details", rank = 1)]
-async fn get_groups_details(org_id: OrganizationId, headers: ManagerHeadersLoose, conn: DbConn) -> JsonResult {
+async fn get_groups_details(org_id: OrganizationId, headers: ManageUsersOrGroupsHeaders, conn: DbConn) -> JsonResult {
-    get_groups_data(true, org_id, headers, conn).await
+    if org_id != headers.org_id {
+        err!("Organization not found", "Organization id's do not match");
+    }
+    get_groups_data(true, org_id, conn).await
 }
 
 #[derive(Deserialize)]

src/auth.rs

@@ -746,6 +746,15 @@ impl OrgHeaders {
     fn can_manage_policies(&self) -> bool {
         self.is_confirmed() && (self.membership_type >= MembershipType::Admin || self.membership.has_manage_policies())
     }
+    // Reading the full member/group *details* (PII, 2FA status, permission flags, access mappings)
+    // requires the ability to manage users or groups, matching Bitwarden's `ReadAll`/`ReadAllWithAccess`
+    // authorization. Basic member mini-details and the plain group list remain member-readable.
+    fn can_manage_users_or_groups(&self) -> bool {
+        self.is_confirmed()
+            && (self.membership_type >= MembershipType::Admin
+                || self.membership.has_manage_users()
+                || self.membership.has_manage_groups())
+    }
 }
 
 // org_id is usually the second path param ("/organizations/<org_id>"),
@@ -923,6 +932,11 @@ generate_manage_headers!(
     can_manage_policies,
     "You need the 'Manage Policies' permission, or to be an Admin or Owner, to call this endpoint"
 );
+generate_manage_headers!(
+    ManageUsersOrGroupsHeaders,
+    can_manage_users_or_groups,
+    "You need the 'Manage Users' or 'Manage Groups' permission, or to be an Admin or Owner, to call this endpoint"
+);
 
 // col_id is usually the fourth path param ("/organizations/<org_id>/collections/<col_id>"),
 // but there could be cases where it is a query value.

src/db/models/cipher.rs

@@ -604,6 +604,24 @@ impl Cipher {
         cipher_sync_data: Option<&CipherSyncData>,
         conn: &DbConn,
     ) -> Option<(bool, bool, bool)> {
+        // Security: central fail-closed check binding cipher -> organization -> confirmed membership.
+        //
+        // In the direct (non-sync) authorization path an organization cipher is only accessible to a
+        // user who has a *confirmed* membership in that same organization. This denies access to
+        // members whose collection/group assignment rows still exist after they were revoked (or are
+        // still only invited/accepted), and to cross-organization collection/group assignments that
+        // another code path might have persisted. Without it, the queries below would keep granting
+        // access from those stale or cross-tenant rows (security audit findings H-1, H-2, H-3).
+        //
+        // The sync path (cipher_sync_data is Some) is intentionally left to the caller: it is built
+        // only from confirmed memberships and evaluated below against that cached data.
+        if cipher_sync_data.is_none()
+            && let Some(ref org_uuid) = self.organization_uuid
+            && Membership::find_confirmed_by_user_and_org(user_uuid, org_uuid, conn).await.is_none()
+        {
+            return None;
+        }
+
         // Check whether this cipher is directly owned by the user, or is in
         // a collection that the user has full access to. If so, there are no
         // access restrictions.
@@ -669,16 +687,34 @@ impl Cipher {
     }
 
     async fn get_user_collections_access_flags(&self, user_uuid: &UserId, conn: &DbConn) -> Vec<(bool, bool, bool)> {
+        let cipher_uuid = self.uuid.clone();
+        let user_uuid = user_uuid.clone();
         conn.run(move |conn| {
             // Check whether this cipher is in any collections accessible to the
             // user. If so, retrieve the access flags for each collection.
+            //
+            // Security: bind the assignment to a *confirmed* membership in the same organization as
+            // both the cipher and the collection. Without this, a `users_collections` row left behind
+            // after a revoke, or an assignment pointing at a collection in a different organization,
+            // would keep granting access (defense in depth for audit findings H-1 and H-3).
             ciphers::table
-                .filter(ciphers::uuid.eq(&self.uuid))
+                .filter(ciphers::uuid.eq(cipher_uuid))
                 .inner_join(ciphers_collections::table.on(ciphers::uuid.eq(ciphers_collections::cipher_uuid)))
+                .inner_join(
+                    collections::table.on(collections::uuid
+                        .eq(ciphers_collections::collection_uuid)
+                        .and(collections::org_uuid.nullable().eq(ciphers::organization_uuid))),
+                )
                 .inner_join(
                     users_collections::table.on(ciphers_collections::collection_uuid
                         .eq(users_collections::collection_uuid)
-                        .and(users_collections::user_uuid.eq(user_uuid))),
+                        .and(users_collections::user_uuid.eq(user_uuid.clone()))),
+                )
+                .inner_join(
+                    users_organizations::table.on(users_organizations::user_uuid
+                        .eq(user_uuid)
+                        .and(users_organizations::org_uuid.eq(collections::org_uuid))
+                        .and(users_organizations::status.eq(MembershipStatus::Confirmed as i32))),
                 )
                 .select((users_collections::read_only, users_collections::hide_passwords, users_collections::manage))
                 .load::<(bool, bool, bool)>(conn)
@@ -691,9 +727,16 @@ impl Cipher {
         if !CONFIG.org_groups_enabled() {
             return Vec::new();
         }
+        let cipher_uuid = self.uuid.clone();
+        let user_uuid = user_uuid.clone();
         conn.run(move |conn| {
+            // Security: bind the group assignment to a *confirmed* membership and require that the
+            // cipher, the collection, the group and the membership all belong to the same
+            // organization. The `collections` join in particular prevents a cross-organization
+            // collection<->group assignment from granting access to a foreign organization's ciphers
+            // (defense in depth for audit findings H-1, H-2 and H-3).
             ciphers::table
-                .filter(ciphers::uuid.eq(&self.uuid))
+                .filter(ciphers::uuid.eq(cipher_uuid))
                 .inner_join(ciphers_collections::table.on(ciphers::uuid.eq(ciphers_collections::cipher_uuid)))
                 .inner_join(
                     collections_groups::table
@@ -701,13 +744,21 @@ impl Cipher {
                 )
                 .inner_join(groups_users::table.on(groups_users::groups_uuid.eq(collections_groups::groups_uuid)))
                 .inner_join(
-                    users_organizations::table.on(users_organizations::uuid.eq(groups_users::users_organizations_uuid)),
+                    users_organizations::table.on(users_organizations::uuid
+                        .eq(groups_users::users_organizations_uuid)
+                        .and(users_organizations::status.eq(MembershipStatus::Confirmed as i32))),
                 )
                 .inner_join(
                     groups::table.on(groups::uuid
                         .eq(collections_groups::groups_uuid)
                         .and(groups::organizations_uuid.eq(users_organizations::org_uuid))),
                 )
+                .inner_join(
+                    collections::table.on(collections::uuid
+                        .eq(ciphers_collections::collection_uuid)
+                        .and(collections::org_uuid.eq(groups::organizations_uuid))
+                        .and(collections::org_uuid.nullable().eq(ciphers::organization_uuid))),
+                )
                 .filter(users_organizations::user_uuid.eq(user_uuid))
                 .select((collections_groups::read_only, collections_groups::hide_passwords, collections_groups::manage))
                 .load::<(bool, bool, bool)>(conn)

src/db/models/group.rs

@@ -13,7 +13,7 @@ use crate::{
 };
 use macros::UuidFromParam;
 
-use super::{CollectionId, Membership, MembershipId, OrganizationId, User, UserId};
+use super::{Collection, CollectionId, Membership, MembershipId, MembershipStatus, OrganizationId, User, UserId};
 
 #[derive(Identifiable, Queryable, Insertable, AsChangeset)]
 #[diesel(table_name = groups)]
@@ -257,6 +257,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()
@@ -268,12 +269,19 @@ impl Group {
 
     pub async fn is_in_full_access_group(user_uuid: &UserId, org_uuid: &OrganizationId, conn: &DbConn) -> bool {
         conn.run(move |conn| {
+            // Security: the membership linked through `groups_users` must itself belong to the same
+            // organization as the group and must be confirmed. Otherwise a cross-organization
+            // `groups_users` row (a member of org A linked to an access-all group of org B) would let
+            // that member pass as having full access to org B (audit finding H-2).
             groups::table
                 .inner_join(groups_users::table.on(groups_users::groups_uuid.eq(groups::uuid)))
                 .inner_join(
-                    users_organizations::table.on(users_organizations::uuid.eq(groups_users::users_organizations_uuid)),
+                    users_organizations::table.on(users_organizations::uuid
+                        .eq(groups_users::users_organizations_uuid)
+                        .and(users_organizations::org_uuid.eq(groups::organizations_uuid))),
                 )
                 .filter(users_organizations::user_uuid.eq(user_uuid))
+                .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32))
                 .filter(groups::organizations_uuid.eq(org_uuid))
                 .filter(groups::access_all.eq(true))
                 .select(groups::access_all)
@@ -319,6 +327,17 @@ impl Group {
 
 impl CollectionGroup {
     pub async fn save(&mut self, org_uuid: &OrganizationId, conn: &DbConn) -> EmptyResult {
+        // Security (audit H-3): never persist a cross-organization link between a collection and a
+        // group. Both must belong to the organization this assignment is scoped to; otherwise a
+        // caller could attach a foreign-tenant group to this organization's collection and thereby
+        // grant that group's members access to it. This is a defense-in-depth guard so no route can
+        // create such a link even if it fails to validate its inputs.
+        if Collection::find_by_uuid_and_org(&self.collections_uuid, org_uuid, conn).await.is_none()
+            || Group::find_by_uuid_and_org(&self.groups_uuid, org_uuid, conn).await.is_none()
+        {
+            err!("Collection and group must belong to the same organization")
+        }
+
         let group_users = GroupUser::find_by_group(&self.groups_uuid, org_uuid, conn).await;
         for group_user in group_users {
             group_user.update_user_revision(conn).await;
@@ -493,6 +512,18 @@ impl CollectionGroup {
 
 impl GroupUser {
     pub async fn save(&mut self, conn: &DbConn) -> EmptyResult {
+        // Security (audit H-2): never persist a cross-organization link between a group and a
+        // membership. The group must belong to the same organization as the membership; otherwise a
+        // caller could grant a member of one organization full access to another organization's
+        // collections through an access-all group. This is a defense-in-depth guard so no route can
+        // create such a link even if it fails to validate its inputs.
+        let Some(member) = Membership::find_by_uuid(&self.users_organizations_uuid, conn).await else {
+            err!("Member not found while assigning to group")
+        };
+        if Group::find_by_uuid_and_org(&self.groups_uuid, &member.org_uuid, conn).await.is_none() {
+            err!("Group and member must belong to the same organization")
+        }
+
         self.update_user_revision(conn).await;
 
         db_run! { conn: