Delete user peers when deleting a user (#1186)

management/server/account.go

@@ -66,7 +66,7 @@ type AccountManager interface {
 	GetPeerByKey(peerKey string) (*Peer, error)
 	GetPeers(accountID, userID string) ([]*Peer, error)
 	MarkPeerConnected(peerKey string, connected bool) error
-	DeletePeer(accountID, peerID, userID string) (*Peer, error)
+	DeletePeer(accountID, peerID, userID string) error
 	GetPeerByIP(accountId string, peerIP string) (*Peer, error)
 	UpdatePeer(accountID, userID string, peer *Peer) (*Peer, error)
 	GetNetworkMap(peerID string) (*NetworkMap, error)

management/server/account_test.go

@@ -1062,7 +1062,7 @@ func TestAccountManager_NetworkUpdates(t *testing.T) {
 			}
 		}()
 
-		if _, err := manager.DeletePeer(account.Id, peer3.ID, userID); err != nil {
+		if err := manager.DeletePeer(account.Id, peer3.ID, userID); err != nil {
 			t.Errorf("delete peer: %v", err)
 			return
 		}
@@ -1129,7 +1129,7 @@ func TestAccountManager_DeletePeer(t *testing.T) {
 		return
 	}
 
-	_, err = manager.DeletePeer(account.Id, peerKey, userID)
+	err = manager.DeletePeer(account.Id, peerKey, userID)
 	if err != nil {
 		return
 	}

management/server/ephemeral.go

@@ -162,7 +162,7 @@ func (e *EphemeralManager) cleanup() {
 
 	for id, p := range deletePeers {
 		log.Debugf("delete ephemeral peer: %s", id)
-		_, err := e.accountManager.DeletePeer(p.account.Id, id, activity.SystemInitiator)
+		err := e.accountManager.DeletePeer(p.account.Id, id, activity.SystemInitiator)
 		if err != nil {
 			log.Tracef("failed to delete ephemeral peer: %s", err)
 		}

management/server/ephemeral_test.go

@@ -29,9 +29,9 @@ type MocAccountManager struct {
 	store *MockStore
 }
 
-func (a MocAccountManager) DeletePeer(accountID, peerID, userID string) (*Peer, error) {
+func (a MocAccountManager) DeletePeer(accountID, peerID, userID string) error {
 	delete(a.store.account.Peers, peerID)
-	return nil, nil //nolint:nilnil
+	return nil //nolint:nil
 }
 
 func TestNewManager(t *testing.T) {

management/server/http/peers_handler.go

@@ -61,7 +61,7 @@ func (h *PeersHandler) updatePeer(account *server.Account, user *server.User, pe
 }
 
 func (h *PeersHandler) deletePeer(accountID, userID string, peerID string, w http.ResponseWriter) {
-	_, err := h.accountManager.DeletePeer(accountID, peerID, userID)
+	err := h.accountManager.DeletePeer(accountID, peerID, userID)
 	if err != nil {
 		util.WriteError(err, w)
 		return

management/server/mock_server/account_mock.go

@@ -24,7 +24,7 @@ type MockAccountManager struct {
 	GetPeerByKeyFunc                func(peerKey string) (*server.Peer, error)
 	GetPeersFunc                    func(accountID, userID string) ([]*server.Peer, error)
 	MarkPeerConnectedFunc           func(peerKey string, connected bool) error
-	DeletePeerFunc                  func(accountID, peerKey, userID string) (*server.Peer, error)
+	DeletePeerFunc                  func(accountID, peerKey, userID string) error
 	GetPeerByIPFunc                 func(accountId string, peerIP string) (*server.Peer, error)
 	GetNetworkMapFunc               func(peerKey string) (*server.NetworkMap, error)
 	GetPeerNetworkFunc              func(peerKey string) (*server.Network, error)
@@ -90,11 +90,11 @@ func (am *MockAccountManager) GetUsersFromAccount(accountID string, userID strin
 }
 
 // DeletePeer mock implementation of DeletePeer from server.AccountManager interface
-func (am *MockAccountManager) DeletePeer(accountID, peerID, userID string) (*server.Peer, error) {
+func (am *MockAccountManager) DeletePeer(accountID, peerID, userID string) error {
 	if am.DeletePeerFunc != nil {
 		return am.DeletePeerFunc(accountID, peerID, userID)
 	}
-	return nil, status.Errorf(codes.Unimplemented, "method DeletePeer is not implemented")
+	return status.Errorf(codes.Unimplemented, "method DeletePeer is not implemented")
 }
 
 // GetOrCreateAccountByUser mock implementation of GetOrCreateAccountByUser from server.AccountManager interface

management/server/peer.go

@@ -372,29 +372,21 @@ func (am *DefaultAccountManager) UpdatePeer(accountID, userID string, update *Pe
 	return peer, nil
 }
 
-// DeletePeer removes peer from the account by its IP
+// deletePeers will delete all specified peers and send updates to the remote peers. Don't call without acquiring account lock
-func (am *DefaultAccountManager) DeletePeer(accountID, peerID, userID string) (*Peer, error) {
+func (am *DefaultAccountManager) deletePeers(account *Account, peerIDs []string, userID string) error {
-	unlock := am.Store.AcquireAccountLock(accountID)
-	defer unlock()
 
-	account, err := am.Store.GetAccount(accountID)
+	var peerError error
-	if err != nil {
+	for _, peerID := range peerIDs {
-		return nil, err
-	}
 
 		peer := account.GetPeer(peerID)
 		if peer == nil {
-		return nil, status.Errorf(status.NotFound, "peer %s not found", peerID)
+			peerError = status.Errorf(status.NotFound, "peer %s not found", peerID)
+			goto save
 		}
 
 		account.DeletePeer(peerID)
 
-	err = am.Store.SaveAccount(account)
+		peerError = am.peersUpdateManager.SendUpdate(peer.ID,
-	if err != nil {
-		return nil, err
-	}
-
-	err = am.peersUpdateManager.SendUpdate(peer.ID,
 			&UpdateMessage{
 				Update: &proto.SyncResponse{
 					// fill those field for backward compatibility
@@ -410,17 +402,49 @@ func (am *DefaultAccountManager) DeletePeer(accountID, peerID, userID string) (*
 					},
 				},
 			})
-	if err != nil {
+		if peerError != nil {
-		return nil, err
+			goto save
-	}
-
-	if err := am.updateAccountPeers(account); err != nil {
-		return nil, err
 		}
 
 		am.peersUpdateManager.CloseChannel(peerID)
 		am.storeEvent(userID, peer.ID, account.Id, activity.PeerRemovedByUser, peer.EventMeta(am.GetDNSDomain()))
-	return peer, nil
+	}
+
+save:
+	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
+func (am *DefaultAccountManager) DeletePeer(accountID, peerID, userID string) error {
+	unlock := am.Store.AcquireAccountLock(accountID)
+	defer unlock()
+
+	account, err := am.Store.GetAccount(accountID)
+	if err != nil {
+		return err
+	}
+
+	return am.deletePeers(account, []string{peerID}, userID)
 }
 
 // GetPeerByIP returns peer by its IP

management/server/user.go

@@ -338,8 +338,13 @@ func (am *DefaultAccountManager) DeleteUser(accountID, initiatorUserID string, t
 		return status.Errorf(status.Internal, "failed to find user peers")
 	}
 
-	if err := am.expireAndUpdatePeers(account, peers); err != nil {
+	peerIDs := make([]string, 0, len(peers))
-		log.Errorf("failed update deleted peers expiration: %s", err)
+	for _, peer := range peers {
+		peerIDs = append(peerIDs, peer.ID)
+	}
+
+	err = am.deletePeers(account, peerIDs, initiatorUserID)
+	if err != nil {
 		return err
 	}
 
@@ -370,6 +375,7 @@ func (am *DefaultAccountManager) DeleteUser(accountID, initiatorUserID string, t
 
 	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