Bug 6094

Summary: Fixing ModAuthority problems
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: MARC Authority data supportAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: normal    
Priority: PATCH-Sent (DO NOT USE) CC: chris, f.demians, januszop, katrin.fischer, koha.sekjal, semarie
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 5572    
Attachments: Patch
Revised patch
Db revision patch
Patch
Patch (functionality)
Patch (functionality)
Signed-off patch
Rebased db revision patch
Signe
Signed-off DB revision patch
Follow up patch needing signoff

Description Marcel de Rooy 2011-04-06 14:50:24 UTC
Pref MergeAuthoritiesOnUpdate does not exist; should be dontmerge. 
In: C4/AuthoritiesMarc.pm

Subfolder tmp/modified_authorities should not be in $cgidir or $intranetdir. The apache user should not be assumed to have write access there.
I would like to move it (back?) to /tmp/modified_authorities.
The subfolder should be created if not existing.
This change applies to AuthoritiesMarc.pm and merge_authority.pl.

Will send a patch.
Comment 1 Marcel de Rooy 2011-04-07 08:23:42 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2011-04-07 10:13:34 UTC
Test this patch with:

A) No auto merging
1 Set dontmerge pref to Do not use..
2 Look for an auth rec and associated bib record; Modify the authority name
3 Check that bib record did not change
4 Now look in /tmp/modified_authorities: Is there a file with the right authid number there?
5 Run misc/migration_tools/merge_authority.pl with -b. Does it update your bib record? Removed it the authid file? (Enough permissions?)

B) Automatic merging
1 Set pref dontmerge to Do use..
2 Look for an auth rec and associated bib record; Modify the authority name
3 Check the bib record; it should have changed!
Comment 3 Chris Cormack 2011-04-07 17:47:58 UTC
Marcel, this feels like a bugfix, not an enhancement to me, do you agree?
Comment 4 Marcel de Rooy 2011-04-08 17:40:05 UTC
(In reply to comment #3)
> Marcel, this feels like a bugfix, not an enhancement to me, do you agree?

I will change the status.
Comment 5 Marcel de Rooy 2011-04-08 17:52:26 UTC
Comments from Janusz (copied from 5572):
> Marcel, because of my other commitments I wouldn't be able to jump into Koha
> coding thing before Monday, though I looked through your changes -- it looks
> fine for me.  The only concern I have -- do you think it is a good idea to put
> authid memo files into general /tmp subdirectory which can be purged beyond
> your control?

Reply from Marcel:
Thanks for responding. You make a point about /tmp. Would you have another solution? I have a problem with the current situation (file permissions and SELinux rights/labels) in the koha clone dir. I think for /tmp this will not be a problem under many distro's.
I guess any other location may need some additional install action.
Comment 6 Marcel de Rooy 2011-04-14 11:15:06 UTC
Janusz: A small additional thougth: If we choose /tmp, we are doing at least the same as for sessionlog. This file is now written also to tmp.
Comment 7 Marcel de Rooy 2011-04-27 10:13:14 UTC
This patch still applies to master (3.05.00.001).
Comment 8 Janusz Kaczmarek 2011-07-24 17:56:13 UTC
Marcel, OK, at last I could find the time to review your patch.  Basically it works as expected, i.e. the value of dontmerge syspref is obeyed and:  

If dontmerge == 1 biblio records associated with the modified authority remain unchanged and a /tmp/XXXX.authid file is created. Then the merge_authority.pl script makes use of it.  Of course one have to have enough rights for merge_authority.pl to be able to delete these files (they are owned, in a standard case, by the user under which apache is running, www-data in my case).  

If dontmerge == 0 authotiry controlled fields are smoothly updated in biblio records.

BUT I still strongly discourage you from using /tmp as a store place for XXXX.auth.  It really gets cleaned upon reboot in standard Debian installation (and in others too, I guess).  What if for some reason the system has to be rebooted, for example after a kernel update?  The two databases--biblio and authorities--become inconsistent than (since you loose, for ever, the information about necessary updates) and there will be no other way to make them consistent again than rewrite every single controlled field in every singe biblio record according to the value of the corresponding authority heading.  We should not do it to the unconscious people :)  

I would love to sign off the patch but I think it is really dangerous now.

How about /var/tmp ? -- it is world writeable and in Debian it is recommended for files that should not be removed upon booting.  Wouldn't it be appropriate for you?


PS  As to the session log -- who cares about it when the system gets rebooted...
Comment 9 Marcel de Rooy 2011-07-25 07:05:23 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2011-07-25 07:07:19 UTC
(In reply to comment #8)
> How about /var/tmp ? 
Good idea. I submitted a revised patch. Could you do the signoff please? Thanks.
Comment 11 Sébastien Marie 2011-07-25 07:27:20 UTC
(In reply to comment #10)
> (In reply to comment #8)
> > How about /var/tmp ? 
> Good idea. I submitted a revised patch. Could you do the signoff please?
> Thanks.

This patch present security issue.

The "/var/tmp/modified_authorities" directory is create which unix mod to 777: so *every* user on the system could create (or remove) file in this directory.

It permit also *any* user to arbitrary override a file owner by the webserver owner (generally www-data), using symlink-attack (see CWE-61, http://cwe.mitre.org/data/definitions/61.html)

Please correct it.

The directory should be created by installer (with root permission) to have owner set to: koha.www-data , and permissions to 2770. This permit the apache daemon to write in this directory (and only him), and permit koha user (the user which should run the crontab for update) to read and delete these files.

I think a common directory for koha should be fine to be created (should be discuted on the ML) which good owner/permissions, and subdirectories for particular tasks (like this one).
Comment 12 Marcel de Rooy 2011-07-25 07:47:48 UTC
Maybe it would be better [easier] to just create temporary records in some table for this purpose instead of temporary files with all related permission and security issues around it?
Comment 13 Marcel de Rooy 2011-07-25 07:57:22 UTC
Comment on attachment 4733 [details] [review]
Revised patch

Will send new proposal.
Comment 14 Marcel de Rooy 2011-07-25 10:39:02 UTC
Found a related error while testing:
{_conn} undefined: has this Connection been destroy()ed? at /usr/local/lib64/perl5/site_perl/5.10.0/x86_64-linux-thread-multi/ZOOM.pm line 343.

The destroy of the connection object in AuthoritiesMarc.pm, sub merge makes that a second subsequent call of merge crashes on creating the connection with C4::Context->Zconn("biblioserver",0). Especially when dontmerge is "Do not", a batch call from merge-authority.pl will do that of course.

Should ideally be solved in the Zconn routine of Context.pm; at this time disabling the destroy call in merge() resolves the situation. (Another bug should be opened for Zconn!)
Comment 15 Marcel de Rooy 2011-07-25 11:14:40 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2011-07-25 11:16:53 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2011-07-25 11:37:31 UTC
UPDATED Test plan:

A) No auto merging
1 Set dontmerge pref to Don't automatically update..
2a Look for an auth rec and associated bib record; Modify the authority name
2b Repeat 2a for a second auth record
3 Check that bib records did not change
4 Now look in mysql table need_merge_authorities: Records with authid and done=0?
5 Run misc/migration_tools/merge_authority.pl with -b. Does it update your bib
records? Check if done=1 in the table for both auth records.

B) Automatic merging
1 Set pref dontmerge to Do automatically update..
2 Look for an auth rec and associated bib record; Modify the authority name
3 Check the bib record; it should have changed!
Comment 18 Janusz Kaczmarek 2011-07-25 11:43:38 UTC
(In reply to comment #14)
> Found a related error while testing:
> {_conn} undefined: has this Connection been destroy()ed? at
> /usr/local/lib64/perl5/site_perl/5.10.0/x86_64-linux-thread-multi/ZOOM.pm line
> 343.
> 
> The destroy of the connection object in AuthoritiesMarc.pm, sub merge makes
> that a second subsequent call of merge crashes on creating the connection with
> C4::Context->Zconn("biblioserver",0). Especially when dontmerge is "Do not", a
> batch call from merge-authority.pl will do that of course.
> 
> Should ideally be solved in the Zconn routine of Context.pm; at this time
> disabling the destroy call in merge() resolves the situation. (Another bug
> should be opened for Zconn!)

I confirm this -- in my instalations $oConnection->destroy(); line in merge is commented out since a very long time.  Forgot to report this issue... :(

Thank you!
Comment 19 Janusz Kaczmarek 2011-07-25 11:58:06 UTC
(In reply to comment #17)
> UPDATED Test plan:

Marcel,

I will test this soon--looks good and should work.  I definitely like the idea of putting the information into DB instead of into temporary files.  But my concern is--should the need_merge_authorities be cleared periodically by a cronjob or we need this history and it will grow forever? 

If we do not need the history of merging (in fact we have the cataloguing log) then wouldn't it be simpler to add a column like `need_merge` to the `auth_header` table and proceed more or less analogously as you proposed with the column `done`?  What do you think?
Comment 20 Marcel de Rooy 2011-07-25 12:02:22 UTC
(In reply to comment #19)
> (In reply to comment #17)

When this patches reaches master, we can add a few lines to misc/cronjobs/cleanup_database.pl for periodically cleaning up. This script already purges records from zebraqueue and others.
Comment 21 Janusz Kaczmarek 2011-07-25 12:40:00 UTC
NB line # 47 from merge_authority.pl should be removed now--it refers to the previous approach.
Comment 22 Marcel de Rooy 2011-07-25 13:27:59 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2011-08-04 06:59:07 UTC
Bug 5737 has been pushed and affected this patch.
Does not apply now; will resubmit new version.
Comment 24 Marcel de Rooy 2011-08-04 07:22:01 UTC Comment hidden (obsolete)
Comment 25 Ian Walls 2011-08-05 12:15:56 UTC
This patch looks very promising.   I'm confident once it's been tested and signed off, it will be ready to pass QA.
Comment 26 Frédéric Demians 2011-10-18 13:36:56 UTC
Created attachment 5951 [details] [review]
Signed-off patch

Thanks Marcel. It works as advertised. Both modes are functionnal
(back):

- Immediate with dontmerge=0: After modifying an authority record, its
  linked biblios are immediately modified. This isn't the case in 3.4.5.
- Delayed with dontmerge=1: After modifying an authority record, its
  linked biblios are not modified. But an entry is added to
  need_merge_authority new table and 'merge_authorities.pl -b' script
  updates biblios.

Comment: need_merge_authority, like zebraqueue, should be cleared
from time to time.
Comment 27 Marcel de Rooy 2011-10-19 12:10:10 UTC Comment hidden (obsolete)
Comment 28 Marcel de Rooy 2011-10-19 12:15:07 UTC
(In reply to comment #26)
> Signed-off patch
Thanks, Frederic for test and signoff. Could you please also put a signoff on the second patch that installs the new table? Probably, QA would like to see that too. I rebased it for current master now.

> Comment: need_merge_authority, like zebraqueue, should be cleared
> from time to time.

True. When this patch gets pushed, I will send an adjustment for cleanupdatabase in order to purge records from the new table.
Comment 29 Frédéric Demians 2011-10-19 13:12:29 UTC Comment hidden (obsolete)
Comment 30 Frédéric Demians 2011-10-19 13:13:37 UTC
Created attachment 5979 [details] [review]
Signed-off DB revision patch
Comment 31 Ian Walls 2011-10-19 17:16:56 UTC
Well written, gets rid of /tmp for storage of merge records.  Both functionality and db patches pass QA.

RM, be sure to thoroughly process the DB revision patch; not only is the DB Rev number XXX'ed out, but other testing modifications are in place and will need removing for production use.

Recommended followup patches:

Add removing 'done' merge_authorities rows to cleanup_database.pl, so we don't drown the DB in old records.

Database documentation on new table
Comment 32 Chris Cormack 2011-10-19 22:41:09 UTC
Pushed, but yes we do need those follow up patches
Comment 33 Marcel de Rooy 2011-10-20 07:35:02 UTC
(In reply to comment #32)
> Pushed, but yes we do need those follow up patches

Patch for documentation sent under the flag of #6716.
Patch for cleanup forthcoming.
Comment 34 Marcel de Rooy 2011-10-20 08:15:40 UTC
Created attachment 6005 [details] [review]
Follow up patch needing signoff

Follow up for periodically cleaning temp table need_merge_authorities.
Comment 35 Ian Walls 2011-10-20 13:47:49 UTC
Follow up patch has my signoff, and is Passed QA.  Thanks for the quick work, Marcel!
Comment 36 Chris Cormack 2011-10-20 15:32:15 UTC
Follow up pushed, speedy work! Well done!