From 27934a4e1fbddfdb809357e5d58a2e81b37fb420 Mon Sep 17 00:00:00 2001 From: Patryk Osmaczko Date: Mon, 10 Jun 2024 16:20:21 +0200 Subject: [PATCH] chore(communities)_: reject outdated community descriptions Prevent the inclusion of CommunityDescription with an outdated clock in MessengerResponse to avoid false-positives in tests and reduce redundant data exchange between status-go and clients. --- protocol/communities/community.go | 6 +-- protocol/communities/community_test.go | 4 +- protocol/communities/errors.go | 1 + protocol/communities_messenger_test.go | 63 ++++++++++++++++++++++++++ protocol/messenger_communities.go | 3 +- 5 files changed, 71 insertions(+), 6 deletions(-) diff --git a/protocol/communities/community.go b/protocol/communities/community.go index e46c378af..0834f0b2a 100644 --- a/protocol/communities/community.go +++ b/protocol/communities/community.go @@ -1130,11 +1130,9 @@ func (o *Community) UpdateCommunityDescription(description *protobuf.CommunityDe return nil, err } - response := o.emptyCommunityChanges() - // Enables processing of identical clocks. Identical descriptions may be reprocessed upon subsequent receipt of the previously missing encryption key. if description.Clock < o.config.CommunityDescription.Clock { - return response, nil + return nil, ErrInvalidCommunityDescriptionClockOutdated } originCommunity := o.CreateDeepCopy() @@ -1146,6 +1144,8 @@ func (o *Community) UpdateCommunityDescription(description *protobuf.CommunityDe o.setControlNode(newControlNode) } + response := o.emptyCommunityChanges() + // We only calculate changes if we joined/spectated the community or we requested access, otherwise not interested if o.config.Joined || o.config.Spectated || o.config.RequestedToJoinAt > 0 { response = EvaluateCommunityChanges(originCommunity, o) diff --git a/protocol/communities/community_test.go b/protocol/communities/community_test.go index b25df35b9..0ef84cac6 100644 --- a/protocol/communities/community_test.go +++ b/protocol/communities/community_test.go @@ -567,8 +567,8 @@ func (s *CommunitySuite) TestHandleCommunityDescription() { name: "updated version but lower clock", description: s.oldCommunityDescription, signer: signer, - changes: buildChanges, - err: nil, + changes: func(c *Community) *CommunityChanges { return nil }, + err: ErrInvalidCommunityDescriptionClockOutdated, }, { name: "removed member from org", diff --git a/protocol/communities/errors.go b/protocol/communities/errors.go index dbbbd87d8..4d6f9022d 100644 --- a/protocol/communities/errors.go +++ b/protocol/communities/errors.go @@ -12,6 +12,7 @@ var ErrChatAlreadyExists = errors.New("chat already exists") var ErrCategoryAlreadyExists = errors.New("category already exists") var ErrCantRequestAccess = errors.New("can't request access") var ErrInvalidCommunityDescription = errors.New("invalid community description") +var ErrInvalidCommunityDescriptionClockOutdated = errors.New("invalid community description outdated clock") var ErrInvalidCommunityDescriptionNoOrgPermissions = errors.New("invalid community description no org permissions") var ErrInvalidCommunityDescriptionNoChatPermissions = errors.New("invalid community description no chat permissions") var ErrInvalidCommunityDescriptionUnknownChatAccess = errors.New("invalid community description unknown chat access") diff --git a/protocol/communities_messenger_test.go b/protocol/communities_messenger_test.go index 9190fea97..d07f7fee0 100644 --- a/protocol/communities_messenger_test.go +++ b/protocol/communities_messenger_test.go @@ -4554,3 +4554,66 @@ func (s *MessengerCommunitiesSuite) TestAliceDidNotProcessOutdatedCommunityReque err = s.alice.HandleCommunityRequestToJoinResponse(state, requestToJoinResponse, nil) s.Require().NoError(err) } + +func (s *MessengerCommunitiesSuite) TestIgnoreOutdatedCommunityDescription() { + community, _ := s.createCommunity() + wrappedDescription1, err := community.ToProtocolMessageBytes() + s.Require().NoError(err) + signer, description1, err := communities.UnwrapCommunityDescriptionMessage(wrappedDescription1) + s.Require().NoError(err) + + _, err = community.AddMember(&s.alice.identity.PublicKey, []protobuf.CommunityMember_Roles{}) + s.Require().NoError(err) + wrappedDescription2, err := community.ToProtocolMessageBytes() + s.Require().NoError(err) + _, description2, err := communities.UnwrapCommunityDescriptionMessage(wrappedDescription2) + s.Require().NoError(err) + + _, err = community.AddMember(&s.bob.identity.PublicKey, []protobuf.CommunityMember_Roles{}) + s.Require().NoError(err) + wrappedDescription3, err := community.ToProtocolMessageBytes() + s.Require().NoError(err) + _, description3, err := communities.UnwrapCommunityDescriptionMessage(wrappedDescription3) + s.Require().NoError(err) + + s.Require().Less(description1.Clock, description2.Clock) + s.Require().Less(description2.Clock, description3.Clock) + + // Handle first community description + { + messageState := s.bob.buildMessageState() + err = s.bob.handleCommunityDescription(messageState, signer, description1, wrappedDescription1, nil, nil) + s.Require().NoError(err) + s.Require().Len(messageState.Response.Communities(), 1) + s.Require().Equal(description1.Clock, messageState.Response.Communities()[0].Clock()) + } + + // Handle third community description + { + messageState := s.bob.buildMessageState() + err = s.bob.handleCommunityDescription(messageState, signer, description3, wrappedDescription3, nil, nil) + s.Require().NoError(err) + s.Require().Len(messageState.Response.Communities(), 1) + s.Require().Equal(description3.Clock, messageState.Response.Communities()[0].Clock()) + + communityFromDB, err := s.bob.communitiesManager.GetByID(community.ID()) + s.Require().NoError(err) + s.Require().Equal(description3.Clock, communityFromDB.Clock()) + s.Require().Len(communityFromDB.Members(), 3) + } + + // Handle second (out of order) community description + // It should be ignored + { + messageState := s.bob.buildMessageState() + err = s.bob.handleCommunityDescription(messageState, signer, description2, wrappedDescription2, nil, nil) + s.Require().Len(messageState.Response.Communities(), 0) + s.Require().Len(messageState.Response.CommunityChanges, 0) + s.Require().ErrorIs(err, communities.ErrInvalidCommunityDescriptionClockOutdated) + + communityFromDB, err := s.bob.communitiesManager.GetByID(community.ID()) + s.Require().NoError(err) + s.Require().Equal(description3.Clock, communityFromDB.Clock()) + s.Require().Len(communityFromDB.Members(), 3) + } +} diff --git a/protocol/messenger_communities.go b/protocol/messenger_communities.go index 646135726..cdb40f951 100644 --- a/protocol/messenger_communities.go +++ b/protocol/messenger_communities.go @@ -3693,7 +3693,8 @@ func (m *Messenger) handleSyncInstallationCommunity(messageState *ReceivedMessag // TODO: handle shard err = m.handleCommunityDescription(messageState, signer, &cd, syncCommunity.Description, signer, nil) - if err != nil { + // Even if the Description is outdated we should proceed in order to sync settings and joined state + if err != nil && err != communities.ErrInvalidCommunityDescriptionClockOutdated { logger.Debug("m.handleCommunityDescription error", zap.Error(err)) return err }