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.
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
MSC2241: Key verification in DMs #2241
MSC2241: Key verification in DMs #2241
Changes from 14 commits
4f65524
fb724bc
0e6286f
0007498
e19fca7
7469198
ab3ed73
1f1d22f
06ee66d
1d165ee
788e987
7466955
f4bad37
a514485
1590ae2
91f51bb
4a77978
affa240
6096416
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a bit sad that we're using m.room.message here. I guess it's for the benefit of clients which don't support this method, but maybe an alternative would be to send two events, one m.room.message for the fallback, which supporting clients can ignore, and another m.key.verification.request which actually starts the process
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's so that clients that don't support it will show something sensible. Sending two messages seems redundant, and you'd need some markup on the
m.room.message
so that supporting clients will know to ignore it, so it doesn't seem like much of a difference from just sticking the verification request payload in them.room.message
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted by @poljar, this makes it very complicated to implement this MSC in Ruma, since previously we could just have a single content struct definition for every event type. With this, we'd have to use different definitions per event "kind" (ephemeral / message / state / to-device / ...).
Is there any chance of unified content objects between the to-device and message events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is no longer relevant. This has been implemented, it turned out manageable. I can't mark it as resolved myself though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably this works fine if a client gets a key verification method via a non-DM room? One of the things with the current canonical DM proposal is that it doesn't actually guarantee there will be a single DM room at any given point (given glare and upgrading rooms and servers not agreeing etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, nothing here actually requires the room to be a DM, but clients should use a DM to avoid cluttering unrelated rooms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug in the proposal intending to be fixed. There will only ever be one DM by the time the proposal gets merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that clients have to be careful to ignore key verification messages not "directed" at them? Is there a situation where the DMness of a room is inconsistent between servers? E.g. you are in a 3-way room that the one of the other participants thinks is a DM room, but you don't, what happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that clients should be able to respond to requests in non-DM rooms. So if clients don't agree on which rooms are DM, it should still work. If you try to do a verification in Matrix HQ, you'll just annoy everyone and someone will tell you to do it elsewhere.