status-im
/
status-go
mirror of https://github.com/status-im/status-go.git
2
0
Fork 0
Code Issues Projects Releases Wiki Activity

fix_: community members reevaluation fixes (#5117) 

* fix_: prevent publishing older community description

* fix_: schedule member reevaluation instead of reevaluating in parallel

* fix_: lock community on members reevaluation

* fix(TestJoinCommunityAsAdminWithMemberAndAdminPermission)_: setup waitOnCommunitiesEvent in advance

* fix(TestEditSharedAddresses)_: remove redundant community description retrieval
This commit is contained in:
Igor Sirotin 2024-05-08 16:32:46 +01:00 committed by GitHub
parent 522f3288b0
commit 3e4367a7cf
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 93 additions and 106 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

99
protocol/communities/manager.go
View File

@ -951,9 +951,21 @@ func (m *Manager) EditCommunityTokenPermission(request *requests.EditCommunityTo
return community, changes, nil return community, changes, nil
} }
func (m *Manager) ReevaluateMembers(community *Community) (map[protobuf.CommunityMember_Roles][]*ecdsa.PublicKey, error) { // WARNING: ReevaluateMembers is only public to be used in messenger tests.
m.communityLock.Lock(community.ID()) func (m *Manager) ReevaluateMembers(communityID types.HexBytes) (*Community, map[protobuf.CommunityMember_Roles][]*ecdsa.PublicKey, error) {
defer m.communityLock.Unlock(community.ID()) m.communityLock.Lock(communityID)
defer m.communityLock.Unlock(communityID)
community, err := m.GetByID(communityID)
if err != nil {
return nil, nil, err
}
// TODO: Control node needs to be notified to do a permission check if TokenMasters did airdrop
// of the token which is using in a community permissions
if !community.IsControlNode() {
return nil, nil, ErrNotEnoughPermissions
}
becomeMemberPermissions := community.TokenPermissionsByType(protobuf.CommunityTokenPermission_BECOME_MEMBER) becomeMemberPermissions := community.TokenPermissionsByType(protobuf.CommunityTokenPermission_BECOME_MEMBER)
becomeAdminPermissions := community.TokenPermissionsByType(protobuf.CommunityTokenPermission_BECOME_ADMIN) becomeAdminPermissions := community.TokenPermissionsByType(protobuf.CommunityTokenPermission_BECOME_ADMIN)
@ -968,7 +980,7 @@ func (m *Manager) ReevaluateMembers(community *Community) (map[protobuf.Communit
for memberKey := range community.Members() { for memberKey := range community.Members() {
memberPubKey, err := common.HexToPubkey(memberKey) memberPubKey, err := common.HexToPubkey(memberKey)
if err != nil { if err != nil {
return nil, err return nil, nil, err
} }
if memberKey == common.PubkeyToHex(&m.identity.PublicKey) || community.IsMemberOwner(memberPubKey) { if memberKey == common.PubkeyToHex(&m.identity.PublicKey) || community.IsMemberOwner(memberPubKey) {
@ -980,7 +992,7 @@ func (m *Manager) ReevaluateMembers(community *Community) (map[protobuf.Communit
requestID := CalculateRequestID(memberKey, community.ID()) requestID := CalculateRequestID(memberKey, community.ID())
revealedAccounts, err := m.persistence.GetRequestToJoinRevealedAddresses(requestID) revealedAccounts, err := m.persistence.GetRequestToJoinRevealedAddresses(requestID)
if err != nil { if err != nil {
return nil, err return nil, nil, err
} }
memberHasWallet := len(revealedAccounts) > 0 memberHasWallet := len(revealedAccounts) > 0
@ -990,7 +1002,7 @@ func (m *Manager) ReevaluateMembers(community *Community) (map[protobuf.Communit
if !memberHasWallet && (hasMemberPermissions || isCurrentRoleTokenMaster || isCurrentRoleAdmin) { if !memberHasWallet && (hasMemberPermissions || isCurrentRoleTokenMaster || isCurrentRoleAdmin) {
_, err = community.RemoveUserFromOrg(memberPubKey) _, err = community.RemoveUserFromOrg(memberPubKey)
if err != nil { if err != nil {
return nil, err return nil, nil, err
} }
continue continue
} }
@ -1001,7 +1013,7 @@ func (m *Manager) ReevaluateMembers(community *Community) (map[protobuf.Communit
protobuf.CommunityMember_ROLE_TOKEN_MASTER, isCurrentRoleTokenMaster) protobuf.CommunityMember_ROLE_TOKEN_MASTER, isCurrentRoleTokenMaster)
if err != nil { if err != nil {
return nil, err return nil, nil, err
} }
if isNewRoleTokenMaster { if isNewRoleTokenMaster {
@ -1017,7 +1029,7 @@ func (m *Manager) ReevaluateMembers(community *Community) (map[protobuf.Communit
protobuf.CommunityMember_ROLE_ADMIN, isCurrentRoleAdmin) protobuf.CommunityMember_ROLE_ADMIN, isCurrentRoleAdmin)
if err != nil { if err != nil {
return nil, err return nil, nil, err
} }
if isNewRoleAdmin { if isNewRoleAdmin {
@ -1032,13 +1044,13 @@ func (m *Manager) ReevaluateMembers(community *Community) (map[protobuf.Communit
if hasMemberPermissions { if hasMemberPermissions {
permissionResponse, err := m.PermissionChecker.CheckPermissions(becomeMemberPermissions, accountsAndChainIDs, true) permissionResponse, err := m.PermissionChecker.CheckPermissions(becomeMemberPermissions, accountsAndChainIDs, true)
if err != nil { if err != nil {
return nil, err return nil, nil, err
} }
if !permissionResponse.Satisfied { if !permissionResponse.Satisfied {
_, err = community.RemoveUserFromOrg(memberPubKey) _, err = community.RemoveUserFromOrg(memberPubKey)
if err != nil { if err != nil {
return nil, err return nil, nil, err
} }
// Skip channels validation if user has been removed // Skip channels validation if user has been removed
continue continue
@ -1056,14 +1068,14 @@ func (m *Manager) ReevaluateMembers(community *Community) (map[protobuf.Communit
// ensure all members are added back if channel permissions were removed // ensure all members are added back if channel permissions were removed
_, err = community.PopulateChatWithAllMembers(channelID) _, err = community.PopulateChatWithAllMembers(channelID)
if err != nil { if err != nil {
return nil, err return nil, nil, err
} }
continue continue
} }
response, err := m.checkChannelPermissions(viewOnlyPermissions, viewAndPostPermissions, accountsAndChainIDs, true) response, err := m.checkChannelPermissions(viewOnlyPermissions, viewAndPostPermissions, accountsAndChainIDs, true)
if err != nil { if err != nil {
return nil, err return nil, nil, err
} }
isMemberAlreadyInChannel := community.IsMemberInChat(memberPubKey, channelID) isMemberAlreadyInChannel := community.IsMemberInChat(memberPubKey, channelID)
@ -1077,22 +1089,25 @@ func (m *Manager) ReevaluateMembers(community *Community) (map[protobuf.Communit
// Add the member back to the chat member list in case the role changed (it replaces the previous values) // Add the member back to the chat member list in case the role changed (it replaces the previous values)
_, err := community.AddMemberToChat(channelID, memberPubKey, []protobuf.CommunityMember_Roles{}, channelRole) _, err := community.AddMemberToChat(channelID, memberPubKey, []protobuf.CommunityMember_Roles{}, channelRole)
if err != nil { if err != nil {
return nil, err return nil, nil, err
} }
} else if isMemberAlreadyInChannel { } else if isMemberAlreadyInChannel {
_, err := community.RemoveUserFromChat(memberPubKey, channelID) _, err := community.RemoveUserFromChat(memberPubKey, channelID)
if err != nil { if err != nil {
return nil, err return nil, nil, err
} }
} }
} }
} }
return newPrivilegedRoles, m.saveAndPublish(community) return community, newPrivilegedRoles, m.saveAndPublish(community)
} }
func (m *Manager) ReevaluateMembersPeriodically(communityID types.HexBytes) { func (m *Manager) StartMembersReevaluationLoop(communityID types.HexBytes, reevaluateOnStart bool) {
logger := m.logger.Named("reevaluate members loop").With(zap.String("communityID", communityID.String())) go m.reevaluateMembersLoop(communityID, reevaluateOnStart)
}
func (m *Manager) reevaluateMembersLoop(communityID types.HexBytes, reevaluateOnStart bool) {
if _, exists := m.membersReevaluationTasks.Load(communityID.String()); exists { if _, exists := m.membersReevaluationTasks.Load(communityID.String()); exists {
return return
@ -1126,11 +1141,14 @@ func (m *Manager) ReevaluateMembersPeriodically(communityID types.HexBytes) {
return nil return nil
} }
if task.lastSuccessTime.Before(time.Now().Add(-memberPermissionsCheckInterval)) || if !task.lastSuccessTime.Before(time.Now().Add(-memberPermissionsCheckInterval)) &&
task.lastSuccessTime.Before(task.onDemandRequestTime) { !task.lastSuccessTime.Before(task.onDemandRequestTime) {
community, err := m.GetByID(communityID) return nil
}
err = m.reevaluateCommunityMembersPermissions(communityID)
if err != nil { if err != nil {
if err == ErrOrgNotFound { if errors.Is(err, ErrOrgNotFound) {
return criticalError{ return criticalError{
error: err, error: err,
} }
@ -1138,31 +1156,30 @@ func (m *Manager) ReevaluateMembersPeriodically(communityID types.HexBytes) {
return err return err
} }
err = m.ReevaluateCommunityMembersPermissions(community)
if err != nil {
return err
}
task.lastSuccessTime = time.Now() task.lastSuccessTime = time.Now()
}
return nil return nil
} }
ticker := time.NewTicker(10 * time.Second) ticker := time.NewTicker(10 * time.Second)
defer ticker.Stop() defer ticker.Stop()
logger.Debug("loop started") reevaluate := reevaluateOnStart
defer logger.Debug("loop stopped")
for { for {
select { if reevaluate {
case <-ticker.C:
err := reevaluateMembers() err := reevaluateMembers()
if err != nil { if err != nil {
logger.Error("reevaluation failed", zap.Error(err)) var criticalError *criticalError
if _, isCritical := err.(*criticalError); isCritical { if errors.As(err, &criticalError) {
return return
} }
} }
}
select {
case <-ticker.C:
reevaluate = true
continue
case <-m.quit: case <-m.quit:
return return
@ -1209,18 +1226,8 @@ func (m *Manager) DeleteCommunityTokenPermission(request *requests.DeleteCommuni
return community, changes, nil return community, changes, nil
} }
func (m *Manager) ReevaluateCommunityMembersPermissions(community *Community) error { func (m *Manager) reevaluateCommunityMembersPermissions(communityID types.HexBytes) error {
if community == nil { community, newPrivilegedMembers, err := m.ReevaluateMembers(communityID)
return ErrOrgNotFound
}
// TODO: Control node needs to be notified to do a permission check if TokenMasters did airdrop
// of the token which is using in a community permissions
if !community.IsControlNode() {
return ErrNotEnoughPermissions
}
newPrivilegedMembers, err := m.ReevaluateMembers(community)
if err != nil { if err != nil {
return err return err
} }
@ -4921,7 +4928,9 @@ func (m *Manager) saveAndPublish(community *Community) error {
if community.IsControlNode() { if community.IsControlNode() {
m.publish(&Subscription{Community: community}) m.publish(&Subscription{Community: community})
return nil return nil
} else if community.HasPermissionToSendCommunityEvents() { }
if community.HasPermissionToSendCommunityEvents() {
err := m.signEvents(community) err := m.signEvents(community)
if err != nil { if err != nil {
return err return err

51
protocol/communities_messenger_token_permissions_test.go
View File

@ -611,19 +611,6 @@ func (s *MessengerCommunitiesTokenPermissionsSuite) TestEditSharedAddresses() {
s.Require().Equal(revealedAccounts[0].Address, aliceAddress1) s.Require().Equal(revealedAccounts[0].Address, aliceAddress1)
s.Require().Equal(true, revealedAccounts[0].IsAirdropAddress) s.Require().Equal(true, revealedAccounts[0].IsAirdropAddress)
// Retrieve community description change
err = tt.RetryWithBackOff(func() error {
response, err := s.alice.RetrieveAll()
if err != nil {
return err
}
if len(response.Communities()) == 0 {
return errors.New("no communities in response (address change reception)")
}
return nil
})
s.Require().NoError(err)
alicesRevealedAccounts, err = s.alice.communitiesManager.GetRevealedAddresses(community.ID(), alicePubkey) alicesRevealedAccounts, err = s.alice.communitiesManager.GetRevealedAddresses(community.ID(), alicePubkey)
s.Require().NoError(err) s.Require().NoError(err)
s.Require().Len(alicesRevealedAccounts, 1) s.Require().Len(alicesRevealedAccounts, 1)
@ -996,14 +983,15 @@ func (s *MessengerCommunitiesTokenPermissionsSuite) TestJoinCommunityAsAdminWith
}, },
}, },
} }
response, err := s.owner.CreateCommunityTokenPermission(&permissionRequestMember)
s.Require().NoError(err)
s.Require().Len(response.Communities(), 1)
waitOnCommunityPermissionCreated := waitOnCommunitiesEvent(s.owner, func(sub *communities.Subscription) bool { waitOnCommunityPermissionCreated := waitOnCommunitiesEvent(s.owner, func(sub *communities.Subscription) bool {
return sub.Community.HasTokenPermissions() return sub.Community.HasTokenPermissions()
}) })
response, err := s.owner.CreateCommunityTokenPermission(&permissionRequestMember)
s.Require().NoError(err)
s.Require().Len(response.Communities(), 1)
err = <-waitOnCommunityPermissionCreated err = <-waitOnCommunityPermissionCreated
s.Require().NoError(err) s.Require().NoError(err)
@ -1021,16 +1009,17 @@ func (s *MessengerCommunitiesTokenPermissionsSuite) TestJoinCommunityAsAdminWith
}, },
}, },
} }
waitOnCommunityPermissionCreated = waitOnCommunitiesEvent(s.owner, func(sub *communities.Subscription) bool {
return len(sub.Community.TokenPermissions()) == 2
})
response, err = s.owner.CreateCommunityTokenPermission(&permissionRequestAdmin) response, err = s.owner.CreateCommunityTokenPermission(&permissionRequestAdmin)
s.Require().NoError(err) s.Require().NoError(err)
s.Require().Len(response.Communities(), 1) s.Require().Len(response.Communities(), 1)
s.Require().Len(response.Communities()[0].TokenPermissionsByType(protobuf.CommunityTokenPermission_BECOME_ADMIN), 1) s.Require().Len(response.Communities()[0].TokenPermissionsByType(protobuf.CommunityTokenPermission_BECOME_ADMIN), 1)
s.Require().Len(response.Communities()[0].TokenPermissions(), 2) s.Require().Len(response.Communities()[0].TokenPermissions(), 2)
waitOnCommunityPermissionCreated = waitOnCommunitiesEvent(s.owner, func(sub *communities.Subscription) bool {
return len(sub.Community.TokenPermissions()) == 2
})
err = <-waitOnCommunityPermissionCreated err = <-waitOnCommunityPermissionCreated
s.Require().NoError(err) s.Require().NoError(err)
@ -1165,9 +1154,7 @@ func (s *MessengerCommunitiesTokenPermissionsSuite) testViewChannelPermissions(v
// force owner to reevaluate channel members // force owner to reevaluate channel members
// in production it will happen automatically, by periodic check // in production it will happen automatically, by periodic check
community, err = s.owner.communitiesManager.GetByID(community.ID()) community, _, err = s.owner.communitiesManager.ReevaluateMembers(community.ID())
s.Require().NoError(err)
_, err = s.owner.communitiesManager.ReevaluateMembers(community)
s.Require().NoError(err) s.Require().NoError(err)
err = <-waitOnChannelKeyToBeDistributedToBob err = <-waitOnChannelKeyToBeDistributedToBob
@ -1338,9 +1325,7 @@ func (s *MessengerCommunitiesTokenPermissionsSuite) TestMemberRoleGetUpdatedWhen
// force owner to reevaluate channel members // force owner to reevaluate channel members
// in production it will happen automatically, by periodic check // in production it will happen automatically, by periodic check
community, err = s.owner.communitiesManager.GetByID(community.ID()) community, _, err = s.owner.communitiesManager.ReevaluateMembers(community.ID())
s.Require().NoError(err)
_, err = s.owner.communitiesManager.ReevaluateMembers(community)
s.Require().NoError(err) s.Require().NoError(err)
err = <-waitOnChannelKeyToBeDistributedToBob err = <-waitOnChannelKeyToBeDistributedToBob
@ -1391,9 +1376,7 @@ func (s *MessengerCommunitiesTokenPermissionsSuite) TestMemberRoleGetUpdatedWhen
// force owner to reevaluate channel members // force owner to reevaluate channel members
// in production it will happen automatically, by periodic check // in production it will happen automatically, by periodic check
community, err = s.owner.communitiesManager.GetByID(community.ID()) community, roles, err := s.owner.communitiesManager.ReevaluateMembers(community.ID())
s.Require().NoError(err)
roles, err := s.owner.communitiesManager.ReevaluateMembers(community)
s.Require().NoError(err) s.Require().NoError(err)
s.Require().Len(roles, 2) s.Require().Len(roles, 2)
community, err = s.owner.communitiesManager.GetByID(community.ID()) community, err = s.owner.communitiesManager.GetByID(community.ID())
@ -1493,7 +1476,7 @@ func (s *MessengerCommunitiesTokenPermissionsSuite) testReevaluateMemberPrivileg
s.Require().True(checkRoleBasedOnThePermissionType(permissionType, &s.alice.identity.PublicKey, community)) s.Require().True(checkRoleBasedOnThePermissionType(permissionType, &s.alice.identity.PublicKey, community))
// the control node re-evaluates the roles of the participants, checking that the privileged user has not lost his role // the control node re-evaluates the roles of the participants, checking that the privileged user has not lost his role
_, err = s.owner.communitiesManager.ReevaluateMembers(community) community, _, err = s.owner.communitiesManager.ReevaluateMembers(community.ID())
s.Require().NoError(err) s.Require().NoError(err)
community, err = s.owner.communitiesManager.GetByID(community.ID()) community, err = s.owner.communitiesManager.GetByID(community.ID())
s.Require().NoError(err) s.Require().NoError(err)
@ -1515,7 +1498,7 @@ func (s *MessengerCommunitiesTokenPermissionsSuite) testReevaluateMemberPrivileg
s.Require().NoError(err) s.Require().NoError(err)
s.Require().False(community.HasTokenPermissions()) s.Require().False(community.HasTokenPermissions())
_, err = s.owner.communitiesManager.ReevaluateMembers(community) community, _, err = s.owner.communitiesManager.ReevaluateMembers(community.ID())
s.Require().NoError(err) s.Require().NoError(err)
community, err = s.owner.communitiesManager.GetByID(community.ID()) community, err = s.owner.communitiesManager.GetByID(community.ID())
@ -1608,7 +1591,7 @@ func (s *MessengerCommunitiesTokenPermissionsSuite) testReevaluateMemberPrivileg
s.Require().True(checkRoleBasedOnThePermissionType(permissionType, &s.alice.identity.PublicKey, community)) s.Require().True(checkRoleBasedOnThePermissionType(permissionType, &s.alice.identity.PublicKey, community))
// the control node reevaluates the roles of the participants, checking that the privileged user has not lost his role // the control node reevaluates the roles of the participants, checking that the privileged user has not lost his role
_, err = s.owner.communitiesManager.ReevaluateMembers(community) community, _, err = s.owner.communitiesManager.ReevaluateMembers(community.ID())
s.Require().NoError(err) s.Require().NoError(err)
community, err = s.owner.communitiesManager.GetByID(community.ID()) community, err = s.owner.communitiesManager.GetByID(community.ID())
@ -1631,7 +1614,7 @@ func (s *MessengerCommunitiesTokenPermissionsSuite) testReevaluateMemberPrivileg
s.Require().NoError(err) s.Require().NoError(err)
s.Require().Len(response.Communities()[0].TokenPermissions(), 1) s.Require().Len(response.Communities()[0].TokenPermissions(), 1)
_, err = s.owner.communitiesManager.ReevaluateMembers(community) community, _, err = s.owner.communitiesManager.ReevaluateMembers(community.ID())
s.Require().NoError(err) s.Require().NoError(err)
community, err = s.owner.communitiesManager.GetByID(community.ID()) community, err = s.owner.communitiesManager.GetByID(community.ID())
@ -1655,7 +1638,7 @@ func (s *MessengerCommunitiesTokenPermissionsSuite) testReevaluateMemberPrivileg
s.Require().NoError(err) s.Require().NoError(err)
s.Require().Len(response.Communities()[0].TokenPermissions(), 0) s.Require().Len(response.Communities()[0].TokenPermissions(), 0)
_, err = s.owner.communitiesManager.ReevaluateMembers(community) community, _, err = s.owner.communitiesManager.ReevaluateMembers(community.ID())
s.Require().NoError(err) s.Require().NoError(err)
community, err = s.owner.communitiesManager.GetByID(community.ID()) community, err = s.owner.communitiesManager.GetByID(community.ID())

2
protocol/messenger.go
View File

@ -820,7 +820,7 @@ func (m *Messenger) Start() (*MessengerResponse, error) {
for _, c := range controlledCommunities { for _, c := range controlledCommunities {
if c.Joined() && c.HasTokenPermissions() { if c.Joined() && c.HasTokenPermissions() {
go m.communitiesManager.ReevaluateMembersPeriodically(c.ID()) go m.communitiesManager.StartMembersReevaluationLoop(c.ID(), false)
} }
} }

27
protocol/messenger_communities.go
View File

@ -292,6 +292,10 @@ func (m *Messenger) handleCommunitiesSubscription(c chan *communities.Subscripti
publishOrgAndDistributeEncryptionKeys := func(community *communities.Community) { publishOrgAndDistributeEncryptionKeys := func(community *communities.Community) {
recentlyPublishedOrg := recentlyPublishedOrgs[community.IDString()] recentlyPublishedOrg := recentlyPublishedOrgs[community.IDString()]
if recentlyPublishedOrg != nil && community.Clock() <= recentlyPublishedOrg.Clock() {
return
}
// evaluate and distribute encryption keys (if any) // evaluate and distribute encryption keys (if any)
encryptionKeyActions := communities.EvaluateCommunityEncryptionKeyActions(recentlyPublishedOrg, community) encryptionKeyActions := communities.EvaluateCommunityEncryptionKeyActions(recentlyPublishedOrg, community)
err := m.communitiesKeyDistributor.Distribute(community, encryptionKeyActions) err := m.communitiesKeyDistributor.Distribute(community, encryptionKeyActions)
@ -2498,14 +2502,7 @@ func (m *Messenger) CreateCommunityTokenPermission(request *requests.CreateCommu
} }
if community.IsControlNode() { if community.IsControlNode() {
// check existing member permission once, then check periodically m.communitiesManager.StartMembersReevaluationLoop(community.ID(), true)
go func() {
if err := m.communitiesManager.ReevaluateCommunityMembersPermissions(community); err != nil {
m.logger.Debug("failed to check member permissions", zap.Error(err))
}
m.communitiesManager.ReevaluateMembersPeriodically(community.ID())
}()
} }
// ensure HRkeys are synced // ensure HRkeys are synced
@ -2538,11 +2535,10 @@ func (m *Messenger) EditCommunityTokenPermission(request *requests.EditCommunity
// //
// We do this in a separate routine to not block this function // We do this in a separate routine to not block this function
if community.IsControlNode() { if community.IsControlNode() {
go func() { err = m.communitiesManager.ScheduleMembersReevaluation(community.ID())
if err := m.communitiesManager.ReevaluateCommunityMembersPermissions(community); err != nil { if err != nil {
m.logger.Debug("failed to check member permissions", zap.Error(err)) return nil, err
} }
}()
} }
response := &MessengerResponse{} response := &MessengerResponse{}
@ -2565,11 +2561,10 @@ func (m *Messenger) DeleteCommunityTokenPermission(request *requests.DeleteCommu
// check if members still fulfill the token criteria // check if members still fulfill the token criteria
// We do this in a separate routine to not block this function // We do this in a separate routine to not block this function
if community.IsControlNode() { if community.IsControlNode() {
go func() { err = m.communitiesManager.ScheduleMembersReevaluation(community.ID())
if err = m.communitiesManager.ReevaluateCommunityMembersPermissions(community); err != nil { if err != nil {
m.logger.Debug("failed to check member permissions", zap.Error(err)) return nil, err
} }
}()
} }
response := &MessengerResponse{} response := &MessengerResponse{}