Bug 8952 - Add http:// to url in social media links on opac-detail
Summary: Add http:// to url in social media links on opac-detail
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
: 7307 (view as bug list)
Depends on:
Blocks: 11575
  Show dependency treegraph
 
Reported: 2012-10-22 06:51 UTC by Marcel de Rooy
Modified: 2015-12-03 22:02 UTC (History)
7 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 8952 - Add http:// to url in social media links on opac-detail (3.33 KB, patch)
2012-10-24 16:39 UTC, Owen Leonard
Details | Diff | Splinter Review
Signed patch (3.44 KB, patch)
2012-10-25 06:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8952: Followup: Change all occurrences of using_https (17.71 KB, patch)
2012-10-25 07:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8952 - Add http:// to url in social media links on opac-detail (3.57 KB, patch)
2012-12-27 11:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8952: Followup: Change all occurrences of using_https (17.14 KB, patch)
2012-12-27 11:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Patch which verifies Attachment 14292 as OK. (17.20 KB, patch)
2013-01-16 02:58 UTC, Corey
Details | Diff | Splinter Review
Bug 8952 - Add http:// to url in social media links on opac-detail (3.61 KB, patch)
2013-03-11 11:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8952: Followup: Change all occurrences of using_https (17.30 KB, patch)
2013-03-11 11:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8952 - Add http:// to url in social media links on opac-detail (3.49 KB, patch)
2013-05-23 07:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8952: Followup: Change all occurrences of using_https (17.30 KB, patch)
2013-05-23 07:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8952: using_https does not deal with Plack (1.73 KB, patch)
2013-05-23 15:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8952: using_https does not deal with Plack (2.51 KB, patch)
2013-05-24 08:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8952 - Add http:// to url in social media links on opac-detail (3.41 KB, patch)
2013-09-23 11:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8952: Followup: Change all occurrences of using_https (17.30 KB, patch)
2013-09-23 11:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8952: using_https does not deal with Plack (2.61 KB, patch)
2013-09-23 11:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2012-10-22 06:51:42 UTC
Currently, the url is built using OPACBaseURL but does not contain a http:// prefix.
Comment 1 Owen Leonard 2012-10-24 16:39:24 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2012-10-25 06:21:48 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2012-10-25 06:25:46 UTC
Works as advertised. Signing off.
Comment 4 Jonathan Druart 2012-10-25 07:29:58 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2012-10-25 07:31:53 UTC
This followup modifies all occurrences of "using_https". I didn't test the syndetics features, I don't know how to test it.
Comment 6 Owen Leonard 2012-10-25 14:03:31 UTC
Thanks for the follow-up Jonathan, I like your solution for the Syndetics stuff. I would sign off just after looking at the updated template, but I wonder if we should get someone with Syndetics and someone with LibraryThing to test?
Comment 7 Jonathan Druart 2012-12-27 11:23:00 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2012-12-27 11:23:11 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2012-12-27 11:23:35 UTC
Rebased patches
Comment 10 Corey 2013-01-16 02:58:59 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2013-03-11 11:07:57 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2013-03-11 11:08:01 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2013-03-11 11:08:43 UTC
QA Comment:
Simplifies template. Looks good to me.

Passed QA
Comment 15 Marcel de Rooy 2013-03-17 13:40:18 UTC
(In reply to comment #14)
> This does not work for me. It creates links like:
> http://www.facebook.com/sharer.php?u=https://kohadev.cpbibliography.com/cgi-
> bin/koha/opac-detail.pl?biblionumber=80&t=M%C3%A1dam%20President,
> %20I%20think%20this%20title%20is%20much%20too%20long%20to%20fit%20on%20a%20bo
> ok%20cover%20and%20that%20troubles%20me;%20moreover,
> %20the%20quick%20brown%20fox%20jumped%20over%20the%20lazy%20dog%20over%20and%
> 20over%20and%20over%20and%20over%20/ even though kohadev does not use https.

Funny? The using_https is determined by calling GGI::https in Auth.pm. If this does not work for you, you should have more of these in current master already? Could you check on e.g. opac-detail in master? If these are at fault now, we should look at the condition in Auth.pm setting the variable.
Comment 16 Marcel de Rooy 2013-05-23 07:43:59 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2013-05-23 07:44:04 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2013-05-23 07:44:27 UTC
Resolved trivial merge conflict.
Comment 19 Marcel de Rooy 2013-05-23 08:06:22 UTC
This still looks good to me, although I could not test Syndetics changes.
Unfortunately Jared did not respond to comment15.
I am resetting the status of this patch to Signed off. For the reason of the problem mentioned in comment14 I am asking an additional QAer to sign off please.
Thanks.
Comment 20 Marcel de Rooy 2013-05-23 12:19:46 UTC
From IRC:
[14:16]	jcamins	marcelr: I confirm that when I apply that patch the social media links all point to https even though I'm using http.
Comment 21 Jonathan Druart 2013-05-23 15:12:25 UTC
(In reply to comment #20)
> From IRC:
> [14:16]	jcamins	marcelr: I confirm that when I apply that patch the social
> media links all point to https even though I'm using http.

Same here using Plack.
Comment 22 Jonathan Druart 2013-05-23 15:32:42 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2013-05-23 16:52:53 UTC
(In reply to comment #22)
> When using Plack, the https method returns 'OFF'.
> We have to test this value before sending the value to templates.

Good catch.
This adjustment imo does deserve some comment in the code.
Comment 24 Jonathan Druart 2013-05-24 08:53:56 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2013-05-31 09:03:54 UTC
Jonathan:
With another patch, I face the same problem as we do here. Should we add http or https to OPACBaseURL?
Note that this is reported under several other bugs too already.
Could we separate your last patch from this report, and make it the solution for that problem?
I am thinking that we could also add a preference. But if we do that, we could just as easy add http or https to OPACBaseURL and staff, and remove some hardcodes http's in a few templates.
What do you think?

Added two see also's.
Comment 26 Jonathan Druart 2013-05-31 09:37:47 UTC
(In reply to comment #25)
> Jonathan:
> With another patch, I face the same problem as we do here. Should we add
> http or https to OPACBaseURL?
> Note that this is reported under several other bugs too already.
> Could we separate your last patch from this report, and make it the solution
> for that problem?
> I am thinking that we could also add a preference. But if we do that, we
> could just as easy add http or https to OPACBaseURL and staff, and remove
> some hardcodes http's in a few templates.
> What do you think?
> 
> Added two see also's.

Yes maybe it is the solution. All what I know is that the behavior of this pref is not clearly defined and it is used in many places into Koha and certainly does not work everywhere (sometimes it is prefixed with 'http', sometimes not, depending where it is used...).
Comment 27 Galen Charlton 2013-05-31 16:22:25 UTC
(In reply to comment #25)
> Jonathan:
> With another patch, I face the same problem as we do here. Should we add
> http or https to OPACBaseURL?

It's a valid configuration to make the OPAC available via both HTTP and HTTPS, so using OPACBaseURL to define the protocol gives me a bit of a twitch.
Comment 28 Marcel de Rooy 2013-05-31 17:51:22 UTC
(In reply to comment #27)
> (In reply to comment #25)
> > Jonathan:
> > With another patch, I face the same problem as we do here. Should we add
> > http or https to OPACBaseURL?
> 
> It's a valid configuration to make the OPAC available via both HTTP and
> HTTPS, so using OPACBaseURL to define the protocol gives me a bit of a
> twitch.

Well, probably this is a misunderstanding.
Currently, http or https should not be inserted into the pref.
But Koha does add at some places http by default (which is wrong indeed) and at other places tries to know what to prefix (see using_https in Auth.pm). But the last test does not work correctly under Plack.

What I did mean, was: If we let the user just once insert http or https for his server, and we do not prefix it anymore, we have a solution..
Comment 29 Magnus Enger 2013-08-09 08:53:14 UTC
(In reply to M. de Rooy from comment #28)
> What I did mean, was: If we let the user just once insert http or https for
> his server, and we do not prefix it anymore, we have a solution..

But isn't it possible to have the same domain available both with and without HTTPS? So both of these can work at the same time:
http://koha.example.org/
https://koha.example.org/

Then it would seem to be a bad idea to "hard code" the protocol in a syspref, and better to figure it out on a per request basis, perhaps?
Comment 30 David Cook 2013-08-20 05:54:27 UTC
(In reply to Magnus Enger from comment #29)
> (In reply to M. de Rooy from comment #28)
> > What I did mean, was: If we let the user just once insert http or https for
> > his server, and we do not prefix it anymore, we have a solution..
> 
> But isn't it possible to have the same domain available both with and
> without HTTPS? So both of these can work at the same time:
> http://koha.example.org/
> https://koha.example.org/
> 
> Then it would seem to be a bad idea to "hard code" the protocol in a
> syspref, and better to figure it out on a per request basis, perhaps?

I think figuring it out on a per request basis is probably best, but it seems like there is a problem using the CGI method https() when Plack is in use, yes?

I know next to nothing about Plack but maybe something like this? (https://github.com/plack/Plack/wiki/How-to-detect-reverse-proxy-and-SSL-frontend).

Now that I think about it, if you're using a proxy, CGI->https() will probably always return NO too unless one's using SSL internally too.
Comment 31 David Cook 2013-08-20 06:13:08 UTC
It's also worth noting that Internet Explorer and at least one feedreader I've tried renders RSS links incorrectly if http or https isn't included with the URL. For some reason, it treats them like relative links, which makes a useless URL.
Comment 32 Marcel de Rooy 2013-08-21 14:56:06 UTC
(In reply to Magnus Enger from comment #29)
> But isn't it possible to have the same domain available both with and
> without HTTPS? So both of these can work at the same time:
> http://koha.example.org/
> https://koha.example.org/
> 
> Then it would seem to be a bad idea to "hard code" the protocol in a
> syspref, and better to figure it out on a per request basis, perhaps?

Yes, both could be available. I think that bug 10325 can help us out here. It allows you to override a systempreference via koha-httpd.conf. In that case you could override the base url for the https port.
At the same time I am wondering if both are available, would it be so bad to let opacbaseurl redirect to one of the two? (Just a pragmatic thought.)
Comment 33 Marcel de Rooy 2013-09-23 11:44:39 UTC
Created attachment 21359 [details] [review]
Bug 8952 - Add http:// to url in social media links on opac-detail

This patch adds a variable to the template, "protocol" based on
whether "using_https" is set. The appropriate protocol is
inserted before social media links.

To test, confirm that  OPACBaseURL is set and lacks "http(s)://"
Social media links should include correct URLs.

A follow-up patch could copy use of this [% protocol %] variable
in other parts of this template which duplicate large chunks of
HTML based on [% using_https %].

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2013-09-23 11:44:44 UTC
Created attachment 21360 [details] [review]
Bug 8952: Followup: Change all occurrences of using_https

Signed-off-by: Corey Fuimaono <agent.075@gmail.com>
Verified.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2013-09-23 11:44:48 UTC
Created attachment 21361 [details] [review]
Bug 8952: using_https does not deal with Plack

When using Plack, the https method returns 'OFF'.
We have to test this value before sending the value to templates.

Test plan:
1/ Fill your OPACBaseUrl

2/ Configure apache for using http
3/ Check the social networks links (should be http://OPACBaseUrl)

4/ Launch Plack
5/ Check the social networks link (should be http://OPACBaseUrl)
6/ Stop Plack

7/ Configure apache for using https
  sudo openssl req -x509 -nodes -days 365 -newkey rsa:1024 -out
/etc/apache2/server.crt -keyout /etc/apache2/server.key
and add in you virtualhost (with :443)
    SSLEngine on
    SSLCertificateFile /etc/apache2/server.crt
    SSLCertificateKeyFile /etc/apache2/server.key
  a2enmod ssl
  service apache2 restart
8/ Check the social networks links (should be https://OPACBaseUrl)

FIXME: Under Plack, with ssl actived, the CGI->https() method always
returns 'OFF'.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Marcel de Rooy 2013-09-23 11:56:23 UTC
With reference to the discussion above:

These patches solve some URLs built with OPACBaseURL. Currently, these URLs are incorrect for every one. With the third patch, they are only incorrect for someone using Plack AND https for OPAC. (Just to mention the scope: These are OPAC URLs for LibraryThing, Syndetics and social media.)
So the patch introduces a FIXME for this particular Plack/HTTPS combination. We should still find a solution for these Plack users. In the meantime, the situation for all others improves.
No complaints from qa tools.

With the last followup already being four months old (first patch from 2012), I am (pragmatically) setting the status to Passed QA. Let the RM decide :) I would not object to pushing them.
Comment 37 Galen Charlton 2013-09-23 19:31:16 UTC
(In reply to M. de Rooy from comment #36)
> With the last followup already being four months old (first patch from
> 2012), I am (pragmatically) setting the status to Passed QA. Let the RM
> decide :) I would not object to pushing them.

Jared and I have been discussing this.  No firm conclusion as yet, but I'm reasonably confident that we'll be able to figure out a recommended Plack config that can allow a script to detect whether it's running under HTTP or HTTPS.  A folow-up is likely, but most likely we'll achieve something that works for all (though it's possible that the solution will make certain requirements of Plack setups).
Comment 38 David Cook 2013-09-24 00:07:18 UTC
(In reply to Galen Charlton from comment #37)
> (In reply to M. de Rooy from comment #36)
> > With the last followup already being four months old (first patch from
> > 2012), I am (pragmatically) setting the status to Passed QA. Let the RM
> > decide :) I would not object to pushing them.
> 
> Jared and I have been discussing this.  No firm conclusion as yet, but I'm
> reasonably confident that we'll be able to figure out a recommended Plack
> config that can allow a script to detect whether it's running under HTTP or
> HTTPS.  A folow-up is likely, but most likely we'll achieve something that
> works for all (though it's possible that the solution will make certain
> requirements of Plack setups).

I'm wondering if anyone has ideas on how this might work using a proxy where the internal requests are all HTTP but the external are HTTPS.
Comment 39 Galen Charlton 2013-09-24 14:53:04 UTC
(In reply to David Cook from comment #38)
> (In reply to Galen Charlton from comment #37)
> > (In reply to M. de Rooy from comment #36)
> > > With the last followup already being four months old (first patch from
> > > 2012), I am (pragmatically) setting the status to Passed QA. Let the RM
> > > decide :) I would not object to pushing them.
> > 
> > Jared and I have been discussing this.  No firm conclusion as yet, but I'm
> > reasonably confident that we'll be able to figure out a recommended Plack
> > config that can allow a script to detect whether it's running under HTTP or
> > HTTPS.  A folow-up is likely, but most likely we'll achieve something that
> > works for all (though it's possible that the solution will make certain
> > requirements of Plack setups).
> 
> I'm wondering if anyone has ideas on how this might work using a proxy where
> the internal requests are all HTTP but the external are HTTPS.

A lot of proxies will set a X-Forwarded-Proto header that could be checked.
Comment 40 Galen Charlton 2014-05-05 04:09:46 UTC
Pushed to master.  Thanks, Owen and Jonathan!
Comment 41 David Cook 2014-06-16 02:33:55 UTC
https://github.com/plack/Plack/wiki/How-to-detect-reverse-proxy-and-SSL-frontend

This might be useful to some folk using HTTPS, Plack, and a reverse proxy out front.
Comment 42 Jonathan Druart 2015-02-20 16:21:54 UTC
*** Bug 7307 has been marked as a duplicate of this bug. ***