search

dani-garcia / vaultwarden

Unofficial Bitwarden compatible server written in Rust, formerly known as bitwarden_rs
GNU Affero General Public License v3.0
37.31k stars 1.82k forks source link

add group support for Cipher::get_collections() #4592

Closed stefan0xC closed 3 months ago

stefan0xC commented 4 months ago

join group infos assigned to a collection to check whether user has been given access to all collections via any group or they have access to a specific collection via any group membership

fixes #4588

BlackDex commented 4 months ago

omg, those queries aren't getting any prettier haha.

stefan0xC commented 4 months ago

I've tested it a bit with some edge cases and I don't think this currently works correctly.

BlackDex commented 4 months ago

I actually think we should redesign the whole group database storage. Currently we use the exact same logic to store it as Bitwarden. But Bitwarden has some programmable logic which we do not. Not sure actually how they solved this with there SQLite implementation.

stefan0xC commented 4 months ago

To fix the issues I've encountered I had to add even more queries. :sweat_smile: (I'll take a look at the expected return values next.)

stefan0xC commented 4 months ago

As far as I've tested it, this should now work as intended but I've not tested every possible combination of access rights and collection assignments so I might have missed something (hopefully not something obvious :flushed: ).

jb2barrels commented 4 months ago

@stefan0xC Question for you. Does this change also fix the Bitwarden browser extension from showing that a password entry collection can be revoked from an entry as a user (when the user shouldn't have that permission) ?

stefan0xC commented 3 months ago

@jb2barrels Not sure if this is related. Is this a visual bug (where it only seems possible but it actually isn't) or can entries actually be changed without write access to a collection?

jb2barrels commented 3 months ago

The extension will show the change was made to the password entries' collections. But upon the next sync made by the extension, the changes are reverted client side (eg the change never actually happens on the server).

I'm wondering if it has anything to do with the client receiving a response (or empty response) thinking its successful, when the user did not have permission.

stefan0xC commented 3 months ago

I'm not sure I can replicate your issue. Even without my changes I get an error (Cipher is not write accessible) when I try to change the assigned collections of a cipher in the browser extension when I don't actually have write permission to the collection.

stefan0xC commented 3 months ago

I've added the missing endpoint for #4681- not sure if something else has changed or is needed. Please test and review (because I've only tested with web-v2024.6.2 https://github.com/dani-garcia/bw_web_builds/pull/169 and not browser extension or Android beta).

BlackDex commented 3 months ago

I actually do not think it stops with only the v2 endpoint. The flexible collections also cause other stuff within the clients to work differently and we are not having those checks in place i think.

Best would be if we turn of flexible collections for now, and work on this a.s.a.p.

stefan0xC commented 3 months ago

Turning off "flexibleCollections" will have no effect on the called endpoint as far as I have tested it (clients like web-v2024.6.2 seem to use collections_v2exclusively).

BlackDex commented 3 months ago

Ah, ok. But still, we should set it to false then. I have not checked any further my self.