Repository navigation
feat(common-contacts): groups on the organization instance - #4976
Conversation
25fffaa to
0a27e1f
Compare
There was a problem hiding this comment.
One path was missed though: MembersFromContacts (model/sharing/member.go L213-L225) still calls contact.FindGroup(inst, id) on the member instance, which has no groups db anymore.
So creating a nested shared drive with a B2B group (POST /sharings/drives/:id/:file-id/drives) returns 404 "Database does not exist". I hit it locally.
Using findGroup + GetAllContacts + isSelf there, like AddGroup does, should be enough. Worth adding the case to TestAddGroupFromOrgInstance too.
| // With the feed, the groups live on the organization instance only. | ||
| if inst.HasCommonContacts() && !inst.IsOrganizationInstance() { | ||
| continue | ||
| } |
There was a problem hiding this comment.
member.removed and group.deleted now only run on the org instance, so the share-group trigger never fires where alice's group sharing actually lives.
Result: a removed member keeps access. Locally carol was still in alice's sharings and shared drive after being removed, and after the group was deleted. Renames are lost too.
Is that accepted until ADR 052? Or should these two events keep running on member instances for now?
There was a problem hiding this comment.
Accepted for now, common_contacts isn't enabled anywhere yet. Keeping the events on member instances wouldn't help: there's no group or copy left there for share-group to fire on. It needs the fan-out from the org instance (open point of #4917) before we turn the flag on.
| setContactDirectoryMetadata(&c.JSONDoc, input, input.Email) | ||
| } else { | ||
| applyManagedContactFields(c, input) | ||
| } |
There was a problem hiding this comment.
In this branch, could you set cozy from input.CozyURL when the contact has none? applyCard already does that.
Otherwise the group member gets a mail invite instead of a cozy to cozy one.
The test at groups_test.go L375 asserts the empty URL right now, so it would need to change.
| } | ||
| group, err := contact.FindGroup(inst, groupID) | ||
| return group, inst, err | ||
| } |
There was a problem hiding this comment.
This reads any group on the org instance, in any context.
A non B2B group an admin creates there can be shared by a member who can't even read it, and that exposes its members. I reproduced that one.
Contexts without common_contacts also start reading the org instance here.
I'd gate it on inst.HasCommonContacts() and IsManagedDirectoryGroupID, and check IsManagedDirectoryDoc on what comes back.
| } | ||
| email, _ := inst.SettingsEMail() | ||
| return m.Email != "" && strings.EqualFold(m.Email, email) | ||
| } |
There was a problem hiding this comment.
isSelf reads the settings doc once per member. I counted 50 GETs for a 20 member group.
Reading SettingsEMail and PageURL once before the loop would fix it.
6ffcc94 to
0ab8429
Compare
7337a6e to
8fc77f2
Compare
6b35180 to
e81976f
Compare
Nested drives and the delegated add now resolve groups like AddGroup, and the settings of the instance are read once per group.
e81976f to
d6bfe5e
Compare
Summary
centralized_org_directoryto write B2B groups only on the organization instance.common_contactsindependently controls contacts-service integration.common_contactsis enabled; resume B2B email/name updates when it is disabled. Keep the contact ID and group relationships, and fill a missing Cozy URL from B2B data.centralized_org_directory: falsefor the initial backend rollout so the existing sharing UI retains its local contacts and groups.Part of linagora/twake-workplace-private#1780.