380928 - Bugzilla::Auth::Verify::create_or_update_user can return stale Bugzilla::User object

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Vance Baarda]

Near the end of Bugzilla::Auth::Verify::create_or_update_user, it instantiates a new Bugzilla::User object. After that, it conditionally updates the profiles table with a new login_name and/or realname. The problem is, the Bugzilla::User object still contains the old info from before the updates happened.

Comment 1

[Vance Baarda]

Attachment: fix version 1

Comment 2

[Max Kanat-Alexander]

Comment on attachment 265044 [details] [diff] [review]
fix version 1

This is fine for 3.0.

For the trunk, though, we need to change this code to use $user->set_ and $user->update().

Oh also, even in this patch, change it to use $user->id instead of $user_id for consistency.

Comment 3

[Vance Baarda]

(In reply to comment #2)
> (From update of attachment 265044 [details] [diff] [review])
> This is fine for 3.0.
> 
> For the trunk, though, we need to change this code to use $user->set_ and
> $user->update().

Do you want me to submit a new patch for this? I don't have a test environment for trunk, and Verify.pm hasn't changed since 3.0....

> Oh also, even in this patch, change it to use $user->id instead of $user_id for
> consistency.

I can, but then it wouldn't be consistent with line 111. Either way is fine with me. :-)
    line 111: my $user = new Bugzilla::User($user_id);

Comment 4

[Max Kanat-Alexander]

(In reply to comment #3)
> Do you want me to submit a new patch for this? I don't have a test environment
> for trunk, and Verify.pm hasn't changed since 3.0....

  Yes, I'd like a new patch. Do you want a landfill account? I could get you one. There are nice tools on there for creating test environments.

> I can, but then it wouldn't be consistent with line 111. Either way is fine
> with me. :-)
>     line 111: my $user = new Bugzilla::User($user_id);

  Yes, but that's because $user->id doesn't exist before that. :-) It just saves anybody reading the code from having to check if $user_id and $user->id are the same.

Comment 5

[Vance Baarda]

(In reply to comment #4)
> (In reply to comment #3)
> > Do you want me to submit a new patch for this? I don't have a test environment
> > for trunk, and Verify.pm hasn't changed since 3.0....
> 
>   Yes, I'd like a new patch. Do you want a landfill account? I could get you
> one. There are nice tools on there for creating test environments.

Yes, please.
 
> > I can, but then it wouldn't be consistent with line 111. Either way is fine
> > with me. :-)
> >     line 111: my $user = new Bugzilla::User($user_id);
> 
>   Yes, but that's because $user->id doesn't exist before that. :-) It just
> saves anybody reading the code from having to check if $user_id and $user->id
> are the same.

Got it -- will do.

Comment 6

[Vance Baarda]

Comment on attachment 265044 [details] [diff] [review]
fix version 1

Will provide a better patch.

Comment 7

[Vance Baarda]

Attachment: fix version 2

This one uses set_login(), set_name(), and update().

Comment 8

[Max Kanat-Alexander]

Comment on attachment 265290 [details] [diff] [review]
fix version 2

What happens if you call $user->update() outside of either block? It's actually designed to be a no-op if nothing has changed.

Comment 9

[Vance Baarda]

If nothing has changed, the DB overhead of $user->update() is this call only:

SELECT
    profiles.userid AS id,
    profiles.login_name,
    profiles.realname,
    profiles.mybugslink AS showmybugslink,
    profiles.disabledtext,
    profiles.disable_mail
    FROM profiles
    WHERE userid = ?

Shall I go ahead and move the update outside the blocks?

Comment 10

[Max Kanat-Alexander]

(In reply to comment #9)
> Shall I go ahead and move the update outside the blocks?

  Sure!

Comment 11

[Max Kanat-Alexander]

Comment on attachment 265290 [details] [diff] [review]
fix version 2

r- per the above.

Comment 12

[Vance Baarda]

Attachment: fix version 3

Moved the update outside the blocks.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 265518 [details] [diff] [review]
fix version 3

Looks good to me! :-)

Comment 14

[Greg Hendricks]

    Checking in Bugzilla/Auth/Verify.pm;
    /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify.pm,v  <--  Verify.pm
    new revision: 1.7; previous revision: 1.6
    done