Bug 5630

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

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

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

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

matts forgot to switch back to Needs signoff I think

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

(I also have squashed all patches in one)

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

Does anyone else in the community use CAS Authentication?  If not, would it be possible to set up a CAS server for community testing?

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.

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.

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 ;-) )

(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 :-)

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 ;-) )

*** Bug 6012 has been marked as a duplicate of this bug. ***

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.

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.

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 :( )

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.

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?

(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)

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.

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)

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)

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

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

Introduces no string changes, Pushing please test

There have been no further reports of problems so I am marking this bug resolved.