Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

stefan0xC
Copy link
Contributor

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
Copy link
Collaborator

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

@stefan0xC stefan0xC marked this pull request as draft May 26, 2024 11:13
@stefan0xC
Copy link
Contributor Author

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

@BlackDex
Copy link
Collaborator

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 stefan0xC force-pushed the fix-collection-permission-issue branch 3 times, most recently from 2bcdc60 to e6ef5ea Compare May 27, 2024 17:40
@stefan0xC
Copy link
Contributor Author

stefan0xC commented May 27, 2024

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

@stefan0xC stefan0xC marked this pull request as ready for review May 27, 2024 19:17
@stefan0xC
Copy link
Contributor Author

stefan0xC commented May 27, 2024

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 😳 ).

@stefan0xC stefan0xC force-pushed the fix-collection-permission-issue branch 2 times, most recently from e78b002 to 46273af Compare May 27, 2024 20:04
@jb2barrels
Copy link

@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
Copy link
Contributor Author

stefan0xC commented Jun 5, 2024

@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
Copy link

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
Copy link
Contributor Author

stefan0xC commented Jun 6, 2024

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 stefan0xC force-pushed the fix-collection-permission-issue branch 2 times, most recently from a395361 to 6ff5f41 Compare June 23, 2024 20:52
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
prevent side effects if groups are disabled
@stefan0xC stefan0xC force-pushed the fix-collection-permission-issue branch from 6ff5f41 to 9af6f86 Compare June 24, 2024 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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