Modify

#10681 closed defect (fixed)

User with empty password can't reset their password

Reported by: rjollos Owned by: hasienda
Priority: normal Component: AccountManagerPlugin
Severity: normal Keywords: empty password
Cc: Trac Release:

Description

This is perhaps a bit insane of a scenario which came about when testing in a dev environment, where I do things like create users with empty passwords which I would never do on a production system. Still, it potentially reveals some odd corner cases, so I wanted to capture it and let the developer decide what to do with it (with myself having a wide open mind to wontfix perhaps being the most appropriate handling).

To reproduce:

  1. Create a new user with an empty password from the admin page.
  2. Select Reset password for the user from the admin page.
  3. Login as the user and verify that you are being forced to change the password.
  4. Attempt to change the password, which will result in:


The user is not logged out and presented with an authentication dialog, which seems to be the typical response on a successful password change. Subsequent attempts to change the password have the same result, except the Thank you notice goes away after the first attempt.

This ties in to the need to have a minimum allowed password length. Is that something the AccountManagerPlugin should eventually support, or should the store be enforcing a minimum length?

Attachments (2)

PasswordCannotBeChanged.png (22.0 KB) - added by rjollos 22 months ago.
PasswordCannotBeEmpty.png (13.6 KB) - added by rjollos 22 months ago.

Download all attachments as: .zip

Change History (11)

Changed 22 months ago by rjollos

comment:1 Changed 22 months ago by rjollos

  • Component changed from AnnouncerPlugin to AccountManagerPlugin

Ah, sorry, I filed this against the wrong plugin (it was a late night and I was multitasking between the two plugins). It finally registered following our discussion on IRC ;)

comment:2 follow-up: Changed 22 months ago by rjollos

Follow-on to the IRC discussion: I did not have BasicCheck enabled. My configuration at the time this ticket was opened is shown in ticket/9942. I've enabled it now:

acct_mgr.admin.accountmanageradminpanel = enabled
acct_mgr.api.accountmanager = enabled
acct_mgr.guard.accountguard = enabled
acct_mgr.htfile.htpasswdstore = enabled
acct_mgr.macros.accountmanagerwikimacros = enabled
acct_mgr.notification.accountchangelistener = enabled
acct_mgr.notification.accountchangenotificationadminpanel = enabled
acct_mgr.pwhash.htpasswdhashmethod = enabled
acct_mgr.register.basiccheck = enabled
acct_mgr.register.usernamepermcheck = enabled
acct_mgr.web_ui.accountmodule = enabled
acct_mgr.web_ui.emailverificationmodule = disabled
acct_mgr.web_ui.loginmodule = enabled
acct_mgr.web_ui.registrationmodule = disabled
acct_mgr.web_ui.resetpwstore = enabled

With BasicCheck enabled, I get the proper warning:


If BasicCheck is not enabled though, the user probably shouldn't blocked from changing their password, even if the old password is empty.

Changed 22 months ago by rjollos

comment:3 in reply to: ↑ 2 Changed 22 months ago by hasienda

  • Keywords empty password added

Replying to rjollos:

If BasicCheck is not enabled though, the user probably shouldn't blocked from changing their password, even if the old password is empty.

I've just checked that it works from me (well I bet, you know, why I had to confirm that immediately) - it does. Glad, you already confirmed that too.

Yes, your suggestion looks right. Still watch out for the side-effect of later registration check configuration changes, as hinted on IRC. You may still shoot yourself in the foot after fixing the issue here, if you enable 'BasicCheck' just later on without fixing empty passwd accounts before.

I see two possible, related enhancements, that may require a dedicated ticket, if they remain valid after discussion and thinking it through a bit longer:

  1. Make AccountManager jump at admins, if checks included in current configuration are disabled, including the default, implicit configuration.
  2. Make AccountManager scan for insane accounts (empty password is just one option), possibly as an added countermeasure against undesired duplicate account entries in the same and/or another authentication store, or even recommend the check somehow while 'BasicCheck' is enabled.

comment:4 Changed 21 months ago by hasienda

(In [12490]) AccountManagerPlugin: Allow users to set a password for an initially password-less account, refs #10681.

Keeping 'no empty password' for user education, but only showing it on password
validation failure. But the story has only just started:

Additionally AccountModule did not check for password change success on
forced password change, unconditionally removing permission lock-down and
reporting password update even on failure. Thanks to Ryan J. Ollos for testing
and providing a detailed description of this behavior.

Closing yet another way to bypass a forced password change by just repeating
the old password as new password, what isn't accepted anymore now.

Admin users may continue to create password-less accounts. Still successful
authentication with acct_mgr.LoginModule required another change to prevent
unconditionally failed logins because of the empty password.

comment:5 Changed 21 months ago by hasienda

(In [12491]) AccountManagerPlugin: Skip BotTrapCheck for admin users, refs #7577 and #10681.

Otherwise admin users couldn't create new user accounts with this check
enabled for the new user registration procedure.
Background: There's no way to pass that check there, because the hidden field
isn't supplied to user admin panel, but to the registration form exclusively.

comment:6 Changed 21 months ago by hasienda

Ryan, as you see, this was far from becoming a wont-fix as is seemed initially.

I confess, that I underestimated this ticket too, or I had hold-off yesterdays acct_mgr-0.4.2 release a bit to include a least these fixes. Well, it was before looking closer at the code tonight. Now I'm exceptionally glad about you taking your time to report the initial issue here.

comment:7 follow-up: Changed 21 months ago by rjollos

Changes look good to me, and following the procedure described in comment:description after updating past r12491 of AccountManager plugin trunk, I am no longer experiencing the issues reported here. I believe this ticket is can be resolved when 0.4.3 of AccountManager is released.

comment:8 in reply to: ↑ 7 Changed 21 months ago by hasienda

Replying to rjollos:

Changes look good to me, and ... I am no longer experiencing the issues reported here. I believe this ticket is can be resolved when 0.4.3 of AccountManager is released.

Thanks for this positive confirmation. I've been testing this too before, of course. As for the release you may notice from my silent planning in changelog that I don't intend to do another maintenance release, rather try to push all further changes towards acct_mgr-0.5, because there are quite a few new features and more invasive fixes, pretty much to justify a new "major" release.

comment:9 Changed 19 months ago by hasienda

  • Resolution set to fixed
  • Status changed from new to closed

(In [12610]) AccountManagerPlugin: Publish maintenance release 0.4.3, closes #8927, #10681, #10765 and #10871.

This is another update for current stable acct_mgr-0.4 to immediatly push
the fix against trac.ini corruption and other recent corrections.

Note though that an unnecessary msgid change from [12490] has been excluded
and - as exception to the rule - there is a solution rated as 'feature' too.

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.