Add Custom Roles account recovery permission (#7965)

This commit is contained in:
Tom authored and GitHub committed 2026-11-15 15:25:06 +01:00
1 parent b0048fa27f
commit 00611ea80d
11 files changed
+168 -38

No files matched your search

@@ -0,0 +1,13 @@
-- Dropping the permission would silently take it away from the Custom members holding it. So while there are any,
-- this fails on the duplicate key below, before anything is dropped: take it away first, or restore the backup
-- taken before the upgrade.
-- A failed attempt leaves the temporary table on this connection. Clear it before retrying.
DROP TEMPORARY TABLE IF EXISTS custom_role_downgrade;
CREATE TEMPORARY TABLE custom_role_downgrade (blocked_by VARCHAR(255) NOT NULL PRIMARY KEY);
INSERT INTO custom_role_downgrade VALUES ('Manage account recovery: take it away first, or restore the backup taken before the upgrade');
INSERT INTO custom_role_downgrade
SELECT 'Manage account recovery: take it away first, or restore the backup taken before the upgrade'
FROM users_organizations WHERE manage_reset_password LIMIT 1;
DROP TEMPORARY TABLE custom_role_downgrade;
ALTER TABLE users_organizations DROP COLUMN manage_reset_password;
@@ -0,0 +1,3 @@
-- The Manage account recovery permission of the Custom member role. Like the other permissions it only takes
-- effect on a Custom membership (atype 4) while EXPERIMENTAL_CUSTOM_ROLES is enabled, and nobody starts with it.
ALTER TABLE users_organizations ADD COLUMN manage_reset_password BOOLEAN NOT NULL DEFAULT FALSE;
@@ -0,0 +1,11 @@
-- Dropping the permission would silently take it away from the Custom members holding it. So while there are any,
-- this fails on the duplicate key below: take it away first, or restore the backup taken before the upgrade.
-- It needs to run in one transaction, as Diesel does.
CREATE TEMPORARY TABLE custom_role_downgrade (blocked_by VARCHAR(255) NOT NULL PRIMARY KEY);
INSERT INTO custom_role_downgrade VALUES ('Manage account recovery: take it away first, or restore the backup taken before the upgrade');
INSERT INTO custom_role_downgrade
SELECT 'Manage account recovery: take it away first, or restore the backup taken before the upgrade'
FROM users_organizations WHERE manage_reset_password LIMIT 1;
DROP TABLE custom_role_downgrade;
ALTER TABLE users_organizations DROP COLUMN manage_reset_password;
@@ -0,0 +1,3 @@
-- The Manage account recovery permission of the Custom member role. Like the other permissions it only takes
-- effect on a Custom membership (atype 4) while EXPERIMENTAL_CUSTOM_ROLES is enabled, and nobody starts with it.
ALTER TABLE users_organizations ADD COLUMN manage_reset_password BOOLEAN NOT NULL DEFAULT FALSE;
@@ -0,0 +1,11 @@
-- Dropping the permission would silently take it away from the Custom members holding it. So while there are any,
-- this fails on the duplicate key below: take it away first, or restore the backup taken before the upgrade.
-- It needs to run in one transaction, as Diesel does.
CREATE TEMPORARY TABLE custom_role_downgrade (blocked_by VARCHAR(255) NOT NULL PRIMARY KEY);
INSERT INTO custom_role_downgrade VALUES ('Manage account recovery: take it away first, or restore the backup taken before the upgrade');
INSERT INTO custom_role_downgrade
SELECT 'Manage account recovery: take it away first, or restore the backup taken before the upgrade'
FROM users_organizations WHERE manage_reset_password LIMIT 1;
DROP TABLE custom_role_downgrade;
ALTER TABLE users_organizations DROP COLUMN manage_reset_password;
@@ -0,0 +1,3 @@
-- The Manage account recovery permission of the Custom member role. Like the other permissions it only takes
-- effect on a Custom membership (atype 4) while EXPERIMENTAL_CUSTOM_ROLES is enabled, and nobody starts with it.
ALTER TABLE users_organizations ADD COLUMN manage_reset_password BOOLEAN NOT NULL DEFAULT 0; -- FALSE
+96 -23
View File
@@ -13,8 +13,9 @@ use crate::{
}, },
auth::{ auth::{
AccessImportExportHeaders, AdminHeaders, CollectionAction, CollectionDeleteHeaders, CollectionReadHeaders, AccessImportExportHeaders, AdminHeaders, CollectionAction, CollectionDeleteHeaders, CollectionReadHeaders,
CollectionUsersReadHeaders, Headers, ManageGroupsHeaders, ManagePoliciesHeaders, ManageUsersHeaders, CollectionUsersReadHeaders, Headers, ManageGroupsHeaders, ManagePoliciesHeaders, ManageResetPasswordHeaders,
ManagerHeaders, ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders, custom_member_may, decode_invite, ManageUsersHeaders, ManagerHeaders, ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders, custom_member_may,
decode_invite,
}, },
db::{ db::{
DbConn, DbConn,
@@ -1007,13 +1008,18 @@ async fn get_members(
err!("Organization not found", "Organization id's do not match"); err!("Organization not found", "Organization id's do not match");
} }
if !headers.membership.has_full_access() && !headers.membership.has_custom_permission(CustomPermission::ManageUsers) // As in Bitwarden, Manage account recovery alone only lists the members whose account it can recover
{ let recovery_only = !headers.membership.has_full_access()
&& !headers.membership.has_custom_permission(CustomPermission::ManageUsers);
if recovery_only && !headers.membership.has_custom_permission(CustomPermission::ManageResetPassword) {
err_code!("Resource not found.", "User does not have full access", Status::NotFound.code); err_code!("Resource not found.", "User does not have full access", Status::NotFound.code);
} }
let mut users_json = Vec::new(); let mut users_json = Vec::new();
for u in Membership::find_by_org(&org_id, &conn).await { for u in Membership::find_by_org(&org_id, &conn).await {
if recovery_only && (!u.has_status(MembershipStatus::Confirmed) || u.reset_password_key.is_none()) {
continue;
}
// The user can be a manager instead of an admin, but we've checked above that they have full access // The user can be a manager instead of an admin, but we've checked above that they have full access
users_json.push( users_json.push(
u.to_json_details_for_admin( u.to_json_details_for_admin(
@@ -1036,7 +1042,7 @@ async fn get_members(
async fn post_org_keys( async fn post_org_keys(
org_id: OrganizationId, org_id: OrganizationId,
data: Json<OrgKeyData>, data: Json<OrgKeyData>,
headers: AdminHeaders, headers: ManageResetPasswordHeaders,
conn: DbConn, conn: DbConn,
) -> JsonResult { ) -> JsonResult {
if org_id != headers.org_id { if org_id != headers.org_id {
@@ -2883,9 +2889,10 @@ async fn get_groups_data(
|| headers.membership.has_custom_permission(CustomPermission::ManageUsers) || headers.membership.has_custom_permission(CustomPermission::ManageUsers)
|| headers.membership.has_custom_permission(CustomPermission::ManageGroups) || headers.membership.has_custom_permission(CustomPermission::ManageGroups)
} else { } else {
// Reports need the basic group list, not group details or management rights. // Reports and account recovery need the basic group list, not group details or management rights.
has_full_access has_full_access
|| headers.membership.has_custom_permission(CustomPermission::AccessReports) || headers.membership.has_custom_permission(CustomPermission::AccessReports)
|| headers.membership.has_custom_permission(CustomPermission::ManageResetPassword)
|| may_assign_collection_access(&headers.membership, &conn).await || may_assign_collection_access(&headers.membership, &conn).await
}; };
if !allowed { if !allowed {
@@ -3322,7 +3329,7 @@ async fn get_organization_keys(org_id: OrganizationId, headers: Headers, conn: D
async fn put_recover_account( async fn put_recover_account(
org_id: OrganizationId, org_id: OrganizationId,
member_id: MembershipId, member_id: MembershipId,
headers: AdminHeaders, headers: ManageResetPasswordHeaders,
data: Json<OrganizationUserRecoverAccountRequest>, data: Json<OrganizationUserRecoverAccountRequest>,
conn: DbConn, conn: DbConn,
nt: Notify<'_>, nt: Notify<'_>,
@@ -3333,7 +3340,7 @@ async fn put_recover_account(
async fn recover_account( async fn recover_account(
org_id: OrganizationId, org_id: OrganizationId,
member_id: MembershipId, member_id: MembershipId,
headers: AdminHeaders, headers: ManageResetPasswordHeaders,
req: OrganizationUserRecoverAccountRequest, req: OrganizationUserRecoverAccountRequest,
conn: DbConn, conn: DbConn,
nt: Notify<'_>, nt: Notify<'_>,
@@ -3353,7 +3360,7 @@ async fn recover_account(
err!("User not found") err!("User not found")
}; };
check_reset_password_applicable_and_permissions(&org_id, &member_id, &headers, &conn).await?; check_reset_password_applicable_and_permissions(&org_id, &member_id, &headers.membership, &conn).await?;
if member.reset_password_key.is_none() { if member.reset_password_key.is_none() {
err!("Password reset not or not correctly enrolled"); err!("Password reset not or not correctly enrolled");
@@ -3423,12 +3430,14 @@ async fn recover_account(
nt.send_logout(&user, None, &conn).await; nt.send_logout(&user, None, &conn).await;
if req.reset_master_password { for (reset, event_type) in [
headers.log_event(EventType::OrganizationUserAdminResetPassword, &member_id, &org_id, &conn).await; (req.reset_master_password, EventType::OrganizationUserAdminResetPassword),
} (req.reset_two_factor, EventType::OrganizationUserAdminResetTwoFactor),
] {
if req.reset_two_factor { if reset {
headers.log_event(EventType::OrganizationUserAdminResetTwoFactor, &member_id, &org_id, &conn).await; log_event(event_type, &member_id, &org_id, &headers.user.uuid, headers.device.atype, &headers.ip.ip, &conn)
.await;
}
} }
Ok(()) Ok(())
@@ -3599,7 +3608,7 @@ async fn get_organization_private_key(org_id: OrganizationId, headers: ManageUse
async fn get_reset_password_details( async fn get_reset_password_details(
org_id: OrganizationId, org_id: OrganizationId,
member_id: MembershipId, member_id: MembershipId,
headers: AdminHeaders, headers: ManageResetPasswordHeaders,
conn: DbConn, conn: DbConn,
) -> JsonResult { ) -> JsonResult {
if org_id != headers.org_id { if org_id != headers.org_id {
@@ -3617,7 +3626,7 @@ async fn get_reset_password_details(
err!("User not found") err!("User not found")
}; };
check_reset_password_applicable_and_permissions(&org_id, &member_id, &headers, &conn).await?; check_reset_password_applicable_and_permissions(&org_id, &member_id, &headers.membership, &conn).await?;
// https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Api/AdminConsole/Models/Response/Organizations/OrganizationUserResponseModel.cs#L190 // https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Api/AdminConsole/Models/Response/Organizations/OrganizationUserResponseModel.cs#L190
Ok(Json(json!({ Ok(Json(json!({
@@ -3636,7 +3645,7 @@ async fn get_reset_password_details(
async fn check_reset_password_applicable_and_permissions( async fn check_reset_password_applicable_and_permissions(
org_id: &OrganizationId, org_id: &OrganizationId,
member_id: &MembershipId, member_id: &MembershipId,
headers: &AdminHeaders, caller: &Membership,
conn: &DbConn, conn: &DbConn,
) -> EmptyResult { ) -> EmptyResult {
check_reset_password_applicable(org_id, conn).await?; check_reset_password_applicable(org_id, conn).await?;
@@ -3645,11 +3654,25 @@ async fn check_reset_password_applicable_and_permissions(
err!("Reset target user not found") err!("Reset target user not found")
}; };
// Resetting user must be higher/equal to user to reset if may_recover_account(caller, &target_user) {
match headers.membership_type { Ok(())
MembershipType::Owner => Ok(()), } else {
MembershipType::Admin if target_user.atype <= MembershipType::Admin => Ok(()), err!("No permission to reset this user's password")
_ => err!("No permission to reset this user's password"), }
}
/// Whether `caller` may recover the account of `target`, who must not have more authority, as in Bitwarden: Owners
/// may recover every member, Admins every member but Owners, and Custom members with Manage account recovery Users
/// and the Custom members whose permissions they all have.
fn may_recover_account(caller: &Membership, target: &Membership) -> bool {
if caller.has_type(MembershipType::Owner) {
true
} else if caller.has_type(MembershipType::Admin) {
target.atype <= MembershipType::Admin
} else {
caller.has_custom_permission(CustomPermission::ManageResetPassword)
&& custom_member_manages(target.atype)
&& caller.has_custom_permissions_of(target)
} }
} }
@@ -4035,4 +4058,54 @@ mod tests {
} }
}); });
} }
#[cfg(sqlite)]
#[test]
fn account_recovery_authorization() {
test_db::run(async |db| {
use CustomPermission::{ManageResetPassword, ManageUsers};
use MembershipType::*;
let conn = db.conn();
let org = test_db::organization(&conn).await;
let mut members = Vec::new();
for (role, permissions) in [
(Owner, vec![]),
(Admin, vec![]),
(Manager, vec![]),
(User, vec![]),
(Custom, vec![ManageResetPassword]),
(Custom, vec![ManageResetPassword, ManageUsers]),
] {
members.push(test_db::member(&conn, &org.uuid, role, &permissions).await.1);
}
// Who may recover whom: a row per caller, a column per target, both in the order above
for (caller, row) in members.iter().zip(["111111", "011111", "000000", "000000", "000110", "000111"]) {
for (target, allowed) in members.iter().zip(row.chars()) {
assert_eq!(
may_recover_account(caller, target),
allowed == '1',
"{} {}",
caller.atype,
target.atype
);
}
}
// Manage account recovery alone lists only the confirmed members enrolled in account recovery
for (i, status) in [(2, MembershipStatus::Accepted), (3, MembershipStatus::Confirmed)] {
members[i].status = status as i32;
members[i].reset_password_key = Some("key".into());
members[i].save(&conn).await.unwrap();
}
let data = GetOrgUserData {
include_collections: None,
include_groups: None,
};
let users = get_members(data, org.uuid.clone(), directory_headers(&members[4], &conn).await, db.conn());
assert_eq!(
users.await.unwrap().0["data"],
json!([members[3].to_json_details_for_admin(false, false, &conn).await])
);
assert!(get_groups(org.uuid.clone(), directory_headers(&members[4], &conn).await, db.conn()).await.is_ok());
});
}
} }
+8 -9
View File
@@ -27,14 +27,14 @@ use rocket::{
use crate::{ use crate::{
CONFIG, CONFIG,
api::{ApiResult, core::log_event}, api::ApiResult,
config::PathType, config::PathType,
db::{ db::{
DbConn, DbConn,
models::{ models::{
AttachmentId, CipherId, Collection, CollectionId, CustomPermission, Device, DeviceId, DeviceType, AttachmentId, CipherId, Collection, CollectionId, CustomPermission, Device, DeviceId, DeviceType,
EmergencyAccessId, EventType, Membership, MembershipId, MembershipStatus, MembershipType, OrgApiKeyId, EmergencyAccessId, Membership, MembershipId, MembershipStatus, MembershipType, OrgApiKeyId, OrganizationId,
OrganizationId, SendFileId, SendId, User, UserId, custom_roles_enabled, SendFileId, SendId, User, UserId, custom_roles_enabled,
}, },
}, },
error::Error, error::Error,
@@ -854,12 +854,6 @@ pub struct AdminHeaders {
pub org_id: OrganizationId, pub org_id: OrganizationId,
} }
impl AdminHeaders {
pub async fn log_event(&self, event_type: EventType, source_uuid: &str, org_id: &OrganizationId, conn: &DbConn) {
log_event(event_type, source_uuid, org_id, &self.user.uuid, self.device.atype, &self.ip.ip, conn).await;
}
}
#[rocket::async_trait] #[rocket::async_trait]
impl<'r> FromRequest<'r> for AdminHeaders { impl<'r> FromRequest<'r> for AdminHeaders {
type Error = &'static str; type Error = &'static str;
@@ -947,6 +941,11 @@ permission_headers!(
AccessImportExportHeaders, AccessImportExportHeaders,
CustomPermission::AccessImportExport CustomPermission::AccessImportExport
); );
permission_headers!(
/// For the routes that recover the accounts of the members of an organization.
ManageResetPasswordHeaders,
CustomPermission::ManageResetPassword
);
// col_id is usually the fourth path param ("/organizations/<org_id>/collections/<col_id>"), // col_id is usually the fourth path param ("/organizations/<org_id>/collections/<col_id>"),
// but there could be cases where it is a query value. // but there could be cases where it is a query value.
+17 -3
View File
@@ -81,6 +81,7 @@ pub struct Membership {
access_event_logs: bool, access_event_logs: bool,
access_import_export: bool, access_import_export: bool,
access_reports: bool, access_reports: bool,
manage_reset_password: bool,
/// The user's v2 upgrade token, for the org to unwrap the v2 user key after an upgrade. Opaque to us. /// The user's v2 upgrade token, for the org to unwrap the v2 user key after an upgrade. Opaque to us.
pub v2_upgrade_token: Option<String>, pub v2_upgrade_token: Option<String>,
@@ -254,6 +255,7 @@ pub enum CustomPermission {
AccessEventLogs, AccessEventLogs,
AccessImportExport, AccessImportExport,
AccessReports, AccessReports,
ManageResetPassword,
} }
/// The permissions this version enforces on every endpoint they open. Only these can be granted, a stored /// The permissions this version enforces on every endpoint they open. Only these can be granted, a stored
@@ -269,10 +271,11 @@ const SUPPORTED_CUSTOM_PERMISSIONS: &[CustomPermission] = &[
CustomPermission::AccessEventLogs, CustomPermission::AccessEventLogs,
CustomPermission::AccessImportExport, CustomPermission::AccessImportExport,
CustomPermission::AccessReports, CustomPermission::AccessReports,
CustomPermission::ManageResetPassword,
]; ];
impl CustomPermission { impl CustomPermission {
pub const ALL: [CustomPermission; 9] = [ pub const ALL: [CustomPermission; 10] = [
CustomPermission::ManageUsers, CustomPermission::ManageUsers,
CustomPermission::ManageGroups, CustomPermission::ManageGroups,
CustomPermission::ManagePolicies, CustomPermission::ManagePolicies,
@@ -282,6 +285,7 @@ impl CustomPermission {
CustomPermission::AccessEventLogs, CustomPermission::AccessEventLogs,
CustomPermission::AccessImportExport, CustomPermission::AccessImportExport,
CustomPermission::AccessReports, CustomPermission::AccessReports,
CustomPermission::ManageResetPassword,
]; ];
/// The key of the permission in the `permissions` object of the Bitwarden clients. /// The key of the permission in the `permissions` object of the Bitwarden clients.
@@ -296,6 +300,7 @@ impl CustomPermission {
CustomPermission::AccessEventLogs => "accessEventLogs", CustomPermission::AccessEventLogs => "accessEventLogs",
CustomPermission::AccessImportExport => "accessImportExport", CustomPermission::AccessImportExport => "accessImportExport",
CustomPermission::AccessReports => "accessReports", CustomPermission::AccessReports => "accessReports",
CustomPermission::ManageResetPassword => "manageResetPassword",
} }
} }
@@ -468,6 +473,7 @@ impl Membership {
access_event_logs: false, access_event_logs: false,
access_import_export: false, access_import_export: false,
access_reports: false, access_reports: false,
manage_reset_password: false,
v2_upgrade_token: None, v2_upgrade_token: None,
} }
@@ -484,6 +490,7 @@ impl Membership {
CustomPermission::AccessEventLogs => &mut self.access_event_logs, CustomPermission::AccessEventLogs => &mut self.access_event_logs,
CustomPermission::AccessImportExport => &mut self.access_import_export, CustomPermission::AccessImportExport => &mut self.access_import_export,
CustomPermission::AccessReports => &mut self.access_reports, CustomPermission::AccessReports => &mut self.access_reports,
CustomPermission::ManageResetPassword => &mut self.manage_reset_password,
} }
} }
@@ -553,6 +560,7 @@ impl Membership {
CustomPermission::AccessEventLogs => self.access_event_logs, CustomPermission::AccessEventLogs => self.access_event_logs,
CustomPermission::AccessImportExport => self.access_import_export, CustomPermission::AccessImportExport => self.access_import_export,
CustomPermission::AccessReports => self.access_reports, CustomPermission::AccessReports => self.access_reports,
CustomPermission::ManageResetPassword => self.manage_reset_password,
} }
} }
@@ -571,6 +579,12 @@ impl Membership {
self.has_status(MembershipStatus::Confirmed) && self.grants_custom_permission(permission) self.has_status(MembershipStatus::Confirmed) && self.grants_custom_permission(permission)
} }
/// Whether this member may act with every Custom role permission stored for `other`, as Bitwarden's
/// `Permissions.Includes`.
pub fn has_custom_permissions_of(&self, other: &Membership) -> bool {
CustomPermission::ALL.into_iter().all(|p| !other.custom_permission(p) || self.has_custom_permission(p))
}
/// The permissions the "Manage all collections" a legacy Manager is shown with stands for: their `access_all`. /// The permissions the "Manage all collections" a legacy Manager is shown with stands for: their `access_all`.
pub fn legacy_manager_permissions(&self) -> HashSet<CustomPermission> { pub fn legacy_manager_permissions(&self) -> HashSet<CustomPermission> {
if self.has_type(MembershipType::Manager) && self.access_all { if self.has_type(MembershipType::Manager) && self.access_all {
@@ -597,7 +611,6 @@ impl Membership {
let shown = self.shown_custom_permissions(); let shown = self.shown_custom_permissions();
let mut permissions = json!({ let mut permissions = json!({
"manageSso": false, // Not supported "manageSso": false, // Not supported
"manageResetPassword": false,
"manageScim": false, // Not supported (Not AGPLv3 Licensed) "manageScim": false, // Not supported (Not AGPLv3 Licensed)
}); });
for permission in CustomPermission::ALL { for permission in CustomPermission::ALL {
@@ -1720,7 +1733,8 @@ mod tests {
DeleteAnyCollection, DeleteAnyCollection,
AccessEventLogs, AccessEventLogs,
AccessImportExport, AccessImportExport,
AccessReports AccessReports,
ManageResetPassword
] ]
); );
} }
+1
View File
@@ -269,6 +269,7 @@ table! {
access_event_logs -> Bool, access_event_logs -> Bool,
access_import_export -> Bool, access_import_export -> Bool,
access_reports -> Bool, access_reports -> Bool,
manage_reset_password -> Bool,
v2_upgrade_token -> Nullable<Text>, v2_upgrade_token -> Nullable<Text>,
} }
} }
@@ -99,9 +99,8 @@ app-security > app-two-factor-setup > form {
/* Hide unsupported Custom Role options */ /* Hide unsupported Custom Role options */
{{#if custom_roles_enabled}} {{#if custom_roles_enabled}}
/* Not supported: Manage SSO and Manage account recovery */ /* Not supported: Manage SSO */
:is(bit-dialog, [bit-dialog]) bit-form-control:has(input[formcontrolname="manageSso"]), :is(bit-dialog, [bit-dialog]) bit-form-control:has(input[formcontrolname="manageSso"]) {
:is(bit-dialog, [bit-dialog]) bit-form-control:has(input[formcontrolname="manageResetPassword"]) {
@extend %vw-hide; @extend %vw-hide;
} }
{{else}} {{else}}