From 52c02ce416434f52015be5cdd71a18f4818e7315 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Mon, 27 Jul 2026 00:10:37 +0200 Subject: [PATCH] Document accessReports scope and drop the unused reports guard Clippy (with --all-targets) reported two issues introduced by this branch: events.rs:356 items_after_test_module events.rs:395 redundant_clone Move the test module to the end of the file and drop the redundant clone. The relocation is content-identical; the only semantic change is the removed `org_id.clone()`, which was the last use of the binding. Note that CI runs clippy without --all-targets, so neither of these was failing the pipeline. Remove `AccessReportsHeaders` and its `can_access_reports` helper. The guard was never applied to any route and could not be: Vaultwarden has no server-side report endpoints, because the clients compute every report locally from the organization cipher list. `accessReports` is therefore enforced inline in `get_org_details`, where that list is actually served. A note at both former sites records this, so the guard is not reintroduced and an endpoint is not gated on "may call reports" instead of "may read these ciphers". Document the resulting authorization semantics above `get_org_details`: the endpoint serializes with CipherSyncType::Organization, which skips the per-cipher access restrictions, so readOnly/hidePasswords are not applied and collection assignments are ignored. `accessReports` is by design a full organization read permission. This matches Bitwarden's CanAccessAllCiphersAsync, which admits Owner/Admin plus Custom members holding AccessImportExport, EditAnyCollection or AccessReports. We stay deliberately stricter than Bitwarden for accessImportExport: it does not open this endpoint, and get_org_export scopes its output to the caller's own collections, so "may export" never widens what a member can read. No behavioural change. --- src/api/core/events.rs | 239 +++++++++++++++++----------------- src/api/core/organizations.rs | 20 +++ src/auth.rs | 16 +-- 3 files changed, 145 insertions(+), 130 deletions(-) diff --git a/src/api/core/events.rs b/src/api/core/events.rs index b856176c..ba859d31 100644 --- a/src/api/core/events.rs +++ b/src/api/core/events.rs @@ -352,6 +352,123 @@ async fn post_events_collect(data: Json>, headers: Headers, Ok(()) } +pub async fn log_user_event(event_type: i32, user_id: &UserId, device_type: i32, ip: &IpAddr, conn: &DbConn) { + if !CONFIG.org_events_enabled() { + return; + } + log_user_event_impl(event_type, user_id, device_type, None, ip, conn).await; +} + +async fn log_user_event_impl( + event_type: i32, + user_id: &UserId, + device_type: i32, + event_date: Option, + ip: &IpAddr, + conn: &DbConn, +) { + let memberships = Membership::find_confirmed_by_user(user_id, conn).await; + let mut events: Vec = Vec::with_capacity(memberships.len() + 1); // We need an event per org and one without an org + + // Upstream saves the event also without any org_id. + let mut event = Event::new(event_type, event_date); + event.user_uuid = Some(user_id.clone()); + event.act_user_uuid = Some(user_id.clone()); + event.device_type = Some(device_type); + event.ip_address = Some(ip.to_string()); + events.push(event); + + // For each org a user is a member of store these events per org + for membership in memberships { + let mut event = Event::new(event_type, event_date); + event.user_uuid = Some(user_id.clone()); + event.org_uuid = Some(membership.org_uuid); + event.org_user_uuid = Some(membership.uuid); + event.act_user_uuid = Some(user_id.clone()); + event.device_type = Some(device_type); + event.ip_address = Some(ip.to_string()); + events.push(event); + } + + Event::save_user_event(events, conn).await.unwrap_or(()); +} + +pub async fn log_event( + event_type: i32, + source_uuid: &str, + org_id: &OrganizationId, + act_user_id: &UserId, + device_type: i32, + ip: &IpAddr, + conn: &DbConn, +) { + if !CONFIG.org_events_enabled() { + return; + } + log_event_impl(event_type, source_uuid, org_id, act_user_id, device_type, None, ip, conn).await; +} + +#[expect(clippy::too_many_arguments)] +async fn log_event_impl( + event_type: i32, + source_uuid: &str, + org_id: &OrganizationId, + act_user_id: &UserId, + device_type: i32, + event_date: Option, + ip: &IpAddr, + conn: &DbConn, +) { + // Create a new empty event + let mut event = Event::new(event_type, event_date); + match event_type { + // 1000..=1099 Are user events, they need to be logged via log_user_event() + // Cipher Events + 1100..=1199 => { + event.cipher_uuid = Some(source_uuid.to_owned().into()); + } + // Collection Events + 1300..=1399 => { + event.collection_uuid = Some(source_uuid.to_owned().into()); + } + // Group Events + 1400..=1499 => { + event.group_uuid = Some(source_uuid.to_owned().into()); + } + // Org User Events + 1500..=1599 => { + event.org_user_uuid = Some(source_uuid.to_owned().into()); + } + // 1600..=1699 Are organizational events, and they do not need the source_uuid + // Policy Events + 1700..=1799 => { + event.policy_uuid = Some(source_uuid.to_owned().into()); + } + // Ignore others + _ => {} + } + + event.org_uuid = Some(org_id.clone()); + event.act_user_uuid = Some(act_user_id.clone()); + event.device_type = Some(device_type); + event.ip_address = Some(ip.to_string()); + event.save(conn).await.unwrap_or(()); +} + +pub async fn event_cleanup_job(pool: DbPool) { + debug!("Start events cleanup job"); + if CONFIG.events_days_retain().is_none() { + debug!("events_days_retain is not configured, abort"); + return; + } + + if let Ok(conn) = pool.get().await { + Event::clean_events(&conn).await.ok(); + } else { + error!("Failed to get DB connection while trying to cleanup the events table"); + } +} + #[cfg(test)] mod tests { use super::*; @@ -390,10 +507,7 @@ mod tests { cipher.organization_uuid = Some(org_id.clone()); let admin = membership(MembershipType::Admin, MembershipStatus::Confirmed); - assert_eq!( - cipher_event_scope(&cipher, &user_id, Some(&admin)), - Some(CipherEventScope::Organization(org_id.clone())) - ); + assert_eq!(cipher_event_scope(&cipher, &user_id, Some(&admin)), Some(CipherEventScope::Organization(org_id))); let accepted_admin = membership(MembershipType::Admin, MembershipStatus::Accepted); assert_eq!(cipher_event_scope(&cipher, &user_id, Some(&accepted_admin)), None); @@ -503,120 +617,3 @@ mod tests { assert!(validate_client_event_batch_size(MAX_CLIENT_EVENT_BATCH_SIZE + 1).is_err()); } } - -pub async fn log_user_event(event_type: i32, user_id: &UserId, device_type: i32, ip: &IpAddr, conn: &DbConn) { - if !CONFIG.org_events_enabled() { - return; - } - log_user_event_impl(event_type, user_id, device_type, None, ip, conn).await; -} - -async fn log_user_event_impl( - event_type: i32, - user_id: &UserId, - device_type: i32, - event_date: Option, - ip: &IpAddr, - conn: &DbConn, -) { - let memberships = Membership::find_confirmed_by_user(user_id, conn).await; - let mut events: Vec = Vec::with_capacity(memberships.len() + 1); // We need an event per org and one without an org - - // Upstream saves the event also without any org_id. - let mut event = Event::new(event_type, event_date); - event.user_uuid = Some(user_id.clone()); - event.act_user_uuid = Some(user_id.clone()); - event.device_type = Some(device_type); - event.ip_address = Some(ip.to_string()); - events.push(event); - - // For each org a user is a member of store these events per org - for membership in memberships { - let mut event = Event::new(event_type, event_date); - event.user_uuid = Some(user_id.clone()); - event.org_uuid = Some(membership.org_uuid); - event.org_user_uuid = Some(membership.uuid); - event.act_user_uuid = Some(user_id.clone()); - event.device_type = Some(device_type); - event.ip_address = Some(ip.to_string()); - events.push(event); - } - - Event::save_user_event(events, conn).await.unwrap_or(()); -} - -pub async fn log_event( - event_type: i32, - source_uuid: &str, - org_id: &OrganizationId, - act_user_id: &UserId, - device_type: i32, - ip: &IpAddr, - conn: &DbConn, -) { - if !CONFIG.org_events_enabled() { - return; - } - log_event_impl(event_type, source_uuid, org_id, act_user_id, device_type, None, ip, conn).await; -} - -#[expect(clippy::too_many_arguments)] -async fn log_event_impl( - event_type: i32, - source_uuid: &str, - org_id: &OrganizationId, - act_user_id: &UserId, - device_type: i32, - event_date: Option, - ip: &IpAddr, - conn: &DbConn, -) { - // Create a new empty event - let mut event = Event::new(event_type, event_date); - match event_type { - // 1000..=1099 Are user events, they need to be logged via log_user_event() - // Cipher Events - 1100..=1199 => { - event.cipher_uuid = Some(source_uuid.to_owned().into()); - } - // Collection Events - 1300..=1399 => { - event.collection_uuid = Some(source_uuid.to_owned().into()); - } - // Group Events - 1400..=1499 => { - event.group_uuid = Some(source_uuid.to_owned().into()); - } - // Org User Events - 1500..=1599 => { - event.org_user_uuid = Some(source_uuid.to_owned().into()); - } - // 1600..=1699 Are organizational events, and they do not need the source_uuid - // Policy Events - 1700..=1799 => { - event.policy_uuid = Some(source_uuid.to_owned().into()); - } - // Ignore others - _ => {} - } - - event.org_uuid = Some(org_id.clone()); - event.act_user_uuid = Some(act_user_id.clone()); - event.device_type = Some(device_type); - event.ip_address = Some(ip.to_string()); - event.save(conn).await.unwrap_or(()); -} - -pub async fn event_cleanup_job(pool: DbPool) { - debug!("Start events cleanup job"); - if CONFIG.events_days_retain().is_none() { - debug!("events_days_retain is not configured, abort"); - return; - } - - if let Ok(conn) = pool.get().await { - Event::clean_events(&conn).await.ok(); - } else { - error!("Failed to get DB connection while trying to cleanup the events table"); - } -} diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index f4975672..8e7c8057 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -1069,6 +1069,26 @@ async fn get_assigned_org_details(data: OrgIdData, headers: Headers, conn: DbCon }))) } +// Returns every cipher in the organization, serialized with `CipherSyncType::Organization` — which +// deliberately skips the per-cipher access restrictions, so `readOnly`/`hidePasswords` are not +// applied and collection assignments are ignored. Whoever passes the check below reads the whole +// organization vault. +// +// `accessReports` is therefore, by design, a full organization *read* permission and not merely +// "may open the reports screen". Vaultwarden implements no server-side reports: the clients fetch +// this list and compute Exposed/Reused/Weak Passwords, Unsecured Websites, Inactive 2FA etc. +// locally, so the permission cannot be satisfied with less data. +// +// This matches Bitwarden upstream, which grants the same endpoint to Owner/Admin and to Custom +// members holding AccessImportExport, EditAnyCollection *or* AccessReports: +// https://github.com/bitwarden/server/blob/main/src/Api/Vault/Controllers/CiphersController.cs +// (`CanAccessAllCiphersAsync`) +// +// We are intentionally *stricter* than Bitwarden for `accessImportExport`: it does not open this +// endpoint, and `get_org_export` scopes its output to the caller's own collections, so +// "may export" never widens what a member can read. Granting `accessReports` does widen it — that +// is the documented trade-off of staying Bitwarden-compatible, and administrators must treat +// `accessReports` as equivalent to read access to every collection in the organization. #[get("/ciphers/organization-details?")] async fn get_org_details(data: OrgIdData, headers: ManagerHeadersLoose, conn: DbConn) -> JsonResult { if data.organization_id != headers.membership.org_uuid { diff --git a/src/auth.rs b/src/auth.rs index d008543d..39533abd 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -766,10 +766,11 @@ impl OrgHeaders { self.is_confirmed() && (self.membership_type >= MembershipType::Admin || self.membership.has_access_import_export()) } - - fn can_access_reports(&self) -> bool { - self.is_confirmed() && (self.membership_type >= MembershipType::Admin || self.membership.has_access_reports()) - } + // NOTE: there is deliberately no `can_access_reports` guard helper. Vaultwarden has no + // server-side report endpoints — the clients compute every report locally from the + // organization cipher list — so `accessReports` is enforced inline where that list is served + // (`get_org_details`), not through a request guard. A guard here would be dead code that + // invites gating an endpoint on "may call reports" instead of "may read these ciphers". } // org_id is usually the second path param ("/organizations/"), @@ -970,11 +971,8 @@ generate_manage_headers!( can_access_import_export, "You need the 'Access Import/Export' permission, or to be an Admin or Owner, to call this endpoint" ); -generate_manage_headers!( - AccessReportsHeaders, - can_access_reports, - "You need the 'Access Reports' permission, or to be an Admin or Owner, to call this endpoint" -); +// NOTE: no `AccessReportsHeaders`. See the note next to `can_access_import_export` above: +// `accessReports` guards data (the organization cipher list), not a dedicated endpoint. // col_id is usually the fourth path param ("/organizations//collections/"), // but there could be cases where it is a query value.