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

MSC3122: Deprecate starting verifications without requesting first #3122

Merged
merged 5 commits into from
May 9, 2021

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Apr 14, 2021

@uhoreg uhoreg changed the title MSCxxxx: Deprecate starting verifications without requesting first MSC3122: Deprecate starting verifications without requesting first Apr 14, 2021
@uhoreg uhoreg added kind:maintenance MSC which clarifies/updates existing spec proposal A matrix spec change proposal proposal-in-review labels Apr 14, 2021
not provide a good user experience, and allowing this adds unnecessary
complexity to implementations.

We propose to deprecate allowing this behaviour.
Copy link
Contributor

@deepbluev7 deepbluev7 Apr 14, 2021

Choose a reason for hiding this comment

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

Yes please! This is one thing that tripped me up a lot when implementing it, since Element either sends request or start. It would make this a lot easier to implement for new clients and allow me to clean out some cruft! It adds a few extra branches to already a complex state machine, not having things start with start would allow me to clean up stuff like this in the future: https://github.com/Nheko-Reborn/nheko/blob/a6c89d13622907e4c4bf8464fc54fd6e98b62b0a/src/DeviceVerificationFlow.cpp#L468 and would be much easier to test then, because you need half as many test cases.

@uhoreg
Copy link
Member Author

uhoreg commented Apr 14, 2021

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Apr 14, 2021

Team member @uhoreg has proposed to merge this. The next step is review by the rest of the tagged people:

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

@mscbot mscbot added disposition-merge proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. and removed proposal-in-review labels Apr 14, 2021
@uhoreg uhoreg added this to Ready for FCP ticks in Spec Core Team Backlog Apr 14, 2021
uhoreg and others added 2 commits April 20, 2021 11:39
…t.md

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Comment on lines +22 to +23
verifications in this way, but will still need to accept verifications begun in
this way, until it is removed from the spec.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this actually changes much at the spec level, given everyone will still have to support it :/.

still, not much else we can do, I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

It at least discourages new implementations from implementing it and in practice, it is only element still doing requests without request. So if a client dev doesn't care for old Element versions, they can drop it. Considering that you probably want to have a secure client anyway, you shouldn't be running very old versions of any client anyway, if you are trying to verify someone securely. Considering that, I feel like this could actually be removed from the spec somewhat soonish, like in a year for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the goal is to eventually remove it from the spec. Currently, Element is the only client that does it, and only in one specific spot, which I hope to remove soonish. So probably a year or two after it gets removed from Element, it should probably be safe to drop from the spec.

@mscbot
Copy link
Collaborator

mscbot commented May 4, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels May 4, 2021
@mscbot
Copy link
Collaborator

mscbot commented May 9, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels May 9, 2021
@turt2live turt2live merged commit 6db3885 into matrix-org:master May 9, 2021
@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels May 9, 2021
@richvdh richvdh moved this from Ready for FCP ticks to Done to some definition in Spec Core Team Backlog May 11, 2021
@uhoreg uhoreg added merged A proposal whose PR has merged into the spec! and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels May 20, 2021
@uhoreg
Copy link
Member Author

uhoreg commented May 20, 2021

Merged! 🎉

richvdh pushed a commit that referenced this pull request Aug 23, 2021
MSC3122: Deprecate starting verifications without requesting first
richvdh pushed a commit that referenced this pull request Aug 27, 2021
MSC3122: Deprecate starting verifications without requesting first
uhoreg added a commit to uhoreg/matrix-js-sdk that referenced this pull request Oct 21, 2021
The old verification methods start a verification by sending an
`m.key.verification.start` message, rather than an `m.key.verification.request`
message.  This behaviour is deprecated in the
spec (matrix-org/matrix-spec-proposals#3122), so these functions
should be deprecated too.
uhoreg added a commit to matrix-org/matrix-js-sdk that referenced this pull request Oct 21, 2021
The old verification methods start a verification by sending an
`m.key.verification.start` message, rather than an `m.key.verification.request`
message.  This behaviour is deprecated in the
spec (matrix-org/matrix-spec-proposals#3122), so these functions
should be deprecated too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:maintenance MSC which clarifies/updates existing spec merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
Archived in project
Spec Core Team Backlog
  
Done to some definition
Development

Successfully merging this pull request may close these issues.

None yet

5 participants