From 28ed5d42a9adf532b6c8c58a7d3c5dfc784548b1 Mon Sep 17 00:00:00 2001 From: abhishandy Date: Tue, 4 Aug 2026 15:08:47 -0400 Subject: [PATCH] Fix revoked org members retaining access to org ciphers The cipher access restriction queries (direct collection access, group collection access, and full access via groups) never checked the organization membership status. Since users_collections and groups_users rows are kept when a membership is revoked, revoked (and not yet confirmed) members kept read, write, delete, attachment and collection-move access to org ciphers via the direct-by-UUID endpoints. Require a confirmed membership in the cipher's organization in all three queries, matching the behavior of the sync queries. --- src/db/models/cipher.rs | 16 +++++++++++++++- src/db/models/group.rs | 5 ++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/db/models/cipher.rs b/src/db/models/cipher.rs index eed5041d..e6e5f849 100644 --- a/src/db/models/cipher.rs +++ b/src/db/models/cipher.rs @@ -665,16 +665,27 @@ impl Cipher { } async fn get_user_collections_access_flags(&self, user_uuid: &UserId, conn: &DbConn) -> Vec<(bool, bool, bool)> { + let user_uuid = user_uuid.to_string(); 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. + // The user must have a confirmed membership in the cipher's + // organization, since users_collections rows are kept when a + // membership is revoked or not confirmed yet. ciphers::table .filter(ciphers::uuid.eq(&self.uuid)) .inner_join(ciphers_collections::table.on(ciphers::uuid.eq(ciphers_collections::cipher_uuid))) + .inner_join( + users_organizations::table.on(ciphers::organization_uuid + .eq(users_organizations::org_uuid.nullable()) + .and(users_organizations::user_uuid.eq(user_uuid)) + .and(users_organizations::status.eq(MembershipStatus::Confirmed as i32))), + ) .inner_join( users_collections::table.on(ciphers_collections::collection_uuid .eq(users_collections::collection_uuid) - .and(users_collections::user_uuid.eq(user_uuid))), + // Only allow collection access via the confirmed membership. + .and(users_organizations::user_uuid.eq(users_collections::user_uuid))), ) .select((users_collections::read_only, users_collections::hide_passwords, users_collections::manage)) .load::<(bool, bool, bool)>(conn) @@ -705,6 +716,9 @@ impl Cipher { .and(groups::organizations_uuid.eq(users_organizations::org_uuid))), ) .filter(users_organizations::user_uuid.eq(user_uuid)) + // Only allow group access via a confirmed membership, since + // groups_users rows are kept when a membership is revoked. + .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) .select((collections_groups::read_only, collections_groups::hide_passwords, collections_groups::manage)) .load::<(bool, bool, bool)>(conn) .expect("Error getting group access restrictions") diff --git a/src/db/models/group.rs b/src/db/models/group.rs index 37037de6..d1cee8e5 100644 --- a/src/db/models/group.rs +++ b/src/db/models/group.rs @@ -13,7 +13,7 @@ use crate::{ }; use macros::UuidFromParam; -use super::{CollectionId, Membership, MembershipId, OrganizationId, User, UserId}; +use super::{CollectionId, Membership, MembershipId, MembershipStatus, OrganizationId, User, UserId}; #[derive(Identifiable, Queryable, Insertable, AsChangeset)] #[diesel(table_name = groups)] @@ -276,6 +276,9 @@ impl Group { .and(users_organizations::org_uuid.eq(groups::organizations_uuid))), ) .filter(users_organizations::user_uuid.eq(user_uuid)) + // Only allow full access via a confirmed membership, since + // groups_users rows are kept when a membership is revoked. + .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)