Open
Conversation
Comment on lines
+346
to
+350
| // The creator field was removed in room version 11 (MSC4239). | ||
| n, err := strconv.Atoi(string(roomVer)) | ||
| if err != nil || n < 11 { | ||
| content["creator"] = creator | ||
| } |
Collaborator
There was a problem hiding this comment.
Feels like something that should be handled by this kind of thing:
iRoomVer := gomatrixserverlib.MustGetRoomVersion(roomVer)
if iRoomVer.CreatorInCreateEvent() {
CreatorInCreateEvent doesn't exist yet but aligns with the other things we have there like DomainlessRoomIDs and PrivilegedCreators
See #840 for how v12 rooms were added
Author
There was a problem hiding this comment.
that's a good idea! done, and also added a change there matrix-org/gomatrixserverlib#465
1 task
Signed-off-by: Thomas Traineau t.traineau@famedly.com
FrenchGithubUser
force-pushed
the
tt/room-v11-default-upstreaming
branch
from
6af47bf to
1feb361
Compare
Comment on lines
+44
to
+47
| // The creator field was removed in room version 11 (MSC4239). | ||
| if defaultVerN < 11 { | ||
| must.Equal(t, ev.Get("content").Get("creator").Str, alice.UserID, "wrong content.creator") | ||
| } |
Collaborator
There was a problem hiding this comment.
Looks like we can use the same CreatorInCreateEvent check in this other spot as well
Comment on lines
500
to
+516
| ver := alice.GetDefaultRoomVersion(t) | ||
| // This test crafts a "bad" event which state_key is 280 bytes but only 70 | ||
| // codepoints. | ||
| // | ||
| // Room version 11 in Synapse switched from using codepoints to using | ||
| // bytes. Which means the 280-byte state_key would be rejected immediately. | ||
| // Use room version 10 in that case so the codepoint-based limit is in effect. | ||
| // | ||
| // Since upgrading a room (for example from v10 to v11) won't carry the event | ||
| // (a new room is created), we don't have to worry about v10 room events in a v11 | ||
| // room. | ||
| // | ||
| // So this test is essentially skipped for any default room v11 or higher. | ||
| verNum, _ := strconv.Atoi(string(ver)) | ||
| if verNum >= 11 { | ||
| ver = gomatrixserverlib.RoomVersion("10") | ||
| } |
Collaborator
There was a problem hiding this comment.
This seems like yet another case that we should handle with a new assert iRoomVer.StrictEventByteLimits() check
You can see how we do this in Synapse synapse/event_auth.py#L487-L499
Comment on lines
500
to
+516
| ver := alice.GetDefaultRoomVersion(t) | ||
| // This test crafts a "bad" event which state_key is 280 bytes but only 70 | ||
| // codepoints. | ||
| // | ||
| // Room version 11 in Synapse switched from using codepoints to using | ||
| // bytes. Which means the 280-byte state_key would be rejected immediately. | ||
| // Use room version 10 in that case so the codepoint-based limit is in effect. | ||
| // | ||
| // Since upgrading a room (for example from v10 to v11) won't carry the event | ||
| // (a new room is created), we don't have to worry about v10 room events in a v11 | ||
| // room. | ||
| // | ||
| // So this test is essentially skipped for any default room v11 or higher. | ||
| verNum, _ := strconv.Atoi(string(ver)) | ||
| if verNum >= 11 { | ||
| ver = gomatrixserverlib.RoomVersion("10") | ||
| } |
Collaborator
There was a problem hiding this comment.
Seems like this test would be better if we just always used room version 10 🤔 . The current setup is only better if we're trying to support homeservers that don't support room version 10 yet (do we care about that?)
Suggested change
| ver := alice.GetDefaultRoomVersion(t) | |
| // This test crafts a "bad" event which state_key is 280 bytes but only 70 | |
| // codepoints. | |
| // | |
| // Room version 11 in Synapse switched from using codepoints to using | |
| // bytes. Which means the 280-byte state_key would be rejected immediately. | |
| // Use room version 10 in that case so the codepoint-based limit is in effect. | |
| // | |
| // Since upgrading a room (for example from v10 to v11) won't carry the event | |
| // (a new room is created), we don't have to worry about v10 room events in a v11 | |
| // room. | |
| // | |
| // So this test is essentially skipped for any default room v11 or higher. | |
| verNum, _ := strconv.Atoi(string(ver)) | |
| if verNum >= 11 { | |
| ver = gomatrixserverlib.RoomVersion("10") | |
| } | |
| // This test crafts a "bad" event which state_key is 280 bytes but only 70 | |
| // codepoints. | |
| // | |
| // Room version 11 in Synapse switched from using codepoints to using | |
| // bytes. Which means the 280-byte state_key would be rejected immediately. | |
| // Use room version 10 in that case so the codepoint-based limit is in effect. | |
| // | |
| // Since upgrading a room (for example from v10 to v11) won't carry the event | |
| // (a new room is created), we don't have to worry about v10 room events in a v11 | |
| // room. | |
| // | |
| // So this test is essentially skipped for any default room v11 or higher. | |
| ver = gomatrixserverlib.RoomVersion("10") |
| // hashes will not be the same. | ||
| "complement_entropy": util.RandomString(18), | ||
| } | ||
| if gomatrixserverlib.MustGetRoomVersion(gomatrixserverlib.RoomVersion(roomVer)).CreatorInCreateEvent() { |
Collaborator
There was a problem hiding this comment.
TODO: Wait for matrix-org/gomatrixserverlib#465 to merge and update the gomatrixserverlib version in this PR
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
creatorfield anymore during room creation, which broke the tests.state_keylength in events (from codepoints to bytes) at room v11, which broke the tests.This PR fixes those issues, making complement work with room v11 as default.
Related to element-hq/synapse#18680