Bug 20854 - Redirect after logout with CAS 3.0 broken
Summary: Redirect after logout with CAS 3.0 broken
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Matthias Meusburger
QA Contact: Testopia
URL:
Keywords: Manual
Depends on:
Blocks:
 
Reported: 2018-06-01 11:33 UTC by Katrin Fischer
Modified: 2021-05-24 16:58 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
This patch adds a new system preference casServerVersion, that will allow Koha to work correctly with different CAS protocol versions. In this case it fixes a problem that arose by changing the name of a parameter in the logout request between CAS 2 and 3 that broke the redirect after successful logout.
Version(s) released in:
21.05.00,20.11.06,20.05.12


Attachments
Bug 20854: Allow correct redirect on logout for Cas servers 3.0 and superior. (5.70 KB, patch)
2020-10-27 11:20 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 20854: Allow correct redirect on logout for Cas servers 3.0 and superior. (5.79 KB, patch)
2021-03-16 19:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20854: Add more CAS tests (3.57 KB, patch)
2021-03-16 19:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20854: (QA follow-up) Improve readability (2.30 KB, patch)
2021-03-16 19:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20854: Allow correct redirect on logout for Cas servers 3.0 and superior. (5.79 KB, patch)
2021-04-17 13:41 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20854: Add more CAS tests (3.63 KB, patch)
2021-04-17 13:41 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20854: (QA follow-up) Improve readability (2.35 KB, patch)
2021-04-17 13:41 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20854: (QA follow-up) Rephrase of the new casServerVersion pref description (1.64 KB, patch)
2021-04-17 13:41 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2018-06-01 11:33:55 UTC
There has been a change in the parameters for the logout request between CAS 2.0 und CAS 3.0:

Redirect URL parameter
CAS 2.0: url
CAS 3.0: service

The CAS module used by Koha officially only support CAS 2.0, so when you logout from a CAS 3.0 server now, the redirect to Koha will be broken.

See here:
https://github.com/apereo/cas/blob/master/docs/cas-server-documentation/protocol/CAS-Protocol-Specification.md

Note: The url parameter defined in the former CAS 2.0 specification is not a valid parameter in CAS 3.0 anymore. CAS Servers MUST ignore given url parameters. A CAS client MAY provide the service parameter as described above, as this ensures the parameter is validated against the registered service URLs when operating in non-open mode. See 2.3.2 for details.

We are using the following 'hack' to make the redirect work correctly:

C4/Auth_with_cas.pm
$uri =~ s/\?logout\.x=1//; # We don't want to keep triggering a logout, if we got here, the borrower is already logged out of Koha
-    print $query->redirect( $cas->logout_url(url => $uri));
+    my $logout_url = $cas->logout_url(url => $uri);
+    $logout_url =~ s/url/service/;
+    print $query->redirect( $logout_url );

We won't have time to fix this properly, but wanted to document problem and workaround at least.
Comment 1 Tomás Cohen Arazi 2018-07-12 12:01:07 UTC
Can we add a syspref to specify the CAS protocol version and have the behaviour adjust as required on each case?
Comment 2 Katrin Fischer 2018-07-12 13:33:20 UTC
It could work - if people are ok with that. Cleaner would be fixing the external module, but much harder to do :(
Comment 3 Tomás Cohen Arazi 2018-07-12 13:37:42 UTC
(In reply to Katrin Fischer from comment #2)
> It could work - if people are ok with that. Cleaner would be fixing the
> external module, but much harder to do :(

Is there a bug filed for the Authen::CAS::Client library?
Comment 4 Katrin Fischer 2018-07-12 14:10:08 UTC
Thx, Tomas!

https://rt.cpan.org/Ticket/Display.html?id=125833
Comment 5 Tomás Cohen Arazi 2018-07-12 14:14:41 UTC
(In reply to Katrin Fischer from comment #4)
> Thx, Tomas!
> 
> https://rt.cpan.org/Ticket/Display.html?id=125833

I still think we should have a 'CASVersion' syspref that controls this behaviour, until there's a response from the library maintainer.
Comment 6 Matthias Meusburger 2020-10-27 11:20:45 UTC
Created attachment 112561 [details] [review]
Bug 20854: Allow correct redirect on logout for Cas servers 3.0 and superior.

Test plan:

 1) Apply the patch
 2) Set the system preference casLogout to "Yes"
 3) Set the new system preference CasServerVersion to "CAS 3 or superior"
 4) Check that you are redirected to Koha after a CAS logout from a CAS 3 server
 5) Set the new system preference CasServerVersion to "CAS 2 or inferior"
 6) Check that you are redirected to Koha after a CAS logout from a CAS 2 server
Comment 7 Tomás Cohen Arazi 2021-03-16 19:59:18 UTC
Created attachment 118347 [details] [review]
Bug 20854: Allow correct redirect on logout for Cas servers 3.0 and superior.

Test plan:

 1) Apply the patch
 2) Set the system preference casLogout to "Yes"
 3) Set the new system preference CasServerVersion to "CAS 3 or superior"
 4) Check that you are redirected to Koha after a CAS logout from a CAS 3 server
 5) Set the new system preference CasServerVersion to "CAS 2 or inferior"
 6) Check that you are redirected to Koha after a CAS logout from a CAS 2 server

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2021-03-16 19:59:26 UTC
Created attachment 118348 [details] [review]
Bug 20854: Add more CAS tests

This patch aims to add full test coverage for 'logout_cas' the way we do
it nowadays (in a subtest, mocking things, etc). It is provided as a
separate patch so we can test the follow-up tidy and see no behavior
change.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Auth_with_cas.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Apply the refactoring follow-up
5. Repeat 3
=> SUCCESS: Tests still pass!
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2021-03-16 19:59:33 UTC
Created attachment 118349 [details] [review]
Bug 20854: (QA follow-up) Improve readability

The logout_cas method does too little tasks. In this case, I consider we
should not split it into a separate method. As the CAS implementation
lacked tests, it made sense to do it like this, so it is testable. But
now we have higher-level tests for logout_cas, we can bake the behavior
inside of it, and it is testable.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2021-03-16 20:26:46 UTC
(In reply to Tomás Cohen Arazi from comment #5)
> (In reply to Katrin Fischer from comment #4)
> > Thx, Tomas!
> > 
> > https://rt.cpan.org/Ticket/Display.html?id=125833
> 
> I still think we should have a 'CASVersion' syspref that controls this
> behaviour, until there's a response from the library maintainer.

Still no response, BTW.
Comment 11 Katrin Fischer 2021-04-17 13:33:23 UTC
For comparison, this is the change we have been using in production for years now:

-    print $query->redirect( $cas->logout_url(url => $uri));
+    my $logout_url = $cas->logout_url(url => $uri);
+    $logout_url =~ s/url/service/;
+    print $query->redirect( $logout_url );

This is from the patch:

+    my $logout_url = $cas->logout_url( url => $uri );
+    $logout_url =~ s/url=/service=/
+      if C4::Context->preference('casServerVersion') eq '3';
+
+    print $query->redirect($logout_url);
Comment 12 Katrin Fischer 2021-04-17 13:41:23 UTC
Created attachment 119789 [details] [review]
Bug 20854: Allow correct redirect on logout for Cas servers 3.0 and superior.

Test plan:

 1) Apply the patch
 2) Set the system preference casLogout to "Yes"
 3) Set the new system preference CasServerVersion to "CAS 3 or superior"
 4) Check that you are redirected to Koha after a CAS logout from a CAS 3 server
 5) Set the new system preference CasServerVersion to "CAS 2 or inferior"
 6) Check that you are redirected to Koha after a CAS logout from a CAS 2 server

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Katrin Fischer 2021-04-17 13:41:27 UTC
Created attachment 119790 [details] [review]
Bug 20854: Add more CAS tests

This patch aims to add full test coverage for 'logout_cas' the way we do
it nowadays (in a subtest, mocking things, etc). It is provided as a
separate patch so we can test the follow-up tidy and see no behavior
change.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Auth_with_cas.t
=> SUCCESS: Tests pass!
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Apply the refactoring follow-up
5. Repeat 3
=> SUCCESS: Tests still pass!
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2021-04-17 13:41:31 UTC
Created attachment 119791 [details] [review]
Bug 20854: (QA follow-up) Improve readability

The logout_cas method does too little tasks. In this case, I consider we
should not split it into a separate method. As the CAS implementation
lacked tests, it made sense to do it like this, so it is testable. But
now we have higher-level tests for logout_cas, we can bake the behavior
inside of it, and it is testable.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2021-04-17 13:41:36 UTC
Created attachment 119792 [details] [review]
Bug 20854: (QA follow-up) Rephrase of the new casServerVersion pref description

For another related change, see:
https://en.wikipedia.org/wiki/Central_Authentication_Service

CAS = Central Authentication Service

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Jonathan Druart 2021-04-21 08:35:20 UTC
Shouldn't the default for new installs be 3?
Comment 17 Jonathan Druart 2021-04-21 08:46:25 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 18 David Cook 2021-04-22 06:58:17 UTC
I didn't realize that people were still using the CAS protocol. So interesting.

By the way, it looks like there is a Docker container for the CAS server. It looks like it supports CAS, SAML, and OpenID Connect protocols, so this could be an interesting server to add to koha-testing-docker for testing.
Comment 19 Fridolin Somers 2021-04-29 12:01:46 UTC
Pushed to 20.11.x for 20.11.06
Comment 20 Andrew Fuerste-Henry 2021-05-24 15:55:50 UTC
Pushed to 20.05.x for 20.05.12
Comment 21 Victor Grousset/tuxayo 2021-05-24 16:58:36 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.