Browse Source

Address further trusted device review findings

pull/7534/head
tom27052006 6 days ago
parent
commit
b953744be7
  1. 192
      src/api/core/accounts.rs
  2. 15
      src/api/core/organizations.rs
  3. 89
      src/api/identity.rs
  4. 4
      src/auth.rs
  5. 2
      src/db/models/event.rs
  6. 207
      src/db/models/organization.rs

192
src/api/core/accounts.rs

@ -1,4 +1,4 @@
use std::collections::HashSet; use std::collections::{HashMap, HashSet};
use chrono::Utc; use chrono::Utc;
use rocket::{ use rocket::{
@ -22,8 +22,8 @@ use crate::{
models::{ models::{
AuthRequest, AuthRequestId, AuthRequestType, Cipher, CipherId, Device, DeviceId, DeviceType, AuthRequest, AuthRequestId, AuthRequestType, Cipher, CipherId, Device, DeviceId, DeviceType,
DeviceWithAuthRequest, EmergencyAccess, EmergencyAccessId, EventType, Folder, FolderId, Invitation, DeviceWithAuthRequest, EmergencyAccess, EmergencyAccessId, EventType, Folder, FolderId, Invitation,
Membership, MembershipId, MembershipStatus, OrgPolicy, OrgPolicyType, Organization, OrganizationId, Send, Membership, MembershipId, OrgPolicy, OrgPolicyType, Organization, OrganizationId, Send, SendId, User,
SendId, User, UserId, UserKdfType, UserId, UserKdfType,
}, },
}, },
mail, mail,
@ -45,6 +45,7 @@ pub fn routes() -> Vec<rocket::Route> {
post_keys, post_keys,
post_password, post_password,
post_set_password, post_set_password,
put_update_tde_offboarding_password,
post_kdf, post_kdf,
post_rotatekey, post_rotatekey,
post_sstamp, post_sstamp,
@ -522,6 +523,88 @@ async fn post_set_password(data: Json<SetPasswordData>, 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<String>,
}
/// 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 = "<data>")]
async fn put_update_tde_offboarding_password(
data: Json<UpdateTdeOffboardingPasswordData>,
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")] #[get("/accounts/profile")]
async fn profile(headers: Headers, conn: DbConn) -> Json<Value> { async fn profile(headers: Headers, conn: DbConn) -> Json<Value> {
Json(headers.user.to_json(&conn).await) Json(headers.user.to_json(&conn).await)
@ -873,36 +956,38 @@ fn validate_device_keydata(
updates: &[UpdateDeviceKeysData], updates: &[UpdateDeviceKeysData],
existing_devices: &[Device], existing_devices: &[Device],
) -> ApiResult<Vec<(DeviceId, String, String)>> { ) -> ApiResult<Vec<(DeviceId, String, String)>> {
let mut listed: HashSet<&DeviceId> = HashSet::with_capacity(updates.len()); // Everything the client sent is checked before any of it is used, so a request that is
let mut rotated = Vec::with_capacity(updates.len()); // 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 { 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") 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)) err!(format!("Device {} does not belong to this user", update.device_id))
}; }
validate_enc_strings(&[ validate_enc_strings(&[
("encryptedUserKey", &update.encrypted_user_key), ("encryptedUserKey", &update.encrypted_user_key),
("encryptedPublicKey", &update.encrypted_public_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)) { // Walked over the devices that are trusted right now rather than over what was sent, because a
err!("All existing trusted devices must be included in the rotation") // rotation may only carry an existing trust over to the new user key. Trusting a device is a
// step of its own, `PUT /devices/<id>/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) Ok(rotated)
@ -2031,13 +2116,14 @@ async fn post_admin_auth_request(data: Json<AuthRequestRequest>, headers: Header
data.validate()?; data.validate()?;
// Only an organization the user really belongs to can answer for them. A pending invitation is // Only an organization that could actually answer is asked. Approving means handing the member
// not a membership yet, and a revoked one is not one anymore; sending either of them the email // their own user key, which an administrator can only do with the key that enrolling into
// address, the address and the device of the asker is more than they are owed. // 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> = Membership::find_by_user(&headers.user.uuid, &conn) let memberships: Vec<Membership> = Membership::find_by_user(&headers.user.uuid, &conn)
.await .await
.into_iter() .into_iter()
.filter(|membership| membership.status == MembershipStatus::Confirmed as i32) .filter(Membership::can_use_admin_approval)
.collect(); .collect();
if memberships.is_empty() { if memberships.is_empty() {
err!("User does not belong to any organization that could approve a device") 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<AuthRequestRequest>, headers: Header
let mut first_request = None; let mut first_request = None;
for membership in memberships { for membership in memberships {
// Asking again from the same device replaces the open request instead of adding one, so a // Repeating the very same request is answered with the row it already has, so a client that
// client that retries does not pile up rows and does not mail the administrators twice. // 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( let existing = AuthRequest::find_pending_admin_approval(
&headers.user.uuid, &headers.user.uuid,
&data.device_identifier, &data.device_identifier,
&membership.org_uuid, &membership.org_uuid,
&conn, &conn,
) )
.await; .await
.filter(|request| request.public_key == data.public_key && request.access_code == data.access_code);
let is_new = existing.is_none(); let is_new = existing.is_none();
let mut auth_request = match existing { let mut auth_request = match existing {
Some(mut auth_request) => { Some(mut auth_request) => {
auth_request.access_code.clone_from(&data.access_code); // Only what says where the request is being made from, never the keys it is made
auth_request.public_key.clone_from(&data.public_key); // with; those are what the id stands for.
auth_request.device_type = headers.device.atype; auth_request.device_type = headers.device.atype;
auth_request.request_ip = headers.ip.ip.to_string(); auth_request.request_ip = headers.ip.ip.to_string();
auth_request.creation_date = Utc::now().naive_utc(); 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) let approvers = Membership::find_confirmed_by_org(org_id, conn)
.await .await
.into_iter() .into_iter()
.filter(Membership::has_manage_reset_password_permission); .filter(Membership::can_manage_reset_password_now);
for approver in approvers { for approver in approvers {
let Some(admin) = User::find_by_uuid(&approver.user_uuid, conn).await else { let Some(admin) = User::find_by_uuid(&approver.user_uuid, conn).await else {
@ -2419,11 +2514,42 @@ mod tests {
#[test] #[test]
fn a_partially_trusted_device_does_not_have_to_be_listed() { 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. // It cannot unlock anything as it stands, so leaving it out is not the loss of a trust.
let mut half = device("b", true); let devices = [device("a", true), half_trusted("b")];
half.encrypted_user_key = None;
let devices = [device("a", true), half];
let result = validate_device_keydata(&[update("a")], &devices).unwrap(); let result = validate_device_keydata(&[update("a")], &devices).unwrap();
assert_eq!(rotated(&result), ["a=4.bmV3dXNlcmtleQ=="]); 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/<id>/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");
}
} }

15
src/api/core/organizations.rs

@ -3232,16 +3232,16 @@ async fn get_organization_auth_requests(
continue; continue;
} }
// A request whose asker is not a confirmed member of this organization is none of its // A request this organization could not answer anyway is none of its business, so it is
// business, so it is quietly left out instead of being offered for approval. Same condition // quietly left out instead of being offered for approval. Same condition as when answering,
// as when answering, so nothing is shown here that would be refused there. // so nothing is shown here that would be refused there.
let (Some(member), Some(user)) = ( let (Some(member), Some(user)) = (
Membership::find_by_user_and_org(&auth_request.user_uuid, &org_id, &conn).await, Membership::find_by_user_and_org(&auth_request.user_uuid, &org_id, &conn).await,
User::find_by_uuid(&auth_request.user_uuid, &conn).await, User::find_by_uuid(&auth_request.user_uuid, &conn).await,
) else { ) else {
continue; continue;
}; };
if member.status != MembershipStatus::Confirmed as i32 { if !member.can_use_admin_approval() {
continue; continue;
} }
@ -3423,10 +3423,11 @@ async fn answer_organization_auth_request(
unanswerable!("AuthRequest doesn't exist", "Request has expired"); 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 // Answering means acting for a member of this organization with the key their enrollment into
// that was never accepted is not a membership yet, and a revoked one is not one anymore. // 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 { 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"), _ => unanswerable!("AuthRequest doesn't exist", "The requesting user is not a member of this organization"),
}; };

89
src/api/identity.rs

@ -31,8 +31,8 @@ use crate::{
DbConn, DbConn,
models::{ models::{
AuthRequest, AuthRequestId, Device, DeviceId, DeviceType, EventType, Invitation, Membership, AuthRequest, AuthRequestId, Device, DeviceId, DeviceType, EventType, Invitation, Membership,
MembershipStatus, OIDCCodeResponseError, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, OIDCCodeResponseError, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, OrganizationId, SendId,
OrganizationId, SendId, SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId, SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId,
}, },
}, },
error::MapResult, error::MapResult,
@ -507,6 +507,15 @@ async fn account_creation_can_succeed(user: &User, conn: &DbConn) -> bool {
return false; 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. // What `check_reset_password_applicable` demands of that organization.
if !CONFIG.mail_enabled() { if !CONFIG.mail_enabled() {
return false; 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; 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, // 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 // which is what enrolling into account recovery does. The same condition the request itself is
// same condition the request itself is created and answered under, so this does not announce a // created and answered under, so this does not announce a way out that would be refused the
// way out that would be refused the moment it is taken. // moment it is taken.
ways_in.has_admin_approval = memberships.iter().any(|member| { ways_in.has_admin_approval = memberships.iter().any(Membership::can_use_admin_approval);
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. // 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) { 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 // 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 // a `ManageResetPassword` permission here, which in Vaultwarden's role model only the
// administrators of an organization have. // 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!({ Some(json!({
"HasAdminApproval": ways_in.has_admin_approval, "HasAdminApproval": ways_in.has_admin_approval,
@ -1495,6 +1507,7 @@ async fn authorize(data: AuthorizeData, cookies: &CookieJar<'_>, secure: Secure,
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use super::*; use super::*;
use crate::db::models::MembershipStatus;
/// A `TrustedDeviceWaysIn` plus the server setting, so the cases below read as what they are. /// 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")] #[expect(clippy::struct_excessive_bools, reason = "Mirrors the struct under test")]
@ -1624,4 +1637,60 @@ mod tests {
}; };
assert_eq!(account.offer(), None); 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));
}
} }

4
src/auth.rs

@ -847,7 +847,7 @@ impl<'r> FromRequest<'r> for AdminHeaders {
/// its device approvals comes down to. /// its device approvals comes down to.
/// ///
/// Upstream guards those endpoints on a permission, `ManageResetPassword`, rather than on a role, /// 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 /// 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 /// 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. /// 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<Self, Self::Error> { async fn from_request(request: &'r Request<'_>) -> Outcome<Self, Self::Error> {
let headers = try_outcome!(OrgHeaders::from_request(request).await); 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 { Outcome::Success(Self {
device: headers.device, device: headers.device,
user: headers.user, user: headers.user,

2
src/db/models/event.rs

@ -58,7 +58,7 @@ pub enum EventType {
// UserUpdatedTempPassword = 1008, // Not supported // UserUpdatedTempPassword = 1008, // Not supported
// UserMigratedKeyToKeyConnector = 1009, // Not supported // UserMigratedKeyToKeyConnector = 1009, // Not supported
UserRequestedDeviceApproval = 1010, UserRequestedDeviceApproval = 1010,
// UserTdeOffboardingPasswordSet = 1011, // Not supported UserTdeOffboardingPasswordSet = 1011,
// Cipher // Cipher
CipherCreated = 1100, CipherCreated = 1100,

207
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: /// Whether this membership is in one of the active states rather than a revoked one.
/// reset their master password, and answer the device approvals they ask their organization for. ///
/// 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 /// 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 /// 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 /// 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. /// decision in one place for when custom roles arrive.
/// https://github.com/bitwarden/server/blob/main/src/Core/Context/CurrentContext.cs /// https://github.com/bitwarden/server/blob/main/src/Core/Context/CurrentContext.cs
pub fn has_manage_reset_password_permission(&self) -> bool { fn has_manage_reset_password_role(&self) -> bool {
self.status == MembershipStatus::Confirmed as i32 MembershipType::from_i32(self.atype).is_some_and(|atype| atype >= MembershipType::Admin)
&& 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 { pub fn restore(&mut self) -> bool {
@ -1318,13 +1375,149 @@ mod tests {
let status = status as i32; let status = status as i32;
membership.status = status; membership.status = status;
assert!( assert!(
!membership.has_manage_reset_password_permission(), !membership.can_manage_reset_password_now(),
"a membership that is not confirmed manages nothing, status {status}" "a membership that is not confirmed manages nothing, status {status}"
); );
} }
membership.status = MembershipStatus::Confirmed as i32; 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);
} }
} }
} }

Loading…
Cancel
Save