Patron password from phone number with extension

Bug #794153 reported by Thomas Berezansky
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Evergreen
Fix Released
Wishlist
Jason Etheridge

Bug Description

When supporting extensions on phone numbers the last 4 characters of the phone number may not be suitable for use with the "use last 4 digits of phone number as password" option.

Since the addition of regular expression validation there is another option: Use a capture group from the regular expression to decide what to pull out as the password.

I have pushed a working branch that does just that.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;<email address hidden>/phone_pw

Revision history for this message
Mike Rylander (mrylander) wrote :

To what should this be targeted?

Revision history for this message
Thomas Berezansky (tsbere) wrote :

Well, the furthest back it can go is 2.1, as I don't believe 2.0 got the validation settings backported for patron registration, and this depends on them. Unless there is a desire to go backport that as well.

Thus, if it should be backported to 2.1 not depends on whether this is a feature or a bugfix.

Argument for feature is "new behavior" in that the system didn't let you pick what part of the phone number was the part to be used and changes (slightly or otherwise) the meaning of a setting.

Argument for bugfix is "fixing existing behavior" as it allows one to fix the case where the last 4 characters of the phone number aren't actually the piece needed and doesn't actually need a new setting.

I would lean towards calling it a bugfix, personally, if only so that the setting can be more consistent from 2.1 to 2.2.

Revision history for this message
Dan Scott (denials) wrote : Re: [Bug 794153] Re: Patron password from phone number with extension

One could argue that this change as described in the commit comment
introduces a bug - at my university, for example, this would be
disastrous because everybody would have the initial password "1151" if
this branch is merged and the setting was enabled with the regex in
the commit comment. I expect the same problem could still occur for
people who work at the same place who are served by a public library.
(Although employees of the same workplace would know each other's
extension, too, so the added security of using a less uniform
phone-based password is minimal).

At a minimum, before this does go in, I think the description for the
phone regex settings in the seed data should be updated to include an
example regex and describe the potential use of a capture group in the
case of day_phone (with another corresponding example showing the
capture group). Otherwise the feature is only usable by those who have
read this bug or the corresponding commit / code.

Overall, though, I think phone-based passwords, or really any sort of
predictable password, are a bad security practice. I would prefer to
rip this anti-feature out, but if we're going to keep the option in
our code base our documentation, we should at least warn against its
use in the configuration UI and in the documentation.

Revision history for this message
Thomas Berezansky (tsbere) wrote :

As much as I also disagree with phone based passwords, we are likely stuck with them.

I force-pushed the branch with a couple of new commits for changing the descriptions on the regex fields.

Changed in evergreen:
importance: Undecided → Wishlist
status: New → In Progress
assignee: nobody → Thomas Berezansky (tsbere)
Changed in evergreen:
assignee: Thomas Berezansky (tsbere) → nobody
Changed in evergreen:
assignee: nobody → Jason Etheridge (phasefx)
Revision history for this message
Jason Etheridge (phasefx) wrote :

Pushed to master. Also tweaked the description for the "Patron: password from phone #" setting for truth in advertising.

http://git.evergreen-ils.org/?p=Evergreen.git;a=commitdiff;h=7de40306a12f46c271441ce2ceac0708d4f56c4e

Changed in evergreen:
status: In Progress → Fix Committed
tags: removed: pullrequest
Changed in evergreen:
milestone: none → 2.2.0
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.