Group support

[MFijak]

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.

[BlackDex]

@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.

[MFijak]

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?

[BlackDex]

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.

[GeekCornerGH]

Looks neat, the thing you can do however is a rebase I assume

[dani-garcia]

So far it looks pretty good to me! I've noticed a couple of small things, but for a rust newbie this is solid work.

I haven't had time to fully review the query logic, but in a few small tests it has worked correctly so far.

[omueller]

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!

[BlackDex]

@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.

[BlackDex]

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.

[dani-garcia]

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 👍

[MFijak]

Closed as superseded by #2846