diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index fb3abf08..2f4b6483 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -1,4 +1,4 @@ -use std::collections::HashSet; +use std::collections::{HashMap, 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, OrgPolicy, OrgPolicyType, Organization, OrganizationId, Send, - SendId, User, UserId, UserKdfType, + Membership, MembershipId, OrgPolicy, OrgPolicyType, Organization, OrganizationId, Send, SendId, User, + UserId, UserKdfType, }, }, mail, @@ -45,6 +45,7 @@ pub fn routes() -> Vec { post_keys, post_password, post_set_password, + put_update_tde_offboarding_password, post_kdf, post_rotatekey, post_sstamp, @@ -522,6 +523,88 @@ async fn post_set_password(data: Json, headers: Headers, conn: }))) } +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct UpdateTdeOffboardingPasswordData { + new_master_password_hash: String, + /// The user key the account already has, re-wrapped for the master key derived from the new + /// password. The vault is not re-encrypted, so this is the only thing that changes about it. + key: String, + master_password_hint: Option, +} + +/// Gives an account that unlocks with a trusted device the master password it needs once the server +/// stops offering trusted devices. +/// +/// This is the endpoint the clients take when a login answered `IsTdeOffboarding`, see +/// `trusted_device_option`. It is deliberately not `/accounts/set-password`: the account is fully +/// set up by this point, so the only thing being added is a second way to unlock the user key it +/// already has. The account key pair and the vault are left exactly as they are, and unlike +/// `/accounts/keys` there is nothing here that could replace them. +/// +/// Upstream keys this on the organization having switched its SSO member decryption away from +/// trusted devices; Vaultwarden configures SSO for the whole server, so the same state is +/// `SSO_ENABLED` without `SSO_TRUSTED_DEVICE_ENCRYPTION`, which is exactly when a login starts +/// answering `IsTdeOffboarding`. +/// https://github.com/bitwarden/server/blob/main/src/Core/Auth/UserFeatures/TdeOffboardingPassword/TdeOffboardingPasswordCommand.cs +#[put("/accounts/update-tde-offboarding-password", data = "")] +async fn put_update_tde_offboarding_password( + data: Json, + headers: Headers, + conn: DbConn, + nt: Notify<'_>, +) -> EmptyResult { + let data = data.into_inner(); + let mut user = headers.user; + + // Adding a master password to an account that has one is changing it, which is + // `/accounts/password` and asks for the current one first. Without this an authenticated caller + // could replace the password of the account they are on, and a second offboarding call would + // overwrite the password the first one just set. + if !user.password_hash.is_empty() { + err!("Account already has a master password") + } + + // The way out of trusted devices only exists while the server still takes SSO logins but no + // longer offers trusted devices. A server that still offers them has nothing to offboard from, + // and one without SSO never had the flow at all. + if !CONFIG.sso_enabled() || CONFIG.sso_trusted_device_encryption() { + err!("Trusted device offboarding is not available on this server") + } + + // A user key that is not an encrypted string unlocks nothing, and this is the only copy the + // master password can reach. Storing it would leave an account that logs in and then cannot + // open its own vault. + if !crate::util::is_valid_enc_string(&data.key) { + err!("key is not a valid encrypted string") + } + + let password_hint = clean_password_hint(data.master_password_hint.as_ref()); + enforce_password_hint_setting(password_hint.as_ref())?; + + // The KDF is left alone: the client derived the master key from the settings the account + // already has, and sends nothing to change them by, as upstream does here. + user.set_password(&data.new_master_password_hash, Some(data.key), true, None, &conn).await?; + user.password_hint = password_hint; + + log_user_event( + EventType::UserTdeOffboardingPasswordSet as i32, + &user.uuid, + headers.device.atype, + &headers.ip.ip, + &conn, + ) + .await; + + user.save(&conn).await?; + + // Upstream logs every session out at this point. The account unlocks a different way from now + // on, so the sessions that were opened against a trusted device do not carry over. + nt.send_logout(&user, None, &conn).await; + + Ok(()) +} + #[get("/accounts/profile")] async fn profile(headers: Headers, conn: DbConn) -> Json { Json(headers.user.to_json(&conn).await) @@ -873,36 +956,38 @@ fn validate_device_keydata( updates: &[UpdateDeviceKeysData], existing_devices: &[Device], ) -> ApiResult> { - let mut listed: HashSet<&DeviceId> = HashSet::with_capacity(updates.len()); - let mut rotated = Vec::with_capacity(updates.len()); + // Everything the client sent is checked before any of it is used, so a request that is + // malformed anywhere is refused as a whole rather than answered in part. + let mut listed: HashMap<&DeviceId, &UpdateDeviceKeysData> = HashMap::with_capacity(updates.len()); for update in updates { - if !listed.insert(&update.device_id) { + if listed.insert(&update.device_id, update).is_some() { 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 { + if !existing_devices.iter().any(|device| device.uuid == update.device_id) { 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") + // Walked over the devices that are trusted right now rather than over what was sent, because a + // rotation may only carry an existing trust over to the new user key. Trusting a device is a + // step of its own, `PUT /devices//keys`, taken by the device itself once it holds the + // device key that these two keys are wrapped for. An entry for anything else is passed over, + // as upstream does; the clients only ever send the devices we reported as trusted. + let mut rotated = Vec::new(); + + for device in existing_devices.iter().filter(|device| device.is_trusted()) { + let Some(update) = listed.get(&device.uuid) else { + err!("All existing trusted devices must be included in the rotation") + }; + + rotated.push((device.uuid.clone(), update.encrypted_user_key.clone(), update.encrypted_public_key.clone())); } Ok(rotated) @@ -2031,13 +2116,14 @@ async fn post_admin_auth_request(data: Json, headers: Header 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. + // Only an organization that could actually answer is asked. Approving means handing the member + // their own user key, which an administrator can only do with the key that enrolling into + // account recovery left them, so an organization without one has nothing to offer and does not + // need the email address, the address and the device of the asker. let memberships: Vec = Membership::find_by_user(&headers.user.uuid, &conn) .await .into_iter() - .filter(|membership| membership.status == MembershipStatus::Confirmed as i32) + .filter(Membership::can_use_admin_approval) .collect(); if memberships.is_empty() { err!("User does not belong to any organization that could approve a device") @@ -2054,21 +2140,30 @@ async fn post_admin_auth_request(data: Json, headers: Header let mut first_request = None; for membership in memberships { - // Asking again from the same device replaces the open request instead of adding one, so a - // client that retries does not pile up rows and does not mail the administrators twice. + // Repeating the very same request is answered with the row it already has, so a client that + // sends it twice does not pile up rows and does not mail the administrators again. + // + // What identifies the request is the key pair the client generated for it: an approval is + // the user key wrapped for that public key, and the fingerprint an administrator reads out + // is derived from it. A client that asks again with a new key pair is therefore asking + // something else, and giving it the id of the pending request would let an administrator + // who is still looking at the old one approve it for a key the requester has thrown away. + // Upstream never reuses a request at all, it creates one per attempt. + // https://github.com/bitwarden/server/blob/main/src/Core/Auth/Services/Implementations/AuthRequestService.cs let existing = AuthRequest::find_pending_admin_approval( &headers.user.uuid, &data.device_identifier, &membership.org_uuid, &conn, ) - .await; + .await + .filter(|request| request.public_key == data.public_key && request.access_code == data.access_code); let is_new = existing.is_none(); let mut auth_request = match existing { Some(mut auth_request) => { - auth_request.access_code.clone_from(&data.access_code); - auth_request.public_key.clone_from(&data.public_key); + // Only what says where the request is being made from, never the keys it is made + // with; those are what the id stands for. auth_request.device_type = headers.device.atype; auth_request.request_ip = headers.ip.ip.to_string(); auth_request.creation_date = Utc::now().naive_utc(); @@ -2117,7 +2212,7 @@ async fn notify_device_approval_requested(user: &User, org_id: &OrganizationId, let approvers = Membership::find_confirmed_by_org(org_id, conn) .await .into_iter() - .filter(Membership::has_manage_reset_password_permission); + .filter(Membership::can_manage_reset_password_now); for approver in approvers { let Some(admin) = User::find_by_uuid(&approver.user_uuid, conn).await else { @@ -2419,11 +2514,42 @@ mod tests { #[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 devices = [device("a", true), half_trusted("b")]; let result = validate_device_keydata(&[update("a")], &devices).unwrap(); assert_eq!(rotated(&result), ["a=4.bmV3dXNlcmtleQ=="]); } + + /// A device left holding nothing but its own key pair, which is what a rotation by a client too + /// old to send `deviceKeyUnlockData` leaves behind. It does not unlock anything as it stands. + fn half_trusted(id: &str) -> Device { + let mut device = device(id, true); + device.encrypted_user_key = None; + device.encrypted_public_key = None; + assert!(!device.is_trusted(), "not trusted"); + assert!(device.holds_private_key(), "but still holds its key pair"); + device + } + + #[test] + fn a_rotation_does_not_trust_a_device_that_was_not_trusted() { + // Trusting a device is `PUT /devices//keys`, taken by the device itself once it holds + // the device key these blobs are wrapped for. A rotation only carries an existing trust + // over to the new user key, so listing an untrusted device here gains it nothing, even + // though its key pair is still around for the trust it could be given later. + let devices = [device("a", true), half_trusted("b")]; + + let result = validate_device_keydata(&[update("a"), update("b")], &devices).unwrap(); + assert_eq!(rotated(&result), ["a=4.bmV3dXNlcmtleQ=="], "`b` is passed over and cleared by the write"); + } + + #[test] + fn a_rotation_cannot_hand_a_user_their_first_trusted_device() { + // The same the other way round: with nothing to carry over, a rotation writes no trust at + // all, however much the request offers. + let devices = [half_trusted("a"), half_trusted("b")]; + + let result = validate_device_keydata(&[update("a"), update("b")], &devices).unwrap(); + assert!(result.is_empty(), "no device was trusted before the rotation, so none is after it"); + } } diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 5d6b759f..e037181e 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -3232,16 +3232,16 @@ async fn get_organization_auth_requests( continue; } - // A request whose asker is not a confirmed member of this organization is none of its - // business, so it is quietly left out instead of being offered for approval. Same condition - // as when answering, so nothing is shown here that would be refused there. + // A request this organization could not answer anyway is none of its business, so it is + // quietly left out instead of being offered for approval. Same condition as when answering, + // so nothing is shown here that would be refused there. let (Some(member), Some(user)) = ( Membership::find_by_user_and_org(&auth_request.user_uuid, &org_id, &conn).await, User::find_by_uuid(&auth_request.user_uuid, &conn).await, ) else { continue; }; - if member.status != MembershipStatus::Confirmed as i32 { + if !member.can_use_admin_approval() { continue; } @@ -3423,10 +3423,11 @@ async fn answer_organization_auth_request( unanswerable!("AuthRequest doesn't exist", "Request has expired"); } - // Answering means acting for a member of this organization, so it has to be one: an invitation - // that was never accepted is not a membership yet, and a revoked one is not one anymore. + // Answering means acting for a member of this organization with the key their enrollment into + // account recovery left behind: an invitation that was never accepted is not a membership yet, + // a revoked one is not one anymore, and without that key there is nothing to answer with. let member = match Membership::find_by_user_and_org(&auth_request.user_uuid, org_id, conn).await { - Some(member) if member.status == MembershipStatus::Confirmed as i32 => member, + Some(member) if member.can_use_admin_approval() => member, _ => unanswerable!("AuthRequest doesn't exist", "The requesting user is not a member of this organization"), }; diff --git a/src/api/identity.rs b/src/api/identity.rs index a56cc65c..84913283 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -31,8 +31,8 @@ use crate::{ DbConn, models::{ AuthRequest, AuthRequestId, Device, DeviceId, DeviceType, EventType, Invitation, Membership, - MembershipStatus, OIDCCodeResponseError, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, - OrganizationId, SendId, SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId, + OIDCCodeResponseError, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, OrganizationId, SendId, + SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId, }, }, error::MapResult, @@ -507,6 +507,15 @@ async fn account_creation_can_succeed(user: &User, conn: &DbConn) -> bool { return false; }; + // That lookup only rules out the `Revoked` status itself, which revoking never actually writes: + // it shifts the status out of the active range instead, so a revoked membership comes back from + // it like any other. The enrolment endpoint runs behind `OrgMemberHeaders` and turns exactly + // those away, so offering the flow on the strength of one would walk the client into the half + // built account this whole function exists to avoid. + if !membership.is_active() { + return false; + } + // What `check_reset_password_applicable` demands of that organization. if !CONFIG.mail_enabled() { return false; @@ -602,13 +611,10 @@ async fn trusted_device_option(user: &User, device: &Device, conn: &DbConn) -> O let memberships = Membership::find_by_user(&user.uuid, conn).await; // 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()) - }); + // which is what enrolling into account recovery does. 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(Membership::can_use_admin_approval); // 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) { @@ -628,7 +634,13 @@ async fn trusted_device_option(user: &User, device: &Device, conn: &DbConn) -> O // 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); + // + // Every active membership counts, not only the confirmed one that may act on the permission + // today: an administrator provisioned into the organization by this very login holds the role + // before anybody has confirmed them, and this is the login that has to tell them to set a + // master password. See `has_manage_reset_password_role_for_tde`. + let has_manage_reset_password_permission = + memberships.iter().any(Membership::has_manage_reset_password_role_for_tde); Some(json!({ "HasAdminApproval": ways_in.has_admin_approval, @@ -1495,6 +1507,7 @@ async fn authorize(data: AuthorizeData, cookies: &CookieJar<'_>, secure: Secure, #[cfg(test)] mod tests { use super::*; + use crate::db::models::MembershipStatus; /// 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")] @@ -1624,4 +1637,60 @@ mod tests { }; assert_eq!(account.offer(), None); } + + /// What `trusted_device_option` reads off the memberships of the user logging in. + fn has_admin_approval(memberships: &[Membership]) -> bool { + memberships.iter().any(Membership::can_use_admin_approval) + } + + fn membership(org: &str, status: MembershipStatus, enrolled: bool) -> Membership { + let mut membership = Membership::new(String::from("user").into(), org.to_owned().into(), None); + membership.status = status as i32; + membership.reset_password_key = enrolled.then(|| String::from("2.aXY=|Y2lwaGVy|bWFj")); + membership + } + + #[test] + fn enrolling_into_trusted_devices_leaves_an_administrator_to_ask() { + // Invited into an organization that unlocks with trusted devices, before enrolling: nobody + // holds a key to approve with yet. + let mut memberships = [membership("org", MembershipStatus::Invited, false)]; + assert!(!has_admin_approval(&memberships)); + + // Enrolling is what `put_reset_password_enrollment` does for an account without a master + // password: it writes the key and accepts the invitation in the same step. Confirming the + // member is an administrator's own, later decision, and until they get round to it the + // member is stuck here. + memberships[0].status = MembershipStatus::Accepted as i32; + memberships[0].reset_password_key = Some(String::from("2.aXY=|Y2lwaGVy|bWFj")); + + assert!(has_admin_approval(&memberships), "the enrolment is what an administrator answers with"); + + // Losing the trusted device at that point is the case this covers: no master password, no + // device that unlocks, and an administrator to ask is the only way back in. + let account = Account { + has_admin_approval: has_admin_approval(&memberships), + ..Account::new() + }; + assert_eq!(account.offer(), Some(false), "the flow leads somewhere, so it is offered"); + } + + #[test] + fn one_organization_that_could_approve_is_enough() { + // A member of several organizations only needs one of them to hold a key for them. + let memberships = [ + membership("invited", MembershipStatus::Invited, true), + membership("not-enrolled", MembershipStatus::Confirmed, false), + membership("enrolled", MembershipStatus::Accepted, true), + ]; + assert!(has_admin_approval(&memberships)); + + // Take that one away and there is nobody left to ask, however many organizations remain. + let memberships = [ + membership("invited", MembershipStatus::Invited, true), + membership("not-enrolled", MembershipStatus::Confirmed, false), + membership("revoked", MembershipStatus::Revoked, true), + ]; + assert!(!has_admin_approval(&memberships)); + } } diff --git a/src/auth.rs b/src/auth.rs index 6431178f..39a030bd 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -847,7 +847,7 @@ impl<'r> FromRequest<'r> for AdminHeaders { /// 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. +/// so this asks `Membership::can_manage_reset_password_now` 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. @@ -865,7 +865,7 @@ impl<'r> FromRequest<'r> for ManageResetPasswordHeaders { async fn from_request(request: &'r Request<'_>) -> Outcome { let headers = try_outcome!(OrgHeaders::from_request(request).await); - if headers.membership.has_manage_reset_password_permission() { + if headers.membership.can_manage_reset_password_now() { Outcome::Success(Self { device: headers.device, user: headers.user, diff --git a/src/db/models/event.rs b/src/db/models/event.rs index 86cbf5d0..58befaf4 100644 --- a/src/db/models/event.rs +++ b/src/db/models/event.rs @@ -58,7 +58,7 @@ pub enum EventType { // UserUpdatedTempPassword = 1008, // Not supported // UserMigratedKeyToKeyConnector = 1009, // Not supported UserRequestedDeviceApproval = 1010, - // UserTdeOffboardingPasswordSet = 1011, // Not supported + UserTdeOffboardingPasswordSet = 1011, // Cipher CipherCreated = 1100, diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index f9628a62..4980362b 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -277,8 +277,18 @@ 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. + /// Whether this membership is in one of the active states rather than a revoked one. + /// + /// Revoking does not write `Revoked`, it shifts the status the membership is to be restored to + /// out of the active range, so a revoked row reads `-128`, `-127` or `-126` and never `-1`. + /// `MembershipStatus::from_i32` only knows the three active values, which is exactly how + /// `OrgHeaders` turns a revoked member away, so it is what decides it here too. Comparing + /// against `Revoked` instead would let every one of those stored values through. + pub fn is_active(&self) -> bool { + MembershipStatus::from_i32(self.status).is_some() + } + + /// The role side of account recovery, without asking what the membership's standing is. /// /// 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 @@ -286,9 +296,56 @@ impl Membership { /// 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) + fn has_manage_reset_password_role(&self) -> bool { + MembershipType::from_i32(self.atype).is_some_and(|atype| atype >= MembershipType::Admin) + } + + /// Whether this membership may act on the account recovery of the organization's members right + /// now: reset their master password, and answer the device approvals they ask their + /// organization for. + /// + /// This is the authorization question, so it asks for a membership that is fully established. + /// An invitation that was never accepted and one that is still waiting to be confirmed are not + /// yet somebody the organization has put in charge of its members' keys. + pub fn can_manage_reset_password_now(&self) -> bool { + self.status == MembershipStatus::Confirmed as i32 && self.has_manage_reset_password_role() + } + + /// Whether a login should tell the client that this member is on the answering side of account + /// recovery, which is what makes it walk a member who has no master password into setting one. + /// + /// A weaker question than `can_manage_reset_password_now`, and deliberately so: it decides what + /// the account is told about itself, not what it may do. Upstream answers it for every active + /// membership, invited and accepted included, because a member who was just provisioned into + /// the organization by their first SSO login holds the role before anyone confirms them, and + /// waiting until then would let an administrator through the trusted device flow without ever + /// being asked for the master password their own role requires of them. + /// + /// A revoked membership is not active and never counts, here or anywhere else. + /// https://github.com/bitwarden/server/blob/main/src/Identity/IdentityServer/UserDecryptionOptionsBuilder.cs + pub fn has_manage_reset_password_role_for_tde(&self) -> bool { + self.is_active() && self.has_manage_reset_password_role() + } + + /// Whether the administrators of this organization can let a new device of this member in. + /// + /// Approving means handing the member their own user key, wrapped for the asking device. The + /// only copy of it the organization has is the one enrolling into account recovery left behind, + /// so without that key there is nothing to approve with, whatever the member's standing is. + /// + /// Enrolling is also what turns an invitation into a membership in the trusted device flow, so + /// the state this has to cover is `Accepted` and not just `Confirmed`: a member who set up + /// trusted devices and then lost the device before an administrator got round to confirming + /// them would otherwise have no way back into their own vault. Upstream asks for the enrollment + /// alone and lets any membership row through; the two ends of the range are kept out here + /// because an invitation is not a membership yet and a revoked one is not one anymore, so + /// neither should have its device let in. + /// https://github.com/bitwarden/server/blob/main/src/Identity/IdentityServer/UserDecryptionOptionsBuilder.cs + pub fn can_use_admin_approval(&self) -> bool { + matches!( + MembershipStatus::from_i32(self.status), + Some(MembershipStatus::Accepted | MembershipStatus::Confirmed) + ) && self.reset_password_key.as_ref().is_some_and(|key| !key.is_empty()) } pub fn restore(&mut self) -> bool { @@ -1318,13 +1375,149 @@ mod tests { let status = status as i32; membership.status = status; assert!( - !membership.has_manage_reset_password_permission(), + !membership.can_manage_reset_password_now(), "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); + assert_eq!(membership.can_manage_reset_password_now(), expected, "type {}", atype as i32); + } + } + + #[test] + fn the_trusted_device_role_signal_covers_a_member_nobody_confirmed_yet() { + let mut membership = Membership::new(String::from("user").into(), String::from("org").into(), None); + + for (atype, holds_role) in [ + (MembershipType::Owner, true), + (MembershipType::Admin, true), + (MembershipType::Manager, false), + (MembershipType::User, false), + ] { + membership.atype = atype as i32; + + // Every active membership answers the same, so an administrator who was provisioned by + // the login that is asking is told to set a master password straight away. + for status in [MembershipStatus::Invited, MembershipStatus::Accepted, MembershipStatus::Confirmed] { + let status = status as i32; + membership.status = status; + assert_eq!( + membership.has_manage_reset_password_role_for_tde(), + holds_role, + "type {}, status {status}", + atype as i32 + ); + } + + // Revoked never counts, whatever the role says. + membership.status = MembershipStatus::Revoked as i32; + assert!(!membership.has_manage_reset_password_role_for_tde(), "revoked, type {}", atype as i32); + + // Nor do the internal statuses a revoked membership is actually stored as, which keep + // the role it is to be restored to. + for was in [MembershipStatus::Invited, MembershipStatus::Accepted, MembershipStatus::Confirmed] { + let was = was as i32; + membership.status = was; + assert!(membership.revoke(), "revoking a {was} membership"); + assert!( + !membership.has_manage_reset_password_role_for_tde(), + "revoked from {was}, stored as {}", + membership.status + ); + } + } + } + + #[test] + fn a_revoked_membership_is_not_active_whatever_it_was_revoked_from() { + let mut membership = Membership::new(String::from("user").into(), String::from("org").into(), None); + + for status in [MembershipStatus::Invited, MembershipStatus::Accepted, MembershipStatus::Confirmed] { + let status = status as i32; + membership.status = status; + assert!(membership.is_active(), "status {status}"); + + // Revoking keeps the status it is to be restored to and shifts it out of the active + // range, so what is stored is never `Revoked` itself. Comparing against that value is + // what would let these through. + assert!(membership.revoke(), "revoking status {status}"); + assert_ne!( + membership.status, + MembershipStatus::Revoked as i32, + "revoked from {status} is not stored as -1" + ); + assert!(!membership.is_active(), "revoked from {status}, stored as {}", membership.status); + + assert!(membership.restore(), "restoring status {status}"); + assert_eq!(membership.status, status, "restored to what it was"); + assert!(membership.is_active()); + } + + // The value the responses show for a revoked membership does not count either. + membership.status = MembershipStatus::Revoked as i32; + assert!(!membership.is_active()); + } + + #[test] + fn only_a_confirmed_membership_may_act_on_account_recovery() { + // The two questions are deliberately not the same one: being told to set a master password + // is not being allowed to reset somebody else's. + let mut membership = Membership::new(String::from("user").into(), String::from("org").into(), None); + membership.atype = MembershipType::Admin as i32; + + for status in [MembershipStatus::Invited, MembershipStatus::Accepted] { + let status = status as i32; + membership.status = status; + assert!(membership.has_manage_reset_password_role_for_tde(), "the login signal covers status {status}"); + assert!(!membership.can_manage_reset_password_now(), "but the endpoints do not, status {status}"); + } + + membership.status = MembershipStatus::Confirmed as i32; + assert!(membership.has_manage_reset_password_role_for_tde()); + assert!(membership.can_manage_reset_password_now()); + } + + #[test] + fn admin_approval_needs_an_accepted_membership_and_an_enrollment() { + let mut membership = Membership::new(String::from("user").into(), String::from("org").into(), None); + + for (status, enrolled, expected, why) in [ + // The invitation was never taken up, so there is no membership to act for yet. + (MembershipStatus::Invited, true, false, "an invitation is not a membership"), + // Where the trusted device enrollment leaves a member until an administrator confirms + // them. Losing the device in that window must not cost them their vault. + (MembershipStatus::Accepted, true, true, "an accepted member enrolled in account recovery"), + (MembershipStatus::Confirmed, true, true, "a confirmed member enrolled in account recovery"), + // Belonging to the organization is not the point, holding the key it would answer with + // is; without an enrollment there is nothing an administrator could hand back. + (MembershipStatus::Accepted, false, false, "accepted, but not enrolled"), + (MembershipStatus::Confirmed, false, false, "confirmed, but not enrolled"), + // Revoking takes the access away but leaves the key behind, which must not keep letting + // new devices in. + (MembershipStatus::Revoked, true, false, "a revoked membership is not one anymore"), + ] { + membership.status = status as i32; + membership.reset_password_key = enrolled.then(|| String::from("2.aXY=|Y2lwaGVy|bWFj")); + + assert_eq!(membership.can_use_admin_approval(), expected, "{why}"); + } + + // Nothing to wrap the user key with, so the same as never having enrolled. + membership.status = MembershipStatus::Confirmed as i32; + membership.reset_password_key = Some(String::new()); + assert!(!membership.can_use_admin_approval(), "an empty key is not an enrollment"); + + // Revoking a member who was enrolled keeps their key, and stores a status that is not + // `Revoked` itself. None of those may keep letting new devices in. + membership.reset_password_key = Some(String::from("2.aXY=|Y2lwaGVy|bWFj")); + for was in [MembershipStatus::Accepted, MembershipStatus::Confirmed] { + let was = was as i32; + membership.status = was; + assert!(membership.can_use_admin_approval(), "enrolled and active, status {was}"); + + assert!(membership.revoke(), "revoking status {was}"); + assert!(!membership.can_use_admin_approval(), "revoked from {was}, stored as {}", membership.status); } } }