Browse Source

Fix client event whitelist and stored collection manage serialization

The client event whitelist no longer drops the newer Bitwarden item-type and
organization events that the previous range check still logged. The admin
console collection lists echo a plain User's stored users_collections.manage
bit again, so saving an unrelated dialog no longer strips it.
pull/7397/head
tom27052006 2 weeks ago
parent
commit
35f54d76fb
  1. 61
      src/api/core/events.rs
  2. 35
      src/db/models/collection.rs
  3. 20
      src/db/models/event.rs
  4. 5
      src/db/models/organization.rs

61
src/api/core/events.rs

@ -247,6 +247,11 @@ fn validate_client_event_batch_size(event_count: usize) -> Result<(), crate::Err
Ok(()) Ok(())
} }
/// The client-generated event types upstream's `/events/collect` accepts. Anything else is ignored,
/// so that an authenticated client cannot write arbitrary event types into an organization's audit
/// log. Keep this in sync with upstream's `CollectController`: a type missing here is silently not
/// logged, which is why the newer item-type events below are listed explicitly rather than matched
/// by range.
fn client_event_kind(event_type: i32) -> Option<ClientEventKind> { fn client_event_kind(event_type: i32) -> Option<ClientEventKind> {
match event_type { match event_type {
event_type if event_type == EventType::UserClientExportedVault as i32 => Some(ClientEventKind::User), event_type if event_type == EventType::UserClientExportedVault as i32 => Some(ClientEventKind::User),
@ -259,11 +264,32 @@ fn client_event_kind(event_type: i32) -> Option<ClientEventKind> {
|| event_type == EventType::CipherClientCopiedHiddenField as i32 || event_type == EventType::CipherClientCopiedHiddenField as i32
|| event_type == EventType::CipherClientCopiedCardCode as i32 || event_type == EventType::CipherClientCopiedCardCode as i32
|| event_type == EventType::CipherClientAutofilled as i32 || event_type == EventType::CipherClientAutofilled as i32
|| event_type == EventType::CipherClientToggledCardNumberVisible as i32 => || event_type == EventType::CipherClientToggledCardNumberVisible as i32
|| event_type == EventType::CipherClientCopiedBankAccountNumber as i32
|| event_type == EventType::CipherClientCopiedBankAccountPin as i32
|| event_type == EventType::CipherClientToggledBankAccountNumberVisible as i32
|| event_type == EventType::CipherClientToggledBankAccountPinVisible as i32
|| event_type == EventType::CipherClientCopiedLicenseNumber as i32
|| event_type == EventType::CipherClientToggledLicenseNumberVisible as i32
|| event_type == EventType::CipherClientCopiedPassportNumber as i32
|| event_type == EventType::CipherClientToggledPassportNumberVisible as i32
|| event_type == EventType::CipherClientCopiedSwiftCode as i32
|| event_type == EventType::CipherClientToggledSwiftCodeVisible as i32
|| event_type == EventType::CipherClientCopiedIban as i32
|| event_type == EventType::CipherClientToggledIbanVisible as i32
|| event_type == EventType::CipherClientCopiedNationalIdentificationNumber as i32
|| event_type == EventType::CipherClientToggledNationalIdentificationNumberVisible as i32 =>
{ {
Some(ClientEventKind::Cipher) Some(ClientEventKind::Cipher)
} }
event_type if event_type == EventType::OrganizationClientExportedVault as i32 => { event_type
if event_type == EventType::OrganizationClientExportedVault as i32
|| event_type == EventType::OrganizationItemOrganizationAccepted as i32
|| event_type == EventType::OrganizationItemOrganizationDeclined as i32
|| event_type == EventType::OrganizationAutoConfirmEnabledAdmin as i32
|| event_type == EventType::OrganizationAutoConfirmDisabledAdmin as i32
|| event_type == EventType::OrganizationInviteLinkClientCopied as i32 =>
{
Some(ClientEventKind::Organization) Some(ClientEventKind::Organization)
} }
_ => None, _ => None,
@ -584,13 +610,36 @@ mod tests {
EventType::CipherClientCopiedCardCode, EventType::CipherClientCopiedCardCode,
EventType::CipherClientAutofilled, EventType::CipherClientAutofilled,
EventType::CipherClientToggledCardNumberVisible, EventType::CipherClientToggledCardNumberVisible,
EventType::CipherClientCopiedBankAccountNumber,
EventType::CipherClientCopiedBankAccountPin,
EventType::CipherClientToggledBankAccountNumberVisible,
EventType::CipherClientToggledBankAccountPinVisible,
EventType::CipherClientCopiedLicenseNumber,
EventType::CipherClientToggledLicenseNumberVisible,
EventType::CipherClientCopiedPassportNumber,
EventType::CipherClientToggledPassportNumberVisible,
EventType::CipherClientCopiedSwiftCode,
EventType::CipherClientToggledSwiftCodeVisible,
EventType::CipherClientCopiedIban,
EventType::CipherClientToggledIbanVisible,
EventType::CipherClientCopiedNationalIdentificationNumber,
EventType::CipherClientToggledNationalIdentificationNumberVisible,
] { ] {
assert_eq!(client_event_kind(event_type as i32), Some(ClientEventKind::Cipher)); assert_eq!(client_event_kind(event_type as i32), Some(ClientEventKind::Cipher));
} }
assert_eq!( for event_type in [
client_event_kind(EventType::OrganizationClientExportedVault as i32), EventType::OrganizationClientExportedVault,
Some(ClientEventKind::Organization) EventType::OrganizationItemOrganizationAccepted,
); EventType::OrganizationItemOrganizationDeclined,
EventType::OrganizationAutoConfirmEnabledAdmin,
EventType::OrganizationAutoConfirmDisabledAdmin,
EventType::OrganizationInviteLinkClientCopied,
] {
assert_eq!(client_event_kind(event_type as i32), Some(ClientEventKind::Organization));
}
// Upstream does not accept the TOTP seed toggle from clients either.
assert_eq!(client_event_kind(1118), None);
for event_type in [ for event_type in [
EventType::UserLoggedIn, EventType::UserLoggedIn,

35
src/db/models/collection.rs

@ -55,6 +55,10 @@ pub struct CollectionCipher {
/// Serialize the assignment-level `manage` capability using the same role boundary as the /// Serialize the assignment-level `manage` capability using the same role boundary as the
/// collection mutation guards. Read/write access is deliberately not management authority. /// collection mutation guards. Read/write access is deliberately not management authority.
///
/// This answers "may this member manage this collection?" and therefore belongs on the objects a
/// member receives about themselves. For the administrative lists that echo a *stored* grant back
/// to the client, use `stored_assignment_manage` instead.
pub(super) fn assignment_manage_for_member(membership_type: i32, stored_manage: bool) -> bool { pub(super) fn assignment_manage_for_member(membership_type: i32, stored_manage: bool) -> bool {
match MembershipType::from_i32(membership_type) { match MembershipType::from_i32(membership_type) {
Some(MembershipType::Owner | MembershipType::Admin) => true, Some(MembershipType::Owner | MembershipType::Admin) => true,
@ -63,6 +67,18 @@ pub(super) fn assignment_manage_for_member(membership_type: i32, stored_manage:
} }
} }
/// Serialize a *stored* per-collection assignment row for the admin-console access lists.
///
/// These lists describe the grant an administrator configured, and the client writes the very same
/// value back when the dialog is saved. Reporting anything other than the persisted bit would make
/// an unrelated save silently strip it — for a plain User that would also revoke the cipher write
/// access `users_collections.manage` still grants (see `Cipher::get_access_restrictions`). Admins
/// and Owners manage implicitly, so they are reported as managing regardless of the stored row.
pub(super) fn stored_assignment_manage(membership_type: i32, stored_manage: bool) -> bool {
matches!(MembershipType::from_i32(membership_type), Some(MembershipType::Owner | MembershipType::Admin))
|| stored_manage
}
/// Local methods /// Local methods
impl Collection { impl Collection {
pub fn new(org_uuid: OrganizationId, name: String, external_id: Option<String>) -> Self { pub fn new(org_uuid: OrganizationId, name: String, external_id: Option<String>) -> Self {
@ -938,7 +954,7 @@ impl CollectionMembership {
"id": self.membership_uuid, "id": self.membership_uuid,
"readOnly": self.read_only, "readOnly": self.read_only,
"hidePasswords": self.hide_passwords, "hidePasswords": self.hide_passwords,
"manage": assignment_manage_for_member(membership_type, self.manage), "manage": stored_assignment_manage(membership_type, self.manage),
}) })
} }
} }
@ -975,9 +991,24 @@ pub struct CollectionId(String);
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use super::assignment_manage_for_member; use super::{assignment_manage_for_member, stored_assignment_manage};
use crate::db::models::MembershipType; use crate::db::models::MembershipType;
// A stored `users_collections.manage` row must survive being listed in the admin console and
// written back unchanged. Reporting `false` for a plain User made an unrelated save strip the
// grant, which also revoked the cipher write access the row still confers.
#[test]
fn stored_assignment_manage_echoes_the_persisted_grant() {
for role in [MembershipType::Owner, MembershipType::Admin] {
assert!(stored_assignment_manage(role as i32, false));
}
for role in [MembershipType::Custom, MembershipType::User] {
assert!(stored_assignment_manage(role as i32, true));
assert!(!stored_assignment_manage(role as i32, false));
}
}
#[test] #[test]
fn assignment_manage_matches_collection_guard_role_boundaries() { fn assignment_manage_matches_collection_guard_role_boundaries() {
for role in [MembershipType::Owner, MembershipType::Admin] { for role in [MembershipType::Owner, MembershipType::Admin] {

20
src/db/models/event.rs

@ -79,6 +79,21 @@ pub enum EventType {
CipherSoftDeleted = 1115, CipherSoftDeleted = 1115,
CipherRestored = 1116, CipherRestored = 1116,
CipherClientToggledCardNumberVisible = 1117, CipherClientToggledCardNumberVisible = 1117,
// CipherClientToggledTOTPSeedVisible = 1118, // Not accepted from clients by upstream either
CipherClientCopiedBankAccountNumber = 1119,
CipherClientCopiedBankAccountPin = 1120,
CipherClientToggledBankAccountNumberVisible = 1121,
CipherClientToggledBankAccountPinVisible = 1122,
CipherClientCopiedLicenseNumber = 1123,
CipherClientToggledLicenseNumberVisible = 1124,
CipherClientCopiedPassportNumber = 1125,
CipherClientToggledPassportNumberVisible = 1126,
CipherClientCopiedSwiftCode = 1127,
CipherClientToggledSwiftCodeVisible = 1128,
CipherClientCopiedIban = 1129,
CipherClientToggledIbanVisible = 1130,
CipherClientCopiedNationalIdentificationNumber = 1131,
CipherClientToggledNationalIdentificationNumberVisible = 1132,
// Collection // Collection
CollectionCreated = 1300, CollectionCreated = 1300,
@ -120,6 +135,11 @@ pub enum EventType {
// OrganizationDisabledKeyConnector = 1607, // Not supported // OrganizationDisabledKeyConnector = 1607, // Not supported
// OrganizationSponsorshipsSynced = 1608, // Not supported // OrganizationSponsorshipsSynced = 1608, // Not supported
// OrganizationCollectionManagementUpdated = 1609, // Not supported // OrganizationCollectionManagementUpdated = 1609, // Not supported
OrganizationItemOrganizationAccepted = 1618,
OrganizationItemOrganizationDeclined = 1619,
OrganizationAutoConfirmEnabledAdmin = 1620,
OrganizationAutoConfirmDisabledAdmin = 1621,
OrganizationInviteLinkClientCopied = 1627,
// Policy // Policy
PolicyUpdated = 1700, PolicyUpdated = 1700,

5
src/db/models/organization.rs

@ -25,7 +25,8 @@ use macros::UuidFromParam;
use super::{ use super::{
Cipher, CipherId, Collection, CollectionGroup, CollectionId, CollectionUser, Group, GroupId, GroupUser, OrgPolicy, Cipher, CipherId, Collection, CollectionGroup, CollectionId, CollectionUser, Group, GroupId, GroupUser, OrgPolicy,
OrgPolicyType, TwoFactor, User, UserId, collection::assignment_manage_for_member as assignment_manage, OrgPolicyType, TwoFactor, User, UserId,
collection::{assignment_manage_for_member as assignment_manage, stored_assignment_manage},
}; };
#[derive(Identifiable, Queryable, Insertable, AsChangeset)] #[derive(Identifiable, Queryable, Insertable, AsChangeset)]
@ -619,7 +620,7 @@ impl Membership {
let (read_only, hide_passwords, manage) = if self.has_full_access() { let (read_only, hide_passwords, manage) = if self.has_full_access() {
(false, false, assignment_manage(self.atype, false)) (false, false, assignment_manage(self.atype, false))
} else if let Some(cu) = cu.get(&c.uuid) { } else if let Some(cu) = cu.get(&c.uuid) {
(cu.read_only, cu.hide_passwords, assignment_manage(self.atype, cu.manage)) (cu.read_only, cu.hide_passwords, stored_assignment_manage(self.atype, cu.manage))
// If previous checks failed it might be that this user has access via a group, but we should not return those elements here // If previous checks failed it might be that this user has access via a group, but we should not return those elements here
// Those are returned via a special group endpoint // Those are returned via a special group endpoint
} else if cg.contains(&c.uuid) { } else if cg.contains(&c.uuid) {

Loading…
Cancel
Save