Last modified: 2010-05-15 15:48:27 UTC
When an AuthPlugin is set to not allow password changes, it will return false; however, since SpecialUserlogin calls $u->setPassword(), if the user's password is not allowed to change it throws an error message. This causes AuthPlugins that use setPassword() to break. New users cannot be created. SpecialUserlogin should check $wgAuth->allowPasswordChange() before calling $u->setPassword().
Created attachment 3152 [details] A patch that makes SpecialUserLogin.php check $wgAuth before setting the password.
User::setPassword is a high-level function which sets the password on the account, through the AuthPlugin (if it's present) or in the local database (otherwise). If password changes are not allowed, then an exception is thrown. AuthPlugin shouldn't be calling User::setPassword, generally, since that calls into AuthPlugin.
Right. I'll be changing that in the next release of my plugin. However, the issue is that SpecialUserlogin calls the User->setPassword() method in its initUser() function without checking if the authentication plugin allows password changes. If the authentication plugin doesn't allow password changes, an exception gets thrown and user creation fails; hence the patch that fixes specifically that. This can be replicated by setting setPassword() in AuthPlugin.php to return false, emulating an authentication plugin that isn't allowing password changes. When you try to create an account it'll fail.
Well, creating new accounts from a manual form which requires a password doesn't make sense if you can't set passwords... o_O
LoginForm->initUser() gets called whether you create accounts from the manual form, or not, like in the case of user auto-creation. The LdapAuthentication plugin actually disables the user creation form (and uses user auto-creation). The process goes the following way when a user logs in and doesn't have an account: LoginForm->execute() LoginForm->processLogin() LoginForm->authenticateUserData() <- sees the user doesn't exist yet, so it calls $wgAuth->authenticate() to make sure the user exists and can authenticate, if so, the process continues: LoginForm->initUser() User->setPassword() In User->setPassword(): $wgAuth->allowPasswordChange() <- returns false because the plugin can't set external passwords In User->setPassword(): throw new PasswordError( wfMsg( 'password-change-forbidden' ) ); <- user creation stops here --- $wgAuth->setPassword() is supposed to be used to set a password in the external database, not the internal one. SpecialUserlogin shouldn't call User->setPassword() in user creation if the authentication database doesn't allow it.
This is a breaking change from version 1.8. And it does make sense to use this feature. In our corporate wiki the users can login through the wiki, get authenticated through the AuthPlugin, and the AuthPlugin will automatically create a user on the wiki database. All this was working before. Now, because of this breaking change, I will have to alter the AuthPlugin to ALLOW password changes, what I wouldn't like to be allowed, just so the password can be set on the first time. In my opinion, either SpecialUserLogin::initUser() shouldn't call setPassword() if the authPlugin does not allow password changes or User::setPassword() shouldn't check for allowPasswordChange() if the user is new, because in this case the password isn't being changed, it is being SET FOR THE FIRST TIME and the "allow password changes" restriction should not apply.
Ok, so what you want to do is *not call setPassword()*, since you're never setting a password. The password is already set remotely, in the LDAP database. Yes?
It seems to me that the behavior of version 1.8 was OK. It did set the password for new user records, even if the AuthPlugin restricted password changes. A new password is not a password change. I think an alternative would be not to call setPassword(), but only if this did not break something else. Actually I wouldn't mind if the password on the user table is null, provided this password field was never used when an AuthPlugin is defined. I am using the "authenticate" method in the AuthPlugin to check the password against the remote database.
I would also like to add that this broke the Shibboleth Authentication (http://meta.wikimedia.org/wiki/Shibboleth_Authentication) and SSL Authentication (http://www.mediawiki.org/wiki/Extension:SSL_authentication) extensions as well. We currently have fixed the bug by implementing code that catches recursive cases and stubs out as well as implementing code that straight up lies to mediawiki about what it's doing. Clearly... non-optimal. (Since this code has already been distributed widely we're likely going to end up with these hacks in our extensions for a long time to come... oh well.) I would ask that this be reverted back to pre-1.9 behavior as it's usefulness to authentication extension developers is sketchy at best (we already can do that elsewhere more cleanly) and forces them to implement really rather nasty hacks to make their extensions compatible with these versions which is bad for everyone. In addition, I'll note in reply to your question above that at least the Shibboleth Authentication Plugin uses setPassword to scramble the password in the database so as to make it difficult to login to a user created by an Authentication Plugin through regular mechanisms. (The Shibboleth Authentication plugin does not always activate itself as an authentication plugin and still allows regular users to exist alongside Shibboleth users for those without Shibboleth accounts, so scrambling the password on accounts created by the plugin is necessary.)
About this proposed patch: http://bugzilla.wikimedia.org/attachment.cgi?id=3152&action=view I agree it works around the behavior change, but I think the semantics is wrong. $wgAuth->allowPasswordChange() means that the user can *change* the password; what if I don't want him to be able to *change* it, but I still want *new users* to have an *initial* password? I think the setting of the initial password of non-persisted user objects should not count as a password change and should not be blocked by User::setPassword().
(In reply to comment #10) > About this proposed patch:http://bugzilla.wikimedia.org/attachment.cgi?id=3152&action=viewI agree it works around the behavior change, but I think the semantics is wrong.$wgAuth->allowPasswordChange() means that the user can *change* the password;what if I don't want him to be able to *change* it, but I still want *new users*to have an *initial* password?I think the setting of the initial password of non-persisted user objects shouldnot count as a password change and should not be blocked by User::setPassword(). You can still set an initial password if the patch is used; however, you would need to set the password in $wgAuth- >initUser() manually and save the user's settings. It would be better if the auth plugin could just call $wgUser- >setPassword() to set the password so that we don't have to duplicate code, but that would cause issues. Shouldn't we instead have two password setting functions in User? One function can have the current behavior, and the other one will just set the password with no checks. In this case auth plugins can call the latter of the functions, instead of duplicating code. I'll send another patch in for that if it is acceptable.
*** Bug 9271 has been marked as a duplicate of this bug. ***
(In reply to comment #11) > You can still set an initial password if the patch is used; however, you would need to set the >password in $wgAuth->initUser() manually and save the user's settings. It would be better if the >auth plugin could just call $wgUser->setPassword() to set the password so that we don't have >to duplicate code, but that would cause issues. > > Shouldn't we instead have two password setting functions in User?... > I'll send another patch in for that if it is acceptable. I don't believe this bug is resolved until Ryan's new patch is created. As it stands, the system is broken under the following conditions: * External auth prohibits user (UI) password changes * External auth, for security, desires to install a non-blank password (as with Shibboleth above) When $wgUser->setPassword() is called, it fails with an exception. (This is what was reported as bug 9271.) So, we do need a second "setPassword but not from the UI" function if the primary one will still contain this test.
I agree that a correct solution of this bug implies in restoring functionality that existed in version 1.8, that is, being able to prevent password changes on the UI while still setting an initial password when the user is created.
Please have a look at revision 20300: http://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/includes/User.php?view=log Authentication plugins should now use $wgUser->setInternalPassword(). This is fixed in 1.10, not sure if it will be fixed for 1.9.
In addition, changing mPassword on the user and calling SaveSettings() is still a workable solution. (My extensions have been changed to do this now.) Marking as Resolved->Fixed once again.
*** Bug 9778 has been marked as a duplicate of this bug. ***
