search

YunoHost-Apps / synapse_ynh

Matrix server (synapse) package for YunoHost
https://matrix.org/
GNU General Public License v3.0
79 stars 42 forks source link

improve config panel #356

Closed Gredin67 closed 10 months ago

Gredin67 commented 1 year ago

TODO :

rosbeef commented 11 months ago

But we have to think about people that edit the config file in CLI

You are talking about yunohost cli command or people which directly modify the homeserver.yaml

I had a strange bug

Don't understand from where is comming this "scre". are you sure you did not misstype the screen command in the config file ?

Maybe I have to modify my searching regex

Gredin67 commented 11 months ago

I was talking about people editing homeserver.yaml (not the cli).

The "scre" is a typo. The .yaml is reset at upgrade and applying the config panel did not change the file. I had to change the values and the apply the config panel. So we should solve this upgrade bug.

Le 25 juillet 2023 19:35:11 GMT+02:00, rosbeef andino @.***> a écrit :

But we have to think about people that edit the config file in CLI following the c You are talking about yunohost cli command or people which directly modify the homeserver.yaml

I had a strange bug Don't understand from where is comming this "scre". are you sure you did not misstype the screen command in the file ?

-- Reply to this email directly or view it on GitHub: https://github.com/YunoHost-Apps/synapse_ynh/pull/356#issuecomment-1650255941 You are receiving this because you were mentioned.

Message ID: @.***>

Gredin67 commented 11 months ago

there was an error in commit 2e4e56b It looks like you did not test it. I think my version was tested and correct. With your version I get : Corrupted TOML read from /etc/yunohost/apps/synapse/config_panel.toml (reason: Reserved escape sequence used (line 49 column 1 char 2040))

I corrected and tested. But each time I activate msisdn mandatory for registration, registration in Element breaks, I get the error message Element configuration is wrong

rosbeef commented 11 months ago

The .yaml is reset at upgrade and applying the config panel did not change the file

Because of the typo the registration_3pids section can not be applied because my regex depends on on the presence of 'email' and 'msisdn' test preceded by 'any space' '-' 'anyspace'. As scremsisdn prohibit the section detection noting is replaced.

I am not sure if I understand well the bug 😕

Gredin67 commented 11 months ago

Because of the typo I did not save the typo at any time. It was just a mistake while doing the screenshot.

each time I activate msisdn mandatory for registration, registration in Element breaks

Element works only if I set disable_msisdn: true. Basically everything related to phone numbers seems to be broken on my element. Do you experience the same ?

rosbeef commented 11 months ago

for the email part ,\. and . in the help text is right but it's not well used. ^[^@]+@matrix\.org$ match only <something>@matrix.org but ^[^@]+@matrix.org$ match <something>@matrix<any unique character>org And to print \ in homserver.yaml we have to escape it with \ so in config panel we need to write ^[^@]+@matrix\\.org$

It looks like you did not test it

effectively i did not test msisdn at registration because i misunderstood the disable_msisdn_registration and was setted as true so registration never asked me for msisdn. i tested after registration and the problem exist too. So I have the same problem.

EDIT: disable_msisdn_registration: true does not ask for msisdn registration disable_msisdn_registration: false and registrations_require_3id with only email is not asking for msisdn but not bugging disable_msisdn_registration: false and registrations_require_3id with empty msisdn fail disable_msisdn_registration: false and registrations_require_3id with \+33 msisdn fail is this a bug from synapse ? did you used or test that option before ?

Jul 26 22:20:52 python[7001]: Traceback (most recent call last):
...
Jul 26 22:20:52 python[7001]:     raise ConfigError(
Jul 26 22:20:52 python[7001]: synapse.config._base.ConfigError: Configuration requires msisdn at registration, but msisdn **validation** is not configured

That seems to mean validation with a token is required and seems to be a long time bug : https://github.com/matrix-org/synapse/issues/4630

i tried enable_registration_without_verification: true but doesn't work too image

i did not try to enable captcha and i tried to enable account_threepid_delegates to allow synapse sending token by sms without success.

account_threepid_delegates

Delegate verification of phone numbers to an identity server.

When a user wishes to add a phone number to their account, we need to verify that they actually own that phone number, which requires sending them a text message (SMS). Currently Synapse does not support sending those texts itself and instead delegates the task to an identity server. The base URI for the identity server to be used is specified by the account_threepid_delegates.msisdn option.

If this is left unspecified, Synapse will not allow users to add phone numbers to their account.

So for me the config script is ok The help of mail and msisdn should be written as before your last modification msisdn registration require thirparty sms provider to send token (i did not try a fake provider).

I think it should be good to explore the account_threepid_delegates.msisdn

Now just to modify the update script to recover manually modified datas in the homeserver.yaml

Gredin67 commented 11 months ago

@YunoHost-Apps/matrix-bridges can someone test ? !testme

yunohost-bot commented 11 months ago

Fingers crossed! Test Badge

rosbeef commented 11 months ago

Hi good that pass the test, but update script is not taking in account manual modifications made in homeserver.yaml. Is it a mandatory functionality to merge ? I ask that because i'm with a personnal emergency which should takes me one more week.

Gredin67 commented 11 months ago

It is OK if settings from homeserver.yaml that are not exposed in the config panel are not kept at upgrade. This is the normal behavior in a YunoHost App. From what I remember, the settings that are exposed are correctly kept at upgrade, so from my point of view it is ready to merge !

Gredin67 commented 11 months ago

@alexAubin @zamentur can you review/merge this plz ? homeserver.yaml

Gredin67 commented 11 months ago

or maybe @tituspijean or @ericgaspar

Gredin67 commented 11 months ago

adapted to match https://github.com/YunoHost-Apps/mautrix_whatsapp_ynh/blob/master/scripts/install#L37

rosbeef commented 10 months ago

hello i'm back Is it possible, in term of usability, to advertise the people which manually modify the config to take note of modification before updating then reapply updates manually inside the config pannel ?

and i need to create an advertizing in help message that peoples which enable phone auth that they need a sms proxy.

Gredin67 commented 10 months ago

We don't need to warn people who manually modify their config, because having it overwritten at upgrade (when the upstream config changes) is standard behavior in yunohost. There is already a mechanism that backs up the old config and warns the user with the diff. If you want to improve this, it should be done in yunohost core, not in this package. Do you confirm @zamentur ?

About the SMS proxy, isn't handled by an identity server? We could just explain that an identity server should be configured for msisdn workflow to fonction properly.

In general, we should keep synapse_ynh for the basic yunohost use-case. It may not be necessary to handle SMS in the basic use-case. I would even say we should not promote msisdn registration because phone numbers are critical private data and Matrix identity servers are too centralized. Not requiring it is a major advantage of Matrix. Server admins who have advanced usage can still configure it in CLI, knowing the consequences in terms of privacy.

Le 21 août 2023 21:13:06 GMT+02:00, rosbeef andino @.***> a écrit :

hello i'm back Is it possible, in term of usability, to advertise the people which manually modify the config to take note of modification before updating then reapply updates manually inside the config pannel ?

and i need to create an advertizing in help message that peoples which enable phone auth that they need a sms proxy.

-- Reply to this email directly or view it on GitHub: https://github.com/YunoHost-Apps/synapse_ynh/pull/356#issuecomment-1686883377 You are receiving this because you were mentioned.

Message ID: @.***>

rosbeef commented 10 months ago

I would even say we should not promote msisdn registration because phone numbers are critical private i'm totally agree with you it's why i don't suggest email registration by default too. and it's why i choose matrix instead of signal .

About the SMS proxy, isn't handled by an identity server? We could just explain that an identity server should be configured for msisdn workflow to fonction properly. Humm, i didn't thought about this option of identity server as i don't use it for "security" reasons too. So my help text modification is too strict if you are true in that ID Server can handle that.

can you help me i have a bug that tell me default value is not set for account_threepid_delegates_msisdn in install or upgrade script. but it is setted and appear in settings.yml

image

rosbeef commented 10 months ago

ok i did some last modifications

account_threepid_delegates>msisdn:/etc/matrix-APP/homeserver.yaml

in config*.toml

account_threepid_delegates:
     msisdn: __ACCOUNT_THREEPID_DELEGATES_MSISDN__

in homeserver.yaml

and set default value to "" (none) in install and upgrade

server is starting, config is showing, modifications are applied

I did not test Element with this kind of empty value :pray: test I have no minuts next 3 days. ;)

Gredin67 commented 10 months ago

lgtm! @Josue-T can you please merge in testing ? @YunoHost-Apps/matrix-bridges anyone could test upgrade and functionality of the new config panel ?

Gredin67 commented 10 months ago

!testme

yunohost-bot commented 10 months ago

May the CI gods be with you! Test Badge

Thatoo commented 10 months ago

Tonight on brand new Yunohost and Synapse install : Install OK In the config-panel, when I wanted to change "End-to-End Encryption by default for locally-created Rooms" from "off" to "invite", and I wanted to save, I got the following error : "Le formulaire contient des erreurs" , and it didn't change the value in the homserver.yaml

Then, I wanted to change "Largest allowed media upload size in bytes." from 10M to 100M and it failed, here is the yunopaste : https://paste.yunohost.org/raw/avaxisujeg but it has indeed changed the value in the homserver.yaml

Finally, I tried to change "Server statistics" from off to on and this worked and it has indeed changed the value in the homserver.yaml

Thatoo commented 10 months ago

Enable Registration for new users : worked to change it from false to true but not from true to false... "Le formulaire contient des erreurs"

Disable asking Phone Number in Registration flow worked to change it from true to false but not the opposite : "Le formulaire contient des erreurs"

Auto-Create room for Auto Join if not existing? didn't work at all : "Le formulaire contient des erreurs"

Enable email notifications for new users? didn't work at all : "Le formulaire contient des erreurs"

Access Public Rooms Directory over Federation? didn't work at all : "Le formulaire contient des erreurs"

Disable content sharing inside push notification. didn't work at all : "Le formulaire contient des erreurs"

Allow non-server-admin Users to create Spaces? didn't work at all : "Le formulaire contient des erreurs"

Enable sending emails for messages the user missed? didn't work at all : "Le formulaire contient des erreurs"

Josue-T commented 10 months ago

lgtm! @Josue-T can you please merge in testing ? @YunoHost-Apps/matrix-bridges anyone could test upgrade and functionality of the new config panel ?

Maybe we need to investigate the last issues message before to merge it.

MayeulC commented 10 months ago

There is already a mechanism that backs up the old config and warns the user with the diff. If you want to improve this, it should be done in yunohost core, not in this package.

Of course, not necessary for this PR, but as a datapoint, here is what I modify in my config:

The above could easily be added as options in the config panel (possibly in an "advanced" section).

Gredin67 commented 10 months ago

Feel free to add it :)

Le 28 août 2023 10:13:26 GMT+02:00, Mayeul Cantan @.***> a écrit :

There is already a mechanism that backs up the old config and warns the user with the diff. If you want to improve this, it should be done in yunohost core, not in this package.

Of course, not necessary for this PR, but as a datapoint, here is what I modify in my config:

  • enable metrics, and add a localhost listener (that I use with yunohost's prometheus package, to then plot everything in grafana)
  • adjust cache factors (both global_factor and sync_response_cache_duration as I have plenty of RAM)
  • whitelist 127.0.0.1 in ip_range_whitelist, as I run a push server (ntfy for UnifiedPush) at this address.

The above could easily be added as options in the config panel (possibly in an "advanced" section).

-- Reply to this email directly or view it on GitHub: https://github.com/YunoHost-Apps/synapse_ynh/pull/356#issuecomment-1695241509 You are receiving this because you were mentioned.

Message ID: @.***>

rosbeef commented 10 months ago

I'm focusing on my last addition account_threepid_delegates_msisdn and enable_registration for now.

I have some clues :

  1. I don't understand why it is at the top of the setting.yml file (maybe sed config script is not working on the first line,(sort of speculation from my part but i read something somewere about that ))
  2. When i run yunohost app config set synapse__2 main.welcome.account_threepid_delegates_msisdn -v 'https://127.0.0.1:2345' do nothing as it don't find any modifications, which is false.
  3. Run yunohost app config get synapse__2 returns me "account_threepid_delegates_msisdn" values as empty, not "none" as others empty values.

Please someone can test if i'm right. When i put account_threepid_delegates_msisdn at the middle of the setting.yml , setting enable_registration from true to false works.

rosbeef commented 10 months ago

Fix "false" value to "none" in select field It seems the bug was related to the value of the requires_registration_3pid . The requires_registration_3pid default value setted to false in the install script and was interpreted as a boolean. So getting the value to show in the config panel generate a bug as 0 is not a valid value to choose in the select type field. To mitigate i change the default value to none in Install, upgrade scripts and config-panel.toml

please proceed with tests i will sleep ;)

Josue-T commented 10 months ago

Hello,

@rosbeef @Gredin67 did you test the last version ? Does everything works ?

Gredin67 commented 10 months ago

Please merge in testing to increase visibility and get more testers. config panel cannot break anything but the config file. @Thatoo should be able to test again soon.

Le 12 septembre 2023 18:32:13 GMT+02:00, Josue-T @.***> a écrit :

Hello,

@rosbeef @Gredin67 did you test the last version ? Does everything works ?

-- Reply to this email directly or view it on GitHub: https://github.com/YunoHost-Apps/synapse_ynh/pull/356#issuecomment-1716058817 You are receiving this because you were mentioned.

Message ID: @.***>

Thatoo commented 9 months ago
rosbeef commented 9 months ago

Sorry i have no time till monday. But this error Come from the $domain variable which does not exist.

rosbeef commented 9 months ago
  • Enable registration option works but it automatically set
registrations_require_3pid:
 - email
 # - msisdn

For this maybe those values are the default in config file and should be set to all commented.

I vote for removing the enable registration switch in the install panel too. This should simplify install for non techies and simplify scripts

Thatoo commented 9 months ago

I just realize that it is quiet urgent as testing has been merged in master now...

rosbeef commented 9 months ago

Dmne is it possible to suspend upgrade?

rosbeef commented 9 months ago

Sorry, just to be informed, how is the "workflow" to merge testing into master generally in yunohost apps?? I will try to do something Saturday, but till this I'm only with my mobile and it's not really usable.

Gredin67 commented 9 months ago

Why would you do this? In the worst case the config panel is not working (as before). And people only have to change settings manually in the config file. Unfortunately i'm out for 2 weeks. So I hope one of you can find time to fix those 2 small settings migration issues.

Le 21 septembre 2023 21:53:14 GMT+02:00, rosbeef andino @.***> a écrit :

Dmne is it possible to suspend upgrade?

-- Reply to this email directly or view it on GitHub: https://github.com/YunoHost-Apps/synapse_ynh/pull/356#issuecomment-1730205889 You are receiving this because you were mentioned.

Message ID: @.***>