From 848efbebd63cbdb02684bdb653be2134b0e1f517 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Wed, 5 Aug 2026 22:14:33 +0200 Subject: [PATCH] Close the emergency access gaps of the auto confirm policy Confirming a member into an organization which confirms automatically now drops the emergency access of that member. Enabling the policy only cleaned up the members present at that time, so anybody who joined afterwards kept a grantee which could take over the account of a member nobody ever vetted and reach the organization vault through it. Bitwarden drops these on every confirmation, manual ones included, so this does the same. Enabling the policy no longer touches invited members. An invitation is created by an admin alone, without any consent of the invited user, so it must not be able to delete the emergency access of an account that never joined the organization. The validation and the cleanup now only run on the step from disabled to enabled. The web vault saves a policy on every edit and re-running the cleanup kept wiping emergency access that members created in the meantime. The cleanup also moved behind the save of the policy so that a failed save can no longer destroy data for nothing. While here: only notify members which may actually confirm, `AdminHeaders` rejects the managers that `find_confirmed_and_manage_all_by_org` also returns, and stop unwrapping the client supplied id in the manual bulk confirm. The referenced Bitwarden sources were pinned to a commit which predates the feature and returned 404, they now point at one which contains them. --- src/api/core/mod.rs | 8 ++++- src/api/core/organizations.rs | 60 +++++++++++++++++++++++++++-------- src/db/models/org_policy.rs | 4 +-- 3 files changed, 56 insertions(+), 16 deletions(-) diff --git a/src/api/core/mod.rs b/src/api/core/mod.rs index 7db34f45..fdf2132f 100644 --- a/src/api/core/mod.rs +++ b/src/api/core/mod.rs @@ -293,7 +293,13 @@ pub async fn notify_pending_auto_confirm(member: &Membership, conn: &DbConn, nt: return; } - for admin in Membership::find_confirmed_and_manage_all_by_org(&member.org_uuid, conn).await { + // Confirming requires `AdminHeaders`, so skip the managers this also returns. They could not act on + // the notification anyway and would only run into a rejected request. + for admin in Membership::find_confirmed_and_manage_all_by_org(&member.org_uuid, conn) + .await + .into_iter() + .filter(|m| m.atype >= MembershipType::Admin) + { nt.send_auto_confirm_member(&admin.user_uuid, &member.org_uuid, &member.uuid, &member.user_uuid).await; } } diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index c6dde105..c4dd97bc 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -1388,7 +1388,11 @@ async fn bulk_confirm_invite( match data.keys { Some(keys) => { for invite in keys { - let member_id = invite.id.unwrap(); + // Never unwrap the id, this is client supplied and a missing one must not take the request down + let Some(member_id) = invite.id else { + error!("Ignoring a bulk confirm entry without a member id"); + continue; + }; let user_key = invite.key.unwrap_or_default(); let err_msg = match confirm_invite_impl(&org_id, &member_id, &user_key, &headers, &conn, &nt).await { Ok(()) => String::new(), @@ -1475,6 +1479,16 @@ async fn confirm_member( // This check is also done at accept_invite, _confirm_invite, _activate_member, edit_member, admin::update_membership_type OrgPolicy::check_user_allowed(&member_to_confirm, "confirm", conn).await?; + // An organization which confirms its members automatically does not tolerate emergency access: the + // grantee could take over the account of a member that nobody ever vetted and reach the organization + // vault through it. Enabling the policy drops the grants of the members present at that time, this + // covers the member which brings one along when it joins afterwards. Bitwarden does the same, and + // like there it applies to the manual confirmation as well. + // https://github.com/bitwarden/server/blob/b3d1eb9a7854322f106efa55c191c1a4da9f8645/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/ConfirmOrganizationUserCommand.cs + if OrgPolicy::is_auto_confirm_enabled(&org_id, conn).await { + EmergencyAccess::delete_all_by_user(&member_to_confirm.user_uuid, conn).await?; + } + log_event( EventType::OrganizationUserConfirmed as i32, &member_to_confirm.uuid, @@ -2280,8 +2294,14 @@ async fn put_policy( // The automatic user confirmation policy hands out organization access without anybody looking at it, // so it needs to be allowed by the server first and it requires the Single Org policy on top. - // https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyEventHandlers/AutomaticUserConfirmationPolicyEventHandler.cs - if pol_type_enum == OrgPolicyType::AutomaticUserConfirmation && data.enabled { + // https://github.com/bitwarden/server/blob/b3d1eb9a7854322f106efa55c191c1a4da9f8645/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyEventHandlers/AutomaticUserConfirmationPolicyEventHandler.cs + let auto_confirm_turned_on = if pol_type_enum == OrgPolicyType::AutomaticUserConfirmation + && data.enabled + // Only the step from disabled to enabled validates and has side effects. The web vault saves a + // policy on every edit, and re-running the below on an already enabled policy would keep wiping + // emergency access that members created in the meantime. Bitwarden guards this the same way. + && !OrgPolicy::is_auto_confirm_enabled(&org_id, &conn).await + { if !CONFIG.org_auto_confirm_enabled() { err!("Automatic user confirmation is not enabled on this server.") } @@ -2299,8 +2319,7 @@ async fn put_policy( // Every member has to be compliant already. Contrary to the Single Org policy below we do not revoke // the members that are not, because this policy also applies to owners and admins and revoking those // could lock the organization out of itself. - let members = Membership::find_by_org(&org_id, &conn).await; - for member in &members { + 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 { @@ -2308,14 +2327,10 @@ async fn put_policy( } } - // Emergency access would hand the account of a member to somebody outside of the control of this - // organization, which defeats the point of vetting members. Bitwarden drops these grants when the - // policy is turned on, and blocks new ones while it is on (see `emergency_access.rs`). - for member in &members { - info!("Removing emergency access of {} because automatic user confirmation was enabled", member.user_uuid); - EmergencyAccess::delete_all_by_user(&member.user_uuid, &conn).await?; - } - } + true + } else { + false + }; // Also prevent the Single Org policy to be disabled while automatic user confirmation depends on it if pol_type_enum == OrgPolicyType::SingleOrg @@ -2381,6 +2396,25 @@ async fn put_policy( policy.data = serde_json::to_string(&data.data)?; policy.save(&conn).await?; + // Emergency access would hand the account of a member to somebody outside of the control of this + // organization, which defeats the point of vetting members. Bitwarden drops these grants when the + // policy is turned on, and blocks new ones while it is on (see `emergency_access.rs`). + // This runs after the policy is stored so that a failed save can not destroy data for nothing, and it + // skips invited members on purpose: an invitation is created by an admin without any consent of the + // invited user, so it must never be able to delete data of an account that never joined. + if auto_confirm_turned_on { + for member in Membership::find_by_org(&org_id, &conn).await { + if member.status == MembershipStatus::Invited as i32 { + continue; + } + info!( + "Removing emergency access of {} because automatic user confirmation was enabled for {org_id}", + member.user_uuid + ); + EmergencyAccess::delete_all_by_user(&member.user_uuid, &conn).await?; + } + } + log_event( EventType::PolicyUpdated as i32, policy.uuid.as_ref(), diff --git a/src/db/models/org_policy.rs b/src/db/models/org_policy.rs index 94e58591..a9781323 100644 --- a/src/db/models/org_policy.rs +++ b/src/db/models/org_policy.rs @@ -283,7 +283,7 @@ impl OrgPolicy { /// Returns true if the user is a member of an organization other than `exclude_org_uuid` which has the /// automatic user confirmation policy enabled. Contrary to `is_applicable_to_user` this does not exempt /// owners and admins, the policy applies to every role and every status. - /// https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyRequirements/AutomaticUserConfirmationPolicyRequirement.cs + /// https://github.com/bitwarden/server/blob/b3d1eb9a7854322f106efa55c191c1a4da9f8645/src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyRequirements/AutomaticUserConfirmationPolicyRequirement.cs pub async fn auto_confirm_enabled_for_other_org( user_uuid: &UserId, exclude_org_uuid: &OrganizationId, @@ -360,7 +360,7 @@ impl OrgPolicy { // The automatic user confirmation policy is a stricter variant of the SingleOrg policy, it does not // exempt owners and admins and it applies to every status. Therefore it is checked outside of the // block above. - // https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Core/AdminConsole/OrganizationFeatures/Policies/Enforcement/AutoConfirm/AutomaticUserConfirmationPolicyEnforcementHandler.cs + // https://github.com/bitwarden/server/blob/b3d1eb9a7854322f106efa55c191c1a4da9f8645/src/Core/AdminConsole/OrganizationFeatures/Policies/Enforcement/AutoConfirm/AutomaticUserConfirmationPolicyEnforcementHandler.cs if Self::auto_confirm_enabled_for_other_org(&m.user_uuid, &m.org_uuid, conn).await { err!(format!( "Cannot {} because another organization confirms its members automatically and forbids other memberships (membership {})",