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.
Created attachment 3767 [details] Patch
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!
Marcel, this feels like a bugfix, not an enhancement to me, do you agree?
(In reply to comment #3) > Marcel, this feels like a bugfix, not an enhancement to me, do you agree? I will change the status.
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.
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.
This patch still applies to master (3.05.00.001).
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...
Created attachment 4733 [details] [review] Revised patch
(In reply to comment #8) > How about /var/tmp ? Good idea. I submitted a revised patch. Could you do the signoff please? Thanks.
(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).
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 on attachment 4733 [details] [review] Revised patch Will send new proposal.
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!)
Created attachment 4737 [details] [review] Db revision patch
Created attachment 4738 [details] [review] Patch Contains functionality.
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!
(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!
(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?
(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.
NB line # 47 from merge_authority.pl should be removed now--it refers to the previous approach.
Created attachment 4739 [details] [review] Patch (functionality) Small adjustment in comments, thanks to Janusz.
Bug 5737 has been pushed and affected this patch. Does not apply now; will resubmit new version.
Created attachment 4833 [details] [review] Patch (functionality) Rebased
This patch looks very promising. I'm confident once it's been tested and signed off, it will be ready to pass QA.
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.
Created attachment 5975 [details] [review] Rebased db revision patch Rebased after functionality patch signoff
(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.
Created attachment 5977 [details] [review] Signe
Created attachment 5979 [details] [review] Signed-off DB revision patch
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
Pushed, but yes we do need those follow up patches
(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.
Created attachment 6005 [details] [review] Follow up patch needing signoff Follow up for periodically cleaning temp table need_merge_authorities.
Follow up patch has my signoff, and is Passed QA. Thanks for the quick work, Marcel!
Follow up pushed, speedy work! Well done!