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.
Can we add a syspref to specify the CAS protocol version and have the behaviour adjust as required on each case?
It could work - if people are ok with that. Cleaner would be fixing the external module, but much harder to do :(
(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?
Thx, Tomas! https://rt.cpan.org/Ticket/Display.html?id=125833
(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.
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
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>
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>
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>
(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.
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);
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>
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>
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>
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>
Shouldn't the default for new installs be 3?
Pushed to master for 21.05, thanks to everybody involved!
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.
Pushed to 20.11.x for 20.11.06
Pushed to 20.05.x for 20.05.12
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.