Fix trusted device review findings

src/api/core/accounts.rs

@@ -1,4 +1,4 @@
-use std::collections::{HashMap, HashSet};
+use std::collections::HashSet;
 
 use chrono::Utc;
 use rocket::{
@@ -22,8 +22,8 @@ use crate::{
         models::{
             AuthRequest, AuthRequestId, AuthRequestType, Cipher, CipherId, Device, DeviceId, DeviceType,
             DeviceWithAuthRequest, EmergencyAccess, EmergencyAccessId, EventType, Folder, FolderId, Invitation,
-            Membership, MembershipId, MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization,
-            OrganizationId, Send, SendId, User, UserId, UserKdfType,
+            Membership, MembershipId, MembershipStatus, OrgPolicy, OrgPolicyType, Organization, OrganizationId, Send,
+            SendId, User, UserId, UserKdfType,
         },
     },
     mail,
@@ -816,6 +816,20 @@ struct RotateAccountUnlockData {
     emergency_access_unlock_data: Vec<UpdateEmergencyAccessData>,
     master_password_unlock_data: MasterPasswordUnlockData,
     organization_account_recovery_unlock_data: Vec<UpdateResetPasswordData>,
+    /// The user key, re-wrapped for every device that unlocks the vault without a master password.
+    ///
+    /// Absent rather than empty tells the two generations of clients apart: one that sends this
+    /// rotates the trust of its devices right here, an older one does it afterwards through
+    /// `POST /devices/update-trust` and leaves this out entirely. See `post_rotatekey`.
+    device_key_unlock_data: Option<Vec<UpdateDeviceKeysData>>,
+}
+
+#[derive(Deserialize)]
+#[serde(rename_all = "camelCase")]
+struct UpdateDeviceKeysData {
+    device_id: DeviceId,
+    encrypted_user_key: String,
+    encrypted_public_key: String,
 }
 
 #[derive(Deserialize)]
@@ -845,6 +859,55 @@ struct RotateAccountData {
     sends: Vec<SendData>,
 }
 
+/// Works out what a key rotation has to write to the user's devices.
+///
+/// Returns the devices that keep their trust, each with the user key freshly wrapped for it.
+/// Whatever the user owns beyond that list ends up untrusted, so the caller can hand the result to
+/// `Device::replace_trust` and be done in one transaction.
+///
+/// Mirrors `DeviceRotationValidator` upstream, which refuses a rotation that would quietly drop the
+/// trust of a device the user still relies on. Untrusting is the client's own separate step, and
+/// the current ones take it before they get here.
+/// https://github.com/bitwarden/server/blob/main/src/Api/KeyManagement/Validators/DeviceRotationValidator.cs
+fn validate_device_keydata(
+    updates: &[UpdateDeviceKeysData],
+    existing_devices: &[Device],
+) -> ApiResult<Vec<(DeviceId, String, String)>> {
+    let mut listed: HashSet<&DeviceId> = HashSet::with_capacity(updates.len());
+    let mut rotated = Vec::with_capacity(updates.len());
+
+    for update in updates {
+        if !listed.insert(&update.device_id) {
+            err!("A device was listed more than once in the rotation")
+        }
+
+        let Some(device) = existing_devices.iter().find(|device| device.uuid == update.device_id) else {
+            err!(format!("Device {} does not belong to this user", update.device_id))
+        };
+
+        validate_enc_strings(&[
+            ("encryptedUserKey", &update.encrypted_user_key),
+            ("encryptedPublicKey", &update.encrypted_public_key),
+        ])?;
+
+        // Without its own key pair a device has nothing these two keys could belong to, so it
+        // cannot be put back into a trust and is left to be untrusted instead.
+        if device.holds_private_key() {
+            rotated.push((
+                update.device_id.clone(),
+                update.encrypted_user_key.clone(),
+                update.encrypted_public_key.clone(),
+            ));
+        }
+    }
+
+    if existing_devices.iter().any(|device| device.is_trusted() && !listed.contains(&device.uuid)) {
+        err!("All existing trusted devices must be included in the rotation")
+    }
+
+    Ok(rotated)
+}
+
 fn validate_keydata(
     data: &KeyData,
     existing_ciphers: &[Cipher],
@@ -949,6 +1012,7 @@ async fn post_rotatekey(data: Json<KeyData>, headers: Headers, conn: DbConn, nt:
     // We only rotate the reset password key if it is set.
     existing_memberships.retain(|m| m.reset_password_key.is_some());
     let mut existing_sends = Send::find_by_user(user_id, &conn).await;
+    let existing_devices = Device::find_by_user(user_id, &conn).await;
 
     validate_keydata(
         &data,
@@ -960,6 +1024,11 @@ async fn post_rotatekey(data: Json<KeyData>, headers: Headers, conn: DbConn, nt:
         &headers.user,
     )?;
 
+    let rotated_devices = match data.account_unlock_data.device_key_unlock_data.as_deref() {
+        Some(updates) => Some(validate_device_keydata(updates, &existing_devices)?),
+        None => None,
+    };
+
     // Update folder data
     for folder_data in data.account_data.folders {
         // Skip `null` folder id entries.
@@ -1023,12 +1092,19 @@ async fn post_rotatekey(data: Json<KeyData>, headers: Headers, conn: DbConn, nt:
     }
 
     // Every device holds the previous user key wrapped for itself, which unlocks nothing anymore.
-    // Drop those copies before the new key is written, never after: the other order leaves a window
-    // in which a device still counts as trusted and hands its owner a key that no longer opens the
-    // vault. This way a failure here means the rotation simply did not happen.
-    // The clients re-wrap the new user key for every device right after this via
-    // `POST /devices/update-trust`; whatever they leave out stays untrusted.
-    Device::invalidate_wrapped_user_keys(&headers.user.uuid, &conn).await?;
+    // Settle that here rather than after the account itself: by this point the ciphers have already
+    // been rewritten under the new user key, so a device that holds the new one is the half that
+    // still works if what follows fails. The other order would leave a device counting itself
+    // trusted while handing its owner the key it just stopped needing.
+    match rotated_devices {
+        // The current clients send the re-wrapped user key for every trusted device along with the
+        // rotation, so their trust survives it. Anything they left out is untrusted here.
+        Some(rotated) => Device::replace_trust(&headers.user.uuid, rotated, &conn).await?,
+        // A client old enough to leave the field out does this afterwards through
+        // `POST /devices/update-trust`. Until it does, no device counts as trusted, so the worst it
+        // costs its owner is another login rather than an unlock that fails.
+        None => Device::invalidate_wrapped_user_keys(&headers.user.uuid, &conn).await?,
+    }
 
     // Update user data
     let mut user = headers.user;
@@ -1707,6 +1783,10 @@ struct UpdateDevicesTrustData {
 ///
 /// Every trusted device that is not listed loses its trust: its stored copy of the user key is the
 /// old one and would no longer unlock anything.
+///
+/// The current clients do this as part of the rotation itself and never come here; this is the
+/// route the older ones take, and the only one that can rotate the trust of a single device without
+/// rotating the account. See `post_rotatekey`.
 #[post("/devices/update-trust", data = "<data>")]
 async fn post_devices_update_trust(data: Json<UpdateDevicesTrustData>, headers: Headers, conn: DbConn) -> EmptyResult {
     let data = data.into_inner();
@@ -1718,55 +1798,48 @@ async fn post_devices_update_trust(data: Json<UpdateDevicesTrustData>, headers:
         ("encryptedPublicKey", &data.current_device.encrypted_public_key),
     ])?;
 
-    let mut updates: HashMap<DeviceId, DeviceTrustUpdateData> = HashMap::new();
-    for other in data.other_devices {
-        if other.device_id == headers.device.uuid {
-            err!("The current device cannot also be part of the optional rotation")
-        }
-        validate_enc_strings(&[
-            ("encryptedUserKey", &other.keys.encrypted_user_key),
-            ("encryptedPublicKey", &other.keys.encrypted_public_key),
-        ])?;
-        if updates.insert(other.device_id, other.keys).is_some() {
-            err!("A device was listed more than once in the rotation")
-        }
-    }
-
     let devices = Device::find_by_user(&headers.user.uuid, &conn).await;
     if !devices.iter().any(|device| device.uuid == headers.device.uuid) {
         err!("No device found")
     }
 
-    // Validate everything before writing anything: a rotation that stops halfway would leave the
-    // devices wrapping a mix of the old and the new user key.
-    if let Some(unknown) = updates.keys().find(|device_id| !devices.iter().any(|device| device.uuid == **device_id)) {
-        err!(format!("Device {unknown} does not belong to this user"))
-    }
+    // The current device is written whatever it holds now, as upstream does: it is the one the
+    // caller is speaking from and just proved it can unlock.
+    let mut updates = vec![(
+        headers.device.uuid.clone(),
+        data.current_device.encrypted_user_key,
+        data.current_device.encrypted_public_key,
+    )];
+    let mut listed: HashSet<DeviceId> = HashSet::from([headers.device.uuid.clone()]);
 
-    for mut device in devices {
-        if device.uuid == headers.device.uuid {
-            device.encrypted_user_key = Some(data.current_device.encrypted_user_key.clone());
-            device.encrypted_public_key = Some(data.current_device.encrypted_public_key.clone());
-        } else if let Some(keys) = updates.remove(&device.uuid) {
-            // A rotation clears the wrapped user key of every device, so the listed ones are not
-            // trusted at this point; their key pair is what they are restored from. Without it
-            // there is nothing the two keys could belong to.
-            if !device.holds_private_key() {
-                continue;
+    // Validate everything before writing anything, so one bad entry cannot leave the devices
+    // wrapping a mix of the old and the new user key.
+    for other in data.other_devices {
+        if !listed.insert(other.device_id.clone()) {
+            if other.device_id == headers.device.uuid {
+                err!("The current device cannot also be part of the optional rotation")
             }
-            device.encrypted_user_key = Some(keys.encrypted_user_key);
-            device.encrypted_public_key = Some(keys.encrypted_public_key);
-        } else if device.holds_any_key() {
-            // Not listed, so whatever it still holds wraps the previous user key.
-            device.untrust();
-        } else {
-            continue;
+            err!("A device was listed more than once in the rotation")
         }
 
-        device.save(true, &conn).await?;
+        let Some(device) = devices.iter().find(|device| device.uuid == other.device_id) else {
+            err!(format!("Device {} does not belong to this user", other.device_id))
+        };
+
+        validate_enc_strings(&[
+            ("encryptedUserKey", &other.keys.encrypted_user_key),
+            ("encryptedPublicKey", &other.keys.encrypted_public_key),
+        ])?;
+
+        // A rotation clears the wrapped user key of every device, so the listed ones are not
+        // trusted at this point; their key pair is what they are restored from. Without it there is
+        // nothing the two keys could belong to, so the device is left to be untrusted instead.
+        if device.holds_private_key() {
+            updates.push((other.device_id, other.keys.encrypted_user_key, other.keys.encrypted_public_key));
+        }
     }
 
-    Ok(())
+    Device::replace_trust(&headers.user.uuid, updates, &conn).await
 }
 
 #[derive(Debug, Deserialize)]
@@ -1779,22 +1852,16 @@ struct UntrustDevicesData {
 async fn post_devices_untrust(data: Json<UntrustDevicesData>, headers: Headers, conn: DbConn) -> EmptyResult {
     let data = data.into_inner();
 
-    let mut devices = Device::find_by_user(&headers.user.uuid, &conn).await;
+    let owned: HashSet<DeviceId> =
+        Device::find_by_user(&headers.user.uuid, &conn).await.into_iter().map(|device| device.uuid).collect();
 
     // Check that the user owns all of them first, so a single foreign id does not leave the request
     // half applied.
-    if let Some(unknown) =
-        data.devices.iter().find(|device_id| !devices.iter().any(|device| &device.uuid == *device_id))
-    {
+    if let Some(unknown) = data.devices.iter().find(|device_id| !owned.contains(*device_id)) {
         err!(format!("Device {unknown} does not belong to this user"))
     }
 
-    for device in devices.iter_mut().filter(|device| data.devices.contains(&device.uuid)) {
-        device.untrust();
-        device.save(true, &conn).await?;
-    }
-
-    Ok(())
+    Device::untrust_many(&headers.user.uuid, data.devices, &conn).await
 }
 
 /// Reported by a client that still holds a device key but did not get any keys back from us.
@@ -1834,14 +1901,46 @@ struct AuthRequestRequest {
     atype: i32,
 }
 
+/// Upstream puts `[StringLength(25)]` on the access code, so no client sends more than that.
+/// https://github.com/bitwarden/server/blob/main/src/Core/Auth/Models/Api/Request/AuthRequest/AuthRequestCreateRequestModel.cs
+const MAX_ACCESS_CODE_LENGTH: usize = 25;
+
+/// A base64 SPKI RSA-4096 public key is under a kilobyte; this leaves room for whatever comes next.
+const MAX_REQUEST_PUBLIC_KEY_LENGTH: usize = 4096;
+
+impl AuthRequestRequest {
+    /// Both of these end up stored, and the admin approval route stores a copy per organization the
+    /// user belongs to, so neither may be unbounded. The public key is handed to the answering
+    /// client as base64 to wrap a key against; one that is not base64 at all would break the page
+    /// listing the requests rather than just this one.
+    fn validate(&self) -> EmptyResult {
+        if self.access_code.is_empty() || self.access_code.len() > MAX_ACCESS_CODE_LENGTH {
+            err!("Invalid access code")
+        }
+
+        if self.public_key.is_empty()
+            || self.public_key.len() > MAX_REQUEST_PUBLIC_KEY_LENGTH
+            || data_encoding::BASE64.decode(self.public_key.as_bytes()).is_err()
+        {
+            err!("Invalid public key")
+        }
+
+        Ok(())
+    }
+}
+
 fn auth_request_json(auth_request: &AuthRequest) -> Value {
     json!({
         "id": auth_request.uuid,
         "publicKey": auth_request.public_key,
         "type": auth_request.atype,
         "requestDeviceType": DeviceType::from_i32(auth_request.device_type).to_string(),
+        // The clients read the raw enum value as well, to pick an icon for the asking device.
+        "requestDeviceTypeValue": auth_request.device_type,
         "requestDeviceIdentifier": auth_request.request_device_identifier,
         "requestIpAddress": auth_request.request_ip,
+        // Not recorded here, but the clients read it, so it is answered rather than missing.
+        "requestCountryName": null,
         "key": auth_request.enc_key,
         "masterPasswordHash": auth_request.master_password_hash,
         "creationDate": format_date(&auth_request.creation_date),
@@ -1867,6 +1966,8 @@ async fn post_auth_request(
         err!("You must be authenticated to create a request of that type")
     }
 
+    data.validate()?;
+
     let Some(user) = User::find_by_mail(&data.email, &conn).await else {
         err!("AuthRequest doesn't exist", "User not found")
     };
@@ -1928,6 +2029,8 @@ async fn post_admin_auth_request(data: Json<AuthRequestRequest>, headers: Header
         err!("AuthRequest doesn't exist", "Device verification failed")
     }
 
+    data.validate()?;
+
     // Only an organization the user really belongs to can answer for them. A pending invitation is
     // not a membership yet, and a revoked one is not one anymore; sending either of them the email
     // address, the address and the device of the asker is more than they are owed.
@@ -2009,10 +2112,12 @@ async fn notify_device_approval_requested(user: &User, org_id: &OrganizationId,
         return;
     };
 
+    // The same set that may answer the request, see `ManageResetPasswordHeaders`. Mailing anyone
+    // else would tell them who is asking for something they cannot do anything about.
     let approvers = Membership::find_confirmed_by_org(org_id, conn)
         .await
         .into_iter()
-        .filter(|member| member.atype <= MembershipType::Admin as i32);
+        .filter(Membership::has_manage_reset_password_permission);
 
     for approver in approvers {
         let Some(admin) = User::find_by_uuid(&approver.user_uuid, conn).await else {
@@ -2112,7 +2217,7 @@ async fn put_auth_request(
         auth_request.save(&conn).await?;
 
         ant.send_auth_response(&auth_request.user_uuid, &auth_request.uuid).await;
-        nt.send_auth_response(&auth_request.user_uuid, &auth_request.uuid, &headers.device, &conn).await;
+        nt.send_auth_response(&auth_request.user_uuid, &auth_request.uuid, Some(&headers.device), &conn).await;
 
         log_user_event(
             EventType::OrganizationUserApprovedAuthRequest as i32,
@@ -2209,3 +2314,116 @@ pub async fn purge_auth_requests(pool: DbPool) {
         error!("Failed to get DB connection while purging auth requests");
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn device(id: &str, trusted: bool) -> Device {
+        let mut device = Device::new(id.to_owned().into(), String::from("user").into(), String::new(), 9);
+        if trusted {
+            device.encrypted_user_key = Some(String::from("4.b2xkdXNlcmtleQ=="));
+            device.encrypted_public_key = Some(String::from("2.aXY=|Y2lwaGVy|bWFj"));
+            device.encrypted_private_key = Some(String::from("2.aXY=|Y2lwaGVy|bWFj"));
+        }
+        device
+    }
+
+    fn update(device_id: &str) -> UpdateDeviceKeysData {
+        UpdateDeviceKeysData {
+            device_id: device_id.to_owned().into(),
+            encrypted_user_key: String::from("4.bmV3dXNlcmtleQ=="),
+            encrypted_public_key: String::from("2.aXY=|bmV3|bWFj"),
+        }
+    }
+
+    /// The ids and keys the rotation would write, so a test can say what it expects in one line.
+    fn rotated(result: &[(DeviceId, String, String)]) -> Vec<String> {
+        result.iter().map(|(device_id, user_key, _)| format!("{device_id}={user_key}")).collect()
+    }
+
+    #[test]
+    fn a_trusted_device_that_is_listed_keeps_its_trust() {
+        let devices = [device("a", true), device("b", true)];
+        let updates = [update("a"), update("b")];
+
+        let result = validate_device_keydata(&updates, &devices).unwrap();
+        assert_eq!(
+            rotated(&result),
+            ["a=4.bmV3dXNlcmtleQ==", "b=4.bmV3dXNlcmtleQ=="],
+            "both are re-wrapped, neither keeps the previous user key"
+        );
+    }
+
+    #[test]
+    fn a_trusted_device_that_is_left_out_takes_the_rotation_down_with_it() {
+        // Silently dropping the trust of a device the user still relies on is not the server's call
+        // to make; the client untrusts it first if that is what it means.
+        let devices = [device("a", true), device("b", true)];
+
+        let err = validate_device_keydata(&[update("a")], &devices).unwrap_err();
+        assert!(format!("{err}").contains("All existing trusted devices must be included"));
+    }
+
+    #[test]
+    fn a_device_of_somebody_else_is_refused() {
+        let devices = [device("a", true)];
+
+        let err = validate_device_keydata(&[update("a"), update("stranger")], &devices).unwrap_err();
+        assert!(format!("{err}").contains("does not belong to this user"));
+    }
+
+    #[test]
+    fn the_same_device_may_not_be_listed_twice() {
+        // Two entries for one device means one of the two keys is dropped without anyone noticing
+        // which, so neither is taken.
+        let devices = [device("a", true)];
+
+        let err = validate_device_keydata(&[update("a"), update("a")], &devices).unwrap_err();
+        assert!(format!("{err}").contains("listed more than once"));
+    }
+
+    #[test]
+    fn a_key_that_is_not_an_encrypted_string_is_refused() {
+        let devices = [device("a", true)];
+
+        let mut broken = update("a");
+        broken.encrypted_user_key = String::from("not an enc string");
+        let err = validate_device_keydata(&[broken], &devices).unwrap_err();
+        assert!(format!("{err}").contains("encryptedUserKey"));
+
+        let mut broken = update("a");
+        broken.encrypted_public_key = String::new();
+        let err = validate_device_keydata(&[broken], &devices).unwrap_err();
+        assert!(format!("{err}").contains("encryptedPublicKey"));
+    }
+
+    #[test]
+    fn a_device_without_its_own_key_pair_is_not_given_a_user_key() {
+        // Half a trust is worth nothing to the client and would only fail at the next unlock, so
+        // the device is dropped from the rotation and ends up untrusted instead.
+        let devices = [device("a", true), device("b", false)];
+
+        let result = validate_device_keydata(&[update("a"), update("b")], &devices).unwrap();
+        assert_eq!(rotated(&result), ["a=4.bmV3dXNlcmtleQ=="]);
+    }
+
+    #[test]
+    fn a_user_who_trusts_no_device_rotates_nothing() {
+        let devices = [device("a", false)];
+
+        let result = validate_device_keydata(&[], &devices).unwrap();
+        assert!(result.is_empty(), "and the leftovers of `a` are cleared by the write that follows");
+    }
+
+    #[test]
+    fn a_partially_trusted_device_does_not_have_to_be_listed() {
+        // It cannot unlock anything as it stands, so leaving it out is not the loss of a trust.
+        let mut half = device("b", true);
+        half.encrypted_user_key = None;
+        let devices = [device("a", true), half];
+
+        let result = validate_device_keydata(&[update("a")], &devices).unwrap();
+        assert_eq!(rotated(&result), ["a=4.bmV3dXNlcmtleQ=="]);
+    }
+}

src/api/core/organizations.rs

@@ -12,13 +12,16 @@ use crate::{
         AnonymousNotify, EmptyResult, JsonResult, Notify, PasswordOrOtpData, UpdateType,
         core::{CipherSyncData, CipherSyncType, accept_org_invite, log_event, two_factor},
     },
-    auth::{AdminHeaders, Headers, ManagerHeaders, ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders, decode_invite},
+    auth::{
+        AdminHeaders, Headers, ManageResetPasswordHeaders, ManagerHeaders, ManagerHeadersLoose, OrgMemberHeaders,
+        OwnerHeaders, decode_invite,
+    },
     db::{
         DbConn,
         models::{
             AuthRequest, AuthRequestId, Cipher, CipherId, Collection, CollectionCipher, CollectionGroup, CollectionId,
-            CollectionUser, Device, DeviceType, EventType, Group, GroupId, GroupUser, Invitation, Membership,
-            MembershipId, MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey,
+            CollectionUser, DeviceType, EventType, Group, GroupId, GroupUser, Invitation, Membership, MembershipId,
+            MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey,
             OrganizationId, User, UserId,
         },
     },
@@ -3214,7 +3217,11 @@ async fn put_reset_password_enrollment(
 
 /// The requests waiting for an answer in this organization.
 #[get("/organizations/<org_id>/auth-requests")]
-async fn get_organization_auth_requests(org_id: OrganizationId, headers: AdminHeaders, conn: DbConn) -> JsonResult {
+async fn get_organization_auth_requests(
+    org_id: OrganizationId,
+    headers: ManageResetPasswordHeaders,
+    conn: DbConn,
+) -> JsonResult {
     if org_id != headers.org_id {
         err!("Organization not found", "Organization id's do not match");
     }
@@ -3290,7 +3297,7 @@ async fn update_organization_auth_request(
     org_id: OrganizationId,
     request_id: AuthRequestId,
     data: Json<AdminAuthRequestUpdateData>,
-    headers: AdminHeaders,
+    headers: ManageResetPasswordHeaders,
     conn: DbConn,
     ant: AnonymousNotify<'_>,
     nt: Notify<'_>,
@@ -3314,7 +3321,7 @@ async fn update_organization_auth_request(
 async fn deny_organization_auth_requests(
     org_id: OrganizationId,
     data: Json<BulkDenyAuthRequestData>,
-    headers: AdminHeaders,
+    headers: ManageResetPasswordHeaders,
     conn: DbConn,
     ant: AnonymousNotify<'_>,
     nt: Notify<'_>,
@@ -3346,7 +3353,7 @@ async fn deny_organization_auth_requests(
 async fn update_many_organization_auth_requests(
     org_id: OrganizationId,
     data: Json<Vec<OrganizationAuthRequestUpdateData>>,
-    headers: AdminHeaders,
+    headers: ManageResetPasswordHeaders,
     conn: DbConn,
     ant: AnonymousNotify<'_>,
     nt: Notify<'_>,
@@ -3381,7 +3388,7 @@ async fn answer_organization_auth_request(
     approved: bool,
     encrypted_user_key: Option<String>,
     on_unanswerable: OnUnanswerable,
-    headers: &AdminHeaders,
+    headers: &ManageResetPasswordHeaders,
     conn: &DbConn,
     ant: &AnonymousNotify<'_>,
     nt: &Notify<'_>,
@@ -3453,14 +3460,11 @@ async fn answer_organization_auth_request(
 
     ant.send_auth_response(&auth_request.user_uuid, &auth_request.uuid).await;
 
-    // The device that asked, not the one the administrator happens to be answering from: that one
-    // belongs to somebody else, and naming it here would both address the notification at a device
-    // of the wrong account and hand its identifiers to the push relay under a foreign user id.
-    if let Some(device) =
-        Device::find_by_uuid_and_user(&auth_request.request_device_identifier, &auth_request.user_uuid, conn).await
-    {
-        nt.send_auth_response(&auth_request.user_uuid, &auth_request.uuid, &device, conn).await;
-    }
+    // No acting device: the answer did not come from one of this user's devices, so every one of
+    // them, the one that asked above all, should hear about it. Naming the administrator's device
+    // here would leave it out of a notification meant for somebody else's account and hand its
+    // identifiers to the push relay under a foreign user id.
+    nt.send_auth_response(&auth_request.user_uuid, &auth_request.uuid, None, conn).await;
 
     if CONFIG.mail_enabled()
         && let Some(user) = User::find_by_uuid(&auth_request.user_uuid, conn).await

src/api/identity.rs

@@ -31,8 +31,8 @@ use crate::{
         DbConn,
         models::{
             AuthRequest, AuthRequestId, Device, DeviceId, DeviceType, EventType, Invitation, Membership,
-            MembershipStatus, MembershipType, OIDCCodeResponseError, OrganizationApiKey, OrganizationId, SendId,
-            SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId,
+            MembershipStatus, OIDCCodeResponseError, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey,
+            OrganizationId, SendId, SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId,
         },
     },
     error::MapResult,
@@ -481,16 +481,98 @@ async fn password_login(
     authenticated_response(&user, &mut device, auth_tokens, twofactor_token, false, conn, ip).await
 }
 
-/// Whether offering the trusted device options can lead anywhere for this account.
+/// Whether the account creation the clients run when nothing else is on offer can succeed here.
 ///
-/// Creating an account this way ends with enrolling into account recovery, which the clients do
-/// unconditionally and which needs an organization to enroll into. An account that has nothing yet
-/// and belongs to nowhere would therefore be shown the screen for a new account and get stuck
-/// halfway through it, with its keys already written and its device still untrusted. Withholding
-/// the options sends it to setting a master password instead, which works and leaves the door to
-/// trusted devices open for the next login.
-fn trusted_device_flow_is_completable(has_account_keys: bool, in_organization: bool) -> bool {
-    has_account_keys || in_organization
+/// A client that gets the trusted device options, but neither a master password nor an approval an
+/// administrator could give, decides it is looking at a fresh account and walks it through
+/// creation: generate the account keys and post them, enrol into the account recovery of the
+/// organization behind the SSO login, then trust the device. Every one of those has to be able to
+/// go through. If enrolment is refused, the keys are already written, and the next login walks into
+/// the same screen and fails at posting them a second time, leaving an account that can never be
+/// unlocked at all.
+///
+/// So the same conditions the enrolment endpoint enforces are checked here, before the client has
+/// written anything. Withholding the options instead sends it to setting a master password, which
+/// works and leaves the door to trusted devices open for the next login.
+async fn account_creation_can_succeed(user: &User, conn: &DbConn) -> bool {
+    // `POST /accounts/keys` refuses to replace the keys of an account that has them, and the
+    // clients post a freshly generated pair without looking.
+    if user.private_key.is_some() || user.public_key.is_some() {
+        return false;
+    }
+
+    // The organization the client enrols into is the one `GET /organizations/<identifier>/auto-enroll-status`
+    // hands it, so ask the same question here.
+    let Some(membership) = Membership::find_main_user_org(&user.uuid, conn).await else {
+        return false;
+    };
+
+    // What `check_reset_password_applicable` demands of that organization.
+    if !CONFIG.mail_enabled() {
+        return false;
+    }
+    if !OrgPolicy::find_by_org_and_type(&membership.org_uuid, OrgPolicyType::ResetPassword, conn)
+        .await
+        .is_some_and(|policy| policy.enabled)
+    {
+        return false;
+    }
+
+    // Enrolling wraps the user key for the organization, so it needs its public key.
+    Organization::find_by_uuid(&membership.org_uuid, conn)
+        .await
+        .is_some_and(|org| org.public_key.is_some_and(|key| !key.is_empty()))
+}
+
+/// The ways an account could get through the trusted device flow, which is what decides whether
+/// offering it leads anywhere.
+#[expect(
+    clippy::struct_excessive_bools,
+    reason = "Four independent facts about one account, not a state that could be an enum"
+)]
+struct TrustedDeviceWaysIn {
+    /// This device already holds the keys, so it unlocks without asking anyone.
+    device_is_trusted: bool,
+    /// A master password to fall back on.
+    has_master_password: bool,
+    /// An administrator of an organization who could let a new device in, which they can only do
+    /// once the member enrolled into account recovery.
+    has_admin_approval: bool,
+    /// Nothing set up yet, but the account creation the clients run in that case would go through.
+    can_create_account: bool,
+}
+
+impl TrustedDeviceWaysIn {
+    /// Whether the trusted device options belong in a login response, and in which of their two
+    /// roles.
+    ///
+    /// `Some(true)` means they are only there to walk a user without a master password off the
+    /// feature after it was switched off; `None` means they are withheld, because nothing the
+    /// client could do with them would work.
+    ///
+    /// The order mirrors how the clients read them: a trusted device unlocks straight away,
+    /// otherwise an administrator to ask or a master password to type is offered, and only when
+    /// there is neither does the client decide it is looking at a fresh account and try to create
+    /// one.
+    fn offer(&self, enabled: bool) -> Option<bool> {
+        // Once the feature is switched off again, a user without a master password would be locked
+        // out of their own vault. Keep telling their still trusted devices about it so their client
+        // can walk them through setting one while they can still unlock.
+        let offboarding = !enabled && self.offboarding_candidate();
+        if !(enabled || offboarding) {
+            return None;
+        }
+
+        let leads_somewhere =
+            self.device_is_trusted || self.has_admin_approval || self.has_master_password || self.can_create_account;
+        leads_somewhere.then_some(offboarding)
+    }
+
+    /// A user who is still on a trusted device and has no master password to fall back on, and so
+    /// has to be told when the feature goes away.
+    fn offboarding_candidate(&self) -> bool {
+        self.device_is_trusted && !self.has_master_password
+    }
 }
 
 /// Trusted device encryption ("passwordless SSO"): instead of deriving the user key from a master
@@ -504,20 +586,37 @@ fn trusted_device_flow_is_completable(has_account_keys: bool, in_organization: b
 async fn trusted_device_option(user: &User, device: &Device, conn: &DbConn) -> Option<Value> {
     let enabled = CONFIG.sso_trusted_device_encryption();
 
-    // Once the feature is switched off again, a user without a master password would be locked out
-    // of their own vault. Keep telling their still trusted devices about it so their client can walk
-    // them through setting one while they can still unlock.
-    let offboarding = !enabled && device.is_trusted() && user.password_hash.is_empty();
-    if !enabled && !offboarding {
+    let mut ways_in = TrustedDeviceWaysIn {
+        device_is_trusted: device.is_trusted(),
+        has_master_password: !user.password_hash.is_empty(),
+        has_admin_approval: false,
+        can_create_account: false,
+    };
+
+    // Answered ahead of everything else so a server that does not offer trusted devices, and has no
+    // user left on them, does no work for the feature at all.
+    if !(enabled || ways_in.offboarding_candidate()) {
         return None;
     }
 
     let memberships = Membership::find_by_user(&user.uuid, conn).await;
 
-    if !trusted_device_flow_is_completable(user.private_key.is_some(), !memberships.is_empty()) {
-        return None;
+    // An admin can only take over the approval once the member handed them a key to work with,
+    // which is what enrolling into account recovery does. Only a confirmed membership counts, the
+    // same condition the request itself is created and answered under, so this does not announce a
+    // way out that would be refused the moment it is taken.
+    ways_in.has_admin_approval = memberships.iter().any(|member| {
+        member.status == MembershipStatus::Confirmed as i32
+            && member.reset_password_key.as_ref().is_some_and(|key| !key.is_empty())
+    });
+
+    // Only worth asking when nothing cheaper already lets the client in.
+    if !(ways_in.device_is_trusted || ways_in.has_admin_approval || ways_in.has_master_password) {
+        ways_in.can_create_account = account_creation_can_succeed(user, conn).await;
     }
 
+    let offboarding = ways_in.offer(enabled)?;
+
     // Any other device of this user that could show an approval prompt. The user unlocks a new
     // device from one of these, or with the master password if they have one.
     let has_login_approving_device = Device::find_by_user(&user.uuid, conn)
@@ -525,24 +624,14 @@ async fn trusted_device_option(user: &User, device: &Device, conn: &DbConn) -> O
         .iter()
         .any(|other| other.uuid != device.uuid && DeviceType::from_i32(other.atype).can_approve_login_requests());
 
-    // An admin can only take over the approval once the member handed them a key to work with,
-    // which is what enrolling into account recovery does. Only a confirmed membership counts, the
-    // same condition the request itself is created and answered under, so this does not announce a
-    // way out that would be refused the moment it is taken.
-    let has_admin_approval = memberships.iter().any(|member| {
-        member.status == MembershipStatus::Confirmed as i32
-            && member.reset_password_key.as_ref().is_some_and(|key| !key.is_empty())
-    });
-
     // Whether the user is on the answering side of that. The clients use it to push someone who
-    // could approve others, but has no master password themselves, into setting one. Matches what
-    // `AdminHeaders` actually lets through.
-    let has_manage_reset_password_permission = memberships.iter().any(|member| {
-        member.status == MembershipStatus::Confirmed as i32 && member.atype <= MembershipType::Admin as i32
-    });
+    // could approve others, but has no master password themselves, into setting one. Upstream reads
+    // a `ManageResetPassword` permission here, which in Vaultwarden's role model only the
+    // administrators of an organization have.
+    let has_manage_reset_password_permission = memberships.iter().any(Membership::has_manage_reset_password_permission);
 
     Some(json!({
-        "HasAdminApproval": has_admin_approval,
+        "HasAdminApproval": ways_in.has_admin_approval,
         "HasLoginApprovingDevice": has_login_approving_device,
         "HasManageResetPasswordPermission": has_manage_reset_password_permission,
         "IsTdeOffboarding": offboarding,
@@ -1407,19 +1496,132 @@ async fn authorize(data: AuthorizeData, cookies: &CookieJar<'_>, secure: Secure,
 mod tests {
     use super::*;
 
+    /// A `TrustedDeviceWaysIn` plus the server setting, so the cases below read as what they are.
+    #[expect(clippy::struct_excessive_bools, reason = "Mirrors the struct under test")]
+    struct Account {
+        enabled: bool,
+        device_is_trusted: bool,
+        has_master_password: bool,
+        has_admin_approval: bool,
+        can_create_account: bool,
+    }
+
+    impl Account {
+        /// A user of a server that offers trusted devices, on a device it does not know yet, with
+        /// nothing set up: the shape everything below varies from.
+        fn new() -> Self {
+            Self {
+                enabled: true,
+                device_is_trusted: false,
+                has_master_password: false,
+                has_admin_approval: false,
+                can_create_account: false,
+            }
+        }
+
+        fn offer(&self) -> Option<bool> {
+            TrustedDeviceWaysIn {
+                device_is_trusted: self.device_is_trusted,
+                has_master_password: self.has_master_password,
+                has_admin_approval: self.has_admin_approval,
+                can_create_account: self.can_create_account,
+            }
+            .offer(self.enabled)
+        }
+    }
+
+    #[test]
+    fn a_server_that_does_not_offer_trusted_devices_says_nothing_about_them() {
+        for (device_is_trusted, has_master_password) in [(false, false), (false, true), (true, true)] {
+            let account = Account {
+                enabled: false,
+                device_is_trusted,
+                has_master_password,
+                ..Account::new()
+            };
+            assert_eq!(account.offer(), None);
+        }
+    }
+
     #[test]
-    fn an_account_with_nothing_and_nowhere_to_go_is_not_offered_trusted_devices() {
-        // The one combination the clients cannot finish: nothing set up yet and no organization
-        // to enroll into.
-        assert!(!trusted_device_flow_is_completable(false, false));
-
-        // A brand new account that was invited somewhere can enroll, so the flow completes.
-        assert!(trusted_device_flow_is_completable(false, true));
-
-        // An account that is already set up does not go through account creation at all, with or
-        // without an organization. This covers the master password first route as well as an
-        // account that already trusts a device.
-        assert!(trusted_device_flow_is_completable(true, false));
-        assert!(trusted_device_flow_is_completable(true, true));
+    fn a_user_left_on_a_trusted_device_is_walked_off_the_feature() {
+        // The feature is gone but this device still unlocks and its owner has no master password.
+        // They are told so, so their client can walk them through setting one while they still can.
+        let account = Account {
+            enabled: false,
+            device_is_trusted: true,
+            ..Account::new()
+        };
+        assert_eq!(account.offer(), Some(true), "offboarding");
+
+        // With the feature on, the same device is simply trusted.
+        let account = Account {
+            device_is_trusted: true,
+            ..Account::new()
+        };
+        assert_eq!(account.offer(), Some(false));
+    }
+
+    #[test]
+    fn an_account_with_no_way_through_the_flow_is_not_offered_it() {
+        // Nothing set up, nobody to ask, and account creation would fail at the enrolment: the one
+        // combination that would leave the account half built. The client is sent to setting a
+        // master password instead.
+        assert_eq!(Account::new().offer(), None);
+    }
+
+    #[test]
+    fn every_way_through_the_flow_is_offered_it() {
+        // A device that can unlock right now.
+        assert_eq!(
+            Account {
+                device_is_trusted: true,
+                ..Account::new()
+            }
+            .offer(),
+            Some(false)
+        );
+
+        // An administrator to ask, which needs the member to be enrolled in account recovery.
+        assert_eq!(
+            Account {
+                has_admin_approval: true,
+                ..Account::new()
+            }
+            .offer(),
+            Some(false)
+        );
+
+        // A master password to fall back on.
+        assert_eq!(
+            Account {
+                has_master_password: true,
+                ..Account::new()
+            }
+            .offer(),
+            Some(false)
+        );
+
+        // A fresh account in an organization that can actually take the enrolment.
+        assert_eq!(
+            Account {
+                can_create_account: true,
+                ..Account::new()
+            }
+            .offer(),
+            Some(false)
+        );
+    }
+
+    #[test]
+    fn a_user_with_a_master_password_is_never_offboarded() {
+        // There is nothing to walk them off, they can unlock either way.
+        let account = Account {
+            enabled: false,
+            device_is_trusted: true,
+            has_master_password: true,
+            ..Account::new()
+        };
+        assert_eq!(account.offer(), None);
     }
 }

src/api/notifications.rs

@@ -529,11 +529,14 @@ impl WebSocketUsers {
         }
     }
 
+    /// `acting_device` is the device of this user that answered the request, and is the one left out
+    /// of the notification. An answer that came from outside their devices, as an approval by an
+    /// administrator of their organization does, passes `None` so that all of them hear about it.
     pub async fn send_auth_response(
         &self,
         user_id: &UserId,
         auth_request_id: &AuthRequestId,
-        device: &Device,
+        acting_device: Option<&Device>,
         conn: &DbConn,
     ) {
         // Skip any processing if both WebSockets and Push are not active
@@ -543,14 +546,14 @@ impl WebSocketUsers {
         let data = create_update(
             vec![("Id".into(), auth_request_id.to_string().into()), ("UserId".into(), user_id.to_string().into())],
             UpdateType::AuthRequestResponse,
-            Some(device.uuid.clone()),
+            acting_device.map(|device| device.uuid.clone()),
         );
         if CONFIG.enable_websocket() {
             self.send_update(user_id, &data).await;
         }
 
         if CONFIG.push_enabled() {
-            push_auth_response(user_id, auth_request_id, device, conn).await;
+            push_auth_response(user_id, auth_request_id, acting_device, conn).await;
         }
     }
 }

src/api/push.rs

@@ -317,13 +317,23 @@ pub async fn push_auth_request(user_id: &UserId, auth_request_id: &str, device:
     }
 }
 
-pub async fn push_auth_response(user_id: &UserId, auth_request_id: &AuthRequestId, device: &Device, conn: &DbConn) {
+/// `acting_device` is the device that answered, and is the one device left out of the notification,
+/// since it already knows. An answer that did not come from a device of this user at all, as an
+/// approval by an administrator of their organization does, leaves it out: naming a device of
+/// somebody else here would both hand its identifiers to the push relay under a foreign user id and
+/// tell the wrong device to ignore the answer.
+pub async fn push_auth_response(
+    user_id: &UserId,
+    auth_request_id: &AuthRequestId,
+    acting_device: Option<&Device>,
+    conn: &DbConn,
+) {
     if Device::check_user_has_push_device(user_id, conn).await {
         tokio::task::spawn(send_to_push_relay(json!({
             "userId": user_id,
             "organizationId": null,
-            "deviceId": device.push_uuid, // Should be the records unique uuid of the acting device (unique uuid per user/device)
-            "identifier": device.uuid, // Should be the acting device id (aka uuid per device/app)
+            "deviceId": acting_device.and_then(|device| device.push_uuid.as_ref()), // Should be the records unique uuid of the acting device (unique uuid per user/device)
+            "identifier": acting_device.map(|device| &device.uuid), // Should be the acting device id (aka uuid per device/app)
             "type": UpdateType::AuthRequestResponse as i32,
             "payload": {
                 "userId": user_id,

src/auth.rs

@@ -843,6 +843,41 @@ impl<'r> FromRequest<'r> for AdminHeaders {
     }
 }
 
+/// A member who may act on the account recovery of an organization, which is also what answering
+/// its device approvals comes down to.
+///
+/// Upstream guards those endpoints on a permission, `ManageResetPassword`, rather than on a role,
+/// so this asks `Membership::has_manage_reset_password_permission` instead of naming roles here.
+/// Today that permission belongs to the administrators of an organization and to nobody else, which
+/// makes this the same set of callers as `AdminHeaders`; keeping it apart is what lets a custom role
+/// hold the permission later without every endpoint having to be revisited.
+/// https://github.com/bitwarden/server/blob/main/src/Api/AdminConsole/Controllers/OrganizationAuthRequestsController.cs
+pub struct ManageResetPasswordHeaders {
+    pub device: Device,
+    pub user: User,
+    pub ip: ClientIp,
+    pub org_id: OrganizationId,
+}
+
+#[rocket::async_trait]
+impl<'r> FromRequest<'r> for ManageResetPasswordHeaders {
+    type Error = &'static str;
+
+    async fn from_request(request: &'r Request<'_>) -> Outcome<Self, Self::Error> {
+        let headers = try_outcome!(OrgHeaders::from_request(request).await);
+        if headers.membership.has_manage_reset_password_permission() {
+            Outcome::Success(Self {
+                device: headers.device,
+                user: headers.user,
+                ip: headers.ip,
+                org_id: headers.membership.org_uuid,
+            })
+        } else {
+            err_handler!("You need permission to manage account recovery to call this endpoint")
+        }
+    }
+}
+
 // col_id is usually the fourth path param ("/organizations/<org_id>/collections/<col_id>"),
 // but there could be cases where it is a query value.
 // First check the path, if this is not a valid uuid, try the query values.

src/db/models/auth_request.rs

@@ -142,8 +142,13 @@ impl AuthRequest {
         })
     }
 
-    /// What an administrator gets to see about a request. Deliberately without the access code:
-    /// that one is the requesting device's proof, not something the answering side needs.
+    /// What an administrator gets to see about a request that is waiting for them, which is the
+    /// public key of the asking device and enough about it to recognise it. Same shape as
+    /// `PendingOrganizationAuthRequestResponseModel` upstream.
+    ///
+    /// Deliberately no access code, which is the asking device's own proof, and no wrapped key: a
+    /// request that is still waiting has none, and handing one out here would be crypto material
+    /// the answering side has no use for.
     pub fn to_json_for_organization(&self, email: &str, member_id: &MembershipId) -> Value {
         json!({
             "id": self.uuid,
@@ -154,11 +159,10 @@ impl AuthRequest {
             "requestDeviceIdentifier": self.request_device_identifier,
             "requestDeviceType": DeviceType::from_i32(self.device_type).to_string(),
             "requestIpAddress": self.request_ip,
-            "key": self.enc_key,
+            // Not recorded here, but the clients read it, so it is answered rather than missing.
+            "requestCountryName": null,
             "creationDate": format_date(&self.creation_date),
-            "requestApproved": self.approved,
-            "responseDate": self.response_date.as_ref().map(format_date),
-            "object": "organizationAuthRequest",
+            "object": "pending-org-auth-request",
         })
     }
 }
@@ -252,12 +256,18 @@ impl AuthRequest {
     ///
     /// Asking again from the same device updates that one instead of adding another, so a client
     /// that retries cannot fill the table or mail the administrators over and over.
+    ///
+    /// A request past its window does not count: it is one nobody can answer any more, and reviving
+    /// it by moving its date forward would leave the user waiting on a request the administrators
+    /// were never told about. Asking again after it ran out is a new request, and is announced.
     pub async fn find_pending_admin_approval(
         user_uuid: &UserId,
         device_uuid: &DeviceId,
         org_uuid: &OrganizationId,
         conn: &DbConn,
     ) -> Option<Self> {
+        let oldest = Utc::now().naive_utc() - Self::admin_request_expiration();
+
         conn.run(move |conn| {
             auth_requests::table
                 .filter(auth_requests::user_uuid.eq(user_uuid))
@@ -265,6 +275,7 @@ impl AuthRequest {
                 .filter(auth_requests::organization_uuid.eq(org_uuid))
                 .filter(auth_requests::atype.eq(AuthRequestType::AdminApproval as i32))
                 .filter(auth_requests::approved.is_null())
+                .filter(auth_requests::creation_date.gt(oldest))
                 .order_by(auth_requests::creation_date.desc())
                 .first::<Self>(conn)
                 .ok()
@@ -421,6 +432,24 @@ mod tests {
         assert!(request(AuthRequestType::AdminApproval, TimeDelta::try_days(8).unwrap()).is_expired());
     }
 
+    #[test]
+    fn a_request_nobody_answered_in_time_is_not_still_pending() {
+        // `find_pending_admin_approval` decides whether asking again reuses the open request or
+        // starts a new one, and filters on the same window as this. A request past it must not come
+        // back: reviving it by moving its date forward would leave the user waiting on something
+        // the administrators were never told about, because only a new request mails them.
+        let mut auth_request =
+            request(AuthRequestType::AdminApproval, AuthRequest::admin_request_expiration() + TimeDelta::seconds(1));
+        assert_eq!(auth_request.approved, None, "still unanswered");
+        assert!(auth_request.is_expired());
+
+        // One minute short of the window is still the same request, and asking again updates it
+        // rather than mailing everyone a second time.
+        auth_request.creation_date =
+            Utc::now().naive_utc() - AuthRequest::admin_request_expiration() + TimeDelta::minutes(1);
+        assert!(!auth_request.is_expired());
+    }
+
     #[test]
     fn the_answer_of_an_administrator_starts_its_own_clock() {
         // Answered right at the end of the week, so the request itself is long past its window.

src/db/models/device.rs

@@ -116,19 +116,6 @@ impl Device {
         Self::present(self.encrypted_private_key.as_ref()).is_some()
     }
 
-    /// Whether any part of a trust is stored, complete or not.
-    pub fn holds_any_key(&self) -> bool {
-        Self::present(self.encrypted_user_key.as_ref()).is_some()
-            || Self::present(self.encrypted_public_key.as_ref()).is_some()
-            || self.holds_private_key()
-    }
-
-    pub fn untrust(&mut self) {
-        self.encrypted_user_key = None;
-        self.encrypted_public_key = None;
-        self.encrypted_private_key = None;
-    }
-
     pub fn to_json(&self) -> Value {
         json!({
             "id": self.uuid,
@@ -264,12 +251,13 @@ impl Device {
 
     /// Invalidates every copy of the user key that is wrapped for one of the user's devices.
     ///
-    /// Called when the user key itself is replaced, which leaves all of those copies pointing at a
-    /// key that no longer unlocks anything. No device counts as trusted afterwards, so a client
-    /// that stops here ends up with an extra login rather than a broken unlock. The device key
-    /// pairs are deliberately left alone: they are wrapped with the device key, which a rotation
-    /// does not touch, so `POST /devices/update-trust` can hand every device the new user key and
-    /// restore its trust. Whatever it does not list is dropped there.
+    /// Called when the user key itself is replaced and the client did not say what to put in their
+    /// place, which leaves all of those copies pointing at a key that no longer unlocks anything. No
+    /// device counts as trusted afterwards, so a client that stops here ends up with an extra login
+    /// rather than a broken unlock. The device key pairs are deliberately left alone: they are
+    /// wrapped with the device key, which a rotation does not touch, so `POST /devices/update-trust`
+    /// can hand every device the new user key and restore its trust. Whatever it does not list is
+    /// dropped there.
     ///
     /// One statement, so there is no half applied state to reason about.
     pub async fn invalidate_wrapped_user_keys(user_uuid: &UserId, conn: &DbConn) -> EmptyResult {
@@ -285,6 +273,82 @@ impl Device {
         .await
     }
 
+    /// Drops every stored key of the named devices, in one statement so it cannot half apply.
+    ///
+    /// The caller has already checked that each id belongs to this user.
+    pub async fn untrust_many(user_uuid: &UserId, device_ids: Vec<DeviceId>, conn: &DbConn) -> EmptyResult {
+        if device_ids.is_empty() {
+            return Ok(());
+        }
+
+        conn.run(move |conn| {
+            diesel::update(
+                devices::table.filter(devices::user_uuid.eq(user_uuid)).filter(devices::uuid.eq_any(device_ids)),
+            )
+            .set((
+                devices::encrypted_user_key.eq::<Option<String>>(None),
+                devices::encrypted_public_key.eq::<Option<String>>(None),
+                devices::encrypted_private_key.eq::<Option<String>>(None),
+            ))
+            .execute(conn)
+            .map_res("Error untrusting the devices")
+        })
+        .await
+    }
+
+    /// Replaces the trust of every device of the user in one go: the listed ones are re-wrapped for
+    /// the current user key, everything else loses whatever it still holds.
+    ///
+    /// This is what both a key rotation and `POST /devices/update-trust` come down to. The caller
+    /// has already checked that every id belongs to this user, that none is listed twice, and that
+    /// no device is asked to keep a trust it cannot complete; this only writes.
+    ///
+    /// One transaction, so the devices cannot be left split between the old and the new user key,
+    /// which is a state no client can tell apart from a working one until an unlock fails.
+    pub async fn replace_trust(
+        user_uuid: &UserId,
+        updates: Vec<(DeviceId, String, String)>,
+        conn: &DbConn,
+    ) -> EmptyResult {
+        conn.run(move |conn| {
+            conn.transaction(|conn| -> EmptyResult {
+                let keep: Vec<DeviceId> = updates.iter().map(|(device_id, ..)| device_id.clone()).collect();
+
+                // Whatever the untouched devices hold wraps the previous user key, or is one half of
+                // a trust that was never finished. Either way it unlocks nothing and must not stay.
+                let cleared = (
+                    devices::encrypted_user_key.eq::<Option<String>>(None),
+                    devices::encrypted_public_key.eq::<Option<String>>(None),
+                    devices::encrypted_private_key.eq::<Option<String>>(None),
+                );
+                let outdated = devices::table.filter(devices::user_uuid.eq(&user_uuid));
+                let _: () = if keep.is_empty() {
+                    diesel::update(outdated).set(cleared).execute(conn)
+                } else {
+                    diesel::update(outdated.filter(devices::uuid.ne_all(keep))).set(cleared).execute(conn)
+                }
+                .map_res("Error untrusting the devices left out of the rotation")?;
+
+                // The device key pair is deliberately not touched here: it is wrapped with the
+                // device key, which the server never sees and a rotation never changes.
+                for (device_id, encrypted_user_key, encrypted_public_key) in updates {
+                    let _: () = diesel::update(
+                        devices::table.filter(devices::uuid.eq(device_id)).filter(devices::user_uuid.eq(&user_uuid)),
+                    )
+                    .set((
+                        devices::encrypted_user_key.eq(Some(encrypted_user_key)),
+                        devices::encrypted_public_key.eq(Some(encrypted_public_key)),
+                    ))
+                    .execute(conn)
+                    .map_res("Error rotating the wrapped user key of a device")?;
+                }
+
+                Ok(())
+            })
+        })
+        .await
+    }
+
     pub async fn find_by_uuid_and_user(uuid: &DeviceId, user_uuid: &UserId, conn: &DbConn) -> Option<Self> {
         conn.run(move |conn| {
             devices::table
@@ -550,32 +614,18 @@ mod tests {
 
         assert!(!device.is_trusted(), "nothing may unlock until the client re-wraps");
         assert!(device.holds_private_key(), "but the device can still be handed a new user key");
-        assert!(device.holds_any_key());
     }
 
     #[test]
     fn a_device_that_never_had_a_trust_holds_nothing() {
         let device = Device::new(String::from("device").into(), String::from("user").into(), String::new(), 9);
         assert!(!device.holds_private_key());
-        assert!(!device.holds_any_key());
 
         let mut device = trusted_device();
         device.encrypted_private_key = Some(String::new());
         assert!(!device.holds_private_key(), "an empty key is as good as a missing one");
     }
 
-    #[test]
-    fn untrusting_clears_every_key() {
-        let mut device = trusted_device();
-        device.untrust();
-
-        assert!(!device.is_trusted());
-        assert!(!device.holds_any_key());
-        assert_eq!(device.encrypted_user_key, None);
-        assert_eq!(device.encrypted_public_key, None);
-        assert_eq!(device.encrypted_private_key, None);
-    }
-
     #[test]
     fn only_interactive_clients_can_approve_a_login_request() {
         for atype in 0..=26 {

src/db/models/organization.rs

@@ -277,6 +277,20 @@ impl Membership {
         }
     }
 
+    /// Whether this membership may act on the account recovery of the organization's members:
+    /// reset their master password, and answer the device approvals they ask their organization for.
+    ///
+    /// Upstream is a permission of its own, `ManageResetPassword`, which an administrator has by
+    /// virtue of the role and a custom role can be granted separately. Vaultwarden folds the custom
+    /// role into `Manager` and drops the permissions that came with it, so only the administrators
+    /// are left holding it. Asking here rather than comparing roles at each call site keeps that one
+    /// decision in one place for when custom roles arrive.
+    /// https://github.com/bitwarden/server/blob/main/src/Core/Context/CurrentContext.cs
+    pub fn has_manage_reset_password_permission(&self) -> bool {
+        self.status == MembershipStatus::Confirmed as i32
+            && MembershipType::from_i32(self.atype).is_some_and(|atype| atype >= MembershipType::Admin)
+    }
+
     pub fn restore(&mut self) -> bool {
         if self.status < MembershipStatus::Invited as i32 {
             self.status += ACTIVATE_REVOKE_DIFF;
@@ -1285,4 +1299,32 @@ mod tests {
         assert!(MembershipType::Manager > MembershipType::User);
         assert!(MembershipType::Manager == MembershipType::from_str("4").unwrap());
     }
+
+    #[test]
+    fn only_a_confirmed_administrator_manages_account_recovery() {
+        let mut membership = Membership::new(String::from("user").into(), String::from("org").into(), None);
+
+        for (atype, expected) in [
+            (MembershipType::Owner, true),
+            (MembershipType::Admin, true),
+            // The custom role is folded into the manager one, losing whatever permissions came
+            // with it, so it cannot be assumed to hold this one.
+            (MembershipType::Manager, false),
+            (MembershipType::User, false),
+        ] {
+            membership.atype = atype as i32;
+
+            for status in [MembershipStatus::Revoked, MembershipStatus::Invited, MembershipStatus::Accepted] {
+                let status = status as i32;
+                membership.status = status;
+                assert!(
+                    !membership.has_manage_reset_password_permission(),
+                    "a membership that is not confirmed manages nothing, status {status}"
+                );
+            }
+
+            membership.status = MembershipStatus::Confirmed as i32;
+            assert_eq!(membership.has_manage_reset_password_permission(), expected, "type {}", atype as i32);
+        }
+    }
 }

src/util.rs

@@ -543,40 +543,81 @@ pub fn is_valid_email(email: &str) -> bool {
     true
 }
 
-/// The most an `EncString` we are willing to store may weigh. The largest legitimate one is an
-/// RSA-4096 envelope with a MAC, which stays an order of magnitude below this.
+/// The most an `EncString` we are willing to store may weigh.
+///
+/// Upstream puts no length on the fields this guards; this is ours, so a client cannot park
+/// megabytes in a column that is supposed to hold a wrapped key. The largest thing that legitimately
+/// lands there is a device's RSA-2048 private key wrapped with AES-CBC plus a MAC, around 1.7 kB, so
+/// this leaves room to spare.
 const MAX_ENC_STRING_LENGTH: usize = 4096;
 
+/// The number of `|` separated parts an `EncString` of the given `EncryptionType` is made of.
+///
+///  - `3` Rsa2048_OaepSha256_B64 and `4` Rsa2048_OaepSha1_B64 are the ciphertext by itself, and
+///    `7` XChaCha20Poly1305_B64 is one blob of COSE bytes;
+///  - `0` AesCbc256_B64 is `iv|ct`, while `5` Rsa2048_OaepSha256_HmacSha256_B64 and
+///    `6` Rsa2048_OaepSha1_HmacSha256_B64 are `rsaCt|mac`;
+///  - `1` AesCbc128_HmacSha256_B64 and `2` AesCbc256_HmacSha256_B64 are `iv|ct|mac`.
+///
+/// https://github.com/bitwarden/server/blob/main/src/Core/Enums/EncryptionType.cs
+fn enc_string_parts(enc_type: u8) -> Option<usize> {
+    match enc_type {
+        3 | 4 | 7 => Some(1),
+        0 | 5 | 6 => Some(2),
+        1 | 2 => Some(3),
+        _ => None,
+    }
+}
+
+/// Whether a single part is base64, as permissively as upstream reads it.
+///
+/// Upstream accepts a final character whose unused padding bits are not zero, because such values
+/// exist in the wild; a strict decoder rejects them. Everything else is the usual shape: a multiple
+/// of four characters from the base64 alphabet, with at most two `=` closing it off.
+/// https://github.com/bitwarden/server/blob/main/src/Core/Utilities/EncryptedStringAttribute.cs
+fn is_valid_base64_permissive(value: &str) -> bool {
+    if value.is_empty() || !value.len().is_multiple_of(4) {
+        return false;
+    }
+
+    // A group of four holds at least two characters of data, so at most two may be padded away.
+    let data = value.strip_suffix("==").or_else(|| value.strip_suffix('=')).unwrap_or(value);
+
+    !data.is_empty() && data.bytes().all(|b| b.is_ascii_alphanumeric() || b == b'+' || b == b'/')
+}
+
 /// Whether a value has the shape of a Bitwarden `EncString`: `<type>.<part>|<part>...`.
 ///
 /// The server cannot tell whether a blob decrypts, but it can refuse everything that is not even
 /// of the right form, which keeps unbounded junk out of the columns that hold key material.
-/// Mirrors `EncryptedStringAttribute` upstream.
+/// Mirrors `EncryptedStringAttribute` upstream, including its header-less legacy form, but not its
+/// acceptance of a type spelled out by name (`AesCbc256_B64.…`), which no client has ever written.
 /// https://github.com/bitwarden/server/blob/main/src/Core/Utilities/EncryptedStringAttribute.cs
 pub fn is_valid_enc_string(value: &str) -> bool {
     if value.is_empty() || value.len() > MAX_ENC_STRING_LENGTH {
         return false;
     }
 
-    let Some((enc_type, data)) = value.split_once('.') else {
-        return false;
-    };
-
-    // The number of `|` separated parts each type is made of.
-    let parts = match enc_type {
-        // An RSA envelope is the ciphertext by itself.
-        "3" | "4" => 1,
-        // An AES value carries its IV, an RSA one from type 5 on carries a MAC.
-        "0" | "5" | "6" => 2,
-        // And an AES value from type 1 on carries both.
-        "1" | "2" => 3,
-        _ => return false,
+    let (parts, data) = if let Some((enc_type, data)) = value.split_once('.') {
+        let Some(parts) = enc_type.parse::<u8>().ok().and_then(enc_string_parts) else {
+            return false;
+        };
+        (parts, data)
+    } else {
+        // Without a header the type is guessed from the number of parts, the same two candidates
+        // upstream picks between: three means it carries a MAC, anything else is read as iv|ct.
+        let parts = if value.matches('|').count() == 2 {
+            3
+        } else {
+            2
+        };
+        (parts, value)
     };
 
     let mut seen = 0;
     for part in data.split('|') {
         seen += 1;
-        if seen > parts || part.is_empty() || data_encoding::BASE64.decode(part.as_bytes()).is_err() {
+        if seen > parts || !is_valid_base64_permissive(part) {
             return false;
         }
     }
@@ -591,18 +632,33 @@ mod enc_string_tests {
     #[test]
     fn a_well_formed_enc_string_of_every_type_is_accepted() {
         for value in [
-            "0.aXY=|Y2lwaGVy",
-            "1.aXY=|Y2lwaGVy|bWFj",
-            "2.aXY=|Y2lwaGVy|bWFj",
-            "3.Y2lwaGVy",
-            "4.Y2lwaGVy",
-            "5.Y2lwaGVy|bWFj",
-            "6.Y2lwaGVy|bWFj",
+            "0.aXY=|Y2lwaGVy",        // AesCbc256_B64
+            "1.aXY=|Y2lwaGVy|bWFj",   // AesCbc128_HmacSha256_B64
+            "2.aXY=|Y2lwaGVy|bWFj",   // AesCbc256_HmacSha256_B64
+            "3.Y2lwaGVy",             // Rsa2048_OaepSha256_B64
+            "4.Y2lwaGVy",             // Rsa2048_OaepSha1_B64
+            "5.Y2lwaGVy|bWFj",        // Rsa2048_OaepSha256_HmacSha256_B64
+            "6.Y2lwaGVy|bWFj",        // Rsa2048_OaepSha1_HmacSha256_B64
+            "7.Y29zZWJ5dGVz",         // XChaCha20Poly1305_B64, one blob of COSE bytes
+            "07.Y29zZWJ5dGVz",        // the header is read as a number, not matched as text
+            "aXY=|Y2lwaGVy",          // header-less legacy form, read as iv|ct
+            "aXY=|Y2lwaGVy|bWFj",     // and as iv|ct|mac when it has three parts
+            "3.Y2lwaGVyLysvdGV4dA==", // the whole base64 alphabet, padded
+            "3.Y2lwaGVyLysvdGV4dGE=", // and with a single pad character
         ] {
             assert!(is_valid_enc_string(value), "{value}");
         }
     }
 
+    #[test]
+    fn a_non_canonical_final_character_is_accepted() {
+        // The unused padding bits of the last character are not zero. A strict decoder refuses
+        // these, upstream deliberately does not, and such values exist in the wild.
+        assert!(is_valid_enc_string("3.QR=="));
+        assert!(is_valid_enc_string("3.QUJDRR=="));
+        assert!(is_valid_enc_string("2.aXY=|QR==|bWFj"));
+    }
+
     #[test]
     fn anything_that_is_not_one_is_refused() {
         for value in [
@@ -612,13 +668,24 @@ mod enc_string_tests {
             "2",
             "2.",
             ".aXY=|Y2lwaGVy|bWFj",
-            "7.Y2lwaGVy",             // no such type
+            "8.Y2lwaGVy",             // no such type
+            "255.Y2lwaGVy",           // nor at the top of the byte the header is read as
+            "256.Y2lwaGVy",           // nor past it
             "-1.Y2lwaGVy",            // and none below zero either
             "2.aXY=|Y2lwaGVy",        // type 2 without its mac
             "2.aXY=|Y2lwaGVy|bWFj|x", // or with one part too many
             "4.Y2lwaGVy|bWFj",        // type 4 carries no mac
+            "7.Y29zZQ==|bWFj",        // and neither does type 7
             "2.aXY=||bWFj",           // an empty part is not base64
             "4.not base64!",
+            "4.Y2lwaGV",                   // a length that is not a multiple of four
+            "4.Y2lwaGVy=",                 // a stray pad character breaks that length
+            "4.====",                      // padding only
+            "4.Y2lw=GVy",                  // padding in the middle
+            "4.Y2lw-GVy",                  // url-safe base64 is a different alphabet
+            "4.Y2lw GVy",                  // whitespace is not part of it either
+            "aXY=",                        // header-less, but only one part
+            "aXY=|Y2lwaGVy|bWFj|Zm91cg==", // header-less with four
         ] {
             assert!(!is_valid_enc_string(value), "{value}");
         }
@@ -629,6 +696,12 @@ mod enc_string_tests {
         let payload = "A".repeat(4096);
         assert!(is_valid_enc_string(&format!("4.{}", &payload[..4000])));
         assert!(!is_valid_enc_string(&format!("4.{payload}")), "must not grow without bound");
+
+        // A wrapped RSA-2048 device private key, the largest thing that legitimately arrives here,
+        // has to fit with room to spare.
+        let private_key = format!("2.{}|{}|{}", "A".repeat(24), "B".repeat(1652), "C".repeat(44));
+        assert!(private_key.len() < 2048);
+        assert!(is_valid_enc_string(&private_key));
     }
 }