@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Fix an error when users receive notifications about objects they can no longer see
Summary:
Ref T13623. The change in D21577 could lead to a case where we try to access stories the user can't see.
Move the story-loading piece to "willFilterPage()" to make our way thorugh this.
Test Plan:
- Made FeedStory return nothing to simulate invisible notifications, loaded page.
- Before: index access fatal.
- After: clean "no notifications".
- Loaded notifications normally, saw normal notifications.
Maniphest Tasks: T13623
Differential Revision: https://secure.phabricator.com/D21603
+38
-23
+38
-23
src/applications/notification/query/PhabricatorNotificationQuery.php
+38
-23
src/applications/notification/query/PhabricatorNotificationQuery.php
···
63
63
$this->buildWhereClause($conn),
64
64
$this->buildLimitClause($conn));
65
65
66
-
// See T13623. Although most queries for notifications return unique
67
-
// stories, this isn't a guarantee.
68
-
$story_map = ipull($data, null, 'chronologicalKey');
69
-
70
-
$stories = PhabricatorFeedStory::loadAllFromRows(
71
-
$story_map,
72
-
$this->getViewer());
73
-
$stories = mpull($stories, null, 'getChronologicalKey');
74
-
75
-
$results = array();
76
-
foreach ($data as $row) {
77
-
$story_key = $row['chronologicalKey'];
78
-
$has_viewed = $row['hasViewed'];
79
-
80
-
$results[] = id(clone $stories[$story_key])
81
-
->setHasViewed($has_viewed);
82
-
}
83
-
84
-
return $results;
66
+
return $data;
85
67
}
86
68
87
69
protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
···
111
93
return $where;
112
94
}
113
95
114
-
protected function willFilterPage(array $stories) {
115
-
foreach ($stories as $key => $story) {
96
+
protected function willFilterPage(array $rows) {
97
+
// See T13623. The policy model here is outdated and awkward.
98
+
99
+
// Users may have notifications about objects they can no longer see.
100
+
// Two ways this can arise: destroy an object; or change an object's
101
+
// view policy to exclude a user.
102
+
103
+
// "PhabricatorFeedStory::loadAllFromRows()" does its own policy filtering.
104
+
// This doesn't align well with modern query sequencing, but we should be
105
+
// able to get away with it by loading here.
106
+
107
+
// See T13623. Although most queries for notifications return unique
108
+
// stories, this isn't a guarantee.
109
+
$story_map = ipull($rows, null, 'chronologicalKey');
110
+
111
+
$viewer = $this->getViewer();
112
+
$stories = PhabricatorFeedStory::loadAllFromRows($story_map, $viewer);
113
+
$stories = mpull($stories, null, 'getChronologicalKey');
114
+
115
+
$results = array();
116
+
foreach ($rows as $row) {
117
+
$story_key = $row['chronologicalKey'];
118
+
$has_viewed = $row['hasViewed'];
119
+
120
+
if (!isset($stories[$story_key])) {
121
+
// NOTE: We can't call "didRejectResult()" here because we don't have
122
+
// a policy object to pass.
123
+
continue;
124
+
}
125
+
126
+
$story = id(clone $stories[$story_key])
127
+
->setHasViewed($has_viewed);
128
+
116
129
if (!$story->isVisibleInNotifications()) {
117
-
unset($stories[$key]);
130
+
continue;
118
131
}
132
+
133
+
$results[] = $story;
119
134
}
120
135
121
-
return $stories;
136
+
return $results;
122
137
}
123
138
124
139
protected function getDefaultOrderVector() {