Reorder peer deletion when deleteing a user (#1191)

management/server/peer.go

@@ -375,18 +375,22 @@ func (am *DefaultAccountManager) UpdatePeer(accountID, userID string, update *Pe
 // deletePeers will delete all specified peers and send updates to the remote peers. Don't call without acquiring account lock
 func (am *DefaultAccountManager) deletePeers(account *Account, peerIDs []string, userID string) error {
 
-	var peerError error
+	// the first loop is needed to ensure all peers present under the account before modifying, otherwise
+	// we might have some inconsistencies
+	peers := make([]*Peer, 0, len(peerIDs))
 	for _, peerID := range peerIDs {
 
 		peer := account.GetPeer(peerID)
 		if peer == nil {
-			peerError = status.Errorf(status.NotFound, "peer %s not found", peerID)
+			return status.Errorf(status.NotFound, "peer %s not found", peerID)
-			goto save
 		}
+		peers = append(peers, peer)
+	}
 
-		account.DeletePeer(peerID)
+	// the 2nd loop performs the actual modification
-
+	for _, peer := range peers {
-		peerError = am.peersUpdateManager.SendUpdate(peer.ID,
+		account.DeletePeer(peer.ID)
+		am.peersUpdateManager.SendUpdate(peer.ID,
 			&UpdateMessage{
 				Update: &proto.SyncResponse{
 					// fill those field for backward compatibility
@@ -402,36 +406,11 @@ func (am *DefaultAccountManager) deletePeers(account *Account, peerIDs []string,
 					},
 				},
 			})
-		if peerError != nil {
+		am.peersUpdateManager.CloseChannel(peer.ID)
-			goto save
-		}
-
-		am.peersUpdateManager.CloseChannel(peerID)
 		am.storeEvent(userID, peer.ID, account.Id, activity.PeerRemovedByUser, peer.EventMeta(am.GetDNSDomain()))
 	}
 
-save:
+	return nil
-	err := am.Store.SaveAccount(account)
-	if err != nil {
-		if peerError != nil {
-			log.Errorf("account save error: %s", err)
-			return peerError
-		} else {
-			return err
-		}
-	}
-
-	err = am.updateAccountPeers(account)
-	if err != nil {
-		if peerError != nil {
-			log.Errorf("update account peers error: %s", err)
-			return peerError
-		} else {
-			return err
-		}
-	}
-
-	return peerError
 }
 
 // DeletePeer removes peer from the account by its IP
@@ -444,7 +423,17 @@ func (am *DefaultAccountManager) DeletePeer(accountID, peerID, userID string) er
 		return err
 	}
 
-	return am.deletePeers(account, []string{peerID}, userID)
+	err = am.deletePeers(account, []string{peerID}, userID)
+	if err != nil {
+		return err
+	}
+
+	err = am.Store.SaveAccount(account)
+	if err != nil {
+		return err
+	}
+
+	return am.updateAccountPeers(account)
 }
 
 // GetPeerByIP returns peer by its IP
@@ -943,10 +932,7 @@ func (am *DefaultAccountManager) updateAccountPeers(account *Account) error {
 		}
 
 		update := toSyncResponse(nil, peer, nil, remotePeerNetworkMap, am.GetDNSDomain())
-		err = am.peersUpdateManager.SendUpdate(peer.ID, &UpdateMessage{Update: update})
+		am.peersUpdateManager.SendUpdate(peer.ID, &UpdateMessage{Update: update})
-		if err != nil {
-			return err
-		}
 	}
 
 	return nil

management/server/turncredentials.go

@@ -118,11 +118,7 @@ func (m *TimeBasedAuthSecretsManager) SetupRefresh(peerID string) {
 					},
 				}
 				log.Debugf("sending new TURN credentials to peer %s", peerID)
-				err := m.updateManager.SendUpdate(peerID, &UpdateMessage{Update: update})
+				m.updateManager.SendUpdate(peerID, &UpdateMessage{Update: update})
-				if err != nil {
-					log.Errorf("error while sending TURN update to peer %s %v", peerID, err)
-					// todo maybe continue trying?
-				}
 			}
 		}
 	}()

management/server/updatechannel.go

@@ -29,7 +29,7 @@ func NewPeersUpdateManager() *PeersUpdateManager {
 }
 
 // SendUpdate sends update message to the peer's channel
-func (p *PeersUpdateManager) SendUpdate(peerID string, update *UpdateMessage) error {
+func (p *PeersUpdateManager) SendUpdate(peerID string, update *UpdateMessage) {
 	p.channelsMux.Lock()
 	defer p.channelsMux.Unlock()
 	if channel, ok := p.peerChannels[peerID]; ok {
@@ -39,10 +39,9 @@ func (p *PeersUpdateManager) SendUpdate(peerID string, update *UpdateMessage) er
 		default:
 			log.Warnf("channel for peer %s is %d full", peerID, len(channel))
 		}
-		return nil
+	} else {
+		log.Debugf("peer %s has no channel", peerID)
 	}
-	log.Debugf("peer %s has no channel", peerID)
-	return nil
 }
 
 // CreateChannel creates a go channel for a given peer used to deliver updates relevant to the peer.

management/server/updatechannel_test.go

@@ -31,10 +31,7 @@ func TestSendUpdate(t *testing.T) {
 	if _, ok := peersUpdater.peerChannels[peer]; !ok {
 		t.Error("Error creating the channel")
 	}
-	err := peersUpdater.SendUpdate(peer, update1)
+	peersUpdater.SendUpdate(peer, update1)
-	if err != nil {
-		t.Error("Error sending update: ", err)
-	}
 	select {
 	case <-peersUpdater.peerChannels[peer]:
 	default:
@@ -42,10 +39,7 @@ func TestSendUpdate(t *testing.T) {
 	}
 
 	for range [channelBufferSize]int{} {
-		err = peersUpdater.SendUpdate(peer, update1)
+		peersUpdater.SendUpdate(peer, update1)
-		if err != nil {
-			t.Errorf("got an early error sending update: %v ", err)
-		}
 	}
 
 	update2 := &UpdateMessage{Update: &proto.SyncResponse{
@@ -54,10 +48,7 @@ func TestSendUpdate(t *testing.T) {
 		},
 	}}
 
-	err = peersUpdater.SendUpdate(peer, update2)
+	peersUpdater.SendUpdate(peer, update2)
-	if err != nil {
-		t.Error("update shouldn't return an error when channel buffer is full")
-	}
 	timeout := time.After(5 * time.Second)
 	for range [channelBufferSize]int{} {
 		select {

management/server/user.go

@@ -307,6 +307,12 @@ func (am *DefaultAccountManager) GetUser(claims jwtclaims.AuthorizationClaims) (
 	return user, nil
 }
 
+func (am *DefaultAccountManager) deleteServiceUser(account *Account, initiatorUserID string, targetUser *User) {
+	meta := map[string]any{"name": targetUser.ServiceUserName}
+	am.storeEvent(initiatorUserID, targetUser.Id, account.Id, activity.ServiceUserDeleted, meta)
+	delete(account.Users, targetUser.Id)
+}
+
 // DeleteUser deletes a user from the given account.
 func (am *DefaultAccountManager) DeleteUser(accountID, initiatorUserID string, targetUserID string) error {
 	if initiatorUserID == targetUserID {
@@ -320,11 +326,6 @@ func (am *DefaultAccountManager) DeleteUser(accountID, initiatorUserID string, t
 		return err
 	}
 
-	targetUser := account.Users[targetUserID]
-	if targetUser == nil {
-		return status.Errorf(status.NotFound, "user not found")
-	}
-
 	executingUser := account.Users[initiatorUserID]
 	if executingUser == nil {
 		return status.Errorf(status.NotFound, "user not found")
@@ -333,6 +334,53 @@ func (am *DefaultAccountManager) DeleteUser(accountID, initiatorUserID string, t
 		return status.Errorf(status.PermissionDenied, "only admins can delete users")
 	}
 
+	targetUser := account.Users[targetUserID]
+	if targetUser == nil {
+		return status.Errorf(status.NotFound, "target user not found")
+	}
+
+	// handle service user first and exit, no need to fetch extra data from IDP, etc
+	if targetUser.IsServiceUser {
+		am.deleteServiceUser(account, initiatorUserID, targetUser)
+		return am.Store.SaveAccount(account)
+	}
+
+	return am.deleteRegularUser(account, initiatorUserID, targetUserID)
+}
+
+func (am *DefaultAccountManager) deleteRegularUser(account *Account, initiatorUserID, targetUserID string) error {
+	tuEmail, tuName, err := am.getEmailAndNameOfTargetUser(account.Id, initiatorUserID, targetUserID)
+	if err != nil {
+		log.Errorf("failed to resolve email address: %s", err)
+		return err
+	}
+
+	if !isNil(am.idpManager) {
+		err = am.deleteUserFromIDP(targetUserID, account.Id)
+		if err != nil {
+			log.Debugf("failed to delete user from IDP: %s", targetUserID)
+			return err
+		}
+	}
+
+	err = am.deleteUserPeers(initiatorUserID, targetUserID, account)
+	if err != nil {
+		return err
+	}
+
+	delete(account.Users, targetUserID)
+	err = am.Store.SaveAccount(account)
+	if err != nil {
+		return err
+	}
+
+	meta := map[string]any{"name": tuName, "email": tuEmail}
+	am.storeEvent(initiatorUserID, targetUserID, account.Id, activity.UserDeleted, meta)
+
+	return am.updateAccountPeers(account)
+}
+
+func (am *DefaultAccountManager) deleteUserPeers(initiatorUserID string, targetUserID string, account *Account) error {
 	peers, err := account.FindUserPeers(targetUserID)
 	if err != nil {
 		return status.Errorf(status.Internal, "failed to find user peers")
@@ -343,45 +391,7 @@ func (am *DefaultAccountManager) DeleteUser(accountID, initiatorUserID string, t
 		peerIDs = append(peerIDs, peer.ID)
 	}
 
-	err = am.deletePeers(account, peerIDs, initiatorUserID)
+	return am.deletePeers(account, peerIDs, initiatorUserID)
-	if err != nil {
-		return err
-	}
-
-	tuEmail, tuName, err := am.getEmailAndNameOfTargetUser(account.Id, initiatorUserID, targetUserID)
-	if err != nil {
-		log.Errorf("failed to resolve email address: %s", err)
-		return err
-	}
-
-	var meta map[string]any
-	var eventAction activity.Activity
-	if targetUser.IsServiceUser {
-		meta = map[string]any{"name": targetUser.ServiceUserName}
-		eventAction = activity.ServiceUserDeleted
-	} else {
-		meta = map[string]any{"name": tuName, "email": tuEmail}
-		eventAction = activity.UserDeleted
-	}
-	am.storeEvent(initiatorUserID, targetUserID, accountID, eventAction, meta)
-
-	if !targetUser.IsServiceUser && !isNil(am.idpManager) {
-		err := am.deleteUserFromIDP(targetUserID, accountID)
-		if err != nil {
-			log.Debugf("failed to delete user from IDP: %s", targetUserID)
-			return err
-		}
-	}
-
-	delete(account.Users, targetUserID)
-
-	// todo should be unnecessary because we save account in the am.deletePeers
-	err = am.Store.SaveAccount(account)
-	if err != nil {
-		return err
-	}
-
-	return nil
 }
 
 // InviteUser resend invitations to users who haven't activated their accounts prior to the expiration period.