fix(notifications): don't notify subscribers who lost access to the entity

Subscriptions outlive access: nothing purges them when a project is unshared, and access can change with no revocation event at all, so a user who can no longer open a task kept receiving its comment bodies, assignment details and deletion notices by mail and in the feed. Filter subscribers by current read permission when the subscription is fetched, so every listener is covered by one check. Rows are kept rather than deleted - a subscription is user intent and resumes if access does. GetSubscriptionsForDeletedTask keeps its own lookup because a soft-deleted task cannot be resolved back to its project, but it now reuses the same filter with the project id it already holds.
2026-08-31 03:46:56 -05:00 · 2026-07-29 07:33:35 +00:00
parent ec4dbb8600
commit e252954229
4 changed files with 318 additions and 22 deletions
@@ -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
 		}
 	}

@@ -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) {
@@ -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
 }
@@ -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()