Missing quotes around value in /.well-known/matrix/client?

[nrgill28]

Describe the bug

I was having trouble logging in to my account with a matrix client, and through the logs it seemed to be complaining that my /.well-known/matrix/client was not valid JSON. Sure enough, it appears to be missing some double quotes around one of the values.

{
        "m.homeserver": { "base_url": "https://home.server" },
        "im.vector.riot.jitsi": {"preferredDomain": "jitsi.riot.im"},
        "im.vector.riot.e2ee": {"default": all } // <-- Should be "all"?
}

The well-known is served directly from the nginx config, and it too is missing the double quotes.

https://github.com/YunoHost-Apps/synapse_ynh/blob/6537f170c3bea49c3cba84ac6709c09d311915e6/conf/server_name.conf#L11

What confuses me though is that this file hasn't been modified in three years, and despite the JSON being invalid multiple other clients seem to not have any issues with it. I'm not really sure what's going on so I figured I'd open an issue instead of blindly making a PR adding the double quotes.

[Josue-T]

Well it's one more issue linked to #356....

@rosbeef, @Gredin67, @Thatoo can you fix it. It should be a json boolean here: true or false, not all, on, off or everything else...

As documented here: https://github.com/vector-im/element-web/blob/develop/docs/e2ee.md

[Thatoo]

Well #356 is about configuring synapse homeserver.yaml config file and, as documented here, https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#encryption_enabled_by_default_for_room_type, encryption_enabled_by_default_for_room_type is indeed taking either all, invite or off.

The problem is that we use __E2E_ENABLED_BY_DEFAULT__ for two different things, for encryption_enabled_by_default_for_room_type in homeserver.yaml file and for "im.vector.riot.e2ee", which by the way should be replaced by "io.element.e2ee" now, in server_name.conf.

So we need to replace one of the two.

[Thatoo]

What do you think about keeping __E2E_ENABLED_BY_DEFAULT__ for encryption_enabled_by_default_for_room_type in homeserver.yaml file and changing in server_name.conf to __E2E_ENABLED_BY_DEFAULT_IN_ELEMENT__ (this one being either true or false)?

[Thatoo]

And I think we should have__E2E_ENABLED_BY_DEFAULT_IN_ELEMENT__ automatically to false if __E2E_ENABLED_BY_DEFAULT__ is off and automatically to true if __E2E_ENABLED_BY_DEFAULT__ is either invite or all.

What do you think?

[Josue-T]

And I think we should have__E2E_ENABLED_BY_DEFAULT_IN_ELEMENT__ automatically to false if __E2E_ENABLED_BY_DEFAULT__ is off and automatically to true if __E2E_ENABLED_BY_DEFAULT__ is either invite or all.

What do you think?

It's ok for me

[Thatoo]

Here is my draft : https://github.com/YunoHost-Apps/synapse_ynh/compare/master...Thatoo:synapse_ynh:Thatoo-update_config_panel_e2e

What do you think?

[Thatoo]

I need to rework my upgrade script but the rest is, I think, ok now.

[Josue-T]

Well, personnally I just prefer to generate the settings dynamically and don't add one more settings which could be a bit confusing for users. So on all script (or in _common.sh) i would just add this:

if [ "$e2e_enabled_by_default" = "all" ] || [ "$e2e_enabled_by_default" = "invite" ] ; then
        e2e_enabled_by_default_in_element="true"
elif [ "$e2e_enabled_by_default" = "off" ] ; then
        e2e_enabled_by_default_in_element="false"
fi

[Thatoo]

I have updated my upgrade script and I have tested everything, install, modification in the config panel, upgrade (from a yunohost that didn't have this PR, and from a Yunohost that had already this PR). Everything is working now.

[Thatoo]

About

Well, personnally I just prefer to generate the settings dynamically and don't add one more settings which could be a bit confusing for users. So on all script (or in _common.sh) i would just add this:

if [ "$e2e_enabled_by_default" = "all" ] || [ "$e2e_enabled_by_default" = "invite" ] ; then
        e2e_enabled_by_default_in_element="true"
elif [ "$e2e_enabled_by_default" = "off" ] ; then
        e2e_enabled_by_default_in_element="false"
fi

I'm not sure I understand what means "generate the settings dynamically" compare to what I'm doing. And I don't understand which "one more settings" I'm adding in my branch. If you just add

if [ "$e2e_enabled_by_default" = "all" ] || [ "$e2e_enabled_by_default" = "invite" ] ; then
        e2e_enabled_by_default_in_element="true"
elif [ "$e2e_enabled_by_default" = "off" ] ; then
        e2e_enabled_by_default_in_element="false"
fi

then the /.well-known/matrix/client in /etc/nginx/conf.d/${server_name}.d/${app}_server_name.conf file is not affected, I have to add the sed line for that : sed -i -r "s|\"im\.vector\.riot\.e2ee\": \{\"default\": [A-Za-z]+ \}|\"im.vector.riot.e2ee\": {\"default\": ${e2e_enabled_by_default_in_element} }|g" "/etc/nginx/conf.d/${server_name}.d/${app}_server_name.conf" but for it to work, I need to define ${server_name}. so I need to add that second line : server_name=$(ynh_app_setting_get --app $app --key server_name)

However, I'm not a very experienced yunohost contributor so I assume to be wrong and I'd like to learn the best practice.

[Thatoo]

I think I got it, maybe you meant that you don't want it to appear in config_panel.toml, is that it? Then, ok, got it, here is an other branch that could do what you prefer I guess :

https://github.com/YunoHost-Apps/synapse_ynh/compare/master...Thatoo:synapse_ynh:Thatoo-update_config_panel_e2e_no_panel

The problem with this is that the user can't choose to have this situation : encryption_enabled_by_default_for_room_type to off in homeserver.yaml and thus have new rooms not encrypted by default but still want to set new DM to be encrypted by default with "im.vector.riot.e2ee": {"default": true }

With this branch, this situation is impossible.

Which branch should I PR then?

[Josue-T]

I think I got it, maybe you meant that you don't want it to appear in config_panel.toml, is that it?

Yes, I think it's better to hide the variable e2e_enabled_by_default_in_element on the control panel.

Which branch should I PR then?

Your PR should always be based on testing.

[Thatoo]

I've just made a PR based on Testing

[Josue-T]

Fixed by #426