fix(task): ensure done_at can never be set by user (#1461)

pkg/models/bulk_task_test.go

@@ -18,6 +18,7 @@ package models
 
 import (
 	"testing"
+	"time"
 
 	"code.vikunja.io/api/pkg/db"
 	"code.vikunja.io/api/pkg/user"
@@ -116,4 +117,21 @@ func TestBulkTask_Update(t *testing.T) {
 		assert.NotZero(t, bt.Tasks[0].DoneAt)
 		assert.NotZero(t, bt.Tasks[1].DoneAt)
 	})
+
+	t.Run("don't update done_at when bulk marking tasks done", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		userProvidedTime := time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC)
+		bt := &BulkTask{
+			TaskIDs: []int64{1, 3},
+			Fields:  []string{"done", "done_at"},
+			Values:  &Task{Done: true, DoneAt: userProvidedTime},
+		}
+
+		err := bt.Update(s, u)
+		require.Error(t, err)
+		assert.IsType(t, ErrInvalidTaskColumn{}, err)
+	})
 }

pkg/models/tasks.go

@@ -59,7 +59,7 @@ type Task struct {
 	Description string `xorm:"longtext null" json:"description"`
 	// Whether a task is done or not.
 	Done bool `xorm:"INDEX null" json:"done"`
-	// The time when a task was marked as done.
+	// The time when a task was marked as done. This field is system-controlled and cannot be set via API.
 	DoneAt time.Time `xorm:"INDEX null 'done_at'" json:"done_at"`
 	// The time when the task is due.
 	DueDate time.Time `xorm:"DATETIME INDEX null 'due_date'" json:"due_date"`
@@ -1047,7 +1047,6 @@ func (t *Task) updateSingleTask(s *xorm.Session, a web.Auth, fields []string) (e
 		"start_date",
 		"end_date",
 		"hex_color",
-		"done_at",
 		"percent_done",
 		"project_id",
 		"bucket_id",
@@ -1190,7 +1189,10 @@ func (t *Task) updateSingleTask(s *xorm.Session, a web.Auth, fields []string) (e
 	}
 
 	// When a repeating task is marked as done, we update all deadlines and reminders and set it as undone
-	updateDone(&ot, t)
+	updateDoneAt := updateDone(&ot, t)
+	if updateDoneAt {
+		colsToUpdate = append(colsToUpdate, "done_at")
+	}
 
 	// Update the reminders
 	if err := ot.updateReminders(s, t); err != nil {
@@ -1563,7 +1565,10 @@ func setTaskDatesFromCurrentDateRepeat(oldTask, newTask *Task) {
 // We make a few assumptions here:
 //  1. Everything in oldTask is the truth - we figure out if we update anything at all if oldTask.RepeatAfter has a value > 0
 //  2. Because of 1., this functions should not be used to update values other than Done in the same go
-func updateDone(oldTask *Task, newTask *Task) {
+func updateDone(oldTask *Task, newTask *Task) (updateDoneAt bool) {
+	// Track if the done status changed before repeat helpers modify it
+	doneStatusChanged := oldTask.Done != newTask.Done
+
 	if !oldTask.Done && newTask.Done {
 		switch oldTask.RepeatMode {
 		case TaskRepeatModeMonth:
@@ -1581,6 +1586,8 @@ func updateDone(oldTask *Task, newTask *Task) {
 	if oldTask.Done && !newTask.Done {
 		newTask.DoneAt = time.Time{}
 	}
+
+	return doneStatusChanged
 }
 
 // Set the absolute trigger dates for Reminders with relative period

pkg/models/tasks_test.go

@@ -334,6 +334,40 @@ func TestTask_Update(t *testing.T) {
 			"bucket_id": 1,
 		}, false)
 	})
+	t.Run("repeating tasks should set done_at when marked done", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		// Get the task before updating to check done_at was empty
+		taskBefore := &Task{ID: 28}
+		err := taskBefore.ReadOne(s, u)
+		require.NoError(t, err)
+		assert.True(t, taskBefore.DoneAt.IsZero())
+		assert.False(t, taskBefore.Done)
+
+		// Mark the repeating task as done
+		task := &Task{
+			ID:          28,
+			Done:        true,
+			RepeatAfter: 3600,
+		}
+		err = task.Update(s, u)
+		require.NoError(t, err)
+		err = s.Commit()
+		require.NoError(t, err)
+
+		// Task should be reset to not done (because it repeats) but done_at should be set
+		assert.False(t, task.Done)
+		assert.False(t, task.DoneAt.IsZero(), "done_at should be set for repeating tasks when marked as done")
+
+		// Verify in database
+		updatedTask := &Task{ID: 28}
+		err = updatedTask.ReadOne(s, u)
+		require.NoError(t, err)
+		assert.False(t, updatedTask.Done)
+		assert.False(t, updatedTask.DoneAt.IsZero(), "done_at should be persisted in database for repeating tasks")
+	})
 	t.Run("moving a task between projects should give it a correct index", func(t *testing.T) {
 		db.LoadAndAssertFixtures(t)
 		s := db.NewSession()
@@ -450,6 +484,44 @@ func TestTask_Update(t *testing.T) {
 		err = s.Commit()
 		require.NoError(t, err)
 	})
+	t.Run("don't allow done_at change when passing fields", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		task := &Task{
+			ID:     1,
+			DoneAt: time.Now(),
+		}
+
+		err := task.updateSingleTask(s, u, []string{"done_at"})
+
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), `Task column done_at is invalid`)
+		require.NoError(t, s.Commit())
+	})
+	t.Run("ignore done_at when updating unrelated values", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		task := &Task{
+			ID:     1,
+			Title:  "updated",
+			DoneAt: time.Now(),
+		}
+
+		err := task.Update(s, u)
+
+		require.NoError(t, err)
+		require.NoError(t, s.Commit())
+
+		updatedTask := &Task{ID: 1}
+		err = updatedTask.ReadOne(s, u)
+		require.NoError(t, err)
+		assert.Equal(t, "updated", updatedTask.Title)
+		assert.True(t, updatedTask.DoneAt.IsZero())
+	})
 }
 
 func TestTask_Delete(t *testing.T) {