Handle remove event out of order in private group chats

When we received a remove event from a private group chat out of order,
the chat would not be created.
This was causing some issues if later on we received the previous event.
This commit changes the behavior so that a chat is created.

protocol/messenger_group_chat.go

@@ -2,6 +2,7 @@ package protocol
 
 import (
 	"context"
+	"crypto/ecdsa"
 	"encoding/hex"
 	"errors"
 
@@ -124,15 +125,16 @@ func (m *Messenger) CreateGroupChatFromInvitation(name string, chatID string, ad
 	return &response, m.saveChat(&chat)
 }
 
-func (m *Messenger) RemoveMembersFromGroupChat(ctx context.Context, chatID string, members []string) (*MessengerResponse, error) {
-	var response MessengerResponse
+type removeMembersFromGroupChatResponse struct {
+	oldRecipients   []*ecdsa.PublicKey
+	group           *v1protocol.Group
+	encodedProtobuf []byte
+}
+
+func (m *Messenger) removeMembersFromGroupChat(ctx context.Context, chat *Chat, members []string) (*removeMembersFromGroupChatResponse, error) {
+	chatID := chat.ID
 	logger := m.logger.With(zap.String("site", "RemoveMembersFromGroupChat"))
 	logger.Info("Removing members form group chat", zap.String("chatID", chatID), zap.Any("members", members))
-	chat, ok := m.allChats.Load(chatID)
-	if !ok {
-		return nil, ErrChatNotFound
-	}
-
 	group, err := newProtocolGroupFromChat(chat)
 	if err != nil {
 		return nil, err
@@ -162,21 +164,42 @@ func (m *Messenger) RemoveMembersFromGroupChat(ctx context.Context, chatID strin
 		}
 	}
 
-	encodedMessage, err := m.sender.EncodeMembershipUpdate(group, nil)
+	encoded, err := m.sender.EncodeMembershipUpdate(group, nil)
 	if err != nil {
 		return nil, err
 	}
+
+	return &removeMembersFromGroupChatResponse{
+		oldRecipients:   oldRecipients,
+		group:           group,
+		encodedProtobuf: encoded,
+	}, nil
+}
+
+func (m *Messenger) RemoveMembersFromGroupChat(ctx context.Context, chatID string, members []string) (*MessengerResponse, error) {
+	var response MessengerResponse
+
+	chat, ok := m.allChats.Load(chatID)
+	if !ok {
+		return nil, ErrChatNotFound
+	}
+
+	removeMembersResponse, err := m.removeMembersFromGroupChat(ctx, chat, members)
+	if err != nil {
+		return nil, err
+	}
+
 	_, err = m.dispatchMessage(ctx, common.RawMessage{
 		LocalChatID: chat.ID,
-		Payload:     encodedMessage,
+		Payload:     removeMembersResponse.encodedProtobuf,
 		MessageType: protobuf.ApplicationMetadataMessage_MEMBERSHIP_UPDATE_MESSAGE,
-		Recipients:  oldRecipients,
+		Recipients:  removeMembersResponse.oldRecipients,
 	})
 	if err != nil {
 		return nil, err
 	}
 
-	chat.updateChatFromGroupMembershipChanges(group)
+	chat.updateChatFromGroupMembershipChanges(removeMembersResponse.group)
 
 	return m.addMessagesAndChat(chat, buildSystemMessages(chat.MembershipUpdates, m.systemMessagesTranslations), &response)
 }

protocol/messenger_group_chat_test.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/golang/protobuf/proto"
 	"github.com/stretchr/testify/suite"
 
 	userimage "github.com/status-im/status-go/images"
@@ -409,3 +410,47 @@ func (s *MessengerGroupChatSuite) TestGroupChatHandleDeleteMemberMessage() {
 	defer s.NoError(admin.Shutdown())
 	defer s.NoError(member.Shutdown())
 }
+
+func (s *MessengerGroupChatSuite) TestGroupChatMembersRemovalOutOfOrder() {
+	admin := s.startNewMessenger()
+	memberA := s.startNewMessenger()
+	members := []string{common.PubkeyToHex(&memberA.identity.PublicKey)}
+
+	s.makeMutualContacts(admin, memberA)
+
+	groupChat := s.createGroupChat(admin, "test_group_chat", members)
+
+	removeMembersResponse, err := admin.removeMembersFromGroupChat(context.Background(), groupChat, []string{common.PubkeyToHex(&memberA.identity.PublicKey)})
+	s.Require().NoError(err)
+
+	encodedMessage := removeMembersResponse.encodedProtobuf
+
+	message := protobuf.MembershipUpdateMessage{}
+	err = proto.Unmarshal(encodedMessage, &message)
+	s.Require().NoError(err)
+
+	response := &MessengerResponse{}
+
+	messageState := &ReceivedMessageState{
+		ExistingMessagesMap: make(map[string]bool),
+		Response:            response,
+		AllChats:            new(chatMap),
+		Timesource:          memberA.getTimesource(),
+	}
+
+	c, err := buildContact(admin.myHexIdentity(), &admin.identity.PublicKey)
+	s.Require().NoError(err)
+
+	messageState.CurrentMessageState = &CurrentMessageState{
+		Contact: c,
+	}
+
+	err = memberA.HandleMembershipUpdate(messageState, nil, &message, memberA.systemMessagesTranslations)
+
+	s.Require().NoError(err)
+	s.Require().NotNil(messageState.Response)
+	s.Require().Len(messageState.Response.Chats(), 1)
+	s.Require().Len(messageState.Response.Chats()[0].Members, 1)
+	defer s.NoError(admin.Shutdown())
+	defer s.NoError(memberA.Shutdown())
+}

protocol/messenger_handler.go

@@ -138,12 +138,17 @@ func (m *Messenger) HandleMembershipUpdate(messageState *ReceivedMessageState, c
 			return err
 		}
 
-		// A new chat must contain us
-		if !group.IsMember(ourKey) {
+		// A new chat must have contained us at some point
+		wasEverMember, err := group.WasEverMember(ourKey)
+		if err != nil {
+			return err
+		}
+
+		if !wasEverMember {
 			return errors.New("can't create a new group chat without us being a member")
 		}
-		// A new chat always adds us
-		wasUserAdded = true
+
+		wasUserAdded = group.IsMember(ourKey)
 		newChat := CreateGroupChat(messageState.Timesource)
 		// We set group chat inactive and create a notification instead
 		// unless is coming from us or a contact or were waiting for approval.

protocol/v1/membership_update_message.go

@@ -490,6 +490,15 @@ func (g Group) Creator() (string, error) {
 	return first.From, nil
 }
 
+func (g Group) isCreator(id string) (bool, error) {
+	c, err := g.Creator()
+	if err != nil {
+		return false, err
+	}
+
+	return id == c, nil
+}
+
 func (g Group) validateChatID(chatID string) bool {
 	creator, err := g.Creator()
 	if err != nil || creator == "" {
@@ -504,6 +513,28 @@ func (g Group) IsMember(id string) bool {
 	return g.members.Has(id)
 }
 
+func (g Group) WasEverMember(id string) (bool, error) {
+	isCreator, err := g.isCreator(id)
+	if err != nil {
+		return false, err
+	}
+
+	if isCreator {
+		return true, nil
+	}
+
+	for _, event := range g.events {
+		if event.Type == protobuf.MembershipUpdateEvent_MEMBERS_ADDED {
+			for _, member := range event.Members {
+				if member == id {
+					return true, nil
+				}
+			}
+		}
+	}
+	return false, nil
+}
+
 // validateEvent returns true if a given event is valid.
 func (g Group) validateEvent(event MembershipUpdateEvent) bool {
 	if len(event.From) == 0 {

protocol/v1/membership_update_message_test.go

@@ -543,3 +543,32 @@ func TestAbridgedEventsAdmins(t *testing.T) {
 	// All the events are relevant here, so it should be the same
 	require.Len(t, g.AbridgedEvents(), 3)
 }
+
+func TestWasEverMember(t *testing.T) {
+	key, err := crypto.GenerateKey()
+	require.NoError(t, err)
+	g, err := NewGroupWithCreator("abc", "#fa6565", 20, key)
+	require.NoError(t, err)
+
+	wasMember, err := g.WasEverMember(publicKeyToString(&key.PublicKey))
+	require.NoError(t, err)
+	require.True(t, wasMember)
+
+	key2, err := crypto.GenerateKey()
+	require.NoError(t, err)
+
+	wasMember, err = g.WasEverMember(publicKeyToString(&key2.PublicKey))
+	require.NoError(t, err)
+	require.False(t, wasMember)
+
+	// Add a new member
+	event := NewMembersAddedEvent([]string{publicKeyToString(&key2.PublicKey)}, 21)
+	event.From = publicKeyToString(&key.PublicKey)
+	event.ChatID = g.chatID
+	err = g.ProcessEvent(event)
+	require.NoError(t, err)
+
+	wasMember, err = g.WasEverMember(publicKeyToString(&key2.PublicKey))
+	require.NoError(t, err)
+	require.True(t, wasMember)
+}