From 10662371bad683c88054ca946a172c8a7d36d827 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sun, 30 Aug 2026 19:30:58 +0200 Subject: [PATCH] Restrict every revoked membership under the auto confirm policy --- src/api/core/organizations.rs | 4 +- src/db/models/org_policy.rs | 41 +++++++++---------- src/db/models/organization.rs | 77 +++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 23 deletions(-) diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 7d14aaf5..651e67dd 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -2332,8 +2332,8 @@ async fn put_policy( // the members that are not, because this policy also applies to owners and admins and revoking those // could lock the organization out of itself. for member in Membership::find_by_org(&org_id, &conn).await { - if member.status != MembershipStatus::Invited as i32 - && Membership::count_accepted_and_confirmed_by_user(&member.user_uuid, &org_id, &conn).await > 0 + if member.counts_for_auto_confirm() + && Membership::count_accepted_confirmed_and_revoked_by_user(&member.user_uuid, &org_id, &conn).await > 0 { err!("This policy forbids members to be part of other organizations, but at least one member still is.") } diff --git a/src/db/models/org_policy.rs b/src/db/models/org_policy.rs index 686e9ef4..ab83cf1f 100644 --- a/src/db/models/org_policy.rs +++ b/src/db/models/org_policy.rs @@ -362,7 +362,7 @@ impl OrgPolicy { } if Self::is_auto_confirm_enabled(&m.org_uuid, conn).await - && Membership::count_accepted_and_confirmed_by_user(&m.user_uuid, &m.org_uuid, conn).await > 0 + && Membership::count_accepted_confirmed_and_revoked_by_user(&m.user_uuid, &m.org_uuid, conn).await > 0 { err!(format!( "Cannot {} because the organization confirms its members automatically and forbids being part of other organizations (membership {})", @@ -465,13 +465,12 @@ impl AutoConfirmRequirement { /// The user may neither grant nor accept emergency access, which would hand its account, and with it /// the organization vault, to somebody the organization never vetted. /// - /// An open invitation does not count, that account did not join yet and may still decline. A revoked - /// membership does count: it is restored without another accept step, so an emergency access created - /// while revoked would outlive the revocation. Vaultwarden stores a revoked membership as its previous - /// status minus 128, hence the comparison against the unrevoked status. + /// Bitwarden restricts the accepted, the confirmed and the revoked status, and exempts only an open + /// invitation. See [`Membership::counts_for_auto_confirm`] for why a revoked membership is restricted + /// no matter which status it was revoked from. /// Mirrors `GrantorCannotInviteToEmergencyAccess()` and `GranteeCannotAcceptEmergencyAccess()`. pub fn forbids_emergency_access(&self) -> bool { - self.0.iter().any(|m| m.get_unrevoked_status() != MembershipStatus::Invited as i32) + self.0.iter().any(Membership::counts_for_auto_confirm) } } @@ -559,7 +558,7 @@ mod tests { assert!(requirement.forbids_membership_outside(&org("other"))); } - /// Accepted, confirmed and revoked memberships all block emergency access. The revoked ones matter + /// Accepted, confirmed and every revoked membership block emergency access. The revoked ones matter /// because a membership is restored without another accept step, so an emergency access created while /// revoked would survive the restore. #[test] @@ -582,22 +581,22 @@ mod tests { } } - /// An invitation is the one membership emergency access is not restricted by, that account did not - /// join yet and may still decline. Revoking an invitation does not change that. + /// A revoked invitation is revoked like any other membership, so it is restricted too. Bitwarden + /// stores one `Revoked` status without looking at what it was revoked from and restricts all of it. #[test] - fn an_invitation_does_not_forbid_emergency_access() { - let auto_confirm_org = org("auto-confirm"); + fn a_revoked_invitation_forbids_emergency_access() { + let member = revoked(&org("auto-confirm"), MembershipType::User, MembershipStatus::Invited as i32); - for member in [ - membership(&auto_confirm_org, MembershipType::User, MembershipStatus::Invited as i32), - revoked(&auto_confirm_org, MembershipType::User, MembershipStatus::Invited as i32), - ] { - let status = member.status; - assert!( - !AutoConfirmRequirement(vec![member]).forbids_emergency_access(), - "status {status} must keep its emergency access" - ); - } + assert!(AutoConfirmRequirement(vec![member]).forbids_emergency_access()); + } + + /// An open invitation is the only membership emergency access is not restricted by, that account did + /// not join yet and may still decline. + #[test] + fn an_open_invitation_does_not_forbid_emergency_access() { + let member = membership(&org("auto-confirm"), MembershipType::User, MembershipStatus::Invited as i32); + + assert!(!AutoConfirmRequirement(vec![member]).forbids_emergency_access()); } /// One joined membership is enough, even next to an invitation which does not restrict by itself. diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index df7473ea..dfe137ea 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -919,6 +919,44 @@ impl Membership { .await } + /// Whether this membership counts for the automatic user confirmation policy, which exempts no role + /// and only one status: an open invitation, because that account did not join yet and may still + /// decline. An accepted, a confirmed and every revoked membership count, a revoked one because it is + /// restored without another accept step and would come back the moment the other organization + /// restores it. Bitwarden reads the memberships of an account the same way, its invitations are not + /// linked to the account at all while every revoked membership is. + /// https://github.com/bitwarden/server/blob/b3d1eb9a7854322f106efa55c191c1a4da9f8645/src/Core/AdminConsole/OrganizationFeatures/Policies/Enforcement/AutoConfirm/AutomaticUserConfirmationPolicyEnforcementHandler.cs + pub fn counts_for_auto_confirm(&self) -> bool { + self.status != MembershipStatus::Invited as i32 + } + + /// The same rule as [`Membership::counts_for_auto_confirm`] as a query: how many organizations besides + /// `excluded_org` the user belongs to as far as the automatic user confirmation policy is concerned. + /// Contrary to `count_accepted_and_confirmed_by_user`, which the SingleOrg policy uses and which must + /// keep ignoring them, this counts revoked memberships as well. A revoked membership is stored as its + /// previous status minus 128, so every revoked status is below `Invited`. + pub async fn count_accepted_confirmed_and_revoked_by_user( + user_uuid: &UserId, + excluded_org: &OrganizationId, + conn: &DbConn, + ) -> i64 { + 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(MembershipStatus::Accepted as i32) + .or(users_organizations::status.eq(MembershipStatus::Confirmed as i32)) + .or(users_organizations::status.lt(MembershipStatus::Invited as i32)), + ) + .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 @@ -1287,4 +1325,43 @@ mod tests { assert!(MembershipType::Manager > MembershipType::User); assert!(MembershipType::Manager == MembershipType::from_str("4").unwrap()); } + + fn member_with_status(status: i32) -> Membership { + let mut member = + Membership::new(UserId::from(String::from("user")), OrganizationId::from(String::from("org")), None); + member.status = status; + member + } + + /// The rule `count_accepted_confirmed_and_revoked_by_user` encodes as a query, and which decides + /// whether a membership in another organization blocks joining, restoring, emergency access and + /// enabling the policy. Only an open invitation is exempt, every revoked membership counts, no matter + /// which status it was revoked from. + #[test] + fn auto_confirm_counts_everything_but_an_open_invitation() { + for status in [MembershipStatus::Invited, MembershipStatus::Accepted, MembershipStatus::Confirmed] { + let unrevoked = status as i32; + let mut member = member_with_status(unrevoked); + assert_eq!( + member.counts_for_auto_confirm(), + unrevoked != MembershipStatus::Invited as i32, + "status {unrevoked} is counted incorrectly" + ); + + assert!(member.revoke(), "status {unrevoked} can not be revoked"); + assert!(member.counts_for_auto_confirm(), "status {unrevoked} must count once it is revoked"); + } + } + + /// The SingleOrg policy keeps ignoring revoked memberships, so the two rules must not be the same. + /// `count_accepted_and_confirmed_by_user` is the one that stays as it is. + #[test] + fn a_revoked_membership_is_below_invited() { + let mut member = member_with_status(MembershipStatus::Confirmed as i32); + assert!(member.revoke()); + + assert!(member.status < MembershipStatus::Invited as i32); + assert_ne!(member.status, MembershipStatus::Accepted as i32); + assert_ne!(member.status, MembershipStatus::Confirmed as i32); + } }