ryan.lane wrote:

A patch that makes SpecialUserLogin.php check $wgAuth before setting the password.

Attached:

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.

ryan.lane wrote:

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

ryan.lane wrote:

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.

fernandoacorreia wrote:

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?

fernandoacorreia wrote:

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.

d1capelis wrote:

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

fernandoacorreia wrote:

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

ryan.lane wrote:

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

Webbed.Pete wrote:

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

fernandoacorreia wrote:

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.

ryan.lane wrote:

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.

d1capelis wrote:

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