I've been seeing an error in my PHP logs whenever the user.login() service is called and the username/password is invalid.

Looking at the code in user_service.inc, the login function performs user_authenticate() and immediately assigns the results to the global $user object. When the login is valid, this behavior is fine. But when user_authenticate() fails (i.e. invalid username or password), the global $user object becomes NULL. This causes errors for any other modules that have code that runs after the user.login() call because they expect a valid $user object, even if it's just an Anonymous user.

Attached is a patch that checks the return value of user_authenticate() before assigning it to the global $user object.

CommentFileSizeAuthor
user_services-anon_fix.patch1.12 KBhargobind

Comments

kylebrowning’s picture

kylebrowning commented
Status: Active » Reviewed & tested by the community

Looks good. Someone else should review once before I commit.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Reviewed & tested by the community » Needs review

If this is for D6, then the user_authenticate() function itself assigns the global user variable and performs hook_user('login') for you when it is successful, so I'm seeing duplication of code.

Status: Needs review » Needs work

The last submitted patch, user_services-anon_fix.patch, failed testing.

marcingy’s picture

marcingy commented
Status: Needs work » Closed (works as designed)

After reading #2 this is a won't fix as the global user has already been updated to value loaded so this patch changes nothing.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Closed (works as designed) » Needs work

Well there's some things still worth fixing here - like not actually assigning $user to the result of user_authenticate() and removing the invocation of hook_user('login')

marcingy’s picture

marcingy commented
Version: 6.x-2.4 » 7.x-3.x-dev

bumping version

kylebrowning’s picture

kylebrowning commented
Status: Needs work » Fixed

This is actually already fixed in the 3.x source and the module invoke is required in 7 because user_authenticate does not invoke that.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Version: 7.x-3.x-dev » 6.x-3.x-dev
Status: Fixed » Needs work

OK, then I'm not sure why this still wouldn't apply to 6.x-3.x?

 323 function _user_resource_login($username, $password) {
 324   global $user;
 325 
 326   if ($user->uid) {
 327     // user is already logged in
 328     return services_error(t('Already logged in as !user.', array('!user' => $user->name)), 406);
 329   }
 330 
 331   $user = user_authenticate(array('name' => $username, 'pass' => $password));
 332 
 333   if (isset($user->uid) && $user->uid) {
 334     // Regenerate the session ID to prevent against session fixation attacks.
 335     sess_regenerate();
 336     module_invoke_all('user', 'login', NULL, $user);
 337 
 338     $return = new stdClass();
 339     $return->sessid = session_id();
 340     $return->session_name = session_name();
 341     $return->user = $user;
 342 
 343     return $return;
 344   }
 345   session_destroy();
 346   return services_error(t('Wrong username or password.'), 401);
 347 }
kylebrowning’s picture

kylebrowning commented
Status: Needs work » Fixed

Fixed in 6.x-3.x, thanks Dave

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.