tpac: Check for password strength at login

Bug #1013786 reported by Kathy Lussier on 2012-06-15
38
This bug affects 8 people
Affects Status Importance Assigned to Milestone
Evergreen
Medium
Unassigned

Bug Description

Evergreen version: 2.2 RC1

Currently, tpac does not check a patron's password strength at login as was done in jspac. This wishlist request is for tpac to also check for password strength, and, if it does not meet the password requirements of the system, prompt the patron to change his/her password. This feature is useful because libraries might be registering users with weak passwords with the expectation that the password will be changed as soon as the patron logs into the catalog for the first time.

Michael Peters (mrpeters) wrote :

+1 on this

I hate the idea that people will get their initial 4 digit pin and never be forced to change it when we migrate to a later 2.x release.

Changed in evergreen:
importance: Undecided → High
status: New → Confirmed
Jason Stephenson (jstephenson) wrote :

I agree this is a bug. Whether or not it needs to be fixed in the 2.2 series is open to discussion, so I've left the Importance there as Undecided.

Changed in evergreen:
milestone: none → 2.3.0-alpha2
Changed in evergreen:
milestone: 2.3.0-alpha2 → 2.3.0-beta1
importance: High → Medium
Changed in evergreen:
milestone: 2.3.0-beta1 → 2.3.0-beta2
Ben Shum (bshum) on 2012-08-24
Changed in evergreen:
milestone: 2.3.0-beta2 → 2.3.0-rc1
Changed in evergreen:
milestone: 2.3.0-rc1 → 2.3.0
Changed in evergreen:
milestone: 2.3.0 → 2.3.1
Changed in evergreen:
milestone: 2.3.1 → 2.4.0-alpha
Jeff Godin (jgodin) wrote :

I'm working on this currently. Plan is to check password against the password regex on login to TPAC, direct the patron to a "we recommend that you set a new password" interface.

Changed in evergreen:
assignee: nobody → Jeff Godin (jgodin)
Jeff Godin (jgodin) on 2012-11-29
tags: added: jspacremovalblocker
Ben Shum (bshum) on 2013-03-03
Changed in evergreen:
milestone: 2.4.0-alpha1 → 2.4.0-beta
Ben Shum (bshum) on 2013-03-17
Changed in evergreen:
milestone: 2.4.0-beta → 2.4.0-rc
Ben Shum (bshum) on 2013-04-27
Changed in evergreen:
milestone: 2.4.0-rc → 2.5.0-alpha
Dan Wells (dbw2) on 2013-06-12
Changed in evergreen:
milestone: 2.5.0-m1 → 2.5.0-m2
Dan Wells (dbw2) on 2013-07-15
Changed in evergreen:
milestone: 2.5.0-m2 → 2.5.0-alpha1
Remington Steed (rjs7) on 2013-08-12
Changed in evergreen:
milestone: 2.5.0-alpha1 → 2.5.0-alpha2
Ben Shum (bshum) on 2013-08-22
no longer affects: evergreen/2.2
Dan Wells (dbw2) on 2013-08-26
Changed in evergreen:
milestone: 2.5.0-alpha2 → 2.5.0-beta1
Dan Wells (dbw2) on 2013-09-29
Changed in evergreen:
milestone: 2.5.0-beta1 → 2.5.0-rc
Dan Wells (dbw2) on 2013-10-07
Changed in evergreen:
milestone: 2.5.0-rc → 2.next
Dan Wells (dbw2) on 2014-02-05
Changed in evergreen:
milestone: 2.6.0-alpha1 → none
Jeff Godin (jgodin) on 2014-02-11
Changed in evergreen:
milestone: none → 2.6.0-beta1
Dan Wells (dbw2) on 2014-02-27
Changed in evergreen:
milestone: 2.6.0-beta1 → 2.next
Jeff Godin (jgodin) on 2015-06-11
no longer affects: evergreen/2.3
no longer affects: evergreen/2.4
Jeff Godin (jgodin) on 2015-06-12
no longer affects: evergreen/master
Jeff Godin (jgodin) on 2015-06-12
Changed in evergreen:
milestone: 2.next → 2.9-alpha
Changed in evergreen:
milestone: 2.9-alpha → 2.9-beta
tags: added: pullrequest
Michael Peters (mrpeters) wrote :

A related bug https://bugs.launchpad.net/evergreen/+bug/1486151 that spawned from discussion on how to fix this. Sharing here so it doesn't get lost in the shuffle.

tags: removed: pullrequest
Jeff Godin (jgodin) wrote :

Branch containing a variant of what we've been using in production for a few years now: user/jeff/lp1013786_tpac_initial_password

Adding pullrequest, for anyone interested enough to accept the challenge. :-)

Changed in evergreen:
assignee: Jeff Godin (jgodin) → nobody
tags: added: pullrequest
Changed in evergreen:
milestone: 2.9-beta → 2.next
Changed in evergreen:
assignee: nobody → Christine Burns (christine-burns)
Kathy Lussier (klussier) wrote :

Based on the discussion at http://irc.evergreen-ils.org/evergreen/2015-12-15#i_219933 , I'm adding a needstest tag to this bug. There was a bit of uncertainty as to whether the guidelines for regression tests from http://wiki.evergreen-ils.org/doku.php?id=dev:contributing:qa applied to new features, but we can have that discussion here if we think they shouldn't be required for this code.

Thanks Jeff!

tags: added: needstest
removed: jspacremovalblocker
Erica Rohlfs (erohlfs) wrote :

I have tested this code and consent to signing off on it with my name, Erica Rohlfs and email address, <email address hidden>.

Changed in evergreen:
assignee: Christine Burns (christine-burns) → nobody
Erica Rohlfs (erohlfs) on 2015-12-15
tags: added: signedoff

I have tested this code and consent to signing off on it with my name, Christine Burns and email address, <email address hidden>.

Galen Charlton (gmc) wrote :

Kathy asked me to comment on the use of needstest tag here. An automated regression test would be a good thing to have -- and I suggest that Jeff or somebody else consider a follow-up to move the password strength check via regex to a self-contained routine (say in OpenILS::WWW::EGCatLoader::Util) that would be reachable to a direct Test::More test.

However, given the age of the patch and the user testing that has taken place so far, for this one it would be sufficient to just meet the new requirement that a written test plan be supplied. This could be done as a new commit message or just via a comment in this bug by one of the testers or by Jeff.

Jason Stephenson (jstephenson) wrote :

I took a look at this branch today because we were reviewing open tickets in our internal ticket system and we have one on this issue from long ago.

I could probably do what Galen suggest in comment #10 to move the check to a self-contained routine in EGCatLoader::Util and add a test.

I wonder if there should be a setting to disable this feature or not? Currently, a default is used if the password regex setting is not set. Maybe, it should only function if the setting is set as a way to disable this functionality. Alternately, YAOUS could be added to turn the checking off.

Some of our staff seem happy that this feature disappeared when we left JSPAC, despite that someone saw fit to open a ticket about it going away.

> I could probably do what Galen suggest in comment #10 to move the check
> to a self-contained routine in EGCatLoader::Util and add a test.

Thanks!

> I wonder if there should be a setting to disable this feature or not?
> Currently, a default is used if the password regex setting is not set.
> Maybe, it should only function if the setting is set as a way to disable
> this functionality. Alternately, YAOUS could be added to turn the
> checking off.

I'm neutral on whether or not it should be possible to disable the
feature (and to be clear, I mean the feature of "nagging upon login if
the password is weak per the regex), but if you write code to do it, I
suggest making a new YAOUS.

Thomas Berezansky (tsbere) wrote :

Disabling could be a matter of "set the regex to wide open" but then you don't have password rules...but that is what I think is basically happening if you don't tell people to change their weak passwords in the first place.

Erica Rohlfs (erohlfs) on 2017-02-13
Changed in evergreen:
assignee: nobody → Erica Rohlfs (erohlfs)
Blake GH (bmagic) wrote :

I am getting a merge conflict OpenILS/WWW/EGCatLoader.pm when merging against master today. I took a look at the details and decided I shouldn't attempt to resolve it. I won't be able to get this setup for bug squashing day.

Kathy Lussier (klussier) wrote :

I'm just adding a note that we already have two signoffs on this code. Once the merge conflict is resolved, it probably should be tested on, but the code was waiting on a couple of things:

- Jason had said he was going to move the check to a self-contained routine as had previously been suggested by Galen. Given that the comment was made a year ago, I'm guessing Jason is no longer planning to do so.

- Galen had asked that somebody (Jeff? Previous tester?) write a test plan in either the bug report or the commit message. I would suggest that it be done by whomever resolves the merge conflicts.

Until that last issue is resolved, I'm going to remove the pullrequet tag. This is a bug/feature that gets a lot of attention on Bug Squashing Day because a lot of people want to see it make its way into the code, but we just need a few small things done to it before it makes its way in.

tags: removed: pullrequest
Changed in evergreen:
assignee: Erica Rohlfs (erohlfs) → nobody
Jason Stephenson (jstephenson) wrote :

Hmm.. Yeah. I don't even remember making that comment, now.

I suppose I could take another look during bug squashing week.

I'll assign myself with that in mind.

Changed in evergreen:
assignee: nobody → Jason Stephenson (jstephenson)
Jason Stephenson (jstephenson) wrote :

I never got around to looking at this during bug squashing week. I'm removing myself as assigned because I'm not sure when I will get the chance to look at this.

Changed in evergreen:
assignee: Jason Stephenson (jstephenson) → nobody
Terran McCanna (tmccanna) wrote :

This one was signed off two years ago, but there was still some discussion going on - anyone want to take a crack and reapplying it to current master and seeing if it works?

tags: added: opac
removed: tpac
Elaine Hardy (ehardy) on 2019-03-08
tags: removed: needstest
tags: added: needstest
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers