Align policy enforcement and status codes with Bitwarden (#7970)

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
 }

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.
+    // As in Bitwarden, missing, repeated or unmanageable collections are Not Found. Invalid groups and members are
-    let org_groups = Group::find_by_organization(&org_id, &conn).await;
+    // checked after them and are a Bad Request.
-    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))
-    }
-
     if data.collection_ids.is_empty()
         || data.collection_ids.iter().collect::<HashSet<_>>().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
+            && (member.has_status(MembershipStatus::Accepted) || member.has_status(MembershipStatus::Confirmed))
-            && Membership::count_accepted_and_confirmed_by_user(&member.user_uuid, &member.org_uuid, conn).await > 0
+            && 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,7 +3435,14 @@ 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)
+            two_factor::enforce_2fa_policy(
+                &user,
+                &headers.user.uuid,
+                headers.device.atype,
+                &headers.ip.ip,
+                true,
+                &conn,
+            )
             .await?;
         }
     }

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,
+            Device, DeviceType, EventType, Membership, MembershipStatus, MembershipType, OrgPolicyType, Organization,
-            TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId,
+            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 {
+    for member in Membership::find_by_org(org_id, conn).await {
-        // Don't revoke Admins, Owners, or the authenticated administrator enabling the policy.
+        // As in Bitwarden, accepted and confirmed members lose access, except Owners, Admins and the member enabling
-        if member.atype < MembershipType::Admin
+        // 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()
         {

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;
 

src/db/models/org_policy.rs

@@ -342,14 +342,22 @@ 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
             {
+                // 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
                     ));
                 }
             }
+        }
 
         Ok(())
     }

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::<i64>(conn)
+                .unwrap_or(0)
+        })
+        .await
+    }
+
     pub async fn find_by_org(org_uuid: &OrganizationId, conn: &DbConn) -> Vec<Self> {
         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<Self> {
-        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::<Self>(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<Self> {
         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::<Self>(conn)
                 .unwrap_or_default()