180635 - Enhance Bugzilla::User to store additional information

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Bradley Baetz (:bbaetz)]

All the stuff currently in GetUserInfo should be moved, and the stuff requring
joins to other tables should possibly be lazily queried.

Comment 1

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

Make sure to change instances of "[% foo.name %] <[% foo.login %]>" (or whatever
it is) to "[% foo.identity %]" in the process.

Comment 2

[Gervase Markham [:gerv]]

Currently, Bugzilla::User has a field $self->{'email'}, which is set to the
database value login_name. However, this value is not necessarily an email
address. Currently, it is if emailsuffix is added, but even that may not always
be true.

We need to change the interface of User.pm to call this field "login", as it is
in GetUserInfo().

Other than that, I can't see any problems with moving the contents of
GetUserInfo() into User.pm. "queries", "canblessany" and the groups stuff would
all be loaded on demand.

Gerv

Comment 3

[Bradley Baetz (:bbaetz)]

Right; this bug is on my todo list as soon as the Auth.pm stuff goes in.

Comment 4

[Gervase Markham [:gerv]]

Er... I've got a patch which I did this afternoon. I'll polish it up and attach
it :-)

Gerv

Comment 5

[Gervase Markham [:gerv]]

Attachment: Patch v.1

This does most of the trick, as far as I can tell. It has one really odd bug,
though - on query.cgi and buglist.cgi user.groups gets set to an empty hash in
some odd way, and so (because it's defined) we never fill it, and so none of
the controls (such as the editproducts or editgroups links) appears.

Any ideas why?

Gerv

Comment 6

[Gervase Markham [:gerv]]

Oops. CCing self, and commenting so I get a notification.

Gerv

Comment 7

[Bradley Baetz (:bbaetz)]

That would probably be because of the cache UserInGroup uses.

UserInGroup should move into User.pm, and become a simple hash lookup. (See the
DBI docs for a nice way to convert a list of the names in the group into a hash,
although you may want the hash value to indicate member vs blesser, or
something. Just lazily load the groups as required, like you're currently doing.)

I was going to hook this off my Auth code, and have Bugzilla->user be the global
currently logged in user. You may want to look at that patch (and possibly
review it :-). The exists stuff is gone, and we get a blank (or possibly undef,
I can't recall) user if it doesn't exist.

That also has the advantage that we don't have to change old compat code at the
same time

Comment 8

[Bradley Baetz (:bbaetz)]

Attachment: Take 2

OK, here we go. This is a bit more involved than gerv's patch.

Comment 9

[Gervase Markham [:gerv]]

Comment on attachment 118177 [details] [diff] [review]
Take 2

> Index: buglist.cgi
> ===================================================================
> +
> +        # Now reset the cached queries
> +        Bugzilla->user->reset_queries_cache;

Something freaks me out about method calls without parentheses. I can bear it
in the case of methods which both get and set variables (even though I think
they are a bit gross), but when it's obviously a method, please can we have
parentheses?

> Index: editusers.cgi
> ===================================================================
> @@ -483,7 +485,7 @@
>      print "OK, done.<br>\n";
>      SendSQL("SELECT last_insert_id()");
>      my ($newuserid) = FetchSQLData();
> -    DeriveGroup($newuserid);
> +

This DeriveGroup call has not (in contrast to below) been replaced by a call to
$user->derive_groups(). Is this intentional?

> Index: query.cgi
> ===================================================================
> -    $userid = quietly_check_login();
> +    quietly_check_login();
>  }
>  
> +my $user = Bugzilla->user;
> +my $userid = $user->id;

Surely Bugzilla->user can be undef if there's no login?

> Index: token.cgi
> ===================================================================
> +
> +            # email has changed, so rederive groups
> +            # Note that this is done _after_ the tables are unlocked
> +            # This is sort of a race condition (given the lack of transactions)
> +            # but the user had access to it just now, so its not a security issue

Nits: line too long, and "it's".

> Index: Bugzilla/BugMail.pm
> ===================================================================
>      #  Drop any non-insiders if the comment is private
>       return if (Param("insidergroup") && 
>                 ($anyprivate != 0) && 
> -               (!UserInGroup(Param("insidergroup"), $userid)));
> +               (!$user->groups->{Param("insidergroup")}));

Nit: could you fix the alignment here to match the brackets? I spent a good 15
seconds staring at this, trying to work it out.

       return if (Param("insidergroup") && 
		  ($anyprivate != 0)	&& 
		  (!$user->groups->{Param("insidergroup")}));

> Index: Bugzilla/Flag.pm
> ===================================================================
> @@ -265,7 +265,7 @@
>                          $flag->{'target'}->{'bug'}->{'id'}, 
>                          $attach_id,
>                          $requestee_id,
> -                        $flag->{'setter'}->{'id'}, 
> +                        " . $flag->{'setter'}->id . ",

Does embedding "$flag->{'setter'}->id" in a string not work right, then?

> Index: Bugzilla/Template.pm
> ===================================================================
> @@ -256,7 +256,7 @@
>              # Generic linear search function
>              'lsearch' => \&Bugzilla::Util::lsearch,
>  
> -            # UserInGroup - you probably want to cache this
> +            # UserInGroup
>              'UserInGroup' => \&::UserInGroup,

Do you want to add a comment about it being deprecated?

> Index: Bugzilla/User.pm
> ===================================================================
> +# Internal helper for the above |new| methods
> +# $cond is a string (including a placeholder ?) for the search
> +# requirement for the profiles table

Urk. That doesn't sound like a desperately transparent or clean interface to
me... I suppose, if it's only exposed inside the module, that's not so bad. But
still, urgh.

> +    my ($id,
> +        $login,
> +        $name,
> +        $mybugslink) = $dbh->selectrow_array(qq{SELECT userid,
> +                                                       login_name,
> +                                                       realname,
> +                                                       mybugslink
> +                                                  FROM profiles
> +                                                 WHERE $cond},
> +                                             undef,
> +                                             $val);

One begins to think that the DBI people could have provided some shorter,
alternate method names... Even so, I'd go for:

    my ($id, $login, $name, $mybugslink) = 
      $dbh->selectrow_array("SELECT userid, login_name, realname, mybugslink
			     FROM profiles 
			     WHERE $cond",
			     undef, $val);

Regardless, we need some consistency, both within this patch and across
Bugzilla. You have a qq{} here, a q{} there, and quotes elsewhere. Similarly,
you appear to have acquired some idea about "centre-alignment" of each SQL
section which isn't anywhere else in the code. :-)

Might I suggest (and we should consult on this):

  my ($comma, $separated, $list, $of, $vars) =
    $dbh->some_longwinded_methodname("SELECT some SQL " .
				     "ON multiple lines " .
				     "IF necessary " .
				     "AND enclosed in quotes " .
				     "BECAUSE we all understand them");

> +# Generate a string to identify the user by name + email if the user
> +# has a name or by email only if she doesn't.
> +sub identity {
> +    my $self = shift;
> +
> +    $self->{identity} = 
> +      $self->{name} ? "$self->{name} <$self->{login}>" : $self->{login}
> +        unless exists $self->{identity};

Nit: I'm not convinced that using unless follows the principle of "most
important part first". I'd say this is a case of "IF there's no identity THEN
create one" and then return it. Same for nick.

> +sub queries {
> +    my $self = shift;
> +
> +    return $self->{queries} if exists $self->{queries};
> +
> +    my $dbh = Bugzilla->dbh;
> +    my $sth = $dbh->prepare(q{  SELECT name, query, linkinfooter
> +                                  FROM namedqueries
> +                                 WHERE userid=?
> +                              ORDER BY UPPER(name)});
> +    $sth->execute($self->{id});

Why are we prepare()ing, execute()ing and fetch()ing here? Unless there's a
special reason for it, we should use the convenience methods; otherwise, the
next person to come along is going to spend a while trying to work out what the
non-existent special reason is.

Same comment for everywhere else you do it where there's no special reason.

> +sub reset_queries_cache {
> +    my $self = shift;
> +
> +    delete $self->{queries};
> +}

Nit: I'd say you "clear" (or, at a pinch, "flush") a cache rather than reset
it.

> +    $self->{can_bless} = $res ? 1 : 0;
> +
> +    return $self->{can_bless};
> +}

Why is the set of groups the user can bless not a hash like the groups hash?
You could check for non-emptiness to get the value of "can_bless". This would
mean we could actually display the right UI in blessing circumstances.

Even if you aren't going to do this, a forwardly compatible interface might
have "bless_groups" instead of "can_bless".

> +Bugzilla allows for group inheritance. When data about the user (or any of the
> +groups) changes, the database must be updated. Handling updated groups is taken
> +care of by the constructor. However, when updating the email address, the
> +user may be placed into different groups, absed on a new email regexp. This

"absed" -> "based".

> Index: template/en/default/sidebar.xul.tmpl
> ===================================================================
> @@ -72,37 +72,41 @@
>        <text class="text-link" onclick="load_relative_url('enter_bug.cgi')" value="new bug"/>
>        <separator class="thin"/>
>  
> -[% IF username %]
> +[% USE Bugzilla %]
> +[% user = Bugzilla.user %]

Shouldn't we generally be passing in "user" as a var, for consistency amd
simplicity of interface, here and elsewhere? What you are really doing is
embedding some Perl in the templates.

> Index: template/en/default/global/useful-links.html.tmpl
> ===================================================================
> +    [% preset_queries = user.showmybugslink;
> +       IF NOT preset_queries;
> +         FOREACH q = user.queries;
> +           IF q.linkinfooter;
> +             preset_queries = 1;
> +             LAST;
> +           END;
> +         END;
> +       END;
> +     %]

This is a style not found elsewhere in our code. Is there a particularly
compelling reaon for using it? (Or for changing this code at all?)

Phew :-)

Gerv

Comment 10

[Bradley Baetz (:bbaetz)]

> This DeriveGroup call has not (in contrast to below) been replaced by a call to
> $user->derive_groups(). Is this intentional?

Yes. We're creating a new user, so the last updated value will default to zero,
and so derive_groups will be run on that user's first login (since zero is < any
group's last updated time). Theres no need to immediately reflect this (since we
knwo that the current user isn't the new user...). This mirrors hte behaviour
for createaccount.cgi

> Surely Bugzilla->user can be undef if there's no login?

Doh! thought I'd caught all of those...

> Does embedding "$flag->{'setter'}->id" in a string not work right, then?

Correct - its parsed as |$flag->{'setter'} . "->id". I cuold probably wrap the
whole thing in ${}, but that wasn't as neat.

> Do you want to add a comment about it being deprecated?

I'll add one to globals.pl

> Urk. That doesn't sound like a desperately transparent or clean interface to
> me... I suppose, if it's only exposed inside the module, that's not so bad.
> But still, urgh.

Yeah, its ugly. Which is why that second bit isn't documented, except in a
|=begin undocumented| block, which won't show up in perldoc (unless you're
formatting for a document type of undocumented, I guess)

> <sql style>

I used q{} instead of quotes because its nicer + neater. I used qq{} when
Ineeded interpolation.

The advantage of my styls is that it handles nested queries - just up the indent
level. Its easy to see where related 'bits' start/stop/etc. All the WHERE bits
are indented togther, as are all the FROM bits, and so on.

The multipule quotes are a pain, and detract from teh flow of just reading the
thing.

> Why are we prepare()ing, execute()ing and fetch()ing here?

the ->select_* shortcuts only work when there is a single row. If you want to
iterate through the resultset, you need to get a statement handle (via prepare)
run it (via execute) and then keep fetching it.

There is, regrettably, no $dbh->execute("string") returning an active statement
handle. Given the number of different ways to prepare/execute something, that
would probbalybe more trouble than its worth, duplicating the functionality.

> Why is the set of groups the user can bless not a hash like the groups hash?

Those aren't predone via derive groups, so its an extra query. Plus, we do care
for the groups, since we never really care if the user is in _any_ group or not,
only which ones (With EXISTS, this would also be more efficient) With blessing,
only editusers cares about which groups we are in. Abstraint ghtis stuff out for
edit* is so not on my todo list :)

> You could check for non-emptiness to get the value of "can_bless". This would
> mean we could actually display the right UI in blessing circumstances.

What does that mean?

> Even if you aren't going to do this, a forwardly compatible interface might
> have "bless_groups" instead of "can_bless".

Well, that depends on what the final API is, I guess. It snot quite forwards
compatable because of the %{} thing needed if there is a hashref vs a simple scalar.

> Shouldn't we generally be passing in "user" as a var, for consistency amd
> simplicity of interface, here and elsewhere? What you are really doing is
> embedding some Perl in the templates.

Well, I'm actually embedding a perl accessor, rather than code. (I still want to
find some way to make that USE not needed everywhere). Passing it in doesn't
work, because every single template needs it, and the global $::vars hash is
going away. I could have a |user| 'sub' in DEFAULTs which just returns
Bugzilla->user, I guess. I didn't want to start grabbing random names globally
though. I could still put it in a Bugzilla namespace though, I suppose, although
teh semantics for that aren't quite identical.

> This is a style not found elsewhere in our code. Is there a particularly
> compelling reaon for using it?

There are 10 lines of only TT code, and the [% %] got annoying. Whats wrong with it?

Comment 11

[Gervase Markham [:gerv]]

> The advantage of my styls is that it handles nested queries - just up
> the indent level. Its easy to see where related 'bits'
> start/stop/etc. All the WHERE bits are indented togther, as are all
> the FROM bits, and so on.
> 
> The multipule quotes are a pain, and detract from teh flow of just
> reading the thing.

Oh, I agree with that; the quotes are a pain. But myk (and, I believe, justdave)
have consistently argued that we can't use one long string because it does bad
things to logs.

Also, we have to ask whether the gain is useful enough to justify varying styles
(see my comments below on this point.)

I've started a thread on developers@ about this.

>> Why are we prepare()ing, execute()ing and fetch()ing here?
> 
> the ->select_* shortcuts only work when there is a single row. If you
> want to iterate through the resultset, you need to get a statement
> handle (via prepare) run it (via execute) and then keep fetching it.

Er... what about selectall_arrayref() and selectall_hashref? These return an
array reference you can iterate over. I've used them several times in recent
Bugzilla code.

>> Shouldn't we generally be passing in "user" as a var, for
>> consistency amd simplicity of interface, here and elsewhere? What
>> you are really doing is embedding some Perl in the templates.
> 
> Well, I'm actually embedding a perl accessor, rather than code. (I
> still want to find some way to make that USE not needed everywhere).
> Passing it in doesn't work, because every single template needs it,

Well, you could do
$vars->{'user'} = Bugzilla->user;
in all CGIs. It's better doing it there than doing it in all templates.

> and the global $::vars hash is going away. I could have a |user|
> 'sub' in DEFAULTs which just returns Bugzilla->user, I guess. I
> didn't want to start grabbing random names globally though. I could
> still put it in a Bugzilla namespace though, I suppose, although teh
> semantics for that aren't quite identical.

I don't think "grabbing random names globally" is so bad; we do that for various
other functions. And, as you say, everyone needs it. The user sub sounds like a
good idea; would that have performance implications?

>> This is a style not found elsewhere in our code. Is there a
>> particularly compelling reaon for using it?
> 
> There are 10 lines of only TT code, and the [% %] got annoying. Whats
> wrong with it?

Well, for a start, it has exactly the same end result as the four lines it
replaces. Yes, it might prevent a call to user.queries, but it's going to get
called below anyway. So there's no real need for the change.

But the main reason is the rule of least surprise; if you have a way of writing
something, and you vary it in a few cases, it will cause confusion as people try
and work out why you did what you did. Exceptions (to anything) are also harder
to read; my mental parser treats lines beginning with [% as directives, and
stuff between them as HTML.

Gerv

Comment 12

[Bradley Baetz (:bbaetz)]

Well, logs aren't the issue. sqllog is gone, remember. Besides, its _easier_ to
read a nicely formate text in a log file than one huge long massive line.

> Er... what about selectall_arrayref() and selectall_hashref? These return an
> array reference you can iterate over. I've used them several times in recent
> Bugzilla code.

Hmm. Where? Please don't.

The issue is that doing so brings it all into memory at once, rather than doing
so in bits. Its also less efficent, because you have data being copied from the
db into the arrayref, then fom the arrayref into the values, and then you do
whatever with them. If you don't neeed to access them all at once, then don't.

> Well, you could do $vars->{'user'} = Bugzilla->user; in all CGIs.

Eww.

> It's better doing it there than doing it in all templates.

No its not. Most templates don't need the user object at all, remember. If we
wantto reserve the name 'user' as a template var globally, then I can fix this
easily, but I'd rather not. you're just seeing all the paces where this is
needed. Most templtes don't care about this at all. I I can get rid of the USE,
would that be better?

Comment 13

[Gervase Markham [:gerv]]

> Hmm. Where? Please don't.

<looks> Nothing checked in yet; some stuff I did for bug 180635, which you have
since written a patch for, and also bug 16009, which you are about to review. 

> If we wantto reserve the name 'user' as a template var globally, then I can 
> fix this easily, but I'd rather not.

I'd rather we did this; after all, if "user" were ever used for anything else
other than a Bugzilla::User object representing the current user, it would be
horribly confusing. As that isn't going to happen, making it globally available
just means you can use it when you need it without worrying.

Gerv

Comment 14

[Bradley Baetz (:bbaetz)]

Attachment: v3

OK, so I think that the style discussion is finished.

I think I fixed the other issues, but its been a few weeks so I may have missed
one.

Comment 15

[Bradley Baetz (:bbaetz)]

Comment on attachment 120199 [details] [diff] [review]
v3

myk, can you try this? The doc patch rejects because of the .sgml->.xml moving;
just ignore that since its fixed locally.

Comment 16

[Bradley Baetz (:bbaetz)]

Attachment: v4

OK, take 4, to deal with cvs changes (mainly myk's stuff)

One note for reviewers - A non logged in user has $user set to |undef|. I chose
this rather than an empty hash which could be auto-vivified because we really
don't want to be trying to get stuff (such as name) for the user if they don't
exist. The one exception is for group memberships, and for most of those things
we're either logged in already, or we should be checking that the user is
logged in anyway (eg we don't need to set the |bugowners| stuff in buglist.cgi
if we're not logged in, so that check should be futher up). Most of the
remaining checks are for the timetrackinggroup stuff, for display purposes.

So, who wants to review? :)

Comment 17

[Bradley Baetz (:bbaetz)]

Also, I'm mssing a ) at the end of line 198 for Flag.pm. Forgot to rediff,
sorry. Just add that manually, since I'm sure that there will be other rounds
for this...

Comment 18

[Jacob Steenhagen]

Comment on attachment 121752 [details] [diff] [review]
v4

I only did a brief scan, but the following looks fishy:

>Index: buglist.cgi
>===================================================================
[...]
>+
>+        # Now reset the cached queries
>+        Bugzilla->user->flush_queries_cache();
[...]
>+        # Make sure to invalidate any cached query data, so that the footer is
>+        # correctly displayed
>+        Bugzilla->user->reset_queries_cache;
>+


>Index: Bugzilla/User.pm
>===================================================================
[...]
>+sub flush_queries_cache {

I don't see reset_queries_cache defined anywhere and would assume that's
supposed to be flush_queries_cache.  Unfortunately, I don't have time to do an
actual review ATM :(

Comment 19

[Bradley Baetz (:bbaetz)]

oops, I issed that when I renamed it. Fixed locally, cand confirmed with grep
that that is the only one.

Comment 20

[Bradley Baetz (:bbaetz)]

Attachment: v1

Fixes the typo previously mentioned, and also updates due to bitrot with the
CGI.pm patch.

Comment 21

[Bradley Baetz (:bbaetz)]

Attachment: v5

Fixes the typo previously mentioned, and also updates due to bitrot with the
CGI.pm patch.

Comment 22

[Bradley Baetz (:bbaetz)]

Comment on attachment 122712 [details] [diff] [review]
v1

oops, wrong patch

Comment 23

[Bradley Baetz (:bbaetz)]

Comment on attachment 122713 [details] [diff] [review]
v5

Can someone please review this? Its basically been sitting for a month now, and
its blocking lots of other stuff.

Comment 24

[Jacob Steenhagen]

Comment on attachment 122713 [details] [diff] [review]
v5

You missed at least one instance of a call to this sub in editusers.cgi. I
discovered it by going to the editusers.cgi page, picking a user and attempting
to add them to some groups. After clicking submit, the group change happened
but I didn't have a page footer. Apache told me the following:

[Thu May 08 09:53:56 2003] [error] [client 127.0.0.1] Undefined subroutine
&main::DeriveGroup called at /var/www/html/bugzilla-patch/editusers.cgi line
851., referer:
http://localhost/bugzilla-patch/editusers.cgi?action=edit&user=jacob%40steenhag
en.us

Also, after adding that user to one of the bless groups and then loggin in as
that user the editusers.cgi link doesn't appear on the footer. Manually
visiting editusers.cgi does work, however.

Another Apache error:

[Thu May 08 10:02:47 2003] [error] [client 127.0.0.1] "my" variable $userid
masks earlier declaration in same scope at
/var/www/html/bugzilla-patch/query.cgi line 66., referer:
http://localhost/bugzilla-patch/


>-# UserInGroup returns information aboout the current user if no second 
>-# parameter is specified
> sub UserInGroup {

This comment (which is obviously being removed) implies that it's possible to
use this sub routine to ask if a specific user (other than the one logged in)
is in a specific group. The modified version doesn't allow for that. If you're
sure that either it was never used for that purpose or you've removed all those
calls, then I guess this comment can be disregarded.

>Index: query.cgi
>===================================================================
[...]
>+my $user = Bugzilla->user;
>+my $userid = $user ? $user->id : 0;
[...]
>-if ($userid) {
>+if ($user) {

Being that you have both $user and $userid I guess I'm not sure why this
change...

>Index: Bugzilla/Flag.pm
>===================================================================
[...]
>+                my $user = Bugzilla::User->new_from_login($requestee_email);

$user normally refers to the currently logged in user... perhaps this should be
$requestee

>+                    && !$user->in_group(Param("insidergroup")))

Why do we use $user->in_group() here and $user->groups->{} elsewhere?

>Index: Bugzilla/FlagType.pm
>===================================================================
[...]
>+            my $user = Bugzilla::User->new_from_login($requestee_email);

Ditto on $user

>+                && !$user->in_group(Param("insidergroup")))

And $user->in_group() vs. $user->groups->{}

>Index: Bugzilla/User.pm
>===================================================================
[...]
>+    my $invocant = shift;

I assume $invocant is what I've typically seen as $self?

>+# The request flag stuff also does this, but it really should be passing
>+# in the id its already had to validate (or the User.pm object, of course)
>+sub new_from_login {

Isn't the idea here to /create/ the User.pm object? If so, then how could they
pass this object to this object? (Or am I misreading the comment?)

>+    my ($id,
>+        $login,
>+        $name,
>+        $mybugslink) = $dbh->selectrow_array(qq{SELECT userid,
>+                                                       login_name,
>+                                                       realname,
>+                                                       mybugslink
>+                                                  FROM profiles
>+                                                 WHERE $cond},
>+                                             undef,
>+                                             $val);

I gotta tell ya, that is one funky lookin' chunk of code.

>+# Generate a string to identify the user by name + email if the user
>+# has a name or by email only if she doesn't.
>+sub identity {
>+    my $self = shift;
>+
>+    if (!defined $self->{identity}) {
>+        $self->{identity} = 
>+          $self->{name} ? "$self->{name} <$self->{login}>" : $self->{login};
>+    }
>+
>+    return $self->{identity};
>+}

I'm sure I could look at the rest of this patch, but where is this identity
used that we'd want to put the e-mail address in <>'s? I assume it's in an
e-mail but wouldn't it make sense to do something like this for the other
places we display realname/email addy/combo?

>+sub groups {
[...]
>+    # The above gives us an arrayref [name, id, name, id, ...]
>+    # Convert that into a hashref
>+    my %groups = @$groups;
>+    $self->{groups} = \%groups;

I thought it was possible to get it out of the database as a hashref?

>+sub in_group {

I think I have an answer for a question I asked previously, $user->groups->{}
caches the information while $user->in_group() doesn't. I find the later easier
to read so perhaps that should be fixed.

OK, I think that's all I have to say.

Comment 25

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

Looks like Jake and I both reviewed.  Good, this needed two reviews anyway. 
Here's mine:

Index: mozilla/webtools/bugzilla/Bugzilla.pm

-Logs in a user, returning the userid, or C<0> if there is no logged in user.
+Logs a user out. For the moment, this will just cause calls to C<user> to

Nit: be consistent, i.e. logs a user in & logs a user out or logs in a user and
logs out a user.



Index: mozilla/webtools/bugzilla/CGI.pl
Index: mozilla/webtools/bugzilla/attachment.cgi
Index: mozilla/webtools/bugzilla/buglist.cgi
Index: mozilla/webtools/bugzilla/editusers.cgi
Index: mozilla/webtools/bugzilla/globals.pl

 sub UserInGroup {

What would it take to kill this function entirely?



Index: mozilla/webtools/bugzilla/post_bug.cgi
Index: mozilla/webtools/bugzilla/process_bug.cgi

-my $whoid = confirm_login();
+my $user = confirm_login();
+my $whoid = $user->id;

Nit: This seems like an unnecessary shortcut in the new world order, I'd rather
see ${user->id} than this here and in many places below.



Index: mozilla/webtools/bugzilla/query.cgi
Index: mozilla/webtools/bugzilla/relogin.cgi
Index: mozilla/webtools/bugzilla/sanitycheck.cgi
Index: mozilla/webtools/bugzilla/sidebar.cgi
Index: mozilla/webtools/bugzilla/token.cgi

+            # email has changed, so rederive groups
+            # Note that this is done _after_ the tables are unlocked
+            # This is sort of a race condition (given the lack of transactions)
+            # but the user had access to it just now, so it's not a security
+            # issue

So it won't compromise security, but it'll fail sometimes?  How bad is it?



Index: mozilla/webtools/bugzilla/userprefs.cgi
Index: mozilla/webtools/bugzilla/votes.cgi
Index: mozilla/webtools/bugzilla/Bugzilla/BugMail.pm
Index: mozilla/webtools/bugzilla/Bugzilla/Flag.pm

-                    && &::Param("insidergroup")
-                    && !&::UserInGroup(&::Param("insidergroup"), $requestee_id))
-                {
+                    && Param("insidergroup")
+                    && !$user->in_group(Param("insidergroup")))
+                  {

Nit: opening bracket is mis-indented.



Index: mozilla/webtools/bugzilla/Bugzilla/Search.pm

+    if ($user) {
+        if (%{$user->groups}) {
+            $query .= " AND bug_group_map.group_id NOT IN (" . join(',',
values(%{$user->groups})) . ") ";
+        }

Aren't hashes always true even if empty?



Index: mozilla/webtools/bugzilla/Bugzilla/Template.pm

-            # UserInGroup - you probably want to cache this
+            # Currently logged in user, if any
+            'user' => sub { return Bugzilla->user; },

I take it making this a sub causes it to lazily create Bugzilla->user? If not,
what's the advantage to not just assigning Bugzilla->user to it?



Index: mozilla/webtools/bugzilla/Bugzilla/User.pm

-my $user_cache = {};

No more cache?  Is this for mod_perl reasons?


+    my $lock_tables_for_derive_groups = shift;

Nit: This sounds like "lock tables for derive groups or not?" instead of "are
tables already locked or not?", which has the opposite meaning.


+                 showmybugslink => $mybugslink,

Nit: showmybugslink but $mybugslink (no show).


+    my $res = $dbh->selectrow_array(q{SELECT 1

Nit: $res took me a while to figure out; use "$result" instead for clarity or
$rv to be standard here and in other places below.


+sub identity {
+sub nick {

Why make these methods instead of properties?  This seems to unnecessarily
complicate matters.


+    while (my $row = $sth->fetch) {

Umm, fetch?  I can't find that in my DBI documentation.


+sub in_group {

Is there any way to hide this so that a reference to $user->groups->{foo}
invokes this method?


+sub derive_groups {
+    my ($self, $do_not_lock) = @_;

Nit: Here "do_not_lock" and "TABLES_ALREADY_LOCKED" have the same implication,
but one is a positive assertion and one is negative.  For hackers who work on
this code, it'll be easier if they are both either positive or negative.



Index: mozilla/webtools/bugzilla/docs/xml/administration.xml
Index: mozilla/webtools/bugzilla/template/en/default/sidebar.xul.tmpl
Index: mozilla/webtools/bugzilla/template/en/default/attachment/create.html.tmpl
Index: mozilla/webtools/bugzilla/template/en/default/bug/show.xml.tmpl
Index: mozilla/webtools/bugzilla/template/en/default/global/useful-links.html.tmpl
Index: mozilla/webtools/bugzilla/template/en/default/list/quips.html.tmpl
Index: mozilla/webtools/bugzilla/template/en/default/request/email.txt.tmpl
Index: mozilla/webtools/bugzilla/template/en/default/search/knob.html.tmpl

Comment 26

[Bradley Baetz (:bbaetz)]

> You missed at least one instance of a call to this sub in editusers.cgi. 

That got added after I wrote this; I'll fix it up

> This comment (which is obviously being removed) implies that it's possible to
> use this sub routine to ask if a specific user (other than the one logged in)
> is in a specific group. 

right, I removed that functionality. You need to ask your own User.pm instance.
I fixed up all the callers.

> Being that you have both $user and $userid I guess I'm not sure why this
> change...

$user is the user object; $userid is the userid. Everywhere should be using
$user, basicaly, but I'm not rewriting all of Bugzilla now. This script has
places which use $userid, so I need to kepp that arround, inclduing the 'no
user==0' thing.

> Why do we use $user->in_group() here and $user->groups->{} elsewhere?

->groups loads all the groups. Thats nice for the currently logged in user,
because we need to query most of them for the footer anyway, and its quicker to
grab them all rather than lazily loading each group one by one.

->in_group is used here because we only care about this one single group, so its
quicker to check for one, rather than loading them all.

> I assume $invocant is what I've typically seen as $self?

No; This alloes for $user->new as well as Bugzilla::User->new - see the ref()
call below. Its a standard perl naming thing.

> Isn't the idea here to /create/ the User.pm object? If so, then how could they
> pass this object to this object? (Or am I misreading the comment?)

It should be passing a User.pm object to its various subroutines, not the email
address.

> I'm sure I could look at the rest of this patch, but where is this identity
> used that we'd want to put the e-mail address in <>'s?

I chnaged a couple of templates, but not all of them. Ever looked at a review
mail from someone who doesn't have a realname set? Theres an extra space in
there, because we don't do comping, and so on. This moves teh 'identity'
somewhere centralised.

> I thought it was possible to get it out of the database as a hashref?

You can get each row as a hashref, but you can't get all of the data as a
hashref. What I'm doing here is in teh docs as the way to do it.

> I think I have an answer for a question I asked previously, $user->groups->{}
> caches the information while $user->in_group() doesn't. I find the later easier
> to read so perhaps that should be fixed.

Yes, but thats a side effect (in_group could be cached, mind you, but theres no
need for any of the callers so far)> The difference is in how we fetch them from
teh db.

I'll fix this up this afternoon, or maybe tomorrow.

Comment 27

[Bradley Baetz (:bbaetz)]

Oh, and myk's comments:

> sub UserInGroup {
> What would it take to kill this function entirely?

More work than is useful/productive ATM.

> Nit: This seems like an unnecessary shortcut in the new world order, I'd rather
> see ${user->id} than this here and in many places below.

Well, I'd rather not use ->id at all, but I'm not rewriting all that yet.

using $whoid lets me keep all the below existing code as-is, plus its quicker -
remember that theres a method call involved.


>+            # email has changed, so rederive groups
>+            # Note that this is done _after_ the tables are unlocked
>+            # This is sort of a race condition (given the lack of transactions)
>+            # but the user had access to it just now, so it's not a security
>+            # issue
                                                                                
>So it won't compromise security, but it'll fail sometimes?  How bad is it?

I just added teh comment; I didn't change code. We could rewrite this to use
your new arg to handle locking for derive groups, I guess.

Basically, theres a very small window where a user could change their email via
the token, and then log in, but still have access to the groups from their old
regexp. Since they're changing the email, not the admin, and they had access
before, I don't think its a security problem.

It shold still be fixed, though, and now that you changed teh derivegroups
locking stuff, its possbible to do so. New bug on that, I think.

> Aren't hashes always true even if empty?

No.

hashrefs are true even if their hash is empy, though, which is why I dereference
here.

>I take it making this a sub causes it to lazily create Bugzilla->user? If not,
>what's the advantage to not just assigning Bugzilla->user to it?

Because we don't have the user when we create the template object, plus the
template is persistant across multiple load under mod_perl, which may be with
different users.

Comment 28

[Bradley Baetz (:bbaetz)]

oops, hit submit too early

> -my $user_cache = {};
> No more cache?  Is this for mod_perl reasons?

Yes. Plus, theres really unlikly to be a benefit - we keep teh current user
arround throught the entire invocation now, so we never have to relaod it. The
only time this could be needed is for emails, where NewProcessOnePerson loads
the user each time. Thats not a loss from teh current code, which did the
name->id mappping each time arround anyway. Since BugMail.pm really needs a
rewrite, it can just cache the usernames locally.

> Why make these methods instead of properties?  This seems to unnecessarily
> complicate matters.

Abstration, plus consistency with ->groups, ->identity, and so on.

> Umm, fetch?  I can't find that in my DBI documentation.

Its an alias for ->fetchrow_arrayref

> Is there any way to hide this so that a reference to $user->groups->{foo}
> invokes this method?

In theory I could tie it, I guess, but TT's XS stash doesn't work with tied
vars, which would defeat the purpose. Even w/o that, I don't really see the
benefit. See my comments to JayPee.

You can write $user->groups{foo}, too, FWIW, I'm 99% sure.

Comment 29

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

Ok, I guess my issues have been resolved, if not all nits picked, so I'll give
this r= once you fix the problems Jake found (but you need a second review).

Comment 30

[Bradley Baetz (:bbaetz)]

Attachment: v6

OK, various bits not commented on have been fixed. I think I got everything....

Comment 31

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

Comment on attachment 122909 [details] [diff] [review]
v6

All changes look fine, but when I request a flag of someone I get a software
error that is described in the Apache error log as: Insecure dependency in
parameter 3 of DBI::db=HASH(0x8703658)->selectrow_array method call while
running with -T switch at Bugzilla/User.pm line 71.

Comment 32

[Gervase Markham [:gerv]]

Comment on attachment 122909 [details] [diff] [review]
v6

[Note: this was a code review; I didn't test the patch.]

> Index: query.cgi
> ===================================================================
> +my $user = Bugzilla->user;
> +my $userid = $user ? $user->id : 0;

Just for my info: why does Bugzilla->user return undef if there's no logged-in
user, rather than a user object with no privs and an ID of 0?

> Index: Bugzilla/Template.pm
> ===================================================================
> +            # UserInGroup
>              'UserInGroup' => \&::UserInGroup,

You might want to add a note that this is now deprecated, if we are deprecating
it in favour of user->groups.

Gerv

Comment 33

[Bradley Baetz (:bbaetz)]

Attachment: v7

gerv: Because we only care in very few places. Most of our code requires the
user to be logged in, and it also makes it moee obvious when someone uses that
'dummy' user as a real user, since we won't catch uerid=0 stuff if we get it
into the database. Its not a real user, so why have a hash for one? I've
discussed this in previous comments in the bug here.

OK, fixed myk's thing (missing trick_taint), and also fixed:

- The correct footer wasn't being shown after a ThrowUserError because of some
compat code (which had a comment in it to remove when B::U happened...)
- logout now unsets $::userid as well as $_user, becuase its the Correct Thing
To Do
- A lot of the templates were using user.email. The new Bugzilla::User didn't
have an email attribute, but TT poked into the hash and just gave an empty
string. I was going to change them to ->login, but some of them do need emails
(ie the email code). The bits which are left are stuff like mailto links, xml
exporting, and user matching stuff. Since we haven't really decided how the
login_name/email separation will work, and thats a separate issue I don't want
to get into now, I just added ->email in as an alias for ->login. This mainly
showed up as the qa_contact always being blank, and the requestee email always
being blank.
- Only create a Bugzilla::User for the flag stuff when the requestee isn't
null. This is the correct thing to do, and also avoids some warnings.

(myk, was there a bug filed on the internal server error when trying to request
a flag from an email that doesn't exist when user matching was disabled?)

Comment 34

[Jacob Steenhagen]

Comment on attachment 124113 [details] [diff] [review]
v7

Looks good to me...

Comment 35

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

Comment on attachment 124113 [details] [diff] [review]
v7

>(myk, was there a bug filed on the internal server error when trying to request
>a flag from an email that doesn't exist when user matching was disabled?)

I don't get an internal server error until I apply this patch.	Without the
patch, the requestee gets silently dropped and the request gets added to the
bug.  That makes the error a problem with your patch, no?

This patch also fails to completely apply because of a conflict in buglist.cgi.

Comment 36

[Bradley Baetz (:bbaetz)]

Hmm. It just silently succeeds w/o my patch. It used to give an error - let me
look into that.

CVS conflict resolved locally; I'll upload it after working out this other issue.

Comment 37

[Bradley Baetz (:bbaetz)]

The internal server error was due to bug 180563. processmail dropped invalid
emails silently, but $user->id gives errors. I've fixed this locally and am just
testing now.

Comment 38

[Bradley Baetz (:bbaetz)]

Attachment: v8

OK, try 8.

Comment 39

[Bradley Baetz (:bbaetz)]

Comment on attachment 124658 [details] [diff] [review]
v8

myk, hows this version?

Comment 40

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

Comment on attachment 124658 [details] [diff] [review]
v8

Looks good.  r=myk.  Please get a second review before checking in.

Comment 41

[Jacob Steenhagen]

Comment on attachment 124658 [details] [diff] [review]
v8

Still looks good to me...

Comment 42

[Bradley Baetz (:bbaetz)]

Fixed!!!!!

Comment 43

[Gervase Markham [:gerv]]

<cough>approval?<cough>

;-)

Gerv

Comment 44

[Bradley Baetz (:bbaetz)]

Err. Oops - I forgot that. Sorry.

/me self-LARTs.

Comment 45

[victory <[e-mail]>]

Attachment: docs patch about 'Bugzilla Database Tables' section, for 2.18

Comment 46

[Vlad Dascalu]

Comment on attachment 214036 [details] [diff] [review]
docs patch about 'Bugzilla Database Tables' section, for 2.18

+user_group_map:  This table stores which user belong to which group,

s/belong/belongs/

+whether the user can bless others, and how the users granted the

s/granted/obtained/

Comment 47

[Vlad Dascalu]

The attachment is not directly related to this bug (user_group_map existed before this bug got resolved).

Checking in xml/customization.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v  <--  customization.xml
new revision: 1.30; previous revision: 1.29
done

Checking in xml/customization.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v  <--  customization.xml
new revision: 1.21.2.3; previous revision: 1.21.2.2
done

Checking in xml/customization.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v  <--  customization.xml
new revision: 1.20.2.3; previous revision: 1.20.2.2
done

Checking in xml/customization.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v  <--  customization.xml
new revision: 1.12.2.9; previous revision: 1.12.2.8
done

Comment 48

[Vlad Dascalu]

I had to follow-up on this patch, by adding 'user_group_map' to the table list.

Checking in xml/customization.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v  <--  customization.xml
new revision: 1.12.2.10; previous revision: 1.12.2.9
done

Checking in xml/customization.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v  <--  customization.xml
new revision: 1.20.2.4; previous revision: 1.20.2.3
done

Checking in xml/customization.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v  <--  customization.xml
new revision: 1.21.2.4; previous revision: 1.21.2.3
done

Checking in xml/customization.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v  <--  customization.xml
new revision: 1.31; previous revision: 1.30
done