Bug 16401 - System preference staffClientBaseURL hardcoded to 'http://'
Summary: System preference staffClientBaseURL hardcoded to 'http://'
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Josef Moravec
QA Contact: Nick Clemens
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-30 06:04 UTC by Zeno Tajoli
Modified: 2017-10-11 16:53 UTC (History)
5 users (show)

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


Attachments
Bug 16401: System preference staffClientBaseURL hardcoded to 'http://' (6.48 KB, patch)
2017-08-15 15:46 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED OFF] Bug 16401: System preference staffClientBaseURL hardcoded to 'http://' (6.55 KB, patch)
2017-08-17 10:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 16401: System preference staffClientBaseURL hardcoded to 'http://' (6.59 KB, patch)
2017-09-01 11:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16401: System preference staffClientBaseURL hardcoded to 'http://' (6.85 KB, patch)
2017-09-20 21:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 16401: (follow-up) Avoid changing empty staffClientBaseURL preference (1.20 KB, patch)
2017-09-20 21:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 16401: System preference staffClientBaseURL hardcoded to 'http://' (6.85 KB, patch)
2017-09-20 21:31 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 16401: (follow-up) Avoid changing empty staffClientBaseURL preference (1.20 KB, patch)
2017-09-20 21:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 16401: System preference staffClientBaseURL hardcoded to 'http://' (6.82 KB, patch)
2017-10-11 00:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16401: (follow-up) Avoid changing empty staffClientBaseURL preference (1.25 KB, patch)
2017-10-11 00:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 16401: Add a warning if letter.content matches staffClientBaseURL (1.39 KB, patch)
2017-10-11 15:51 UTC, Jonathan Druart
Details | Diff | Splinter Review

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