Skip to content
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

Clarify the distinction between *m.key.verification.start* and its *m.sas.v1* variant. #2132

Merged

Conversation

@jimmycuadra
Copy link
Contributor

jimmycuadra commented Jun 14, 2019

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 jimmy@jimmycuadra.com

@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 <jimmy@jimmycuadra.com>
@jimmycuadra jimmycuadra force-pushed the jimmycuadra:clarify-m.key.verification.start branch from 537cdf8 to bc71dac Jun 14, 2019
Copy link
Member

turt2live left a comment

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

@uhoreg is probably better suited about the contents.

scripts/templating/matrix_templates/units.py Outdated Show resolved Hide resolved
event-schemas/schema/m.key.verification.start Outdated Show resolved Hide resolved
@turt2live turt2live requested a review from uhoreg Jun 14, 2019
Signed-off-by: Jimmy Cuadra <jimmy@jimmycuadra.com>
@jimmycuadra

This comment has been minimized.

Copy link
Contributor Author

jimmycuadra commented Jun 15, 2019

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

This comment has been minimized.

Copy link
Member

turt2live commented Jun 15, 2019

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

This comment has been minimized.

Copy link
Contributor Author

jimmycuadra commented Jun 15, 2019

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

@turt2live turt2live self-requested a review Jun 15, 2019
@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Jun 18, 2019

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
7 checks passed
7 checks passed
ci/circleci: build-dev-scripts Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
ci/circleci: check-docs Your tests passed on CircleCI!
Details
ci/circleci: validate-docs Your tests passed on CircleCI!
Details
docs Click details to preview the HTML documentation.
Details
swagger Click to preview the swagger build.
Details
@jimmycuadra jimmycuadra deleted the jimmycuadra:clarify-m.key.verification.start branch Jun 19, 2019
@jplatte jplatte referenced this pull request Nov 14, 2019
1 of 21 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.