chore: Complement tests for MSC3911 #3

jason-famedly · 2025-09-25T14:27:26Z

MSC3911 Testing for Complement

For: https://github.com/famedly/product-management/issues/3367

Testing(both local and federated):

4 room history visibility variants with image messages restricted
4 room history visibility variants with membership avatar's restricted
room creation membership event is a proper copy of the global profile avatar
media copy api endpoint
Figure out a way to skip these tests if the client versions show it's not available

Adjusted existing test:

TestAvatarUrlUpdate - This test was using a url string for the mxc that was against the spec. This is now fixed

Depends on #4

.github/workflows/ci.yaml

itsoyou · 2025-10-27T11:29:13Z

Do we need to add msc3911 package to packages: ./tests/msc3874 ./tests/msc3902 ./tests/msc4306 in the ci.yaml file?

jason-famedly · 2025-10-27T11:39:44Z

Do we need to add msc3911 package to packages: ./tests/msc3874 ./tests/msc3902 ./tests/msc4306 in the ci.yaml file?

No, I don't think so. Those packages were chosen to represent a bare minimum of support. This particular test run just makes sure we haven't broken anything for the internals of Complement itself if/when we change something.

The actual line up of tests is decided on that repo/platform that runs it in it's own CI(like I added in famedly/synapse#139)

itsoyou · 2025-10-27T13:58:40Z

client/client.go


+// UncheckedDownloadContentAuthenticated makes the raw request for a piece of media and returns the http.Response.
+// Response is unchecked in any way. The existing DownloadContentAuthenticated() should have been a "Must" variant. Rather
+// than refactor that across the code base, this version just uses an explict name


a little typo explict -> explicit

itsoyou · 2025-10-27T14:04:35Z

client/client.go


+// MustSetDisplayName sets the global display name for this account or fails the test.
+func (c *CSAPI) MustSetProfileAvatar(t ct.TestLike, mxcUri string) {
+	c.MustDo(t, "PUT", []string{"_matrix", "client", "v3", "profile", c.UserID, "avatar_url"}, WithJSONBody(t, map[string]any{


would it be better to also add t.Helper() here?

🤔 Probably, nice catch

I think I missed it since the display setter just above did not have it. Looks like the display name "getter" below doesn't either. I wonder just how significant it really it. I think it's just to make the logging simpler in it's display and otherwise has no functional use. Oh well, putting it in anyways

itsoyou · 2025-10-27T14:58:12Z

tests/msc3911/cs_api_restricted_media_test.go

+		// We will reuse that and copy it
+
+		// There seem to be no existing utilities to split an mxc uri into it's components, which is needed for the copy endpoint.
+		// Mxc uri's are formatted as "mxc://server_name/media_id"


there is the func SplitMxc(mxcUri string) (string, string) in client.go, maybe we can reuse it

Nice catch, not sure how I missed that

itsoyou · 2025-10-27T15:00:39Z

tests/msc3911/cs_api_restricted_media_test.go

+		if !bytes.Equal(aliceOriginalProfileBytes, aliceNewAvatarBytes) {
+			t.Fatalf("Media is differing and should be identical")
+		}
+


should we add that the newMxcUri is not equal to the aliceGlobalProfileAvatarMxcUri?

Added comparison test also, good idea.

itsoyou · 2025-10-27T15:35:48Z

Looks good 👍 I didn't expected that the shared visibility is actually act like a world_readable. Would it make difference if the user is banned from a room?

jason-famedly · 2025-10-29T15:34:08Z

Looks good 👍

🎉

I didn't expected that the shared visibility is actually act like a world_readable.

It's not supposed to be. I believe it's actually a Synapse bug, and appears to be part of the "peeking" capability for seeing into a room in advance. I actually have the test scenario written up, thanks for reminding me about that.

Would it make difference if the user is banned from a room?

🤔 I don't know. It shouldn't, as there is a priority indexing structure that we use in Synapse that places bans below leaves. It should just work, but let me stare at this a while and see if I can produce a test for that.

jason-famedly · 2025-10-29T16:35:45Z

Looks good 👍

🎉

I didn't expected that the shared visibility is actually act like a world_readable.

It's not supposed to be. I believe it's actually a Synapse bug, and appears to be part of the "peeking" capability for seeing into a room in advance. I actually have the test scenario written up, thanks for reminding me about that.

Would it make difference if the user is banned from a room?

🤔 I don't know. It shouldn't, as there is a priority indexing structure that we use in Synapse that places bans below leaves. It should just work, but let me stare at this a while and see if I can produce a test for that.

So, to properly test this, would have to basically duplicate(or at least add to) each existing test. Do we want this? Just like with "bob", either is invited to or joins a room, just to get banned and then test ability to see the media. I want to say it comes across the same as a "leave", but am open to suggestion

EDIT: It also looks like it would end up depending on exactly when we ban the user(before or after the media is available). Because we check to see if they were ever able to actually see the media, if they ever could they should still be able to even after a ban

itsoyou · 2025-10-31T17:55:00Z

EDIT: It also looks like it would end up depending on exactly when we ban the user(before or after the media is available). Because we check to see if they were ever able to actually see the media, if they ever could they should still be able to even after a ban

hmm. Banned user still able to see the media doesn't sound right. Should we maybe remove the copied media for banned user?

jason-famedly · 2025-11-03T15:24:47Z

hmm. Banned user still able to see the media doesn't sound right. Should we maybe remove the copied media for banned user?

I'm not sure. I don't think so. A ban is just a "forced" leave event on a room. I think they still have the right to see the past of when they were in there. Consider the case of a malicious ban from a room that was for not a legitimate reason. Would you like to be able to go back and collect that evidence to make an appeal?

itsoyou

As I heard yesterday standup that according to the spec, banned user is still able to see the past events when the person was still in the room. All good 👍

Adjust TestAvatarUrlUpdate to make MXC's match the matrix spec

jason-famedly mentioned this pull request Sep 25, 2025

chore: Complement tests for MSC3911 #2

Closed

4 tasks

jason-famedly force-pushed the msc3911 branch from fe34c11 to 4578690 Compare September 25, 2025 14:31

jason-famedly marked this pull request as ready for review September 29, 2025 17:22

jason-famedly commented Sep 29, 2025

View reviewed changes

.github/workflows/ci.yaml Outdated Show resolved Hide resolved

jason-famedly force-pushed the msc3911 branch from 07b8df1 to c7e7dd2 Compare September 30, 2025 17:17

jason-famedly force-pushed the msc3911 branch from c7e7dd2 to d473269 Compare October 27, 2025 11:07

jason-famedly changed the base branch from main to jason/remove-signoff October 27, 2025 11:22

Base automatically changed from jason/remove-signoff to main October 27, 2025 11:48

itsoyou reviewed Oct 27, 2025

View reviewed changes

jason-famedly mentioned this pull request Oct 29, 2025

chore(MSC3911): Complement famedly/synapse#139

Merged

itsoyou approved these changes Nov 5, 2025

View reviewed changes

chore: Complement tests for MSC3911

4cbce73

Adjust TestAvatarUrlUpdate to make MXC's match the matrix spec

jason-famedly force-pushed the msc3911 branch from 7b65328 to 4cbce73 Compare November 6, 2025 11:33

jason-famedly merged commit ba7b6e5 into main Nov 6, 2025
3 of 4 checks passed

jason-famedly deleted the msc3911 branch November 6, 2025 15:17

chore: Complement tests for MSC3911 #3

chore: Complement tests for MSC3911 #3

Uh oh!

Conversation

jason-famedly commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

itsoyou commented Oct 27, 2025

Uh oh!

jason-famedly commented Oct 27, 2025

Uh oh!

itsoyou Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

itsoyou Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

jason-famedly Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

jason-famedly Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

itsoyou Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

jason-famedly Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itsoyou Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

jason-famedly Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

itsoyou commented Oct 27, 2025

Uh oh!

jason-famedly commented Oct 29, 2025

Uh oh!

jason-famedly commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itsoyou commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jason-famedly commented Nov 3, 2025

Uh oh!

itsoyou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jason-famedly commented Sep 25, 2025 •

edited

Loading

jason-famedly Oct 29, 2025 •

edited

Loading

jason-famedly commented Oct 29, 2025 •

edited

Loading

itsoyou commented Oct 31, 2025 •

edited

Loading