Skip to content

Conversation

@jimmycuadra
Copy link
Contributor

Currently the m.key.verification.start event appears twice with the exact same title, in the "Key verification framework" section and the "Short Authentication (SAS) verification" section. It's not immediately clear that the first occurrence describes the format of the event in general terms and that the second occurrence describes the fields when the m.sas.v1 verification method is being used. This is a similar relationship to the m.room.message event and its various msgtype
variants.

This commit does three things:

  • It tweaks the generation of the documentation to change the title of the second occurrence of m.key.verification.start to distinguish it from the first.
  • It updates the language in the description of the two versions of the event to better describe the relationship between the two.
  • It adds the optional next_method field to the schema of the m.sas.v1 variant, as specified in the general form of m.key.verification.start.

Note: These changes are based on my intuition of what the intent was, but I might be completely wrong about any or all of this!

Signed-off-by: Jimmy Cuadra [email protected]

@jimmycuadra jimmycuadra changed the title Clarify the distinction between *m.key.verification.start* and its *msas.v1* variant. Clarify the distinction between *m.key.verification.start* and its *m.sas.v1* variant. Jun 14, 2019
….sas.v1* variant.

Currently the *m.key.verification.start* event appears twice with the
exact same title, in the "Key verification framework" section and the
"Short Authentication (SAS) verification" section. It's not immediately
clear that the first occurrence describes the format of the event in
general terms and that the second occurrence describes the fields when
the *m.sas.v1* verification method is being used. This is a similar
relationship to the *m.room.message* event and its various *msgtype*
variants.

This commit does three things:

* It tweaks the generation of the documentation to change the title
  of the second occurrence of *m.key.verification.start* to
  distinguish it from the first.
* It updates the language in the description of the two versions of the
  event to better describe the relationship between the two.
* It adds the optional `next_method` field to the schema of the
  *m.sas.v1* variant, as specified in the general form of
  *m.key.verification.start*.

Signed-off-by: Jimmy Cuadra <[email protected]>
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This review is mostly a bunch of small things.

@uhoreg is probably better suited about the contents.

@turt2live turt2live requested a review from uhoreg June 14, 2019 23:22
@jimmycuadra
Copy link
Contributor Author

Another issue related to this that I noticed is that m.key.verification.accept has equivalent fields to the m.sas.v1-specific version of m.key.verification.start. This makes me think that the accept event should have a "generic" form and method-specific forms just like the start event does. In other words, I think there ought to be separate "m.key.verification.accept" and "m.key.verification.accept (m.sas.v1)" sections to mirror the two already addressed in this PR. Or perhaps just adding "(m.sas.v1)" to the title of the current section without adding a "generic" version. Thoughts?

@turt2live
Copy link
Member

re: keeping the accept event separate: I'd keep it separate for now. The event isn't really generic enough to be promoted to the general structure yet, as some verification methods may not use the event.

It's a little confusing at the moment, however we anticipate other verification methods being introduced. The spec is built to support the future case, as hopefully it'll be near future rather than distant future.

* Switch "an SAS" back to "a SAS"
* Remove the `next_method` field from m.key.verification.start$m.sas.v1
  but add additional clarification to its description on
  m.key.verification.start that it is never present for methods that
  verify keys both ways.
@jimmycuadra
Copy link
Contributor Author

Okay, I think I've addressed all feedback. Ready for another review.

@turt2live turt2live self-requested a review June 15, 2019 20:59
@turt2live
Copy link
Member

It looks like I'm unable to commit the changelog change myself, but this PR looks good to go once the changelog is addressed.

Thank you!

@turt2live turt2live merged commit ace94f0 into matrix-org:master Jun 19, 2019
@jimmycuadra jimmycuadra deleted the clarify-m.key.verification.start branch June 19, 2019 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants