fix(sharing): preserve group removals on conflicts

Author: shepilov

model/sharing/group.go

@@ -90,19 +90,9 @@ func (s *Sharing) RevokeGroup(inst *instance.Instance, index int) error {
 		if !inGroup {
 			continue
 		}
-		if len(m.Groups) == 1 {
-			s.Members[i].Groups = nil
-		} else {
-			var groups []int
-			for _, idx := range m.Groups {
-				if idx != index {
-					groups = append(groups, idx)
-				}
-			}
-			s.Members[i].Groups = groups
-		}
+		removeGroupFromMember(&s.Members[i], index)
 		if m.OnlyInGroups && len(s.Members[i].Groups) == 0 {
-			if err := s.RevokeRecipient(inst, i); err != nil {
+			if err := s.revokeRecipientAfterRemovingGroup(inst, i, index); err != nil {
 				errm = multierror.Append(errm, err)
 			}
 		}
@@ -304,6 +294,16 @@ func addGroupToMember(member *Member, groupIndex int) bool {
 	return true
 }
 
+func removeGroupFromMember(member *Member, groupIndex int) {
+	var groups []int
+	for _, index := range member.Groups {
+		if index != groupIndex {
+			groups = append(groups, index)
+		}
+	}
+	member.Groups = groups
+}
+
 func (s *Sharing) contactIsMemberOfGroup(groupIndex int, c *contact.Contact) bool {
 	var email string
 	if addr, err := c.ToMailAddress(); err == nil {
@@ -354,16 +354,10 @@ func (s *Sharing) RemoveMemberFromGroup(inst *instance.Instance, groupIndex int,
 			continue
 		}
 
-		var groups []int
-		for _, idx := range m.Groups {
-			if idx != groupIndex {
-				groups = append(groups, idx)
-			}
-		}
-		s.Members[i].Groups = groups
+		removeGroupFromMember(&s.Members[i], groupIndex)
 
 		if m.OnlyInGroups && len(s.Members[i].Groups) == 0 {
-			return s.RevokeRecipient(inst, i)
+			return s.revokeRecipientAfterRemovingGroup(inst, i, groupIndex)
 		} else {
 			return couchdb.UpdateDoc(inst, s)
 		}
@@ -425,16 +419,10 @@ func (s *Sharing) SendRemoveMemberFromGroup(inst *instance.Instance, groupIndex,
 }
 
 func (s *Sharing) DelegatedRemoveMemberFromGroup(inst *instance.Instance, groupIndex, memberIndex int) error {
-	var groups []int
-	for _, idx := range s.Members[memberIndex].Groups {
-		if idx != groupIndex {
-			groups = append(groups, idx)
-		}
-	}
-	s.Members[memberIndex].Groups = groups
+	removeGroupFromMember(&s.Members[memberIndex], groupIndex)
 
 	if s.Members[memberIndex].OnlyInGroups && len(s.Members[memberIndex].Groups) == 0 {
-		return s.RevokeRecipient(inst, memberIndex)
+		return s.revokeRecipientAfterRemovingGroup(inst, memberIndex, groupIndex)
 	} else {
 		return couchdb.UpdateDoc(inst, s)
 	}

model/sharing/group_test.go

@@ -360,6 +360,33 @@ func TestGroups(t *testing.T) {
 		assert.False(t, s.Groups[0].Revoked)
 	})
 
+	t.Run("RemoveMemberFromGroupPreservesRemovalAfterConflict", func(t *testing.T) {
+		team := createGroup(t, inst, "Conflicting Drive Team")
+		otherTeam := createGroup(t, inst, "Remaining Drive Team")
+		alice := createContactInGroups(t, inst, "ConflictAlice", []string{team.ID()})
+
+		s := createDriveSharingForGroupTest(t, inst, "Conflicting group removal")
+		s.OrgDrive = true
+		sid := s.SID
+		require.NoError(t, s.AddGroup(inst, team.ID(), false))
+		require.NoError(t, s.AddGroup(inst, otherTeam.ID(), false))
+		require.NoError(t, couchdb.UpdateDoc(inst, s))
+
+		concurrent := &Sharing{}
+		require.NoError(t, couchdb.GetDoc(inst, consts.Sharings, sid, concurrent))
+		concurrent.Description = "Updated concurrently"
+		require.NoError(t, couchdb.UpdateDoc(inst, concurrent))
+
+		require.NoError(t, s.RemoveMemberFromGroup(inst, 0, alice))
+
+		stored := &Sharing{}
+		require.NoError(t, couchdb.GetDoc(inst, consts.Sharings, sid, stored))
+		require.Len(t, stored.Members, 2)
+		assert.Equal(t, MemberStatusRevoked, stored.Members[1].Status)
+		assert.Empty(t, stored.Members[1].Groups)
+		assert.Equal(t, "Updated concurrently", stored.Description)
+	})
+
 	t.Run("RevokeLastDriveGroupDeletesSharing", func(t *testing.T) {
 		team := createGroup(t, inst, "Drive Group")
 		_ = createContactInGroups(t, inst, "DriveGroupAlice", []string{team.ID()})

model/sharing/member.go

@@ -1188,6 +1188,10 @@ func (s *Sharing) DelegateRevokeRecipient(inst *instance.Instance, index int) er
 
 // RevokeMember revoke the access granted to a member and contact it
 func (s *Sharing) RevokeMember(inst *instance.Instance, index int) error {
+	return s.revokeMember(inst, index, nil)
+}
+
+func (s *Sharing) revokeMember(inst *instance.Instance, index int, removedGroupIndex *int) error {
 	m := &s.Members[index]
 
 	// skip if member is already revoked
@@ -1220,6 +1224,9 @@ func (s *Sharing) RevokeMember(inst *instance.Instance, index int) error {
 	// operation several times.
 	leftRetries := 3
 	for {
+		if removedGroupIndex != nil {
+			removeGroupFromMember(m, *removedGroupIndex)
+		}
 		m.Status = MemberStatusRevoked
 		// Do not remove the credentials from the array to preserve the members /
 		// credentials order, just empty them

model/sharing/sharing.go

@@ -758,10 +758,18 @@ func (s *Sharing) RemoveInteractPermissionsForAMember(inst *instance.Instance, m
 // sharing has still at least one active member, we keep it as is. Else, we
 // disable the sharing.
 func (s *Sharing) RevokeRecipient(inst *instance.Instance, index int) error {
+	return s.revokeRecipient(inst, index, nil)
+}
+
+func (s *Sharing) revokeRecipientAfterRemovingGroup(inst *instance.Instance, index, groupIndex int) error {
+	return s.revokeRecipient(inst, index, &groupIndex)
+}
+
+func (s *Sharing) revokeRecipient(inst *instance.Instance, index int, removedGroupIndex *int) error {
 	if !s.Owner {
 		return ErrInvalidSharing
 	}
-	if err := s.RevokeMember(inst, index); err != nil {
+	if err := s.revokeMember(inst, index, removedGroupIndex); err != nil {
 		return err
 	}
 	m := &s.Members[index]