From 4a6ec50c625419cc0f3884142b7f4307cd2a4370 Mon Sep 17 00:00:00 2001
From: Owen Leonard <oleonard@myacpl.org>
Date: Wed, 5 Oct 2011 12:04:12 -0400
Subject: [PATCH] Fix for Bug 5280 - Fix password field so that the password is
 masked as it is entered

This patch changes the password field to a password type input on
member-password.pl and adds a confirmation field to both member-password.pl
and memberentry.pl requiring that the password be re-entered to
confirm.

Client-side and server-side validation for the two password fields has been added
to both pages. Multiple error messages can now be displayed together on
member-password.pl.

If the user wishes for Koha to suggest a random password on member-password.pl
they can click a link which will remove the password-type input fields, replace
them with text-type fields, and automatically fill them with the random
password suggestion.

Follow-up fix lets the members.js correctly handling errors when there are
no mandatory fields

LR followup: fixing slight error that corrects previously reported template error.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Tested password setting/changing utilities - all work as expected and described.
Passes prove t xt t/db_dependent tests congruent with current master failures (adds no new fails).

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
---
 koha-tmpl/intranet-tmpl/prog/en/js/members.js | 15 +++-
 .../en/modules/members/member-password.tt     | 52 ++++++++++++--
 .../prog/en/modules/members/memberentrygen.tt | 70 +++++++++++++++----
 members/member-password.pl                    | 36 +++++-----
 members/memberentry.pl                        |  2 +
 5 files changed, 135 insertions(+), 40 deletions(-)

diff --git a/koha-tmpl/intranet-tmpl/prog/en/js/members.js b/koha-tmpl/intranet-tmpl/prog/en/js/members.js
index 796db0a880..3ed6195e8d 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/js/members.js
+++ b/koha-tmpl/intranet-tmpl/prog/en/js/members.js
@@ -85,6 +85,8 @@ var myDate2=document.form.dateexpiry.value.split ('/');
 // function to test all fields in forms and nav in different forms(1 ,2 or 3)
 function check_form_borrowers(nav){
 	var statut=0;
+	var message = "";
+	var message_champ="";
 	if (document.form.check_member.value == 1 )
 	{
 		if (document.form_double.answernodouble) {
@@ -101,9 +103,8 @@ function check_form_borrowers(nav){
 	else
 	{
 	    var champ_verif = document.form.BorrowerMandatoryField.value.split ('|');
-	    var message = MSG_MISSING_MANDATORY
+	    message += MSG_MISSING_MANDATORY
 	    message += "\n";
-	    var message_champ="";
 		for (var i=0; i<champ_verif.length; i++) {
 			if (document.getElementsByName(""+champ_verif[i]+"")[0]) {
 				var val_champ=eval("document.form."+champ_verif[i]+".value");
@@ -128,10 +129,18 @@ function check_form_borrowers(nav){
 			}
 		}
 	}
+
+	if ( document.form.password.value != document.form.password2.value ){
+			if ( message_champ != '' ){
+				message_champ += "\n";
+			}
+			message_champ+= MSG_PASSWORD_MISMATCH;
+			statut=1;
+	}
+
 	//patrons form to test if you checked no to the question of double
  	if (statut!=1 && document.form.check_member.value > 0 ) {
 		if (!(document.form_double.answernodouble.checked)){
-			message ="";
 			message_champ+= MSG_DUPLICATE_SUSPICION;
 			statut=1;
 			document.form.nodouble.value=0;
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/member-password.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/member-password.tt
index 93b1828c05..126927053f 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/member-password.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/member-password.tt
@@ -1,6 +1,25 @@
 [% INCLUDE 'doc-head-open.inc' %]
 <title>Koha &rsaquo; Patrons &rsaquo; [% IF ( newpassword ) %]Password Updated [% ELSE %]Update Password for [% surname %], [% firstname %][% END %]</title>
 [% INCLUDE 'doc-head-close.inc' %]
+<script type="text/JavaScript">
+//<![CDATA[
+    $(document).ready(function() {
+        $("#changepasswordf").submit(function(){
+            if($("input[name='newpassword']").val() != $("input[name='newpassword2']").val()){
+                alert(_("Passwords do not match"));
+                return false;
+            } else {
+                return true;
+            }
+        });
+        $("#fillrandom").live('click', function() {
+            $("#newpassword").after("<input type=\"text\" name=\"newpassword\" value=\"[% defaultnewpassword %]\">").remove();
+            $("#newpassword2").after("<input type=\"text\" name=\"newpassword2\" value=\"[% defaultnewpassword %]\">").remove();
+        });
+        $("div.hint").eq(0).after(" <div class=\"hint\"><a href=\"#\" id=\"fillrandom\">"+_("Click to fill with a randomly generated suggestion. ")+"<strong>"+_("Passwords will be displayed as text")+"</strong>.</a></div>");
+    });
+//]]>
+</script>
 </head>
 <body>
 [% INCLUDE 'header.inc' %]
@@ -20,31 +39,50 @@
 
 [% ELSE %]
 
-<form method="post" action="/cgi-bin/koha/members/member-password.pl">
+<form method="post" id="changepasswordf" action="/cgi-bin/koha/members/member-password.pl">
 <input type="hidden" name="destination" value="[% destination %]" />	
 <input type="hidden" name="cardnumber" value="[% cardnumber %]" />
 <input type="hidden" name="borrowernumber" id="borrowernumber" value="[% borrowernumber %]" />
 	[% IF ( errormsg ) %]
+		<div class="dialog alert">
+		<h4>The following errors have occurred:</h4>
+		<ul>
 		[% IF ( BADUSERID ) %]
-		<div class="dialog alert">You have entered a User ID that already exists.  Please choose another one.</div>
+		<li>You have entered a username that already exists.  Please choose another one.</li>
 		[% END %]
 		[% IF ( SHORTPASSWORD ) %]
-		<div class="dialog alert"><strong>The password entered is too short</strong>. Password must be at least [% minPasswordLength %] characters.</div>
+		<li><strong>The password entered is too short</strong>. Password must be at least [% minPasswordLength %] characters.</li>
 		[% END %]
 		[% IF ( NOPERMISSION ) %]
-		<div class="dialog alert">You do not have permission to edit this patron's login information.</div>
+		<li>You do not have permission to edit this patron's login information.</li>
 		[% END %]
+		[% IF ( NOMATCH ) %]
+		<li><strong>The passwords entered do not match</strong>. Please re-enter the new password.</li>
+		[% END %]
+		</ul>
+		</div>
 	[% END %]
 
 
 	<fieldset class="brief"><legend>Change Username and/or Password for [% firstname %] [% surname %]</legend>
 	<ol>
 	<li><label for="newuserid">New Username:</label>
-	<input type="hidden" name="member" value="[% member %]" /><input type="text" id="newuserid" name="newuserid" size="20" value="[% userid %]" /></li>
+	<input type="hidden" name="member" value="[% borrowernumber %]" /><input type="text" id="newuserid" name="newuserid" size="20" value="[% userid %]" /></li>
 	<li><label for="newpassword">New Password:</label>
-	<div class="hint">Koha cannot display existing passwords. Below is a randomly generated suggestion.  Leave the field blank to leave password unchanged.</div>
+	<div class="hint">Koha cannot display existing passwords. Leave the field blank to leave password unchanged.</div>
 	[% IF ( minPasswordLength ) %]<div class="hint">Minimum password length: [% minPasswordLength %]</div>[% END %]
-	<input name="newpassword"  id="newpassword" type="text" size="20" value="[% defaultnewpassword %]" /></li>
+	[% IF ( NOMATCH ) %]
+	<input name="newpassword"  id="newpassword" type="password" size="20" class="focus" />
+	<input name="newpassword" id="newpassword_random" readonly="readonly" disabled="disabled" type="hidden" />
+	[% ELSE %]
+	<input name="newpassword"  id="newpassword" type="password" size="20" />
+	<input name="newpassword" readonly="readonly" disabled="disabled" type="hidden" />
+	[% END %]
+	</li>
+	<li><label for="newpassword2">Confirm New Password:</label>
+	<input name="newpassword2"  id="newpassword2" type="password" size="20" />
+	<input name="newpassword2" id="newpassword2_random" readonly="readonly" disabled="disabled" type="hidden" />
+	</li>
 	</ol>
 </fieldset>
 	<fieldset class="action"><input type="submit" value="Save" /> <a class="cancel" href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% borrowernumber %]">Cancel</a></fieldset>
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
index 02d39f5e05..34fef892ed 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
@@ -3,7 +3,7 @@
 [% IF ( opadd ) %]Add[% ELSIF ( opduplicate ) %]Duplicate[% ELSE %] Modify[% END %] [% IF ( categoryname ) %] [% categoryname %] patron[% ELSE %][% IF ( I ) %] Organization patron[% END %][% IF ( A ) %] Adult patron[% END %][% IF ( C ) %] Child patron[% END %][% IF ( P ) %] Professional patron[% END %][% IF ( S ) %] Staff patron[% END %][% END %][% UNLESS ( opadd ) %] [% surname %], [% firstname %][% END %]</title>
 [% INCLUDE 'doc-head-close.inc' %]
 [% INCLUDE 'calendar.inc' %]
-<script type="text/JavaScript" language="JavaScript">
+<script type="text/JavaScript">
 //<![CDATA[
     $(document).ready(function() {
 		$("fieldset.rows input").keydown(function(e){ return checkEnter(e); });
@@ -60,6 +60,7 @@
         var MSG_LATE_EXPIRY = _("Warning: Expiration date falls before enrollment date");
         var MSG_MISSING_MANDATORY = _("The following fields are mandatory:");
         var MSG_DUPLICATE_SUSPICION = _("Please confirm whether this is a duplicate patron");
+        var MSG_PASSWORD_MISMATCH = _("The passwords entered do not match");
 //]]>
 </script>
 <script type="text/javascript" src="[% themelang %]/js/members.js"></script>
@@ -143,6 +144,9 @@
 			[% IF ( ERROR_short_password ) %]
 				<li id="ERROR_short_password">Password must be at least [% minPasswordLength %] characters long.</li>
 			[% END %]
+			[% IF ( ERROR_password_mismatch ) %]
+				<li id="ERROR_password_mismatch">Passwords do not match.</li>
+			[% END %]
             [% IF ( ERROR_extended_unique_id_failed ) %]
                 <li id="ERROR_extended_unique_id_failed">The attribute value 
                     [% ERROR_extended_unique_id_failed %] is already is use by another patron record.</li>
@@ -1182,41 +1186,82 @@
 			[% IF ( opadd ) %]
 			[% IF ( NoUpdateLogin ) %]
 				[% IF ( opduplicate ) %]
-					<input type="text" id="password" name="password" size="20"  disabled="disabled" />
+					<input type="password" id="password" name="password" size="20"  disabled="disabled" />
 				[% ELSE %]
-					<input type="text" id="password" name="password" size="20"  disabled="disabled" value="[% password %]" />
+					<input type="password" id="password" name="password" size="20"  disabled="disabled" value="[% password %]" />
 				[% END %]
 [% ELSE %]
 				[% IF ( opduplicate ) %]
-					<input type="text" id="password" name="password" size="20" />
+					<input type="password" id="password" name="password" size="20" />
 				[% ELSE %]
-					<input type="text" id="password" name="password" size="20" value="[% password %]" />
+					<input type="password" id="password" name="password" size="20" value="[% password %]" />
 				[% END %]
 [% END %]
 			[% ELSE %]
 			[% IF ( password ) %]
 				[% IF ( NoUpdateLogin ) %]
-					<input type="text" id="password" name="password" size="20"  disabled="disabled" value="****" />
+					<input type="password" id="password" name="password" size="20"  disabled="disabled" value="****" />
 				[% ELSE %]
 					[% IF ( opduplicate ) %]
-						<input type="text" id="password" name="password" size="20" />
+						<input type="password" id="password" name="password" size="20" />
 					[% ELSE %]
-						<input type="text" id="password" name="password" size="20" value="****" />
+						<input type="password" id="password" name="password" size="20" value="****" />
 					[% END %]
 				[% END %]
 			[% ELSE %]
 				[% IF ( NoUpdateLogin ) %]
-					<input type="text" id="password" name="password" size="20"  disabled="disabled" value="" />
+					<input type="password" id="password" name="password" size="20"  disabled="disabled" value="" />
 				[% ELSE %]
-					<input type="text" id="password" name="password" size="20" value="" />
+					<input type="password" id="password" name="password" size="20" value="" />
 				[% END %]
 			[% END %]
 			[% END %]
 	  [% IF ( mandatorypassword ) %]<span class="required">Required</span>[% END %][% IF ( ERROR_short_password ) %]<span class="required">Password is too short</span>[% END %]
 [% IF ( minPasswordLength ) %]<div class="hint">Minimum password length: [% minPasswordLength %]</div>[% END %]
 		</li>
-        [% END %]
-    </ol>
+		<li>
+			[% IF ( mandatorypassword ) %]
+			<label for="password2" class="required">
+			[% ELSE %]
+			<label for="password2">
+			[% END %]
+			Confirm password: </label>
+			[% IF ( opadd ) %]
+			[% IF ( NoUpdateLogin ) %]
+				[% IF ( opduplicate ) %]
+					<input type="password" id="password2" name="password2" size="20"  disabled="disabled" />
+				[% ELSE %]
+					<input type="password" id="password2" name="password2" size="20"  disabled="disabled" value="[% password %]" />
+				[% END %]
+[% ELSE %]
+				[% IF ( opduplicate ) %]
+					<input type="password" id="password2" name="password2" size="20" />
+				[% ELSE %]
+					<input type="password" id="password2" name="password2" size="20" value="[% password %]" />
+				[% END %]
+[% END %]
+			[% ELSE %]
+			[% IF ( password ) %]
+				[% IF ( NoUpdateLogin ) %]
+					<input type="password" id="password2" name="password2" size="20"  disabled="disabled" value="****" />
+				[% ELSE %]
+					[% IF ( opduplicate ) %]
+						<input type="password" id="password2" name="password2" size="20" />
+					[% ELSE %]
+						<input type="password" id="password2" name="password2" size="20" value="****" />
+					[% END %]
+				[% END %]
+			[% ELSE %]
+				[% IF ( NoUpdateLogin ) %]
+					<input type="password" id="password2" name="password2" size="20"  disabled="disabled" value="" />
+				[% ELSE %]
+					<input type="password" id="password2" name="password2" size="20" value="" />
+				[% END %]
+			[% END %]
+			[% END %]
+	  [% IF ( mandatorypassword ) %]<span class="required">Required</span>[% END %][% IF ( ERROR_password_mismatch ) %]<span class="required">Passwords do not match</span>[% END %]
+		</li>
+		</ol>
 		</fieldset>
         [% END # hide fieldset %]
 		<!--this zones are not necessary in modif mode -->
@@ -1431,6 +1476,7 @@
 [% INCLUDE 'members-menu.inc' %]
 </div>[% END %]
 [% END %]
+[% END %]
 </div>
 [% INCLUDE 'intranet-bottom.inc' %]
 
diff --git a/members/member-password.pl b/members/member-password.pl
index 8128a3dff8..fe24df3a4b 100755
--- a/members/member-password.pl
+++ b/members/member-password.pl
@@ -40,17 +40,21 @@ $flagsrequired->{borrowers}=1;
 my $member=$input->param('member');
 my $cardnumber = $input->param('cardnumber');
 my $destination = $input->param('destination');
-my $errormsg;
+my @errors;
 my ($bor)=GetMember('borrowernumber' => $member);
 if(( $member ne $loggedinuser ) && ($bor->{'category_type'} eq 'S' ) ) {
-	$errormsg = 'NOPERMISSION' unless($staffflags->{'superlibrarian'} || $staffflags->{'staffaccess'} );
+	push(@errors,'NOPERMISSION') unless($staffflags->{'superlibrarian'} || $staffflags->{'staffaccess'} );
 	# need superlibrarian for koha-conf.xml fakeuser.
 }
 my $newpassword = $input->param('newpassword');
+my $newpassword2 = $input->param('newpassword2');
+
+push(@errors,'NOMATCH') if ( ( $newpassword && $newpassword2 ) && ($newpassword ne $newpassword2) );
+
 my $minpw = C4::Context->preference('minPasswordLength');
-$errormsg = 'SHORTPASSWORD' if( $newpassword && $minpw && (length($newpassword) < $minpw ) );
+push(@errors,'SHORTPASSWORD') if( $newpassword && $minpw && (length($newpassword) < $minpw ) );
 
-if ( $newpassword  && ! $errormsg ) {
+if ( $newpassword  && !scalar(@errors) ) {
     my $digest=md5_base64($input->param('newpassword'));
     my $uid = $input->param('newuserid');
     my $dbh=C4::Context->dbh;
@@ -62,13 +66,7 @@ if ( $newpassword  && ! $errormsg ) {
 		    print $input->redirect("/cgi-bin/koha/members/moremember.pl?borrowernumber=$member");
 		}
     } else {
-			$errormsg = 'BADUSERID';
-    	    $template->param(othernames => $bor->{'othernames'},
-						surname     => $bor->{'surname'},
-						firstname   => $bor->{'firstname'},
-						userid      => $bor->{'userid'},
-						defaultnewpassword => $newpassword 
-						);
+			push(@errors,'BADUSERID');
     }
 } else {
     my $userid = $bor->{'userid'};
@@ -79,7 +77,9 @@ if ( $newpassword  && ! $errormsg ) {
     for (my $i=0; $i<$length; $i++) {
 	$defaultnewpassword.=substr($chars, int(rand(length($chars))),1);
     }
-	
+
+	$template->param( defaultnewpassword => $defaultnewpassword );
+}
     if ( $bor->{'category_type'} eq 'C') {
         my  ( $catcodes, $labels ) =  GetborCatFromCatType( 'A', 'WHERE category_type = ?' );
         my $cnt = scalar(@$catcodes);
@@ -120,16 +120,16 @@ if (C4::Context->preference('ExtendedPatronAttributes')) {
 	    userid      => $bor->{'userid'},
 	    destination => $destination,
 		is_child        => ($bor->{'category_type'} eq 'C'),
-	    defaultnewpassword => $defaultnewpassword,
 		activeBorrowerRelationship => (C4::Context->preference('borrowerRelationship') ne ''),
+        minPasswordLength => $minpw
 	);
 
+if( scalar(@errors )){
+	$template->param( errormsg => 1 );
+	foreach my $error (@errors) {
+        $template->param($error) || $template->param( $error => 1);
+	}
 
 }
 
-$template->param( member => $member,
-					errormsg => $errormsg,
-					$errormsg => 1 ,
-					minPasswordLength => $minpw );
-
 output_html_with_http_headers $input, $cookie, $template->output;
diff --git a/members/memberentry.pl b/members/memberentry.pl
index 6de07bf50b..86dd6ed561 100755
--- a/members/memberentry.pl
+++ b/members/memberentry.pl
@@ -292,6 +292,8 @@ if ($op eq 'save' || $op eq 'insert'){
   }
   
   my $password = $input->param('password');
+  my $password2 = $input->param('password2');
+  push @errors, "ERROR_password_mismatch" if ( $password ne $password2 );
   push @errors, "ERROR_short_password" if( $password && $minpw && $password ne '****' && (length($password) < $minpw) );
 
   if (C4::Context->preference('ExtendedPatronAttributes')) {
-- 
2.39.5