Bug 6094 - Fixing ModAuthority problems
Summary: Fixing ModAuthority problems
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: master
Hardware: All All
: PATCH-Sent (DO NOT USE) normal (vote)
Target Milestone: ---
Assignee: Marcel de Rooy
QA Contact: Bugs List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 5572
  Show dependency treegraph
 
Reported: 2011-04-06 14:50 UTC by Marcel de Rooy
Modified: 2012-10-25 23:10 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Patch (4.74 KB, text/plain)
2011-04-07 08:23 UTC, Marcel de Rooy
Details
Revised patch (4.75 KB, patch)
2011-07-25 07:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Db revision patch (2.11 KB, patch)
2011-07-25 11:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Patch (5.45 KB, patch)
2011-07-25 11:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Patch (functionality) (5.08 KB, patch)
2011-07-25 13:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Patch (functionality) (4.55 KB, patch)
2011-08-04 07:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Signed-off patch (5.17 KB, patch)
2011-10-18 13:36 UTC, Frédéric Demians
Details | Diff | Splinter Review
Rebased db revision patch (2.14 KB, patch)
2011-10-19 12:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Signe (2.27 KB, patch)
2011-10-19 13:12 UTC, Frédéric Demians
Details | Diff | Splinter Review
Signed-off DB revision patch (2.27 KB, patch)
2011-10-19 13:13 UTC, Frédéric Demians
Details | Diff | Splinter Review
Follow up patch needing signoff (2.64 KB, patch)
2011-10-20 08:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review

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