YunoHost-Apps / shaarli_ynh

Shaarli package for YunoHost
GNU General Public License v3.0
20 stars 13 forks source link

"/var/www/shaarli/data/log.txt" is actualy a directoy for unknow reason :| #45

Closed Psycojoker closed 4 years ago

Psycojoker commented 5 years ago

Hello,

I'm helping someone debug a broken yunohost apps upgrade and for a totally weird and unknown reason the "file" "/var/www/shaarli/data/log.txt" turned out the actually be a ... directory :|

The problem is that this breaks fail2ban which wants to parse this file and complains because it is a directory :x

Also "/var/www/shaarli/data/log.txt" is owned by root for some reason? The other files are own by "shaarli".

After a quick look in the code I really have no idea on how it has append but here is the full upgrade logs https://paste.yunohost.org/raw/dapixodeni

lapineige commented 5 years ago

Oh, that might be the reason why Shaarli install is broken, cause fail2ban doesn't work… and I did not figured out why.

Also "/var/www/shaarli/data/log.txt" is owned by root for some reason? The other files are own by "shaarli".

I didn't even know that…

Psycojoker commented 5 years ago

Yeah, had exactly the same problem, you need to do journalctl -u fail2ban and explore /var/log/fail2ban.log to analyse it and guess the errors :/

lapineige commented 5 years ago

If it's still possible, could you try to upgrade the app again ?

I just merged some changes #44 , I don't know if it changes anything but I'd appreciate a feedback. (upgrading from last version worked for me)

alexAubin commented 5 years ago

Pinpointed the issue : https://github.com/YunoHost/issues/issues/1390 (pretty technical)

This is a bug in ynh_use_logrotate

lapineige commented 5 years ago

How can I solve this to all existing instances with this bug ?

alexAubin commented 5 years ago

I guess we can add something in the upgrade script like :

# Test if log.txt is a directory
if [[ -d "$final_path/data/log.txt" ]]
then
   # Then delete it
   ynh_secure_remove "$final_path/data/log.txt"
   # And run touch to make sure it's a regular file
   touch "$final_path/data/log.txt"
fi

(not tested, not 100% about all the syntax and namings)

Note though that it might still trigger another issue, being that ynh_use_logrotate will try to mkdir a file that already exists ? (idk)

Anyway, gotta also fix the ynh_use_logrotate one way or another (you can imagine shipping your own version of the helper in the meantime) but that's what I would imagine for the upgrad e script

lapineige commented 5 years ago

Note though that it might still trigger another issue, being that ynh_use_logrotate will try to mkdir a file that already exists ? (idk)

I'm asking that for later use, when it's fixed.

JOduMonT commented 5 years ago

Hi;

I really like shaarli and yunohost. It is possible of what I'm saying make no sense because I learning how to contribute luckily for me or sadly for you I choose this application to start.


for the path and the log.txt this is define by shaarli itself Shaarli configJson.json.php so unless we convince them to change this, yunohost have not really any power on this.

since this file /data/log.txt is create by the apps shaarli would be not better to simply let's the apps create this file instead or doing a touch for it ?

lapineige commented 5 years ago

Well, this would solve the problem right now. But still, I believe Yunohost should be able to adapt itself to the different apps (we can't always change the name of the file).

Right now it doesn't seems that simple: https://github.com/YunoHost/issues/issues/1390 I'll add a temporary fix.

lapineige commented 5 years ago

OK, can someone with this fail2ban issue while upgrading can test this branch ? yunohost app upgrade shaarli -u https://github.com/YunoHost-Apps/shaarli_ynh/tree/testing

I can't do it myself right know, but I believe this symlink should solve the issue...

alexAubin commented 4 years ago

Fix proposed in https://github.com/YunoHost/yunohost/pull/810 and https://github.com/YunoHost-Apps/shaarli_ynh/pull/47 ...