Bug 28417 - Authen::CAS::Client always loaded even if CAS is not used
Summary: Authen::CAS::Client always loaded even if CAS is not used
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks: 28410
  Show dependency treegraph
 
Reported: 2021-05-21 12:26 UTC by Jonathan Druart
Modified: 2021-11-05 18:19 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00


Attachments
Bug 28417: Don't use C4::Auth_with_cas if CAS is not used (2.39 KB, patch)
2021-05-21 12:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28417: Don't use C4::Auth_with_cas if CAS is not used (2.44 KB, patch)
2021-10-29 18:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28417: (follow-up) Fix typo (2.51 KB, patch)
2021-10-29 19:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28417: Don't use C4::Auth_with_cas if CAS is not used (2.49 KB, patch)
2021-10-29 23:23 UTC, David Nind
Details | Diff | Splinter Review
Bug 28417: (follow-up) Fix typo (2.56 KB, patch)
2021-10-29 23:24 UTC, David Nind
Details | Diff | Splinter Review
Bug 28417: Don't require C4::Auth_with_cas from opac-user if not needed (942 bytes, patch)
2021-11-03 12:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28417: Don't require C4::Auth_with_cas from opac-user if not needed (1.76 KB, patch)
2021-11-03 13:21 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-05-21 12:26:23 UTC

    
Comment 1 Jonathan Druart 2021-05-21 12:28:05 UTC
Created attachment 121276 [details] [review]
Bug 28417: Don't use C4::Auth_with_cas if CAS is not used

We should follow the same pattern as ldap and not use C4::Auth_with_cas
if the cas syspref is not turned on.
Comment 2 Jonathan Druart 2021-05-21 12:28:51 UTC
This script shows that 3Mb is saved in RAM

use Memory::Usage;
my $mu = Memory::Usage->new();
$mu->record('Loading');
require Koha::Biblios;
$mu->record('module loaded');
print $mu->report();
Comment 3 Jonathan Druart 2021-05-21 12:29:39 UTC
Test plan: confirm that CAS is still working
Comment 4 David Cook 2021-05-24 22:56:46 UTC
(In reply to Jonathan Druart from comment #3)
> Test plan: confirm that CAS is still working

What's the suggested way to test CAS?

I've never used it but I think I thought about trying out https://hub.docker.com/r/apereo/cas/ a little while ago...
Comment 5 Jonathan Druart 2021-06-07 10:18:48 UTC
(In reply to David Cook from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > Test plan: confirm that CAS is still working
> 
> What's the suggested way to test CAS?

I don't know, Tomas promised me to test this patch.
Comment 6 Tomás Cohen Arazi 2021-10-29 18:15:19 UTC
Created attachment 127103 [details] [review]
Bug 28417: Don't use C4::Auth_with_cas if CAS is not used

We should follow the same pattern as ldap and not use C4::Auth_with_cas
if the cas syspref is not turned on.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2021-10-29 19:08:27 UTC
This actually fixes something bigger, because C4::Auth was using a couple methods that weren't exported correctly.
Comment 8 Tomás Cohen Arazi 2021-10-29 19:47:31 UTC
Created attachment 127104 [details] [review]
Bug 28417: (follow-up) Fix typo

- There are CAS demo servers, pick one [1]
- They all seem to have the same user/pass as of writing this:
  User: casuser
  Password: Mellon

To test:
1. Enable CAS and all related preferences. I chose v3 or higher, it only affects the logout action.
2. Set the CAS URL: https://casserver.herokuapp.com/cas (note that if you click on any of the examples, they include a trailing /login, don't put it).
3. Generate a Koha user, with userid: casuser (I also used that for cardnumber, just in case). Set no password to the user, or its irrelevant anyway. Add it stff permissions.
4. Open your Koha OPAC, I use KTD thus:
   http://kohadev.myDNSname.org:8080
5. Try to login using CAS
=> SUCCESS: You are redirected to the right login page
6. Login using casuser/Mellon as advertised in the 'demos' site.
=> SUCCESS: You are logged in!
7. Now logout
=> SUCCESS: You are sent to CAS so they know you wanted to logout (if you enabled CAS logout)
8. Repeat on the staff interface.
=> SUCCESS: Things work!
9. Apply this patches, restart all the things:
   $ restart_all
10. Repeat all in OPAC and admin interfaces
=> SUCCESS: Things work!
11. Sign off :-D

[1] https://apereo.github.io/cas/Demos.html

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2021-10-29 19:55:52 UTC
I took the liberty to straight PQA. but I'm CC'ing the other David just in case he can give it a try in case I missed some use case.

Also, having a simple way to use demo servers will help with other CAS reports. So I want everyone to be familiar.
Comment 10 Tomás Cohen Arazi 2021-10-29 19:56:33 UTC
(In reply to Jonathan Druart from comment #5)
> (In reply to David Cook from comment #4)
> > (In reply to Jonathan Druart from comment #3)
> > > Test plan: confirm that CAS is still working
> > 
> > What's the suggested way to test CAS?
> 
> I don't know, Tomas promised me to test this patch.

Sorry for the delay :-/
Comment 11 David Nind 2021-10-29 23:23:42 UTC
Created attachment 127105 [details] [review]
Bug 28417: Don't use C4::Auth_with_cas if CAS is not used

We should follow the same pattern as ldap and not use C4::Auth_with_cas
if the cas syspref is not turned on.

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2021-10-29 23:24:03 UTC
Created attachment 127106 [details] [review]
Bug 28417: (follow-up) Fix typo

- There are CAS demo servers, pick one [1]
- They all seem to have the same user/pass as of writing this:
  User: casuser
  Password: Mellon

To test:
1. Enable CAS and all related preferences. I chose v3 or higher, it only affects the logout action.
2. Set the CAS URL: https://casserver.herokuapp.com/cas (note that if you click on any of the examples, they include a trailing /login, don't put it).
3. Generate a Koha user, with userid: casuser (I also used that for cardnumber, just in case). Set no password to the user, or its irrelevant anyway. Add it stff permissions.
4. Open your Koha OPAC, I use KTD thus:
   http://kohadev.myDNSname.org:8080
5. Try to login using CAS
=> SUCCESS: You are redirected to the right login page
6. Login using casuser/Mellon as advertised in the 'demos' site.
=> SUCCESS: You are logged in!
7. Now logout
=> SUCCESS: You are sent to CAS so they know you wanted to logout (if you enabled CAS logout)
8. Repeat on the staff interface.
=> SUCCESS: Things work!
9. Apply this patches, restart all the things:
   $ restart_all
10. Repeat all in OPAC and admin interfaces
=> SUCCESS: Things work!
11. Sign off :-D

[1] https://apereo.github.io/cas/Demos.html

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2021-10-30 01:04:56 UTC
(In reply to Tomás Cohen Arazi from comment #9)
> I took the liberty to straight PQA. but I'm CC'ing the other David just in
> case he can give it a try in case I missed some use case.
> 
> Also, having a simple way to use demo servers will help with other CAS
> reports. So I want everyone to be familiar.

I've added my sign off - not sure I'm the right person for use cases for authentication related things, but things worked as per the test plan.

Feel free to cc me in on any CAS bugs that required sign off - you always have great test plans!

A couple of minor things I noticed when testing:

1. After step 3 a restart_all is required, otherwise the link for CAS login is not displayed.

2. As mentioned in step 1 using CAS 3 or later for casServerVersion affects the logout action. I found this confusing as it sends you back to the CAS server, then when you click on login it logs you in to the CAS server rather than sending you to Koha - I'm sure there is a use case for that, and probably something those using a CAS environment would be familiar with. Changing casServerVersion to Version 2 or earlier means you get sent back to Koha.

3. For the staff interface the heading before the link is "Cas login", I think this should be "CAS login", but pretty minor really and is the how it is currently.
Comment 14 Katrin Fischer 2021-10-31 10:15:15 UTC
> 2. As mentioned in step 1 using CAS 3 or later for casServerVersion affects
> the logout action. I found this confusing as it sends you back to the CAS
> server, then when you click on login it logs you in to the CAS server rather
> than sending you to Koha - I'm sure there is a use case for that, and
> probably something those using a CAS environment would be familiar with.
> Changing casServerVersion to Version 2 or earlier means you get sent back to
> Koha.

Hm, it should always send you back to Koha. When you start the login process from Koha or any application its URL should be part of the link to the CAS server. It's then used to send you back after a successful login. Part of the problem with logout and the CAS versions is that this parameter name was changed.

Maybe the test server is CAS 2 and it doesn't work quite right with both settings because of that? We have a CAS 3 installation and it does work there (18.11 and 20.11). Looking at the patch set I think it looks like it did not change anything in that area.
Comment 15 Jonathan Druart 2021-11-03 12:43:08 UTC
Tomas, I think your patch is adding back the problem in opac-user.pl.

What about the following change?

diff --git a/opac/opac-user.pl b/opac/opac-user.pl
index 6feb144e508..e610cabc2ab 100755
--- a/opac/opac-user.pl
+++ b/opac/opac-user.pl
@@ -57,8 +57,10 @@ my $query = CGI->new;
 
 # CAS single logout handling
 # Will print header and exit
-require C4::Auth_with_cas;
-C4::Context->preference('casAuthentication') and C4::Auth_with_cas::logout_if_required($query);
+if ( C4::Context->preference('casAuthentication') ) {
+    require C4::Auth_with_cas;
+    C4::Auth_with_cas::logout_if_required($query);
+}
 
 my ( $template, $borrowernumber, $cookie ) = get_template_and_user(
     {
Comment 16 Jonathan Druart 2021-11-03 12:43:49 UTC
Created attachment 127249 [details] [review]
Bug 28417: Don't require C4::Auth_with_cas from opac-user if not needed
Comment 17 Jonathan Druart 2021-11-03 13:21:34 UTC
Created attachment 127251 [details] [review]
Bug 28417: Don't require C4::Auth_with_cas from opac-user if not needed

Note that without the use URI::QueryParam statement in C4::Auth we get:
Can't locate object method "query_param_delete" via package "URI::_generic" at /kohadevbox/koha/C4/Auth.pm line 1254
Comment 18 Jonathan Druart 2021-11-03 14:30:52 UTC
Pushed to master for 21.11, thanks to everybody involved!