Add MsgType.DecryptionFailure #2289
Conversation
| @@ -106,6 +106,7 @@ export enum MsgType { | |||
| Location = "m.location", | |||
| Video = "m.video", | |||
| KeyVerificationRequest = "m.key.verification.request", | |||
| DecryptionFailure = "m.bad.encrypted", | |||
There was a problem hiding this comment.
This doesn't appear in the spec - should it be namespaced? (or is it missing?)
There was a problem hiding this comment.
Good question; afaik, this is just a thing matrix-js-sdk does internally to denote decryption failures, see https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/models/event.ts#L884 . I don't know whether that's a good idea, but it's what we've been doing for the past 5 years...
There was a problem hiding this comment.
screaming internally
I'd vote breaking change to the SDK that removes that off-spec field, now that it's been unearthed. We should also use the enum in all the places we currently use the string.
To account for it being off-spec, I sort of wonder if it should be in this enum or a different enum altogether. Even with documentation we end up in a situation where someone using the SDK could try and use it as a valid MsgType (for reasons I can't really comprehend), though on the other hand naming this InternalDecryptionFailure and documentation might discourage usage enough? Thoughts welcome.
There was a problem hiding this comment.
Yeah, naming the field InternalDecryptionFailure, changing its value to org.matrix.bad.encrypted, and replacing the string with the enum everywhere seems like a reasonable course of action to me. No strong feelings about whether it should stay in the MsgType enum or live somewhere else; the way I'm using it in matrix-org/matrix-react-sdk#8272 is completely analogous to the other MsgTypes, but it's definitely a special case in other ways.
There was a problem hiding this comment.
Any checks for m.bad.encrypted should probably be replaced with calls to isDecryptionFailure().
Using the msgtype to indicate some internal state is really hacky. Ages ago, when I was thinking about improving UISI display, I was thinking that the Event class should really be storing encryption errors differently, rather than shoving the decryption error message in the event body. Maybe it could even store the exception itself somehow. Then, the react SDK can get more information so that it can display the UISI more intelligently.
|
Looks like this isn't the right way forward, I'll update matrix-org/matrix-react-sdk#8272 to use a different approach. |
This PR currently has no changelog labels, so will not be included in changelogs.
A reviewer can add one of:
T-Deprecation,T-Enhancement,T-Defect,T-Taskto indicate what type of change this is, or addType: [enhancement/defect/task]to the description and I'll add them for you.