From 1791ae492874add2e367aeccb1c799cc00c0a96f Mon Sep 17 00:00:00 2001 From: Aaron Raimist Date: Sat, 16 Oct 2021 16:27:46 -0500 Subject: [PATCH 1/6] Show error when timestamp in seconds is provided to the /purge_media_cache API Signed-off-by: Aaron Raimist --- docs/admin_api/media_admin_api.md | 6 +- synapse/rest/admin/media.py | 21 +++++ tests/rest/admin/test_media.py | 124 +++++++++++++++++++++++++++--- tests/utils.py | 3 + 4 files changed, 139 insertions(+), 15 deletions(-) diff --git a/docs/admin_api/media_admin_api.md b/docs/admin_api/media_admin_api.md index ea05bd6e4465..60b8bc7379a2 100644 --- a/docs/admin_api/media_admin_api.md +++ b/docs/admin_api/media_admin_api.md @@ -257,9 +257,9 @@ POST /_synapse/admin/v1/media//delete?before_ts= URL Parameters * `server_name`: string - The name of your local server (e.g `matrix.org`). -* `before_ts`: string representing a positive integer - Unix timestamp in ms. +* `before_ts`: string representing a positive integer - Unix timestamp in milliseconds. Files that were last used before this timestamp will be deleted. It is the timestamp of -last access and not the timestamp creation. +last access, not the timestamp when the file was created. * `size_gt`: Optional - string representing a positive integer - Size of the media in bytes. Files that are larger will be deleted. Defaults to `0`. * `keep_profiles`: Optional - string representing a boolean - Switch to also delete files @@ -302,7 +302,7 @@ POST /_synapse/admin/v1/purge_media_cache?before_ts= URL Parameters -* `unix_timestamp_in_ms`: string representing a positive integer - Unix timestamp in ms. +* `unix_timestamp_in_ms`: string representing a positive integer - Unix timestamp in milliseconds. All cached media that was last accessed before this timestamp will be removed. Response: diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index 8ce443049e23..d6de69ac8121 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -231,6 +231,20 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: before_ts = parse_integer(request, "before_ts", required=True) logger.info("before_ts: %r", before_ts) + if before_ts < 0: + raise SynapseError( + 400, + "Query parameter before_ts must be a string representing a positive integer.", + errcode=Codes.INVALID_PARAM, + ) + elif before_ts < 30000000000: # Dec 1970 in milliseconds, Aug 2920 in seconds + raise SynapseError( + 400, + "Query parameter before_ts you provided is from the year 1970. " + + "Double check that you are providing a timestamp in milliseconds.", + errcode=Codes.INVALID_PARAM, + ) + ret = await self.media_repository.delete_old_remote_media(before_ts) return 200, ret @@ -294,6 +308,13 @@ async def on_POST( "Query parameter before_ts must be a string representing a positive integer.", errcode=Codes.INVALID_PARAM, ) + elif before_ts < 30000000000: # Dec 1970 in milliseconds, Aug 2920 in seconds + raise SynapseError( + 400, + "Query parameter before_ts you provided is from the year 1970. " + + "Double check that you are providing a timestamp in milliseconds.", + errcode=Codes.INVALID_PARAM, + ) if size_gt < 0: raise SynapseError( 400, diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index ce30a19213ab..a05bf4d2918f 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -26,6 +26,10 @@ from tests import unittest from tests.server import FakeSite, make_request from tests.test_utils import SMALL_PNG +from tests.utils import MockClock + +VALID_TIMESTAMP = 1609459200000 # 2021-01-01 in milliseconds +INVALID_TIMESTAMP_IN_S = 1893456000 # 2030-01-01 in seconds class DeleteMediaByIDTestCase(unittest.HomeserverTestCase): @@ -203,6 +207,8 @@ def prepare(self, reactor, clock, hs): self.filepaths = MediaFilePaths(hs.config.media.media_store_path) self.url = "/_synapse/admin/v1/media/%s/delete" % self.server_name + self.clock = MockClock() + def test_no_auth(self): """ Try to delete media without authentication. @@ -237,7 +243,7 @@ def test_media_is_not_local(self): channel = self.make_request( "POST", - url + "?before_ts=1234", + url + f"?before_ts={VALID_TIMESTAMP}", access_token=self.admin_user_tok, ) @@ -279,7 +285,21 @@ def test_invalid_parameter(self): channel = self.make_request( "POST", - self.url + "?before_ts=1234&size_gt=-1234", + self.url + f"?before_ts={INVALID_TIMESTAMP_IN_S}", + access_token=self.admin_user_tok, + ) + + self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) + self.assertEqual( + "Query parameter before_ts you provided is from the year 1970. " + + "Double check that you are providing a timestamp in milliseconds.", + channel.json_body["error"], + ) + + channel = self.make_request( + "POST", + self.url + f"?before_ts={VALID_TIMESTAMP}&size_gt=-1234", access_token=self.admin_user_tok, ) @@ -292,7 +312,7 @@ def test_invalid_parameter(self): channel = self.make_request( "POST", - self.url + "?before_ts=1234&keep_profiles=not_bool", + self.url + f"?before_ts={VALID_TIMESTAMP}&keep_profiles=not_bool", access_token=self.admin_user_tok, ) @@ -319,7 +339,7 @@ def test_delete_media_never_accessed(self): self.assertTrue(os.path.exists(local_path)) # timestamp after upload/create - now_ms = self.clock.time_msec() + now_ms = self.clock.real_time_msec() channel = self.make_request( "POST", self.url + "?before_ts=" + str(now_ms), @@ -340,7 +360,7 @@ def test_keep_media_by_date(self): """ # timestamp before upload - now_ms = self.clock.time_msec() + now_ms = self.clock.real_time_msec() server_and_media_id = self._create_media() self._access_media(server_and_media_id) @@ -356,7 +376,7 @@ def test_keep_media_by_date(self): self._access_media(server_and_media_id) # timestamp after upload - now_ms = self.clock.time_msec() + now_ms = self.clock.real_time_msec() channel = self.make_request( "POST", self.url + "?before_ts=" + str(now_ms), @@ -380,7 +400,7 @@ def test_keep_media_by_size(self): self._access_media(server_and_media_id) - now_ms = self.clock.time_msec() + now_ms = self.clock.real_time_msec() channel = self.make_request( "POST", self.url + "?before_ts=" + str(now_ms) + "&size_gt=67", @@ -391,7 +411,7 @@ def test_keep_media_by_size(self): self._access_media(server_and_media_id) - now_ms = self.clock.time_msec() + now_ms = self.clock.real_time_msec() channel = self.make_request( "POST", self.url + "?before_ts=" + str(now_ms) + "&size_gt=66", @@ -424,7 +444,7 @@ def test_keep_media_by_user_avatar(self): ) self.assertEqual(200, channel.code, msg=channel.json_body) - now_ms = self.clock.time_msec() + now_ms = self.clock.real_time_msec() channel = self.make_request( "POST", self.url + "?before_ts=" + str(now_ms) + "&keep_profiles=true", @@ -435,7 +455,7 @@ def test_keep_media_by_user_avatar(self): self._access_media(server_and_media_id) - now_ms = self.clock.time_msec() + now_ms = self.clock.real_time_msec() channel = self.make_request( "POST", self.url + "?before_ts=" + str(now_ms) + "&keep_profiles=false", @@ -469,7 +489,7 @@ def test_keep_media_by_room_avatar(self): ) self.assertEqual(200, channel.code, msg=channel.json_body) - now_ms = self.clock.time_msec() + now_ms = self.clock.real_time_msec() channel = self.make_request( "POST", self.url + "?before_ts=" + str(now_ms) + "&keep_profiles=true", @@ -480,7 +500,7 @@ def test_keep_media_by_room_avatar(self): self._access_media(server_and_media_id) - now_ms = self.clock.time_msec() + now_ms = self.clock.real_time_msec() channel = self.make_request( "POST", self.url + "?before_ts=" + str(now_ms) + "&keep_profiles=false", @@ -767,3 +787,83 @@ def test_protect_media(self): media_info = self.get_success(self.store.get_local_media(self.media_id)) self.assertFalse(media_info["safe_from_quarantine"]) + + +class PurgeMediaCacheTestCase(unittest.HomeserverTestCase): + + servlets = [ + synapse.rest.admin.register_servlets, + synapse.rest.admin.register_servlets_for_media_repo, + login.register_servlets, + profile.register_servlets, + room.register_servlets, + ] + + def prepare(self, reactor, clock, hs): + self.media_repo = hs.get_media_repository_resource() + self.server_name = hs.hostname + + self.admin_user = self.register_user("admin", "pass", admin=True) + self.admin_user_tok = self.login("admin", "pass") + + self.filepaths = MediaFilePaths(hs.config.media.media_store_path) + self.url = "/_synapse/admin/v1/purge_media_cache" + + self.clock = MockClock() + + def test_no_auth(self): + """ + Try to delete media without authentication. + """ + + channel = self.make_request("POST", self.url, b"{}") + + self.assertEqual(401, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(Codes.MISSING_TOKEN, channel.json_body["errcode"]) + + def test_requester_is_not_admin(self): + """ + If the user is not a server admin, an error is returned. + """ + self.other_user = self.register_user("user", "pass") + self.other_user_token = self.login("user", "pass") + + channel = self.make_request( + "POST", + self.url, + access_token=self.other_user_token, + ) + + self.assertEqual(403, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(Codes.FORBIDDEN, channel.json_body["errcode"]) + + def test_invalid_parameter(self): + """ + If parameters are invalid, an error is returned. + """ + channel = self.make_request( + "POST", + self.url + "?before_ts=-1234", + access_token=self.admin_user_tok, + ) + + self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) + self.assertEqual( + "Query parameter before_ts must be a string representing a positive integer.", + channel.json_body["error"], + ) + + channel = self.make_request( + "POST", + self.url + f"?before_ts={INVALID_TIMESTAMP_IN_S}", + access_token=self.admin_user_tok, + ) + + self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) + self.assertEqual( + "Query parameter before_ts you provided is from the year 1970. " + + "Double check that you are providing a timestamp in milliseconds.", + channel.json_body["error"], + ) diff --git a/tests/utils.py b/tests/utils.py index cf8ba5c5db35..ec505395b3bf 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -477,6 +477,9 @@ def time(self): def time_msec(self): return self.time() * 1000 + def real_time_msec(self): + return int(time.time() * 1000) + def call_later(self, delay, callback, *args, **kwargs): ctx = current_context() From cb5a6223a17bfbd69f657bb0ef23e1f2204d7158 Mon Sep 17 00:00:00 2001 From: Aaron Raimist Date: Sat, 16 Oct 2021 16:29:09 -0500 Subject: [PATCH 2/6] Prevent media admin APIs from being called if there are extra characters at end of URL Signed-off-by: Aaron Raimist --- synapse/rest/admin/media.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index d6de69ac8121..b7afbf167f1b 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -40,7 +40,7 @@ class QuarantineMediaInRoom(RestServlet): """ PATTERNS = [ - *admin_patterns("/room/(?P[^/]+)/media/quarantine"), + *admin_patterns("/room/(?P[^/]+)/media/quarantine$"), # This path kept around for legacy reasons *admin_patterns("/quarantine_media/(?P[^/]+)"), ] @@ -70,7 +70,7 @@ class QuarantineMediaByUser(RestServlet): this server. """ - PATTERNS = admin_patterns("/user/(?P[^/]+)/media/quarantine") + PATTERNS = admin_patterns("/user/(?P[^/]+)/media/quarantine$") def __init__(self, hs: "HomeServer"): self.store = hs.get_datastore() @@ -199,7 +199,7 @@ async def on_POST( class ListMediaInRoom(RestServlet): """Lists all of the media in a given room.""" - PATTERNS = admin_patterns("/room/(?P[^/]+)/media") + PATTERNS = admin_patterns("/room/(?P[^/]+)/media$") def __init__(self, hs: "HomeServer"): self.store = hs.get_datastore() @@ -219,7 +219,7 @@ async def on_GET( class PurgeMediaCacheRestServlet(RestServlet): - PATTERNS = admin_patterns("/purge_media_cache") + PATTERNS = admin_patterns("/purge_media_cache$") def __init__(self, hs: "HomeServer"): self.media_repository = hs.get_media_repository() @@ -285,7 +285,7 @@ class DeleteMediaByDateSize(RestServlet): timestamp and size. """ - PATTERNS = admin_patterns("/media/(?P[^/]+)/delete") + PATTERNS = admin_patterns("/media/(?P[^/]+)/delete$") def __init__(self, hs: "HomeServer"): self.store = hs.get_datastore() From 14719065929926f5256b73691f7c5739793f0cca Mon Sep 17 00:00:00 2001 From: Aaron Raimist Date: Sat, 16 Oct 2021 17:31:00 -0500 Subject: [PATCH 3/6] Add changelog Signed-off-by: Aaron Raimist --- changelog.d/11101.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11101.bugfix diff --git a/changelog.d/11101.bugfix b/changelog.d/11101.bugfix new file mode 100644 index 000000000000..2cbe4d693d16 --- /dev/null +++ b/changelog.d/11101.bugfix @@ -0,0 +1 @@ +Show an error when timestamp in seconds is provided to the purge_media_cache Admin API. \ No newline at end of file From 082fe45f065e0e50c83a2a10db4c84776f718313 Mon Sep 17 00:00:00 2001 From: Aaron Raimist Date: Sun, 17 Oct 2021 03:42:31 -0500 Subject: [PATCH 4/6] Fix clock so tests pass Signed-off-by: Aaron Raimist --- tests/rest/admin/test_media.py | 24 +++++++++++------------- tests/utils.py | 3 --- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index a05bf4d2918f..819527f5f906 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -26,7 +26,6 @@ from tests import unittest from tests.server import FakeSite, make_request from tests.test_utils import SMALL_PNG -from tests.utils import MockClock VALID_TIMESTAMP = 1609459200000 # 2021-01-01 in milliseconds INVALID_TIMESTAMP_IN_S = 1893456000 # 2030-01-01 in seconds @@ -207,7 +206,8 @@ def prepare(self, reactor, clock, hs): self.filepaths = MediaFilePaths(hs.config.media.media_store_path) self.url = "/_synapse/admin/v1/media/%s/delete" % self.server_name - self.clock = MockClock() + # Move clock up to somewhat realistic time + self.reactor.advance(1000000000) def test_no_auth(self): """ @@ -339,7 +339,7 @@ def test_delete_media_never_accessed(self): self.assertTrue(os.path.exists(local_path)) # timestamp after upload/create - now_ms = self.clock.real_time_msec() + now_ms = self.clock.time_msec() channel = self.make_request( "POST", self.url + "?before_ts=" + str(now_ms), @@ -360,7 +360,7 @@ def test_keep_media_by_date(self): """ # timestamp before upload - now_ms = self.clock.real_time_msec() + now_ms = self.clock.time_msec() server_and_media_id = self._create_media() self._access_media(server_and_media_id) @@ -376,7 +376,7 @@ def test_keep_media_by_date(self): self._access_media(server_and_media_id) # timestamp after upload - now_ms = self.clock.real_time_msec() + now_ms = self.clock.time_msec() channel = self.make_request( "POST", self.url + "?before_ts=" + str(now_ms), @@ -400,7 +400,7 @@ def test_keep_media_by_size(self): self._access_media(server_and_media_id) - now_ms = self.clock.real_time_msec() + now_ms = self.clock.time_msec() channel = self.make_request( "POST", self.url + "?before_ts=" + str(now_ms) + "&size_gt=67", @@ -411,7 +411,7 @@ def test_keep_media_by_size(self): self._access_media(server_and_media_id) - now_ms = self.clock.real_time_msec() + now_ms = self.clock.time_msec() channel = self.make_request( "POST", self.url + "?before_ts=" + str(now_ms) + "&size_gt=66", @@ -444,7 +444,7 @@ def test_keep_media_by_user_avatar(self): ) self.assertEqual(200, channel.code, msg=channel.json_body) - now_ms = self.clock.real_time_msec() + now_ms = self.clock.time_msec() channel = self.make_request( "POST", self.url + "?before_ts=" + str(now_ms) + "&keep_profiles=true", @@ -455,7 +455,7 @@ def test_keep_media_by_user_avatar(self): self._access_media(server_and_media_id) - now_ms = self.clock.real_time_msec() + now_ms = self.clock.time_msec() channel = self.make_request( "POST", self.url + "?before_ts=" + str(now_ms) + "&keep_profiles=false", @@ -489,7 +489,7 @@ def test_keep_media_by_room_avatar(self): ) self.assertEqual(200, channel.code, msg=channel.json_body) - now_ms = self.clock.real_time_msec() + now_ms = self.clock.time_msec() channel = self.make_request( "POST", self.url + "?before_ts=" + str(now_ms) + "&keep_profiles=true", @@ -500,7 +500,7 @@ def test_keep_media_by_room_avatar(self): self._access_media(server_and_media_id) - now_ms = self.clock.real_time_msec() + now_ms = self.clock.time_msec() channel = self.make_request( "POST", self.url + "?before_ts=" + str(now_ms) + "&keep_profiles=false", @@ -809,8 +809,6 @@ def prepare(self, reactor, clock, hs): self.filepaths = MediaFilePaths(hs.config.media.media_store_path) self.url = "/_synapse/admin/v1/purge_media_cache" - self.clock = MockClock() - def test_no_auth(self): """ Try to delete media without authentication. diff --git a/tests/utils.py b/tests/utils.py index ec505395b3bf..cf8ba5c5db35 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -477,9 +477,6 @@ def time(self): def time_msec(self): return self.time() * 1000 - def real_time_msec(self): - return int(time.time() * 1000) - def call_later(self, delay, callback, *args, **kwargs): ctx = current_context() From f04ccc6be54fe4e8b5f74649cf32e9eda2678066 Mon Sep 17 00:00:00 2001 From: Aaron Raimist Date: Tue, 19 Oct 2021 15:49:50 -0500 Subject: [PATCH 5/6] Change phrasing for positive integer error Signed-off-by: Aaron Raimist --- synapse/rest/admin/media.py | 4 ++-- tests/rest/admin/test_media.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index b7afbf167f1b..30a687d234e3 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -234,7 +234,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: if before_ts < 0: raise SynapseError( 400, - "Query parameter before_ts must be a string representing a positive integer.", + "Query parameter before_ts must be a positive integer.", errcode=Codes.INVALID_PARAM, ) elif before_ts < 30000000000: # Dec 1970 in milliseconds, Aug 2920 in seconds @@ -305,7 +305,7 @@ async def on_POST( if before_ts < 0: raise SynapseError( 400, - "Query parameter before_ts must be a string representing a positive integer.", + "Query parameter before_ts must be a positive integer.", errcode=Codes.INVALID_PARAM, ) elif before_ts < 30000000000: # Dec 1970 in milliseconds, Aug 2920 in seconds diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index 819527f5f906..db0e78c03995 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -279,7 +279,7 @@ def test_invalid_parameter(self): self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) self.assertEqual( - "Query parameter before_ts must be a string representing a positive integer.", + "Query parameter before_ts must be a positive integer.", channel.json_body["error"], ) @@ -848,7 +848,7 @@ def test_invalid_parameter(self): self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) self.assertEqual( - "Query parameter before_ts must be a string representing a positive integer.", + "Query parameter before_ts must be a positive integer.", channel.json_body["error"], ) From 4f18cd84cf72b3a2f09c048c1f8a542971fd36a3 Mon Sep 17 00:00:00 2001 From: reivilibre Date: Wed, 20 Oct 2021 15:15:52 +0100 Subject: [PATCH 6/6] Update changelog.d/11101.bugfix --- changelog.d/11101.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/11101.bugfix b/changelog.d/11101.bugfix index 2cbe4d693d16..0de507848fd7 100644 --- a/changelog.d/11101.bugfix +++ b/changelog.d/11101.bugfix @@ -1 +1 @@ -Show an error when timestamp in seconds is provided to the purge_media_cache Admin API. \ No newline at end of file +Show an error when timestamp in seconds is provided to the `/purge_media_cache` Admin API. \ No newline at end of file