diff --git a/pkg/models/listeners.go b/pkg/models/listeners.go index 8bc28fac0..4daa34ac0 100644 --- a/pkg/models/listeners.go +++ b/pkg/models/listeners.go @@ -568,7 +568,8 @@ func (s *SendTaskCommentNotification) Handle(msg *message.Message) (err error) { } err = notifications.Notify(subscriber.User, n, sess) if err != nil { - return + log.Errorf("Could not send task comment notification to user %d for task %d: %s", subscriber.UserID, event.Task.ID, err) + continue } } @@ -676,7 +677,8 @@ func (s *SendTaskAssignedNotification) Handle(msg *message.Message) (err error) } err = notifications.Notify(subscriber.User, n, sess) if err != nil { - return + log.Errorf("Could not send task assigned notification to user %d for task %d: %s", subscriber.UserID, event.Task.ID, err) + continue } notifiedUsers[subscriber.UserID] = true @@ -723,7 +725,8 @@ func (s *SendTaskDeletedNotification) Handle(msg *message.Message) (err error) { } err = notifications.Notify(subscriber.User, n, sess) if err != nil { - return + log.Errorf("Could not send task deleted notification to user %d for task %d: %s", subscriber.UserID, event.Task.ID, err) + continue } } @@ -1066,7 +1069,8 @@ func (s *SendProjectCreatedNotification) Handle(msg *message.Message) (err error } err = notifications.Notify(subscriber.User, n, sess) if err != nil { - return + log.Errorf("Could not send project created notification to user %d for project %d: %s", subscriber.UserID, event.Project.ID, err) + continue } } diff --git a/pkg/models/listeners_test.go b/pkg/models/listeners_test.go index 0271af624..1bec19964 100644 --- a/pkg/models/listeners_test.go +++ b/pkg/models/listeners_test.go @@ -137,6 +137,136 @@ func TestUpdateTaskInSavedFilterViews_InactiveFilterOwner(t *testing.T) { }) } +// Subscriptions survive losing access to the entity they point at: nothing +// purges them when a share is removed, and access can change with no +// revocation event at all. Every notification listener must therefore re-check +// read permission before delivering. +// +// Task 32 lives on project 3, which user 2 can read and user 6 cannot. +func TestSubscriberNotifications_SkipUsersWithoutReadAccess(t *testing.T) { + const ( + taskID int64 = 32 + projectID int64 = 3 + withAccess int64 = 2 + lostAccess int64 = 6 + doerID int64 = 1 + assigneeID int64 = 3 + childProjID int64 = 9990 + ) + + subscribeBoth := func(t *testing.T, s *xorm.Session, entityType SubscriptionEntityType, entityID int64) { + for _, userID := range []int64{withAccess, lostAccess} { + _, err := s.Insert(&Subscription{ + UserID: userID, + EntityType: entityType, + EntityID: entityID, + }) + require.NoError(t, err) + } + } + + assertOnlySubscriberWithAccessNotified := func(t *testing.T, notificationName string) { + db.AssertExists(t, "notifications", map[string]interface{}{ + "notifiable_id": withAccess, + "name": notificationName, + }, false) + db.AssertMissing(t, "notifications", map[string]interface{}{ + "notifiable_id": lostAccess, + "name": notificationName, + }) + } + + t.Run("task comment", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + subscribeBoth(t, s, SubscriptionEntityTask, taskID) + + task, err := GetTaskByIDSimple(s, taskID) + require.NoError(t, err) + + comment := &TaskComment{Comment: "secret", TaskID: taskID, AuthorID: doerID} + _, err = s.Insert(comment) + require.NoError(t, err) + require.NoError(t, s.Commit()) + _ = s.Close() + + events.TestListener(t, &TaskCommentCreatedEvent{ + Task: &task, + Doer: &user.User{ID: doerID}, + Comment: comment, + }, &SendTaskCommentNotification{}) + + assertOnlySubscriberWithAccessNotified(t, (&TaskCommentNotification{}).Name()) + }) + + t.Run("task assigned", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + subscribeBoth(t, s, SubscriptionEntityTask, taskID) + + task, err := GetTaskByIDSimple(s, taskID) + require.NoError(t, err) + assignee, err := user.GetUserByID(s, assigneeID) + require.NoError(t, err) + require.NoError(t, s.Commit()) + _ = s.Close() + + events.TestListener(t, &TaskAssigneeCreatedEvent{ + Task: &task, + Assignee: assignee, + Doer: &user.User{ID: doerID}, + }, &SendTaskAssignedNotification{}) + + assertOnlySubscriberWithAccessNotified(t, (&TaskAssignedNotification{}).Name()) + }) + + t.Run("task deleted", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + subscribeBoth(t, s, SubscriptionEntityTask, taskID) + + task, err := GetTaskByIDSimple(s, taskID) + require.NoError(t, err) + require.NoError(t, s.Commit()) + _ = s.Close() + + events.TestListener(t, &TaskDeletedEvent{ + Task: &task, + Doer: &user.User{ID: doerID}, + }, &SendTaskDeletedNotification{}) + + assertOnlySubscriberWithAccessNotified(t, (&TaskDeletedNotification{}).Name()) + }) + + // Subscribers are inherited from the parent project, but the notification + // discloses the newly created child, so the child is what gets checked. + t.Run("project created", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + subscribeBoth(t, s, SubscriptionEntityProject, projectID) + + parentID := projectID + child := &Project{ + ID: childProjID, + Title: "child", + Identifier: "CHILD", + OwnerID: doerID, + ParentProjectID: &parentID, + } + _, err := s.Insert(child) + require.NoError(t, err) + require.NoError(t, s.Commit()) + _ = s.Close() + + events.TestListener(t, &ProjectCreatedEvent{ + Project: child, + Doer: &user.User{ID: doerID}, + }, &SendProjectCreatedNotification{}) + + assertOnlySubscriberWithAccessNotified(t, (&ProjectCreatedNotification{}).Name()) + }) +} + // The listener runs after the deleting transaction committed, so the task is already // soft-deleted by the time it looks up who to notify. func TestSendTaskDeletedNotification(t *testing.T) { diff --git a/pkg/models/subscription.go b/pkg/models/subscription.go index 86a20a274..7e0ab2abd 100644 --- a/pkg/models/subscription.go +++ b/pkg/models/subscription.go @@ -231,26 +231,15 @@ func GetSubscriptionsForDeletedTask(s *xorm.Session, task *Task) (subscriptions return nil, err } - for _, sub := range subs[task.ID] { - if sub.User == nil { - continue - } - - p := &Project{ID: task.ProjectID} - canRead, _, err := p.CanRead(s, sub.User) - if err != nil { - if IsErrProjectDoesNotExist(err) { - return nil, nil - } - return nil, err - } - - if canRead { - subscriptions = append(subscriptions, sub) + filtered, err := filterSubscriptionsByProjectPermission(s, subs, map[int64]int64{task.ID: task.ProjectID}) + if err != nil { + if IsErrProjectDoesNotExist(err) { + return nil, nil } + return nil, err } - return subscriptions, nil + return filtered[task.ID], nil } // This function returns a matching subscription for an entity and user. @@ -449,5 +438,111 @@ ORDER BY t.id, sh.user_id`, subscriptions[sub.OriginalEntityID] = append(subscriptions[sub.OriginalEntityID], &sub.SubscriptionWithUser) } - return subscriptions, nil + // A soft-deleted task cannot be resolved back to its project here, so that + // caller filters with the project id it already holds. + if userOnly || includeDeletedTasks { + return subscriptions, nil + } + + return filterSubscriptionsByReadPermission(s, entityType, subscriptions) +} + +type subscribedEntityProject struct { + ID int64 + ProjectID int64 +} + +// Subscription rows outlive access, so subscribers who lost read access to the entity's project are +// filtered out here rather than deleted: a subscription is user intent and resumes once access returns. +func filterSubscriptionsByReadPermission(s *xorm.Session, entityType SubscriptionEntityType, subscriptions map[int64][]*SubscriptionWithUser) (map[int64][]*SubscriptionWithUser, error) { + if len(subscriptions) == 0 { + return subscriptions, nil + } + + entityIDs := make([]int64, 0, len(subscriptions)) + for entityID := range subscriptions { + entityIDs = append(entityIDs, entityID) + } + + projectIDForEntity := make(map[int64]int64, len(subscriptions)) + switch entityType { + case SubscriptionEntityProject: + for _, entityID := range entityIDs { + projectIDForEntity[entityID] = entityID + } + case SubscriptionEntityTask: + tasks := []*subscribedEntityProject{} + err := s.Table("tasks"). + In("id", entityIDs). + Cols("id", "project_id"). + Find(&tasks) + if err != nil { + return nil, err + } + for _, t := range tasks { + projectIDForEntity[t.ID] = t.ProjectID + } + } + + return filterSubscriptionsByProjectPermission(s, subscriptions, projectIDForEntity) +} + +// filterSubscriptionsByProjectPermission drops subscribers who can no longer read +// the project each entity belongs to. +func filterSubscriptionsByProjectPermission(s *xorm.Session, subscriptions map[int64][]*SubscriptionWithUser, projectIDForEntity map[int64]int64) (map[int64][]*SubscriptionWithUser, error) { + subscribers := make(map[int64]*user.User) + projectIDs := make([]int64, 0, len(projectIDForEntity)) + seenProjectID := make(map[int64]bool, len(projectIDForEntity)) + for entityID, subs := range subscriptions { + projectID, has := projectIDForEntity[entityID] + if !has { + continue + } + + for _, sub := range subs { + if sub.User == nil { + continue + } + + if _, has := subscribers[sub.User.ID]; !has { + subscribers[sub.User.ID] = sub.User + } + if !seenProjectID[projectID] { + seenProjectID[projectID] = true + projectIDs = append(projectIDs, projectID) + } + } + } + + readable := make(map[int64]map[int64]*projectReadPermission, len(subscribers)) + for userID, u := range subscribers { + permissions, err := checkReadPermissionsForProjects(s, u, projectIDs) + if err != nil { + return nil, err + } + readable[userID] = permissions + } + + filtered := make(map[int64][]*SubscriptionWithUser, len(subscriptions)) + for entityID, subs := range subscriptions { + projectID, has := projectIDForEntity[entityID] + if !has { + continue + } + + for _, sub := range subs { + if sub.User == nil { + continue + } + + permission, has := readable[sub.User.ID][projectID] + if !has || !permission.canRead { + continue + } + + filtered[entityID] = append(filtered[entityID], sub) + } + } + + return filtered, nil } diff --git a/pkg/models/subscription_test.go b/pkg/models/subscription_test.go index 7757ce61a..aeb372ec1 100644 --- a/pkg/models/subscription_test.go +++ b/pkg/models/subscription_test.go @@ -24,6 +24,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "xorm.io/xorm" ) func TestSubscriptionGetTypeFromString(t *testing.T) { @@ -354,6 +355,72 @@ func TestSubscriptionGet(t *testing.T) { }) } +func TestGetSubscriptionsForEntitySkipsUsersWithoutReadAccess(t *testing.T) { + const ( + taskID int64 = 32 + projectID int64 = 3 + withAccess int64 = 2 + lostAccess int64 = 6 + ) + + subscribeBoth := func(t *testing.T, s *xorm.Session, entityType SubscriptionEntityType, entityID int64) { + for _, userID := range []int64{withAccess, lostAccess} { + _, err := s.Insert(&Subscription{ + UserID: userID, + EntityType: entityType, + EntityID: entityID, + }) + require.NoError(t, err) + } + } + + subscriberIDs := func(subs []*SubscriptionWithUser) (ids []int64) { + for _, sub := range subs { + ids = append(ids, sub.UserID) + } + return ids + } + + t.Run("task", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + subscribeBoth(t, s, SubscriptionEntityTask, taskID) + + subs, err := GetSubscriptionsForEntity(s, SubscriptionEntityTask, taskID) + require.NoError(t, err) + assert.Equal(t, []int64{withAccess}, subscriberIDs(subs)) + }) + t.Run("project", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + subscribeBoth(t, s, SubscriptionEntityProject, projectID) + + subs, err := GetSubscriptionsForEntity(s, SubscriptionEntityProject, projectID) + require.NoError(t, err) + assert.Equal(t, []int64{withAccess}, subscriberIDs(subs)) + }) + t.Run("user only lookup is not filtered", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + subscribeBoth(t, s, SubscriptionEntityTask, taskID) + subscribeBoth(t, s, SubscriptionEntityProject, projectID) + + subs, err := GetSubscriptionsForEntitiesAndUser(s, SubscriptionEntityTask, []int64{taskID}, &user.User{ID: lostAccess}) + require.NoError(t, err) + assert.Equal(t, []int64{lostAccess}, subscriberIDs(subs[taskID])) + + subs, err = GetSubscriptionsForEntitiesAndUser(s, SubscriptionEntityProject, []int64{projectID}, &user.User{ID: lostAccess}) + require.NoError(t, err) + assert.Equal(t, []int64{lostAccess}, subscriberIDs(subs[projectID])) + }) +} + func TestSubscription_NoCrossUserProjectInheritance(t *testing.T) { db.LoadAndAssertFixtures(t) s := db.NewSession()