Bug 12398 - CAS authentication not working
Summary: CAS authentication not working
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Target Milestone: Oldversions
Assignee: Julian Maurice
QA Contact: Katrin Fischer
URL:
Whiteboard:
Keywords:
Depends on: 11219
Blocks:
  Show dependency treegraph
 
Reported: 2014-06-10 10:47 UTC by Julian Maurice
Modified: 2018-12-16 19:34 UTC (History)
8 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:
Version(s) released in:


Attachments
Bug 12398: Fix CAS authentication validation (1.34 KB, patch)
2014-06-10 10:49 UTC, Julian Maurice
Details | Diff | Splinter Review
signed patch (1.41 KB, patch)
2014-06-17 14:28 UTC, Matthias Meusburger
Details | Diff | Splinter Review
[PASSED QA] Bug 12398: Fix CAS authentication validation (1.80 KB, patch)
2014-07-24 20:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12398: Add test for C4::Auth_with_cas::_url_with_get_params (1.12 KB, patch)
2014-07-30 12:03 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12398: Use mocks to make the test "database independent" (818 bytes, patch)
2014-07-30 12:47 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12398: Use mocks to make the test "database independent" (1.19 KB, patch)
2014-07-30 12:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 12398: Move t/Auth_with_cas.t to t/db_dependent/Auth_with_cas.t (2.10 KB, patch)
2014-07-31 07:12 UTC, Julian Maurice
Details | Diff | Splinter Review
[PASSED QA] Bug 12398: Fix CAS authentication validation (1.80 KB, patch)
2014-08-01 07:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12398: Add test for C4::Auth_with_cas::_url_with_get_params (1.41 KB, patch)
2014-08-01 07:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12398: (followup) tidy unit tests (2.58 KB, patch)
2014-08-01 13:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2014-06-10 10:47:29 UTC
The CAS authentication doesn't work because the 'ticket' parameter is not removed when building service url which is passed to service_validate().
$query->delete('ticket') works, but $query->url_param() doesn't seem to take into account that parameter was deleted.
Checked with last version of CGI. Same behaviour.
Comment 1 Julian Maurice 2014-06-10 10:49:22 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2014-06-10 10:53:25 UTC
To test, you can follow the test plan of bug 12046
Comment 3 Julian Maurice 2014-06-11 07:29:46 UTC
Test plan of bug 12046 is not enough.
So here is the test plan:

1 enable the casAuthentication syspref
2 fill the casServerUrl syspref with a working CAS server url
3 go on the opac home page
4 click on "Log in to your account" link (top right)
5 click on "click here to login" after "if you have a CAS account"
6 login on the cas server authentication page
7 you should be redirected to Koha with an error message
8 apply the patch
9 repeat steps 5 and 6
10 you should be redirected to Koha logged in
Comment 4 Julian Maurice 2014-06-11 07:30:08 UTC
Changed severity to critical
Comment 5 Matthias Meusburger 2014-06-17 14:28:06 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2014-07-12 16:54:06 UTC
Working on testing this with the help of Biblibre.
Comment 7 Katrin Fischer 2014-07-20 16:58:26 UTC
I can't get this to work, even with Matthias help it seems. I'd be glad if someone else willing to test could contact Matthias for the required certificate and give this a try.
Comment 8 Katrin Fischer 2014-07-24 20:20:10 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2014-07-25 22:59:55 UTC
I'm not sure about CAS specific stuff, but there are problems with this patch:

- No regression tests.
- Koha's code usually expects OPACBaseUrl not to contain "http://" in front of it. So, something's wrong.

Could you please at least provide a sample call to the function where all the assertions (like "url_param() returns the deleted params too"). Think of a black box test.

My feeling is that CAS working with this patch is a side effect of another problem.

I am open to comments and willing to push this ASAP.
Comment 10 Matthias Meusburger 2014-07-28 14:39:45 UTC
Hi,

About OPACBaseURL, as already stated in bug 7770 : 

To my opinion, http:// should not be added automatically, for one simple reason: it just might not be http:// !

OPAC might be using https, and in that case, automatic prefill would be wrong.

When talking about CAS, some CAS servers won't allow authentication for http services, only for https one, so we have to be able to use https for the OPAC.
Comment 11 Katrin Fischer 2014-07-28 14:49:29 UTC
OpacBaseUrl is kind of an annoying system preference - lots of bugs about it.

If the CAS code only allows for https://, maybe it could be part of the CAS code?
Comment 12 Martin Renvoize 2014-07-28 15:54:43 UTC
Hmm.. bug 8952 and bug 11575, both pushed the master are doing 'stuff' to the prefix in opacbaseurl.. I think it worth looking at those here.

For Shibboleth, it is recognised practice to throw away not https requests at the IdP end for security reasons.. so for this reason I've forced https in my patches there.. I don't know if that's the case for CAS but maybe worth considering.

As for the bigger problem.. the two bugs above seem to come close at least to fixing that mentioned in bug 7770.
Comment 13 Matthias Meusburger 2014-07-29 07:35:18 UTC
Katrin, the koha CAS code (client-side) allows to use http or https, what I said is that some CAS servers only allow https. But I think we should keep both in koha.
Comment 14 Katrin Fischer 2014-07-30 07:02:31 UTC
Hi Matthias, 

I was only trying to suggest a possible solution here. Tomas is right in that the CAS code requires the OpacBaseURL parameter to be filled out differently than other places in Koha expect it to be and also different to documentation. I am not sure if it won't break other things actually. I passed it anyway, because I felt that this problem was introduced earlier and was not part of the problem that the patch was trying to solve and I didn't want to leave the CAS authentication broken.
Comment 15 Martin Renvoize 2014-07-30 08:12:17 UTC
Hi Matthias, Katrin, 

So.. the requirement to include http/https in the OpacBaseURL is not a regression caused by this patch. I personally still feel it's wrong, but I can't see it  as a reason to fail this particular patch.

However, agreeing with Tomas's original point. Could you/julian provide some test code that proves the original assumption regarding url_param not taking note of deleted params?  Neither me, nor Tomas could prove this in our tests so we're not really sure that this patch fixes the issue as  coincidence, or is actually solving the problem outright.

As for the OPACBaseURL stuff, we should move that conversation to one of the other related bugs and maybe open a further bug to remind us that it should also be resolved for the CAS case.
Comment 16 Julian Maurice 2014-07-30 12:03:50 UTC Comment hidden (obsolete)
Comment 17 Tomás Cohen Arazi 2014-07-30 12:44:59 UTC
Back to signed off, the QA team should mark it passed-qa as soon as the test is tidied so I push it.

FTR: I'd put the test in the t/db_dependent dir, and use something like this for the test description (missing in the current one):

is(C4::Auth_with_cas::_url_with_get_params($cgi),
   "$opac_base_url/cgi-bin/koha/opac-user.pl?bar=baz",
   "_url_with_get_params should return URL without deleted parameters (Bug 12398)");
Comment 18 Julian Maurice 2014-07-30 12:47:47 UTC Comment hidden (obsolete)
Comment 19 Julian Maurice 2014-07-30 12:49:17 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2014-07-30 20:58:35 UTC
I am sorry, but the patch fails for me when I turn off MySQL.

bumblebee:~/kohaclone (91-12398-cas) $ perl t/Auth_with_cas.t 
1..1
DBI connect('dbname=koha;host=localhost;port=3306','katrin',...) failed: Can't connect to local MySQL server through socket '/var/run/mysqld/mysqld.sock' (2) at /home/katrin/kohaclone/C4/Context.pm line 802.
Can't connect to local MySQL server through socket '/var/run/mysqld/mysqld.sock' (2) at /home/katrin/kohaclone/C4/Context.pm line 802.
Compilation failed in require at t/Auth_with_cas.t line 7.
BEGIN failed--compilation aborted at t/Auth_with_cas.t line 7.
# Looks like your test exited with 255 before it could output anything.
Comment 21 Julian Maurice 2014-07-31 07:11:11 UTC
This is due to C4::Context->preference being called in C4::Auth_with_cas outside of any functions (it gets called on "use").
Rather than writing a workaround for this, I will move the test in t/db_dependent.
Comment 22 Julian Maurice 2014-07-31 07:12:13 UTC Comment hidden (obsolete)
Comment 23 Katrin Fischer 2014-08-01 07:53:38 UTC
Created attachment 30442 [details] [review]
[PASSED QA] Bug 12398: Fix CAS authentication validation

CGI::url_param() also returns deleted parameters so we have to check
with CGI::param() too.

Signed-off-by: Matthias Meusburger <matthias.meusburger@biblibre.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Took a while to get it working, but I can confirm CAS login is not
working without this patch, but does with it.

Some notes:
In order for this to work you have to add http:// in front of your
OpacBaseURL.
You will also need a CAS test server and install the certificate
on your system.

Tested with CAS test server provided by Biblibre.
Comment 24 Katrin Fischer 2014-08-01 07:53:56 UTC
Created attachment 30443 [details] [review]
[PASSED QA] Bug 12398: Add test for C4::Auth_with_cas::_url_with_get_params

Run `prove t/db_dependent/Auth_with_cas.t` to run the test

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 25 Katrin Fischer 2014-08-01 07:54:52 UTC
I took the liberty to squash the last 2 patches for the test - both from Julian. As one added a file and the second moved it, this is nicer to read now :)
Comment 26 Tomás Cohen Arazi 2014-08-01 13:12:44 UTC
Created attachment 30454 [details] [review]
Bug 12398: (followup) tidy unit tests

This followup removes unnecesary warnings generated by the test,
and also prints a proper message for the tests.

Added a can_ok test for all the exported functions btw.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 27 Tomás Cohen Arazi 2014-08-01 13:15:03 UTC
Patches pushed to master.

Thanks Julian!
Comment 28 Fridolin SOMERS 2014-10-17 10:11:07 UTC
Can this be pushed to 3.16.x ?

I have feedbacks from 3.14 libraries that use CAS, its broken and corrected by this patch.