Bug 5280 - Fix password field in members.pl so that the password is masked as it is entered
Summary: Fix password field in members.pl so that the password is masked as it is entered
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: 3.8
Hardware: All All
: P4 enhancement (vote)
Assignee: Owen Leonard
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-05 18:44 UTC by Chris Nighswonger
Modified: 2013-05-23 06:24 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed fix (16.55 KB, patch)
2011-10-20 20:02 UTC, Owen Leonard
Details | Diff | Splinter Review
Fix for Bug 5280 - Fix password field so that the password is masked as it is entered (16.58 KB, patch)
2011-11-04 00:58 UTC, Chris Cormack
Details | Diff | Splinter Review
"undefined" error (301.28 KB, image/png)
2011-11-09 14:44 UTC, Paul Poulain
Details
Revised fix (17.50 KB, patch)
2011-11-09 16:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Fix for Bug 5280 - Fix password field so that the password is masked as it is entered (17.52 KB, patch)
2011-11-10 08:06 UTC, Chris Cormack
Details | Diff | Splinter Review
Fix for Bug 5280 - Fix password field so that the password is masked as it is entered (17.53 KB, patch)
2012-01-29 22:43 UTC, Chris Cormack
Details | Diff | Splinter Review
Fix for Bug 5280 - Fix password field so that the password is masked as it is entered (17.61 KB, patch)
2012-02-07 14:08 UTC, Owen Leonard
Details | Diff | Splinter Review
[Signed Off] Fix for Bug 5280 - Fix password field so that the password is masked as it is entered (18.07 KB, patch)
2012-02-07 17:52 UTC, Liz Rea
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Nighswonger 2010-10-05 18:44:12 UTC
This is standard practice with password entry on forms.
Comment 1 Owen Leonard 2010-10-05 18:52:04 UTC
Changing it to a password type input is easy enough, but if you're going to mask the input you have to offer another field to re-type the password. Otherwise there's no way for the user to know they've entered it correctly.
Comment 2 Owen Leonard 2011-10-05 18:28:01 UTC
I've got a work in progress for this, which adds second password-confirmation entry field on both the change password page and the patron entry page.

The main blocker to adding this is that I don't see how the update to the change password page (member-password.pl) can work with the "suggest random password" feature currently in place. If we accept the random password suggestion it's masked by the password-type input field. What good is entering a password if the patron can't know what it is?

Is the "suggest random password" feature worth keeping?
Comment 3 Owen Leonard 2011-10-20 20:02:01 UTC Comment hidden (obsolete)
Comment 4 Paul Poulain 2011-10-25 15:07:32 UTC
bug affected to Koha 3.6. Entries must be attached to rel_3_8 only when the patch is pushed here (and if it's an ENH)
Comment 5 Chris Cormack 2011-11-04 00:58:30 UTC Comment hidden (obsolete)
Comment 6 Ian Walls 2011-11-04 08:29:09 UTC
This is primarily a template change; the changes to the perl are brief and easily readable.  Implements a needed security feature for double-entry.  Random generation still supported.  Marking as Passed QA
Comment 7 Paul Poulain 2011-11-04 10:35:37 UTC
I've found a small bug when testing : if you enter a different password on the 2 forms, you get:

undefined
undefined
The passwords entered do not match

Marking failed QA. Owen, when you submit your follow-up, I think you can go directly "passed QA", it's a small glitch
Comment 8 Owen Leonard 2011-11-04 13:42:15 UTC
Paul, I can't reproduce this error. Which page is it on? What browser are you using?
Comment 9 Paul Poulain 2011-11-09 14:44:45 UTC Comment hidden (obsolete)
Comment 10 Owen Leonard 2011-11-09 16:52:24 UTC Comment hidden (obsolete)
Comment 11 Chris Cormack 2011-11-10 08:06:41 UTC Comment hidden (obsolete)
Comment 12 Paul Poulain 2011-11-18 22:22:17 UTC
QA comment: 
I get a "Debug is on (level 1)"
just below the "Modify Adult patron xxxx"
(I have DEBUG set to yes in my setup)
Is it because DEBUG=1 or a test that should be removed ?
Comment 13 Paul Poulain 2011-12-06 18:28:20 UTC
(In reply to comment #12)
> QA comment: 
> I get a "Debug is on (level 1)"
> just below the "Modify Adult patron xxxx"
> (I have DEBUG set to yes in my setup)
> Is it because DEBUG=1 or a test that should be removed ?

Owen / chris, bumping my question !
Comment 14 Owen Leonard 2011-12-06 19:12:31 UTC
That debug messages dates back to 2007 and commit c102b3c6f5197fc695cadc1941d3331889fd6561
Comment 15 Marcel de Rooy 2012-01-04 15:24:00 UTC
Paul:
Could you update the status of this bug report?
It seems to be pending as a result of your comments.
Comment 16 Paul Poulain 2012-01-09 19:30:00 UTC
Marcel: Owen answered my question, you can QA this patch if you want/can
Comment 17 Marcel de Rooy 2012-01-11 12:39:36 UTC
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
Comment 18 Chris Cormack 2012-01-29 22:43:38 UTC Comment hidden (obsolete)
Comment 19 Paul Poulain 2012-02-03 15:35:33 UTC
Sorry, but patch does not apply anymore.
I tried to fix the conflict on members/member-password.pl, but a few pages after, I got:
Template process failed: file error - parse error - /home/paul/koha.dev/koha-community/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt line 1477: unexpected end of input at /home/paul/koha.dev/koha-community/C4/Templates.pm line 126.

So I suspect there's something wrong, either in your patch or git make an undetected mistake (no conflict but wrong things)
Comment 20 Owen Leonard 2012-02-07 14:08:21 UTC Comment hidden (obsolete)
Comment 21 Owen Leonard 2012-02-07 14:10:05 UTC
I'm not sure how to handle patch rebases to keep the sign-off, or even if that is appropriate? A rebase could certainly introduce new bugs, so it might warrant another signoff. I'll leave this "Needs signoff" and let QA decide.
Comment 22 Liz Rea 2012-02-07 16:02:24 UTC
I got this patch to apply cleanly, but when I go to add a patron, I get the following:


Template process failed: file error - parse error - /home/liz/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt line 1480: unexpected end of input at /home/liz/kohaclone/C4/Templates.pm line 126.
Comment 23 Liz Rea 2012-02-07 17:46:43 UTC
I was able to fix this - will test and submit the fixed patch as my signoff - the fix was trivial.
Comment 24 Liz Rea 2012-02-07 17:52:43 UTC
Created attachment 7488 [details] [review]
[Signed Off] 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).
Comment 25 Paul Poulain 2012-02-13 15:46:15 UTC
QA comment:
* nothing wrong in the feature itself
* template still translatable after the patch
* switches permission control on member-password to a list of errors, instead of just one, that's good

passed QA
Comment 26 Paul Poulain 2012-02-13 15:48:21 UTC
Note to RMaint: not sure this feature should be in 3.6.x or no: it's an important change in the behaviour and, even if ppl will be happy to see it, I think it's more something for a major upgrade: less experienced librarians will be surprised and troubled by the change.
Switching to ENH and 3.8.

If someone disagree, arguments welcomed.
Comment 27 David Cook 2012-06-08 06:25:39 UTC
(In reply to comment #13)
> (In reply to comment #12)
> > QA comment: 
> > I get a "Debug is on (level 1)"
> > just below the "Modify Adult patron xxxx"
> > (I have DEBUG set to yes in my setup)
> > Is it because DEBUG=1 or a test that should be removed ?
> 
> Owen / chris, bumping my question !

I'm still confused by this :/. 

I'm getting 3 "Use of uninitialized value in concatenation (.) or string" errors (for "dateofbirth: \tdateenrolled: \tdateexpiry", I think). 

I'm also getting 3 "xsub: [(($6||0),($5||0),($4||0),$3, $2 - 1, $1 - 1900)]" errors. 

I don't see any fundamental changes from 3.2 in regards to these fields, so I'm at a loss for why this "Debug is on (level 1)" message is still being displayed in memberentrygen.pl, even when DebugLevel is set to "no" :S. 

Any thoughts?
Comment 28 Owen Leonard 2012-11-27 17:59:03 UTC
(In reply to comment #27)
> I'm getting 3 "Use of uninitialized value in concatenation (.) or string"
> errors (for "dateofbirth: \tdateenrolled: \tdateexpiry", I think). 
> 
> I'm also getting 3 "xsub: [(($6||0),($5||0),($4||0),$3, $2 - 1, $1 - 1900)]"
> errors. 

Are these still problems? I have heard no other reports about it.
Comment 29 David Cook 2012-11-28 20:18:28 UTC
(In reply to comment #28)
> (In reply to comment #27)
> > I'm getting 3 "Use of uninitialized value in concatenation (.) or string"
> > errors (for "dateofbirth: \tdateenrolled: \tdateexpiry", I think). 
> > 
> > I'm also getting 3 "xsub: [(($6||0),($5||0),($4||0),$3, $2 - 1, $1 - 1900)]"
> > errors. 
> 
> Are these still problems? I have heard no other reports about it.

Well, the warning messages themselves aren't really a problem, but there is still this "Debug is on (level 1)" message on the memberentry.pl page which is irritating. It even appears when DebugLevel is turned off :/.
Comment 30 David Cook 2012-11-28 20:19:54 UTC
(In reply to comment #28)
> (In reply to comment #27)
> > I'm getting 3 "Use of uninitialized value in concatenation (.) or string"
> > errors (for "dateofbirth: \tdateenrolled: \tdateexpiry", I think). 
> > 
> > I'm also getting 3 "xsub: [(($6||0),($5||0),($4||0),$3, $2 - 1, $1 - 1900)]"
> > errors. 
> 
> Are these still problems? I have heard no other reports about it.

However, since it's most likely not related to this bug, I'll file a new report for it.