From 7fc3e4440f919005d53600a4cbf8f8284dd6e8cb Mon Sep 17 00:00:00 2001 From: Mikhail Rogachev Date: Fri, 22 Dec 2023 00:38:14 +0700 Subject: [PATCH] Fix: Don't create extra mutual state messages when accepting the CR again (#4488) * Fix: don't create extra system messages when accepting the CR again * chore: add one more test for cr flows repeated * chore: use deprecation.AddChatsCount in contact requests tests --- protocol/messenger_contact_requests_test.go | 92 ++++++++++++++++++++- protocol/messenger_handler.go | 39 +++++---- 2 files changed, 113 insertions(+), 18 deletions(-) diff --git a/protocol/messenger_contact_requests_test.go b/protocol/messenger_contact_requests_test.go index 25d606708..dfbeb50e6 100644 --- a/protocol/messenger_contact_requests_test.go +++ b/protocol/messenger_contact_requests_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/suite" "go.uber.org/zap" + "github.com/status-im/status-go/deprecation" "github.com/status-im/status-go/eth-node/crypto" "github.com/status-im/status-go/eth-node/types" "github.com/status-im/status-go/multiaccounts/settings" @@ -486,6 +487,32 @@ func (s *MessengerContactRequestSuite) TestReceiveAcceptAndRetractContactRequest s.retractContactRequest(contactID, theirMessenger) } +// The scenario tested is as follow: +// 1. Repeat 5 times: +// 2.1) Alice sends a contact request to Bob +// 2.2) Bob accepts the contact request +// 2.3) Alice removes bob from contacts +func (s *MessengerContactRequestSuite) TestAcceptCRRemoveAndRepeat() { + theirMessenger := s.newMessenger() + _, err := theirMessenger.Start() + s.Require().NoError(err) + defer TearDownMessenger(&s.Suite, theirMessenger) + + contactID := types.EncodeHex(crypto.FromECDSAPub(&theirMessenger.identity.PublicKey)) + + for i := 0; i < 5; i++ { + messageText := fmt.Sprintf("hello %d", i) + request := &requests.SendContactRequest{ + ID: contactID, + Message: messageText, + } + s.sendContactRequest(request, s.m) + contactRequest := s.receiveContactRequest(messageText, theirMessenger) + s.acceptContactRequest(contactRequest, s.m, theirMessenger) + s.retractContactRequest(contactID, theirMessenger) + } +} + // The scenario tested is as follow: // 1) Alice sends a contact request to Bob // 2) Bob declines the contact request @@ -539,7 +566,68 @@ func (s *MessengerContactRequestSuite) TestAliceTriesToSpamBobWithContactRequest ) s.Require().Error(err) s.Require().ErrorContains(err, "no messages") +} +// The scenario tested is as follow: +// 1) Alice sends a contact request to Bob +// 2) Bob accepts the contact +// 3) Bob accepts the contact request (again!) +// 4) No extra mesages on Alice's side +func (s *MessengerContactRequestSuite) TestAliceSeesOnlyOneAcceptFromBob() { + messageTextAlice := "You wanna play with fire, Bobby?!" + alice := s.m + + bob := s.newMessenger() + _, err := bob.Start() + s.Require().NoError(err) + defer TearDownMessenger(&s.Suite, bob) + + bobID := types.EncodeHex(crypto.FromECDSAPub(&bob.identity.PublicKey)) + + // Alice sends a contact request to Bob + request := &requests.SendContactRequest{ + ID: bobID, + Message: messageTextAlice, + } + s.sendContactRequest(request, alice) + + contactRequest := s.receiveContactRequest(messageTextAlice, bob) + s.Require().NotNil(contactRequest) + + // Bob accepts the contact request + s.acceptContactRequest(contactRequest, alice, bob) + + // Accept contact request again + _, err = bob.AcceptContactRequest(context.Background(), &requests.AcceptContactRequest{ID: types.Hex2Bytes(contactRequest.ID)}) + s.Require().NoError(err) + + // Check we don't have extra messages on Alice's side + resp, err := WaitOnMessengerResponse(alice, + func(r *MessengerResponse) bool { + return len(r.ActivityCenterNotifications()) == 1 && len(r.Messages()) == 1 + }, + "contact request acceptance not received", + ) + s.Require().NoError(err) + s.Require().NotNil(resp) + + // Check activity center notification is of the right type + s.Require().Len(resp.ActivityCenterNotifications(), 1) + s.Require().Equal(ActivityCenterNotificationTypeContactRequest, resp.ActivityCenterNotifications()[0].Type) + s.Require().Equal(common.ContactRequestStateAccepted, resp.ActivityCenterNotifications()[0].Message.ContactRequestState) + s.Require().Equal(resp.ActivityCenterNotifications()[0].Read, true) + s.Require().Equal(resp.ActivityCenterNotifications()[0].Accepted, true) + s.Require().Equal(resp.ActivityCenterNotifications()[0].Dismissed, false) + s.Require().NotNil(resp.ActivityCenterNotifications()[0].Message) + + // Make sure the message is updated, sender side + s.Require().Len(resp.Messages(), 1) + + contactRequest = s.findFirstByContentType(resp.Messages(), protobuf.ChatMessage_CONTACT_REQUEST) + s.Require().NotNil(contactRequest) + + s.Require().Equal(common.ContactRequestStateAccepted, contactRequest.ContactRequestState) + s.Require().Equal(request.Message, contactRequest.Text) } func (s *MessengerContactRequestSuite) TestReceiveAndAcceptContactRequestTwice() { //nolint: unused @@ -1472,7 +1560,7 @@ func (s *MessengerContactRequestSuite) blockContactAndSync(alice1 *Messenger, al s.Require().Equal(ContactRequestStateReceived, respContact.ContactRequestRemoteState) // Check chats list - //s.Require().Len(alice2.Chats(), 1) // FIXME: Uncomment after https://github.com/status-im/status-go/issues/3800 + s.Require().Len(alice2.Chats(), deprecation.AddChatsCount(2)) } func (s *MessengerContactRequestSuite) unblockContactAndSync(alice1 *Messenger, alice2 *Messenger, bob *Messenger) { @@ -1503,7 +1591,7 @@ func (s *MessengerContactRequestSuite) unblockContactAndSync(alice1 *Messenger, s.Require().Equal(respContact.ContactRequestRemoteState, ContactRequestStateNone) // Check chats list - //s.Require().Len(alice2.Chats(), 1) // FIXME: Uncomment after https://github.com/status-im/status-go/issues/3800 + s.Require().Len(alice2.Chats(), deprecation.AddChatsCount(2)) } func (s *MessengerContactRequestSuite) TestBlockedContactSyncing() { diff --git a/protocol/messenger_handler.go b/protocol/messenger_handler.go index 39703193c..44dba993e 100644 --- a/protocol/messenger_handler.go +++ b/protocol/messenger_handler.go @@ -980,8 +980,12 @@ func (m *Messenger) handleAcceptContactRequestMessage(state *ReceivedMessageStat return err } + // We still want to handle acceptance of the CR even it was already accepted + previouslyAccepted := request != nil && request.ContactRequestState == common.ContactRequestStateAccepted + contact := state.CurrentMessageState.Contact + // The request message will be added to the response here processingResponse, err := m.handleAcceptContactRequest(state.Response, contact, request, clock) if err != nil { return err @@ -1010,25 +1014,28 @@ func (m *Messenger) handleAcceptContactRequestMessage(state *ReceivedMessageStat } // Add mutual state update message for incoming contact request - clock, timestamp := chat.NextClockAndTimestamp(m.transport) + if !previouslyAccepted { + clock, timestamp := chat.NextClockAndTimestamp(m.transport) - updateMessage, err := m.prepareMutualStateUpdateMessage(contact.ID, MutualStateUpdateTypeAdded, clock, timestamp, false) - if err != nil { - return err + updateMessage, err := m.prepareMutualStateUpdateMessage(contact.ID, MutualStateUpdateTypeAdded, clock, timestamp, false) + if err != nil { + return err + } + + m.prepareMessage(updateMessage, m.httpServer) + err = m.persistence.SaveMessages([]*common.Message{updateMessage}) + if err != nil { + return err + } + state.Response.AddMessage(updateMessage) + + err = chat.UpdateFromMessage(updateMessage, m.getTimesource()) + if err != nil { + return err + } + chat.UnviewedMessagesCount++ } - m.prepareMessage(updateMessage, m.httpServer) - err = m.persistence.SaveMessages([]*common.Message{updateMessage}) - if err != nil { - return err - } - state.Response.AddMessage(updateMessage) - - err = chat.UpdateFromMessage(updateMessage, m.getTimesource()) - if err != nil { - return err - } - chat.UnviewedMessagesCount++ state.Response.AddChat(chat) state.AllChats.Store(chat.ID, chat) }