diff --git a/src/api/admin.rs b/src/api/admin.rs index 62004d50..621601ad 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -535,7 +535,7 @@ async fn remove_2fa(user_id: UserId, token: AdminToken, conn: DbConn) -> EmptyRe let mut user = get_user_or_404(&user_id, &conn).await?; TwoFactor::delete_all_by_user(&user.uuid, &conn).await?; Device::clear_twofactor_remember_by_user(&user.uuid, &conn).await?; - two_factor::enforce_2fa_policy(&user, &ACTING_ADMIN_USER.into(), 14, &token.ip.ip, &conn).await?; + two_factor::enforce_2fa_policy(&user, &ACTING_ADMIN_USER.into(), 14, &token.ip.ip, true, &conn).await?; user.totp_recover = None; user.save(&conn).await } diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index f0d44534..ee2165a0 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -590,23 +590,18 @@ async fn post_bulk_access_collections( err!("Can't find organization details") } - // The collections and members are checked below, the groups only here. - let org_groups = Group::find_by_organization(&org_id, &conn).await; - let org_group_ids: HashSet<&GroupId> = org_groups.iter().map(|g| &g.uuid).collect(); - if let Some(g) = data.groups.iter().find(|g| !org_group_ids.contains(&g.id)) { - err!("Invalid group", format!("Group {} does not belong to organization {}!", g.id.escape_debug(), org_id)) - } - + // As in Bitwarden, missing, repeated or unmanageable collections are Not Found. Invalid groups and members are + // checked after them and are a Bad Request. if data.collection_ids.is_empty() || data.collection_ids.iter().collect::>().len() != data.collection_ids.len() { - err!("Invalid collection list") + err_code!("One or more collections not found.", Status::NotFound.code) } 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") + err_code!("One or more collections not found.", Status::NotFound.code) }; let manageable = if headers.membership.has_type(MembershipType::Custom) { @@ -617,12 +612,22 @@ async fn post_bulk_access_collections( 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") + err_code!( + "Resource not found.", + "The current user isn't a manager for this collection", + Status::NotFound.code + ) } collections.push(collection); } + let org_groups = Group::find_by_organization(&org_id, &conn).await; + let org_group_ids: HashSet<&GroupId> = org_groups.iter().map(|g| &g.uuid).collect(); + if let Some(g) = data.groups.iter().find(|g| !org_group_ids.contains(&g.id)) { + err!("Invalid group", format!("Group {} does not belong to organization {}!", g.id.escape_debug(), org_id)) + } + let mut members = Vec::with_capacity(data.users.len()); for user in &data.users { let Some(member) = Membership::find_by_uuid_and_org(&user.id, &org_id, &conn).await else { @@ -2573,8 +2578,9 @@ async fn enforce_single_org_policy_for_org( // Later membership activation still checks the enabled policy independently. if member.atype < MembershipType::Admin && member.user_uuid != *acting_user_id - && member.status != MembershipStatus::Invited as i32 - && Membership::count_accepted_and_confirmed_by_user(&member.user_uuid, &member.org_uuid, conn).await > 0 + && (member.has_status(MembershipStatus::Accepted) || member.has_status(MembershipStatus::Confirmed)) + && Membership::count_accepted_confirmed_or_revoked_by_user(&member.user_uuid, &member.org_uuid, conn).await + > 0 { if CONFIG.mail_enabled() { let org = Organization::find_by_uuid(&member.org_uuid, conn).await.unwrap(); @@ -2896,6 +2902,14 @@ async fn get_groups_data( || may_assign_collection_access(&headers.membership, &conn).await }; if !allowed { + // Bitwarden's permission requirement on the details answers Forbidden + if details { + err_code!( + "You don't have permission to call this endpoint", + "User does not have access", + Status::Forbidden.code + ); + } err_code!("Resource not found.", "User does not have access", Status::NotFound.code); } @@ -3421,8 +3435,15 @@ async fn recover_account( TwoFactor::delete_all_by_user(&user.uuid, &conn).await?; Device::clear_twofactor_remember_by_user(&user.uuid, &conn).await?; if !fallback_2fa_email || two_factor::email::find_and_activate_email_2fa(&user.uuid, &conn).await.is_err() { - two_factor::enforce_2fa_policy(&user, &headers.user.uuid, headers.device.atype, &headers.ip.ip, &conn) - .await?; + two_factor::enforce_2fa_policy( + &user, + &headers.user.uuid, + headers.device.atype, + &headers.ip.ip, + true, + &conn, + ) + .await?; } } diff --git a/src/api/core/two_factor/mod.rs b/src/api/core/two_factor/mod.rs index 55e374c0..e947abe3 100644 --- a/src/api/core/two_factor/mod.rs +++ b/src/api/core/two_factor/mod.rs @@ -13,8 +13,8 @@ use crate::{ db::{ DbConn, DbPool, models::{ - Device, DeviceType, EventType, Membership, MembershipType, OrgPolicyType, Organization, OrganizationId, - TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId, + Device, DeviceType, EventType, Membership, MembershipStatus, MembershipType, OrgPolicyType, Organization, + OrganizationId, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId, }, }, mail, @@ -127,21 +127,25 @@ async fn generate_recover_code(user: &mut User, conn: &DbConn) { pub async fn check_2fa_state(user: &User, device_type: i32, ip: &std::net::IpAddr, conn: &DbConn) -> EmptyResult { Device::clear_twofactor_remember_by_user(&user.uuid, conn).await?; if TwoFactor::find_by_user(&user.uuid, conn).await.is_empty() { - enforce_2fa_policy(user, &user.uuid, device_type, ip, conn).await?; + enforce_2fa_policy(user, &user.uuid, device_type, ip, true, conn).await?; } Ok(()) } +/// Revokes the user's memberships that the 2FA policy no longer allows. With `accepted_too`, which is for removing the +/// user's last 2FA method, accepted memberships are revoked as well, as in Bitwarden. Otherwise only confirmed ones +/// are: an accepted member still has to set up 2FA before being confirmed. pub async fn enforce_2fa_policy( user: &User, act_user_id: &UserId, device_type: i32, ip: &std::net::IpAddr, + accepted_too: bool, conn: &DbConn, ) -> EmptyResult { for member in Membership::find_by_user_and_policy(&user.uuid, OrgPolicyType::TwoFactorAuthentication, conn).await { // Policy only applies to non-Owner/non-Admin members who have accepted joining the org - if member.atype < MembershipType::Admin { + if member.atype < MembershipType::Admin && (accepted_too || member.has_status(MembershipStatus::Confirmed)) { if CONFIG.mail_enabled() { let org = Organization::find_by_uuid(&member.org_uuid, conn).await.unwrap(); mail::send_2fa_removed_from_org(&user.email, &org.name).await?; @@ -174,9 +178,11 @@ pub async fn enforce_2fa_policy_for_org( conn: &DbConn, ) -> EmptyResult { let org = Organization::find_by_uuid(org_id, conn).await.unwrap(); - for member in Membership::find_confirmed_by_org(org_id, conn).await { - // Don't revoke Admins, Owners, or the authenticated administrator enabling the policy. - if member.atype < MembershipType::Admin + for member in Membership::find_by_org(org_id, conn).await { + // As in Bitwarden, accepted and confirmed members lose access, except Owners, Admins and the member enabling + // the policy. + if (member.has_status(MembershipStatus::Accepted) || member.has_status(MembershipStatus::Confirmed)) + && member.atype < MembershipType::Admin && member.user_uuid != *act_user_id && TwoFactor::find_by_user(&member.user_uuid, conn).await.is_empty() { diff --git a/src/api/identity.rs b/src/api/identity.rs index d7ae5068..e1da7421 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -874,7 +874,7 @@ async fn twofactor_auth( // No twofactor token if twofactor is disabled if twofactors.is_empty() { - enforce_2fa_policy(user, &user.uuid, device.atype, &ip.ip, conn).await?; + enforce_2fa_policy(user, &user.uuid, device.atype, &ip.ip, false, conn).await?; return Ok(None); } @@ -970,7 +970,7 @@ async fn twofactor_auth( Device::clear_twofactor_remember_by_user(&user.uuid, conn).await?; device.delete_twofactor_remember(); - enforce_2fa_policy(user, &user.uuid, device.atype, &ip.ip, conn).await?; + enforce_2fa_policy(user, &user.uuid, device.atype, &ip.ip, true, conn).await?; log_user_event(EventType::UserRecovered2fa, &user.uuid, device.atype, &ip.ip, conn).await; diff --git a/src/db/models/org_policy.rs b/src/db/models/org_policy.rs index 554f51fc..ccb738b2 100644 --- a/src/db/models/org_policy.rs +++ b/src/db/models/org_policy.rs @@ -342,12 +342,20 @@ impl OrgPolicy { if let Some(p) = Self::find_by_org_and_type(&m.org_uuid, OrgPolicyType::SingleOrg, conn).await && p.enabled - && Membership::count_accepted_and_confirmed_by_user(&m.user_uuid, &m.org_uuid, conn).await > 0 { - err!(format!( - "Cannot {} because the organization policy forbids being part of other organization (membership {})", - action, m.uuid - )); + // Joining, confirming and restoring count memberships revoked elsewhere too, as in Bitwarden. Bitwarden + // does not check a modification at all, so that keeps counting only the active ones. + let other_memberships = if action == "modify" { + Membership::count_accepted_and_confirmed_by_user(&m.user_uuid, &m.org_uuid, conn).await + } else { + Membership::count_accepted_confirmed_or_revoked_by_user(&m.user_uuid, &m.org_uuid, conn).await + }; + if other_memberships > 0 { + err!(format!( + "Cannot {} because the organization policy forbids being part of other organization (membership {})", + action, m.uuid + )); + } } } diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index 3c02cb65..4b859406 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -1241,6 +1241,31 @@ impl Membership { .await } + /// Counts the user's accepted and confirmed memberships in other organizations, revoked ones included, which is + /// what Bitwarden's Single organization policy counts, both when it is enabled and when a member joins. + pub async fn count_accepted_confirmed_or_revoked_by_user( + user_uuid: &UserId, + excluded_org: &OrganizationId, + conn: &DbConn, + ) -> i64 { + let statuses = [ + MembershipStatus::Accepted as i32, + MembershipStatus::Confirmed as i32, + MembershipStatus::Accepted as i32 - ACTIVATE_REVOKE_DIFF, + MembershipStatus::Confirmed as i32 - ACTIVATE_REVOKE_DIFF, + ]; + conn.run(move |conn| { + users_organizations::table + .filter(users_organizations::user_uuid.eq(user_uuid)) + .filter(users_organizations::org_uuid.ne(excluded_org)) + .filter(users_organizations::status.eq_any(statuses)) + .count() + .first::(conn) + .unwrap_or(0) + }) + .await + } + pub async fn find_by_org(org_uuid: &OrganizationId, conn: &DbConn) -> Vec { conn.run(move |conn| { users_organizations::table @@ -1251,17 +1276,6 @@ impl Membership { .await } - pub async fn find_confirmed_by_org(org_uuid: &OrganizationId, conn: &DbConn) -> Vec { - conn.run(move |conn| { - users_organizations::table - .filter(users_organizations::org_uuid.eq(org_uuid)) - .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) - .load::(conn) - .unwrap_or_default() - }) - .await - } - // 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. @@ -1444,6 +1458,7 @@ impl Membership { .await } + /// The user's accepted and confirmed memberships in organizations that have enabled `policy_type`. pub async fn find_by_user_and_policy(user_uuid: &UserId, policy_type: OrgPolicyType, conn: &DbConn) -> Vec { conn.run(move |conn| { users_organizations::table @@ -1454,7 +1469,11 @@ impl Membership { .and(org_policies::atype.eq(policy_type as i32)) .and(org_policies::enabled.eq(true))), ) - .filter(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) + .filter( + users_organizations::status + .eq(MembershipStatus::Accepted as i32) + .or(users_organizations::status.eq(MembershipStatus::Confirmed as i32)), + ) .select(users_organizations::all_columns) .load::(conn) .unwrap_or_default()