Bug 13876 - Permission issues on updating via installer - dev installations
Summary: Permission issues on updating via installer - dev installations
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (web-based installer) (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 14427
  Show dependency treegraph
 
Reported: 2015-03-19 15:52 UTC by Jonathan Druart
Modified: 2016-12-05 21:22 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 13876: Fallback to /tmp if logdir is not writable (1.44 KB, patch)
2015-03-19 15:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13876: Fallback to /tmp if logdir is not writable (1.64 KB, patch)
2015-03-20 08:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13876: Fallback to /tmp if logdir is not writable (1.82 KB, patch)
2015-03-20 15:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues (2.26 KB, patch)
2015-03-20 15:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13876: Fallback to /tmp if logdir is not writable (1.83 KB, patch)
2015-03-22 20:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues (2.28 KB, patch)
2015-03-22 20:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues (2.60 KB, patch)
2015-04-15 12:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues (2.64 KB, patch)
2015-04-16 14:53 UTC, Chris Nighswonger
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13876: Fallback to /tmp if logdir is not writable (1.89 KB, patch)
2015-04-16 14:54 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues (2.67 KB, patch)
2015-04-17 07:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13876: Fallback to /tmp if logdir is not writable (1.94 KB, patch)
2015-04-17 07:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues (2.69 KB, patch)
2015-04-17 07:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13876: Add the from => to versions in the filenames (1.49 KB, patch)
2015-04-17 07:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13876: Add the from => to versions in the filenames (1.59 KB, patch)
2015-04-17 08:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PASSED QA] Bug 13876: Fallback to /tmp if logdir is not writable (2.03 KB, patch)
2015-04-17 10:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13876: [QA Follow-up] Resolve updatedatabase logfile issues (2.76 KB, patch)
2015-04-17 10:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13876: Add the from => to versions in the filenames (1.62 KB, patch)
2015-04-17 10:28 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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