Bug 12267 - Allow password option in Patron Attribute non functional
Summary: Allow password option in Patron Attribute non functional
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 16377
  Show dependency treegraph
 
Reported: 2014-05-15 21:10 UTC by Joy Nelson
Modified: 2016-12-05 21:27 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 12267: Remove borrower_attributes.password (21.57 KB, patch)
2016-04-11 09:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12267: Remove borrower_attributes.password (21.66 KB, patch)
2016-04-12 08:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12267: Remove borrower_attributes.password (21.81 KB, patch)
2016-04-22 09:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12267: [QA Follow-up] Adjust installer files; tiny text edit (10.19 KB, patch)
2016-04-22 09:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joy Nelson 2014-05-15 21:10:29 UTC
When adding (or editing) a Patron Attribute, the option to "Allow password" can be selected.  The purpose here is to allow this attribute/password to be a secondary login for the patron account.  [ Allow password: Check to make it possible to associate a password with this attribute ]

However, checking the box and then adding a patron with an attribute/password combination does not allow the patron to log on using that attribute password combination.


1. Set up New Patron Attribute, clicking Allow password option.
2. Create a new patron.  Enter patron data including the attribute and password value
3. Save patron
4. Using that attribute/password values, attempt to log on to the OPAC.
Comment 1 Katrin Fischer 2014-05-15 21:12:13 UTC
Hi Joy, I am not sure if that was the purpose of the field. The way someone explained it to me in the past was, that it allows you to store a password for another application.
Comment 2 Galen Charlton 2014-05-19 17:27:05 UTC
(In reply to Katrin Fischer from comment #1)
> Hi Joy, I am not sure if that was the purpose of the field. The way someone
> explained it to me in the past was, that it allows you to store a password
> for another application.

Actually, Joy's right - the original intent was to support secondary logins.
Comment 3 Jonathan Druart 2015-04-28 11:23:03 UTC
(In reply to Galen Charlton from comment #2)
> (In reply to Katrin Fischer from comment #1)
> > Hi Joy, I am not sure if that was the purpose of the field. The way someone
> > explained it to me in the past was, that it allows you to store a password
> > for another application.
> 
> Actually, Joy's right - the original intent was to support secondary logins.

http://lists.koha-community.org/pipermail/koha-patches/2008-May/000988.html

    - password_allowed (if set, staff patron editor will
      allow a password to be associated with a value; this
      is mostly a hook for functionality to be implemented
      in the future.

Does it mean we should keep it or the feature won't never be implemented and we can remove the 2 columns in DB?
Comment 4 Magnus Enger 2016-04-07 09:32:53 UTC
(In reply to Jonathan Druart from comment #3)
> Does it mean we should keep it or the feature won't never be implemented and
> we can remove the 2 columns in DB?

About one year since this question was asked. Anyone got an opinion?
Comment 5 Owen Leonard 2016-04-07 13:38:35 UTC
If the feature hasn't been implemented yet...

I say remove it.
Comment 6 Jonathan Druart 2016-04-11 09:19:00 UTC Comment hidden (obsolete)
Comment 7 Mark Tompsett 2016-04-12 00:06:53 UTC
Comment on attachment 50122 [details] [review]
Bug 12267: Remove borrower_attributes.password

Review of attachment 50122 [details] [review]:
-----------------------------------------------------------------

::: C4/Members/AttributeTypes.pm
@@ +217,3 @@
>                                                  ?, ?, ?, ?, ?, ?)");
>      }
> +    $sth->execute( $self->{description}, $self->{repeatable}, $self->{unique_id}, $self->{opac_display}, $self->{staff_searchable}, $self->{authorised_value_category}, $self->{display_checkout}, $self->{category_code} || undef, $self->{class}, $self->{code} );

Keeping it vertical would have been easier to read.
Comment 8 Jonathan Druart 2016-04-12 08:10:40 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2016-04-12 08:11:08 UTC
(In reply to M. Tompsett from comment #7)
> Comment on attachment 50122 [details] [review] [review]
> Bug 12267: Remove borrower_attributes.password
> 
> Review of attachment 50122 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Members/AttributeTypes.pm
> @@ +217,3 @@
> >                                                  ?, ?, ?, ?, ?, ?)");
> >      }
> > +    $sth->execute( $self->{description}, $self->{repeatable}, $self->{unique_id}, $self->{opac_display}, $self->{staff_searchable}, $self->{authorised_value_category}, $self->{display_checkout}, $self->{category_code} || undef, $self->{class}, $self->{code} );
> 
> Keeping it vertical would have been easier to read.

Patch amended.
Comment 10 Katrin Fischer 2016-04-19 06:34:41 UTC
I am a bit worried about removing those - could we bring this up on the mailing list?

Would it be possible that people used it as a hook to store login information other applications? When testing I noticed it stores the password in the database as cleartext, while hiding it **** in the GUI.
Comment 11 Jonathan Druart 2016-04-19 07:17:21 UTC
The column is deleted if not null, so we won't loose data.
The idea to store password as text in DB is a really bad one, and nobody should use this field.
Comment 12 Jonathan Druart 2016-04-19 07:24:20 UTC
(In reply to Katrin Fischer from comment #10)
> I am a bit worried about removing those - could we bring this up on the
> mailing list?

https://lists.katipo.co.nz/pipermail/koha/2016-April/045235.html
Comment 13 Katrin Fischer 2016-04-19 07:33:47 UTC
That's a good point actually.
Comment 14 Marcel de Rooy 2016-04-22 09:44:23 UTC
On April 12 Marc Veron signed off, but forgot to add a signoff line.
Will add that now.
Comment 15 Marcel de Rooy 2016-04-22 09:48:03 UTC
Created attachment 50573 [details] [review]
Bug 12267: Remove borrower_attributes.password

When creating a patron attribute type, there is a "Allow password"
checkbox. If checked, the librarian will be able to enter a password for
this patron attribute when editing a patron.
The goal was to allow a patron to log in with a secondary password.
However, this feature has never been implemented.

"""
commit 6fc62bcd321eddb0fd3ae46903e9ab6c8b1db2cd
  CommitDate: Mon May 12 09:03:00 2008 -0500
  extended patron attributes tables & syspref (DB rev 081)

- password_allowed (if set, staff patron editor will
  allow a password to be associated with a value; this
  is mostly a hook for functionality to be implemented
  in the future.
"""

To decrease maintainability, this patch suggest to remove the 2 DB fields
borrower_attributes.password and
borrower_attribute_types.password_allowed
If they have not used by the library.

Test plan:
- Edit a patron attribute type and select "allow password"
- Edit a patron and defined a password for this attribute
- Execute the DB entry
- Note that you get a warning
- Empty the password field
- Execute the DB entry
- You do not get the warning and the 2 DB fields have been removed

Signed-off-by: Marc Veron <veron@veron.ch>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2016-04-22 09:48:08 UTC
Created attachment 50574 [details] [review]
Bug 12267: [QA Follow-up] Adjust installer files; tiny text edit

Some installer files still refer to password_allowed.
The print statement of the db revision has been slightly adjusted.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2016-04-22 11:49:05 UTC
@RM: Please do not forget to update the DBIx schema !
Comment 18 Brendan Gallagher 2016-04-22 23:18:14 UTC
Pushed to Master - Should be in the May 2016 Release.  Thanks!  (running schema updates right now too)
Comment 19 Jonathan Druart 2016-04-23 07:15:24 UTC
@Rm Please update schema files.
Comment 20 Marcel de Rooy 2016-04-28 06:57:02 UTC
t/db_dependent/Members/Attributes.t adjusted on bug 16377
Comment 21 Marcel de Rooy 2016-04-28 07:04:27 UTC
And still another one: t/db_dependent/ILSDI_Services.t on bug 16320