Bug 13876

Summary: Permission issues on updating via installer - dev installations
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Installation and upgrade (web-based installer)Assignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: chris, gmcharlt, katrin.fischer, m.de.rooy, mtompset, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9006
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14427
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 14427    
Attachments: Bug 13876: Fallback to /tmp if logdir is not writable
Bug 13876: Fallback to /tmp if logdir is not writable
Bug 13876: Fallback to /tmp if logdir is not writable
Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues
[SIGNED OFF] Bug 13876: Fallback to /tmp if logdir is not writable
[SIGNED OFF] Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues
Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues
[SIGNED OFF] Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues
[SIGNED-OFF] Bug 13876: Fallback to /tmp if logdir is not writable
Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues
Bug 13876: Fallback to /tmp if logdir is not writable
Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues
Bug 13876: Add the from => to versions in the filenames
Bug 13876: Add the from => to versions in the filenames
[PASSED QA] Bug 13876: Fallback to /tmp if logdir is not writable
[PASSED QA] Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues
[PASSED QA] Bug 13876: Add the from => to versions in the filenames

Description Jonathan Druart 2015-03-19 15:52:59 UTC
On dev installations, bug 13793 introduced a regression.
If the $logdir directory is not writable, the installer script explodes.
Comment 1 Jonathan Druart 2015-03-19 15:58:34 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2015-03-19 17:56:00 UTC
Maybe use File::Temp instead of hardcoding /tmp ?
Comment 3 Marcel de Rooy 2015-03-19 17:58:13 UTC
BTW Note that if I would have permissions to write in the original log dir, but SELinux would not allow it, your test here would not work.
Comment 4 Jonathan Druart 2015-03-20 08:14:49 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-03-20 08:15:48 UTC
(In reply to M. de Rooy from comment #2)
> Maybe use File::Temp instead of hardcoding /tmp ?

Done.

(In reply to M. de Rooy from comment #3)
> BTW Note that if I would have permissions to write in the original log dir,
> but SELinux would not allow it, your test here would not work.

I don't know a lot about SELinux, how can you know if a directory is writable?
Comment 6 Marcel de Rooy 2015-03-20 14:56:35 UTC
Will sign off on this one with a follow-up. Please wait :)
Comment 7 Marcel de Rooy 2015-03-20 15:26:51 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2015-03-20 15:26:59 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2015-03-20 15:31:55 UTC
Katrin or Jonathan: Please check. Thanks.
Comment 10 Katrin Fischer 2015-03-22 20:29:32 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2015-03-22 20:29:45 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2015-03-22 20:32:17 UTC
This fixes the problem for me, my database updates are working again!

As the QA follow-up has more lines than the initial patch ;) I'd like to get another QA view from Jonathan or another QA person on this.
Comment 13 Jonathan Druart 2015-03-25 14:09:20 UTC
What's the point of the suffix?
Now the log filename is updatedatabase_2015-03-25T15:06:59_uyj4.log
Your patch does not call the tempdir subroutine, you should modify the use File::Temp line:
   10 use File::Temp qw( tempdir );
Comment 14 Mark Tompsett 2015-04-02 06:17:21 UTC
I'm just curious why not use IPC::Run::run? Is the time stamp on the file of when we ran the updates important? It seems like update log files will just build up forever. Is there a point to their accumulation? What questions are we trying to answer with them? I'm wondering if there is an optimization that is being missed.
Comment 15 Mark Tompsett 2015-04-02 06:21:13 UTC
Never mind. I saw the 13793 bug now.
Comment 16 Marcel de Rooy 2015-04-15 12:41:17 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2015-04-15 12:42:56 UTC
(In reply to Jonathan Druart from comment #13)
> What's the point of the suffix?
> Now the log filename is updatedatabase_2015-03-25T15:06:59_uyj4.log
> Your patch does not call the tempdir subroutine, you should modify the use
> File::Temp line:
>    10 use File::Temp qw( tempdir );

Now do a require File::Temp if needed.
Only use the particular four character suffix of File::Temp in the fallback case.
Could you retest, Jonathan?
Comment 18 Chris Nighswonger 2015-04-16 14:53:52 UTC Comment hidden (obsolete)
Comment 19 Chris Nighswonger 2015-04-16 14:54:55 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2015-04-17 07:11:46 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2015-04-17 07:13:10 UTC
Reordered the patches.
Katrin: did you forget to sign the second one?
Comment 22 Katrin Fischer 2015-04-17 07:17:38 UTC
Hm I probably intended to sign off the second, but wanted to have someone else to have another look at it
Comment 23 Jonathan Druart 2015-04-17 07:52:28 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2015-04-17 07:52:30 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2015-04-17 07:52:33 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2015-04-17 08:13:41 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2015-04-17 10:27:57 UTC
Created attachment 38021 [details] [review]
[PASSED QA] Bug 13876: Fallback to /tmp if logdir is not writable

If the logdir directory is not writable by the koha user, the installer
should not explode.
This patch uses a temporary directory as a fallback.
That means that the history won't be kept.

Test plan:
0/ Generate an update (modify updatedatabase.pl and kohaversion)
1/ Make sure the permission are not correct on $logdir
2/ Go on the interface and execute the update entry.
3/ Confirm that the log has been created in a temporaty directory.

Retry with correct permissions and confirm the log will be created in
$logdir.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Adding a follow-up for completely resolving encountered issues.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Kyle M Hall 2015-04-17 10:28:07 UTC
Created attachment 38022 [details] [review]
[PASSED QA] Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues

This patch removes the test on write permissions of the logdir.
It uses File::Temp to generate a file in the logdir or otherwise in
a system tmp dir.
This resolves issues with both write permissions as well as SELinux
denials.
Note that after generating an empty file, we are appending to it in
the system statement.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: only use File::Temp in the fallback case. Do not add the four
character suffix in the first attempt.

Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Kyle M Hall 2015-04-17 10:28:09 UTC
Created attachment 38023 [details] [review]
[PASSED QA] Bug 13876: Add the from => to versions in the filenames

Additionally to the datetime, it could be useful to have the version
numbers in the log filenames.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Tomás Cohen Arazi 2015-04-20 14:52:00 UTC
Patches pushed to master.

Thanks guys!
Comment 31 Chris Cormack 2015-05-01 08:05:48 UTC
This doesn't apply on 3.18.x please rebase if needed