Bug 19160 - CAS Single Logout
Summary: CAS Single Logout
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low new feature
Assignee: Chris Cormack
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-21 15:21 UTC by Katrin Fischer
Modified: 2019-10-14 19:57 UTC (History)
6 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This adds support for the CAS Single Logout feature. Single logout means that the user gets logged out not only from the CAS Server, but also from all visited CAS client applications when logging out in one of them or after reaching a timeout. The CAS server has to be set up for single logout for this to take effect, otherwise behaviour will remain unchanged.
Version(s) released in:
Circulation function:

Attachments
Bug 19160 CAS Single logout (11.77 KB, patch)
2017-08-21 20:12 UTC, Chris Cormack 		Details | Diff | Splinter Review
[SIGNED OFF] Bug 19160 CAS Single logout (11.86 KB, patch)
2017-08-22 07:35 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Bug 19160 CAS Single logout (11.90 KB, patch)
2018-01-10 15:30 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 19160: Move duplicated code to its own private subroutine (4.26 KB, patch)
2018-01-11 18:46 UTC, Jonathan Druart 		Details | Diff | Splinter Review
Bug 19160: Move the code from controller to C4::Auth_with_cas (2.78 KB, patch)
2018-01-11 18:46 UTC, Jonathan Druart 		Details | Diff | Splinter Review
Bug 19160: Isolate CAS code into its own module (3.99 KB, patch)
2018-01-11 18:47 UTC, Jonathan Druart 		Details | Diff | Splinter Review
Bug 19160: CAS Single logout (11.90 KB, patch)
2018-02-16 17:07 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Bug 19160: Move duplicated code to its own private subroutine (4.26 KB, patch)
2018-02-16 17:07 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Bug 19160: CAS Single logout (11.96 KB, patch)
2018-02-16 17:09 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Bug 19160: Move duplicated code to its own private subroutine (4.32 KB, patch)
2018-02-16 17:09 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Bug 19160: Move the code from controller to C4::Auth_with_cas (2.83 KB, patch)
2018-02-16 17:09 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Bug 19160: Isolate CAS code into its own module (4.05 KB, patch)
2018-02-16 17:09 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Bug 19160: (follow-up) Fix problems introduced by renaming logout_required to logout_if_required (1.80 KB, patch)
2018-02-16 17:09 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Bug 19160: CAS Single logout (12.01 KB, patch)
2018-03-02 13:44 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 19160: Move duplicated code to its own private subroutine (4.37 KB, patch)
2018-03-02 13:44 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 19160: Move the code from controller to C4::Auth_with_cas (2.89 KB, patch)
2018-03-02 13:44 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 19160: Isolate CAS code into its own module (4.10 KB, patch)
2018-03-02 13:44 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 19160: (follow-up) Fix problems introduced by renaming logout_required to logout_if_required (1.86 KB, patch)
2018-03-02 13:44 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 19160: Remove tab characters causing qa script to fail (1019 bytes, patch)
2018-03-02 13:44 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Bug 19160: Add POD (1.07 KB, patch)
2018-03-02 13:44 UTC, Kyle M Hall (khall) 		Details | Diff | Splinter Review
Show Obsolete (13) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2017-08-21 15:21:53 UTC 
CAS allows not only Single Sign On, but also Single Log out. 

Specification:
https://apereo.github.io/cas/5.0.x/protocol/CAS-Protocol-Specification.html#233-single-logout

2.3.3 Single Logout

The CAS Server MAY support Single Logout (SLO). SLO means that the user gets logged out not only from the CAS Server, but also from all visited CAS client applications. If SLO is supported by the CAS Server, the CAS Server MUST send a HTTP POST request containing a logout XML document (see Appendix C) to all service URLs provided to CAS during this CAS session whenever a Ticket Granting Ticket is explicitly expired by the user (e.g. during logout). CAS Clients that do not support the SLO POST requests MUST ignore these requests. SLO requests MAY also be initiated by the CAS Server upon TGT idle timeout.

Koha currently doesn't support this, but it should in order to improves patron privacy and data security.
Comment 1 Chris Cormack 2017-08-21 20:12:34 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
Comment 2 Katrin Fischer 2017-08-22 07:35:57 UTC 
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.
Comment 3 Katrin Fischer 2018-01-10 12:36:51 UTC 
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.
Comment 4 Kyle M Hall (khall) 2018-01-10 15:30:44 UTC 
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>
Comment 5 Jonathan Druart 2018-01-10 19:34:40 UTC 
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."
Comment 6 Chris Cormack 2018-01-10 19:43:55 UTC 
(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.
Comment 7 Jonathan Druart 2018-01-10 19:58:52 UTC 
(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.
Comment 8 Katrin Fischer 2018-01-10 21:42:44 UTC 
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.
Comment 9 Jonathan Druart 2018-01-11 18:46:44 UTC 
Created attachment 70440 [details] [review]
Bug 19160: Move duplicated code to its own private subroutine
Comment 10 Jonathan Druart 2018-01-11 18:46:57 UTC 
Created attachment 70441 [details] [review]
Bug 19160: Move the code from controller to C4::Auth_with_cas
Comment 11 Jonathan Druart 2018-01-11 18:47:02 UTC 
Created attachment 70442 [details] [review]
Bug 19160: Isolate CAS code into its own module
Comment 12 Jonathan Druart 2018-01-11 18:49:01 UTC 
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.
Comment 13 Chris Cormack 2018-01-12 01:36:37 UTC 
Looks good to me
Comment 14 Katrin Fischer 2018-01-19 14:09:44 UTC 
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.
Comment 15 Katrin Fischer 2018-02-12 16:37:32 UTC 
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.
Comment 16 Katrin Fischer 2018-02-16 17:07:20 UTC 
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>
Comment 17 Katrin Fischer 2018-02-16 17:07:23 UTC 
Created attachment 71830 [details] [review]
Bug 19160: Move duplicated code to its own private subroutine
Comment 18 Katrin Fischer 2018-02-16 17:09:40 UTC 
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>
Comment 19 Katrin Fischer 2018-02-16 17:09:44 UTC 
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>
Comment 20 Katrin Fischer 2018-02-16 17:09:48 UTC 
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>
Comment 21 Katrin Fischer 2018-02-16 17:09:52 UTC 
Created attachment 71834 [details] [review]
Bug 19160: Isolate CAS code into its own module

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 22 Katrin Fischer 2018-02-16 17:09:56 UTC 
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>
Comment 23 Katrin Fischer 2018-02-16 17:12:03 UTC 
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!
Comment 24 Kyle M Hall (khall) 2018-03-02 13:44:16 UTC 
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>
Comment 25 Kyle M Hall (khall) 2018-03-02 13:44:24 UTC 
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>
Comment 26 Kyle M Hall (khall) 2018-03-02 13:44:27 UTC 
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>
Comment 27 Kyle M Hall (khall) 2018-03-02 13:44:31 UTC 
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>
Comment 28 Kyle M Hall (khall) 2018-03-02 13:44:34 UTC 
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>
Comment 29 Kyle M Hall (khall) 2018-03-02 13:44:38 UTC 
Created attachment 72363 [details] [review]
Bug 19160: Remove tab characters causing qa script to fail

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Kyle M Hall (khall) 2018-03-02 13:44:41 UTC 
Created attachment 72364 [details] [review]
Bug 19160: Add POD

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Katrin Fischer 2018-03-04 12:18:25 UTC 
Sorry for missing the last 2 and thx for fixing them!
Comment 32 Jonathan Druart 2018-03-19 18:52:11 UTC 
Should this considered as a new feature (instead of enhancement)?
Comment 33 Chris Cormack 2018-03-19 18:54:32 UTC 
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
Comment 34 Jonathan Druart 2018-03-23 15:35:53 UTC 
Pushed to master for 18.05, thanks to everybody involved!
Comment 35 Nick Clemens (kidclamp) 2018-03-26 11:58:59 UTC 
Hmm...should this be considered for backport - I like the idea of fixing CAS issues, but don't like futzing with Auth code :-)
Comment 36 Katrin Fischer 2018-03-26 12:07:59 UTC 
We have tested with older versions and it should work, but it can also be considered a new feature - grey area, I'd say.