From b23011fbe80342ab3a7ec8ab8610073aa0a9168e Mon Sep 17 00:00:00 2001 From: Misha Bragin Date: Sun, 1 Oct 2023 19:51:39 +0200 Subject: [PATCH] Delete user peers when deleting a user (#1186) --- management/server/account.go | 2 +- management/server/account_test.go | 4 +- management/server/ephemeral.go | 2 +- management/server/ephemeral_test.go | 4 +- management/server/http/peers_handler.go | 2 +- management/server/mock_server/account_mock.go | 6 +- management/server/peer.go | 106 +++++++++++------- management/server/user.go | 10 +- 8 files changed, 83 insertions(+), 53 deletions(-) diff --git a/management/server/account.go b/management/server/account.go index b4f939166..d186beb66 100644 --- a/management/server/account.go +++ b/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) diff --git a/management/server/account_test.go b/management/server/account_test.go index 8c8a1b001..5f711df80 100644 --- a/management/server/account_test.go +++ b/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 } diff --git a/management/server/ephemeral.go b/management/server/ephemeral.go index a7b423983..0e76e58ac 100644 --- a/management/server/ephemeral.go +++ b/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) } diff --git a/management/server/ephemeral_test.go b/management/server/ephemeral_test.go index a763f4cef..d271e5fca 100644 --- a/management/server/ephemeral_test.go +++ b/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) { diff --git a/management/server/http/peers_handler.go b/management/server/http/peers_handler.go index 100549aad..adf4a9721 100644 --- a/management/server/http/peers_handler.go +++ b/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 diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index ed1130e09..889bbb221 100644 --- a/management/server/mock_server/account_mock.go +++ b/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 diff --git a/management/server/peer.go b/management/server/peer.go index f9631719f..31ebd7cd5 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -372,55 +372,79 @@ func (am *DefaultAccountManager) UpdatePeer(accountID, userID string, update *Pe return peer, nil } +// 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 + for _, peerID := range peerIDs { + + peer := account.GetPeer(peerID) + if peer == nil { + peerError = status.Errorf(status.NotFound, "peer %s not found", peerID) + goto save + } + + account.DeletePeer(peerID) + + peerError = am.peersUpdateManager.SendUpdate(peer.ID, + &UpdateMessage{ + Update: &proto.SyncResponse{ + // fill those field for backward compatibility + RemotePeers: []*proto.RemotePeerConfig{}, + RemotePeersIsEmpty: true, + // new field + NetworkMap: &proto.NetworkMap{ + Serial: account.Network.CurrentSerial(), + RemotePeers: []*proto.RemotePeerConfig{}, + RemotePeersIsEmpty: true, + FirewallRules: []*proto.FirewallRule{}, + FirewallRulesIsEmpty: true, + }, + }, + }) + if peerError != nil { + goto save + } + + am.peersUpdateManager.CloseChannel(peerID) + am.storeEvent(userID, peer.ID, account.Id, activity.PeerRemovedByUser, peer.EventMeta(am.GetDNSDomain())) + } + +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) (*Peer, error) { +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 nil, err + return err } - peer := account.GetPeer(peerID) - if peer == nil { - return nil, status.Errorf(status.NotFound, "peer %s not found", peerID) - } - - account.DeletePeer(peerID) - - err = am.Store.SaveAccount(account) - if err != nil { - return nil, err - } - - err = am.peersUpdateManager.SendUpdate(peer.ID, - &UpdateMessage{ - Update: &proto.SyncResponse{ - // fill those field for backward compatibility - RemotePeers: []*proto.RemotePeerConfig{}, - RemotePeersIsEmpty: true, - // new field - NetworkMap: &proto.NetworkMap{ - Serial: account.Network.CurrentSerial(), - RemotePeers: []*proto.RemotePeerConfig{}, - RemotePeersIsEmpty: true, - FirewallRules: []*proto.FirewallRule{}, - FirewallRulesIsEmpty: true, - }, - }, - }) - if err != nil { - return nil, err - } - - 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 + return am.deletePeers(account, []string{peerID}, userID) } // GetPeerByIP returns peer by its IP diff --git a/management/server/user.go b/management/server/user.go index 9466a801b..a20a8b2b0 100644 --- a/management/server/user.go +++ b/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 { - log.Errorf("failed update deleted peers expiration: %s", err) + peerIDs := make([]string, 0, len(peers)) + 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