User with empty password can't reset their password

[Ryan J Ollos]

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?

[Ryan J Ollos]

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 ;)

[Ryan J Ollos]

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.

[Steffen Hoffmann]

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.

[Steffen Hoffmann]

(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.

[Steffen Hoffmann]

(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.

[Steffen Hoffmann]

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.

[Ryan J Ollos]

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.

[Steffen Hoffmann]

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.

[Steffen Hoffmann]

(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.