Check /versions response for "unstable" MSC3916 (authenticated media) support

[S7evinK]

This allows clients to use authed media with server implementations not announcing support for v1.11 but still having support for MSC3916. (for example Dendrite)

Checklist

• Tests written for new code (and old code if feasible).
• New or updated public/exported symbols have accurate TSDoc documentation.
• Linter and other CI checks pass.
• Sign-off given on the changes (see CONTRIBUTING.md).

[richvdh]

Is this written down anywhere? I feel like it should be in the MSC if we're going to support it

[S7evinK]

Only asked internally if this was fine, and @dbkr seemed fine with it? Also the matrix-dart-sdk is doing the same, which I (personally) think is a sensible thing to do.

[dbkr]

Ah, I just gave some context on why it was the way that it was. @richvdh is right though, this prefix isn't mentioned anywhere in the MSC... where is it from? Also the PR title says 'unstable' but the prefix says 'stable'... and also it's a prefix so, by definition, not stable... I'm very confused :)

[S7evinK]

Then again, why has it ever existed in the unstable section, if it wasn't mentioned in the MSC?

[neilalexander]

this prefix isn't mentioned anywhere in the MSC... where is it from?

I feel like it should be in the MSC if we're going to support it

Ideally every single MSC that adds a feature should have an org.matrix.stable.mscXXXX or org.matrix.mscXXXX-style capability identifier automatically assigned as the rule, not the exception. Otherwise it's nigh-on impossible for homeserver implementations to declare partial support for a spec version or for specific features.

Once upon a time, graceful degradation was supposed to be a pillar of the Matrix protocol design, but that's simply and demonstrably not possible without finer-grained capability negotiation like this. It should be possible for a server to state either support for MSC3916 or support for spec version 1.11 (in which case, the feature is implicit and the capability identifier does not need to be called out separately) and have clients do the right thing.

[deepbluev7]

I think supporting this flag is pretty important from an ecosystem perspective, because otherwise homeservers, that are "behind" in spec support can't enable authenticated media.

The stable/unstable identifier isn't in the spec, but it is a convention that came up in a discussion between FluffyChat/dart-matrix-sdk and Conduit, that other homeserver developers seem to also have been involved in. The MSC didn't have any stable flags, but imo to support a quick rollout of authenticated media, probably should have one.

Without this flag, other homeservers have the choice of lying about their version support and claim support for 1.11 with only partial support for it or not supporting authenticated media in Element clients at all. I think the better option is to standardize clients on also supporting the unstable flag for a while until server support is somewhat caught up to recent Matrix versions.

If you can agree with that, I guess we could move the conversation then to how we move this PR forward. Imo we have 3 options:

• We pick this "stable" identifier as an informal standard
• We do another MSC just for the stable flag
• We do a PR to the merged MSC and add the stable flag after the fact without an MSC

The first option can imo also be done in parallel with the other options. But I think it would be a very good thing for the ecosystem, if Element supported this flag and imo the maintenance burden of it seems to not be that high.

I hope it is okay for me to throw my opinion on this in here, if I am disrupting the review process, feel free to mark this comment as offtopic. Thank you!

EDIT: I opened matrix-org/matrix-spec-proposals#4180, which may help this discussion.

[richvdh]

Ideally every single MSC that adds a feature should have an org.matrix.stable.mscXXXX or org.matrix.mscXXXX-style capability identifier automatically assigned as the rule, not the exception. Otherwise it's nigh-on impossible for homeserver implementations to declare partial support for a spec version or for specific features.

This isn't really the place for this discussion, but hard disagree. That sort of spec fragmentation is exactly what happened to XMPP, and it didn't go well.

I'm prepared to make an exception in this case, but I see this as a dangerous pattern.

[richvdh]

Then again, why has it ever existed in the unstable section, if it wasn't mentioned in the MSC?

well, great question. I guess we hallucinated this bit of the MSC. Wrongs of the past don't excuse more wrongs though.

[S7evinK]

https://github.com/element-hq/synapse/blob/f1a1c7fc537514ce5919d04b492ad3b2dba74646/synapse/rest/client/versions.py#L177

"past". Where did it come from, where did it go?

[richvdh]

https://github.com/element-hq/synapse/blob/f1a1c7fc537514ce5919d04b492ad3b2dba74646/synapse/rest/client/versions.py#L177

"past". Where did it come from, where did it go?

Not really following your point here.

Anyway now that this is in the MSC doc thanks to Nico, I'm happier about it landing here. It would be good to expand the PR title so that it reads better in the changelog.

[t3chguy]

This same change would likely need mirroring to element-desktop which does not run the serviceworker

[CLAassistant]

All committers have signed the CLA.

[S7evinK]

FTR: I'm not going to update this PR further (or mirror it to ED), it was born out of frustration that other server implementations (other than Synapse) are simply not taken into account.

[S7evinK]

(Also unsure why I had to sign the CLA as a member of element-hq)

[Xiretza]

Thanks for trying.

[S7evinK]

Thanks for trying.

CI is happy, and with the now updated title, it may have a chance to be merged.

[t3chguy]

CI is happy, and with the now updated title, it may have a chance to be merged.

Unfortunately not if it doesn't match the behaviour of ED, both would need to be changed in lockstep

[neilalexander]

CI is happy, and with the now updated title, it may have a chance to be merged.

Unfortunately not if it doesn't match the behaviour of ED, both would need to be changed in lockstep

Can it not be cherry-picked from one into the other..? It’s a grand total of 4 lines.

[t3chguy]

Element Desktop doesn't have service workers, so the code would not be the same. I believe all the changes for ED would happen in this repo though, in ElectronPlatform or similar.

[S7evinK]

I've had a look at ED yesterday, and since my environment isn't setup for Typescript, I didn't want to change stuff here (only returns the versions instead of the whole response to /versions)

I'd be happy if you (or someone else) could mirror the change to ED as I'm not using EW anymore and never used ED to begin with.

[richvdh]

Having discussed this with the rest of the EW team:

The team are unhappy about the support implications of having the Web and Desktop versions of Element support different homeserver implementations. So, before this can land, we will need equivalent support in Element Desktop. Unfortunately that's not something the team has bandwidth to work on.

If anyone would like to contribute equivalent functionality for Element Desktop, we'd be happy to merge both PRs.

We'll keep this PR open for now, but if nobody is able to to contribute the ED functionality, we'll have to close it, unfortunately.