From 2a43f9b076f3c84c1c16209c78ffa8b3b5e0f2d2 Mon Sep 17 00:00:00 2001 From: kolaente Date: Thu, 9 Oct 2025 13:20:29 +0200 Subject: [PATCH] fix(reminders): refactor and check permissions when fetching task users --- pkg/models/task_overdue_reminder_test.go | 20 ++- pkg/models/task_reminder.go | 155 ++++++++++++---- pkg/models/task_reminder_test.go | 219 +++++++++++++++++++++++ 3 files changed, 356 insertions(+), 38 deletions(-) diff --git a/pkg/models/task_overdue_reminder_test.go b/pkg/models/task_overdue_reminder_test.go index 35566f901..6d2ad3fcb 100644 --- a/pkg/models/task_overdue_reminder_test.go +++ b/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) +} diff --git a/pkg/models/task_reminder.go b/pkg/models/task_reminder.go index 8afd30f9e..9eabefc52 100644 --- a/pkg/models/task_reminder.go +++ b/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 + } } } diff --git a/pkg/models/task_reminder_test.go b/pkg/models/task_reminder_test.go index 7d720835c..46cf15543 100644 --- a/pkg/models/task_reminder_test.go +++ b/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") + }) +}