Like many here, I've been having issues with my users logging into a recently converted forum, from SMF 1.1 to MyBB 1.6
I updated the loginconvert.php plugin as per the instructions elsewhere, to no avail.
The symptoms were that if a user mistyped their password on their very first post-merge login attempt, they would be effectively locked out of the system. When this happened, the mybb_users would have two password entries: One for MyBB, and the original SMF password as well. The short-term fix was to delete the MyBB password entry, and have them try again, or to use the lost password link.
A little digging in loginconvert.php, and I have a fix for the problem that seems to be holding on my board. The problem is that the very first time a password is entered, a MyBB hash is created BEFORE the password is checked against the original SMF hash. Once there is a MyBB hash in the DB, the SMF hash (which doesn't get deleted on a failed login) is never referenced again. So the user cannot log in.
The simple fix is to prefer the SMF hash if it is available, regardless of what the MyBB hash might be. Thus, in function loginconvert_validate_password_from_uid change line 246 from
if(isset($user['passwordconvert']) && trim($user['passwordconvert']) != '' && trim($user['passwordconverttype']) != '' && trim($user['password']) == '')
to
if(isset($user['passwordconvert']) && trim($user['passwordconvert']) != '' && trim($user['passwordconverttype']) != '')
Problem solved: While the user mistypes their password, even as MyBB hashes are created and stored, the plugin will always prefer the SMF hash to the MyBB hash. On successful login, the old hash is deleted from the DB, so the last password stored in the MyBB hash is the correct one.
Capt.
I didn't ignore this when it was first posted, however I was busy at that time. When I revisited it, I was on too many pain meds from the tooth I broke and it didn't make sense. I'm off the meds now and checked this out again, and you have a very valid issue here.
I am going to address this in a different way, but the end result will be the same. While your method is a simple change, I feel it is better to verify the password before creating a MyBB password hash & inserting it into the database. Thus, the plugin will be re-written to reflect this. Thank you for reporting it and giving such a clear and concise description of the issue.
FYI, this affects all boards, not just SMF.
Ok, talk about open mouth and insert foot. I went only by your post since it was so well written, and now that I've actually looked at the code I see that you are incorrect. A password hash is already only created if a login attempt is correct.
/*
* This class allows us to take the encryption algorithm used by the convertee bulletin board with the plain text password
* the user just logged in with, and match it against the encrypted password stored in the passwordconvert column added by
* the Merge System. If we have success then apply MyBB's encryption to the plain-text password.
*/
class loginConvert
{
var $user;
function loginConvert($user)
{
$user['passwordconvert'] = trim($user['passwordconvert']);
$this->user = $user;
}
function login($type, $uid, $password)
{
global $db;
$password = trim($password);
switch($type)
{
case 'vb3':
$return = $this->authenticate_vb3($password);
break;
case 'ipb2':
$return = $this->authenticate_ipb2($password);
break;
case 'smf11':
$return = $this->authenticate_smf11($password);
break;
case 'smf2':
$return = $this->authenticate_smf2($password);
break;
case 'smf':
$return = $this->authenticate_smf($password);
break;
case 'punbb':
$return = $this->authenticate_punbb($password);
break;
case 'phpbb3':
$return = $this->authenticate_phpbb3($password);
break;
default:
return false;
}
if($return == true)
{
// Generate a salt for this user and assume the password stored in db is a plain md5 password
$user['salt'] = generate_salt();
$this->user['salt'] = $user['salt'];
$user['password'] = salt_password(md5($password), $user['salt']);
$this->user['password'] = $user['password'];
$user['loginkey'] = generate_loginkey();
$this->user['loginkey'] = $user['loginkey'];
$user['passwordconverttype'] = '';
$this->user['passwordconverttype'] = '';
$user['passwordconvert'] = '';
$this->user['passwordconvert'] = '';
$db->update_query("users", $user, "uid='{$uid}'", 1);
return $this->user;
}
return false;
}
As you can see in the class snippet above, a MyBB password hash is only created and inserted into the database if $return = true, otherwise it simply skips to a return false; and exits. So this isn't correct, and your suggested fixed above will actually force the plugin to run every single time instead of only once. Sorry.
The problem I do see elsewhere however is that we are validating the password twice if it exists in the database already. OOPS! We really don't need to be doing that in the plugin just so that it can return after the hook call and do it all over again. Nor do we need to check and see if its an unsalted password for the same reasons.
That's very weird, because I can readily replicate the issue: If I do a fresh MyBB install, and merge my SMF DB with it, and then attempt to log in with a gibberish password (bang-on-keyboard style), then look at the mybb_users table, sure enough, there is a hash for both the mybb password and the smf password…so now I am deeply confused, because looking at the code, you should be right…
Just a guess, but perhaps $this->authenticate_smf($password) is always returning true, or at least returning true when it shouldn't?
Oh! And glad to hear you're feeling better!
(2010-10-26, 12:40 PM)CaptOblivious Wrote: [ -> ]That's very weird, because I can readily replicate the issue: If I do a fresh MyBB install, and merge my SMF DB with it, and then attempt to log in with a gibberish password (bang-on-keyboard style), then look at the mybb_users table, sure enough, there is a hash for both the mybb password and the smf password…so now I am deeply confused, because looking at the code, you should be right…
Yeah, gotta love obscure bugs eh?
Actually I do, I enjoy a challenge fixing them.
Quote:Just a guess, but perhaps $this->authenticate_smf($password) is always returning true, or at least returning true when it shouldn't?
This is the only explanation I can come up with either, but... I don't see how. Hmm. I guess some seriously in depth testing is needed.
Quote:Oh! And glad to hear you're feeling better!
Thanks, I'm glad to be feeling better too.
(2010-10-18, 08:05 PM)CaptOblivious Wrote: [ -> ]The simple fix is to prefer the SMF hash if it is available, regardless of what the MyBB hash might be. Thus, in function loginconvert_validate_password_from_uid change line 246 from
if(isset($user['passwordconvert']) && trim($user['passwordconvert']) != '' && trim($user['passwordconverttype']) != '' && trim($user['password']) == '')
to
if(isset($user['passwordconvert']) && trim($user['passwordconvert']) != '' && trim($user['passwordconverttype']) != '')
I know this is an old thread, but for what it's worth, this fixed the login issue we were having with a site I just migrated from smf 1.1 to mybb 1.6.