From d105029780cbf638afbe9cab79dfa17148889709 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Fri, 21 May 2021 22:56:41 +0200 Subject: [PATCH 1/6] Add an admin API for unprotecting local media from quarantine --- docs/admin_api/media_admin_api.md | 20 ++++ synapse/rest/admin/media.py | 17 +++- .../databases/main/media_repository.py | 7 +- tests/rest/admin/test_media.py | 98 +++++++++++++++++++ 4 files changed, 137 insertions(+), 5 deletions(-) diff --git a/docs/admin_api/media_admin_api.md b/docs/admin_api/media_admin_api.md index 9dbec68c19e6..7cd0b640d7a5 100644 --- a/docs/admin_api/media_admin_api.md +++ b/docs/admin_api/media_admin_api.md @@ -159,6 +159,26 @@ Response: {} ``` +## Unprotecting media from being quarantined + +This API reverts the protection of a media. + +Request: + +``` +DELETE /_synapse/admin/v1/media/protect/ + +{} +``` + +Where `media_id` is in the form of `abcdefg12345...`. + +Response: + +```json +{} +``` + # Delete local media This API deletes the *local* media from the disk of your own server. This includes any local thumbnails and copies of media downloaded from diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index 24dd46113a29..b83e407a73f5 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -137,8 +137,21 @@ async def on_POST( logging.info("Protecting local media by ID: %s", media_id) - # Quarantine this media id - await self.store.mark_local_media_as_safe(media_id) + # Protect this media id + await self.store.mark_local_media_as_safe(media_id, safe=True) + + return 200, {} + + async def on_DELETE( + self, request: SynapseRequest, media_id: str + ) -> Tuple[int, JsonDict]: + requester = await self.auth.get_user_by_req(request) + await assert_user_is_admin(self.auth, requester.user) + + logging.info("Unprotecting local media by ID: %s", media_id) + + # Unprotect this media id + await self.store.mark_local_media_as_safe(media_id, safe=False) return 200, {} diff --git a/synapse/storage/databases/main/media_repository.py b/synapse/storage/databases/main/media_repository.py index c584868188e9..2fa945d171f0 100644 --- a/synapse/storage/databases/main/media_repository.py +++ b/synapse/storage/databases/main/media_repository.py @@ -143,6 +143,7 @@ async def get_local_media(self, media_id: str) -> Optional[Dict[str, Any]]: "created_ts", "quarantined_by", "url_cache", + "safe_from_quarantine", ), allow_none=True, desc="get_local_media", @@ -296,12 +297,12 @@ async def store_local_media( desc="store_local_media", ) - async def mark_local_media_as_safe(self, media_id: str) -> None: - """Mark a local media as safe from quarantining.""" + async def mark_local_media_as_safe(self, media_id: str, safe: bool = True) -> None: + """Mark a local media as safe or unsafe from quarantining.""" await self.db_pool.simple_update_one( table="local_media_repository", keyvalues={"media_id": media_id}, - updatevalues={"safe_from_quarantine": True}, + updatevalues={"safe_from_quarantine": safe}, desc="mark_local_media_as_safe", ) diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index ac7b21970033..900b5061a000 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -16,6 +16,8 @@ import os from binascii import unhexlify +from parameterized import parameterized_class + import synapse.rest.admin from synapse.api.errors import Codes from synapse.rest.client.v1 import login, profile, room @@ -562,3 +564,99 @@ def _access_media(self, server_and_media_id, expect_success=True): ) # Test that the file is deleted self.assertFalse(os.path.exists(local_path)) + + +@parameterized_class( + ("method",), + [ + ("POST",), + ("DELETE",), + ], +) +class ProtectMediaByIDTestCase(unittest.HomeserverTestCase): + + servlets = [ + synapse.rest.admin.register_servlets, + synapse.rest.admin.register_servlets_for_media_repo, + login.register_servlets, + ] + + def prepare(self, reactor, clock, hs): + media_repo = hs.get_media_repository_resource() + self.store = hs.get_datastore() + + self.admin_user = self.register_user("admin", "pass", admin=True) + self.admin_user_tok = self.login("admin", "pass") + + # Create media + upload_resource = media_repo.children[b"upload"] + # file size is 67 Byte + image_data = unhexlify( + b"89504e470d0a1a0a0000000d4948445200000001000000010806" + b"0000001f15c4890000000a49444154789c63000100000500010d" + b"0a2db40000000049454e44ae426082" + ) + + # Upload some media into the room + response = self.helper.upload_media( + upload_resource, image_data, tok=self.admin_user_tok, expect_code=200 + ) + # Extract media ID from the response + server_and_media_id = response["content_uri"][6:] # Cut off 'mxc://' + self.media_id = server_and_media_id.split("/")[1] + + self.url = "/_synapse/admin/v1/media/protect/%s" % self.media_id + + def test_no_auth(self): + """ + Try to protect media without authentication. + """ + + channel = self.make_request(self.method, 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_no_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( + self.method, + 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_protect_media(self): + """ + Tests that protect and unprotect a media is successfully + """ + + media_info = self.get_success(self.store.get_local_media(self.media_id)) + self.assertFalse(media_info["safe_from_quarantine"]) + + # protect + channel = self.make_request("POST", self.url, access_token=self.admin_user_tok) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertFalse(channel.json_body) + + media_info = self.get_success(self.store.get_local_media(self.media_id)) + self.assertTrue(media_info["safe_from_quarantine"]) + + # unprotect + channel = self.make_request( + "DELETE", self.url, access_token=self.admin_user_tok + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + self.assertFalse(channel.json_body) + + media_info = self.get_success(self.store.get_local_media(self.media_id)) + self.assertFalse(media_info["safe_from_quarantine"]) From b54d5e79da87e94a528b314eb5b4101d8c762e35 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Fri, 21 May 2021 22:59:51 +0200 Subject: [PATCH 2/6] add changelog --- changelog.d/10040.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10040.feature diff --git a/changelog.d/10040.feature b/changelog.d/10040.feature new file mode 100644 index 000000000000..f992ba0cb9f2 --- /dev/null +++ b/changelog.d/10040.feature @@ -0,0 +1 @@ +Add an admin API for unprotecting local media from quarantine. \ No newline at end of file From 58f63a4b7790be54c8c9ed5975bbf7c953adff95 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Fri, 21 May 2021 23:10:57 +0200 Subject: [PATCH 3/6] update content of doc --- docs/admin_api/media_admin_api.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/admin_api/media_admin_api.md b/docs/admin_api/media_admin_api.md index 7cd0b640d7a5..b4df542c6452 100644 --- a/docs/admin_api/media_admin_api.md +++ b/docs/admin_api/media_admin_api.md @@ -7,6 +7,7 @@ * [Quarantining media in a room](#quarantining-media-in-a-room) * [Quarantining all media of a user](#quarantining-all-media-of-a-user) * [Protecting media from being quarantined](#protecting-media-from-being-quarantined) + * [Unprotecting media from being quarantined](#unprotecting-media-from-being-quarantined) - [Delete local media](#delete-local-media) * [Delete a specific local media](#delete-a-specific-local-media) * [Delete local media by date or size](#delete-local-media-by-date-or-size) From 97c35649805953223a5598367495e0b1108a421b Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Sat, 22 May 2021 16:07:58 +0200 Subject: [PATCH 4/6] improve parameterized --- tests/rest/admin/test_media.py | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index 900b5061a000..65ef9e6bd8dc 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -16,7 +16,7 @@ import os from binascii import unhexlify -from parameterized import parameterized_class +from parameterized import parameterized import synapse.rest.admin from synapse.api.errors import Codes @@ -566,13 +566,6 @@ def _access_media(self, server_and_media_id, expect_success=True): self.assertFalse(os.path.exists(local_path)) -@parameterized_class( - ("method",), - [ - ("POST",), - ("DELETE",), - ], -) class ProtectMediaByIDTestCase(unittest.HomeserverTestCase): servlets = [ @@ -607,17 +600,19 @@ def prepare(self, reactor, clock, hs): self.url = "/_synapse/admin/v1/media/protect/%s" % self.media_id - def test_no_auth(self): + @parameterized.expand(["POST", "DELETE"]) + def test_no_auth(self, method: str): """ Try to protect media without authentication. """ - channel = self.make_request(self.method, self.url, b"{}") + channel = self.make_request(method, 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_no_admin(self): + @parameterized.expand(["POST", "DELETE"]) + def test_requester_is_no_admin(self, method: str): """ If the user is not a server admin, an error is returned. """ @@ -625,7 +620,7 @@ def test_requester_is_no_admin(self): self.other_user_token = self.login("user", "pass") channel = self.make_request( - self.method, + method, self.url, access_token=self.other_user_token, ) From 37205b7e69245efa3192a0c03564238f0a519b4f Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Tue, 25 May 2021 21:46:37 +0200 Subject: [PATCH 5/6] change API to `POST` --- docs/admin_api/media_admin_api.md | 2 +- synapse/rest/admin/media.py | 13 ++++++++++++- tests/rest/admin/test_media.py | 26 ++++++++++++++++---------- 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/docs/admin_api/media_admin_api.md b/docs/admin_api/media_admin_api.md index b4df542c6452..d1b7e390d5a9 100644 --- a/docs/admin_api/media_admin_api.md +++ b/docs/admin_api/media_admin_api.md @@ -167,7 +167,7 @@ This API reverts the protection of a media. Request: ``` -DELETE /_synapse/admin/v1/media/protect/ +POST /_synapse/admin/v1/media/unprotect/ {} ``` diff --git a/synapse/rest/admin/media.py b/synapse/rest/admin/media.py index b83e407a73f5..2c71af4279bf 100644 --- a/synapse/rest/admin/media.py +++ b/synapse/rest/admin/media.py @@ -142,7 +142,17 @@ async def on_POST( return 200, {} - async def on_DELETE( + +class UnprotectMediaByID(RestServlet): + """Unprotect local media from being quarantined.""" + + PATTERNS = admin_patterns("/media/unprotect/(?P[^/]+)") + + def __init__(self, hs: "HomeServer"): + self.store = hs.get_datastore() + self.auth = hs.get_auth() + + async def on_POST( self, request: SynapseRequest, media_id: str ) -> Tuple[int, JsonDict]: requester = await self.auth.get_user_by_req(request) @@ -282,6 +292,7 @@ def register_servlets_for_media_repo(hs: "HomeServer", http_server): QuarantineMediaByID(hs).register(http_server) QuarantineMediaByUser(hs).register(http_server) ProtectMediaByID(hs).register(http_server) + UnprotectMediaByID(hs).register(http_server) ListMediaInRoom(hs).register(http_server) DeleteMediaByID(hs).register(http_server) DeleteMediaByDateSize(hs).register(http_server) diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index 65ef9e6bd8dc..f741121ea236 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -598,21 +598,21 @@ def prepare(self, reactor, clock, hs): server_and_media_id = response["content_uri"][6:] # Cut off 'mxc://' self.media_id = server_and_media_id.split("/")[1] - self.url = "/_synapse/admin/v1/media/protect/%s" % self.media_id + self.url = "/_synapse/admin/v1/media/%s/%s" - @parameterized.expand(["POST", "DELETE"]) - def test_no_auth(self, method: str): + @parameterized.expand(["protect", "unprotect"]) + def test_no_auth(self, action: str): """ Try to protect media without authentication. """ - channel = self.make_request(method, self.url, b"{}") + channel = self.make_request("POST", self.url % (action, self.media_id), b"{}") self.assertEqual(401, int(channel.result["code"]), msg=channel.result["body"]) self.assertEqual(Codes.MISSING_TOKEN, channel.json_body["errcode"]) - @parameterized.expand(["POST", "DELETE"]) - def test_requester_is_no_admin(self, method: str): + @parameterized.expand(["protect", "unprotect"]) + def test_requester_is_no_admin(self, action: str): """ If the user is not a server admin, an error is returned. """ @@ -620,8 +620,8 @@ def test_requester_is_no_admin(self, method: str): self.other_user_token = self.login("user", "pass") channel = self.make_request( - method, - self.url, + "POST", + self.url % (action, self.media_id), access_token=self.other_user_token, ) @@ -637,7 +637,11 @@ def test_protect_media(self): self.assertFalse(media_info["safe_from_quarantine"]) # protect - channel = self.make_request("POST", self.url, access_token=self.admin_user_tok) + channel = self.make_request( + "POST", + self.url % ("protect", self.media_id), + access_token=self.admin_user_tok, + ) self.assertEqual(200, channel.code, msg=channel.json_body) self.assertFalse(channel.json_body) @@ -647,7 +651,9 @@ def test_protect_media(self): # unprotect channel = self.make_request( - "DELETE", self.url, access_token=self.admin_user_tok + "POST", + self.url % ("unprotect", self.media_id), + access_token=self.admin_user_tok, ) self.assertEqual(200, channel.code, msg=channel.json_body) From 4621e4971d72cf080bd5d1869d602a9cd7854754 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Wed, 26 May 2021 10:52:24 +0100 Subject: [PATCH 6/6] Update changelog.d/10040.feature --- changelog.d/10040.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/10040.feature b/changelog.d/10040.feature index f992ba0cb9f2..ec78a30f0074 100644 --- a/changelog.d/10040.feature +++ b/changelog.d/10040.feature @@ -1 +1 @@ -Add an admin API for unprotecting local media from quarantine. \ No newline at end of file +Add an admin API for unprotecting local media from quarantine. Contributed by @dklimpel.