Restrict every revoked membership under the auto confirm policy

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.")
             }

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.

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::<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
@@ -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);
+    }
 }