Last modified: 2010-05-15 15:48:27 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 8815 - Setting password in initUser() breaks LdapAuthentication plugin
Setting password in initUser() breaks LdapAuthentication plugin
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
User login and signup (Other open bugs)
1.9.x
PC Linux
: Normal normal with 4 votes (vote)
: ---
Assigned To: Nobody - You can work on this!
:
: 9271 9778 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-28 23:18 UTC by Ryan Lane
Modified: 2010-05-15 15:48 UTC (History)
2 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
A patch that makes SpecialUserLogin.php check $wgAuth before setting the password. (735 bytes, patch)
2007-01-28 23:20 UTC, Ryan Lane
Details

Description Ryan Lane 2007-01-28 23:18:51 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().
Comment 1 Ryan Lane 2007-01-28 23:20:48 UTC
Created attachment 3152 [details]
A patch that makes SpecialUserLogin.php check $wgAuth before setting the password.
Comment 2 Brion Vibber 2007-01-29 13:56:06 UTC
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.
Comment 3 Ryan Lane 2007-01-29 14:06:11 UTC
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.
Comment 4 Brion Vibber 2007-02-02 18:50:13 UTC
Well, creating new accounts from a manual form which requires a password doesn't
make sense if you can't set passwords... o_O

Comment 5 Ryan Lane 2007-02-02 19:46:14 UTC
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.
Comment 6 Fernando Correia 2007-02-22 16:38:26 UTC
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.
Comment 7 Brion Vibber 2007-02-23 06:32:31 UTC
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?
Comment 8 Fernando Correia 2007-02-23 10:19:26 UTC
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.
Comment 9 D.J. Capelis 2007-03-07 01:00:11 UTC
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.)
Comment 10 Fernando Correia 2007-03-07 11:17:22 UTC
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().
Comment 11 Ryan Lane 2007-03-07 15:02:02 UTC
(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.
Comment 12 Brion Vibber 2007-03-13 18:13:10 UTC
*** Bug 9271 has been marked as a duplicate of this bug. ***
Comment 13 MrPete 2007-03-13 20:51:23 UTC
(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.

Comment 14 Fernando Correia 2007-03-13 20:54:25 UTC
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.
Comment 15 Ryan Lane 2007-03-13 20:55:11 UTC
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.
Comment 16 D.J. Capelis 2007-03-13 20:58:37 UTC
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.
Comment 17 Brion Vibber 2007-05-04 14:13:23 UTC
*** Bug 9778 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links