297794 - initBug's verification of user_id being an email is bogus

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Christian Reis]

So initBug's verification of user_id being an email is bogus:

  unless (defined $user_id) {
    $user_id = 0;
  }
  else {
     if ($user_id =~ /^\@/) {
        $user_id = &::DBname_to_id($user_id); 
     }
  }

This is slightly outdated, but no email starts with an @ sign. Patch in hand.

Comment 1

[Christian Reis]

Attachment: kiko_v1: the important part is s/^//

The rest is cosmetics.

Comment 2

[Frédéric Buclin]

Comment on attachment 186336 [details] [diff] [review]
kiko_v1: the important part is s/^//

>+  } elsif ($user_id =~ /\@/) {
>+    $user_id = login_to_id($user_id); 
>   }
> 
>   $self->{'who'} = new Bugzilla::User($user_id);

This does not help, because I could set Param('emailsuffix') to
'@my_company.com' and $user_id would be something like 'my_username'. Then new
Bugzilla::User($user_id) would fail because of an invalid $user_id. Why not
testing if $user_id is numerical using if (!detaint_natural()) {login_to_id()}
?

Adding a comment specifying that the login name, not the full email address, is
required would be fine too.

Comment 3

[Tiago Mello [:timello]]

Attachment: v2_timello: detaint_natural instead

Comment 4

[Christian Reis]

Comment on attachment 186488 [details] [diff] [review]
v2_timello: detaint_natural instead

I don't really like stored_user_id, but I see why we need it and it does avoid
some redundant code. 

I am more concerned, however, with login_to_id(). That function returns zero if
the login supplied doesn't exist, but I think we should explicitly raise an
error	in that case -- I don't like us silently failing   here because it can
mask other errors. How about doing a ThrowUserError("invalid_username")  if
!$user_id inside that elsif?

Comment 5

[Frédéric Buclin]

Comment on attachment 186488 [details] [diff] [review]
v2_timello: detaint_natural instead

kiko, you say it's unsafe to let a user with an invalid login name to go
through. But the code actually does not check that $user_id, if numeric, is
valid either. This is part of another bug IMO.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

> I am more concerned, however, with login_to_id(). That function returns zero
> if the login supplied doesn't exist, but I think we should explicitly raise
> an error in that case -- I don't like us silently failing here because it
> can mask other errors.

Yeah, we should stop doing such things.

> How about doing a ThrowUserError("invalid_username")  if !$user_id inside that
elsif?

Our other option is to return undef.  In practice, some situations will probably
call for one, while others will call for the other.

a=myk for check in during 2.20 freeze.

Comment 7

[Frédéric Buclin]

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.78; previous revision: 1.77
done