From c2eaf8a1c0984d2a8beed5b322e6b6be2bc9eaf9 Mon Sep 17 00:00:00 2001 From: Maycon Santos Date: Tue, 28 Nov 2023 14:23:38 +0100 Subject: [PATCH] Add account deletion endpoint (#1331) Adding support to account owners to delete an account This will remove all users from local, and if --user-delete-from-idp is set it will remove from the remote IDP --- management/server/account.go | 52 +++++++++++++ management/server/account_test.go | 25 ++++++ management/server/file_store.go | 35 +++++++++ management/server/file_store_test.go | 54 +++++++++++++ management/server/http/accounts_handler.go | 24 ++++++ management/server/http/api/openapi.yml | 26 +++++++ management/server/http/handler.go | 2 + management/server/http/users_handler.go | 2 +- management/server/mock_server/account_mock.go | 9 +++ management/server/sqlite_store.go | 42 ++++++++-- management/server/sqlite_store_test.go | 77 ++++++++++++++++++- management/server/store.go | 1 + 12 files changed, 342 insertions(+), 7 deletions(-) diff --git a/management/server/account.go b/management/server/account.go index 09071d103b4..cbdb247db14 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -64,6 +64,7 @@ type AccountManager interface { GetAccountByUserOrAccountID(userID, accountID, domain string) (*Account, error) GetAccountFromToken(claims jwtclaims.AuthorizationClaims) (*Account, *User, error) GetAccountFromPAT(pat string) (*Account, *User, *PersonalAccessToken, error) + DeleteAccount(accountID, userID string) error MarkPATUsed(tokenID string) error GetUser(claims jwtclaims.AuthorizationClaims) (*User, error) ListUsers(accountID string) ([]*User, error) @@ -1004,6 +1005,57 @@ func (am *DefaultAccountManager) warmupIDPCache() error { return nil } +// DeleteAccount deletes an account and all its users from local store and from the remote IDP if the requester is an admin and account owner +func (am *DefaultAccountManager) DeleteAccount(accountID, userID string) error { + unlock := am.Store.AcquireAccountLock(accountID) + defer unlock() + account, err := am.Store.GetAccount(accountID) + if err != nil { + return err + } + + user, err := account.FindUser(userID) + if err != nil { + return err + } + + if !user.IsAdmin() { + return status.Errorf(status.PermissionDenied, "user is not allowed to delete account") + } + + if user.Id != account.CreatedBy { + return status.Errorf(status.PermissionDenied, "user is not allowed to delete account. Only account owner can delete account") + } + for _, otherUser := range account.Users { + if otherUser.IsServiceUser { + continue + } + + if otherUser.Id == userID { + continue + } + + deleteUserErr := am.deleteRegularUser(account, userID, otherUser.Id) + if deleteUserErr != nil { + return deleteUserErr + } + } + + err = am.deleteRegularUser(account, userID, userID) + if err != nil { + log.Errorf("failed deleting user %s. error: %s", userID, err) + return err + } + + err = am.Store.DeleteAccount(account) + if err != nil { + log.Errorf("failed deleting account %s. error: %s", accountID, err) + return err + } + log.Debugf("account %s deleted", accountID) + return nil +} + // GetAccountByUserOrAccountID looks for an account by user or accountID, if no account is provided and // userID doesn't have an account associated with it, one account is created func (am *DefaultAccountManager) GetAccountByUserOrAccountID(userID, accountID, domain string) (*Account, error) { diff --git a/management/server/account_test.go b/management/server/account_test.go index ad0ccfbcea7..3343acbde77 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -746,6 +746,31 @@ func TestAccountManager_GetAccount(t *testing.T) { } } +func TestAccountManager_DeleteAccount(t *testing.T) { + manager, err := createManager(t) + if err != nil { + t.Fatal(err) + return + } + + expectedId := "test_account" + userId := "account_creator" + account, err := createAccount(manager, expectedId, userId, "") + if err != nil { + t.Fatal(err) + } + + err = manager.DeleteAccount(account.Id, userId) + if err != nil { + t.Fatal(err) + } + + getAccount, err := manager.Store.GetAccount(account.Id) + if err == nil { + t.Fatal(fmt.Errorf("expected to get an error when trying to get deleted account, got %v", getAccount)) + } +} + func TestAccountManager_AddPeer(t *testing.T) { manager, err := createManager(t) if err != nil { diff --git a/management/server/file_store.go b/management/server/file_store.go index 73c52927e81..97fdc9a92e0 100644 --- a/management/server/file_store.go +++ b/management/server/file_store.go @@ -351,6 +351,41 @@ func (s *FileStore) SaveAccount(account *Account) error { return s.persist(s.storeFile) } +func (s *FileStore) DeleteAccount(account *Account) error { + s.mux.Lock() + defer s.mux.Unlock() + + if account.Id == "" { + return status.Errorf(status.InvalidArgument, "account id should not be empty") + } + + for keyID := range account.SetupKeys { + delete(s.SetupKeyID2AccountID, strings.ToUpper(keyID)) + } + + // enforce peer to account index and delete peer to route indexes for rebuild + for _, peer := range account.Peers { + delete(s.PeerKeyID2AccountID, peer.Key) + delete(s.PeerID2AccountID, peer.ID) + } + + for _, user := range account.Users { + for _, pat := range user.PATs { + delete(s.TokenID2UserID, pat.ID) + delete(s.HashedPAT2TokenID, pat.HashedToken) + } + delete(s.UserID2AccountID, user.Id) + } + + if account.DomainCategory == PrivateCategory && account.IsDomainPrimaryAccount { + delete(s.PrivateDomain2AccountID, account.Domain) + } + + delete(s.Accounts, account.Id) + + return s.persist(s.storeFile) +} + // DeleteHashedPAT2TokenIDIndex removes an entry from the indexing map HashedPAT2TokenID func (s *FileStore) DeleteHashedPAT2TokenIDIndex(hashedToken string) error { s.mux.Lock() diff --git a/management/server/file_store_test.go b/management/server/file_store_test.go index 206873d2022..f5baf985852 100644 --- a/management/server/file_store_test.go +++ b/management/server/file_store_test.go @@ -121,6 +121,60 @@ func TestSaveAccount(t *testing.T) { } } +func TestDeleteAccount(t *testing.T) { + storeDir := t.TempDir() + storeFile := filepath.Join(storeDir, "store.json") + err := util.CopyFileContents("testdata/store.json", storeFile) + if err != nil { + t.Fatal(err) + } + + store, err := NewFileStore(storeDir, nil) + if err != nil { + t.Fatal(err) + } + var account *Account + for _, a := range store.Accounts { + account = a + break + } + + require.NotNil(t, account, "failed to restore a FileStore file and get at least one account") + + err = store.DeleteAccount(account) + require.NoError(t, err, "failed to delete account, error: %v", err) + + _, ok := store.Accounts[account.Id] + require.False(t, ok, "failed to delete account") + + for id := range account.Users { + _, ok := store.UserID2AccountID[id] + assert.False(t, ok, "failed to delete UserID2AccountID index") + for _, pat := range account.Users[id].PATs { + _, ok := store.HashedPAT2TokenID[pat.HashedToken] + assert.False(t, ok, "failed to delete HashedPAT2TokenID index") + _, ok = store.TokenID2UserID[pat.ID] + assert.False(t, ok, "failed to delete TokenID2UserID index") + } + } + + for _, p := range account.Peers { + _, ok := store.PeerKeyID2AccountID[p.Key] + assert.False(t, ok, "failed to delete PeerKeyID2AccountID index") + _, ok = store.PeerID2AccountID[p.ID] + assert.False(t, ok, "failed to delete PeerID2AccountID index") + } + + for id := range account.SetupKeys { + _, ok := store.SetupKeyID2AccountID[id] + assert.False(t, ok, "failed to delete SetupKeyID2AccountID index") + } + + _, ok = store.PrivateDomain2AccountID[account.Domain] + assert.False(t, ok, "failed to delete PrivateDomain2AccountID index") + +} + func TestStore(t *testing.T) { store := newStore(t) diff --git a/management/server/http/accounts_handler.go b/management/server/http/accounts_handler.go index a5d7a9501c4..e5e0dff8d85 100644 --- a/management/server/http/accounts_handler.go +++ b/management/server/http/accounts_handler.go @@ -98,6 +98,30 @@ func (h *AccountsHandler) UpdateAccount(w http.ResponseWriter, r *http.Request) util.WriteJSONObject(w, &resp) } +// DeleteAccount is a HTTP DELETE handler to delete an account +func (h *AccountsHandler) DeleteAccount(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodDelete { + util.WriteErrorResponse("wrong HTTP method", http.StatusMethodNotAllowed, w) + return + } + + claims := h.claimsExtractor.FromRequestContext(r) + vars := mux.Vars(r) + targetAccountID := vars["accountId"] + if len(targetAccountID) == 0 { + util.WriteError(status.Errorf(status.InvalidArgument, "invalid account ID"), w) + return + } + + err := h.accountManager.DeleteAccount(targetAccountID, claims.UserId) + if err != nil { + util.WriteError(err, w) + return + } + + util.WriteJSONObject(w, emptyObject{}) +} + func toAccountResponse(account *server.Account) *api.Account { return &api.Account{ Id: account.Id, diff --git a/management/server/http/api/openapi.yml b/management/server/http/api/openapi.yml index 64e97426a61..0ed8261480f 100644 --- a/management/server/http/api/openapi.yml +++ b/management/server/http/api/openapi.yml @@ -1074,6 +1074,32 @@ paths: '500': "$ref": "#/components/responses/internal_error" /api/accounts/{accountId}: + delete: + summary: Delete an Account + description: Deletes an account and all its resources. Only administrators and account owners can delete accounts. + tags: [ Accounts ] + security: + - BearerAuth: [ ] + - TokenAuth: [ ] + parameters: + - in: path + name: accountId + required: true + schema: + type: string + description: The unique identifier of an account + responses: + '200': + description: Delete account status code + content: { } + '400': + "$ref": "#/components/responses/bad_request" + '401': + "$ref": "#/components/responses/requires_authentication" + '403': + "$ref": "#/components/responses/forbidden" + '500': + "$ref": "#/components/responses/internal_error" put: summary: Update an Account description: Update information about an account diff --git a/management/server/http/handler.go b/management/server/http/handler.go index c589512e5d4..8c77d27dc4a 100644 --- a/management/server/http/handler.go +++ b/management/server/http/handler.go @@ -7,6 +7,7 @@ import ( "github.com/rs/cors" "github.com/netbirdio/management-integrations/integrations" + s "github.com/netbirdio/netbird/management/server" "github.com/netbirdio/netbird/management/server/http/middleware" "github.com/netbirdio/netbird/management/server/jwtclaims" @@ -105,6 +106,7 @@ func APIHandler(accountManager s.AccountManager, jwtValidator jwtclaims.JWTValid func (apiHandler *apiHandler) addAccountsEndpoint() { accountsHandler := NewAccountsHandler(apiHandler.AccountManager, apiHandler.AuthCfg) apiHandler.Router.HandleFunc("/accounts/{accountId}", accountsHandler.UpdateAccount).Methods("PUT", "OPTIONS") + apiHandler.Router.HandleFunc("/accounts/{accountId}", accountsHandler.DeleteAccount).Methods("DELETE", "OPTIONS") apiHandler.Router.HandleFunc("/accounts", accountsHandler.GetAllAccounts).Methods("GET", "OPTIONS") } diff --git a/management/server/http/users_handler.go b/management/server/http/users_handler.go index e474ac19ab8..5d92b65e5d8 100644 --- a/management/server/http/users_handler.go +++ b/management/server/http/users_handler.go @@ -94,7 +94,7 @@ func (h *UsersHandler) UpdateUser(w http.ResponseWriter, r *http.Request) { util.WriteJSONObject(w, toUserResponse(newUser, claims.UserId)) } -// DeleteUser is a DELETE request to delete a user (only works for service users right now) +// DeleteUser is a DELETE request to delete a user func (h *UsersHandler) DeleteUser(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodDelete { util.WriteErrorResponse("wrong HTTP method", http.StatusMethodNotAllowed, w) diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index f6b2e1641d2..84b23a4f257 100644 --- a/management/server/mock_server/account_mock.go +++ b/management/server/mock_server/account_mock.go @@ -68,6 +68,7 @@ type MockAccountManager struct { ListNameServerGroupsFunc func(accountID string) ([]*nbdns.NameServerGroup, error) CreateUserFunc func(accountID, userID string, key *server.UserInfo) (*server.UserInfo, error) GetAccountFromTokenFunc func(claims jwtclaims.AuthorizationClaims) (*server.Account, *server.User, error) + DeleteAccountFunc func(accountID, userID string) error GetDNSDomainFunc func() string StoreEventFunc func(initiatorID, targetID, accountID string, activityID activity.Activity, meta map[string]any) GetEventsFunc func(accountID, userID string) ([]*activity.Event, error) @@ -157,6 +158,14 @@ func (am *MockAccountManager) GetAccountFromPAT(pat string) (*server.Account, *s return nil, nil, nil, status.Errorf(codes.Unimplemented, "method GetAccountFromPAT is not implemented") } +// DeleteAccount mock implementation of DeleteAccount from server.AccountManager interface +func (am *MockAccountManager) DeleteAccount(accountID, userID string) error { + if am.DeleteAccountFunc != nil { + return am.DeleteAccountFunc(accountID, userID) + } + return status.Errorf(codes.Unimplemented, "method DeleteAccount is not implemented") +} + // MarkPATUsed mock implementation of MarkPATUsed from server.AccountManager interface func (am *MockAccountManager) MarkPATUsed(pat string) error { if am.MarkPATUsedFunc != nil { diff --git a/management/server/sqlite_store.go b/management/server/sqlite_store.go index bbb13f8c6db..0cd0abe4a47 100644 --- a/management/server/sqlite_store.go +++ b/management/server/sqlite_store.go @@ -7,15 +7,16 @@ import ( "sync" "time" - nbdns "github.com/netbirdio/netbird/dns" - "github.com/netbirdio/netbird/management/server/status" - "github.com/netbirdio/netbird/management/server/telemetry" - "github.com/netbirdio/netbird/route" log "github.com/sirupsen/logrus" "gorm.io/driver/sqlite" "gorm.io/gorm" "gorm.io/gorm/clause" "gorm.io/gorm/logger" + + nbdns "github.com/netbirdio/netbird/dns" + "github.com/netbirdio/netbird/management/server/status" + "github.com/netbirdio/netbird/management/server/telemetry" + "github.com/netbirdio/netbird/route" ) // SqliteStore represents an account storage backed by a Sqlite DB persisted to disk @@ -202,6 +203,37 @@ func (s *SqliteStore) SaveAccount(account *Account) error { return err } +func (s *SqliteStore) DeleteAccount(account *Account) error { + start := time.Now() + + err := s.db.Transaction(func(tx *gorm.DB) error { + result := tx.Select(clause.Associations).Delete(account.Policies, "account_id = ?", account.Id) + if result.Error != nil { + return result.Error + } + + result = tx.Select(clause.Associations).Delete(account.UsersG, "account_id = ?", account.Id) + if result.Error != nil { + return result.Error + } + + result = tx.Select(clause.Associations).Delete(account) + if result.Error != nil { + return result.Error + } + + return nil + }) + + took := time.Since(start) + if s.metrics != nil { + s.metrics.StoreMetrics().CountPersistenceDuration(took) + } + log.Debugf("took %d ms to delete an account to the SQLite", took.Milliseconds()) + + return err +} + func (s *SqliteStore) SaveInstallationID(ID string) error { installation := installation{InstallationIDValue: ID} installation.ID = uint(s.installationPK) @@ -336,7 +368,7 @@ func (s *SqliteStore) GetAccount(accountID string) (*Account, error) { var rules []*PolicyRule err := s.db.Model(&PolicyRule{}).Find(&rules, "policy_id = ?", policy.ID).Error if err != nil { - return nil, status.Errorf(status.NotFound, "account not found") + return nil, status.Errorf(status.NotFound, "rule not found") } account.Policies[i].Rules = rules } diff --git a/management/server/sqlite_store_test.go b/management/server/sqlite_store_test.go index 4a16e25255d..eef469f40d7 100644 --- a/management/server/sqlite_store_test.go +++ b/management/server/sqlite_store_test.go @@ -9,9 +9,10 @@ import ( "time" "github.com/google/uuid" - "github.com/netbirdio/netbird/util" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/util" ) func TestSqlite_NewStore(t *testing.T) { @@ -98,6 +99,80 @@ func TestSqlite_SaveAccount(t *testing.T) { } } +func TestSqlite_DeleteAccount(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("The SQLite store is not properly supported by Windows yet") + } + + store := newSqliteStore(t) + + testUserID := "testuser" + user := NewAdminUser(testUserID) + user.PATs = map[string]*PersonalAccessToken{"testtoken": { + ID: "testtoken", + Name: "test token", + }} + + account := newAccountWithId("account_id", testUserID, "") + setupKey := GenerateDefaultSetupKey() + account.SetupKeys[setupKey.Key] = setupKey + account.Peers["testpeer"] = &Peer{ + Key: "peerkey", + SetupKey: "peerkeysetupkey", + IP: net.IP{127, 0, 0, 1}, + Meta: PeerSystemMeta{}, + Name: "peer name", + Status: &PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, + } + account.Users[testUserID] = user + + err := store.SaveAccount(account) + require.NoError(t, err) + + if len(store.GetAllAccounts()) != 1 { + t.Errorf("expecting 1 Accounts to be stored after SaveAccount()") + } + + err = store.DeleteAccount(account) + require.NoError(t, err) + + if len(store.GetAllAccounts()) != 0 { + t.Errorf("expecting 0 Accounts to be stored after DeleteAccount()") + } + + _, err = store.GetAccountByPeerPubKey("peerkey") + require.Error(t, err, "expecting error after removing DeleteAccount when getting account by peer public key") + + _, err = store.GetAccountByUser("testuser") + require.Error(t, err, "expecting error after removing DeleteAccount when getting account by user") + + _, err = store.GetAccountByPeerID("testpeer") + require.Error(t, err, "expecting error after removing DeleteAccount when getting account by peer id") + + _, err = store.GetAccountBySetupKey(setupKey.Key) + require.Error(t, err, "expecting error after removing DeleteAccount when getting account by setup key") + + _, err = store.GetAccount(account.Id) + require.Error(t, err, "expecting error after removing DeleteAccount when getting account by id") + + for _, policy := range account.Policies { + var rules []*PolicyRule + err = store.db.Model(&PolicyRule{}).Find(&rules, "policy_id = ?", policy.ID).Error + require.NoError(t, err, "expecting no error after removing DeleteAccount when searching for policy rules") + require.Len(t, rules, 0, "expecting no policy rules to be found after removing DeleteAccount") + + } + + for _, accountUser := range account.Users { + var pats []*PersonalAccessToken + err = store.db.Model(&PersonalAccessToken{}).Find(&pats, "user_id = ?", accountUser.Id).Error + require.NoError(t, err, "expecting no error after removing DeleteAccount when searching for personal access token") + require.Len(t, pats, 0, "expecting no personal access token to be found after removing DeleteAccount") + + } + +} + func TestSqlite_SavePeerStatus(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("The SQLite store is not properly supported by Windows yet") diff --git a/management/server/store.go b/management/server/store.go index 66b239f9614..25511539a28 100644 --- a/management/server/store.go +++ b/management/server/store.go @@ -14,6 +14,7 @@ import ( type Store interface { GetAllAccounts() []*Account GetAccount(accountID string) (*Account, error) + DeleteAccount(account *Account) error GetAccountByUser(userID string) (*Account, error) GetAccountByPeerPubKey(peerKey string) (*Account, error) GetAccountByPeerID(peerID string) (*Account, error)