From a8de8c78200ef0b78e28679da1f2da18a16d586e Mon Sep 17 00:00:00 2001 From: Tom <83423411+tom27052006@users.noreply.github.com> Date: Fri, 13 Nov 2026 20:24:59 +0100 Subject: [PATCH] [PR 3] Add collection permissions for Custom Roles (#7951) * Add Custom Roles collection permissions * Return NotFound for denied collection access * Grant Manage on imported collections to every Custom member --- src/api/admin.rs | 6 +- src/api/core/ciphers.rs | 17 +- src/api/core/organizations.rs | 397 +++++++++++++----- src/auth.rs | 191 ++++++--- src/db/mod.rs | 65 +++ src/db/models/cipher.rs | 37 +- src/db/models/collection.rs | 52 ++- src/db/models/mod.rs | 2 + src/db/models/organization.rs | 294 +++++++++---- .../templates/scss/vaultwarden.scss.hbs | 2 +- 10 files changed, 795 insertions(+), 268 deletions(-) diff --git a/src/api/admin.rs b/src/api/admin.rs index 40df44ae..62004d50 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -597,9 +597,11 @@ async fn update_membership_type(data: Json, token: AdminToke } } - // A Manager only becomes a Custom member if that changes nothing they may do + // A Manager only becomes a Custom member if that changes nothing they may do. Here they would keep the permissions + // they are shown with, the admin panel can't give up their access to all collections, so that is refused. if new_type == MembershipType::Custom - && let Some(loss) = member_to_edit.custom_role_conversion_loss(&conn).await + && let Some(loss) = + member_to_edit.custom_role_conversion_loss(&member_to_edit.legacy_manager_permissions(), &conn).await { err!(loss) } diff --git a/src/api/core/ciphers.rs b/src/api/core/ciphers.rs index 7bbabcc0..410c7ab5 100644 --- a/src/api/core/ciphers.rs +++ b/src/api/core/ciphers.rs @@ -1185,7 +1185,7 @@ async fn update_collections_by_scope( let current = HashSet::::from_iter(match scope { CipherAccessScope::User => cipher.get_collections(headers.user.uuid.clone(), &conn).await, CipherAccessScope::OrganizationAdmin => { - cipher.get_admin_collections(headers.user.uuid.clone(), &conn).await + cipher.get_admin_collections(headers.user.uuid.clone(), scope, &conn).await } }); // The admin route accepts Edit Any for all collections of this cipher's organization. @@ -1241,11 +1241,15 @@ async fn update_collections_by_scope( &conn, ) .await; - let sync_type = match scope { - CipherAccessScope::User => CipherSyncType::User, - CipherAccessScope::OrganizationAdmin => CipherSyncType::Organization, + let response = match scope { + CipherAccessScope::User => { + cipher.to_json(&headers.host, &headers.user.uuid, None, CipherSyncType::User, &conn).await? + } + CipherAccessScope::OrganizationAdmin => { + cipher.to_json_org_admin_collections(&headers.host, &headers.user.uuid, &conn).await? + } }; - Ok(Json(cipher.to_json(&headers.host, &headers.user.uuid, None, sync_type, &conn).await?)) + Ok(Json(response)) } #[derive(Deserialize)] @@ -2907,8 +2911,9 @@ mod custom_role_tests { let admin = cipher.to_json_org_admin("http://localhost", &member.user_uuid, &conn).await.unwrap(); assert_eq!(personal["collectionIds"], json!([collection.uuid])); let admin_ids = admin["collectionIds"].as_array().unwrap(); - assert_eq!(admin_ids.len(), 1); + assert_eq!(admin_ids.len(), 2); assert!(admin_ids.contains(&json!(collection.uuid))); + assert!(admin_ids.contains(&json!(hidden.uuid))); // Exercise PR2's SQL authorization independently of Custom permission support added in PR3. let sql = format!("DELETE FROM users_collections WHERE user_uuid='{}'", user.uuid); conn.run(move |c| c.batch_execute(&sql)).await.unwrap(); diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 2460ae93..5b6f5cb0 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -11,7 +11,10 @@ use crate::{ ApiResult, EmptyResult, JsonResult, Notify, PasswordOrOtpData, UpdateType, core::{CipherSyncData, CipherSyncType, accept_org_invite, log_event, two_factor}, }, - auth::{AdminHeaders, Headers, ManagerHeaders, ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders, decode_invite}, + auth::{ + AdminHeaders, CollectionAction, CollectionDeleteHeaders, CollectionReadHeaders, Headers, ManagerHeaders, + ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders, custom_member_may, decode_invite, + }, db::{ DbConn, models::{ @@ -365,12 +368,24 @@ async fn get_org_collections(org_id: OrganizationId, headers: ManagerHeadersLoos err!("Organization not found", "Organization id's do not match"); } - if !headers.membership.has_full_access() { + let collections = if headers.membership.can_view_all_collections() { + get_org_collections_impl(&org_id, &conn).await + } else if headers.membership.has_type(MembershipType::Custom) { + // A Custom member gets the collections they may manage + let manageable = + Collection::find_explicitly_manageable_by_user(&org_id, &headers.membership.user_uuid, &conn).await; + Collection::find_by_organization(&org_id, &conn) + .await + .iter() + .filter(|collection| manageable.contains(&collection.uuid)) + .map(Collection::to_json) + .collect::() + } else { err_code!("Resource not found.", "User does not have full access", Status::NotFound.code); - } + }; Ok(Json(json!({ - "data": get_org_collections_impl(&org_id, &conn).await, + "data": collections, "object": "list", "continuationToken": null, }))) @@ -397,6 +412,12 @@ async fn get_org_collections_details(org_id: OrganizationId, headers: ManagerHea // 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() || (CONFIG.org_groups_enabled() && GroupUser::has_full_access_by_member(&org_id, &member.uuid, &conn).await); + // A Custom member gets every collection when they may see all of them, else the ones they may manage + let custom_collections = if member.has_type(MembershipType::Custom) && !member.can_view_all_collections() { + Some(Collection::find_explicitly_manageable_by_user(&org_id, &member.user_uuid, &conn).await) + } else { + None + }; // Get all admins, owners and managers who can manage/access all // Those are currently not listed in the col_users but need to be listed too. @@ -422,7 +443,12 @@ async fn get_org_collections_details(org_id: OrganizationId, headers: ManagerHea && GroupUser::has_access_to_collection_by_member(&col.uuid, &member.uuid, &conn).await); // If the user is a manager, and is not assigned to this collection, skip this and continue with the next collection - if !assigned { + let listed = if member.has_type(MembershipType::Custom) { + custom_collections.as_ref().is_none_or(|manageable| manageable.contains(&col.uuid)) + } else { + assigned + }; + if !listed { continue; } @@ -482,7 +508,10 @@ async fn post_organization_collections( let data: FullCollectionData = data.into_inner(); data.validate(&org_id, &conn).await?; - if headers.membership.atype == MembershipType::Manager && !headers.membership.access_all { + if (headers.membership.atype == MembershipType::Manager && !headers.membership.access_all) + || (headers.membership.has_type(MembershipType::Custom) + && !headers.membership.has_custom_permission(CustomPermission::CreateNewCollections)) + { err!("You don't have permission to create collections") } @@ -565,7 +594,12 @@ async fn post_bulk_access_collections( err!("Collection not found") }; - if !collection.is_manageable_by_user(&headers.membership.user_uuid, &conn).await { + let manageable = if headers.membership.has_type(MembershipType::Custom) { + custom_member_may(CollectionAction::Edit, &headers.membership, &collection.uuid, &conn).await + } else { + collection.is_manageable_by_user(&headers.membership.user_uuid, &conn).await + }; + if !manageable { err!("Collection not found", "The current user isn't a manager for this collection") } @@ -678,7 +712,7 @@ async fn put_organization_collection_update( async fn delete_organization_collection_impl( org_id: &OrganizationId, col_id: &CollectionId, - headers: &ManagerHeaders, + headers: &CollectionDeleteHeaders, conn: &DbConn, ) -> EmptyResult { if org_id != &headers.org_id { @@ -704,7 +738,7 @@ async fn delete_organization_collection_impl( async fn delete_organization_collection( org_id: OrganizationId, col_id: CollectionId, - headers: ManagerHeaders, + headers: CollectionDeleteHeaders, conn: DbConn, ) -> EmptyResult { delete_organization_collection_impl(&org_id, &col_id, &headers, &conn).await @@ -730,7 +764,7 @@ async fn bulk_delete_organization_collections( let collections = data.ids; - let headers = ManagerHeaders::from_loose(headers, &collections, &conn).await?; + let headers = CollectionDeleteHeaders::from_loose(headers, &collections, &conn).await?; for col_id in collections { delete_organization_collection_impl(&org_id, &col_id, &headers, &conn).await?; @@ -742,23 +776,28 @@ async fn bulk_delete_organization_collections( async fn get_org_collection_detail( org_id: OrganizationId, col_id: CollectionId, - headers: ManagerHeaders, + headers: CollectionReadHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id { err!("Organization not found", "Organization id's do not match"); } - match Collection::find_by_uuid_and_user(&col_id, headers.user.uuid.clone(), &conn).await { + let Some(member) = Membership::find_by_user_and_org(&headers.user.uuid, &org_id, &conn).await else { + err!("User is not part of organization") + }; + // A Custom member may be allowed to read a collection they are not assigned to + let collection = if member.has_type(MembershipType::Custom) { + Collection::find_by_uuid_and_org(&col_id, &org_id, &conn).await + } else { + Collection::find_by_uuid_and_user(&col_id, headers.user.uuid.clone(), &conn).await + }; + match collection { None => err!("Collection not found"), Some(collection) => { if collection.org_uuid != org_id { err!("Collection is not owned by organization") } - let Some(member) = Membership::find_by_user_and_org(&headers.user.uuid, &org_id, &conn).await else { - err!("User is not part of organization") - }; - let groups: Vec = if CONFIG.org_groups_enabled() { CollectionGroup::find_by_collection(&collection.uuid, &conn) .await @@ -807,7 +846,7 @@ async fn get_org_collection_detail( async fn get_collection_users( org_id: OrganizationId, col_id: CollectionId, - headers: ManagerHeaders, + headers: CollectionReadHeaders, conn: DbConn, ) -> JsonResult { if org_id != headers.org_id { @@ -1053,7 +1092,13 @@ async fn send_invite( let Some(requested_type) = MembershipType::from_str(raw_type) else { err!("Invalid type") }; - let new_type = MembershipType::requested(requested_type, None, custom_roles_enabled()); + // Checked before anything is created, so a rejected permission leaves nothing behind + let custom_permissions = if requested_type == MembershipType::Custom && custom_roles_enabled() { + Some(CustomPermission::requested(&data.permissions)?) + } else { + None + }; + let new_type = MembershipType::requested(requested_type, None, custom_permissions.as_ref()); if new_type != MembershipType::User && headers.membership_type != MembershipType::Owner { err!("Only Owners can invite Managers, Admins or Owners") @@ -1069,13 +1114,6 @@ async fn send_invite( && data.permissions.get("deleteAnyCollection") == Some(&json!(true)) && data.permissions.get("createNewCollections") == Some(&json!(true))); - // Checked before anything is created, so a rejected permission leaves nothing behind - let custom_permissions = if new_type == MembershipType::Custom { - Some(CustomPermission::requested(&data.permissions)?) - } else { - None - }; - let mut user_created: bool; for email in &data.emails { let mut member_status = MembershipStatus::Invited as i32; @@ -1498,6 +1536,11 @@ async fn get_org_user_mini_details(org_id: OrganizationId, headers: ManagerHeade if org_id != headers.membership.org_uuid { err!("Organization not found", "Organization id's do not match"); } + if headers.membership.has_type(MembershipType::Custom) + && !may_assign_collection_access(&headers.membership, &conn).await + { + err_code!("Resource not found.", "User does not have access", Status::NotFound.code); + } let mut members_json = Vec::new(); for m in Membership::find_by_org(&org_id, &conn).await { members_json.push(m.to_json_mini_details(&conn).await); @@ -1566,7 +1609,20 @@ async fn edit_member( let Some(mut member_to_edit) = Membership::find_by_uuid_and_org(&member_id, &org_id, &conn).await else { err!("The specified user isn't member of the organization") }; - let new_type = MembershipType::requested(requested_type, Some(&member_to_edit), custom_roles_enabled()); + // While Custom roles are disabled, a Custom member keeps their stored permissions untouched + let custom_permissions = if requested_type == MembershipType::Custom && custom_roles_enabled() { + Some(CustomPermission::requested(&data.permissions)?) + } else { + None + }; + let new_type = MembershipType::requested(requested_type, Some(&member_to_edit), custom_permissions.as_ref()); + // A Manager whose permissions are changed only becomes a Custom member if that keeps what they may do + if new_type == MembershipType::Custom + && let Some(permissions) = &custom_permissions + && let Some(loss) = member_to_edit.custom_role_conversion_loss(permissions, &conn).await + { + err!(loss) + } // HACK: This converts the Custom role which has the `Manage all collections` box checked into an access_all flag // Since the parent checkbox is not sent to the server we need to check and verify the child checkboxes @@ -1578,13 +1634,6 @@ async fn edit_member( && data.permissions.get("deleteAnyCollection") == Some(&json!(true)) && data.permissions.get("createNewCollections") == Some(&json!(true))); - // While Custom roles are disabled, a Custom member keeps their stored permissions untouched - let custom_permissions = if new_type == MembershipType::Custom && custom_roles_enabled() { - Some(CustomPermission::requested(&data.permissions)?) - } else { - None - }; - if new_type != member_to_edit.atype && (member_to_edit.atype >= MembershipType::Admin || new_type >= MembershipType::Admin) && headers.membership_type != MembershipType::Owner @@ -1608,7 +1657,7 @@ async fn edit_member( member_to_edit.set_type(new_type); member_to_edit.access_all = access_all; - if let Some(permissions) = &custom_permissions { + if let Some(permissions) = custom_permissions.as_ref().filter(|_| new_type == MembershipType::Custom) { member_to_edit.set_custom_permissions(permissions); } @@ -1839,6 +1888,46 @@ struct RelationsData { value: usize, } +fn validate_org_import_relationships(data: &ImportData) -> ApiResult> { + let mut relations = Vec::with_capacity(data.collection_relationships.len()); + let mut seen = HashSet::new(); + for relation in &data.collection_relationships { + if relation.key >= data.ciphers.len() || relation.value >= data.collections.len() { + err!(Compact, "Invalid collection relationship") + } + if seen.insert((relation.key, relation.value)) { + relations.push((relation.key, relation.value)); + } + } + Ok(relations) +} + +async fn validate_org_import_collections( + imported: &[ImportCollectionData], + org_id: &OrganizationId, + membership: &Membership, + conn: &DbConn, +) -> ApiResult> { + let existing: HashMap = + Collection::find_by_organization(org_id, conn).await.into_iter().map(|c| (c.uuid.clone(), c)).collect(); + for col in imported { + if let Some(collection) = col.id.as_ref().and_then(|id| existing.get(id)) { + if membership.atype < MembershipType::Admin + && !membership.can_edit_any_collection() + && !collection.is_writable_by_user(&membership.user_uuid, conn).await + { + err!(Compact, "The current user isn't allowed to manage this collection") + } + } else if membership.atype <= MembershipType::Manager + && !membership.has_full_access() + && !membership.has_custom_permission(CustomPermission::CreateNewCollections) + { + err!(Compact, "The current user isn't allowed to create new collections") + } + } + Ok(existing) +} + // https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Api/Tools/Controllers/ImportCiphersController.cs#L62 #[post("/ciphers/import-organization?", data = "")] async fn post_org_import( @@ -1864,74 +1953,171 @@ 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)?; - let existing_collections: HashMap = - Collection::find_by_organization(&org_id, &conn).await.into_iter().map(|c| (c.uuid.clone(), c)).collect(); - let mut collections: Vec = Vec::with_capacity(data.collections.len()); - for col in data.collections { - let existing = col.id.as_ref().and_then(|col_id| existing_collections.get(col_id)); - let collection_uuid = if let Some(collection) = existing { - // When not an Owner or Admin, check if the member is allowed to write to the collection. - if headers.membership.atype < MembershipType::Admin - && !collection.is_writable_by_user(&headers.membership.user_uuid, &conn).await - { - err!(Compact, "The current user isn't allowed to manage this collection") + let relations = validate_org_import_relationships(&data)?; + let ImportData { + ciphers: imported_ciphers, + collections: imported_collections, + .. + } = data; + let grant_new_collection_manage = headers.membership.has_type(MembershipType::Custom); + let importer_uuid = headers.membership.user_uuid.clone(); + let OrgMemberHeaders { + host, + device, + user, + membership, + ip, + } = headers; + let headers = Headers { + host, + device, + user, + ip, + }; + + // Keep authorization and every write on the same connection. A failure at any later cipher or + // relationship must also roll back newly created collections and their Manage assignments. + conn.transaction(async || { + let existing = validate_org_import_collections(&imported_collections, &org_id, &membership, &conn).await?; + let mut collections: Vec = Vec::with_capacity(imported_collections.len()); + for col in imported_collections { + let collection_uuid = if let Some(collection) = col.id.as_ref().and_then(|id| existing.get(id)) { + collection.uuid.clone() + } else { + let new_collection = Collection::new(org_id.clone(), col.name, col.external_id); + new_collection.save(&conn).await?; + if grant_new_collection_manage { + CollectionUser::save(&importer_uuid, &new_collection.uuid, false, false, true, &conn).await?; + } + new_collection.uuid + }; + collections.push(collection_uuid); + } + + let mut assigned = vec![Vec::::new(); imported_ciphers.len()]; + for (cipher_index, collection_index) in relations { + assigned[cipher_index].push(collections[collection_index].clone()); + } + for (mut cipher_data, collection_ids) in imported_ciphers.into_iter().zip(assigned) { + cipher_data.folder_id = None; + // The authenticated membership, rather than the imported file, selects the organization. + cipher_data.organization_id = Some(org_id.clone()); + let mut cipher = Cipher::new(cipher_data.r#type); + update_cipher_from_data( + &mut cipher, + cipher_data, + &headers, + Some(collection_ids.clone()), + &conn, + &nt, + UpdateType::None, + false, + ) + .await?; + for collection_id in collection_ids { + CollectionCipher::save(&cipher.uuid, &collection_id, &conn).await?; } - collection.uuid.clone() - } else { - // We do not allow users or managers which can not manage all collections to create new collections - // If there is any collection other than an existing import collection, abort the import. - if headers.membership.atype <= MembershipType::Manager && !headers.membership.has_full_access() { - err!(Compact, "The current user isn't allowed to create new collections") + } + + let mut user = headers.user; + user.update_revision(&conn).await + }) + .await +} + +#[cfg(all(test, feature = "sqlite"))] +mod org_import_tests { + use super::*; + use crate::{ + api::notifications::{WS_USERS, WebSocketUsers}, + db::{models::test_flag, test_db}, + }; + use diesel::connection::SimpleConnection; + use std::sync::Arc; + + #[test] + fn imports_preserve_authorization_atomicity_and_creator_access() { + test_db::run(async |db| { + let conn = db.conn(); + for (case, role, permission) in [ + ("owner", MembershipType::Owner, CustomPermission::CreateNewCollections), + ("admin", MembershipType::Admin, CustomPermission::CreateNewCollections), + ("cipher", MembershipType::Custom, CustomPermission::CreateNewCollections), + ("custom", MembershipType::Custom, CustomPermission::CreateNewCollections), + ("legacy", MembershipType::Manager, CustomPermission::CreateNewCollections), + ("authorization", MembershipType::Custom, CustomPermission::CreateNewCollections), + ("rollback", MembershipType::Custom, CustomPermission::CreateNewCollections), + ("index", MembershipType::Custom, CustomPermission::CreateNewCollections), + ("disabled", MembershipType::Custom, CustomPermission::CreateNewCollections), + ("export", MembershipType::Owner, CustomPermission::CreateNewCollections), + ] { + test_flag::CUSTOM_ROLES_ENABLED.set(Some(case != "disabled")); + let org = test_db::organization(&conn).await; + let permissions = if role == MembershipType::Custom { + vec![permission] + } else { + vec![] + }; + let (user, mut membership) = test_db::member(&conn, &org.uuid, role, &permissions).await; + membership.access_all = role == MembershipType::Manager; + membership.save(&conn).await.unwrap(); + let mut payload = json!({"ciphers":[{"name":"Cipher","type":2,"encryptedFor":user.uuid,"secureNote":{"type":0}}], + "collections":[{"name":"New"}],"collectionRelationships":[{"key":0,"value":0}]}); + match case { + "authorization" => { + let existing=test_db::collection(&conn,&org.uuid).await; + payload["collections"].as_array_mut().unwrap().push(json!({"name":"Existing","id":existing.uuid})); + } + "cipher" => { + payload["ciphers"].as_array_mut().unwrap().push(json!({"name":"Invalid","type":99,"encryptedFor":user.uuid,"secureNote":{"type":0}})); + payload["collectionRelationships"].as_array_mut().unwrap().push(json!({"key":1,"value":0})); + } + "rollback" => conn.run(|c| c.batch_execute("CREATE TRIGGER reject_link BEFORE INSERT ON ciphers_collections BEGIN SELECT RAISE(ABORT,'test failure'); END")).await.unwrap(), + "index" => payload["collectionRelationships"][0]["value"]=json!(1), + "export" => payload["collectionRelationships"]=json!([]), + _ => {} + } + let user_id = user.uuid.clone(); + let headers = OrgMemberHeaders { + host: "http://localhost".into(), + device: Device::new("test".to_owned().into(), user_id.clone(), "test".into(), 1), + user, + membership, + ip: crate::auth::ClientIp { + ip: "127.0.0.1".parse().unwrap(), + }, + }; + let rocket = rocket::build().manage(Arc::clone(&WS_USERS)); + let notify = rocket.state::>().unwrap(); + let result = post_org_import( + OrgIdData { + organization_id: org.uuid.clone(), + }, + Json(serde_json::from_value(payload).unwrap()), + headers, + db.conn(), + notify.into(), + ) + .await; + conn.run(|c| c.batch_execute("DROP TRIGGER IF EXISTS reject_link")).await.unwrap(); + let success = matches!(case, "owner" | "admin" | "custom" | "legacy" | "export"); + assert_eq!(result.is_ok(), success, "{case}"); + assert_eq!( + Collection::find_by_organization(&org.uuid, &conn).await.len(), + usize::from(success || case == "authorization") + ); + assert_eq!(Cipher::find_by_org(&org.uuid, &conn).await.len(), usize::from(success)); + assert_eq!( + CollectionUser::find_by_user(&user_id, &conn).await.len(), + usize::from(success && role == MembershipType::Custom) + ); + assert_eq!( + Cipher::get_collections_with_cipher_by_organization(&org.uuid, &conn).await.len(), + usize::from(success && case != "export") + ); } - let new_collection = Collection::new(org_id.clone(), col.name, col.external_id); - new_collection.save(&conn).await?; - new_collection.uuid - }; - - collections.push(collection_uuid); + }); } - - // Read the relations between collections and ciphers - // Ciphers can be in multiple collections at the same time - let mut relations = Vec::with_capacity(data.collection_relationships.len()); - for relation in data.collection_relationships { - relations.push((relation.key, relation.value)); - } - - let headers: Headers = headers.into(); - - let mut ciphers: Vec = Vec::with_capacity(data.ciphers.len()); - for mut cipher_data in data.ciphers { - // Always clear folder_id's via an organization import - cipher_data.folder_id = None; - // Replace the client-provided, unvalidated organizationId with the real target org - cipher_data.organization_id = Some(org_id.clone()); - let mut cipher = Cipher::new(cipher_data.r#type); - update_cipher_from_data( - &mut cipher, - cipher_data, - &headers, - Some(collections.clone()), - &conn, - &nt, - UpdateType::None, - true, - ) - .await - .ok(); - ciphers.push(cipher.uuid); - } - - // Assign the collections - for (cipher_index, col_index) in relations { - let (Some(cipher_id), Some(col_id)) = (ciphers.get(cipher_index), collections.get(col_index)) else { - err!(Compact, "Invalid collection relationship") - }; - CollectionCipher::save(cipher_id, col_id, &conn).await?; - } - - let mut user = headers.user; - user.update_revision(&conn).await } #[derive(Deserialize)] @@ -2529,6 +2715,14 @@ async fn restore_member_impl( Ok(()) } +/// Whether a member may give others access to collections, and so needs to see the members and groups: by seeing +/// or creating any collection, or by the manage option on a collection. +async fn may_assign_collection_access(member: &Membership, conn: &DbConn) -> bool { + member.can_view_all_collections() + || member.has_custom_permission(CustomPermission::CreateNewCollections) + || Collection::has_manageable_collection_by_user(&member.org_uuid, &member.user_uuid, conn).await +} + async fn get_groups_data( details: bool, org_id: OrganizationId, @@ -2542,14 +2736,15 @@ async fn get_groups_data( // The details view (group→collection/user mappings) needs full org access; the plain list only // needs manage access to a collection, so a manager of a collection (directly or via a group) // can load it to assign groups. + // A group with access to all collections gives a Custom member access to them, but not to the Admin Console. let has_full_access = headers.membership.has_full_access() || (CONFIG.org_groups_enabled() + && !headers.membership.has_type(MembershipType::Custom) && GroupUser::has_full_access_by_member(&org_id, &headers.membership.uuid, &conn).await); let allowed = if details { has_full_access } else { - has_full_access - || Collection::has_manageable_collection_by_user(&org_id, &headers.membership.user_uuid, &conn).await + has_full_access || may_assign_collection_access(&headers.membership, &conn).await }; if !allowed { err_code!("Resource not found.", "User does not have access", Status::NotFound.code); diff --git a/src/auth.rs b/src/auth.rs index ff858c0e..d6f526c2 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -32,9 +32,9 @@ use crate::{ db::{ DbConn, models::{ - AttachmentId, CipherId, Collection, CollectionId, Device, DeviceId, DeviceType, EmergencyAccessId, - EventType, Membership, MembershipId, MembershipStatus, MembershipType, OrgApiKeyId, OrganizationId, - SendFileId, SendId, User, UserId, + AttachmentId, CipherId, Collection, CollectionId, CustomPermission, Device, DeviceId, DeviceType, + EmergencyAccessId, EventType, Membership, MembershipId, MembershipStatus, MembershipType, OrgApiKeyId, + OrganizationId, SendFileId, SendId, User, UserId, custom_roles_enabled, }, }, error::Error, @@ -757,6 +757,12 @@ impl OrgHeaders { fn is_confirmed_and_owner(&self) -> bool { self.membership_status == MembershipStatus::Confirmed && self.membership_type == MembershipType::Owner } + // A Custom member, while Custom roles are enabled. What they may do depends on their permissions. + fn is_confirmed_and_custom(&self) -> bool { + self.membership_status == MembershipStatus::Confirmed + && self.membership_type == MembershipType::Custom + && custom_roles_enabled() + } } // org_id is usually the second path param ("/organizations/"), @@ -895,62 +901,134 @@ fn get_col_id(request: &Request<'_>) -> Option { None } -/// The ManagerHeaders are used to check if you are at least a Manager -/// and have access to the specific collection provided via the /collections/collectionId. -/// This does strict checking on the collection_id, ManagerHeadersLoose does not. -pub struct ManagerHeaders { - pub host: String, - pub device: Device, - pub user: User, - pub ip: ClientIp, - pub org_id: OrganizationId, +/// What a collection route does with its collection, which decides what a Custom member needs for it. +#[derive(Clone, Copy)] +pub enum CollectionAction { + Edit, + Read, + Delete, } -#[rocket::async_trait] -impl<'r> FromRequest<'r> for ManagerHeaders { - type Error = &'static str; +/// Whether `member`, a confirmed Custom member, may `action` the collection `col_id` of their organization. +/// +/// Deleting needs Delete any collection: the clients are told collection deletion is limited, so the manage option +/// on a collection is no permission to delete it. Editing is open with Edit any collection, reading also with Delete +/// any collection, and otherwise both need the manage option on the collection itself. Neither the role nor any +/// `access_all` counts as that. +pub async fn custom_member_may( + action: CollectionAction, + member: &Membership, + col_id: &CollectionId, + conn: &DbConn, +) -> bool { + if !(member.has_type(MembershipType::Custom) + && member.has_status(MembershipStatus::Confirmed) + && custom_roles_enabled()) + { + return false; + } + match action { + CollectionAction::Delete => member.has_custom_permission(CustomPermission::DeleteAnyCollection), + CollectionAction::Edit if member.can_edit_any_collection() => true, + CollectionAction::Read if member.can_view_all_collections() => true, + CollectionAction::Edit | CollectionAction::Read => { + Collection::find_explicitly_manageable_by_user(&member.org_uuid, &member.user_uuid, conn) + .await + .contains(col_id) + } + } +} - async fn from_request(request: &'r Request<'_>) -> Outcome { - let headers = try_outcome!(OrgHeaders::from_request(request).await); - if headers.is_confirmed_and_manager() { - if let Some(col_id) = get_col_id(request) { - let Outcome::Success(conn) = DbConn::from_request(request).await else { - err_handler!("Error getting DB") - }; +/// Checks a collection route for the collection provided via the /collections/collectionId: a Manager, Admin +/// or Owner needs to be able to manage the collection, a Custom member is checked for the `action`. +async fn collection_headers(request: &Request<'_>, action: CollectionAction) -> Outcome { + let headers = try_outcome!(OrgHeaders::from_request(request).await); + let is_custom = headers.is_confirmed_and_custom(); + if !headers.is_confirmed_and_manager() && !is_custom { + err_handler!("You need to be a Manager, Admin or Owner to call this endpoint") + } - if !Collection::is_coll_manageable_by_user(&col_id, &headers.membership.user_uuid, &conn).await { - err_handler!("The current user isn't a manager for this collection") - } - } else { - err_handler!("Error getting the collection id") + let Some(col_id) = get_col_id(request) else { + err_handler!("Error getting the collection id") + }; + let Outcome::Success(conn) = DbConn::from_request(request).await else { + err_handler!("Error getting DB") + }; + + let allowed = if is_custom { + custom_member_may(action, &headers.membership, &col_id, &conn).await + } else { + Collection::is_coll_manageable_by_user(&col_id, &headers.membership.user_uuid, &conn).await + }; + if !allowed { + // NotFound as in Bitwarden, the web vault logs the user out on a 401 + error!(target: "auth", "Collection not found. The current user isn't a manager for this collection"); + return Outcome::Error((rocket::http::Status::NotFound, "Collection not found")); + } + Outcome::Success(headers) +} + +macro_rules! collection_headers { + ($(#[$doc:meta])* $name:ident, $action:expr) => { + $(#[$doc])* + pub struct $name { + pub host: String, + pub device: Device, + pub user: User, + pub ip: ClientIp, + pub org_id: OrganizationId, + } + + #[rocket::async_trait] + impl<'r> FromRequest<'r> for $name { + type Error = &'static str; + + async fn from_request(request: &'r Request<'_>) -> Outcome { + let headers = try_outcome!(collection_headers(request, $action).await); + Outcome::Success(Self { + host: headers.host, + device: headers.device, + user: headers.user, + ip: headers.ip, + org_id: headers.membership.org_uuid, + }) } - - Outcome::Success(Self { - host: headers.host, - device: headers.device, - user: headers.user, - ip: headers.ip, - org_id: headers.membership.org_uuid, - }) - } else { - err_handler!("You need to be a Manager, Admin or Owner to call this endpoint") } - } + + impl From<$name> for Headers { + fn from(h: $name) -> Headers { + Headers { + host: h.host, + device: h.device, + user: h.user, + ip: h.ip, + } + } + } + }; } -impl From for Headers { - fn from(h: ManagerHeaders) -> Headers { - Headers { - host: h.host, - device: h.device, - user: h.user, - ip: h.ip, - } - } -} +collection_headers!( + /// The ManagerHeaders are used to check if you are at least a Manager + /// and have access to the specific collection provided via the /collections/collectionId. + /// This does strict checking on the collection_id, ManagerHeadersLoose does not. + ManagerHeaders, + CollectionAction::Edit +); +collection_headers!( + /// The same as ManagerHeaders, for the routes which only read the collection and its access. + CollectionReadHeaders, + CollectionAction::Read +); +collection_headers!( + /// The same as ManagerHeaders, for the routes which delete the collection. + CollectionDeleteHeaders, + CollectionAction::Delete +); /// The ManagerHeadersLoose is used when you at least need to be a Manager, /// but there is no collection_id sent with the request (either in the path or as form data). +/// A Custom member is let through as well, the routes check what their permissions allow. pub struct ManagerHeadersLoose { pub host: String, pub device: Device, @@ -965,7 +1043,7 @@ impl<'r> FromRequest<'r> for ManagerHeadersLoose { async fn from_request(request: &'r Request<'_>) -> Outcome { let headers = try_outcome!(OrgHeaders::from_request(request).await); - if headers.is_confirmed_and_manager() { + if headers.is_confirmed_and_manager() || headers.is_confirmed_and_custom() { Outcome::Success(Self { host: headers.host, device: headers.device, @@ -990,22 +1068,29 @@ impl From for Headers { } } -impl ManagerHeaders { +impl CollectionDeleteHeaders { pub async fn from_loose( h: ManagerHeadersLoose, collections: &Vec, conn: &DbConn, - ) -> Result { + ) -> Result { + let is_custom = h.membership.has_type(MembershipType::Custom) && custom_roles_enabled(); for col_id in collections { if uuid::Uuid::parse_str(col_id.as_ref()).is_err() { err!("Collection Id is malformed!"); } - if !Collection::is_coll_manageable_by_user(col_id, &h.membership.user_uuid, conn).await { + let allowed = if is_custom { + Collection::find_by_uuid_and_org(col_id, &h.membership.org_uuid, conn).await.is_some() + && custom_member_may(CollectionAction::Delete, &h.membership, col_id, conn).await + } else { + Collection::is_coll_manageable_by_user(col_id, &h.membership.user_uuid, conn).await + }; + if !allowed { err!("Collection not found", "The current user isn't a manager for this collection") } } - Ok(ManagerHeaders { + Ok(CollectionDeleteHeaders { host: h.host, device: h.device, user: h.user, diff --git a/src/db/mod.rs b/src/db/mod.rs index 3caa6cde..39aea1d3 100644 --- a/src/db/mod.rs +++ b/src/db/mod.rs @@ -626,6 +626,7 @@ pub mod test_db { } // DbConn::run uses block_in_place, requiring a multi-threaded runtime. tokio::runtime::Builder::new_multi_thread().enable_all().build().unwrap().block_on(async { + super::models::test_flag::CUSTOM_ROLES_ENABLED.set(Some(true)); test(&TestDb::new()).await; }); } @@ -758,4 +759,68 @@ pub mod test_db { } }); } + + #[test] + fn collection_and_cipher_access_requires_active_membership_and_enabled_groups() { + use super::models::{Cipher, CipherAccessScope, CollectionCipher, CollectionUser, test_flag}; + use crate::{ + api::core::{CipherSyncData, CipherSyncType}, + auth::{CollectionAction, custom_member_may}, + }; + run(async |db| { + let conn = db.conn(); + let (org, user, mut member) = custom_member(&conn, CustomPermission::EditAnyCollection).await; + let collection = collection(&conn, &org.uuid).await; + let mut cipher = Cipher::new(2); + cipher.name = "Cipher".into(); + cipher.organization_uuid = Some(org.uuid.clone()); + cipher.save(&conn).await.unwrap(); + CollectionCipher::save(&cipher.uuid, &collection.uuid, &conn).await.unwrap(); + for (enabled, status, allowed) in [(true, 2, true), (false, 2, false), (true, 1, false)] { + test_flag::CUSTOM_ROLES_ENABLED.set(Some(enabled)); + member.status = status; + member.save(&conn).await.unwrap(); + assert!(!cipher.is_accessible_to_user(&user.uuid, CipherAccessScope::User, &conn).await); + assert_eq!( + cipher.is_write_accessible_to_user(&user.uuid, CipherAccessScope::OrganizationAdmin, &conn).await, + allowed + ); + } + test_flag::CUSTOM_ROLES_ENABLED.set(Some(true)); + member.status = 2; + member.save(&conn).await.unwrap(); + let foreign = organization(&conn).await; + cipher.organization_uuid = Some(foreign.uuid); + assert!(!cipher.is_accessible_to_user(&user.uuid, CipherAccessScope::OrganizationAdmin, &conn).await); + cipher.organization_uuid = Some(org.uuid.clone()); + member.set_custom_permissions(&std::collections::HashSet::new()); + member.save(&conn).await.unwrap(); + let sql=format!("INSERT INTO groups (uuid,organizations_uuid,name,access_all,creation_date,revision_date) + VALUES ('g','{}','Group',FALSE,CURRENT_TIMESTAMP,CURRENT_TIMESTAMP); + INSERT INTO groups_users (groups_uuid,users_organizations_uuid) VALUES ('g','{}'); + INSERT INTO collections_groups (collections_uuid,groups_uuid,read_only,hide_passwords,manage) VALUES ('{}','g',FALSE,FALSE,TRUE);", + org.uuid,member.uuid,collection.uuid); + conn.run(move |c| c.batch_execute(&sql)).await.unwrap(); + for status in [2, 1, -3, -126] { + member.status = status; + member.save(&conn).await.unwrap(); + let allowed = status == 2 && crate::CONFIG.org_groups_enabled(); + assert_eq!(custom_member_may(CollectionAction::Edit, &member, &collection.uuid, &conn).await, allowed); + let sync = CipherSyncData::new(&user.uuid, CipherSyncType::User, &conn).await; + for cached in [None, Some(&sync)] { + assert_eq!( + cipher + .get_access_restrictions(&user.uuid, CipherAccessScope::User, cached, &conn) + .await + .is_some(), + allowed + ); + } + } + member.status = 2; + member.save(&conn).await.unwrap(); + CollectionUser::save(&user.uuid, &collection.uuid, false, false, true, &conn).await.unwrap(); + assert!(custom_member_may(CollectionAction::Edit, &member, &collection.uuid, &conn).await); + }); + } } diff --git a/src/db/models/cipher.rs b/src/db/models/cipher.rs index e940511c..68f97c13 100644 --- a/src/db/models/cipher.rs +++ b/src/db/models/cipher.rs @@ -25,7 +25,8 @@ use macros::UuidFromParam; use super::{ Archive, Attachment, CollectionCipher, CollectionId, Favorite, FolderCipher, FolderId, Group, Membership, - MembershipStatus, OrganizationId, User, UserId, organization::ORG_ADMIN_ATYPES, + MembershipStatus, OrganizationId, User, UserId, + organization::{ORG_ADMIN_ATYPES, custom_edit_any_collection}, }; #[derive(Identifiable, Queryable, Insertable, AsChangeset)] @@ -230,6 +231,23 @@ impl Cipher { .await } + pub async fn to_json_org_admin_collections( + &self, + host: &str, + user_uuid: &UserId, + conn: &DbConn, + ) -> Result { + self.to_json_scoped( + host, + user_uuid, + None, + CipherSyncType::Organization, + CipherAccessScope::OrganizationAdmin, + conn, + ) + .await + } + async fn to_json_scoped( &self, host: &str, @@ -352,7 +370,7 @@ impl Cipher { Cow::from(Vec::new()) } } else { - Cow::from(self.get_admin_collections(user_uuid.clone(), conn).await) + Cow::from(self.get_admin_collections(user_uuid.clone(), scope, conn).await) }; // There are three types of cipher response models in upstream @@ -1194,7 +1212,12 @@ impl Cipher { } } - pub async fn get_admin_collections(&self, user_uuid: UserId, conn: &DbConn) -> Vec { + pub async fn get_admin_collections( + &self, + user_uuid: UserId, + scope: CipherAccessScope, + conn: &DbConn, + ) -> Vec { if CONFIG.org_groups_enabled() { conn.run(move |conn| { ciphers_collections::table @@ -1234,7 +1257,9 @@ impl Cipher { .or(collections_groups::collections_uuid .is_not_null() // Access via groups .and(collections_groups::read_only.eq(false))) - .or(users_organizations::atype.eq_any(ORG_ADMIN_ATYPES)), // User is admin or owner + .or(users_organizations::atype.eq_any(ORG_ADMIN_ATYPES)) // User is admin or owner + .or(custom_edit_any_collection() + .and::<_, diesel::sql_types::Bool>(scope == CipherAccessScope::OrganizationAdmin)), ) .select(ciphers_collections::collection_uuid) .load::(conn) @@ -1263,7 +1288,9 @@ impl Cipher { .or(users_collections::user_uuid .eq(user_uuid) // User has access to collection .and(users_collections::read_only.eq(false))) - .or(users_organizations::atype.eq_any(ORG_ADMIN_ATYPES)), // User is admin or owner + .or(users_organizations::atype.eq_any(ORG_ADMIN_ATYPES)) // User is admin or owner + .or(custom_edit_any_collection() + .and::<_, diesel::sql_types::Bool>(scope == CipherAccessScope::OrganizationAdmin)), ) .select(ciphers_collections::collection_uuid) .load::(conn) diff --git a/src/db/models/collection.rs b/src/db/models/collection.rs index 68c48ce9..4f1533e3 100644 --- a/src/db/models/collection.rs +++ b/src/db/models/collection.rs @@ -18,7 +18,7 @@ use macros::UuidFromParam; use super::{ CipherId, CollectionGroup, GroupUser, Membership, MembershipId, MembershipStatus, MembershipType, OrganizationId, - User, UserId, organization::ORG_ADMIN_ATYPES, + User, UserId, custom_roles_enabled, organization::ORG_ADMIN_ATYPES, }; // See (v2026.7.0): https://github.com/bitwarden/server/blob/5d4461aa42cadbacfef8fe2166c5453a5c52773a/src/Core/AdminConsole/Entities/Collection.cs @@ -109,18 +109,22 @@ impl Collection { Some(m) if m.has_full_access() => (false, false, m.atype >= MembershipType::Manager), Some(m) => { // Only let a manager manage collections when the have full read/write access + // A Custom member only manages a collection with the manage option let is_manager = m.atype == MembershipType::Manager; + let is_custom = m.has_type(MembershipType::Custom) && custom_roles_enabled(); if let Some(cu) = cipher_sync_data.user_collections.get(&self.uuid) { ( cu.read_only, cu.hide_passwords, - is_manager && (cu.manage || (!cu.read_only && !cu.hide_passwords)), + (is_manager && (cu.manage || (!cu.read_only && !cu.hide_passwords))) + || (is_custom && cu.manage), ) } else if let Some(cg) = cipher_sync_data.user_collections_groups.get(&self.uuid) { ( cg.read_only, cg.hide_passwords, - is_manager && (cg.manage || (!cg.read_only && !cg.hide_passwords)), + (is_manager && (cg.manage || (!cg.read_only && !cg.hide_passwords))) + || (is_custom && cg.manage), ) } else { (false, false, false) @@ -134,6 +138,17 @@ impl Collection { Some(m) if m.atype == MembershipType::Manager && self.is_manageable_by_user(user_uuid, conn).await => { (false, false, true) } + // A Custom member reads and writes every collection with Edit any collection in the Admin Console, but + // only manages a collection with the manage option on it + Some(m) if m.has_type(MembershipType::Custom) && custom_roles_enabled() => { + let manage = self.is_explicitly_manageable_by_user(user_uuid, conn).await; + if manage || m.can_edit_any_collection() { + (false, false, manage) + } else { + let read_only = !self.is_writable_by_user(user_uuid, conn).await; + (read_only, self.hide_passwords_for_user(user_uuid, conn).await, false) + } + } Some(m) => { let is_manager = m.atype == MembershipType::Manager; let read_only = !self.is_writable_by_user(user_uuid, conn).await; @@ -635,8 +650,23 @@ impl Collection { user_uuid: &UserId, conn: &DbConn, ) -> bool { + !Self::find_explicitly_manageable_by_user(org_uuid, user_uuid, conn).await.is_empty() + } + + pub async fn is_explicitly_manageable_by_user(&self, user_uuid: &UserId, conn: &DbConn) -> bool { + Self::find_explicitly_manageable_by_user(&self.org_uuid, user_uuid, conn).await.contains(&self.uuid) + } + + // The collections of the org the user has manage access to by the manage option, directly or via a group. + // Unlike is_coll_manageable_by_user, neither the role nor any access_all counts. + pub async fn find_explicitly_manageable_by_user( + org_uuid: &OrganizationId, + user_uuid: &UserId, + conn: &DbConn, + ) -> Vec { let org_uuid = org_uuid.to_string(); let user_uuid = user_uuid.to_string(); + let groups_enabled = CONFIG.org_groups_enabled(); conn.run(move |conn| { collections::table .left_join( @@ -658,14 +688,16 @@ impl Collection { .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)), + users_collections::manage.eq(true).or(users_collections::user_uuid + .is_null() + .or(users_organizations::atype.eq(MembershipType::Manager as i32)) + .and(collections_groups::manage.eq(true)) + .and::<_, diesel::sql_types::Bool>(groups_enabled)), ) - .count() - .first::(conn) - .ok() - .unwrap_or(0) - != 0 + .select(collections::uuid) + .distinct() + .load::(conn) + .unwrap_or_default() }) .await } diff --git a/src/db/models/mod.rs b/src/db/models/mod.rs index b41d36ae..e4e77c4d 100644 --- a/src/db/models/mod.rs +++ b/src/db/models/mod.rs @@ -33,6 +33,8 @@ pub use self::favorite::Favorite; pub use self::folder::{Folder, FolderCipher, FolderId}; pub use self::group::{CollectionGroup, Group, GroupId, GroupUser}; pub use self::org_policy::{FillAssistPolicyData, OrgPolicy, OrgPolicyId, OrgPolicyType}; +#[cfg(all(test, sqlite))] +pub(crate) use self::organization::test_flag; pub use self::organization::{ CustomPermission, Membership, MembershipId, MembershipStatus, MembershipType, OrgApiKeyId, Organization, OrganizationApiKey, OrganizationId, custom_roles_enabled, diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index f1e03354..b9d81f72 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -24,8 +24,8 @@ use crate::{ use macros::UuidFromParam; use super::{ - Cipher, CipherId, Collection, CollectionGroup, CollectionId, CollectionUser, Group, GroupId, GroupUser, OrgPolicy, - OrgPolicyType, TwoFactor, User, UserId, + Cipher, CipherId, Collection, CollectionId, CollectionUser, Group, GroupId, GroupUser, OrgPolicy, OrgPolicyType, + TwoFactor, User, UserId, }; #[derive(Identifiable, Queryable, Insertable, AsChangeset)] @@ -134,22 +134,28 @@ impl MembershipType { /// role (`None` for an invite). /// /// The clients no longer know the legacy Manager role and are shown a Custom role in its place, so a - /// requested Custom role is the legacy Manager unless EXPERIMENTAL_CUSTOM_ROLES is enabled. An existing - /// Manager or Custom member keeps their role when the request repeats it, so an unrelated edit never - /// turns one into the other. + /// requested Custom role is the legacy Manager unless EXPERIMENTAL_CUSTOM_ROLES is enabled, in which case + /// `custom_permissions` are the permissions requested along with it. An existing Manager or Custom member + /// keeps their role when the request repeats it, so an unrelated edit never turns one into the other. A + /// Manager only becomes a Custom member when the permissions they are shown with are changed. pub fn requested( requested: MembershipType, current: Option<&Membership>, - custom_roles_enabled: bool, + custom_permissions: Option<&HashSet>, ) -> MembershipType { if requested != MembershipType::Custom { return requested; } - match current { - Some(member) if member.has_type(MembershipType::Custom) => MembershipType::Custom, - Some(member) if member.has_type(MembershipType::Manager) => MembershipType::Manager, - _ if custom_roles_enabled => MembershipType::Custom, - _ => MembershipType::Manager, + match (current, custom_permissions) { + (Some(member), _) if member.has_type(MembershipType::Custom) => MembershipType::Custom, + (Some(member), Some(permissions)) + if member.has_type(MembershipType::Manager) && *permissions != member.legacy_manager_permissions() => + { + MembershipType::Custom + } + (Some(member), _) if member.has_type(MembershipType::Manager) => MembershipType::Manager, + (_, Some(_)) => MembershipType::Custom, + (_, None) => MembershipType::Manager, } } @@ -240,7 +246,11 @@ pub enum CustomPermission { /// The permissions this version enforces on every endpoint they open. Only these can be granted, a stored /// grant of any other one grants nothing. They are implemented one at a time, so this grows with them. -const SUPPORTED_CUSTOM_PERMISSIONS: &[CustomPermission] = &[]; +const SUPPORTED_CUSTOM_PERMISSIONS: &[CustomPermission] = &[ + CustomPermission::CreateNewCollections, + CustomPermission::EditAnyCollection, + CustomPermission::DeleteAnyCollection, +]; impl CustomPermission { pub const ALL: [CustomPermission; 9] = [ @@ -306,9 +316,38 @@ pub(super) const ORG_ADMIN_ATYPES: [i32; 2] = [MembershipType::Owner as i32, Mem /// Whether the experimental Custom role is enabled. Nothing is ever granted through a Custom role otherwise. pub fn custom_roles_enabled() -> bool { + #[cfg(test)] + if let Some(enabled) = test_flag::CUSTOM_ROLES_ENABLED.get() { + return enabled; + } CONFIG.experimental_custom_roles() } +type CustomEditAnyCollection = diesel::dsl::And< + diesel::dsl::And< + diesel::dsl::Eq, + diesel::dsl::Eq, + >, + bool, + diesel::sql_types::Bool, +>; + +/// SQL: a Custom member with Edit any collection, while Custom roles are enabled. +pub(super) fn custom_edit_any_collection() -> CustomEditAnyCollection { + users_organizations::atype + .eq(MembershipType::Custom as i32) + .and(users_organizations::edit_any_collection.eq(true)) + .and::<_, diesel::sql_types::Bool>(custom_roles_enabled()) +} + +#[cfg(test)] +pub(crate) mod test_flag { + thread_local! { + /// Overrides EXPERIMENTAL_CUSTOM_ROLES for the tests running on this thread. + pub static CUSTOM_ROLES_ENABLED: std::cell::Cell> = const { std::cell::Cell::new(None) }; + } +} + /// Local methods impl Organization { pub fn new(name: String, billing_email: &str, private_key: Option, public_key: Option) -> Self { @@ -452,40 +491,36 @@ impl Membership { } } - /// Why this legacy Manager can't become a Custom member without losing what they may do now, if they can't. A - /// conversion must not silently change their access, and a Custom role can't keep access to all collections - /// (which also puts every collection in the Manager's own vault), nor manage any collection yet. - pub async fn custom_role_conversion_loss(&self, conn: &DbConn) -> Option<&'static str> { + /// Why this legacy Manager can't become a Custom member with `permissions` without losing what they may do now, if + /// they can't. A conversion must not silently change their access. A Custom role can't keep access to all + /// collections, which also puts every collection in the Manager's own vault, unless the permissions it is shown as + /// are given up too. And it only manages every collection with Edit any collection, not through a group with access + /// to all collections, which stays dynamic. The collections they manage with the manage permission stay managed. + pub async fn custom_role_conversion_loss( + &self, + permissions: &HashSet, + conn: &DbConn, + ) -> Option<&'static str> { if !self.has_type(MembershipType::Manager) { return None; } - if self.access_all { + if self.access_all && permissions.is_superset(&self.legacy_manager_permissions()) { return Some( "This Manager has access to all collections, which a Custom member can't have: the collections they \ - aren't assigned to would disappear from their vault. Uncheck Manage all collections for them first, \ - or keep the Manager role.", + aren't assigned to would disappear from their vault. Uncheck Manage all collections for them to give \ + that access up, or keep the Manager role.", ); } - // Whatever their status, the collections they manage directly, through a group or through a group with - // access to all collections - let mut manages = GroupUser::has_full_access_by_member(&self.org_uuid, &self.uuid, conn).await - || CollectionUser::find_by_organization_and_user_uuid(&self.org_uuid, &self.user_uuid, conn) - .await - .iter() - .any(|assignment| assignment.manage); - for group_user in GroupUser::find_by_member(&self.uuid, conn).await { - if manages { - break; - } - manages = CollectionGroup::find_by_group(&group_user.groups_uuid, &self.org_uuid, conn) - .await - .iter() - .any(|assignment| assignment.manage); + if !permissions.contains(&CustomPermission::EditAnyCollection) + && GroupUser::has_full_access_by_member(&self.org_uuid, &self.uuid, conn).await + { + return Some( + "This Manager manages every collection through a group with access to all collections, which a Custom \ + member only does with Edit any collection. Grant that permission, or remove them from the group \ + first.", + ); } - manages.then_some( - "This Manager manages collections, with the manage permission or through a group with access to all \ - collections, which a Custom member can't do yet. Remove that first, or keep the Manager role.", - ) + None } fn custom_permission(&self, permission: CustomPermission) -> bool { @@ -517,6 +552,41 @@ impl Membership { self.has_status(MembershipStatus::Confirmed) && self.grants_custom_permission(permission) } + /// The permissions the "Manage all collections" a legacy Manager is shown with stands for: their `access_all`. + pub fn legacy_manager_permissions(&self) -> HashSet { + if self.has_type(MembershipType::Manager) && self.access_all { + HashSet::from([ + CustomPermission::CreateNewCollections, + CustomPermission::EditAnyCollection, + CustomPermission::DeleteAnyCollection, + ]) + } else { + HashSet::new() + } + } + + /// The permissions the clients are shown, which a legacy Manager is shown as a Custom member with. + pub fn shown_custom_permissions(&self) -> HashSet { + if self.has_type(MembershipType::Manager) { + self.legacy_manager_permissions() + } else { + CustomPermission::ALL.into_iter().filter(|p| self.grants_custom_permission(*p)).collect() + } + } + + fn custom_permissions_json(&self) -> Value { + let shown = self.shown_custom_permissions(); + let mut permissions = json!({ + "manageSso": false, // Not supported + "manageResetPassword": false, + "manageScim": false, // Not supported (Not AGPLv3 Licensed) + }); + for permission in CustomPermission::ALL { + permissions[permission.json_key()] = json!(shown.contains(&permission)); + } + permissions + } + pub fn restore(&mut self) -> bool { if self.status < MembershipStatus::Invited as i32 { self.status += ACTIVATE_REVOKE_DIFF; @@ -685,24 +755,8 @@ impl Membership { // It will be converted back on other locations let membership_type = self.type_manager_as_custom(); - let permissions = json!({ - // TODO: Add full support for Custom User Roles - // See: https://bitwarden.com/help/article/user-types-access-control/#custom-role - // Currently we use the custom role as a manager role and link the 3 Collection roles to mimic the access_all permission - "accessEventLogs": false, - "accessImportExport": false, - "accessReports": false, - // If the following 3 Collection roles are set to true a custom user has access all permission - "createNewCollections": membership_type == 4 && self.access_all, - "editAnyCollection": membership_type == 4 && self.access_all, - "deleteAnyCollection": membership_type == 4 && self.access_all, - "manageGroups": false, - "managePolicies": false, - "manageSso": false, // Not supported - "manageUsers": false, - "manageResetPassword": false, - "manageScim": false // Not supported (Not AGPLv3 Licensed) - }); + // A Manager is shown with the 3 Collection permissions set, when they have the access_all permission + let permissions = self.custom_permissions_json(); // https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Api/AdminConsole/Models/Response/ProfileOrganizationResponseModel.cs json!({ @@ -832,29 +886,11 @@ impl Membership { // It will be converted back on other locations let membership_type = self.type_manager_as_custom(); - // HACK: Only return permissions if the user is of type custom and has access_all - // Else Bitwarden will assume the defaults of all false - let permissions = if membership_type == 4 && self.access_all { - json!({ - // TODO: Add full support for Custom User Roles - // See: https://bitwarden.com/help/article/user-types-access-control/#custom-role - // Currently we use the custom role as a manager role and link the 3 Collection roles to mimic the access_all permission - "accessEventLogs": false, - "accessImportExport": false, - "accessReports": false, - // If the following 3 Collection roles are set to true a custom user has access all permission - "createNewCollections": true, - "editAnyCollection": true, - "deleteAnyCollection": true, - "manageGroups": false, - "managePolicies": false, - "manageSso": false, // Not supported - "manageUsers": false, - "manageResetPassword": false, - "manageScim": false // Not supported (Not AGPLv3 Licensed) - }) - } else { + // Only return permissions if there are any, else Bitwarden will assume the defaults of all false + let permissions = if self.shown_custom_permissions().is_empty() { json!(null) + } else { + self.custom_permissions_json() }; json!({ @@ -1042,6 +1078,12 @@ impl Membership { self.has_full_access() || self.has_custom_permission(CustomPermission::EditAnyCollection) } + /// Whether this member may see every collection and who has access to them in the Admin Console. Deleting any + /// collection needs that, but not the items in them. + pub fn can_view_all_collections(&self) -> bool { + self.can_edit_any_collection() || self.has_custom_permission(CustomPermission::DeleteAnyCollection) + } + pub async fn find_by_uuid(uuid: &MembershipId, conn: &DbConn) -> Option { conn.run(move |conn| { users_organizations::table.filter(users_organizations::uuid.eq(uuid)).first::(conn).ok() @@ -1164,6 +1206,8 @@ impl Membership { } // Get all users which are either owner or admin, or a manager which can manage/access all + // A Custom member with Edit any collection is not listed: the Admin Console sends this list back when a collection + // is saved, which would assign them to it for good, while the others are skipped there for their access_all. pub async fn find_confirmed_and_manage_all_by_org(org_uuid: &OrganizationId, conn: &DbConn) -> Vec { conn.run(move |conn| { users_organizations::table @@ -1484,6 +1528,7 @@ pub struct OrgApiKeyId(String); #[cfg(test)] mod tests { use super::*; + use CustomPermission::*; use MembershipType::{Admin, Custom, Manager, Owner, User}; fn member(role: MembershipType) -> Membership { @@ -1513,21 +1558,33 @@ mod tests { #[test] fn requested_membership_type() { - for (current, disabled, enabled) in [ - (None, Manager, Custom), - (Some(User), Manager, Custom), - (Some(Manager), Manager, Manager), - (Some(Custom), Custom, Custom), + let none = HashSet::new(); + let edit = HashSet::from([EditAnyCollection]); + let legacy = HashSet::from([CreateNewCollections, EditAnyCollection, DeleteAnyCollection]); + for (current, all, permissions, disabled, enabled) in [ + (None, false, &none, Manager, Custom), + (Some(User), false, &edit, Manager, Custom), + (Some(Owner), false, &none, Manager, Custom), + (Some(Manager), false, &none, Manager, Manager), + (Some(Manager), true, &legacy, Manager, Manager), + (Some(Manager), false, &edit, Manager, Custom), + (Some(Manager), true, &none, Manager, Custom), + (Some(Manager), true, &edit, Manager, Custom), + (Some(Custom), false, &none, Custom, Custom), ] { - let current = current.map(member); - for (flag, expected) in [(false, disabled), (true, enabled)] { - assert_eq!(MembershipType::requested(Custom, current.as_ref(), flag), expected); + let current = current.map(|role| { + let mut m = member(role); + m.access_all = all; + m + }); + for (permissions, expected) in [(None, disabled), (Some(permissions), enabled)] { + assert_eq!(MembershipType::requested(Custom, current.as_ref(), permissions), expected); } } - assert_eq!(MembershipType::requested(User, Some(&member(Custom)), true), User); - assert_eq!(MembershipType::requested(User, Some(&member(Manager)), true), User); + assert_eq!(MembershipType::requested(User, Some(&member(Custom)), None), User); + assert_eq!(MembershipType::requested(User, Some(&member(Manager)), None), User); for requested in [Owner, Admin, Manager] { - assert_eq!(MembershipType::requested(requested, Some(&member(Custom)), true), requested); + assert_eq!(MembershipType::requested(requested, Some(&member(Custom)), None), requested); } } @@ -1546,7 +1603,7 @@ mod tests { } } } - assert_eq!(SUPPORTED_CUSTOM_PERMISSIONS, []); + assert_eq!(SUPPORTED_CUSTOM_PERMISSIONS, [CreateNewCollections, EditAnyCollection, DeleteAnyCollection]); } #[test] @@ -1573,4 +1630,61 @@ mod tests { assert_eq!(member.access_all, to != Custom); } } + #[test] + fn permission_matrix_preserves_role_status_flag_and_collection_boundaries() { + let legacy = HashSet::from([CreateNewCollections, EditAnyCollection, DeleteAnyCollection]); + for (role, all) in + [(Owner, false), (Admin, false), (Manager, false), (Manager, true), (Custom, false), (User, false)] + { + for permission in CustomPermission::ALL { + let mut member = member(role); + member.access_all = all; + *member.custom_permission_mut(permission) = true; + for enabled in [false, true] { + test_flag::CUSTOM_ROLES_ENABLED.set(Some(enabled)); + let shown = if role == Manager && all { + legacy.clone() + } else if role == Custom && enabled && permission.is_supported() { + HashSet::from([permission]) + } else { + HashSet::new() + }; + assert_eq!(member.shown_custom_permissions(), shown); + for status in [-128, -127, -126, -3, -2, -1, 0, 1, 2] { + member.status = status; + let allowed = enabled && role == Custom && status == 2 && permission.is_supported(); + let full = status == 2 && (role == Owner || role == Admin || all); + let edit = full || (allowed && permission == EditAnyCollection); + let view = full || (allowed && matches!(permission, EditAnyCollection | DeleteAnyCollection)); + assert_eq!( + ( + member.has_custom_permission(permission), + member.can_edit_any_collection(), + member.can_view_all_collections() + ), + (allowed, edit, view), + "{role:?} {all} {enabled} {status} {permission:?}" + ); + } + } + } + } + for raw in [99, -1] { + let mut member = member(Custom); + member.atype = raw; + for p in CustomPermission::ALL { + *member.custom_permission_mut(p) = true; + } + for enabled in [false, true] { + test_flag::CUSTOM_ROLES_ENABLED.set(Some(enabled)); + assert!(CustomPermission::ALL.into_iter().all(|p| !member.has_custom_permission(p))); + assert!(!member.can_edit_any_collection() && !member.can_view_all_collections()); + } + } + let empty = member(Custom); + assert!(!empty.can_edit_any_collection() && !empty.can_view_all_collections()); + assert!(empty.shown_custom_permissions().is_empty()); + + test_flag::CUSTOM_ROLES_ENABLED.set(None); + } } diff --git a/src/static/templates/scss/vaultwarden.scss.hbs b/src/static/templates/scss/vaultwarden.scss.hbs index fc6d22c6..2427b082 100644 --- a/src/static/templates/scss/vaultwarden.scss.hbs +++ b/src/static/templates/scss/vaultwarden.scss.hbs @@ -99,7 +99,7 @@ app-security > app-two-factor-setup > form { /* Hide unsupported Custom Role options */ {{#if custom_roles_enabled}} -:is(bit-dialog, [bit-dialog]) div.tw-ml-4:has(bit-form-control input), +/* Only the collection permissions can be granted yet */ :is(bit-dialog, [bit-dialog]) div.tw-col-span-4:has(input[formcontrolname*="access"], input[formcontrolname*="manage"]) { @extend %vw-hide; }