Add Custom Roles group and policy permissions (#7962)

src/api/core/organizations.rs

@@ -13,8 +13,8 @@ use crate::{
     },
     auth::{
         AdminHeaders, CollectionAction, CollectionDeleteHeaders, CollectionReadHeaders, CollectionUsersReadHeaders,
-        Headers, ManageUsersHeaders, ManagerHeaders, ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders,
+        Headers, ManageGroupsHeaders, ManagePoliciesHeaders, ManageUsersHeaders, ManagerHeaders, ManagerHeadersLoose,
-        custom_member_may, decode_invite,
+        OrgMemberHeaders, OwnerHeaders, custom_member_may, decode_invite,
     },
     db::{
         DbConn,
@@ -369,7 +369,10 @@ async fn get_org_collections(org_id: OrganizationId, headers: ManagerHeadersLoos
         err!("Organization not found", "Organization id's do not match");
     }
 
-    let collections = if headers.membership.can_view_all_collections() {
+    // Manage groups may list organization collections for group access administration without editing them.
+    let collections = if headers.membership.can_view_all_collections()
+        || headers.membership.has_custom_permission(CustomPermission::ManageGroups)
+    {
         get_org_collections_impl(&org_id, &conn).await
     } else if headers.membership.has_type(MembershipType::Custom) {
         // A Custom member gets the collections they may manage
@@ -413,13 +416,15 @@ async fn get_org_collections_details(org_id: OrganizationId, headers: ManagerHea
     // check if current user has full access to the organization (either directly or via any group)
     let has_full_access_to_org = member.has_full_access()
         || (CONFIG.org_groups_enabled() && GroupUser::has_full_access_by_member(&org_id, &member.uuid, &conn).await);
-    // Manage users may read collection access details without editing the collection itself.
+    // Manage users and Manage groups may read collection access details without editing collection metadata.
-    let custom_collections =
+    let custom_collections = if member.has_type(MembershipType::Custom)
-        if member.has_type(MembershipType::Custom) && !member.can_view_all_collection_access_details() {
+        && !member.can_view_all_collection_access_details()
-            Some(Collection::find_explicitly_manageable_by_user(&org_id, &member.user_uuid, &conn).await)
+        && !member.has_custom_permission(CustomPermission::ManageGroups)
-        } else {
+    {
-            None
+        Some(Collection::find_explicitly_manageable_by_user(&org_id, &member.user_uuid, &conn).await)
-        };
+    } else {
+        None
+    };
 
     // Get all admins, owners and managers who can manage/access all
     // Those are currently not listed in the col_users but need to be listed too.
@@ -591,13 +596,22 @@ async fn post_bulk_access_collections(
         err!("Invalid group", format!("Group {} does not belong to organization {}!", g.id.escape_debug(), org_id))
     }
 
-    for col_id in data.collection_ids {
+    if data.collection_ids.is_empty()
-        let Some(collection) = Collection::find_by_uuid_and_org(&col_id, &org_id, &conn).await else {
+        || data.collection_ids.iter().collect::<HashSet<_>>().len() != data.collection_ids.len()
+    {
+        err!("Invalid collection list")
+    }
+
+    let mut collections = Vec::with_capacity(data.collection_ids.len());
+    for col_id in &data.collection_ids {
+        let Some(collection) = Collection::find_by_uuid_and_org(col_id, &org_id, &conn).await else {
             err!("Collection not found")
         };
 
         let manageable = if headers.membership.has_type(MembershipType::Custom) {
             custom_member_may(CollectionAction::Edit, &headers.membership, &collection.uuid, &conn).await
+                || (headers.membership.has_custom_permission(CustomPermission::ManageUsers)
+                    && headers.membership.has_custom_permission(CustomPermission::ManageGroups))
         } else {
             collection.is_manageable_by_user(&headers.membership.user_uuid, &conn).await
         };
@@ -605,6 +619,19 @@ async fn post_bulk_access_collections(
             err!("Collection not found", "The current user isn't a manager for this collection")
         }
 
+        collections.push(collection);
+    }
+
+    let mut members = Vec::with_capacity(data.users.len());
+    for user in &data.users {
+        let Some(member) = Membership::find_by_uuid_and_org(&user.id, &org_id, &conn).await else {
+            err!("User is not part of organization")
+        };
+        members.push(member);
+    }
+
+    for collection in collections {
+        let col_id = collection.uuid.clone();
         // update collection modification date
         collection.save(&conn).await?;
 
@@ -619,19 +646,13 @@ async fn post_bulk_access_collections(
         )
         .await;
 
-        CollectionGroup::delete_all_by_collection(&col_id, &org_id, &conn).await?;
         for group in &data.groups {
             CollectionGroup::new(col_id.clone(), group.id.clone(), group.read_only, group.hide_passwords, group.manage)
                 .save(&org_id, &conn)
                 .await?;
         }
 
-        CollectionUser::delete_all_by_collection(&col_id, &conn).await?;
+        for (user, member) in data.users.iter().zip(&members) {
-        for user in &data.users {
-            let Some(member) = Membership::find_by_uuid_and_org(&user.id, &org_id, &conn).await else {
-                err!("User is not part of organization")
-            };
-
             if member.access_all {
                 continue;
             }
@@ -2262,7 +2283,7 @@ async fn post_bulk_collections(data: Json<BulkCollectionsData>, headers: Headers
 }
 
 #[get("/organizations/<org_id>/policies")]
-async fn list_policies(org_id: OrganizationId, headers: AdminHeaders, conn: DbConn) -> JsonResult {
+async fn list_policies(org_id: OrganizationId, headers: ManagePoliciesHeaders, conn: DbConn) -> JsonResult {
     if org_id != headers.org_id {
         err!("Organization not found", "Organization id's do not match");
     }
@@ -2328,7 +2349,7 @@ async fn get_master_password_policy(org_id: OrganizationId, _headers: OrgMemberH
 }
 
 #[get("/organizations/<org_id>/policies/<pol_type>", rank = 3)]
-async fn get_policy(org_id: OrganizationId, pol_type: i32, headers: AdminHeaders, conn: DbConn) -> JsonResult {
+async fn get_policy(org_id: OrganizationId, pol_type: i32, headers: ManagePoliciesHeaders, conn: DbConn) -> JsonResult {
     if org_id != headers.org_id {
         err!("Organization not found", "Organization id's do not match");
     }
@@ -2365,7 +2386,7 @@ async fn put_policy(
     org_id: OrganizationId,
     pol_type: i32,
     data: Json<PutPolicy>,
-    headers: AdminHeaders,
+    headers: ManagePoliciesHeaders,
     conn: DbConn,
 ) -> JsonResult {
     if org_id != headers.org_id {
@@ -2463,8 +2484,11 @@ async fn put_policy(
         }
     }
 
+    // Bitwarden enforces these policies when they first become enabled, not on every update to an enabled policy.
+    let newly_enabled = data.enabled && !is_currently_enabled;
+
     // When enabling the TwoFactorAuthentication policy, revoke all members that do not have 2FA
-    if pol_type_enum == OrgPolicyType::TwoFactorAuthentication && data.enabled {
+    if pol_type_enum == OrgPolicyType::TwoFactorAuthentication && newly_enabled {
         two_factor::enforce_2fa_policy_for_org(
             &org_id,
             &headers.user.uuid,
@@ -2476,38 +2500,9 @@ async fn put_policy(
     }
 
     // When enabling the SingleOrg policy, remove this org's members that are members of other orgs
-    if pol_type_enum == OrgPolicyType::SingleOrg && data.enabled {
+    if pol_type_enum == OrgPolicyType::SingleOrg && newly_enabled {
-        for mut member in Membership::find_by_org(&org_id, &conn).await {
+        enforce_single_org_policy_for_org(&org_id, &headers.user.uuid, headers.device.atype, &headers.ip.ip, &conn)
-            // Policy only applies to non-Owner/non-Admin members who have accepted joining the org
+            .await?;
-            // Exclude invited and revoked users when checking for this policy.
-            // Those users will not be allowed to accept or be activated because of the policy checks done there.
-            if member.atype < MembershipType::Admin
-                && member.status != MembershipStatus::Invited as i32
-                && Membership::count_accepted_and_confirmed_by_user(&member.user_uuid, &member.org_uuid, &conn).await
-                    > 0
-            {
-                if CONFIG.mail_enabled() {
-                    let org = Organization::find_by_uuid(&member.org_uuid, &conn).await.unwrap();
-                    let user = User::find_by_uuid(&member.user_uuid, &conn).await.unwrap();
-
-                    mail::send_single_org_removed_from_org(&user.email, &org.name).await?;
-                }
-
-                log_event(
-                    EventType::OrganizationUserRevokedSingleOrganizationNonCompliance,
-                    &member.uuid,
-                    &org_id,
-                    &headers.user.uuid,
-                    headers.device.atype,
-                    &headers.ip.ip,
-                    &conn,
-                )
-                .await;
-
-                member.revoke();
-                member.save(&conn).await?;
-            }
-        }
     }
 
     let mut policy = match current_policy {
@@ -2533,6 +2528,45 @@ async fn put_policy(
     Ok(Json(policy.to_json()))
 }
 
+async fn enforce_single_org_policy_for_org(
+    org_id: &OrganizationId,
+    acting_user_id: &UserId,
+    device_type: i32,
+    ip: &std::net::IpAddr,
+    conn: &DbConn,
+) -> EmptyResult {
+    for mut member in Membership::find_by_org(org_id, conn).await {
+        // Enforcement at activation excludes the actor, Owners, Admins, invited and revoked members.
+        // Later membership activation still checks the enabled policy independently.
+        if member.atype < MembershipType::Admin
+            && member.user_uuid != *acting_user_id
+            && member.status != MembershipStatus::Invited as i32
+            && Membership::count_accepted_and_confirmed_by_user(&member.user_uuid, &member.org_uuid, conn).await > 0
+        {
+            if CONFIG.mail_enabled() {
+                let org = Organization::find_by_uuid(&member.org_uuid, conn).await.unwrap();
+                let user = User::find_by_uuid(&member.user_uuid, conn).await.unwrap();
+                mail::send_single_org_removed_from_org(&user.email, &org.name).await?;
+            }
+
+            log_event(
+                EventType::OrganizationUserRevokedSingleOrganizationNonCompliance,
+                &member.uuid,
+                org_id,
+                acting_user_id,
+                device_type,
+                ip,
+                conn,
+            )
+            .await;
+
+            member.revoke();
+            member.save(conn).await?;
+        }
+    }
+    Ok(())
+}
+
 #[get("/plans")]
 fn get_plans() -> Json<Value> {
     // Respond with a minimal json just enough to allow the creation of an new organization.
@@ -2790,11 +2824,12 @@ async fn restore_member_impl(
 }
 
 /// Whether a member may give others access to collections, and so needs to see the members and groups: by seeing
-/// or creating any collection, by managing the members, or by the manage option on a collection.
+/// or creating any collection, by managing the members or groups, or by the manage option on a collection.
 async fn may_assign_collection_access(member: &Membership, conn: &DbConn) -> bool {
     member.can_view_all_collections()
         || member.has_custom_permission(CustomPermission::CreateNewCollections)
         || member.has_custom_permission(CustomPermission::ManageUsers)
+        || member.has_custom_permission(CustomPermission::ManageGroups)
         || Collection::has_manageable_collection_by_user(&member.org_uuid, &member.user_uuid, conn).await
 }
 
@@ -2817,7 +2852,9 @@ async fn get_groups_data(
             && !headers.membership.has_type(MembershipType::Custom)
             && GroupUser::has_full_access_by_member(&org_id, &headers.membership.uuid, &conn).await);
     let allowed = if details {
-        has_full_access || headers.membership.has_custom_permission(CustomPermission::ManageUsers)
+        has_full_access
+            || headers.membership.has_custom_permission(CustomPermission::ManageUsers)
+            || headers.membership.has_custom_permission(CustomPermission::ManageGroups)
     } else {
         has_full_access || may_assign_collection_access(&headers.membership, &conn).await
     };
@@ -2866,8 +2903,7 @@ async fn get_groups_details(org_id: OrganizationId, headers: ManagerHeadersLoose
 #[serde(rename_all = "camelCase")]
 struct GroupRequest {
     name: String,
-    #[serde(default)]
+    access_all: Option<bool>,
-    access_all: bool,
     external_id: Option<String>,
     collections: Vec<CollectionData>,
     users: Vec<MembershipId>,
@@ -2875,12 +2911,14 @@ struct GroupRequest {
 
 impl GroupRequest {
     pub fn to_group(&self, org_uuid: &OrganizationId) -> Group {
-        Group::new(org_uuid.clone(), self.name.clone(), self.access_all, self.external_id.clone())
+        Group::new(org_uuid.clone(), self.name.clone(), self.access_all.unwrap_or(false), self.external_id.clone())
     }
 
     pub fn update_group(&self, mut group: Group) -> Group {
         group.name.clone_from(&self.name);
-        group.access_all = self.access_all;
+        if let Some(access_all) = self.access_all {
+            group.access_all = access_all;
+        }
         // Group Updates do not support changing the external_id
         // These input fields are in a disabled state, and can only be updated/added via ldap_import
 
@@ -2926,7 +2964,7 @@ impl CollectionData {
 #[post("/organizations/<org_id>/groups", data = "<data>")]
 async fn post_groups(
     org_id: OrganizationId,
-    headers: AdminHeaders,
+    headers: ManageGroupsHeaders,
     data: Json<GroupRequest>,
     conn: DbConn,
 ) -> JsonResult {
@@ -2961,7 +2999,7 @@ async fn put_group(
     org_id: OrganizationId,
     group_id: GroupId,
     data: Json<GroupRequest>,
-    headers: AdminHeaders,
+    headers: ManageGroupsHeaders,
     conn: DbConn,
 ) -> JsonResult {
     if org_id != headers.org_id {
@@ -3002,7 +3040,7 @@ async fn add_update_group(
     collections: Vec<CollectionData>,
     members: Vec<MembershipId>,
     org_id: OrganizationId,
-    headers: &AdminHeaders,
+    headers: &ManageGroupsHeaders,
     conn: &DbConn,
 ) -> JsonResult {
     group.save(conn).await?;
@@ -3041,7 +3079,7 @@ async fn add_update_group(
 async fn get_group_details(
     org_id: OrganizationId,
     group_id: GroupId,
-    headers: AdminHeaders,
+    headers: ManageGroupsHeaders,
     conn: DbConn,
 ) -> JsonResult {
     if org_id != headers.org_id {
@@ -3059,14 +3097,19 @@ async fn get_group_details(
 }
 
 #[delete("/organizations/<org_id>/groups/<group_id>")]
-async fn delete_group(org_id: OrganizationId, group_id: GroupId, headers: AdminHeaders, conn: DbConn) -> EmptyResult {
+async fn delete_group(
+    org_id: OrganizationId,
+    group_id: GroupId,
+    headers: ManageGroupsHeaders,
+    conn: DbConn,
+) -> EmptyResult {
     delete_group_impl(&org_id, &group_id, &headers, &conn).await
 }
 
 async fn delete_group_impl(
     org_id: &OrganizationId,
     group_id: &GroupId,
-    headers: &AdminHeaders,
+    headers: &ManageGroupsHeaders,
     conn: &DbConn,
 ) -> EmptyResult {
     if org_id != &headers.org_id {
@@ -3098,7 +3141,7 @@ async fn delete_group_impl(
 async fn bulk_delete_groups(
     org_id: OrganizationId,
     data: Json<BulkGroupIds>,
-    headers: AdminHeaders,
+    headers: ManageGroupsHeaders,
     conn: DbConn,
 ) -> EmptyResult {
     if org_id != headers.org_id {
@@ -3117,7 +3160,12 @@ async fn bulk_delete_groups(
 }
 
 #[get("/organizations/<org_id>/groups/<group_id>", rank = 2)]
-async fn get_group(org_id: OrganizationId, group_id: GroupId, headers: AdminHeaders, conn: DbConn) -> JsonResult {
+async fn get_group(
+    org_id: OrganizationId,
+    group_id: GroupId,
+    headers: ManageGroupsHeaders,
+    conn: DbConn,
+) -> JsonResult {
     if org_id != headers.org_id {
         err!("Organization not found", "Organization id's do not match");
     }
@@ -3136,7 +3184,7 @@ async fn get_group(org_id: OrganizationId, group_id: GroupId, headers: AdminHead
 async fn get_group_members(
     org_id: OrganizationId,
     group_id: GroupId,
-    headers: AdminHeaders,
+    headers: ManageGroupsHeaders,
     conn: DbConn,
 ) -> JsonResult {
     if org_id != headers.org_id {
@@ -3666,4 +3714,83 @@ mod tests {
             );
         });
     }
+    #[cfg(sqlite)]
+    #[test]
+    fn group_requests_stay_in_their_organization_and_keep_access_all() {
+        test_db::run(async |db| {
+            let conn = db.conn();
+            let (org, _, member) = test_db::custom_member(&conn, CustomPermission::ManageGroups).await;
+            let foreign = test_db::organization(&conn).await;
+            for org_id in [&org.uuid, &foreign.uuid] {
+                let collection = test_db::collection(&conn, org_id).await;
+                let request: GroupRequest = serde_json::from_value(json!({"name":"Group","collections":[
+                    {"id":collection.uuid,"readOnly":false,"hidePasswords":false,"manage":true}],"users":[]}))
+                .unwrap();
+                assert_eq!(request.validate(&org.uuid, &conn).await.is_ok(), org_id == &org.uuid);
+                assert!(!Collection::can_access_collection(&member, &collection.uuid, &conn).await);
+                assert!(!custom_member_may(CollectionAction::Edit, &member, &collection.uuid, &conn).await);
+            }
+            let group = Group::new(org.uuid.clone(), "Group".into(), true, None);
+            let omitted: GroupRequest =
+                serde_json::from_value(json!({"name":"Group","collections":[],"users":[member.uuid]})).unwrap();
+            assert!(omitted.update_group(group).access_all);
+        });
+    }
+
+    #[cfg(sqlite)]
+    #[test]
+    fn enabling_policy_exempts_actor_and_admin_and_revokes_only_on_transition() {
+        test_db::run(async |db| {
+            let conn = db.conn();
+            for policy in [OrgPolicyType::TwoFactorAuthentication, OrgPolicyType::SingleOrg] {
+                let (org, _, actor) = test_db::custom_member(&conn, CustomPermission::ManagePolicies).await;
+                let (_, other) = test_db::member(&conn, &org.uuid, MembershipType::User, &[]).await;
+                let (_, admin) = test_db::member(&conn, &org.uuid, MembershipType::Admin, &[]).await;
+                if policy == OrgPolicyType::SingleOrg {
+                    let foreign = test_db::organization(&conn).await;
+                    for member in [&actor, &other, &admin] {
+                        let mut m = Membership::new(member.user_uuid.clone(), foreign.uuid.clone(), None);
+                        m.status = 2;
+                        m.save(&conn).await.unwrap();
+                    }
+                }
+                for first_enable in [true, false] {
+                    let headers = users_headers(&actor, &conn).await;
+                    let headers = ManagePoliciesHeaders {
+                        device: headers.device,
+                        user: headers.user,
+                        membership: headers.membership,
+                        membership_type: headers.membership_type,
+                        ip: headers.ip,
+                        org_id: headers.org_id,
+                    };
+                    put_policy(
+                        org.uuid.clone(),
+                        policy as i32,
+                        Json(PutPolicy {
+                            policy: PolicyData {
+                                enabled: true,
+                                data: None,
+                            },
+                        }),
+                        headers,
+                        db.conn(),
+                    )
+                    .await
+                    .unwrap();
+                    for member in [&actor, &other, &admin] {
+                        let mut after = Membership::find_by_uuid(&member.uuid, &conn).await.unwrap();
+                        assert_eq!(
+                            after.has_status(MembershipStatus::Confirmed),
+                            !first_enable || member.uuid != other.uuid
+                        );
+                        if first_enable && member.uuid == other.uuid {
+                            assert!(after.restore());
+                            after.save(&conn).await.unwrap();
+                        }
+                    }
+                }
+            }
+        });
+    }
 }

src/api/core/two_factor/mod.rs

@@ -175,8 +175,11 @@ pub async fn enforce_2fa_policy_for_org(
 ) -> EmptyResult {
     let org = Organization::find_by_uuid(org_id, conn).await.unwrap();
     for member in Membership::find_confirmed_by_org(org_id, conn).await {
-        // Don't enforce the policy for Admins and Owners.
+        // Don't revoke Admins, Owners, or the authenticated administrator enabling the policy.
-        if member.atype < MembershipType::Admin && TwoFactor::find_by_user(&member.user_uuid, conn).await.is_empty() {
+        if member.atype < MembershipType::Admin
+            && member.user_uuid != *act_user_id
+            && TwoFactor::find_by_user(&member.user_uuid, conn).await.is_empty()
+        {
             if CONFIG.mail_enabled() {
                 let user = User::find_by_uuid(&member.user_uuid, conn).await.unwrap();
                 mail::send_2fa_removed_from_org(&user.email, &org.name).await?;

src/auth.rs

@@ -887,6 +887,7 @@ impl<'r> FromRequest<'r> for AdminHeaders {
 macro_rules! permission_headers {
     ($(#[$doc:meta])* $name:ident, $permission:expr) => {
         $(#[$doc])*
+        #[allow(dead_code, reason = "Not every route reads every field")]
         pub struct $name {
             pub device: Device,
             pub user: User,
@@ -926,6 +927,16 @@ permission_headers!(
     ManageUsersHeaders,
     CustomPermission::ManageUsers
 );
+permission_headers!(
+    /// For the routes that manage the policies of an organization.
+    ManagePoliciesHeaders,
+    CustomPermission::ManagePolicies
+);
+permission_headers!(
+    /// For the routes that manage the groups of an organization.
+    ManageGroupsHeaders,
+    CustomPermission::ManageGroups
+);
 
 // 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.

src/db/models/organization.rs

@@ -248,6 +248,8 @@ pub enum CustomPermission {
 /// grant of any other one grants nothing. They are implemented one at a time, so this grows with them.
 const SUPPORTED_CUSTOM_PERMISSIONS: &[CustomPermission] = &[
     CustomPermission::ManageUsers,
+    CustomPermission::ManageGroups,
+    CustomPermission::ManagePolicies,
     CustomPermission::CreateNewCollections,
     CustomPermission::EditAnyCollection,
     CustomPermission::DeleteAnyCollection,
@@ -1612,7 +1614,7 @@ mod tests {
         }
         assert_eq!(
             SUPPORTED_CUSTOM_PERMISSIONS,
-            [ManageUsers, CreateNewCollections, EditAnyCollection, DeleteAnyCollection]
+            [ManageUsers, ManageGroups, ManagePolicies, CreateNewCollections, EditAnyCollection, DeleteAnyCollection]
         );
     }
 

src/static/templates/scss/vaultwarden.scss.hbs

@@ -99,10 +99,8 @@ app-security > app-two-factor-setup > form {
 
 /* Hide unsupported Custom Role options */
 {{#if custom_roles_enabled}}
-/* Only the collection permissions and Manage users can be granted yet */
+/* Only the collection permissions, Manage users, Manage groups and Manage policies can be granted yet */
 :is(bit-dialog, [bit-dialog]) div.tw-col-span-4:has(input[formcontrolname*="access"]),
-:is(bit-dialog, [bit-dialog]) bit-form-control:has(input[formcontrolname="manageGroups"]),
-:is(bit-dialog, [bit-dialog]) bit-form-control:has(input[formcontrolname="managePolicies"]),
 :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;