Description
Katrin Fischer
2017-08-21 15:21:53 UTC
Created attachment 66285 [details] [review] Bug 19160 CAS Single logout CAS supports single logout, where if you logout of one application it logs you out of all of them. This bug implements this You will need a CAS server (with single logout configure), and at least 2 applications (one being Koha) 1/ In Koha login via CAS 2/ Login to the other application via CAS 3/ Logout of the other application 4/ Notice you are still logged into Koha 5/ Log out of Koha 6/ Apply patch 7/ Login to Koha via CAS, login to other app via CAS 8/ Log out of other app 9/ Notice you are logged out of Koha If you dont have CAS, this patch should be a no op, you could test that 1/ Login and logout normally 2/ Apply patch 3/ Login and logout still work fine Created attachment 66306 [details] [review] [SIGNED OFF] Bug 19160 CAS Single logout CAS supports single logout, where if you logout of one application it logs you out of all of them. This bug implements this You will need a CAS server (with single logout configure), and at least 2 applications (one being Koha) 1/ In Koha login via CAS 2/ Login to the other application via CAS 3/ Logout of the other application 4/ Notice you are still logged into Koha 5/ Log out of Koha 6/ Apply patch 7/ Login to Koha via CAS, login to other app via CAS 8/ Log out of other app 9/ Notice you are logged out of Koha If you dont have CAS, this patch should be a no op, you could test that 1/ Login and logout normally 2/ Apply patch 3/ Login and logout still work fine Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Patch works as described, local login still works correctly. We have since used these patches successfully in production for several months, at first as a backport to 3.22, now in 16.11. No issues so far. The CAS logout will only happen if the CAS server is set up to sent a logout request. Created attachment 70408 [details] [review] Bug 19160 CAS Single logout CAS supports single logout, where if you logout of one application it logs you out of all of them. This bug implements this You will need a CAS server (with single logout configure), and at least 2 applications (one being Koha) 1/ In Koha login via CAS 2/ Login to the other application via CAS 3/ Logout of the other application 4/ Notice you are still logged into Koha 5/ Log out of Koha 6/ Apply patch 7/ Login to Koha via CAS, login to other app via CAS 8/ Log out of other app 9/ Notice you are logged out of Koha If you dont have CAS, this patch should be a no op, you could test that 1/ Login and logout normally 2/ Apply patch 3/ Login and logout still work fine Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Patch works as described, local login still works correctly. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> I am not going to test this patch, but I would like to make sure the changes make sense. 1. I do not understand the code in opac-user.pl, why is it here? It sounds like it will be better at its place in C4::Auth. 2. You should move out get_session the code you copy/pasted from there 3. I guess it is just my undestanding of CAS that is bad but why do we call get_session then delete it? Is cas_ticket == sessionID? Side note: it will not work if SessionStorage=memcached (but we should remove that), CGI::Session::Driver::memcached->traverse does nothing. Another one: from the pod of CGI::Session->find : "Experimental feature. Note: find() is meant to be convenient, not necessarily efficient. It's best suited in cron scripts." (In reply to Jonathan Druart from comment #5) > > Another one: from the pod of CGI::Session->find : > "Experimental feature. > Note: find() is meant to be convenient, not necessarily efficient. It's best > suited in cron scripts." Yes, but it is still a lot faster than opening every session and checking them all by hand. It's only called when a user logs out of another application other than Koha, and only by the CAS server, never by the user. So it is essentially a background process. (In reply to Jonathan Druart from comment #5) > 3. I guess it is just my undestanding of CAS that is bad but why do we call > get_session then delete it? > Is cas_ticket == sessionID? And of course it's not. Hi Joubu, trying to answer some of your questions: > 1. I do not understand the code in opac-user.pl, why is it here? It sounds > like it will be better at its place in C4::Auth. opac-user.pl is the script that the logout request will be sent to in our scenario. Would it work to create a new routine get_cas_session() in Auth.pm that is called from opac-user.pl? > 2. You should move out get_session the code you copy/pasted from there Can you point me to the bit of code you mean? Changing get_session seems a bit more dangerous than I was hoping for. > 3. I guess it is just my undestanding of CAS that is bad but why do we call > get_session then delete it? > Is cas_ticket == sessionID? Is this one ok with comment#7? Or can you explain a bit more? > Side note: it will not work if SessionStorage=memcached (but we should > remove that), CGI::Session::Driver::memcached->traverse does nothing We don't use memcached for session storage - so not sure how to test. Is this a blocker? Hea shows only 22 people using memcached for sessionStorage (https://hea.koha-community.org/systempreferences). > Another one: from the pod of CGI::Session->find : > "Experimental feature. > Note: find() is meant to be convenient, not necessarily efficient. It's best > suited in cron scripts. I think Chris answer here is better than mine could be :) It happens in the background, so not a problem if it's a bit slow as the user won't wait for it. Created attachment 70440 [details] [review] Bug 19160: Move duplicated code to its own private subroutine Created attachment 70441 [details] [review] Bug 19160: Move the code from controller to C4::Auth_with_cas Created attachment 70442 [details] [review] Bug 19160: Isolate CAS code into its own module This is what I had in mind. The first 2 patches just move code, the last one may be problematic if I missed something in the logic. In discussion. Please obsolete these patches if you prefer another QAer opinion or switch back to NSO if you agree with testing them. Looks good to me Some first feedback: We successfully tested the patches on our 16.11.x branch, but want to run some more tests. So far it's looking good! We will try to sign off before end of January. Found a small issue. The last patch renamed the routine logout_required to logout_if_required, but misses to change the import: C4/Auth.pm: import C4::Auth_with_cas qw(check_api_auth_cas checkpw_cas login_cas logout_cas login_cas_url logout_required); We'll fix locally and continue testing. Created attachment 71829 [details] [review] Bug 19160: CAS Single logout CAS supports single logout, where if you logout of one application it logs you out of all of them. This bug implements this You will need a CAS server (with single logout configure), and at least 2 applications (one being Koha) 1/ In Koha login via CAS 2/ Login to the other application via CAS 3/ Logout of the other application 4/ Notice you are still logged into Koha 5/ Log out of Koha 6/ Apply patch 7/ Login to Koha via CAS, login to other app via CAS 8/ Log out of other app 9/ Notice you are logged out of Koha If you dont have CAS, this patch should be a no op, you could test that 1/ Login and logout normally 2/ Apply patch 3/ Login and logout still work fine Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Patch works as described, local login still works correctly. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 71830 [details] [review] Bug 19160: Move duplicated code to its own private subroutine Created attachment 71831 [details] [review] Bug 19160: CAS Single logout CAS supports single logout, where if you logout of one application it logs you out of all of them. This bug implements this You will need a CAS server (with single logout configure), and at least 2 applications (one being Koha) 1/ In Koha login via CAS 2/ Login to the other application via CAS 3/ Logout of the other application 4/ Notice you are still logged into Koha 5/ Log out of Koha 6/ Apply patch 7/ Login to Koha via CAS, login to other app via CAS 8/ Log out of other app 9/ Notice you are logged out of Koha If you dont have CAS, this patch should be a no op, you could test that 1/ Login and logout normally 2/ Apply patch 3/ Login and logout still work fine Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Patch works as described, local login still works correctly. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 71832 [details] [review] Bug 19160: Move duplicated code to its own private subroutine Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 71833 [details] [review] Bug 19160: Move the code from controller to C4::Auth_with_cas Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 71834 [details] [review] Bug 19160: Isolate CAS code into its own module Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 71835 [details] [review] Bug 19160: (follow-up) Fix problems introduced by renaming logout_required to logout_if_required Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> We found some small issues in testing as side effects of the rewrite, fixed in a follow-up. Kyle, could you take another look? Thx! Created attachment 72358 [details] [review] Bug 19160: CAS Single logout CAS supports single logout, where if you logout of one application it logs you out of all of them. This bug implements this You will need a CAS server (with single logout configure), and at least 2 applications (one being Koha) 1/ In Koha login via CAS 2/ Login to the other application via CAS 3/ Logout of the other application 4/ Notice you are still logged into Koha 5/ Log out of Koha 6/ Apply patch 7/ Login to Koha via CAS, login to other app via CAS 8/ Log out of other app 9/ Notice you are logged out of Koha If you dont have CAS, this patch should be a no op, you could test that 1/ Login and logout normally 2/ Apply patch 3/ Login and logout still work fine Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Patch works as described, local login still works correctly. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 72359 [details] [review] Bug 19160: Move duplicated code to its own private subroutine Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 72360 [details] [review] Bug 19160: Move the code from controller to C4::Auth_with_cas Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 72361 [details] [review] Bug 19160: Isolate CAS code into its own module Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 72362 [details] [review] Bug 19160: (follow-up) Fix problems introduced by renaming logout_required to logout_if_required Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 72363 [details] [review] Bug 19160: Remove tab characters causing qa script to fail Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 72364 [details] [review] Bug 19160: Add POD Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Sorry for missing the last 2 and thx for fixing them! Should this considered as a new feature (instead of enhancement)? It could be, but really it's just trying to complete the CAS spec you could equally consider it a Bugfix :) Might be nicer for the release notes to say new feature though Pushed to master for 18.05, thanks to everybody involved! Hmm...should this be considered for backport - I like the idea of fixing CAS issues, but don't like futzing with Auth code :-) We have tested with older versions and it should work, but it can also be considered a new feature - grey area, I'd say. |