Bug 3674 - Creating users with empty password
Summary: Creating users with empty password
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: PATCH-Sent (DO NOT USE) normal (vote)
Assignee: Fernando L. Canizo
QA Contact: Bugs List
URL:
Keywords:
Depends on: 6218 6224
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-29 09:18 UTC by Nahuel Angelinetti
Modified: 2012-10-25 23:09 UTC (History)
7 users (show)

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


Attachments
bugfix (1.54 KB, patch)
2009-09-29 09:20 UTC, Chris Cormack
Details | Diff | Splinter Review
reimplementation (1.92 KB, patch)
2011-04-26 15:05 UTC, Fernando L. Canizo
Details | Diff | Splinter Review
Patch reworked to avoid conflict with patch from 6218 (1.74 KB, patch)
2011-05-30 17:33 UTC, Fernando L. Canizo
Details | Diff | Splinter Review
Signed-off patch with improvement (1.72 KB, patch)
2011-05-30 19:03 UTC, Frédéric Demians
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:13:35 UTC


---- Reported by nahuel.angelinetti@biblibre.com 2009-09-29 09:18:35 ----

If an user is created with empty password, his login is dropped.
Koha should allow librarians to create logins with disabled password.



---- Additional Comments From nahuel.angelinetti@biblibre.com 2009-09-29 09:20:24 ----

Created an attachment
bugfix





---- Additional Comments From nengard@gmail.com 2010-02-07 15:54:23 ----

It looks like if I don't put in a password one is auto generated - is that what this patch is supposed to do? and if so - how do I know what the password that was generated was?



---- Additional Comments From nengard@gmail.com 2010-05-14 13:00:13 ----

I never got an answer so I'm closing this bug and it can reopened if I'm wrong.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:13 UTC  ---

This bug was previously known as _bug_ 3674 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3674
Imported an attachment (id=1508)

Unknown Component Authentication
   Using default product and component set in Parameters 
Actual time not defined. Setting to 0.0
The original submitter of attachment 1508 [details] [review] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.

Comment 1 Chris Cormack 2011-04-21 02:47:14 UTC
This has been reverted due to mismatch, the password was being md5'd then compared to a non md5'd one, so the account wasnt disabled, it in turn ended up being a valid login, with a weak password
Comment 2 Fernando L. Canizo 2011-04-26 15:05:41 UTC Comment hidden (obsolete)
Comment 3 Frédéric Demians 2011-05-14 06:17:22 UTC
(In reply to comment #2)
> Created attachment 4006 [details] [review]
> reimplementation
> 
> This re-implements the fix to provide disabled logins and also adds
> auto-generated logins when none provided.
Comment 4 Frédéric Demians 2011-05-14 06:43:01 UTC
(In reply to comment #2)
> Created attachment 4006 [details] [review]
> reimplementation
> 
> This re-implements the fix to provide disabled logins and also adds
> auto-generated logins when none provided.

This patch contains code from your patch for bug 6224. So there will be
a merge conflict. It would be great to base it on a branch containing
already bug 6224 patch and note that it must be apply after 6224.

Your patch solves the bug, ie put a correct value in password field, but
there is no UI feedback telling the librarian what has been done
internally. I think it's confusing. And it's also not clear what happens
when password is modified and cleared for an existing borrower. 

So I sign-off this patch, since it fixes the describe bug but something
has to be done to improve userid/password management. The merge conflict
should be manageable by the RM...
Comment 5 Fernando L. Canizo 2011-05-30 17:33:01 UTC Comment hidden (obsolete)
Comment 6 Fernando L. Canizo 2011-05-30 18:52:07 UTC
(In reply to comment #4)
> (In reply to comment #2)
> Your patch solves the bug, ie put a correct value in password field, but
> there is no UI feedback telling the librarian what has been done
> internally. I think it's confusing. And it's also not clear what happens
> when password is modified and cleared for an existing borrower.

Regarding your concerns I'm not sure if informing the user would not add to more confusion. I think it's enough to document this behavior.

I tested this for the 4 user creation cases: no login/no password, no login/yes password, yes login/no password and yes login/yes password. Where yes/no stand for "provided". All went ok.

Also I tried modifying an already created user password, and it also went ok.

Trying to clear the password by deleting '*' in form or by deleting login+password doesn't do anything, password and login remains. In fact there's no way to clear a password or mark a user as disabled, but that's material for another bug, we will have to change C4::Members::ModMember
Comment 7 Frédéric Demians 2011-05-30 19:03:27 UTC
Created attachment 4309 [details] [review]
Signed-off patch with improvement
Comment 8 Ian Walls 2011-07-08 19:47:01 UTC
This patch does correctly set the password directly to '!', rather than the hash thereof.  I'm not necessarily a fan of the autogenerated userid, but it's consistent with existing code, so I'm marking this as Passed QA
Comment 9 Chris Cormack 2011-07-10 08:24:22 UTC
Patch pushed, please test
Comment 10 Owen Leonard 2011-08-22 13:35:41 UTC
This seems to be working as expected: If I create a patron with no password, the password column in the database is set to "!", which will not match any password submission.