Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prepare for authenticated media freeze #17433

Merged
merged 8 commits into from
Jul 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
10 changes: 10 additions & 0 deletions docs/usage/configuration/config_documentation.md
Original file line number Diff line number Diff line change
Expand Up @@ -1863,6 +1863,16 @@ federation_rr_transactions_per_room_per_second: 40
## Media Store
Config options related to Synapse's media store.

---
Copy link
Contributor

Choose a reason for hiding this comment

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

(on the fence; do you think we ought to add an upgrade notes entry about this change, so people can get ahead of the curve and set the option to false explicitly if they want to make sure their special client won't be bitten by the upcoming freeze? It'd also be a good place to just lay out the plan a bit, since I think for most who don't read TWIM etc, the upcoming changes may not be obvious?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to get @turt2live's opinion on this - perhaps it would make sense to link to the blog post announcing the freeze? I think for now though we can merge this (I'd like to get it into the next rc so people can test against it on beta.matrix.org) and figure out how to handle the upgrade notes, etc soon.

Copy link
Member

@turt2live turt2live Jul 23, 2024

Choose a reason for hiding this comment

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

Server operators are going to be increasingly incentivized to ensure this option is actually set to true rather than false, regardless of client breakage. When we change the default to true we should definitely call out matrix.org's plan as reference, but I wouldn't include language about setting it to false.

Copy link
Member

Choose a reason for hiding this comment

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

From brief discussion in backend internal: we should provide users with instructions on how to set it to false, but we can have the changelog read as eventually removing the option (making the feature inevitable)

### `enable_authenticated_media`
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this would be clearer as new_media_requires_authentication or something like that? It seems like once the media gets the flag, it stays in that state forever, so the current name doesn't give quite the right impression.

Thanks for writing this up though, this is way clearer about what we're hoping this feature will do, it's worth it just for that alone.

Copy link
Contributor

Choose a reason for hiding this comment

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

mrh, actually, it looks like the two options were merged and this option is named properly, but it'd probably be worth pointing out that disabling this option means all media, including those flagged as 'authenticated media', will be available over the unauthenticated endpoint? Since we disable the auth checks when this is turned off.


When set to true, all subsequent media uploads will be marked as authenticated, and will not be available over legacy
unauthenticated media endpoints (`/_matrix/media/(r0|v3|v1)/download` and `/_matrix/media/(r0|v3|v1)/thumbnail`) - requests for authenticated media over these endpoints will result in a 404. All media, including authenticated media, will be available over the authenticated media endpoints `_matrix/client/v1/media/download` and `_matrix/client/v1/media/thumbnail`. Media uploaded prior to setting this option to true will still be available over the legacy endpoints.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Defaults to false, but this will change to true in a future Synapse release.

Example configuration:
```yaml
enable_authenticated_media
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
enable_authenticated_media
enable_authenticated_media: true

```
---
### `enable_media_repo`

Expand Down
5 changes: 3 additions & 2 deletions synapse/_scripts/synapse_port_db.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,18 +119,19 @@
"e2e_room_keys": ["is_verified"],
"event_edges": ["is_state"],
"events": ["processed", "outlier", "contains_url"],
"local_media_repository": ["safe_from_quarantine"],
"local_media_repository": ["safe_from_quarantine", "authenticated"],
"per_user_experimental_features": ["enabled"],
"presence_list": ["accepted"],
"presence_stream": ["currently_active"],
"public_room_list_stream": ["visibility"],
"pushers": ["enabled"],
"redactions": ["have_censored"],
"remote_media_cache": ["authenticated"],
"room_stats_state": ["is_federatable"],
"rooms": ["is_public", "has_auth_chain_index"],
"users": ["shadow_banned", "approved", "locked", "suspended"],
"un_partial_stated_event_stream": ["rejection_status_changed"],
"users_who_share_rooms": ["share_private"],
"per_user_experimental_features": ["enabled"],
}


Expand Down
5 changes: 2 additions & 3 deletions synapse/config/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -272,9 +272,8 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
remote_media_lifetime
)

self.authenticate_new_media = config.get("authenticate_new_media", False)
self.enforce_authenticated_media = config.get(
"enforce_authenticated_media", False
self.enable_authenticated_media = config.get(
"enable_authenticated_media", False
)

def generate_config_section(self, data_dir_path: str, **kwargs: Any) -> str:
Expand Down
18 changes: 7 additions & 11 deletions synapse/media/media_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -453,9 +453,9 @@ async def get_local_media(
if not media_info:
return

if self.hs.config.media.enforce_authenticated_media and not allow_authenticated:
if self.hs.config.media.enable_authenticated_media and not allow_authenticated:
if media_info.authenticated:
raise NotFoundError
raise NotFoundError()

self.mark_recently_accessed(None, media_id)

Expand Down Expand Up @@ -634,14 +634,10 @@ async def _get_remote_media_impl(
"""
media_info = await self.store.get_cached_remote_media(server_name, media_id)

if self.hs.config.media.enforce_authenticated_media and not allow_authenticated:
# if it isn't cached then don't fetch it
if not media_info:
if self.hs.config.media.enable_authenticated_media and not allow_authenticated:
# if it isn't cached then don't fetch it or if it's authenticated then don't serve it
if not media_info or media_info.authenticated:
raise NotFoundError()
# and if it's authenticated don't serve it
else:
if media_info.authenticated:
raise NotFoundError()

# file_id is the ID we use to track the file locally. If we've already
# seen the file then reuse the existing ID, otherwise generate a new
Expand Down Expand Up @@ -816,7 +812,7 @@ async def _download_remote_file(

logger.info("Stored remote media in file %r", fname)

if self.hs.config.media.authenticate_new_media:
if self.hs.config.media.enable_authenticated_media:
authenticated = True
else:
authenticated = False
Expand Down Expand Up @@ -945,7 +941,7 @@ async def _federation_download_remote_file(

logger.debug("Stored remote media in file %r", fname)

if self.hs.config.media.authenticate_new_media:
if self.hs.config.media.enable_authenticated_media:
authenticated = True
else:
authenticated = False
Expand Down
14 changes: 7 additions & 7 deletions synapse/media/thumbnailer.py
Original file line number Diff line number Diff line change
Expand Up @@ -284,9 +284,9 @@ async def respond_local_thumbnail(

# if the media the thumbnail is generated from is authenticated, don't serve the
# thumbnail over an unauthenticated endpoint
if self.hs.config.media.enforce_authenticated_media and not allow_authenticated:
if self.hs.config.media.enable_authenticated_media and not allow_authenticated:
if media_info.authenticated:
raise NotFoundError
raise NotFoundError()

thumbnail_infos = await self.store.get_local_media_thumbnails(media_id)
await self._select_and_respond_with_thumbnail(
Expand Down Expand Up @@ -324,9 +324,9 @@ async def select_or_generate_local_thumbnail(

# if the media the thumbnail is generated from is authenticated, don't serve the
# thumbnail over an unauthenticated endpoint
if self.hs.config.media.enforce_authenticated_media and not allow_authenticated:
if self.hs.config.media.enable_authenticated_media and not allow_authenticated:
if media_info.authenticated:
raise NotFoundError
raise NotFoundError()

thumbnail_infos = await self.store.get_local_media_thumbnails(media_id)
for info in thumbnail_infos:
Expand Down Expand Up @@ -410,7 +410,7 @@ async def select_or_generate_remote_thumbnail(

# if the media the thumbnail is generated from is authenticated, don't serve the
# thumbnail over an unauthenticated endpoint
if self.hs.config.media.enforce_authenticated_media and not allow_authenticated:
if self.hs.config.media.enable_authenticated_media and not allow_authenticated:
if media_info.authenticated:
respond_404(request)
return
Expand Down Expand Up @@ -490,9 +490,9 @@ async def respond_remote_thumbnail(

# if the media the thumbnail is generated from is authenticated, don't serve the
# thumbnail over an unauthenticated endpoint
if self.hs.config.media.enforce_authenticated_media and not allow_authenticated:
if self.hs.config.media.enable_authenticated_media and not allow_authenticated:
if media_info.authenticated:
raise NotFoundError
raise NotFoundError()

thumbnail_infos = await self.store.get_remote_media_thumbnails(
server_name, media_id
Expand Down
6 changes: 3 additions & 3 deletions synapse/storage/databases/main/media_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ async def store_local_media_id(
time_now_ms: int,
user_id: UserID,
) -> None:
if self.hs.config.media.authenticate_new_media:
if self.hs.config.media.enable_authenticated_media:
authenticated = True
else:
authenticated = False
Expand All @@ -450,7 +450,7 @@ async def store_local_media(
user_id: UserID,
url_cache: Optional[str] = None,
) -> None:
if self.hs.config.media.authenticate_new_media:
if self.hs.config.media.enable_authenticated_media:
authenticated = True
else:
authenticated = False
Expand Down Expand Up @@ -686,7 +686,7 @@ async def store_cached_remote_media(
upload_name: Optional[str],
filesystem_id: str,
) -> None:
if self.hs.config.media.authenticate_new_media:
if self.hs.config.media.enable_authenticated_media:
authenticated = True
else:
authenticated = False
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,5 @@
-- See the GNU Affero General Public License for more details:
-- <https://www.gnu.org/licenses/agpl-3.0.html>.


ALTER TABLE remote_media_cache ADD COLUMN authenticated BOOLEAN;
ALTER TABLE local_media_repository ADD COLUMN authenticated BOOLEAN;
ALTER TABLE remote_media_cache ADD COLUMN authenticated BOOLEAN DEFAULT FALSE NOT NULL;
H-Shay marked this conversation as resolved.
Show resolved Hide resolved
ALTER TABLE local_media_repository ADD COLUMN authenticated BOOLEAN DEFAULT FALSE NOT NULL;
46 changes: 44 additions & 2 deletions tests/rest/client/test_media.py
Original file line number Diff line number Diff line change
Expand Up @@ -2453,8 +2453,7 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer:
os.mkdir(self.storage_path)
os.mkdir(self.media_store_path)
config["media_store_path"] = self.media_store_path
config["authenticate_new_media"] = (True,)
config["enforce_authenticated_media"] = True
config["enable_authenticated_media"] = True

provider_config = {
"module": "synapse.media.storage_provider.FileStorageProviderBackend",
Expand Down Expand Up @@ -2593,3 +2592,46 @@ def test_authenticated_media(self) -> None:
access_token=self.tok,
)
self.assertEqual(channel9.code, 404)

# Inject a piece of local media that isn't authenticated
file_id = "abcdefg123456"
file_info = FileInfo(None, file_id=file_id)

ctx = media_storage.store_into_file(file_info)
(f, fname) = self.get_success(ctx.__aenter__())
f.write(SMALL_PNG)
self.get_success(ctx.__aexit__(None, None, None))

self.get_success(
self.store.db_pool.simple_insert(
"local_media_repository",
{
"media_id": "abcdefg123456",
"media_type": "image/png",
"created_ts": self.clock.time_msec(),
"upload_name": "test_local",
"media_length": 1,
"user_id": "someone",
"url_cache": None,
"authenticated": False,
},
desc="store_local_media",
)
)

# check that unauthenticated media is still available over both endpoints
channel9 = self.make_request(
"GET",
"/_matrix/client/v1/media/download/test/abcdefg123456",
shorthand=False,
access_token=self.tok,
)
self.assertEqual(channel9.code, 200)

channel10 = self.make_request(
"GET",
"/_matrix/media/r0/download/test/abcdefg123456",
shorthand=False,
access_token=self.tok,
)
self.assertEqual(channel10.code, 200)
Loading