fix: don't allow setting a list namespace to 0

See go-vikunja/app#13
go-vikunja · Oct 1, 2022 · 96ed1e3 · 96ed1e3
1 parent 374a0f9
commit 96ed1e3
Show file tree

Hide file tree

Showing 7 changed files with 59 additions and 22 deletions.
diff --git a/docs/content/doc/usage/errors.md b/docs/content/doc/usage/errors.md
@@ -54,13 +54,14 @@ This document describes the different errors Vikunja can return.
 
 | ErrorCode | HTTP Status Code | Description                                                                                                                   |
 |-----------|------------------|-------------------------------------------------------------------------------------------------------------------------------|
-| 3001 | 404 | The list does not exist.                                                                                                      |
-| 3004 | 403 | The user needs to have read permissions on that list to perform that action.                                                  |
-| 3005 | 400 | The list title cannot be empty.                                                                                               |
-| 3006 | 404 | The list share does not exist.                                                                                                |
-| 3007 | 400 | A list with this identifier already exists.                                                                                   |
-| 3008 | 412 | The list is archived and can therefore only be accessed read only. This is also true for all tasks associated with this list. |
-| 3009 | 412 | The list cannot belong to a dynamically generated namespace like "Favorites".                                                 |
+| 3001      | 404 | The list does not exist.                                                                                                      |
+| 3004      | 403 | The user needs to have read permissions on that list to perform that action.                                                  |
+| 3005      | 400 | The list title cannot be empty.                                                                                               |
+| 3006      | 404 | The list share does not exist.                                                                                                |
+| 3007      | 400 | A list with this identifier already exists.                                                                                   |
+| 3008      | 412 | The list is archived and can therefore only be accessed read only. This is also true for all tasks associated with this list. |
+| 3009      | 412 | The list cannot belong to a dynamically generated namespace like "Favorites".                                                 |
+| 3010      | 412 | The list must belong to a namespace.                                                                                          |
 
 ## Task
 

diff --git a/pkg/integrations/archived_test.go b/pkg/integrations/archived_test.go
@@ -227,7 +227,7 @@ func TestArchived(t *testing.T) {
 				assertHandlerErrorCode(t, err, models.ErrCodeListIsArchived)
 			})
 			t.Run("unarchivable", func(t *testing.T) {
-				rec, err := testListHandler.testUpdateWithUser(nil, map[string]string{"list": "22"}, `{"title":"LoremIpsum","is_archived":false}`)
+				rec, err := testListHandler.testUpdateWithUser(nil, map[string]string{"list": "22"}, `{"title":"LoremIpsum","is_archived":false,"namespace_id":1}`)
 				assert.NoError(t, err)
 				assert.Contains(t, rec.Body.String(), `"is_archived":false`)
 			})

diff --git a/pkg/integrations/link_sharing_test.go b/pkg/integrations/link_sharing_test.go
@@ -232,12 +232,12 @@ func TestLinkSharing(t *testing.T) {
 					assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 				})
 				t.Run("Shared write", func(t *testing.T) {
-					rec, err := testHandlerListWrite.testUpdateWithLinkShare(nil, map[string]string{"list": "2"}, `{"title":"TestLoremIpsum"}`)
+					rec, err := testHandlerListWrite.testUpdateWithLinkShare(nil, map[string]string{"list": "2"}, `{"title":"TestLoremIpsum","namespace_id":1}`)
 					assert.NoError(t, err)
 					assert.Contains(t, rec.Body.String(), `"title":"TestLoremIpsum"`)
 				})
 				t.Run("Shared admin", func(t *testing.T) {
-					rec, err := testHandlerListAdmin.testUpdateWithLinkShare(nil, map[string]string{"list": "3"}, `{"title":"TestLoremIpsum"}`)
+					rec, err := testHandlerListAdmin.testUpdateWithLinkShare(nil, map[string]string{"list": "3"}, `{"title":"TestLoremIpsum","namespace_id":2}`)
 					assert.NoError(t, err)
 					assert.Contains(t, rec.Body.String(), `"title":"TestLoremIpsum"`)
 				})

diff --git a/pkg/integrations/list_test.go b/pkg/integrations/list_test.go
@@ -171,7 +171,7 @@ func TestList(t *testing.T) {
 	t.Run("Update", func(t *testing.T) {
 		t.Run("Normal", func(t *testing.T) {
 			// Check the list was loaded successfully afterwards, see testReadOneWithUser
-			rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "1"}, `{"title":"TestLoremIpsum"}`)
+			rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "1"}, `{"title":"TestLoremIpsum","namespace_id":1}`)
 			assert.NoError(t, err)
 			assert.Contains(t, rec.Body.String(), `"title":"TestLoremIpsum"`)
 			// The description should not be updated but returned correctly
@@ -183,7 +183,7 @@ func TestList(t *testing.T) {
 			assertHandlerErrorCode(t, err, models.ErrCodeListDoesNotExist)
 		})
 		t.Run("Normal with updating the description", func(t *testing.T) {
-			rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "1"}, `{"title":"TestLoremIpsum","description":"Lorem Ipsum dolor sit amet"}`)
+			rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "1"}, `{"title":"TestLoremIpsum","description":"Lorem Ipsum dolor sit amet","namespace_id":1}`)
 			assert.NoError(t, err)
 			assert.Contains(t, rec.Body.String(), `"title":"TestLoremIpsum"`)
 			assert.Contains(t, rec.Body.String(), `"description":"Lorem Ipsum dolor sit amet`)
@@ -211,12 +211,12 @@ func TestList(t *testing.T) {
 				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via Team write", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "7"}, `{"title":"TestLoremIpsum"}`)
+				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "7"}, `{"title":"TestLoremIpsum","namespace_id":6}`)
 				assert.NoError(t, err)
 				assert.Contains(t, rec.Body.String(), `"title":"TestLoremIpsum"`)
 			})
 			t.Run("Shared Via Team admin", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "8"}, `{"title":"TestLoremIpsum"}`)
+				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "8"}, `{"title":"TestLoremIpsum","namespace_id":6}`)
 				assert.NoError(t, err)
 				assert.Contains(t, rec.Body.String(), `"title":"TestLoremIpsum"`)
 			})
@@ -227,12 +227,12 @@ func TestList(t *testing.T) {
 				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via User write", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "10"}, `{"title":"TestLoremIpsum"}`)
+				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "10"}, `{"title":"TestLoremIpsum","namespace_id":6}`)
 				assert.NoError(t, err)
 				assert.Contains(t, rec.Body.String(), `"title":"TestLoremIpsum"`)
 			})
 			t.Run("Shared Via User admin", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "11"}, `{"title":"TestLoremIpsum"}`)
+				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "11"}, `{"title":"TestLoremIpsum","namespace_id":6}`)
 				assert.NoError(t, err)
 				assert.Contains(t, rec.Body.String(), `"title":"TestLoremIpsum"`)
 			})
@@ -243,12 +243,12 @@ func TestList(t *testing.T) {
 				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via NamespaceTeam write", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "13"}, `{"title":"TestLoremIpsum"}`)
+				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "13"}, `{"title":"TestLoremIpsum","namespace_id":8}`)
 				assert.NoError(t, err)
 				assert.Contains(t, rec.Body.String(), `"title":"TestLoremIpsum"`)
 			})
 			t.Run("Shared Via NamespaceTeam admin", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "14"}, `{"title":"TestLoremIpsum"}`)
+				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "14"}, `{"title":"TestLoremIpsum","namespace_id":9}`)
 				assert.NoError(t, err)
 				assert.Contains(t, rec.Body.String(), `"title":"TestLoremIpsum"`)
 			})
@@ -259,12 +259,12 @@ func TestList(t *testing.T) {
 				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via NamespaceUser write", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "16"}, `{"title":"TestLoremIpsum"}`)
+				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "16"}, `{"title":"TestLoremIpsum","namespace_id":11}`)
 				assert.NoError(t, err)
 				assert.Contains(t, rec.Body.String(), `"title":"TestLoremIpsum"`)
 			})
 			t.Run("Shared Via NamespaceUser admin", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "17"}, `{"title":"TestLoremIpsum"}`)
+				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"list": "17"}, `{"title":"TestLoremIpsum","namespace_id":12}`)
 				assert.NoError(t, err)
 				assert.Contains(t, rec.Body.String(), `"title":"TestLoremIpsum"`)
 			})

diff --git a/pkg/models/error.go b/pkg/models/error.go
@@ -283,6 +283,34 @@ func (err *ErrListCannotBelongToAPseudoNamespace) HTTPError() web.HTTPError {
 	}
 }
 
+// ErrListMustBelongToANamespace represents an error where a list must belong to a namespace
+type ErrListMustBelongToANamespace struct {
+	ListID      int64
+	NamespaceID int64
+}
+
+// IsErrListMustBelongToANamespace checks if an error is a list must belong to a namespace error.
+func IsErrListMustBelongToANamespace(err error) bool {
+	_, ok := err.(*ErrListMustBelongToANamespace)
+	return ok
+}
+
+func (err *ErrListMustBelongToANamespace) Error() string {
+	return fmt.Sprintf("List must belong to a namespace [ListID: %d, NamespaceID: %d]", err.ListID, err.NamespaceID)
+}
+
+// ErrCodeListMustBelongToANamespace holds the unique world-error code of this error
+const ErrCodeListMustBelongToANamespace = 3010
+
+// HTTPError holds the http error description
+func (err *ErrListMustBelongToANamespace) HTTPError() web.HTTPError {
+	return web.HTTPError{
+		HTTPCode: http.StatusPreconditionFailed,
+		Code:     ErrCodeListMustBelongToANamespace,
+		Message:  "This list must belong to a namespace.",
+	}
+}
+
 // ================
 // List task errors
 // ================

diff --git a/pkg/models/list.go b/pkg/models/list.go
@@ -640,6 +640,13 @@ func UpdateList(s *xorm.Session, list *List, auth web.Auth, updateListBackground
 		return
 	}
 
+	if list.NamespaceID == 0 {
+		return &ErrListMustBelongToANamespace{
+			ListID:      list.ID,
+			NamespaceID: list.NamespaceID,
+		}
+	}
+
 	// We need to specify the cols we want to update here to be able to un-archive lists
 	colsToUpdate := []string{
 		"title",

diff --git a/pkg/models/list_test.go b/pkg/models/list_test.go
@@ -140,8 +140,9 @@ func TestList_CreateOrUpdate(t *testing.T) {
 			db.LoadAndAssertFixtures(t)
 			s := db.NewSession()
 			list := List{
-				ID:    99999999,
-				Title: "test",
+				ID:          99999999,
+				Title:       "test",
+				NamespaceID: 1,
 			}
 			err := list.Update(s, usr)
 			assert.Error(t, err)