search

YunoHost-Apps / mautrix_signal_ynh

Matrix signal package for YunoHost
GNU Affero General Public License v3.0
4 stars 10 forks source link

Restore: Fix permission for config.yaml #104

Closed CodeShakingSheep closed 2 weeks ago

CodeShakingSheep commented 3 weeks ago

Problem

However, afterwards the service stopped again with another error PermissionError: [Errno 13] Permission denied: 'config.yaml'. I could solve this by running chown mautrix_signal:mautrix_signal config.yaml from /opt/yunohost/mautrix_signal. Previously the folder was owned by root:root.

Problem from https://github.com/YunoHost-Apps/mautrix_signal_ynh/issues/101

Solution

Just like in the restore script of the Mautrix Whatsapp bridge, I added a chown command for config.yaml. See chown command in Mautrix Whatsapp bridge restore script: https://github.com/YunoHost-Apps/mautrix_whatsapp_ynh/blob/master/scripts/restore#L47

Previously there was a chown command for the whole $install_dir. However, this chown command is already defined in https://github.com/YunoHost-Apps/mautrix_signal_ynh/blob/master/scripts/restore#L25 making it redundant.

PR Status

Automatic tests

Automatic tests can be triggered on https://ci-apps-dev.yunohost.org/ after creating the PR, by commenting "!testme", "!gogogadgetoci" or "By the power of systemd, I invoke The Great App CI to test this Pull Request!". (N.B. : for this to work you need to be a member of the Yunohost-Apps organization)

CodeShakingSheep commented 3 weeks ago

!testme

yunohost-bot commented 3 weeks ago

:stuck_out_tongue_winking_eye: Test Badge

yunohost-bot commented 3 weeks ago

:books: :bug: Test Badge

nathanael-h commented 2 weeks ago

Hello, I don't understand why this PR should improve something... :thinking: Actually there is on chown on the whole $install_dir/ directory with the recursive option , and you suggest to do it only for the $install_dir/config.yaml file. Why?

CodeShakingSheep commented 2 weeks ago

Hello, I don't understand why this PR should improve something... đŸ¤” Actually there is on chown on the whole $install_dir/ directory with the recursive option , and you suggest to do it only for the $install_dir/config.yaml file. Why?

Hi, atm there are two identical chown commands in mautrix_signal restore script (which doesn't make sense either):

With that there is the problem that config.yaml isn't owned by mautrix_signal user but by root instead. See my comment from here https://github.com/YunoHost-Apps/mautrix_signal_ynh/issues/101 .

However, afterwards the service stopped again with another error PermissionError: [Errno 13] Permission denied: 'config.yaml'. I could solve this by running chown mautrix_signal:mautrix_signal config.yaml from /opt/yunohost/mautrix_signal. Previously the folder was owned by root:root.

As I wrote chowning config.yaml manually fixed the problem. So, apparently the first chown command from the restore script doesn't apply to config.yaml causing a permission problem (even though it's recursive). I don't know why but it's like that.

If you take a look into mautrix_whatsapp restore script there are the following two different chowncommands (one for the whole $install_dir and one explicitly for config.yaml.

Restoring mautrix_whatsapp works flawlessly. So, explicitly chowning config.yaml in the restore script is the way to go (as proof is also given in the mautrix_whatsapp app).

I also tested the fix on my own server. When I used the branch with my fix for restoring mautrix_signal everything worked fine. You can test it yourself to verify it by removing mautrix_signal first and then restoring it (one time with the restore script from master branch and one time with my fix) and you'll see that restoring with master causes the permission issue for config.yaml and restoring with my fix works.

nathanael-h commented 2 weeks ago

Thank you for the detailed explanation. I got it, almost. I do not understand why at some point the config file belongs to root, but the PR fixes the issue, so OK.

CodeShakingSheep commented 2 weeks ago

Thank you for the detailed explanation. I got it, almost. I do not understand why at some point the config file belongs to root, but the PR fixes the issue, so OK.

Thanks for taking your time with this. It's confusing for me too why the config file belongs to root despite the -R option. Anyway, thanks for merging :pray: