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

Quarantine media by ID or user ID #6681

Merged
merged 15 commits into from
Jan 13, 2020
Merged

Conversation

anoadragon453
Copy link
Member

Fixes #5956

This PR adds some admin APIs to:

  • Quarantine media by ID
  • Quarantine all local media that a local user has uploaded

Most of the code here is test-related.

@anoadragon453 anoadragon453 requested a review from a team January 10, 2020 17:42
@anoadragon453 anoadragon453 self-assigned this Jan 10, 2020
Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, mostly just a bad merge conflict has messed things up a bit I think.

synapse/storage/data_stores/main/room.py Outdated Show resolved Hide resolved
non_admin_user_tok = self.login("nonadmin", "pass")

# Attempt quarantine media APIs as non-admin
url = "/_synapse/admin/v1/quarantine_media/id/example.org/abcde12345"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
url = "/_synapse/admin/v1/quarantine_media/id/example.org/abcde12345"
url = "/_synapse/admin/v1/quarantine_media/example.org/abcde12345"

Copy link
Member Author

@anoadragon453 anoadragon453 Jan 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the endpoints to:

  • /_synapse/admin/v1/quarantine_media/by_id/example.org/abcde12345
  • /_synapse/admin/v1/quarantine_media/by_user/@bob:example.com
  • /_synapse/admin/v1/quarantine_media/by_room/!someroom:example.com (with .../quarantine_media/!room... as a legacy fallback)

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another iteration:

  • /_synapse/admin/v1/quarantine_media_by_id/example.org/abcde12345
  • /_synapse/admin/v1/quarantine_media_by_user/@bob:example.com
  • /_synapse/admin/v1/quarantine_media_by_room/!someroom:example.com (with .../quarantine_media/!room... as a legacy fallback)

synapse/storage/data_stores/main/room.py Outdated Show resolved Hide resolved
Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will want to point these at the media repo worker, so let's update docs/workers.rst. We might also want to run the tests against the media repo worker rather than the main homeserver. I think there is an example of it somewhere.

Mainly though, looks like we still have a dupe function

synapse/storage/data_stores/main/room.py Outdated Show resolved Hide resolved
@anoadragon453
Copy link
Member Author

Erik and I tried to point these tests at the media worker, however it doesn't have servlets for creating or joining a room (which the test for quarantining media in a room needs), so we need some way of testing against multiple workers.

However, we're not going to block this PR on that.

@anoadragon453 anoadragon453 merged commit 1177d3f into develop Jan 13, 2020
@anoadragon453 anoadragon453 deleted the anoa/quarantine_media_ids branch January 13, 2020 18:10
historical_admin_path_patterns("/room/(?P<room_id>[^/]+)/media/quarantine")
+
# This path kept around for legacy reasons
historical_admin_path_patterns("/quarantine_media/(?P<room_id>![^/]+)")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra ! snuck in here 🤦‍♂️

anoadragon453 pushed a commit that referenced this pull request Jun 2, 2021
Related to: #6681, #5956, #10040

Signed-off-by: Dirk Klimpel dirk@klimpel.org
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit '1177d3f3a':
  Quarantine media by ID or user ID (#6681)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants