Bug 5630 - CAS improvements
Summary: CAS improvements
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: 3.4
Hardware: All All
: PATCH-Sent (DO NOT USE) normal (vote)
Deadline: 2010-01-20
Assignee: Paul Poulain
QA Contact: Bugs List
URL:
Keywords:
: 6012 (view as bug list)
Depends on: 6012
Blocks: 18237 5983 5992 6694 8279
  Show dependency treegraph
 
Reported: 2011-01-19 20:21 UTC by Paul Poulain
Modified: 2019-10-02 19:32 UTC (History)
6 users (show)

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


Attachments
Proposed Patch (28.41 KB, patch)
2011-03-30 21:08 UTC, Chris Cormack
Details | Diff | Splinter Review
Follow up patch number 1 (5.25 KB, patch)
2011-03-30 21:13 UTC, Chris Cormack
Details | Diff | Splinter Review
Follow up 2 (3.62 KB, patch)
2011-03-30 21:29 UTC, Chris Cormack
Details | Diff | Splinter Review
Followup 3 (1.05 KB, patch)
2011-03-30 21:30 UTC, Chris Cormack
Details | Diff | Splinter Review
Followup 4 (1.04 KB, patch)
2011-03-30 21:31 UTC, Chris Cormack
Details | Diff | Splinter Review
Compilation errors and syspref edition (2.33 KB, patch)
2011-04-07 15:18 UTC, Matthias Meusburger
Details | Diff | Splinter Review
proposed patch squashed, rebased and signed-off (33.45 KB, patch)
2011-07-22 09:49 UTC, Paul Poulain
Details | Diff | Splinter Review
Follow up Patch (977 bytes, patch)
2011-08-30 14:44 UTC, Ian Walls
Details | Diff | Splinter Review
proposed patch follow-up 2 (2.17 KB, patch)
2011-09-02 16:22 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2011-01-19 20:21:48 UTC
* adding some documentation
* cleaning code
* adding a new feature to have multiple CAS server on a single Koha instance
Comment 1 Chris Cormack 2011-03-30 21:08:40 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2011-03-30 21:12:49 UTC
There are 4 more patches that could belong here but they do not apply.

Working on refactoring them and adding them as a follow up patch
Comment 3 Chris Cormack 2011-03-30 21:13:21 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2011-03-30 21:29:37 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2011-03-30 21:30:02 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2011-03-30 21:31:59 UTC Comment hidden (obsolete)
Comment 7 claire.hernandez@biblibre.com 2011-04-05 10:13:01 UTC
Follow up 2 and 3 gives me:

fatal: sha1 information is lacking or useless (C4/Auth.pm).
Repository lacks necessary blobs to fall back on 3-way merge.

so I applied others and on koha pro homepage I have:
Global symbol "$version" requires explicit package name at /home/koha/versions//koha_community/C4/Auth.pm line 573.
Global symbol "$version" requires explicit package name at /home/koha/versions//koha_community/C4/Auth.pm line 574.
Global symbol "$type" requires explicit package name at /home/koha/versions//koha_community/C4/Auth.pm line 575.
Global symbol "$query" requires explicit package name at /home/koha/versions//koha_community/C4/Auth.pm line 577.
Global symbol "$query" requires explicit package name at /home/koha/versions//koha_community/C4/Auth.pm line 580.
Unmatched right curly bracket at /home/koha/versions//koha_community/C4/Auth.pm line 584, at end of line
syntax error at /home/koha/versions//koha_community/C4/Auth.pm line 584, near "}"
syntax error at /home/koha/versions//koha_community/C4/Auth.pm line 591, near "}"
Bareword "C4::Auth_with_cas::multipleAuth" not allowed while "strict subs" in use at /home/koha/versions//koha_community/C4/Auth.pm line 960.
syntax error at /home/koha/versions//koha_community/C4/Auth.pm line 996, near "}"
/home/koha/versions//koha_community/C4/Auth.pm has too many errors.
Compilation failed in require at /home/koha/versions/koha_community/mainpage.pl line 23.
BEGIN failed--compilation aborted at /home/koha/versions/koha_community/mainpage.pl line 23
Comment 8 claire.hernandez@biblibre.com 2011-04-05 12:33:41 UTC
Try to add to proposedpatch lines

- C4/Auth.pm
		warn "OPAC Install required, redirecting to maintenance";		
553			print $query->redirect("/cgi-bin/koha/maintenance.pl");
554		}
555	    unless ($version = C4::Context->preference('Version')) {    # assignment, not comparison
556	      if ($type ne 'opac') {
557	        warn "Install required, redirecting to Installer";
558	        print $query->redirect("/cgi-bin/koha/installer/install.pl");
559	      }
560	      else {
(no more compilations errors)

-  Auth_with_cas.pm
add "use YAML" 

Tests done:
- syspref casAUthentication to true and an ip set -> log in opac ok
- with this configuration sysprefs weren't editable anymore (error)
- syspref casAUthentication to true and yaml file with differents servers -> ko
Comment 9 Matthias Meusburger 2011-04-07 15:18:56 UTC Comment hidden (obsolete)
Comment 10 MJ Ray (software.coop) 2011-04-08 06:29:16 UTC
This bug is mentioned in:
Bug 5630 CAS improvements http://lists.koha-community.org/pipermail/koha-patches/2011-March/014374.html
Bug 5630 : MT4057: Let the user choose	the CAS server to authenticate against in case of multiple	CAS servers http://lists.koha-community.org/pipermail/koha-patches/2011-March/014375.html
Bug 5630 add PSGI ability to C4::Auth http://lists.koha-community.org/pipermail/koha-patches/2011-March/014376.html
Bug 5630 : MT4057: [Follow-up] Let the user	choose the CAS server to authenticate against in case of	multiple CAS servers http://lists.koha-community.org/pipermail/koha-patches/2011-March/014378.html
Bug 5630 : MT4704: Fix multiple CAS	authentication http://lists.koha-community.org/pipermail/koha-patches/2011-March/014377.html
Comment 11 Paul Poulain 2011-04-08 13:05:01 UTC
matts forgot to switch back to Needs signoff I think
Comment 12 Paul Poulain 2011-07-22 09:49:48 UTC
Created attachment 4716 [details] [review]
proposed patch squashed, rebased and signed-off

I've :
* rebased the patch
* fixed a small H::T::P string remaining
* tested on a test CAS server

works fine, signing-off
Comment 13 Paul Poulain 2011-07-22 09:50:24 UTC
(I also have squashed all patches in one)
Comment 14 Paul Poulain 2011-08-03 15:40:32 UTC
QA comment

Ian/chris, won't mark it "passed QA", as i've signed-off. Just can guarantee i've tested it against the servers of one of our customer, and it works.

There is a documentation provided (docs/ directory), as well as an example C4/Auth_cas_servers.yaml.orig

I've detected a 
 docs/CAS/CASProxy/examples/casSession.tmp        |  Bin 0 -> 134 bytes

file in git, that could probably be not here, but it's created 

My opinion is that it can be applied to master, but i'll let you decide
Comment 15 Ian Walls 2011-08-05 11:34:54 UTC
Does anyone else in the community use CAS Authentication?  If not, would it be possible to set up a CAS server for community testing?
Comment 16 Paul Poulain 2011-08-05 11:47:35 UTC
Ian, FYI:
I don't know for anyone in the community that uses cas. I've searched for a demo server on internet and couldn't find one.

What I suggest would be to test that it doesn't break anything without CAS. If someone else tries to setup CAS and has problem, he will report it. Note most of our universities (5 so far) uses CAS, I don't know the details, but that's not just one.
Comment 17 Ian Walls 2011-08-05 12:05:32 UTC
OK, so a testing plan to check against regressions:

Situation 1:  Koha-only authentication

Check logins:
a) by userid for valid userid/password
b) by userid for invalid userid/password
c) by cardnumber for valid cardnumber/password
d) by cardnumber for invalid cardnumber/password
e) by root database user

Situation 2: LDAP authentication
(I believe the particular configurations of the LDAP are irrelevant here, but for more rigour, check with all combinations of update/replicate on/off)

Check logins:
a) by valid LDAP userid/password
b) by invalid LDAP userid/password, but valid Koha userid/password
c) by invalid LDAP userid/password, but valid Koha cardnumber/password
d) by invalid LDAP userid/password and invalid Koha userid/password
e) by root database user

All that said, I suspect that this will apply fine to Koha without any breakages, given the limited changes to C4/Auth.pm and opac-user.tt.  But, please test all 10 conditions and provide results to earn "Passed QA" status.
Comment 18 Paul Poulain 2011-08-09 13:33:52 UTC
QA comment

(works = I could connect)

I've checked
a) by userid for valid userid/password
=> works
b) by userid for invalid userid/password
=> works
c) by cardnumber for valid cardnumber/password
=> works
d) by cardnumber for invalid cardnumber/password
=> login rejected
e) by root database user
=> works

Situation 2: LDAP authentication
Check logins:
a) by valid LDAP userid/password
=> works
b) by invalid LDAP userid/password, but valid Koha userid/password
=> works
c) by invalid LDAP userid/password, but valid Koha cardnumber/password
=> works
d) by invalid LDAP userid/password and invalid Koha userid/password
=> login rejected
e) by root database user
=> works

(tested against BibLibre internal LDAP. I haven't checked all possible combination of replicate/update, as the code really has nothing to do with those flags. Same comment for auth_by_bind ON and OFF)

marking passed QA, no specific problem detected.

(ouch... was quite long & complex to set up all that stuff ;-) )
Comment 19 Sébastien Marie 2011-08-09 13:45:14 UTC
(In reply to comment #18)
> QA comment
> 
> (works = I could connect)
> 
> I've checked
[...]
> b) by userid for invalid userid/password
> => works
[...]
> 
> marking passed QA, no specific problem detected.
> 

I hope the (b) case should be read as "login rejected"... else I think there is a specific problem: user could log with wrong password :-)
Comment 20 Paul Poulain 2011-08-09 13:48:46 UTC
yikes ! you're right. At the beginning, I wrote "works" everywhere when testing. Then I realized in fact when login is rejected, "works" can be suspicious. So I changed what I wrote and forgot c) :'((
(time to head for 2 weeks AFK ;-) )
Comment 21 Paul Poulain 2011-08-09 15:07:59 UTC
*** Bug 6012 has been marked as a duplicate of this bug. ***
Comment 22 Chris Cormack 2011-08-10 01:18:28 UTC
Without a CAS server to test against, I have no real way of checking, however if it works for Biblibre who have clients using it, and Paul has signed off, that is enough for me to push it. 

I have tested it doesnt touch non CAS auth, so no regressions.

Pushed please test.
Comment 23 Ian Walls 2011-08-30 14:44:07 UTC
Created attachment 5219 [details] [review]
Follow up Patch

Previous patch stopped checkpw_ldap from being imported, resulting in an error message the C4::Auth::checkpw_ldap is undefined.
Comment 24 Paul Poulain 2011-08-30 15:36:08 UTC
It's no more an ENH, it now a follow-up to fix a bug.

Ian = Was there something specific in your configuration that could explain my tests worked and not yours ?

( There's nothing in my test, I just used biblibre internal ldap server :( )
Comment 25 Ian Walls 2011-08-30 15:57:22 UTC
Paul,


No, no explanation.  We're running off master (as of yesterday, I believe).  Looking at the code, I'm not sure how your tests worked, if checkpw_ldap was never imported into C4/Auth.pm.  Either it would have needed to have been explicitly specified as C4::Auth_with_ldap::checkpw_ldap, or checkpw_ldap added to @INC in C4/Auth_with_ldap.om.  Or, I'm missing some nuance of how perl require/import works.
Comment 26 Ian Walls 2011-08-31 10:35:05 UTC
One other thing to note:  Authen::CAS::Client has gone from optional to required.  While C4/Installer/PerlDependencies.pm already has it marked as such, many libraries were able up to this point to use Koha without it without incident.

Do the Debian dependencies need to be updated, as well?
Comment 27 Marcel de Rooy 2011-09-01 13:18:59 UTC
(In reply to comment #22)
> Without a CAS server to test against, I have no real way of checking, however
> if it works for Biblibre who have clients using it, and Paul has signed off,
> that is enough for me to push it. 
> I have tested it doesnt touch non CAS auth, so no regressions.
> Pushed please test.

Recent error
[Thu Sep 01 14:22:45 2011] [error] [client 192.87.126.61] [Thu Sep  1 14:22:45 2011] opac-MARCdetail.pl: DBD::mysql::st execute failed: Unknown column 'limit_desc' in 'field list' at /usr/share/koha/testclone/C4/Auth.pm line 276.
Seems to come from this commit 9a3950f673c28688a2ead6514c3320e34ec0cc4c (5630 CAS Improvements)
Comment 28 Chris Cormack 2011-09-02 01:22:41 UTC
Sorry my bad, I shouldn't have pushed this.

It contains code that changes more than just CAS, including code that changes the way search history is stored, and without a corresponding change to the database. 

I am reverting it, it needs to be tidied up, the changes like

+VALUES                    (     ?,         ?,          ?,         ?,          ?,         ?,     ?, FROM_UNIXTIME(?))
 EOQ
 sub get_template_and_user {
     my $in       = shift;
@@ -264,13 +271,9 @@ sub get_template_and_user {
 			        my @recentSearches = @{thaw($searchcookie) || []};
 				if (@recentSearches) {
 					my $sth = $dbh->prepare($SEARCH_HISTORY_INSERT_SQL);
-					$sth->execute( $borrowernumber,
-						       $in->{'query'}->cookie("CGISESSID"),
-						       $_->{'query_desc'},
-						       $_->{'query_cgi'},
-						       $_->{'total'},
-						       $_->{'time'},
-                                        ) foreach @recentSearches;
+                    
+                    $sth->execute( $borrowernumber, $in->{'query'}->cookie("CGISESSID"), $_->{'query_desc'}, $_->{'query_cgi'}, $_->{'limit_desc'}, $_->{'limit_cgi'}, $_->{'total'}, $_->{'time'}, )
+                                foreach @recentSearches;
 
Need to be removed and put in their own bug. With the corresponding database change, the lack of which is causing the error Marcel reports
Plus the patch from Ian integrated.
Comment 29 Paul Poulain 2011-09-02 07:49:26 UTC
it's probably a mistake when splitting the "BibLibre big branch".
bug 5981 is related to the search history problem (and i'll submit a new patch in the next minutes, I was working on it)
Comment 30 Paul Poulain 2011-09-02 10:39:42 UTC
I've reviewed the code, I've found another strange thing, the file docs/CAS/CASProxy/examples/casSession.tmp

I've asked matts to confirm it can be removed.

Will submit a follow-up in a few hours (removing the search_history stuff)
Comment 31 Paul Poulain 2011-09-02 16:22:00 UTC
Created attachment 5297 [details] [review]
proposed patch follow-up 2

After speaking with Matthias, this file is necessary if you want to check the test provided in docs. It could have been empty.

I've reviewed all the patch, and don't see anything that should not be here.
I did some tests, nothing seems brokent (and nothing in the message log)

chris = not sure what to do with the patch status in such a case. will switch back to QA passed, but this may be disputable
Comment 32 Ian Walls 2011-10-12 20:27:15 UTC
Applying the 3 patches in order seems to resolve any of the bleed-through issues with LDAP authentication and Search History.  Required installation of Authen::CAS::Client is noted in the C4/Installer/PerlDependencies.pm modules.  The rest of the changes are all to Auth_with_cas.pm, which I don't have the means to test right now (but I'm sure Paul has working to his libraries' satisfaction).

Marking as Passed QA
Comment 33 Chris Cormack 2011-10-12 21:54:26 UTC
Introduces no string changes, Pushing please test
Comment 34 Jared Camins-Esakov 2012-12-31 01:15:16 UTC
There have been no further reports of problems so I am marking this bug resolved.