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

Prevent local quarantined media from being claimed by media retention #12972

Merged
merged 11 commits into from
Jun 7, 2022
Merged

Prevent local quarantined media from being claimed by media retention #12972

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b51107a
Prevent local quarantined media from being claimed by media retention
anoadragon453 Jun 6, 2022
d0d3a69
Protect remote quarantined media from media retention
anoadragon453 Jun 6, 2022
62b17cd
Test that media retention does not remove quarantined media
anoadragon453 Jun 6, 2022
2d837fc
Rename *_before methods, as they do more these days
anoadragon453 Jun 6, 2022
e60beb9
Remove 'local' from log line about potentially remote media
anoadragon453 Jun 6, 2022
e4837e6
Changelog
anoadragon453 Jun 6, 2022
97f0c5d
Additionally prevent protected media from being purged
anoadragon453 Jun 7, 2022
bfba18b
Update media retention tests to include protected media
anoadragon453 Jun 7, 2022
548b333
Merge branch 'develop' of github.com:matrix-org/synapse into anoa/med…
anoadragon453 Jun 7, 2022
66c33bf
Add missing arg to docstring
anoadragon453 Jun 7, 2022
45a8d38
Use false, not 0, in SQL to check booleans
anoadragon453 Jun 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions synapse/rest/media/v1/media_repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -993,6 +993,7 @@ async def delete_old_local_media(
keep_profiles: Switch to delete also files that are still used in image data
(e.g user profile, room avatar). If false these files will be deleted.
delete_quarantined_media: If True, media marked as quarantined will be deleted.
delete_protected_media: If True, media marked as protected will be deleted.

Returns:
A tuple of (list of deleted media IDs, total deleted media IDs).
Expand Down
2 changes: 1 addition & 1 deletion synapse/storage/databases/main/media_repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ async def get_local_media_ids(
if include_protected_media is False:
# Do not include media that has been protected from quarantine
sql += """
AND safe_from_quarantine = 0
AND safe_from_quarantine = false
Copy link
Member

Choose a reason for hiding this comment

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

this will fail on old sqlite

try AND NOT safe_from_quarantine

And read https://matrix-org.github.io/synapse/latest/development/database_schema.html#boolean-columns

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops. Thanks for catching! Fixed in #12977.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a minimum supported sqlite version? Should CI have caught this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Our deprecation policy only mentions Python and PostgreSQL. Our unit tests look to only test different postgres versions as well.

I'm not sure if we do have a minimum supported version of SQLite, but https://matrix-org.github.io/synapse/latest/development/database_schema.html?highlight=database#boolean-columns does mention older versions, and it may be good to test an older SQLite in CI (maybe similar mistakes have slipped in already and we don't even know!).

Copy link
Member

Choose a reason for hiding this comment

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

I thought at least one of our CI runs did old SQLite by using an old version of ubuntu? Though it does look like our CI is using latest ubuntu images everywhere...

Copy link
Member

Choose a reason for hiding this comment

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

let's take this to #12983

"""

def _get_local_media_ids_txn(txn: LoggingTransaction) -> List[str]:
Expand Down