Summary: | Redirect after logout with CAS 3.0 broken | ||
---|---|---|---|
Product: | Koha | Reporter: | Katrin Fischer <katrin.fischer> |
Component: | Authentication | Assignee: | Matthias Meusburger <matthias.meusburger> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | normal | ||
Priority: | P5 - low | CC: | andrewfh, dcook, dpavlin, fridolin.somers, jonathan.druart, matthias.meusburger, tomascohen, victor |
Version: | master | Keywords: | Manual |
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
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.
Bug 20854: Allow correct redirect on logout for Cas servers 3.0 and superior. Bug 20854: Add more CAS tests Bug 20854: (QA follow-up) Improve readability Bug 20854: Allow correct redirect on logout for Cas servers 3.0 and superior. Bug 20854: Add more CAS tests Bug 20854: (QA follow-up) Improve readability Bug 20854: (QA follow-up) Rephrase of the new casServerVersion pref description |
Description
Katrin Fischer
2018-06-01 11:33:55 UTC
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? (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. |