Add manage role for collections and groups

[BlackDex]

This commit will add the manage role/column to collections and groups. We need this to allow users part of a collection either directly or via groups to be able to delete ciphers. Without this, they are only able to either edit or view them when using new clients, since these check the manage role.

Still trying to keep it compatible with previous versions and able to revert to an older Vaultwarden version and the access_all feature of the older installations. In a future version we should really check and fix these rights and create some kind of migration step to also remove the access_all feature and convert that to a manage option. But this commit at least creates the base for this already.

This should resolve #5367

[BlackDex]

I think i have covered all the items here to make it work. Managers without access_all and users who are allowed to delete items from collections will need the new manage role, only edit will not work.

If some daredevil users want to tryout this PR, please do so and report issues.

[tessus]

I've run it on a test VM and it seems fine (when using it with roles for collections and groups). But I have missed certain actions before.

It would be a huge help, if there was a list of steps to test. e.g. I am not using groups on my primary vw intance, so it's easy to miss something in that area. I am also not using a lot of collections. (I think I have one or two so far.) Otherwise I would have already created such a list.

Maybe someone who is using those features extensively can compile a list.

P.S.: Or maybe there is a way to test this via the API. In that case a pipeline could be created. Beats manual testing. ;-)

[stefan0xC]

Well, I don't have a list yet because I'm kinda waiting on the SSO PR. Mainly because it also contains some playwright tests. Once they have been added I was planning to add even more tests (e.g. basic item management, folder management, organization creation, membership management, including collections and groups management ...) to ensure compatibility with the web-vault.

On my local test instance I have multiple users with various roles and organizations of various sizes. I mean one has a bunch of collections with items that are named accordingly (from the User's permission not the Owner's):


So that's something I can normally just test by logging into the User account and check if everything is editable/viewable according to their given permissions.

Some things are also rather obscure things to look out for. E.g. testing whether you can invite 15 users at once does not necessarily mean that there is not also a check at exactly 10 users that forbids you from inviting more. (We have got rid of a bunch of issues like #4770 by just changing the product tier so at least that example should not happen anymore but it would still be great if we tested for stuff like that to prevent regressions.)

[BlackDex]

Ok, there was an issue where if a admin or owner did not had the access_all flag set, they would not see any collections at all anymore.

I the latest commit i have fixed this (i think) but if other people can check and verify, that would be cool :)

[stefan0xC]

I thought that this was partly intentional? Cf. https://bitwarden.com/help/collection-management/#collection-management-settings

And I have not yet checked what difference this would make but we could maybe change

vaultwarden/src/api/core/organizations.rs

Line 356 in cc09bb7

let has_full_access_to_org = member.access_all

to member.has_full_access() instead?

[BlackDex]

Well, we need to overhaul the whole access_all stuff anyway. The new database structure of Bitwarden does not have that column anymore.

https://github.com/bitwarden/server/blob/9efcbec041902a64c366e539cbdf3dabc9b62958/util/SqliteMigrations/Migrations/20240924010540_DropOrganizationUserAccessAll.Designer.cs

[tessus]

To be honest, the question is if you really want to keep backwards compatibility so that people can revert to an older vw version.
On one side it is nice to have, but if it hinders development of the real solution, it's just in the way of progress.

[BlackDex]

My only intention is to have it for now with the new UI etc..
Mainly to be able to revert back a bit more easily then having to jump through hoops or write migration scripts or code.

Having this in already lays a base for removing the access_all and ironing out all the wrinkles.

[tessus]

I only meant that if you had to jump through hoops to make it work right now (still needing many, many commits), then it would make sense to maybe not do it (jump through hoops).

But if everything is already done for this PR so that it works with the new UI, my comment is moot.

[BlackDex]

I think it works right now. But since it is complex in the sense of many combinations possible, some extra checks are always nice.