fix(reminders): refactor and check permissions when fetching task users

pkg/models/task_overdue_reminder_test.go

@@ -46,7 +46,7 @@ func TestGetUndoneOverDueTasks(t *testing.T) {
 		require.NoError(t, err)
 		uts, err := getUndoneOverdueTasks(s, now)
 		require.NoError(t, err)
-		assert.Len(t, uts, 1)
+		require.Len(t, uts, 1)
 		assert.Len(t, uts[1].tasks, 2)
 		// The tasks don't always have the same order, so we only check their presence, not their position.
 		var task5Present bool
@@ -74,3 +74,21 @@ func TestGetUndoneOverDueTasks(t *testing.T) {
 		assert.Empty(t, tasks)
 	})
 }
+
+func TestGetTaskUsersForTasksPermissionFiltering(t *testing.T) {
+	db.LoadAndAssertFixtures(t)
+	s := db.NewSession()
+	defer s.Close()
+
+	usersWithAccess, err := getTaskUsersForTasks(s, []int64{35}, nil)
+	require.NoError(t, err)
+
+	var hasAssignee bool
+	for _, tu := range usersWithAccess {
+		if tu.User.ID == 2 {
+			hasAssignee = true
+			break
+		}
+	}
+	assert.True(t, hasAssignee)
+}

pkg/models/task_reminder.go

@@ -68,58 +68,131 @@ type taskUser struct {
 
 const dbTimeFormat = `2006-01-02 15:04:05`
 
+//nolint:gocyclo
 func getTaskUsersForTasks(s *xorm.Session, taskIDs []int64, cond builder.Cond) (taskUsers []*taskUser, err error) {
 	if len(taskIDs) == 0 {
 		return
 	}
 
-	// Get all creators of tasks
-	creators := make(map[int64]*user.User, len(taskIDs))
-	err = s.
-		Select("users.*").
-		Join("LEFT", "tasks", "tasks.created_by_id = users.id").
-		In("tasks.id", taskIDs).
-		Where(cond).
-		GroupBy("tasks.id, users.id, users.username, users.email, users.name, users.timezone").
-		Find(&creators)
-	if err != nil {
-		return
-	}
-
+	taskUsers = []*taskUser{}
 	taskMap := make(map[int64]*Task, len(taskIDs))
 	err = s.In("id", taskIDs).Find(&taskMap)
 	if err != nil {
 		return
 	}
 
+	projectIDs := []int64{}
 	for _, task := range taskMap {
-		u, exists := creators[task.CreatedByID]
-		if !exists {
-			continue
-		}
-
-		taskUsers = append(taskUsers, &taskUser{
-			Task: taskMap[task.ID],
-			User: u,
-		})
+		projectIDs = append(projectIDs, task.ProjectID)
+	}
+	projects := make(map[int64]*Project)
+	err = s.In("id", projectIDs).Find(&projects)
+	if err != nil {
+		return
 	}
 
-	var assignees []*TaskAssigneeWithUser
+	// user_id -> project_id -> has read access
+	userPermissionOnProject := make(map[int64]map[int64]bool)
+
+	seen := make(map[int64]map[int64]struct{})
+	appendUser := func(taskID int64, u *user.User) (err error) {
+		if u == nil {
+			return
+		}
+		task, hasTask := taskMap[taskID]
+		if !hasTask {
+			return
+		}
+		if seen[taskID] == nil {
+			seen[taskID] = make(map[int64]struct{})
+		}
+		if _, exists := seen[taskID][u.ID]; exists {
+			return
+		}
+		seen[taskID][u.ID] = struct{}{}
+
+		userProjects, has := userPermissionOnProject[u.ID]
+		if !has {
+			userPermissionOnProject[u.ID] = make(map[int64]bool)
+			userProjects = userPermissionOnProject[u.ID]
+		}
+		_, projectExists := userProjects[task.ProjectID]
+		if !projectExists {
+			p, exists := projects[task.ProjectID]
+			if !exists {
+				return
+			}
+
+			userProjects[task.ProjectID] = p.isOwner(u)
+
+			if !p.isOwner(u) {
+				userProjects[task.ProjectID], _, err = p.checkPermission(s, u, PermissionRead)
+				if err != nil {
+					return err
+				}
+			}
+		}
+
+		if !userProjects[task.ProjectID] {
+			return
+		}
+
+		taskUsers = append(taskUsers, &taskUser{Task: task, User: u})
+
+		return
+	}
+
+	type userWithTask struct {
+		TaskID    int64
+		user.User `xorm:"extends"`
+	}
+
+	conditions := []builder.Cond{
+		builder.In("tasks.id", taskIDs),
+	}
+	if cond != nil {
+		conditions = append(conditions, cond)
+	}
+
+	creators := []*userWithTask{}
+	err = s.Table("tasks").
+		Select("DISTINCT tasks.id AS task_id, users.id, users.name, users.username, users.email, users.email_reminders_enabled, users.overdue_tasks_reminders_enabled, users.overdue_tasks_reminders_time, users.language, users.timezone, users.created, users.updated").
+		Join("INNER", "users", "tasks.created_by_id = users.id").
+		Where(builder.And(conditions...)).
+		Find(&creators)
+	if err != nil {
+		return
+	}
+
+	for _, creator := range creators {
+		err = appendUser(creator.TaskID, &creator.User)
+		if err != nil {
+			return
+		}
+	}
+
+	assigneeConds := []builder.Cond{
+		builder.In("task_assignees.task_id", taskIDs),
+	}
+	if cond != nil {
+		assigneeConds = append(assigneeConds, cond)
+	}
+
+	assignees := []*TaskAssigneeWithUser{}
 	err = s.Table("task_assignees").
-		Select("task_id, users.*").
-		In("task_id", taskIDs).
+		Select("DISTINCT task_assignees.task_id, users.id, users.name, users.username, users.email, users.email_reminders_enabled, users.overdue_tasks_reminders_enabled, users.overdue_tasks_reminders_time, users.language, users.timezone, users.created, users.updated").
 		Join("INNER", "users", "task_assignees.user_id = users.id").
-		Where(cond).
+		Where(builder.And(assigneeConds...)).
 		Find(&assignees)
 	if err != nil {
 		return
 	}
 
 	for i := range assignees {
-		taskUsers = append(taskUsers, &taskUser{
-			Task: taskMap[assignees[i].TaskID],
-			User: &assignees[i].User,
-		})
+		err = appendUser(assignees[i].TaskID, &assignees[i].User)
+		if err != nil {
+			return
+		}
 	}
 
 	subscriptions, err := GetSubscriptionsForEntities(s, SubscriptionEntityTask, taskIDs)
@@ -134,10 +207,18 @@ func getTaskUsersForTasks(s *xorm.Session, taskIDs []int64, cond builder.Cond) (
 		}
 	}
 
-	subscribers, err := user.GetUsersByCond(s, builder.And(
+	if len(subscriberIDs) == 0 {
+		return
+	}
+
+	subscriberCond := []builder.Cond{
 		builder.In("id", subscriberIDs),
-		cond,
-	))
+	}
+	if cond != nil {
+		subscriberCond = append(subscriberCond, cond)
+	}
+
+	subscribers, err := user.GetUsersByCond(s, builder.And(subscriberCond...))
 	if err != nil {
 		return nil, err
 	}
@@ -148,10 +229,10 @@ func getTaskUsersForTasks(s *xorm.Session, taskIDs []int64, cond builder.Cond) (
 			if !has {
 				continue
 			}
-			taskUsers = append(taskUsers, &taskUser{
-				Task: taskMap[taskID],
-				User: u,
-			})
+			err = appendUser(taskID, u)
+			if err != nil {
+				return
+			}
 		}
 	}
 

pkg/models/task_reminder_test.go

@@ -51,3 +51,222 @@ func TestReminderGetTasksInTheNextMinute(t *testing.T) {
 		assert.Empty(t, taskIDs)
 	})
 }
+
+func TestGetTaskUsersForTasks(t *testing.T) {
+	t.Run("task owner", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		// Task 1 is owned by user 1 (created_by_id: 1) in project 1 (owned by user 1)
+		taskUsers, err := getTaskUsersForTasks(s, []int64{1}, nil)
+		require.NoError(t, err)
+		require.NotEmpty(t, taskUsers)
+
+		// Should include the task creator
+		hasUser1 := false
+		for _, tu := range taskUsers {
+			if tu.User.ID == 1 && tu.Task.ID == 1 {
+				hasUser1 = true
+				break
+			}
+		}
+		assert.True(t, hasUser1, "task owner should be included in task users")
+	})
+
+	t.Run("project shared directly with user", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		// Task 32 is in project 3, which is shared directly with user 1 (users_projects id: 1)
+		taskUsers, err := getTaskUsersForTasks(s, []int64{32}, nil)
+		require.NoError(t, err)
+		require.NotEmpty(t, taskUsers)
+
+		// Should include user 1 who has direct share
+		hasUser1 := false
+		for _, tu := range taskUsers {
+			if tu.User.ID == 1 && tu.Task.ID == 32 {
+				hasUser1 = true
+				break
+			}
+		}
+		assert.True(t, hasUser1, "user with direct project share should be included")
+	})
+
+	t.Run("creator who lost project access", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		// Task 1 is in project 1 (owned by user 1)
+		// Task 1 was created by user 1 (created_by_id: 1)
+		// User 13 has no access to project 1
+		// Create a scenario by pretending user 13 created the task but has no access
+
+		_, err := s.
+			Cols("created_by_id").
+			Where("id = ?", 1).
+			Update(&Task{CreatedByID: 13})
+		require.NoError(t, err)
+
+		taskUsers, err := getTaskUsersForTasks(s, []int64{1}, nil)
+		require.NoError(t, err)
+
+		// Should only include users with access
+		// User 13 should not be in the results (no access to project 1)
+		hasUser13 := false
+		for _, tu := range taskUsers {
+			if tu.User.ID == 13 && tu.Task.ID == 1 {
+				hasUser13 = true
+				break
+			}
+		}
+		assert.False(t, hasUser13, "creator without project access should be filtered out")
+	})
+
+	t.Run("subscriber who lost project access", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		// Task 2 is in project 1 (owned by user 1)
+		// Create a subscription for user 13 who has no access to project 1
+		subscription := &Subscription{
+			EntityType: SubscriptionEntityTask,
+			EntityID:   2,
+			UserID:     13,
+		}
+		_, err := s.Insert(subscription)
+		require.NoError(t, err)
+
+		taskUsers, err := getTaskUsersForTasks(s, []int64{2}, nil)
+		require.NoError(t, err)
+
+		// User 13 should NOT be in the results (subscribed but no access to project 1)
+		hasUser13 := false
+		for _, tu := range taskUsers {
+			if tu.User.ID == 13 && tu.Task.ID == 2 {
+				hasUser13 = true
+				break
+			}
+		}
+		assert.False(t, hasUser13, "subscriber without project access should be filtered out")
+	})
+
+	t.Run("assignees - with and without project access", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		// Task 30 has assignees: user 1 and user 2 (task_assignees)
+		// Task 30 is in project 1, owned by user 1
+		// User 1 has access (owner), user 2 does NOT have access to project 1
+		taskUsers, err := getTaskUsersForTasks(s, []int64{30}, nil)
+		require.NoError(t, err)
+		require.NotEmpty(t, taskUsers)
+
+		// Should include user 1 (assignee WITH project access)
+		// Should NOT include user 2 (assignee WITHOUT project access)
+		hasUser1 := false
+		hasUser2 := false
+		for _, tu := range taskUsers {
+			if tu.Task.ID == 30 {
+				if tu.User.ID == 1 {
+					hasUser1 = true
+				}
+				if tu.User.ID == 2 {
+					hasUser2 = true
+				}
+			}
+		}
+		assert.True(t, hasUser1, "assignee with project access should be included")
+		assert.False(t, hasUser2, "assignee without project access should be filtered out")
+	})
+
+	t.Run("subscribers - with project access", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		// Task 2 has subscription from user 1 (subscriptions id: 1)
+		// Task 2 is in project 1, owned by user 1
+		// User 1 has access as the owner
+		taskUsers, err := getTaskUsersForTasks(s, []int64{2}, nil)
+		require.NoError(t, err)
+		require.NotEmpty(t, taskUsers)
+
+		// Should include the subscriber who has access
+		hasUser1 := false
+		for _, tu := range taskUsers {
+			if tu.User.ID == 1 && tu.Task.ID == 2 {
+				hasUser1 = true
+				break
+			}
+		}
+		assert.True(t, hasUser1, "subscriber with project access should be included")
+	})
+
+	t.Run("no duplicate users", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		// Task 30: user 1 is both creator and assignee
+		taskUsers, err := getTaskUsersForTasks(s, []int64{30}, nil)
+		require.NoError(t, err)
+		require.NotEmpty(t, taskUsers)
+
+		// Count how many times user 1 appears for task 30
+		user1Count := 0
+		for _, tu := range taskUsers {
+			if tu.User.ID == 1 && tu.Task.ID == 30 {
+				user1Count++
+			}
+		}
+		assert.Equal(t, 1, user1Count, "each user should appear only once per task")
+	})
+
+	t.Run("empty task list", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		taskUsers, err := getTaskUsersForTasks(s, []int64{}, nil)
+		require.NoError(t, err)
+		assert.Empty(t, taskUsers)
+	})
+
+	t.Run("multiple tasks with various relationships", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		// Task 1: user 1 is creator and owner
+		// Task 2: user 1 is subscriber and owner
+		// Task 30: user 1 is assignee and owner, user 2 is assignee without access
+		taskUsers, err := getTaskUsersForTasks(s, []int64{1, 2, 30}, nil)
+		require.NoError(t, err)
+		require.NotEmpty(t, taskUsers)
+
+		// Count unique task IDs in results
+		taskIDs := make(map[int64]bool)
+		for _, tu := range taskUsers {
+			taskIDs[tu.Task.ID] = true
+		}
+		assert.True(t, taskIDs[1], "should include users for task 1")
+		assert.True(t, taskIDs[2], "should include users for task 2")
+		assert.True(t, taskIDs[30], "should include users for task 30")
+
+		// Verify user 2 is NOT included for any task (no access to project 1)
+		hasUser2 := false
+		for _, tu := range taskUsers {
+			if tu.User.ID == 2 {
+				hasUser2 = true
+				break
+			}
+		}
+		assert.False(t, hasUser2, "user without project access should not be included for any task")
+	})
+}