Bug 11219 - CAS authentication fails with URL parameters
Summary: CAS authentication fails with URL parameters
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Fridolin Somers
QA Contact: Testopia
URL:
Keywords:
Depends on: 10029
Blocks: 12398
  Show dependency treegraph
 
Reported: 2013-11-08 10:11 UTC by Fridolin Somers
Modified: 2019-06-27 09:24 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11219 - CAS authentication fails with URL parameters (5.60 KB, patch)
2013-11-08 10:21 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 11219 - CAS authentication fails with URL parameters (5.66 KB, patch)
2013-11-25 14:58 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
[PASSED QA] Bug 11219 - CAS authentication fails with URL parameters (6.21 KB, patch)
2013-12-21 10:37 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11219 - Bug 11219 - CAS authentication fails with URL parameters - followup (2.16 KB, patch)
2014-04-10 11:26 UTC, Fridolin Somers
Details | Diff | Splinter Review
signed patch (2.23 KB, patch)
2014-04-10 12:42 UTC, Matthias Meusburger
Details | Diff | Splinter Review
[PASSED QA] Bug 11219 - CAS authentication fails with URL parameters (6.29 KB, patch)
2014-04-12 17:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11219 - CAS authentication fails with URL parameters - followup (2.29 KB, patch)
2014-04-12 17:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11219 - CAS authentication fails with URL parameters (6.33 KB, patch)
2014-04-14 15:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11219 - CAS authentication fails with URL parameters - followup (2.33 KB, patch)
2014-04-14 15:29 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2013-11-08 10:11:54 UTC
Bug 10029 tries to fix the use of URL parameters in CAS authentication.
But is does not work.
The full URL must be used in all methods of C4::Auth_with_cas.
Also, in checkpw_cas(), the 'ticket' parameter must be removed to find the original URL.
Comment 1 Fridolin Somers 2013-11-08 10:21:13 UTC Comment hidden (obsolete)
Comment 2 Biblibre Sandboxes 2013-11-25 14:58:12 UTC
Patch tested with a sandbox, by Koha team AMU <koha.aixmarseille@gmail.com>
Comment 3 Biblibre Sandboxes 2013-11-25 14:58:29 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2013-11-25 21:14:58 UTC
Hi Fridolin and Team AMU, 
is there a way to test this without a working CAS environment?
Comment 5 Fridolin Somers 2013-11-29 08:10:14 UTC
(In reply to Katrin Fischer from comment #4)
> Hi Fridolin and Team AMU, 
> is there a way to test this without a working CAS environment?

I don't know.
But I'd say same has Bug 10029.
Comment 6 Katrin Fischer 2013-12-21 10:37:44 UTC Comment hidden (obsolete)
Comment 7 Fridolin Somers 2014-04-10 11:26:13 UTC Comment hidden (obsolete)
Comment 8 Matthias Meusburger 2014-04-10 12:42:29 UTC Comment hidden (obsolete)
Comment 9 Fridolin Somers 2014-04-11 09:57:51 UTC
I set as critical because it may show a user/password
Comment 10 Katrin Fischer 2014-04-12 17:36:56 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2014-04-12 17:37:54 UTC
First patch is PQA, I signed off on the second patch.
Comment 12 Katrin Fischer 2014-04-12 17:54:47 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2014-04-14 15:28:53 UTC
Created attachment 27095 [details] [review]
Bug 11219 - CAS authentication fails with URL parameters

Bug 10029 tries to fix the use of URL parameters in CAS authentication.
But is does not work.
The full URL must be used in all methods of C4::Auth_with_cas.
Also, in checkpw_cas(), the 'ticket' parameter must be removed to find the original URL.

This patch removes the 'ticket' parameter from query before calling checkpw_cas() since the ticket is passed as method arguemnt.
In C4::Auth_with_cas, many methods use the same code to get the CAS handler and the service URI. This patch adds a private method _get_cas_and_service() to do the job.

Test plan:
- Enable CAS
- Go to opac without been logged-in
- Try to place hold on a record
=> You get to /cgi-bin/koha/opac-reserve.pl?biblionumber=XXX showing authentication page
=> Check that CAS link contains query param "biblionumber"
- Click on CAS link and log in
=> Check you return well logged-in to reserve page with biblionumber param
- Check CAS loggout
- Check Proxy CAS auth

Signed-off-by: Koha team AMU <koha.aixmarseille@gmail.com>

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests in t, xt, and t/db_dependent/Auth.t.
Also passes QA script.

As I have no working CAS server, I focused on regression testing:
Activated Persona and casAuthentication.
- Verified normal login against database still works.
- Verified Persona login works.
  Note: With Persona you are always forwarded to the patron
  account - so you have to search for the record again before
  you can place a hold.
- Verified that the CAS URL contains the biblionumber when
  logging in while placing a hold.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Retested 2014-04-12

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall 2014-04-14 15:29:09 UTC
Created attachment 27096 [details] [review]
Bug 11219 - CAS authentication fails with URL parameters - followup

This followup corrects the fact that when using $query->url(), both GET and POST params are get.
Using $query->url_param() will only get params directly in URL.

Test plan :
- Enable CAS
- Go to login page : cgi-bin/koha/opac-user.pl
- Try to connect with local login using random login and password (they will be transmitted by POST)
- You stay to login page
- Look at CAS login URL
=> Without this patch it will contain the random login and password as parameters of opac-user.pl
=> With this patch it does not contain any parameter

Signed-off-by: Matthias Meusburger <matthias.meusburger@biblibre.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Galen Charlton 2014-05-05 05:18:19 UTC
Pushed to master.  Thanks, Fridolin!
Comment 16 Fridolin Somers 2014-08-01 13:30:26 UTC
Pushed to 3.14.x, will be in 3.14.10
Comment 17 Mason James 2015-02-25 12:40:35 UTC
(In reply to Galen Charlton from comment #15)
> Pushed to master.  Thanks, Fridolin!

Patch already added to 3.16.x branch