Bug 10325 - Allow system preferences to be overridable from koha-httpd.conf
Summary: Allow system preferences to be overridable from koha-httpd.conf
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-24 11:09 UTC by Kyle M Hall
Modified: 2014-12-07 20:02 UTC (History)
4 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 10325 - Allow system preferences to be overridable from koha-httpd.conf (2.71 KB, patch)
2013-05-24 11:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10325 - Allow system preferences to be overridable from koha-httpd.conf (7.02 KB, patch)
2013-08-09 14:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10325 - Allow system preferences to be overridable from koha-httpd.conf (7.02 KB, patch)
2013-08-09 14:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10325 - Allow system preferences to be overridable from koha-httpd.conf (7.03 KB, patch)
2013-08-09 14:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10325 - Allow system preferences to be overridable from koha-httpd.conf (7.12 KB, patch)
2013-08-15 08:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10325: QA Followup for lowercase in pref hash and few typos (3.09 KB, patch)
2013-08-15 08:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10325 - QA Followup - Fix typos (1.17 KB, patch)
2013-08-15 17:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10325 - QA Followup - Fix typos (1.17 KB, patch)
2013-08-15 18:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Script to test for badly cases preference calls (987 bytes, application/x-perl)
2013-08-15 18:08 UTC, Kyle M Hall
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2013-05-24 11:09:28 UTC
For Koha installations with multiple OPAC URLs, It would be nice to be
able to override systeprefs from the http conf file. Case in point,
a library wants to have two separate opacs, one the is only viewable
from within the library that allows patrons to place holds, and a second
public one that does not. In this case, overriding the system preference
RequestOnOpac would accomplish this simply, and with no ill affects.
 
This feature would of course be should only be used to override
cosmetic effects on the system, and should not be used for system
preferences such as CircControl, but would be great for preferences
such as OpacStarRatings, opacuserjs, OpacHighlightedWords and many
others!
Comment 1 Kyle M Hall 2013-05-24 11:09:57 UTC Comment hidden (obsolete)
Comment 2 Galen Charlton 2013-05-31 18:26:12 UTC
Comment on attachment 18374 [details] [review]
Bug 10325 - Allow system preferences to be overridable from koha-httpd.conf

Review of attachment 18374 [details] [review]:
-----------------------------------------------------------------

I can see the utility of this, and it would have applications for the command line, too.

I can imagine an edge case, though -- there's nothing stopping one from using this in the *intranet* virtual host definitions (and I could imagine a Koha administrator wanting to do this to allow local admins access to system preferences while locking down ones such as 'marcflavour' that should never be changed).  But that could really confuse somebody who is wondering why their system preference change isn't having the desired effect.

Perhaps the system preference editor should be taught how to recognize overriden sysprefs?

::: C4/Context.pm
@@ +548,5 @@
>  
>      my $dbh  = C4::Context->dbh or return 0;
>  
> +    my $value;
> +    if ( defined $ENV{"OVERRIDE_SYSPREF_$var"} ) {

The test should be 'exists', no?
Comment 3 Galen Charlton 2013-05-31 18:34:24 UTC
Another thing that occurred to me -- some system preference names contain hyphens and colons, neither of which can be used as part of environment variable names.  Not a huge deal, but perhaps the override mechanism should do some normalization of the syspref name before checking the environment.  

For example, it could normalize hyphens and colons to underscores, so that $context->preference('ILS-DI:AuthorisedIPs') would check for an environment variable named OVERRIDE_SYSPREF_ILS_DI_AuthorisedIPs.
Comment 4 Marcel de Rooy 2013-06-14 09:35:05 UTC
(In reply to comment #3)
> Another thing that occurred to me -- some system preference names contain
> hyphens and colons, neither of which can be used as part of environment
> variable names.  Not a huge deal, but perhaps the override mechanism should
> do some normalization of the syspref name before checking the environment.  

I would not be sad if we got rid of these special chars in pref names btw :)
Comment 5 Marcel de Rooy 2013-06-14 09:47:58 UTC
(In reply to comment #2)
> I can imagine an edge case, though -- there's nothing stopping one from
> using this in the *intranet* virtual host definitions (and I could imagine a
> Koha administrator wanting to do this to allow local admins access to system
> preferences while locking down ones such as 'marcflavour' that should never
> be changed).  But that could really confuse somebody who is wondering why
> their system preference change isn't having the desired effect.
> 
> Perhaps the system preference editor should be taught how to recognize
> overriden sysprefs?

I think that this feature is indeed very interesting, but dangerous at the other side. Galen's idea to make it visible in the pref editor is a good one too. Why not e.g. disable editing the pref when it is overridden? Other color?
But it will be very difficult to say which prefs should be excluded from overriding. You could start with Version, marcflavour, etc. but will become very arbitrary at the end.
And yet another idea: If you have a pref, yes.., AllowOverridingPrefValues or something, you could say that the admin at least knows which adventure he started?

> > +    if ( defined $ENV{"OVERRIDE_SYSPREF_$var"} ) {
> 
> The test should be 'exists', no?

If it does not exist, it is undefined? It could even exist and be undefined (pure theoretically)..

Setting status to In Discussion.
Comment 6 Marcel de Rooy 2013-06-14 09:51:59 UTC
(In reply to comment #5)
> And yet another idea: If you have a pref, yes.., AllowOverridingPrefValues
> or something, you could say that the admin at least knows which adventure he
> started?

Ideas come and go: A pref value that consists of the separated names of preferences that you allow to be overridden from the environment?
Comment 7 Galen Charlton 2013-06-14 15:45:57 UTC
(In reply to comment #5)
> But it will be very difficult to say which prefs should be excluded from
> overriding. You could start with Version, marcflavour, etc. but will become
> very arbitrary at the end.

To be clear, I wasn't calling for the creation of a list of system preferences that should not be overridden; what I was proposing was that a system administrator might choose to override marcflavour to prevent staff who only have access to the staff interface from changing its value.

> And yet another idea: If you have a pref, yes.., AllowOverridingPrefValues
> or something, you could say that the admin at least knows which adventure he
> started?

I don't think this is necessary.  To use the proposed mechanism, one has to have root access in order to change the Apache configuration, which is itself a higher bar to mischief than what YASP would provide.
Comment 8 Marcel de Rooy 2013-06-17 07:43:31 UTC
(In reply to comment #7)
> To be clear, I wasn't calling for the creation of a list of system
> preferences that should not be overridden; what I was proposing was that a
> system administrator might choose to override marcflavour to prevent staff
> who only have access to the staff interface from changing its value.

If these are two individuals, you will not always know who should prevent who from doing what..

> I don't think this is necessary.  To use the proposed mechanism, one has to
> have root access in order to change the Apache configuration, which is
> itself a higher bar to mischief than what YASP would provide.

Agreed, it is not essential but changing the Apache config and restarting http services will perhaps be not as easy as 'stop overriding this pref' by changing a koha preference..

What we do need in addition to this patch, is a way to make very clear in the preferences editor that a specific preference has been overridden.
Comment 9 Galen Charlton 2013-06-17 14:29:11 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > To be clear, I wasn't calling for the creation of a list of system
> > preferences that should not be overridden; what I was proposing was that a
> > system administrator might choose to override marcflavour to prevent staff
> > who only have access to the staff interface from changing its value.
> 
> If these are two individuals, you will not always know who should prevent
> who from doing what..

Well, ultimately that's up to the Koha consortium or library to work out.  In practice, however, I suspect most consortial users of Koha would be hesitant to let local library sysadmins have access to system preference until such time as Koha support per-library syspref values.

> What we do need in addition to this patch, is a way to make very clear in
> the preferences editor that a specific preference has been overridden.

Agreed.
Comment 10 Galen Charlton 2013-06-18 18:50:39 UTC
An observation: one situation where this would be *very* useful is setting OPACBaseURL and staffClientBaseURL from the Apache virtual host definition.
Comment 11 Marcel de Rooy 2013-06-19 11:33:38 UTC
Kyle:
Reading the comments now, we seem to reach consensus that this patch just needs an additional change in the pref editor to make visible that the pref is overridden.

Changing status to reflect that.
Comment 12 Kyle M Hall 2013-07-16 10:42:41 UTC
I don't think that is possible, at least not to my knowledge. What you are asking for is for one website ( the intranet ) to know the internal settings of another website ( the opac ). 

The only thing I can think of is to add an callable ajax function for the opac and intranet that can be passed each system preference and return true if it is overridden and false otherwise. We'd need to add a section to koha to list all the opacs and intranets in the system, and then for each system preference iterate though the list of opacs and intranets and check each one to see if the system preference is overridden. If it is, then we display some kind of not by the preference with the opac's url and new preference value.

This seems terribly and overly complicated just to see which system preferences are overridden. Is there a simpler way I am not seeing?


(In reply to M. de Rooy from comment #11)
> Kyle:
> Reading the comments now, we seem to reach consensus that this patch just
> needs an additional change in the pref editor to make visible that the pref
> is overridden.
> 
> Changing status to reflect that.
Comment 13 Marcel de Rooy 2013-07-16 11:08:40 UTC
> This seems terribly and overly complicated just to see which system 
> preferences are overridden. Is there a simpler way I am not seeing? 

When initializing the form, you could check %ENV for all OVERRIDE_SYSPREF keys and mark what you find in a special way? Would that be simpler?
Comment 14 Marcel de Rooy 2013-07-16 11:15:02 UTC
Too fast: yes too simple.
Comment 15 Kyle M Hall 2013-07-16 11:55:09 UTC
(In reply to M. de Rooy from comment #13)
> > This seems terribly and overly complicated just to see which system 
> > preferences are overridden. Is there a simpler way I am not seeing? 
> 
> When initializing the form, you could check %ENV for all OVERRIDE_SYSPREF
> keys and mark what you find in a special way? Would that be simpler?

Yes, the would unfortunately only work for staff intranet the preference editor is being used on. Since we have a minimum of two virtual hosts per koha installation ( one opac, one staff ) it won't work.

I'm wondering if maybe a simpler/better solution would be to add a parameter to koha-conf.xml to control if this feature is enabled or not, and if it is enabled, display a nice big warning at the top of the system preferences edit along the lines of "Warning: You have the system preferences override feature enabled, please check your apache configuration files before assuming a change in a system preference will affect your installation."
Comment 16 Galen Charlton 2013-07-16 15:43:16 UTC
My proposal, as I agree that it's impossible for one vhost to be 100% sure about the configuration of another one:

- only check the environment passed to the staff interface virtual host to determine if a syspref is overridden
- add a (commented-out) example to koha-httpd.conf that alerts the the sysadmin that they probably want to override any syspref in an OPAC vhost in the staff interface vhost as well.

Since overriding sysprefs via the Apache configuration would be an "advanced" feature, having a couple rough edges is not a showstopper IMO as long as we document them.
Comment 17 Marcel de Rooy 2013-07-17 13:09:37 UTC
Yes, I think that checking all possible opac and staff vhosts is overkill.
Parsing the apache conf is also not really what you want to do in Koha.

We could just introduce e.g. an environment var for the staff vhost like OVERRIDE_SYSPREF_NAMES. This could just be a comma separated list of all sysprefs that are overridden in one or more of the corresponding opac vhosts. (As a variant of what Galen also suggests.)

No further checking; using this feature wisely requires skills. Note that 
seeing a few overridden prefs should trigger the Koha admin to expect one more too..
Comment 18 Kyle M Hall 2013-08-09 14:24:02 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2013-08-09 14:26:57 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2013-08-09 14:28:03 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2013-08-15 08:32:45 UTC
Created attachment 20360 [details] [review]
Bug 10325 - Allow system preferences to be overridable from koha-httpd.conf

For Koha installations with multiple OPAC URLs, It would be nice to be
able to override systeprefs from the http conf file. Case in point,
a library wants to have two separate opacs, one the is only viewable
from within the library that allows patrons to place holds, and a second
public one that does not. In this case, overriding the system preference
RequestOnOpac would accomplish this simply, and with no ill affects.

This feature would of course be should only be used to override
cosmetic effects on the system, and should not be used for system
preferences such as CircControl, but would be great for preferences
such as OpacStarRatings, opacuserjs, OpacHighlightedWords and many
others!

Test Plan:
1) Apply this patch
2) Disable the system pref OpacHighlightedWords
3) Do a seach in the OPAC, not the term is not highlighted
4) Edit your koha-http.conf file, add the line
     SetEnv OVERRIDE_SYSPREF_OpacHighlightedWords "1"
   to your koha-http.conf file's OPAC section.
   Also add the line
     SetEnv OVERRIDE_SYSPREF_NAMES "OpacHighlightedWords"
   to the Intranet section
5) Restart your web server, or just reload it's config
6) Do a seach, now your search term should be highlighted!
7) From the intranet preference editor, view the pref,
   You should see a warning the this preference has been overridden.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2013-08-15 08:34:42 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2013-08-15 08:37:36 UTC
QA Comment: 
Fine addition! I added a small (trivial) QA followup.
Code looks good to me. No complaints from koha-qa. 
Before passing QA on this one, I have still two (simple) requests in connection with case insensitive pref names in koha-httpd.conf (easier use):

1) Would it be possible to make the check in Context.pm:
defined $ENV{"OVERRIDE_SYSPREF_$var"} 
case insensitive somehow?
Replace it perhaps by something like:
    my @k= grep {/OVERRIDE_SYSPREF_$var/i} keys %ENV;
    if ( @k ) {
        $value = $ENV{$k[0]};

2) Check in preferences.pl with any: Could this be case insensitive too?
 any { lc $name eq lc $_ } @override_syspref_names  ?

If you could add a followup for this, I will be happy to sign it off too :)
Comment 24 Marcel de Rooy 2013-08-15 09:21:45 UTC
my @k= grep {/OVERRIDE_SYSPREF_$var/i} keys %ENV;

Safer variant: my @k= grep {/^OVERRIDE_SYSPREF_${var}$/i} keys %ENV;
Comment 25 Kyle M Hall 2013-08-15 17:50:02 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2013-08-15 18:05:05 UTC
Marcel,

I've been talking to Galen about the case insensitivity for system preferences. We are both of the opinion that it's not good and it should go away. It's extremely odd in that the preference value is looked up in a case sensitive fashion, but cached in all lowercase.

This situation where the first call to C4::Context->preference() *must* be correctly cased, but any further calls do not have to be.

I had thought that it was possible that somewhere in the code base this situation might occur, so I wrote a script to extract the preference strings from all the calls to C4::Context->preference() anywhere in the code base. The script proved to me that there are no incorrectly cased calls to preference(). I will attach it for your perusal.

So, in summary:
1) Galen and I both agree your followup to add case insensitivity for this particular feature, as per your request, is not necessary. 
2) Galen and I both agree we should not cache the preferences in a case sensitive fashion.
3) I have written a script that proves no system preferences are called in an incorrectly cased manner.
3) My patch on it's own removes the case insensitivity from caching, while your qa followup re-adds it. Do to points 2 and 3, I think it's best that we do not use your followup ( except for the typo correction ), Though I do thank your for thinking of it and providing a followup!

If that was the only issue of note, I believe it's now safe to pass QA on this!
Comment 27 Kyle M Hall 2013-08-15 18:05:55 UTC
Created attachment 20376 [details] [review]
Bug 10325 - QA Followup - Fix typos
Comment 28 Kyle M Hall 2013-08-15 18:08:28 UTC
Created attachment 20377 [details]
Script to test for badly cases preference calls

To run, copy this to the root directory of your git Koha clone and execute it.
Comment 29 Marcel de Rooy 2013-08-15 19:41:06 UTC
(In reply to Kyle M Hall from comment #26)
> I've been talking to Galen about the case insensitivity for system
> preferences. We are both of the opinion that it's not good and it should go
> away. It's extremely odd in that the preference value is looked up in a case
> sensitive fashion, but cached in all lowercase.

It is not a matter of life and death, of course. But I would opt for case insensitive. For me, you may look it up in uppercase or whatever if you like that better :)

So I do not simply agree with "It is not good and it should go away" (without further reasons given).

But no problem for me to pass qa on this patch either.

Passed QA
Comment 30 Kyle M Hall 2013-08-16 09:35:08 UTC
(In reply to M. de Rooy from comment #29)
> (In reply to Kyle M Hall from comment #26)
> > I've been talking to Galen about the case insensitivity for system
> > preferences. We are both of the opinion that it's not good and it should go
> > away. It's extremely odd in that the preference value is looked up in a case
> > sensitive fashion, but cached in all lowercase.
> 
> It is not a matter of life and death, of course. But I would opt for case
> insensitive. For me, you may look it up in uppercase or whatever if you like
> that better :)
> 
> So I do not simply agree with "It is not good and it should go away"
> (without further reasons given).
> 
> But no problem for me to pass qa on this patch either.
> 
> Passed QA

Excellent. Thanks Marcel!
Comment 31 Galen Charlton 2013-09-08 02:30:08 UTC
Pushed to master, along with a follow-up that adds a test case and another that fixes typos.

Thanks, Kyle!