-
Notifications
You must be signed in to change notification settings - Fork 347
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
Update on E2E implementation guide #201
Conversation
@benparsons I think it's mostly just a case of nobody having had time to review it yet. Ugh. It's Friday already? I guess I should put it near the top of my list for next week. |
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.
Sorry for taking so long to review this. I haven't looked at it in detail yet, but I have a few wording changes, questions, and some technical issues. But overall, it's looking good, and we really appreciate the work that you've put into this.
``{}`` if you don’t want to upload the device keys.) | ||
|
||
However, a client should not rely on this in order to find out how many |
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.
Why should it not be relied on? Is it just because the value can change, and so the value can be out of date?
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 meant for performance reasons. I rephrased it.
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.
OK, I see. "should not rely on" usually means that the value will be inaccurate, so I'd use different wording.
Looking at the whole section, though, it kind of seems weird to say "you can find the value by doing this, but don't do that, do this other thing instead". So I think it would be better to just drop the mention of checking keys/upload
for checking the number of keys.
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.
Agreed and made the change.
Encrypted events using this algorithm should have a ``sender_key`` and a | ||
``ciphertext`` property. | ||
In the spec, this algorithm is detailed `here`__ and an example payload can be | ||
seen `here`__ . |
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.
Generally, links called "here" give poor usability. Links should have descriptive text whenever possible. For example, this sentence could be reworded as "The spec gives details on this algorithm and an example payload".
@@ -372,6 +308,12 @@ follows [#]_. | |||
``m.megolm.v1.aes-sha2`` | |||
~~~~~~~~~~~~~~~~~~~~~~~~ | |||
|
|||
In the spec, this algorithm is detailed `here`__ and an example payload can be |
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.
same comment as above
@@ -558,44 +459,29 @@ __ `m.room_key`_ | |||
|
|||
The client must then share the keys for this session with each device in the | |||
room. It must therefore `download the device list`_ if it hasn't already done | |||
so, and for each device in the room which has not been `blocked`__, the client | |||
should: | |||
so. and for each device in the room which has not been `blocked`__, the client |
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 looks like a bunch of this line was meant to be removed, but wasn't.
should: | ||
so. and for each device in the room which has not been `blocked`__, the client | ||
Then it should build a unique |room_key|_ event, and send it encrypted to each | ||
device in the room which has not been `blocked`__, `using Olm`__. |
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.
The "using Olm" at the end of the sentence seems a bit out of place, because it's not clear what it refers to. I'm guessing that it means that it's encrypted using Olm, so I think changing the sentence to "... and send it encrypted using Olm to each device..."
.. _`room_key`: https://matrix.org/docs/spec/client_server/r0.4.0.html#m-room-key | ||
|
||
.. |m.room_key_request| replace:: ``m.room_key_request`` | ||
.. _`room_key_request`: https://matrix.org/docs/spec/client_server/r0.4.0.html#m-room-key-request |
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.
missing the m.
before room_key_request
(also in forwarded_room_key
below)
want to reply to a key request with the associated key if it can assert that | ||
the requesting device is allowed to see the messages encrypted with this key. | ||
|
||
Those capabilities are achieved using |m.room_key_request| and |
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.
missing the underscore after |m.room_key_request|
, which means that this doesn't get turned into a link. Also in other places in this section.
Fixed, should I add a commit to update the guide name as it includes a date? |
I think you leave the date the same. I think the date is just to make jekyll happy, and doesn't actually mean anything. |
The client should build a JSON query object as follows: | ||
|
||
.. code:: json | ||
__ `blocking` |
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.
The last underscore here needs to be kept, or else the link breaks. (I'm guessing it was deleted by accident.)
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.
Looks good! Thanks for writing this up! Just one minor change, which I'll fix up.
Attempt to bring the implementation guide up-to-date with the current state of E2E in Matrix.
It can't be merged before matrix-org/matrix-spec-proposals#1465, matrix-org/matrix-spec-proposals#1420, and matrix-org/matrix-spec-proposals#1284 are, since it contains links to those parts.
Signed-off-by: Valentin Deniaud <valentin.deniaud@inpt.fr>