Bug 16615

Summary: OpenLibrary: always use SSL when referencing external resources
Product: Koha Reporter: Oliver Behnke <oliver.behnke>
Component: OPACAssignee: Oliver Behnke <oliver.behnke>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: frederic, gwilliams, jonathan.druart+koha, katrin.fischer, kyle, mtompset
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14571
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 17137    
Attachments: Patch: Open Library API via SSL
Bug 16615: Open Library API fails for full-site-SSL
Bug 16615: Follow up - missed a spot
Bug 16615: Open Library API fails for full-site-SSL
Bug 16615: Follow up - missed a spot
Bug 17103 - Google API Loader jsapi called over http
Bug 16615: Follow up - missed a spot (signed-off)
Bug 16615: Open Library API fails for full-site-SSL
Bug 16615: Follow up - missed a spot
Bug 16615: Update OPACSearchForTitleIn to access openlibrary.org via https

Description Oliver Behnke 2016-05-30 07:35:26 UTC Comment hidden (obsolete)
Comment 1 Katrin Fischer 2016-08-14 14:52:12 UTC
Hi Oliver, just found this bug:

1) Could you reattach the file as a complete patch file? It's only the diff, but it doesn't have the information about the authorship etc. as you can see on other bugs.

There are at least 2 ways to do this:
- git format-patch (https://wiki.koha-community.org/wiki/Version_Control_Using_Git)
- git bz (https://wiki.koha-community.org/wiki/Git_bz_configuration)

In both cases, make sure you set up your git config before you create the file so it contains your name and e-mail address.

2) After attaching a patch file, don't forget to change the status to "Needs Sign-off". This will make the bug show up in the lists the testers look at - otherwise your work can be easily missed.

3) If you provide a patch, don't forget to assign the bug to yourself, changing the 'Assignee' in the bug report.

Please let us know if we can help you with any of that or if there are other questions about the development process.
Comment 2 Mark Tompsett 2016-08-14 17:01:16 UTC Comment hidden (obsolete)
Comment 3 Mark Tompsett 2016-08-14 17:02:52 UTC Comment hidden (obsolete)
Comment 4 Mark Tompsett 2016-08-14 17:04:56 UTC Comment hidden (obsolete)
Comment 5 Mark Tompsett 2016-08-14 17:05:01 UTC Comment hidden (obsolete)
Comment 6 Mark Tompsett 2016-08-14 17:07:51 UTC Comment hidden (obsolete)
Comment 7 Mark Tompsett 2016-08-14 17:08:37 UTC
Oops... Sorry, I was trying to set the signed off status.
Comment 8 Mark Tompsett 2016-08-14 17:12:20 UTC
Comment on attachment 54444 [details] [review]
Bug 17103 - Google API Loader jsapi called over http

My mistake. Sorry.
Comment 9 Mark Tompsett 2016-08-14 17:13:50 UTC
I've signed off the first, and added the second. A single 's' shouldn't really need a secondary sign off should it?
Comment 10 Katrin Fischer 2016-08-14 17:25:33 UTC
Now this moved faster than I expected :) 

Oliver, would you be able to sign-off Mark's follow-up?
Comment 11 Oliver Behnke 2016-08-15 09:00:52 UTC
Hi guys,

I would sign it off code-wise but I'm not able to sign it off based on the test plan just because I can't test it that way (no HTTP/non-SSL access possible). That said, in principle I'm fine with the patch.

Best,
Oliver

PS: FYI, I stripped my local git details from the original patch as I didn't remember that you use git at all ;-)
Comment 12 Katrin Fischer 2016-08-15 09:06:30 UTC
Feel free to sign off - I or someone else will probably test without HTTPS in QA... :)

Take a look at the git bz link - I think it will explain some about our workflow with git and Bugzilla.
Comment 13 Oliver Behnke 2016-08-15 09:44:29 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2016-08-15 12:16:20 UTC
Created attachment 54462 [details] [review]
Bug 16615: Open Library API fails for full-site-SSL

When running Koha in a full-site-SSL configuration accessing
external resouces like Open Library cover images via HTTP
doesn't work (mixed SSL/non-SSL content). It thus breaks a
core feature of Koha. Referencing such resources always via
HTTPS resolves the problem and also enhances privacy as a
side-effect.

Previous tickets took care of Amazon resources. This ticket
is supposed to address the remaining external resource providers,
starting with Open Library.

TEST PLAN
---------
1) Over https access your OPAC with the Enhanced Content system
   preferences 'OpenLibraryCovers' set to 'Add', and
   'OpenLibrarySearch' set to 'Show'.
2) Search for something (e.g. Mo Willems)
   -- Your browser console log will show messages about
      mixed content.
3) Apply the patch
4) Making sure to reload the java script, attempt steps 1 & 2
   again.
   -- no mixed content messages should be given
5) run koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Jonathan Druart 2016-08-15 12:16:25 UTC
Created attachment 54463 [details] [review]
Bug 16615: Follow up - missed a spot

Also, it is recommended to apply bug 17103 before this.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Oliver Bock <oliver.bock@aei.mpg.de>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Jonathan Druart 2016-08-15 12:16:29 UTC
Created attachment 54464 [details] [review]
Bug 16615: Update OPACSearchForTitleIn to access openlibrary.org via https

Note that we could also update the syspref for existing installs

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Oliver Behnke 2016-08-15 13:48:47 UTC
The previous patch only updates the OpenLibrary URL. Why not change all of them (after testing they support it)?
Comment 18 Jonathan Druart 2016-08-15 13:58:14 UTC
(In reply to Oliver Bock from comment #17)
> The previous patch only updates the OpenLibrary URL. Why not change all of
> them (after testing they support it)?

Because I did not test them :)
Feel free to submit a patch to replace it.
Comment 19 Mark Tompsett 2016-08-16 22:46:26 UTC
Since the Passed QA scope is OpenLibrary only. I figure another bug is better suited to fixing other issues. That's why I renamed this.
Comment 20 Oliver Behnke 2016-08-17 07:25:11 UTC
Too bad. I intentionally named this bug without a specific service because all previous fixes so far typically only covered a single service. That means all the rest doesn't get fixed. My hope was to kick off a general fixing round, starting things of myself with OpenLibrary. This (intended) effort depends of course on others stepping in to fix the rest. Changing this back to the old scheme seems like a missed opportunity.

Best,
Oliver
Comment 21 Katrin Fischer 2016-08-17 08:30:46 UTC
It's just a workflow thing. Once patches have been pushed we don't 'reuse' the bug for more patches - it causes too much confusion with the status and also our release notes are created from the bug titles. 
We use one Omnibus bug with linked bugs for the separate issues. I will create an Omnibus bug and links this one to show how it works.
Comment 23 Oliver Behnke 2016-08-17 08:35:43 UTC
Ok, sounds good. This should also ensure that completed patches can already be rolled out with the next release.

Thanks,
Oliver
Comment 24 Kyle M Hall 2016-08-18 15:39:47 UTC
Pushed to master for 16.11, thanks Oliver, Mark and Jonathan!
Comment 25 Oliver Behnke 2016-08-19 09:43:58 UTC
Sure, no problem. Hopefully my other tickets will also be processed soon...

One final question: is this going to be backported to 3.22 as well? We're still on that release.

Thanks,
Oliver
Comment 26 Katrin Fischer 2016-08-19 09:45:21 UTC
It's marked as a bug (not enh) so the RMaints will take a look at this and decide.
Comment 27 Frédéric Demians 2016-08-23 10:29:22 UTC
Pushed in 16.05. Will be in 16.05.03.
Comment 28 Oliver Behnke 2016-11-11 15:35:33 UTC
Hm, this bug is still assigned to me although there's no pending task. How to deal with this? Should it just be closed?

Thanks.
Comment 29 Jonathan Druart 2016-11-14 11:31:25 UTC
(In reply to Oliver Bock from comment #28)
> Hm, this bug is still assigned to me although there's no pending task. How
> to deal with this? Should it just be closed?
> 
> Thanks.

Status is "pushed to stable" and should be marked as closed soon.