Bug 8268 - Koha should offer way to backup entire db
Summary: Koha should offer way to backup entire db
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: 3.10
Hardware: All All
: P2 enhancement (vote)
Assignee: Jared Camins-Esakov
QA Contact: Paul Poulain
URL:
Keywords:
: 8467 (view as bug list)
Depends on: 8437 8202 8467
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-18 18:20 UTC by Jared Camins-Esakov
Modified: 2013-12-05 20:01 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8268: Add database dump to export tool (20.15 KB, patch)
2012-06-18 21:44 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 8268: Add database dump to export tool (20.88 KB, patch)
2012-06-25 11:10 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 8268 follow-up: incorporate QA comments (4.42 KB, patch)
2012-07-07 12:57 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 8268 follow-up: incorporate QA comments (4.32 KB, patch)
2012-07-10 14:44 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 8268 follow-up: incorporate QA comments (4.33 KB, patch)
2012-07-10 15:07 UTC, Robin Sheat
Details | Diff | Splinter Review
Followup for fixing install (703 bytes, patch)
2012-07-16 11:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8268 follow-up: rewrite config correctly (873 bytes, patch)
2012-07-28 01:47 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Signed followup (947 bytes, patch)
2012-07-30 08:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8268: improve error checking (1.10 KB, patch)
2012-07-30 18:11 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 8268: improve error checking (1.12 KB, patch)
2012-08-01 14:30 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jared Camins-Esakov 2012-06-18 18:20:32 UTC
Right now, if libraries with hosted installations want copies of the database for backup purposes, they have to request that their vendors provide them with a copy. It would be helpful if it were possible for vendors to easily enable their customers to easily download backups of the database via the Export tool.
Comment 1 Jared Camins-Esakov 2012-06-18 21:44:51 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2012-06-18 21:45:34 UTC
Note that the patch will not apply without bug 8202.
Comment 3 Marc Balmer 2012-06-19 09:18:12 UTC
This is incomplete.  There should be a way to create a backup in a database independent way.  E.g., in the backup.sh script use a switch statement to call whatever dumper is needed.
Comment 4 Marc Balmer 2012-06-19 09:35:35 UTC
(Mit Bezug zu comment 3)
> This is incomplete.  There should be a way to create a backup in a database
> independent way.  E.g., in the backup.sh script use a switch statement to
> call whatever dumper is needed.

My comment does not apply to the patch, but to the script backup.sh (which has more problems...), so please ignore my comment for this particular patch.
Comment 5 Chris Cormack 2012-06-24 08:05:12 UTC
Applying: Bug 8268: Add database dump to export tool
fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/tools/export.tt).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 8268: Add database dump to export tool
Comment 6 Jared Camins-Esakov 2012-06-24 11:31:25 UTC
(In reply to comment #5)
> Applying: Bug 8268: Add database dump to export tool
> fatal: sha1 information is lacking or useless
> (koha-tmpl/intranet-tmpl/prog/en/modules/tools/export.tt).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 Bug 8268: Add database dump to export tool

Did you apply this on top of the patch for bug 8202?
Comment 7 Robin Sheat 2012-06-25 10:16:09 UTC
It applies for me:

robin@momomoto:~/catalyst/koha$ git bz apply 8202
Bug 8202 - Add authorities to export tool

[SIGNED-OFF] Bug 8202: Add authority export to tools/export.pl
Apply? [yn] y

Applying: Bug 8202: Add authority export to tools/export.pl
robin@momomoto:~/catalyst/koha$ git bz apply 8268
Bug 8268 - Koha should offer way to backup entire db

Bug 8268: Add database dump to export tool
Apply? [yn] y

Applying: Bug 8268: Add database dump to export tool
/home/robin/catalyst/koha/.git/rebase-apply/patch:117: trailing whitespace.
    ls "$backupdir/" | 
/home/robin/catalyst/koha/.git/rebase-apply/patch:307: trailing whitespace.
    print $query->header(   -type => $mimetype, 
warning: 2 lines add whitespace errors.
robin@momomoto:~/catalyst/koha$
Comment 8 Robin Sheat 2012-06-25 11:10:09 UTC Comment hidden (obsolete)
Comment 9 Robin Sheat 2012-06-25 11:10:49 UTC
Things noted:

10-1. If you forgot to run the backup command, then there is nothing to choose from but the download button is still there. Not a big issue, just slightly cosmetic. Also, the "no downloads exist" message is missing, I think due to different semantics between T::T and perl about whether an empty array is considered "true."

10-2. This doesn't work with the packages, as it's testing -o (do I own this file) not -r (can I read this file.) Dumps provided by the packages are owned by root, but readable by the webserver due to group permissions. This needs to be fixed in getbackupfilelist and download_backup.

My signed off patch fixes these.
Comment 10 Paul Poulain 2012-07-06 13:52:29 UTC
Comment on attachment 10358 [details] [review]
Bug 8268: Add database dump to export tool

patch obsoleted by signed-off patch
Comment 11 Paul Poulain 2012-07-06 14:46:38 UTC
QA comment:

You add 2 subs: getbackupfilelist and download_backup => bad point, we usually use capitals (GetBackupFileList and DownloadBackup). However, this is not in a package, so I won't fail QA for that.

Other than this, I can't find anything wrong in your code, the security triple check (ENV and unix permission, and superlibrarian) is perfect.

I'm adding semarie, that is a boss at finding security holes. (Cher frère, si tu trouves un problème, ne te prive pas !)

Marking passed QA, I'll delay pushing, I'll wait for some feedback from semarie, just in case
Comment 12 Sébastien Marie 2012-07-07 12:10:51 UTC
(En réponse au commentaire 11)
>
> Marking passed QA, I'll delay pushing, I'll wait for some feedback from
> semarie, just in case

The code seems globally ok: just 4 remarks.

Some security remarks (in tools/export.pl):
 1. a problem (common in koha): use of user-input in warn. An attacker can forge entries in log file (see CWE-93) (resume: if variable contains CRLF, the log file too... and the next line is forged: so who write in log ? koha or attacker ?)

> warn "A suspicious attempt was made to download the db at '$filename' by ..."


 2. about the regex for $filename verification
> return unless ( ... && not $filename =~ m#(^\.\.|/)# );

The regex could be translated as: $filename should not:
 - start with '..'
OR
 - contains '/'

I don't understand the purpose of "^..", if '/' is forbidden; as you can't escape from directory without '/'.

So I think m#/# is suffisent (please correct me if not).



Some generals remarks:
 3. the directory for backup is not consitent in differents files:

in debian/templates/koha-conf-site.xml.in, the directory use for backupdir is:
> <backupdir>/var/lib/koha/__KOHASITE__</backupdir>

in Makefile.PL, the directory seems to be in /var/spool :
> './skel/var/spool/koha'       => { target => 'BACKUP_DIR', trimdir => -1 },

in debian/scripts/koha-dump:
> [ -z "$backupdir" ] && backupdir="/var/spool/koha/$name"


 4. in misc/cronjobs/backup.sh

The lasts lines are for mail admin if ok, or mail admin if not ok.
> [ -f $KOHA_BACKUP] && echo "$KOHA_BACKUP was successfully created." | mail kohaadmin -s $KOHA_BACKUP ||
> echo "$KOHA_BACKUP was NOT successfully created." | mail kohaadmin -s $KOHA_BACKUP

First, there are *two* lines. I haven't test it, but it should be an error as "cmd_a && cmd_b ||" is not a valid shell expression. Should be one-line "cmd_a && cmd_b || cmd_c"

Secondly, I'm not sure that the behaviour is correct.
"cmd_a && cmd_b || cmd_c" could be expanded as:

if cmd_a ; then
  if ! cmd_b ; then
    cmd_c
  fi
else
  cmd_c
fi

so if cmd_b failed (echo "backup ok" | mail admin), the cmd_c (echo "backup err" | mail admin) will be run (and expected to failed too, as the first try).

Proposed correction:
> if [ -f $KOHA_BACKUP] ; then
> echo "$KOHA_BACKUP was successfully created." | mail kohaadmin -s $KOHA_BACKUP
> else
> echo "$KOHA_BACKUP was NOT successfully created." | mail kohaadmin -s $KOHA_BACKUP
> fi
Comment 13 Sébastien Marie 2012-07-07 12:39:08 UTC
Just another error (and that I kept in my "proposed correction", so please correct also my correction :-) ), in 4. "misc/cronjobs/backup.sh"

[ -f $KOHA_BACKUP] is not correct, but [ -f $KOHA_BACKUP ] is.

-sh: [: missing `]'
Comment 14 Jared Camins-Esakov 2012-07-07 12:57:43 UTC Comment hidden (obsolete)
Comment 15 Jared Camins-Esakov 2012-07-07 13:01:20 UTC
(In reply to comment #12)
> (En réponse au commentaire 11)
> >
> > Marking passed QA, I'll delay pushing, I'll wait for some feedback from
> > semarie, just in case
> 
> The code seems globally ok: just 4 remarks.
> 
> Some security remarks (in tools/export.pl):
>  1. a problem (common in koha): use of user-input in warn. An attacker can
> forge entries in log file (see CWE-93) (resume: if variable contains CRLF,
> the log file too... and the next line is forged: so who write in log ? koha
> or attacker ?)
> 
> > warn "A suspicious attempt was made to download the db at '$filename' by ..."
> 
> 
>  2. about the regex for $filename verification
> > return unless ( ... && not $filename =~ m#(^\.\.|/)# );
> 
> The regex could be translated as: $filename should not:
>  - start with '..'
> OR
>  - contains '/'
> 
> I don't understand the purpose of "^..", if '/' is forbidden; as you can't
> escape from directory without '/'.
> 
> So I think m#/# is suffisent (please correct me if not).
> 
> 
> 
> Some generals remarks:
>  3. the directory for backup is not consitent in differents files:
> 
> in debian/templates/koha-conf-site.xml.in, the directory use for backupdir
> is:
> > <backupdir>/var/lib/koha/__KOHASITE__</backupdir>
> 
> in Makefile.PL, the directory seems to be in /var/spool :
> > './skel/var/spool/koha'       => { target => 'BACKUP_DIR', trimdir => -1 },
> 
> in debian/scripts/koha-dump:
> > [ -z "$backupdir" ] && backupdir="/var/spool/koha/$name"
> 
> 
>  4. in misc/cronjobs/backup.sh
> 
> The lasts lines are for mail admin if ok, or mail admin if not ok.
> > [ -f $KOHA_BACKUP] && echo "$KOHA_BACKUP was successfully created." | mail kohaadmin -s $KOHA_BACKUP ||
> > echo "$KOHA_BACKUP was NOT successfully created." | mail kohaadmin -s $KOHA_BACKUP
> 
> First, there are *two* lines. I haven't test it, but it should be an error
> as "cmd_a && cmd_b ||" is not a valid shell expression. Should be one-line
> "cmd_a && cmd_b || cmd_c"
> 
> Secondly, I'm not sure that the behaviour is correct.
> "cmd_a && cmd_b || cmd_c" could be expanded as:
> 
> if cmd_a ; then
>   if ! cmd_b ; then
>     cmd_c
>   fi
> else
>   cmd_c
> fi
> 
> so if cmd_b failed (echo "backup ok" | mail admin), the cmd_c (echo "backup
> err" | mail admin) will be run (and expected to failed too, as the first
> try).
> 
> Proposed correction:
> > if [ -f $KOHA_BACKUP] ; then
> > echo "$KOHA_BACKUP was successfully created." | mail kohaadmin -s $KOHA_BACKUP
> > else
> > echo "$KOHA_BACKUP was NOT successfully created." | mail kohaadmin -s $KOHA_BACKUP
> > fi

Thank you for catching those issues. I corrected them all with the follow-up.

(In reply to comment #11)
> QA comment:
> 
> You add 2 subs: getbackupfilelist and download_backup => bad point, we
> usually use capitals (GetBackupFileList and DownloadBackup). However, this
> is not in a package, so I won't fail QA for that.

Our standard is to use lowercase in .pl files and uppercase in packages, I think. I suppose that makes sense, since it ensures that there will be a visual distinction between local functions and core functions.
Comment 16 Paul Poulain 2012-07-10 09:55:10 UTC
Since the follow-up changes sensible things like:
+ <backupdir>/var/spool/koha/__KOHASITE__</backupdir>

I think it's worth a signoff before pushing. Robin, could you please signoff the follow-up ?
Comment 17 Robin Sheat 2012-07-10 13:43:03 UTC
Jared, something you did causes this in your patch:

Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="UTF-8"

which causes git to say:

fatal: cannot convert from UTF-8UTF-8 to UTF-8

I should be able to work around it easily, but just so you know you did something weird :)
Comment 18 Robin Sheat 2012-07-10 13:53:31 UTC
OK, I think there are more issues:

$ git am /tmp/Bug-8268-follow-up-incorporate-QA-comments-l9YCje.patch
Applying: Bug 8268 follow-up: incorporate QA comments
error: patch failed: tools/export.pl:340
error: tools/export.pl: patch does not apply
Patch failed at 0001 Bug 8268 follow-up: incorporate QA comments

Jared, I expect you can remake this easily, can you do that and I'll have a look and sign it off (looking over it, it seems fine: that said, I didn't spot the path issue that Frère Sébastien caught, so take that for what it's worth :)
Comment 19 Jared Camins-Esakov 2012-07-10 14:44:14 UTC Comment hidden (obsolete)
Comment 20 Robin Sheat 2012-07-10 15:07:34 UTC Comment hidden (obsolete)
Comment 21 Paul Poulain 2012-07-12 15:40:07 UTC
QA comments: Frere Sebastien Marie suggestions/concern taken in account, passed QA

About
> > You add 2 subs: getbackupfilelist and download_backup => bad point, we
> > usually use capitals (GetBackupFileList and DownloadBackup). However, this
> > is not in a package, so I won't fail QA for that.
> 
> Our standard is to use lowercase in .pl files and uppercase in packages, I
> think. I suppose that makes sense, since it ensures that there will be a
> visual distinction between local functions and core functions.

You're right, sorry for the annoyance.
Comment 22 Paul Poulain 2012-07-12 15:42:02 UTC
Patch pushed
Comment 23 Paul Poulain 2012-07-13 08:16:15 UTC
I've found a problem with this patch, that you can see when you run plack:
Variable "$backupdir" is not available at /home/paul/koha.dev/koha-community/tools/export.pl line 334.

=> the backup features won't work under plack (problem of variable scoping with Plack that put the script in a sub, making main() variable unaccessible in other subs. Problem solved everywhere with a our instead of my. The best option, that I would prefer -from far- for this new feature, being that we don't use such a our variable, but define backupdir where it's needed (+ it's just reading of a syspref, so not something costly)
Comment 24 Marcel de Rooy 2012-07-16 07:52:08 UTC
Suspecting this recently pushed patch from following:

Koha installation crashes on not existent directory: var/spool.
This was a dev install.

chmod: cannot access `/usr/share/kohadev/newclone/var/spool': No such file or directory
make: *** [set_koha_permissions] Error 1
Comment 25 Marcel de Rooy 2012-07-16 08:43:16 UTC
Just for the record (and future cases..):
I see changes to Makefile.PL (among others, incl koha-conf.xml) but the test plan does not [even] include testing the various installations. Although the changes are small, it is better to do.
Comment 26 Marcel de Rooy 2012-07-16 11:21:30 UTC Comment hidden (obsolete)
Comment 27 Jared Camins-Esakov 2012-07-17 20:04:48 UTC
This follow-up is required for new, non-package installations to work.
Comment 28 Marcel de Rooy 2012-07-18 14:42:42 UTC
*** Bug 8467 has been marked as a duplicate of this bug. ***
Comment 29 Paul Poulain 2012-07-23 08:40:40 UTC
Trivial and neede follow-up, passed QA
Comment 30 Marcel de Rooy 2012-07-25 14:40:19 UTC
Jared,
In a manual dev install I do still come across this line in koha-conf.xml:
<backupdir>__BACKUP_DIR__</backupdir>

Note that install finished apparently correct..
Comment 31 Jared Camins-Esakov 2012-07-25 14:45:53 UTC
(In reply to comment #30)
> Jared,
> In a manual dev install I do still come across this line in koha-conf.xml:
> <backupdir>__BACKUP_DIR__</backupdir>
> 
> Note that install finished apparently correct..

I will look into this later today.
Comment 32 Jared Camins-Esakov 2012-07-28 01:47:12 UTC Comment hidden (obsolete)
Comment 33 Marcel de Rooy 2012-07-30 08:36:36 UTC
Created attachment 11187 [details] [review]
Signed followup

Problem fixed.
Signed off and passed QA
Comment 34 Jared Camins-Esakov 2012-07-30 18:11:57 UTC Comment hidden (obsolete)
Comment 35 Jared Camins-Esakov 2012-07-30 18:12:50 UTC
The first patch has been signed off and passed QA, but the second has not.
Comment 36 Jonathan Druart 2012-08-01 14:30:30 UTC
Created attachment 11262 [details] [review]
Bug 8268: improve error checking

Valid download attempts were being denied thanks to an incorrect
regular expression. This patch fixes that, and makes it easier to
understand what's going on in the code.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 37 Mason James 2012-08-10 04:08:16 UTC
both (2) patches signed-off. 
looks good, passing QA on this


mason@xen1:~/git/head$ koha-qa.pl -c 2
        * 8374da8 Bug 8268: improve error checking
                tools/export.pl
        * 5a8dcc7 Bug 8268 follow-up: rewrite config correctly
                rewrite-config.PL

- perlcritic-progressive tests... OK
- perl -c syntax tests... OK
- xt/tt_valid.t tests... OK
- xt/author/valid-template.t tests... OK
Comment 38 Paul Poulain 2012-08-28 16:04:33 UTC
follow-ups http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11187 and http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11262 pushed today 

(status switched back to "ENH", as this bug describe an enhancement
Comment 39 Jared Camins-Esakov 2012-12-03 13:25:11 UTC
This feature is now available, though it doesn't play so nicely with Plack (see bug 8437).