Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Commit 420484a

Browse files
authored
Allow capping a room's retention policy (#8104)
1 parent 64e8a46 commit 420484a

File tree

6 files changed

+127
-107
lines changed

6 files changed

+127
-107
lines changed

changelog.d/8104.bugfix

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fix a bug introduced in v1.7.2 impacting message retention policies that would allow federated homeservers to dictate a retention period that's lower than the configured minimum allowed duration in the configuration file.

docs/sample_config.yaml

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -378,11 +378,10 @@ retention:
378378
# min_lifetime: 1d
379379
# max_lifetime: 1y
380380

381-
# Retention policy limits. If set, a user won't be able to send a
382-
# 'm.room.retention' event which features a 'min_lifetime' or a 'max_lifetime'
383-
# that's not within this range. This is especially useful in closed federations,
384-
# in which server admins can make sure every federating server applies the same
385-
# rules.
381+
# Retention policy limits. If set, and the state of a room contains a
382+
# 'm.room.retention' event in its state which contains a 'min_lifetime' or a
383+
# 'max_lifetime' that's out of these bounds, Synapse will cap the room's policy
384+
# to these limits when running purge jobs.
386385
#
387386
#allowed_lifetime_min: 1d
388387
#allowed_lifetime_max: 1y
@@ -408,12 +407,19 @@ retention:
408407
# (e.g. every 12h), but not want that purge to be performed by a job that's
409408
# iterating over every room it knows, which could be heavy on the server.
410409
#
410+
# If any purge job is configured, it is strongly recommended to have at least
411+
# a single job with neither 'shortest_max_lifetime' nor 'longest_max_lifetime'
412+
# set, or one job without 'shortest_max_lifetime' and one job without
413+
# 'longest_max_lifetime' set. Otherwise some rooms might be ignored, even if
414+
# 'allowed_lifetime_min' and 'allowed_lifetime_max' are set, because capping a
415+
# room's policy to these values is done after the policies are retrieved from
416+
# Synapse's database (which is done using the range specified in a purge job's
417+
# configuration).
418+
#
411419
#purge_jobs:
412-
# - shortest_max_lifetime: 1d
413-
# longest_max_lifetime: 3d
420+
# - longest_max_lifetime: 3d
414421
# interval: 12h
415422
# - shortest_max_lifetime: 3d
416-
# longest_max_lifetime: 1y
417423
# interval: 1d
418424

419425
# Inhibits the /requestToken endpoints from returning an error that might leak

synapse/config/server.py

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -961,11 +961,10 @@ def generate_config_section(
961961
# min_lifetime: 1d
962962
# max_lifetime: 1y
963963
964-
# Retention policy limits. If set, a user won't be able to send a
965-
# 'm.room.retention' event which features a 'min_lifetime' or a 'max_lifetime'
966-
# that's not within this range. This is especially useful in closed federations,
967-
# in which server admins can make sure every federating server applies the same
968-
# rules.
964+
# Retention policy limits. If set, and the state of a room contains a
965+
# 'm.room.retention' event in its state which contains a 'min_lifetime' or a
966+
# 'max_lifetime' that's out of these bounds, Synapse will cap the room's policy
967+
# to these limits when running purge jobs.
969968
#
970969
#allowed_lifetime_min: 1d
971970
#allowed_lifetime_max: 1y
@@ -991,12 +990,19 @@ def generate_config_section(
991990
# (e.g. every 12h), but not want that purge to be performed by a job that's
992991
# iterating over every room it knows, which could be heavy on the server.
993992
#
993+
# If any purge job is configured, it is strongly recommended to have at least
994+
# a single job with neither 'shortest_max_lifetime' nor 'longest_max_lifetime'
995+
# set, or one job without 'shortest_max_lifetime' and one job without
996+
# 'longest_max_lifetime' set. Otherwise some rooms might be ignored, even if
997+
# 'allowed_lifetime_min' and 'allowed_lifetime_max' are set, because capping a
998+
# room's policy to these values is done after the policies are retrieved from
999+
# Synapse's database (which is done using the range specified in a purge job's
1000+
# configuration).
1001+
#
9941002
#purge_jobs:
995-
# - shortest_max_lifetime: 1d
996-
# longest_max_lifetime: 3d
1003+
# - longest_max_lifetime: 3d
9971004
# interval: 12h
9981005
# - shortest_max_lifetime: 3d
999-
# longest_max_lifetime: 1y
10001006
# interval: 1d
10011007
10021008
# Inhibits the /requestToken endpoints from returning an error that might leak

synapse/events/validator.py

Lines changed: 3 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -74,15 +74,14 @@ def validate_new(self, event, config):
7474
)
7575

7676
if event.type == EventTypes.Retention:
77-
self._validate_retention(event, config)
77+
self._validate_retention(event)
7878

79-
def _validate_retention(self, event, config):
79+
def _validate_retention(self, event):
8080
"""Checks that an event that defines the retention policy for a room respects the
81-
boundaries imposed by the server's administrator.
81+
format enforced by the spec.
8282
8383
Args:
8484
event (FrozenEvent): The event to validate.
85-
config (Config): The homeserver's configuration.
8685
"""
8786
min_lifetime = event.content.get("min_lifetime")
8887
max_lifetime = event.content.get("max_lifetime")
@@ -95,32 +94,6 @@ def _validate_retention(self, event, config):
9594
errcode=Codes.BAD_JSON,
9695
)
9796

98-
if (
99-
config.retention_allowed_lifetime_min is not None
100-
and min_lifetime < config.retention_allowed_lifetime_min
101-
):
102-
raise SynapseError(
103-
code=400,
104-
msg=(
105-
"'min_lifetime' can't be lower than the minimum allowed"
106-
" value enforced by the server's administrator"
107-
),
108-
errcode=Codes.BAD_JSON,
109-
)
110-
111-
if (
112-
config.retention_allowed_lifetime_max is not None
113-
and min_lifetime > config.retention_allowed_lifetime_max
114-
):
115-
raise SynapseError(
116-
code=400,
117-
msg=(
118-
"'min_lifetime' can't be greater than the maximum allowed"
119-
" value enforced by the server's administrator"
120-
),
121-
errcode=Codes.BAD_JSON,
122-
)
123-
12497
if max_lifetime is not None:
12598
if not isinstance(max_lifetime, int):
12699
raise SynapseError(
@@ -129,32 +102,6 @@ def _validate_retention(self, event, config):
129102
errcode=Codes.BAD_JSON,
130103
)
131104

132-
if (
133-
config.retention_allowed_lifetime_min is not None
134-
and max_lifetime < config.retention_allowed_lifetime_min
135-
):
136-
raise SynapseError(
137-
code=400,
138-
msg=(
139-
"'max_lifetime' can't be lower than the minimum allowed value"
140-
" enforced by the server's administrator"
141-
),
142-
errcode=Codes.BAD_JSON,
143-
)
144-
145-
if (
146-
config.retention_allowed_lifetime_max is not None
147-
and max_lifetime > config.retention_allowed_lifetime_max
148-
):
149-
raise SynapseError(
150-
code=400,
151-
msg=(
152-
"'max_lifetime' can't be greater than the maximum allowed"
153-
" value enforced by the server's administrator"
154-
),
155-
errcode=Codes.BAD_JSON,
156-
)
157-
158105
if (
159106
min_lifetime is not None
160107
and max_lifetime is not None

synapse/handlers/pagination.py

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,9 @@ def __init__(self, hs):
8282

8383
self._retention_default_max_lifetime = hs.config.retention_default_max_lifetime
8484

85+
self._retention_allowed_lifetime_min = hs.config.retention_allowed_lifetime_min
86+
self._retention_allowed_lifetime_max = hs.config.retention_allowed_lifetime_max
87+
8588
if hs.config.retention_enabled:
8689
# Run the purge jobs described in the configuration file.
8790
for job in hs.config.retention_purge_jobs:
@@ -111,7 +114,7 @@ async def purge_history_for_rooms_in_range(self, min_ms, max_ms):
111114
the range to handle (inclusive). If None, it means that the range has no
112115
upper limit.
113116
"""
114-
# We want the storage layer to to include rooms with no retention policy in its
117+
# We want the storage layer to include rooms with no retention policy in its
115118
# return value only if a default retention policy is defined in the server's
116119
# configuration and that policy's 'max_lifetime' is either lower (or equal) than
117120
# max_ms or higher than min_ms (or both).
@@ -152,13 +155,32 @@ async def purge_history_for_rooms_in_range(self, min_ms, max_ms):
152155
)
153156
continue
154157

155-
max_lifetime = retention_policy["max_lifetime"]
158+
# If max_lifetime is None, it means that the room has no retention policy.
159+
# Given we only retrieve such rooms when there's a default retention policy
160+
# defined in the server's configuration, we can safely assume that's the
161+
# case and use it for this room.
162+
max_lifetime = (
163+
retention_policy["max_lifetime"] or self._retention_default_max_lifetime
164+
)
156165

157-
if max_lifetime is None:
158-
# If max_lifetime is None, it means that include_null equals True,
159-
# therefore we can safely assume that there is a default policy defined
160-
# in the server's configuration.
161-
max_lifetime = self._retention_default_max_lifetime
166+
# Cap the effective max_lifetime to be within the range allowed in the
167+
# config.
168+
# We do this in two steps:
169+
# 1. Make sure it's higher or equal to the minimum allowed value, and if
170+
# it's not replace it with that value. This is because the server
171+
# operator can be required to not delete information before a given
172+
# time, e.g. to comply with freedom of information laws.
173+
# 2. Make sure the resulting value is lower or equal to the maximum allowed
174+
# value, and if it's not replace it with that value. This is because the
175+
# server operator can be required to delete any data after a specific
176+
# amount of time.
177+
if self._retention_allowed_lifetime_min is not None:
178+
max_lifetime = max(self._retention_allowed_lifetime_min, max_lifetime)
179+
180+
if self._retention_allowed_lifetime_max is not None:
181+
max_lifetime = min(max_lifetime, self._retention_allowed_lifetime_max)
182+
183+
logger.debug("[purge] max_lifetime for room %s: %s", room_id, max_lifetime)
162184

163185
# Figure out what token we should start purging at.
164186
ts = self.clock.time_msec() - max_lifetime

tests/rest/client/test_retention.py

Lines changed: 66 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -45,50 +45,63 @@ def make_homeserver(self, reactor, clock):
4545
}
4646

4747
self.hs = self.setup_test_homeserver(config=config)
48+
4849
return self.hs
4950

5051
def prepare(self, reactor, clock, homeserver):
5152
self.user_id = self.register_user("user", "password")
5253
self.token = self.login("user", "password")
5354

54-
def test_retention_state_event(self):
55-
"""Tests that the server configuration can limit the values a user can set to the
56-
room's retention policy.
55+
self.store = self.hs.get_datastore()
56+
self.serializer = self.hs.get_event_client_serializer()
57+
self.clock = self.hs.get_clock()
58+
59+
def test_retention_event_purged_with_state_event(self):
60+
"""Tests that expired events are correctly purged when the room's retention policy
61+
is defined by a state event.
5762
"""
5863
room_id = self.helper.create_room_as(self.user_id, tok=self.token)
5964

65+
# Set the room's retention period to 2 days.
66+
lifetime = one_day_ms * 2
6067
self.helper.send_state(
6168
room_id=room_id,
6269
event_type=EventTypes.Retention,
63-
body={"max_lifetime": one_day_ms * 4},
70+
body={"max_lifetime": lifetime},
6471
tok=self.token,
65-
expect_code=400,
6672
)
6773

74+
self._test_retention_event_purged(room_id, one_day_ms * 1.5)
75+
76+
def test_retention_event_purged_with_state_event_outside_allowed(self):
77+
"""Tests that the server configuration can override the policy for a room when
78+
running the purge jobs.
79+
"""
80+
room_id = self.helper.create_room_as(self.user_id, tok=self.token)
81+
82+
# Set a max_lifetime higher than the maximum allowed value.
6883
self.helper.send_state(
6984
room_id=room_id,
7085
event_type=EventTypes.Retention,
71-
body={"max_lifetime": one_hour_ms},
86+
body={"max_lifetime": one_day_ms * 4},
7287
tok=self.token,
73-
expect_code=400,
7488
)
7589

76-
def test_retention_event_purged_with_state_event(self):
77-
"""Tests that expired events are correctly purged when the room's retention policy
78-
is defined by a state event.
79-
"""
80-
room_id = self.helper.create_room_as(self.user_id, tok=self.token)
90+
# Check that the event is purged after waiting for the maximum allowed duration
91+
# instead of the one specified in the room's policy.
92+
self._test_retention_event_purged(room_id, one_day_ms * 1.5)
8193

82-
# Set the room's retention period to 2 days.
83-
lifetime = one_day_ms * 2
94+
# Set a max_lifetime lower than the minimum allowed value.
8495
self.helper.send_state(
8596
room_id=room_id,
8697
event_type=EventTypes.Retention,
87-
body={"max_lifetime": lifetime},
98+
body={"max_lifetime": one_hour_ms},
8899
tok=self.token,
89100
)
90101

91-
self._test_retention_event_purged(room_id, one_day_ms * 1.5)
102+
# Check that the event is purged after waiting for the minimum allowed duration
103+
# instead of the one specified in the room's policy.
104+
self._test_retention_event_purged(room_id, one_day_ms * 0.5)
92105

93106
def test_retention_event_purged_without_state_event(self):
94107
"""Tests that expired events are correctly purged when the room's retention policy
@@ -140,7 +153,27 @@ def test_visibility(self):
140153
# That event should be the second, not outdated event.
141154
self.assertEqual(filtered_events[0].event_id, valid_event_id, filtered_events)
142155

143-
def _test_retention_event_purged(self, room_id, increment):
156+
def _test_retention_event_purged(self, room_id: str, increment: float):
157+
"""Run the following test scenario to test the message retention policy support:
158+
159+
1. Send event 1
160+
2. Increment time by `increment`
161+
3. Send event 2
162+
4. Increment time by `increment`
163+
5. Check that event 1 has been purged
164+
6. Check that event 2 has not been purged
165+
7. Check that state events that were sent before event 1 aren't purged.
166+
The main reason for sending a second event is because currently Synapse won't
167+
purge the latest message in a room because it would otherwise result in a lack of
168+
forward extremities for this room. It's also a good thing to ensure the purge jobs
169+
aren't too greedy and purge messages they shouldn't.
170+
171+
Args:
172+
room_id: The ID of the room to test retention in.
173+
increment: The number of milliseconds to advance the clock each time. Must be
174+
defined so that events in the room aren't purged if they are `increment`
175+
old but are purged if they are `increment * 2` old.
176+
"""
144177
# Get the create event to, later, check that we can still access it.
145178
message_handler = self.hs.get_message_handler()
146179
create_event = self.get_success(
@@ -156,7 +189,7 @@ def _test_retention_event_purged(self, room_id, increment):
156189
expired_event_id = resp.get("event_id")
157190

158191
# Check that we can retrieve the event.
159-
expired_event = self.get_event(room_id, expired_event_id)
192+
expired_event = self.get_event(expired_event_id)
160193
self.assertEqual(
161194
expired_event.get("content", {}).get("body"), "1", expired_event
162195
)
@@ -174,26 +207,31 @@ def _test_retention_event_purged(self, room_id, increment):
174207
# one should still be kept.
175208
self.reactor.advance(increment / 1000)
176209

177-
# Check that the event has been purged from the database.
178-
self.get_event(room_id, expired_event_id, expected_code=404)
210+
# Check that the first event has been purged from the database, i.e. that we
211+
# can't retrieve it anymore, because it has expired.
212+
self.get_event(expired_event_id, expect_none=True)
179213

180-
# Check that the event that hasn't been purged can still be retrieved.
181-
valid_event = self.get_event(room_id, valid_event_id)
214+
# Check that the event that hasn't expired can still be retrieved.
215+
valid_event = self.get_event(valid_event_id)
182216
self.assertEqual(valid_event.get("content", {}).get("body"), "2", valid_event)
183217

184218
# Check that we can still access state events that were sent before the event that
185219
# has been purged.
186220
self.get_event(room_id, create_event.event_id)
187221

188-
def get_event(self, room_id, event_id, expected_code=200):
189-
url = "/_matrix/client/r0/rooms/%s/event/%s" % (room_id, event_id)
222+
def get_event(self, event_id, expect_none=False):
223+
event = self.get_success(self.store.get_event(event_id, allow_none=True))
190224

191-
request, channel = self.make_request("GET", url, access_token=self.token)
192-
self.render(request)
225+
if expect_none:
226+
self.assertIsNone(event)
227+
return {}
193228

194-
self.assertEqual(channel.code, expected_code, channel.result)
229+
self.assertIsNotNone(event)
195230

196-
return channel.json_body
231+
time_now = self.clock.time_msec()
232+
serialized = self.get_success(self.serializer.serialize_event(event, time_now))
233+
234+
return serialized
197235

198236

199237
class RetentionNoDefaultPolicyTestCase(unittest.HomeserverTestCase):

0 commit comments

Comments
 (0)