vaultwarden
vaultwarden copied to clipboard
dani-garcia
Group support
Hello all, This is my first ever pull request and I also never used Rust before. Please tell me if I did anything wrong. I implemented the group support, which is related to issue #1623.
To test my changes following sites can be used:
- Organizations->Manage->People
- Groups can be assigned to people (hover over a member and click the gear symbol)
- Delete a member
- Organizations->Manage->Collections
- Collections can be assigned to groups (click on a collection)
- Delete a collection
- Add a collection
- Organizations->Manage->Groups
- Create a group (top right “+ New Group”)
- Modify a group (click on a group)
- Delete a group (click on a group)
- Users can be added to groups (hover over a group and click the gear symbol)
- Vaults
- Should list any collections which are visible by the user
There was one code segment I was not so sure about if my implementation is the correct approach:
pub async fn find_by_user_uuid(user_uuid: &str, conn: &DbConn)
Db/models/collection.rs
This function basically returns all collections which the user has access to. I squeezed my group implementation into the query builder. Maybe a better approach would be to fire two independent queries. Each for user collections and group collections. I think this would be the better approach, because you can much clearer see what the query does. But I would assume there comes a small performance penalty.
@MFijak Thanks for the contribution. I have quickly scanned the PR and added some remarks. There may be more items i need to check. But this is a nice start for someone's first Rust code.
I'm not sure when i have some time to check the rest of the code, and how it works etc.. But if you have any questions please leave a comment here, or come and join on our Matrix channel.
I am glad to hear that! No worries take your time.
Just a small question. You wrote that you added some remarks. Are these visible to me? I clicked through the tabs but somehow I can not see any remarks. I probably misunderstood, because you wrote them down locally on your computer?
I am glad to hear that! No worries take your time.
Just a small question. You wrote that you added some remarks. Are these visible to me? I clicked through the tabs but somehow I can not see any remarks. I probably misunderstood, because you wrote them down locally on your computer?
No, i wrote them on Github. But i everytime forget that i need to Start a review hehe.
They should be visible now.
Oh wow, it seems I missed this really promising PR, thanks a lot for your work and time @MFijak (and everybody involved since of course!), once production-ready & merged it will make a lot of people happy ! Sticking around and ready to test this on our small family-wide setup :) Thanks & cheers!
@MFijak i did some further checking, and indeed the Group::is_in_full_access_group() function is returning true for all ciphers even if they are not part the of the organization to which they belong. So that function needs a org_uuid filter also.
And using the check where the user_groups is HashMap<String, Vec<Group>> works great with a function Group::find_by_user_all_access(). That will save queries, and keeps the speed into parsing each ciphers access for the user.
Also, in general, I would like this whole PR to be squashed into one commit instead of the 23 commits currently, that would also make the merge/commit nicer to check in the future 😄. But that is maybe something for right before we are going to merge it.
This is ready for merging for me, if you can rebase it against the latest main and squash it into 1-5 commits, that would be perfect, otherwise I'll see if I can do it myself.
We've had a release last week which means if we merge this soon it's gonna have some time for user testing before the next release.
Nice job 👍
