[Bug] Group Permissions Collection Deletion Issue (Manager role)

[jb2barrels]

Subject of the issue

When a user is in the manager role and has permissions to collections via the beta feature groups, they will be unable to delete collections from the password entry. Initially it will look like the change succeeded, but if you reload the page you will notice the collection is checked again.

Deployment environment

• vaultwarden version: Tested on both 1.30.5 and testing docker branches

• Install method: Docker

• Clients used: Web Vault and Bitwarden Extension (Primarily tested the bug using Web Vault)

• Reverse proxy and version: Nginx Proxy Manager

• MySQL/MariaDB or PostgreSQL version: PostgreSQL

• Other relevant details:

Steps to reproduce

To reproduce:

• Enable groups beta feature
• Create new test user and give it role: Manager
• Create two new vaults 'TESTING1' and 'TESTING2'
• Create a group named 'TESTING' and assign the above two collections to that group.
• Assign that group to the new test user you created that has manager role.
• Login to webvault and create a password entry owned by the organization assigned to collections: TESTING1 and TESTING2
• Reload page
• Now attempt to modify the test entry and remove one of the collections and save
• Reload page
• You will see the collection permission delete did not retain.

NOTE: If you add the test user to the groups directly, this bug goes away. This only is reproducible if the collection the user has access to was granted via groups.

Expected behavior

Collection should be revoked from the password entry.

Actual behavior

Collection is not revoked from the password entry.

[jb2barrels]

I would like to add an additional note: Promoting the user to Admin removes this problem. So it seems isolated to the Manager role.

[jb2barrels]

I see according to the bitwarden.com documentation, that the Manager role will no longer exist. So I wonder if the issue above is related to the new web vault changes.

https://bitwarden.com/help/user-types-access-control/

Snippet:

Starting on March 3, 2024, organizations that haven't turned on collection management will begin to be migrated in batches to an updated permissions structure. If not migrated yet, your organization will be within the next few weeks or if you manually turn on collection management.
During migration, all Managers are migrated to members with the User role and automatically provided with a new Can manage permission over assigned collections. They will retain the ability to fully manage those collections, including the ability to assign new members or groups access.

[BlackDex]

The issue has nothing to do with the manager role, as that currently is under a feature flag and not enabled for Vaultwarden.
The old way still works for both Self Hosted and Bitwarden Cloud unless you are migrated already or opt-in for it your self.

We would have to check what the reason here is, or maybe fix the manager to user role right away.

[stefan0xC]

As far as I've looked into it, the removal will be ignored because Cipher::get_collections() does not have support for collections via groups, so it won't be part of the symmetric difference (if I understand the logic correctly).

vaultwarden/src/api/core/ciphers.rs

Lines 766 to 770 in f05398a

	let posted_collections: HashSet<String> = data.CollectionIds.iter().cloned().collect();
	let current_collections: HashSet<String> =
	cipher.get_collections(headers.user.uuid.clone(), &mut conn).await.iter().cloned().collect();
	for collection in posted_collections.symmetric_difference(&current_collections) {

I've not looked more into why there is no immediate visual indication of this change. (If it works with the bitwarden/server the web-vault might expect the function to return something instead of an EmptyResult?)

[jb2barrels]

Did some additional testing incase it helps. It looks like the extension (2024.4.2) is affected as well.

People with the User or manager rank will be able to see that they can uncheck a collection from a password entry using the Bitwarden extension. On the latest web vault version in testing, you will see those collections are greyed out and unable to be modified.
Upon saving the change via the extension, it'll show as change was updated. Although upon the next sync to server, the change is undone on the client side. So anything before the sync, gives a false sense of successful change.

[stefan0xC]

While trying to fix the issue I've run into couple of scenarios where it would either not work for Admins/Owners (deleting the collections which aren't visible for an Admin/Owner in the Password Manager, or not being able to delete collections even though they have been unselected) or there would still be a problem with Managers because for them it's also important whether or not they have write access to a collection (because read-only collections will still be visible/selectable in the Admin Console but hidden in the Password Manager). Also updating the collections in the Admin Console sometimes caused an exception for Managers or for Admins/Owners by switching to the Admin Console.

Testing the different cases is a bit tedious because I need to use at least two different accounts (with Manager and Admin or Owner role) assigned to different collections and not using the access_all permission.

So I understand why Bitwarden decided to get rid of the current approach and use their more flexible permission system which you can probably check for more easily. (E.g. only get those collections where you have the Can manage permission unless you are admin/owner, where you will always have access to all collections irregardless.)

But now I'm wondering if we should not get rid of the manager role and the access all flag as well and implement custom permissions instead.

[jb2barrels]

get rid of the manager role and the access all flag as well and implement custom permissions instead.

Preferably it'd probably be best to go forward with the custom permissions, if that's the way Bitwarden has already went.
Based on discussions from above, I assume this would mean a custom roles feature flag would need to be activated as a pre-requisite to using groups?

[BlackDex]

It also needs flexible collections then and a migration code for it.

[stefan0xC]

I am still trying to fix the bug with the current approach, so not sure if it is required right now. But eventually we probably want/have to implement flexible collections and migrate to the new approach. 🙈

[stefanpflug]

So I understand why Bitwarden decided to get rid of the current approach and use

You're probably right.

Over the last few days, we have been playing around with groups and collections (although most of the problems are likely to occur even without groups).
The goal was to create different teams within an organization and give them the possibility to share individual elements on demand. This should primarily be done via separate collections (e.g. "Share_GroupA_to_GroupB").

Group A shares something with group B. Group B can share it with group C without group A noticing.
If group A removes the sharing with group B, group C will no longer see anything (usually).

Group A shares something with group B by reading it.
The element is read-only for all users in group B :-)
The manager in group B can extend the right of group B to the collection to "Edit" and all users have full access :-(

As you have already noticed: If a group has read-only access to collections, the webui sometimes allows editing of functions that are actually not permitted. Sometimes this is acknowledged with an error message (...cipher....), sometimes the dialog simply closes and the setting has not been changed.

In my opinion, the balancing act is that a role has priority over the collection rights and at the same time the same users should be restricted via rights to the collections.

P.S. Thank you stefan0XC for the quick fix to the problem with the Directory Connector. I just got around to compiling it and will try to test it tomorrow.