Works as advertised. Signing off.

This followup modifies all occurrences of "using_https". I didn't test the syndetics features, I don't know how to test it.

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?

Rebased patches

QA Comment:
Simplifies template. Looks good to me.

Passed QA

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%20book%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.

(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.

Resolved trivial merge conflict.

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.

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.

(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.

(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.

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.

(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...).

(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.

(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..

(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?

(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.

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.

(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.)

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>

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>

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>

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.

(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).

(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.

(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.

Pushed to master.  Thanks, Owen and Jonathan!

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.

*** Bug 7307 has been marked as a duplicate of this bug. ***