From f8a0a7e9539a44b2f790a08eb1b03028b56eaac3 Mon Sep 17 00:00:00 2001
From: kolaente <k@knt.li>
Date: Mon, 22 Nov 2021 22:34:20 +0100
Subject: [PATCH] fix: deleting users with no namespaces

resolves #984
---
 pkg/models/user_delete.go      | 57 +++++++++++++++++++++++++---------
 pkg/models/user_delete_test.go | 44 +++++++++++++++++---------
 2 files changed, 71 insertions(+), 30 deletions(-)

diff --git a/pkg/models/user_delete.go b/pkg/models/user_delete.go
index 69327baae..c527121ab 100644
--- a/pkg/models/user_delete.go
+++ b/pkg/models/user_delete.go
@@ -87,17 +87,18 @@ func deleteUsers() {
 	}
 }
 
-// DeleteUser completely removes a user and all their associated lists, namespaces and tasks.
-// This action is irrevocable.
-// Public to allow deletion from the CLI.
-func DeleteUser(s *xorm.Session, u *user.User) (err error) {
-	namespacesToDelete := []*Namespace{}
-	// Get all namespaces and lists this u has access to
+func getNamespacesToDelete(s *xorm.Session, u *user.User) (namespacesToDelete []*Namespace, err error) {
+	namespacesToDelete = []*Namespace{}
 	nm := &Namespace{IsArchived: true}
 	res, _, _, err := nm.ReadAll(s, u, "", 1, -1)
 	if err != nil {
-		return err
+		return nil, err
 	}
+
+	if res == nil {
+		return nil, nil
+	}
+
 	namespaces := res.([]*NamespaceWithLists)
 	for _, n := range namespaces {
 		if n.ID < 0 {
@@ -106,14 +107,14 @@ func DeleteUser(s *xorm.Session, u *user.User) (err error) {
 
 		hadUsers, err := ensureNamespaceAdminUser(s, &n.Namespace)
 		if err != nil {
-			return err
+			return nil, err
 		}
 		if hadUsers {
 			continue
 		}
 		hadTeams, err := ensureNamespaceAdminTeam(s, &n.Namespace)
 		if err != nil {
-			return err
+			return nil, err
 		}
 		if hadTeams {
 			continue
@@ -122,13 +123,21 @@ func DeleteUser(s *xorm.Session, u *user.User) (err error) {
 		namespacesToDelete = append(namespacesToDelete, &n.Namespace)
 	}
 
-	// Get all lists to delete
-	listsToDelete := []*List{}
+	return
+}
+
+func getListsToDelete(s *xorm.Session, u *user.User) (listsToDelete []*List, err error) {
+	listsToDelete = []*List{}
 	lm := &List{IsArchived: true}
-	res, _, _, err = lm.ReadAll(s, u, "", 0, -1)
+	res, _, _, err := lm.ReadAll(s, u, "", 0, -1)
 	if err != nil {
-		return err
+		return nil, err
 	}
+
+	if res == nil {
+		return nil, nil
+	}
+
 	lists := res.([]*List)
 	for _, l := range lists {
 		if l.ID < 0 {
@@ -137,15 +146,16 @@ func DeleteUser(s *xorm.Session, u *user.User) (err error) {
 
 		hadUsers, err := ensureListAdminUser(s, l)
 		if err != nil {
-			return err
+			return nil, err
 		}
 		if hadUsers {
 			continue
 		}
 		hadTeams, err := ensureListAdminTeam(s, l)
 		if err != nil {
-			return err
+			return nil, err
 		}
+
 		if hadTeams {
 			continue
 		}
@@ -153,6 +163,23 @@ func DeleteUser(s *xorm.Session, u *user.User) (err error) {
 		listsToDelete = append(listsToDelete, l)
 	}
 
+	return
+}
+
+// DeleteUser completely removes a user and all their associated lists, namespaces and tasks.
+// This action is irrevocable.
+// Public to allow deletion from the CLI.
+func DeleteUser(s *xorm.Session, u *user.User) (err error) {
+	namespacesToDelete, err := getNamespacesToDelete(s, u)
+	if err != nil {
+		return err
+	}
+
+	listsToDelete, err := getListsToDelete(s, u)
+	if err != nil {
+		return err
+	}
+
 	// Delete everything not shared with anybody else
 	for _, n := range namespacesToDelete {
 		err = deleteNamespace(s, n, u, false)
diff --git a/pkg/models/user_delete_test.go b/pkg/models/user_delete_test.go
index 151dd2cc2..bed04ffd7 100644
--- a/pkg/models/user_delete_test.go
+++ b/pkg/models/user_delete_test.go
@@ -27,21 +27,35 @@ import (
 )
 
 func TestDeleteUser(t *testing.T) {
-	db.LoadAndAssertFixtures(t)
-	s := db.NewSession()
-	defer s.Close()
-	notifications.Fake()
+	t.Run("normal", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+		notifications.Fake()
 
-	u := &user.User{ID: 6}
-	err := DeleteUser(s, u)
+		u := &user.User{ID: 6}
+		err := DeleteUser(s, u)
 
-	assert.NoError(t, err)
-	db.AssertMissing(t, "users", map[string]interface{}{"id": u.ID})
-	db.AssertMissing(t, "lists", map[string]interface{}{"id": 24}) // only user6 had access to this list
-	db.AssertExists(t, "lists", map[string]interface{}{"id": 6}, false)
-	db.AssertExists(t, "lists", map[string]interface{}{"id": 7}, false)
-	db.AssertExists(t, "lists", map[string]interface{}{"id": 8}, false)
-	db.AssertExists(t, "lists", map[string]interface{}{"id": 9}, false)
-	db.AssertExists(t, "lists", map[string]interface{}{"id": 10}, false)
-	db.AssertExists(t, "lists", map[string]interface{}{"id": 11}, false)
+		assert.NoError(t, err)
+		db.AssertMissing(t, "users", map[string]interface{}{"id": u.ID})
+		db.AssertMissing(t, "lists", map[string]interface{}{"id": 24}) // only user6 had access to this list
+		db.AssertExists(t, "lists", map[string]interface{}{"id": 6}, false)
+		db.AssertExists(t, "lists", map[string]interface{}{"id": 7}, false)
+		db.AssertExists(t, "lists", map[string]interface{}{"id": 8}, false)
+		db.AssertExists(t, "lists", map[string]interface{}{"id": 9}, false)
+		db.AssertExists(t, "lists", map[string]interface{}{"id": 10}, false)
+		db.AssertExists(t, "lists", map[string]interface{}{"id": 11}, false)
+	})
+	t.Run("user with no namespaces", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+		notifications.Fake()
+
+		u := &user.User{ID: 4}
+		err := DeleteUser(s, u)
+
+		assert.NoError(t, err)
+		// No assertions for deleted lists and namespaces since that user doesn't have any
+	})
 }