fix broken avatar checks when server_name contains a port

[ashfame]

This PR fixes check_avatar_size_and_mime_type() to successfully update avatars on homeservers running on non-default ports which it would mistakenly treat as remote homeserver while validating the avatar's size and mime type.

Essentially it was incorrectly using host as server_name inside of check_avatar_size_and_mime_type() without minding the port its actually running on.

Eg: If running synapse on synapse.dev:8008 this function would infer the current homeserver url is synapse.dev and treat synapse.dev:8008 as remote server while trying to fetch media's metadata to check for size and mime type. As a result, it fails to find the image itself & fails avatar validation & hence setting a new avatar.

Happy to address any feedback given so that we can land this change.

Signed-off-by: Ashish Kumar ashfame@users.noreply.github.com

[clokep]

This looks reasonable, but seems like a thing that we would possibly break in the future again. Do you think a test-case could be added to ProfileTestCase for this?

[ashfame]

@clokep Thanks for taking a look! I am not sure how I can write the test for check_avatar_size_and_mime_type() since that requires an actual valid mxc url since we pull out metadata by the mxc url and then we need two homeserver instances, one of which runs on non-default port.

In existing tests, I only saw dummy mxc urls like mxc://test/good and mxc://test/small

I am pretty new to both Python & Synapse, so I may be missing something obvious/established.

[ashfame]

@richvdh Thanks for the feedback in this PR! I have addressed all the points and its ready for another review. ✌️

Regarding the failing checks, I am not sure what the problem is since the exact failing test pass when I run them locally:

--- PASS: TestMembersLocal (8.83s)
    --- PASS: TestMembersLocal/Parallel (0.00s)
        --- PASS: TestMembersLocal/Parallel/New_room_members_see_their_own_join_event (0.05s)
        --- PASS: TestMembersLocal/Parallel/Existing_members_see_new_members'_presence (0.05s)
        --- PASS: TestMembersLocal/Parallel/Existing_members_see_new_members'_join_events (0.06s)
PASS

[DMRobertson]

@richvdh Thanks for the feedback in this PR! I have addressed all the points and its ready for another review. v

Regarding the failing checks, I am not sure what the problem is since the exact failing test pass when I run them locally:

--- PASS: TestMembersLocal (8.83s)
    --- PASS: TestMembersLocal/Parallel (0.00s)
        --- PASS: TestMembersLocal/Parallel/New_room_members_see_their_own_join_event (0.05s)
        --- PASS: TestMembersLocal/Parallel/Existing_members_see_new_members'_presence (0.05s)
        --- PASS: TestMembersLocal/Parallel/Existing_members_see_new_members'_join_events (0.06s)
PASS

The tests are probably just flakey, see #13199

[ashfame]

@richvdh Is this ready to be merged now? Tests are failing because Connection to files.pythonhosted.org timed out in Complement tests.

[ashfame]

@richvdh Allow me to re-phrase the issue and the unit test:

check_avatar_size_and_mime_type() had a logical bug of incorrectly using parse_and_validate_mxc_uri() to figure out the server_name. Now the only test against that bug would be to check for the impact caused by the value of server_name, which is either get_local_media() gets called or get_cached_remote_media() gets called.

This is what the unit test tries to test, whether the expected method to retrieve metadata gets called or not, as a way to check whether we have the right server_name figured out or not. And since we pass it both: a local file first and then a remote file, we don't really need to test with an overriden config of server_name and the test works for both default and non-default ports of server_name in this manner.

Hope that explains my intention with this PR. I re-worked the test to make it more elaborative and much more descriptive.

Sorry for the back-and-forth on this issue and I am happy with any changes you want to make on top of it.

[ashfame]

@richvdh I have simplified it further & it now specifies a port in server_name for the test. Hope this is easy to make sense of now :)

Ideally, I would like the same unit test to run for both cases, one with default server_name value and one with overridden value. But I am not sure what would be the best way to go about it.

[ashfame]

@richvdh Do you know when you will be able to take another look at this? I have simplified the test with lot more comments since your last review, as I put it in my 2 comments above.

[richvdh]

@ashfame as it happens I was just looking at it. I've cleaned up the unit tests a bit more; if you are happy and the CI passes I will merge it.

[ashfame]

@richvdh No problem! Sounds good 👍

[richvdh]

Thanks for your contribution!