Skip to content

Don't double cancel verification request - #1064

Merged
dbkr merged 2 commits into
developfrom
bwindels/dmverif-cancellation
Nov 8, 2019
Merged

dbkr merged 2 commits into
developfrom
bwindels/dmverif-cancellation

Conversation

@bwindels

@bwindels bwindels commented Nov 4, 2019

Copy link
Copy Markdown
Contributor

This fixes the verifier not handling incoming m.key.verification.cancel events, and just treating them as unexpected and sending a m.key.verification.cancel event of its own with an error. These double m.key.verification.cancel events make it harder to detect which user actually pressed cancel.

Initial PR for verification over DM was #1050

@bwindels
bwindels requested review from a team and uhoreg November 4, 2019 13:41
@bwindels
bwindels force-pushed the bwindels/dmverif-cancellation branch from d8b50c4 to 37558ac Compare November 4, 2019 13:44
this._resetTimer();
this._resolveEvent(e);
} else if (e.getType() === "m.key.verification.cancel") {
const reject = this._reject;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that if you just do a this.cancel(e), then it should do the right thing, because the cancel function says that it handles cancellations from the other user (if it gets passed a MatrixEvent). Though we probably also want to call this._rejectEvent if it's defined, so it may be better to just stick this in with the else clause, and assign to exception based on whether e.getType() === "m.key.verification.cancel".

Though it looks like the else clause isn't doing the right thing right now, because it really should be causing the client to send an m.key.verification.cancel with code set to m.unexpected_message. So, I think if you do something like

const exception = e.getType() === "m.key.verification.cancel" ? e : newUnexpectedMessageError();

in the else clause (you'll need to import newUnexpectedMessageError from Error.js), then it should fix that and the double-cancel issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I tried calling _rejectEvent, but that made it send an m.key.verification.cancel event like before. Will have another look

@bwindels
bwindels removed the request for review from a team November 4, 2019 16:52
@dbkr
dbkr merged commit 37558ac into develop Nov 8, 2019
@t3chguy
t3chguy deleted the bwindels/dmverif-cancellation branch May 10, 2022 14:26
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