304583 - Remove all remaining need to derive groups

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P2)

Description

[Joel Peshkin]

It should be possible to keep groups properly up to date at all times without
the need to derive groups ever.

editusers, Auth::Env,  should update REGEXP groups on login change
syncldap should be checked for login changes
new account already does this
email change calls derive groups so it needs to be fixed in token.cgi

Then, all remaining places that use user_group_map should be checked to
eliminate any use of DERIVED groups and use an IN() clause with flattened
group_group_map entries instead.

Comment 1

[Joel Peshkin]

Attachment: Sneak preview ... not done yet

This is most of it.  Still need to ensure that every place that touches
login_name keeps the regexp groups up to date.

Comment 2

[Joel Peshkin]

Attachment: Patch - eliminate caching of inherited groups

Comment 3

[Max Kanat-Alexander]

Comment on attachment 192702 [details] [diff] [review]
Patch - eliminate caching of inherited groups

>@@ -4064,6 +4049,38 @@
>+# 2005-08-14 [e-mail] -- Bug 304583
>+# Get rid of leftover DERIVED group permissions
>+$dbh->do("DELETE FROM user_group_map WHERE grant_type = " . GRANT_DERIVED);

  I think that this should probably only happen once, as a time-based thing (up
in the table-changes section). We can wrap it in some other schema change,
perhaps, that we check in after this bug.

  And so as part of that, we should move the GRANT_DERIVED constant to be part
of this block, and out of Bugzilla::Constants.

>+my $sth2 = $dbh->prepare("INSERT IGNORE INTO user_group_map 

  Does that work on Pg...? I'm not sure it does...

>+while (my ($uid, $login, $gid, $rexp, $present) = $sth->fetchrow_array()) {
>+    if ($login =~ m/$rexp/i) {
>+        $sth2->execute($uid, $gid) unless $present;

  Since we're moving this code, could we use more specific names than $sth2 and
$sth3?

>-my $editusers = $user->in_group('editusers');
>+my $editusers = UserInGroup('editusers');

  This is a regression, we're trying to get rid of UserInGroup. :-)

>+            my $newobject = new Bugzilla::User($otherUserID);
>+            $newobject->derive_regexp_groups(1);

  Won't that happen automatically as part of the new Bugzilla::User?

  A better name that $newobject would be good, too. :-)

>-    $otheruser->derive_groups();
>+    my $grouplist = join(',',(-1,values(%{$otheruser->groups})));

  As LpSolit said, we should have a groups_as_string method for that.

>Index: Bugzilla/User.pm
>[snip]
>+    UserInGroupId

  If it's possible to avoid adding this (without having to go back to using
direct SQL in the CGIs) it would be good.

> sub in_group {
>[snip]
>+    return defined $self->groups->{$group} ? 1 : 0;

  Nit: Let's use "exists" there, since it's really what we mean.

>+sub UserInGroupId {
>+    my %j = reverse(%{Bugzilla->user->groups});
>+    return defined $j{$_[0]} ? 1 : 0;

  You could make this a method, and call it in_group_id.

> This routine and C<new> both take an extra optional argument, which is
>-passed as the argument to C<derive_groups> to avoid locking. See that
>+passed as the argument to C<derive_regexp_groups> to avoid locking. See that

  Except is that really needed anymore?

>[Auth]
>+    if ($new_login_name) {
>+        my $userprofile = Bugzilla::User->New($matched_userid);

  Nit: We usually use "new Bugzilla::User"

Comment 4

[Joel Peshkin]

Attachment: v2

I incorporated all of the review comments except the locking for derive groups.
 That is still needed, but derive is no longer called anytime except when a
login_name may have changed.

Comment 5

[Joel Peshkin]

Comment on attachment 192731 [details] [diff] [review]
v2

My tests for this included 
1) adding a user with no privs and operating as that user
(post/edit/buglist/prefs) 
2) creating/editing/viewing bugs I reported but are in groups of which I am not
a member
3) editing bug in groups of which I am a member *** DRAT!!!***

Comment 6

[Joel Peshkin]

Attachment: v3

OK... I think I got it all.  Once this lands, we should expunge the remaining
UserInGroup calls, but there is no sense in cluttering this patch up with
those.

Comment 7

[Marc Schumann [:Wurblzap]]

There is a comment regarding derive_groups in editusers.cgi. Can you please
adjust it?

Comment 8

[Joel Peshkin]

I'll yank the comment on next edit or checkin.

Comment 9

[Max Kanat-Alexander]

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

>Index: checksetup.pl
>[snip]
>+# 2005-08-14 [e-mail] -- Bug 304583
>+# Get rid of leftover DERIVED group permissions
>+$dbh->do("DELETE FROM user_group_map WHERE grant_type = " . GRANT_DERIVED);
> [snip]

  Nit: This whole section depends on the tables having the exact structure they
do now, so it should be in the --TABLE-- section and not in the --GROUPS--
section, even though it deals with groups.

>+# Evaluate regexp-based group memberships
>+$sth = $dbh->prepare("SELECT profiles.userid, profiles.login_name,
>+                         groups.id, groups.userregexp,
>+                         user_group_map.group_id
>+                         FROM profiles
>+                         INNER JOIN groups
>+                         LEFT JOIN user_group_map

  There's an INNER JOIN with no ON clause, there. Maybe that makes sense in
MySQL, but it doesn't make sense to me. :-)

>Index: editusers.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v
>retrieving revision 1.96
>diff -u -r1.96 editusers.cgi
>--- editusers.cgi	13 Aug 2005 20:22:11 -0000	1.96
>+++ editusers.cgi	15 Aug 2005 18:54:22 -0000
>@@ -287,6 +287,8 @@
>                      'WHERE userid = ?',
>                      undef, @values);
>             # XXX: should create profiles_activity entries.
>+            my $newprofile = new Bugzilla::User($otherUserID);
>+            $newprofile->derive_regexp_groups(1);

  We already have an $otherUser object, you can just use that.

>@@ -645,7 +647,7 @@
>         qq{SELECT id,
>                   COUNT(directmember.group_id) AS directmember,
>                   COUNT(regexpmember.group_id) AS regexpmember,
>-                  COUNT(derivedmember.group_id) AS derivedmember,
>+                  groups.id IN ($grouplist),

  Booleans in the SELECT clause aren't ANSI, and don't work in Pg. Use CASE
WHEN groups.id IN ($grouplist) THEN 1 ELSE 0.

>Index: token.cgi
> [snip]
>@@ -267,7 +267,7 @@
> 
>     # The email address has been changed, so we need to rederive the groups
>     my $user = new Bugzilla::User($userid);
>-    $user->derive_groups;
>+    $user->derive_regexp_groups;

  Just as a note, it would be good for the future to have these happen inside
Bugzilla::Token subs. (Not necessary now.)

>Index: whine.pl
> [snip]
>@@ -275,10 +275,13 @@
>                     my $group_id = Bugzilla::Group::ValidateGroupName(
>                         $groupname, $owner);
>                     if ($group_id) {
>+                        my $glist = join(',',
>+                            Bugzilla::User::flatten_group_membership(
>+                            $group_id));

  flatten_group_membership is a method, and thus actually takes $self as the
first argument. You could do Bugzilla::User->flatten_group_membership() to get
around that, if you wanted, or modify flatten_group_membership to be a real
subroutine.

>Index: Bugzilla/User.pm
>
>@@ -283,11 +263,36 @@
>     # The above gives us an arrayref [name, id, name, id, ...]
>     # Convert that into a hashref
>     my %groups = @$groups;
>+    my $sth;
>+    my @groupidstocheck = values(%groups);
>+    my %groupidschecked = ();
> [snip]

  Hrm... is there some way that we could combine both SQL statements into one?
I managed to do it for bless_groups.

>@@ -1347,7 +1302,7 @@
> userids instead.
> 
> This routine and C<new> both take an extra optional argument, which is
>-passed as the argument to C<derive_groups> to avoid locking. See that
>+passed as the argument to C<derive_regexp_groups> to avoid locking. See that

  Except that new() doesn't call derive_regexp_groups anymore, right? At least,
from what I can see.

>+=item C<groups_as_string>
>+
>+Returns a string containing a comma-seperated list of numeric group ids.  If
>+the user is not a member of any groups, returns "-1".

  I think it would be nice to note that we did that for SQL "IN" statements.

>+Determines whether or not a user is in the given group by name. This method is 
>+mainly intended for cases where we are not looking at the currently logged in 
>+user, and only need to make a quick check for the group, where calling 
>+C<groups> and getting all of the groups would be overkill.

  That's actually not true anymore, since we changed the implementation to
always get groups. We might want to now say that this is the preferred method
of checking for groups, for purposes of readability, unless you need the whole
groups list for some reason, which case you should use C<groups>.

>Index: Bugzilla/Auth/Login/WWW/Env.pm
> [snip]

  I'm just trusting you on this one... I have no knowledge of this code at all.


  I'll also test this code, now.

Comment 10

[Max Kanat-Alexander]

OK. I tested some complex group structures and bug editing, and it worked quite
well.

Comment 11

[Joel Peshkin]

Attachment: v4

Here it is... +165/-287 and has all the cleanups we discussed. I'll do a bit
more testing and then I'll request review.

Comment 12

[Joel Peshkin]

Attachment: v5

OK....	I think I got all the remaining bugs and I did try it out a bit on
Postgres as well.

Also tested the upgrade from 2.18

Comment 13

[Max Kanat-Alexander]

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

Looks good, works properly in basic testing on landfill.

Comment 14

[Max Kanat-Alexander]

This probably also solves bug 282199.

Comment 15

[Joel Peshkin]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.429; previous revision: 1.428
done
Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.58; previous revision: 1.57
done
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.98; previous revision: 1.97
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.124; previous revision: 1.123
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.278; previous revision: 1.277
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.103; previous revision: 1.102
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.36; previous revision: 1.35
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.87; previous revision: 1.86
done
Checking in whine.pl;
/cvsroot/mozilla/webtools/bugzilla/whine.pl,v  <--  whine.pl
new revision: 1.14; previous revision: 1.13
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.91; previous revision: 1.90
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.28; previous revision: 1.27
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.49; previous revision: 1.48
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.75; previous revision: 1.74
done
Checking in Bugzilla/Auth/Login/WWW/Env.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/WWW/Env.pm,v  <--  Env.pm

new revision: 1.6; previous revision: 1.5
done

Comment 16

[Max Kanat-Alexander]

Added to the Bugzilla 2.22 Release Notes in bug 322960.