Bug 16401

Summary: System preference staffClientBaseURL hardcoded to 'http://'
Product: Koha Reporter: Zeno Tajoli <ztajoli>
Component: Web servicesAssignee: Josef Moravec <josef.moravec>
Status: CLOSED FIXED QA Contact: Nick Clemens <nick>
Severity: minor    
Priority: P5 - low CC: jonathan.druart, josef.moravec, katrin.fischer, mtompset, nick
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12027
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 12027    
Attachments: Bug 16401: System preference staffClientBaseURL hardcoded to 'http://'
[SIGNED OFF] Bug 16401: System preference staffClientBaseURL hardcoded to 'http://'
Bug 16401: System preference staffClientBaseURL hardcoded to 'http://'
Bug 16401: System preference staffClientBaseURL hardcoded to 'http://'
Bug 16401: (follow-up) Avoid changing empty staffClientBaseURL preference
Bug 16401: System preference staffClientBaseURL hardcoded to 'http://'
Bug 16401: (follow-up) Avoid changing empty staffClientBaseURL preference
Bug 16401: System preference staffClientBaseURL hardcoded to 'http://'
Bug 16401: (follow-up) Avoid changing empty staffClientBaseURL preference
Bug 16401: Add a warning if letter.content matches staffClientBaseURL

Description Zeno Tajoli 2016-04-30 06:04:23 UTC
In many points now, in particular in CAS authentication, we use the system preference staffClientBaseURL as  OPACBaseURL.

The preference should be a complete URL, starting with http:// or https://.

So we need to change svc/import_bib line 101.
From 
my $url = 'http://'. C4::Context->preference('staffClientBaseURL') .'/cgi-bin/koha/catalogue/detail.pl?biblionumber='. $biblionumber;

To
my $url = C4::Context->preference('staffClientBaseURL') .'/cgi-bin/koha/catalogue/detail.pl?biblionumber='. $biblionumber;

It is probably a good idea to change also koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/staff_client.pref.
Like OPACBaseURL:
" . This should be a complete URL, starting with http:// or https://. Do not include a trailing slash in the URL. (This must be filled in correctly for CAS, svc, and load_testing to work.)
Comment 1 Josef Moravec 2017-08-15 15:46:25 UTC
Created attachment 66025 [details] [review]
Bug 16401: System preference staffClientBaseURL hardcoded to 'http://'

Test plan:
1) Insert some value to staffBaseURL preference, without starting http://
2) Apply the patch
3) Update database
4) Go to system preferences adminsitration, find the staffBaseURL
preference, the inserted value should be prepended with 'http://' and
the comment should be "This should be a complete URL, starting with
http:// or https://. Do not include a trailing slash in the URL. (This
must be filled in correctly for CAS, svc, and load_testing to work.)"
5) prove t/db_dependent/Auth_with_cas.t
6) prove t/db_dependent/selenium/basic_workflow.t
7) prove t/db_dependent/check_sysprefs.t
Comment 2 Katrin Fischer 2017-08-17 10:31:44 UTC
Glad to see this patch - When using CAS for example it's already required that you add https:// in order to make things work right.
Comment 3 Katrin Fischer 2017-08-17 10:32:51 UTC
Created attachment 66143 [details] [review]
[SIGNED OFF] Bug 16401: System preference staffClientBaseURL hardcoded to 'http://'

Test plan:
1) Insert some value to staffBaseURL preference, without starting http://
2) Apply the patch
3) Update database
4) Go to system preferences adminsitration, find the staffBaseURL
preference, the inserted value should be prepended with 'http://' and
the comment should be "This should be a complete URL, starting with
http:// or https://. Do not include a trailing slash in the URL. (This
must be filled in correctly for CAS, svc, and load_testing to work.)"
5) prove t/db_dependent/Auth_with_cas.t
6) prove t/db_dependent/selenium/basic_workflow.t
7) prove t/db_dependent/check_sysprefs.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 4 Nick Clemens 2017-09-01 11:32:14 UTC
Created attachment 66712 [details] [review]
Bug 16401: System preference staffClientBaseURL hardcoded to 'http://'

Test plan:
1) Insert some value to staffBaseURL preference, without starting http://
2) Apply the patch
3) Update database
4) Go to system preferences adminsitration, find the staffBaseURL
preference, the inserted value should be prepended with 'http://' and
the comment should be "This should be a complete URL, starting with
http:// or https://. Do not include a trailing slash in the URL. (This
must be filled in correctly for CAS, svc, and load_testing to work.)"
5) prove t/db_dependent/Auth_with_cas.t
6) prove t/db_dependent/selenium/basic_workflow.t
7) prove t/db_dependent/check_sysprefs.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Jonathan Druart 2017-09-01 15:37:45 UTC
What about the following query instead?
UPDATE systempreferences SET value = CONCAT('http://', value) WHERE variable = 'staffClientBaseURL' AND value <> "" AND value NOT LIKE '^http%';

staffClientBaseURL is not supposed to be empty, but it will avoid to have it set to "http://"
Comment 6 Katrin Fischer 2017-09-20 21:30:37 UTC
Created attachment 67268 [details] [review]
Bug 16401: System preference staffClientBaseURL hardcoded to 'http://'

Test plan:
1) Insert some value to staffBaseURL preference, without starting http://
2) Apply the patch
3) Update database
4) Go to system preferences adminsitration, find the staffBaseURL
preference, the inserted value should be prepended with 'http://' and
the comment should be "This should be a complete URL, starting with
http:// or https://. Do not include a trailing slash in the URL. (This
must be filled in correctly for CAS, svc, and load_testing to work.)"
5) prove t/db_dependent/Auth_with_cas.t
6) prove t/db_dependent/selenium/basic_workflow.t
7) prove t/db_dependent/check_sysprefs.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Katrin Fischer 2017-09-20 21:30:40 UTC
Created attachment 67269 [details] [review]
Bug 16401: (follow-up) Avoid changing empty staffClientBaseURL preference

Adds a condition to check that the staffClientBaseUrl is not empty
before updating it with 'http://'.
Comment 8 Katrin Fischer 2017-09-20 21:31:57 UTC
Created attachment 67270 [details] [review]
Bug 16401: System preference staffClientBaseURL hardcoded to 'http://'

Test plan:
1) Insert some value to staffBaseURL preference, without starting http://
2) Apply the patch
3) Update database
4) Go to system preferences adminsitration, find the staffBaseURL
preference, the inserted value should be prepended with 'http://' and
the comment should be "This should be a complete URL, starting with
http:// or https://. Do not include a trailing slash in the URL. (This
must be filled in correctly for CAS, svc, and load_testing to work.)"
5) prove t/db_dependent/Auth_with_cas.t
6) prove t/db_dependent/selenium/basic_workflow.t
7) prove t/db_dependent/check_sysprefs.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Katrin Fischer 2017-09-20 21:32:01 UTC
Created attachment 67271 [details] [review]
Bug 16401: (follow-up) Avoid changing empty staffClientBaseURL preference

Adds a condition to check that the staffClientBaseUrl is not empty
before updating it with 'http://'.
Comment 10 Katrin Fischer 2017-09-20 21:32:25 UTC
I like Jonathan's idea and have written a follow-up for it. The change is tiny, so pushing it back to QA.
Comment 11 Mark Tompsett 2017-09-21 05:50:33 UTC
C4::Context has a OPACBaseURL change to make sure a protocol is prepended. Probably should do something similar for staffBaseUrl. Also, are there any letters/noticed which might be affected?
Comment 12 Katrin Fischer 2017-09-21 05:52:52 UTC
I don't see how this change would affect notices or letters. When I tested this initially I checked all occurrences of the preference had been taken into account.
Comment 13 Nick Clemens 2017-10-11 00:11:51 UTC
Created attachment 67918 [details] [review]
Bug 16401: System preference staffClientBaseURL hardcoded to 'http://'

Test plan:
1) Insert some value to staffBaseURL preference, without starting http://
2) Apply the patch
3) Update database
4) Go to system preferences adminsitration, find the staffBaseURL
preference, the inserted value should be prepended with 'http://' and
the comment should be "This should be a complete URL, starting with
http:// or https://. Do not include a trailing slash in the URL. (This
must be filled in correctly for CAS, svc, and load_testing to work.)"
5) prove t/db_dependent/Auth_with_cas.t
6) prove t/db_dependent/selenium/basic_workflow.t
7) prove t/db_dependent/check_sysprefs.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens 2017-10-11 00:11:54 UTC
Created attachment 67919 [details] [review]
Bug 16401: (follow-up) Avoid changing empty staffClientBaseURL preference

Adds a condition to check that the staffClientBaseUrl is not empty
before updating it with 'http://'.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Mark Tompsett 2017-10-11 01:39:09 UTC
(In reply to Katrin Fischer from comment #12)
> I don't see how this change would affect notices or letters. When I tested
> this initially I checked all occurrences of the preference had been taken
> into account.

Are there any letters with something like <<staffClientBaseURL>>? Because OPACBaseURL had some with http:// in front for some letters/notices. I vaguely remember checking and not finding anything, but that is how. So Passed QA is fine with me.
Comment 16 Nick Clemens 2017-10-11 13:46:58 UTC
(In reply to M. Tompsett from comment #15)
> (In reply to Katrin Fischer from comment #12)
> Are there any letters with something like <<staffClientBaseURL>>? 

I didn't see anything in my check, doesn't mean a library isn't using it in their custom notices, but I think it is a minor local adjustment if needed
Comment 17 Jonathan Druart 2017-10-11 15:51:02 UTC
Created attachment 67967 [details] [review]
Bug 16401: Add a warning if letter.content matches staffClientBaseURL
Comment 18 Jonathan Druart 2017-10-11 16:53:23 UTC
Pushed to master for 17.11, thanks to everybody involved!