summaryrefslogtreecommitdiffstats
path: root/Bugzilla
diff options
context:
space:
mode:
authorbugreport%peshkin.net <>2005-08-19 05:09:36 +0200
committerbugreport%peshkin.net <>2005-08-19 05:09:36 +0200
commit9d4872bef3b679b020b6678445ec84504e1f8a1e (patch)
treeba83ecbc319e12000d9ee00d2da2f04facded3d6 /Bugzilla
parentd11ebe02d5e293f88992090878db4c95523f1809 (diff)
downloadbugzilla-9d4872bef3b679b020b6678445ec84504e1f8a1e.tar.gz
bugzilla-9d4872bef3b679b020b6678445ec84504e1f8a1e.tar.xz
Bug 304583: Remove all remaining need to rederive inherited groups
Patch by Joel Peshkin <bugreport@peshkin.net> r=mkanat, a=justdave
Diffstat (limited to 'Bugzilla')
-rw-r--r--Bugzilla/Auth/Login/WWW/Env.pm9
-rwxr-xr-xBugzilla/Bug.pm9
-rw-r--r--Bugzilla/Constants.pm8
-rw-r--r--Bugzilla/Flag.pm4
-rw-r--r--Bugzilla/User.pm219
5 files changed, 89 insertions, 160 deletions
diff --git a/Bugzilla/Auth/Login/WWW/Env.pm b/Bugzilla/Auth/Login/WWW/Env.pm
index 39bea28df..64487884c 100644
--- a/Bugzilla/Auth/Login/WWW/Env.pm
+++ b/Bugzilla/Auth/Login/WWW/Env.pm
@@ -38,6 +38,7 @@ sub login {
my $matched_userid = '';
my $matched_extern_id = '';
my $disabledtext = '';
+ my $new_login_name = 0;
my $dbh = Bugzilla->dbh;
my $sth;
@@ -122,6 +123,7 @@ sub login {
") VALUES ( ?, ?, ?, '' )");
$sth->execute($env_email, '*', $env_realname);
$matched_userid = $dbh->bz_last_key('profiles', 'userid');
+ $new_login_name = $matched_userid;
}
}
}
@@ -147,9 +149,16 @@ sub login {
($env_realname || $this_realname),
$matched_userid);
$sth->execute;
+ $new_login_name = $matched_userid;
}
}
+ # If the login name may be new, make sure the regexp groups are current
+ if ($new_login_name) {
+ my $userprofile = new Bugzilla::User($matched_userid);
+ $userprofile->derive_regexp_groups;
+ }
+
# Now we throw an error if the user has been disabled
if ($disabledtext) {
ThrowUserError("account_disabled",
diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index f281dbda8..ec1927355 100755
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -541,25 +541,22 @@ sub groups {
# user_group_map record putting the user in that group.
# The LEFT JOINs are checking for record existence.
#
+ my $grouplist = Bugzilla->user->groups_as_string;
my $sth = $dbh->prepare(
"SELECT DISTINCT groups.id, name, description," .
" bug_group_map.group_id IS NOT NULL," .
- " user_group_map.group_id IS NOT NULL," .
+ " CASE WHEN groups.id IN($grouplist) THEN 1 ELSE 0 END," .
" isactive, membercontrol, othercontrol" .
" FROM groups" .
" LEFT JOIN bug_group_map" .
" ON bug_group_map.group_id = groups.id" .
" AND bug_id = ?" .
- " LEFT JOIN user_group_map" .
- " ON user_group_map.group_id = groups.id" .
- " AND user_id = ?" .
- " AND isbless = 0" .
" LEFT JOIN group_control_map" .
" ON group_control_map.group_id = groups.id" .
" AND group_control_map.product_id = ? " .
" WHERE isbuggroup = 1" .
" ORDER BY description");
- $sth->execute($self->{'bug_id'}, Bugzilla->user->id,
+ $sth->execute($self->{'bug_id'},
$self->{'product_id'});
while (my ($groupid, $name, $description, $ison, $ingroup, $isactive,
diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm
index 2ebb7f10c..078c988b6 100644
--- a/Bugzilla/Constants.pm
+++ b/Bugzilla/Constants.pm
@@ -53,7 +53,6 @@ use base qw(Exporter);
LOGOUT_KEEP_CURRENT
GRANT_DIRECT
- GRANT_DERIVED
GRANT_REGEXP
GROUP_MEMBERSHIP
@@ -68,8 +67,6 @@ use base qw(Exporter);
COMMENT_COLS
- DERIVE_GROUPS_TABLES_ALREADY_LOCKED
-
UNLOCK_ABORT
RELATIONSHIPS
@@ -157,7 +154,6 @@ use constant contenttypes =>
};
use constant GRANT_DIRECT => 0;
-use constant GRANT_DERIVED => 1;
use constant GRANT_REGEXP => 2;
use constant GROUP_MEMBERSHIP => 0;
@@ -180,10 +176,6 @@ use constant DEFAULT_QUERY_NAME => '(Default query)';
# The column length for displayed (and wrapped) bug comments.
use constant COMMENT_COLS => 80;
-# Used to indicate to User::new and User::new_from_login calls
-# that the derive_groups tables are already locked
-use constant DERIVE_GROUPS_TABLES_ALREADY_LOCKED => 1;
-
# used by Bugzilla::DB to indicate that tables are being unlocked
# because of error
use constant UNLOCK_ABORT => 1;
diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index 4c4864ea7..02ddb65ba 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -906,9 +906,7 @@ sub notify {
{
my @new_cc_list;
foreach my $cc (split(/[, ]+/, $flag->{'type'}->{'cc_list'})) {
- my $ccuser = Bugzilla::User->new_from_login($cc,
- DERIVE_GROUPS_TABLES_ALREADY_LOCKED)
- || next;
+ my $ccuser = Bugzilla::User->new_from_login($cc) || next;
next if $flag->{'target'}->{'bug'}->{'restricted'}
&& !$ccuser->can_see_bug($flag->{'target'}->{'bug'}->{'id'});
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index ab70f06fa..0ad345979 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -111,8 +111,6 @@ sub _create {
# We're checking for validity here, so any value is OK
trick_taint($val);
- my $tables_locked_for_derive_groups = shift;
-
my $dbh = Bugzilla->dbh;
my ($id,
@@ -137,26 +135,6 @@ sub _create {
$self->{'disabledtext'} = $disabledtext;
$self->{'showmybugslink'} = $mybugslink;
- # Now update any old group information if needed
- my $result = $dbh->selectrow_array(q{SELECT 1
- FROM profiles, groups
- WHERE userid=?
- AND profiles.refreshed_when <=
- groups.last_changed},
- undef,
- $id);
-
- if ($result) {
- my $is_main_db;
- unless ($is_main_db = Bugzilla->dbwritesallowed()) {
- Bugzilla->switch_to_main_db();
- }
- $self->derive_groups($tables_locked_for_derive_groups);
- unless ($is_main_db) {
- Bugzilla->switch_to_shadow_db();
- }
- }
-
return $self;
}
@@ -283,11 +261,36 @@ sub groups {
# 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 = ();
+ $sth = $dbh->prepare("SELECT groups.name, groups.id
+ FROM group_group_map
+ INNER JOIN groups
+ ON groups.id = grantor_id
+ WHERE member_id = ?
+ AND grant_type = " . GROUP_MEMBERSHIP);
+ while (my $node = shift @groupidstocheck) {
+ $sth->execute($node);
+ my ($member_name, $member_id);
+ while (($member_name, $member_id) = $sth->fetchrow_array) {
+ if (!$groupidschecked{$member_id}) {
+ $groupidschecked{$member_id} = 1;
+ push @groupidstocheck, $member_id;
+ $groups{$member_name} = $member_id;
+ }
+ }
+ }
$self->{groups} = \%groups;
return $self->{groups};
}
+sub groups_as_string {
+ my $self = shift;
+ return (join(',',values(%{$self->groups})) || '-1');
+}
+
sub bless_groups {
my $self = shift;
@@ -308,8 +311,6 @@ sub bless_groups {
# Get all groups for the user where:
# + They have direct bless privileges
# + They are a member of a group that inherits bless privs.
- # Because of the second requirement, derive_groups must be up-to-date
- # for this to function properly in all circumstances.
$query = q{
SELECT DISTINCT groups.id, groups.name, groups.description
FROM groups, user_group_map, group_group_map AS ggm
@@ -318,8 +319,9 @@ sub bless_groups {
AND groups.id=user_group_map.group_id)
OR (groups.id = ggm.grantor_id
AND ggm.grant_type = ?
- AND user_group_map.group_id = ggm.member_id
- AND user_group_map.isbless = 0))};
+ AND ggm.member_id IN(} .
+ $self->groups_as_string .
+ q{)))};
$connector = 'AND';
@bindValues = ($self->id, GROUP_BLESS);
}
@@ -340,26 +342,13 @@ sub bless_groups {
sub in_group {
my ($self, $group) = @_;
+ return exists $self->groups->{$group} ? 1 : 0;
+}
- # If we already have the info, just return it.
- return defined($self->{groups}->{$group}) if defined $self->{groups};
- return 0 unless $self->id;
-
- # Otherwise, go check for it
-
- my $dbh = Bugzilla->dbh;
-
- my ($res) = $dbh->selectrow_array(q{SELECT 1
- FROM groups, user_group_map
- WHERE groups.id=user_group_map.group_id
- AND user_group_map.user_id=?
- AND isbless=0
- AND groups.name=?},
- undef,
- $self->id,
- $group);
-
- return defined($res);
+sub in_group_id {
+ my ($self, $id) = @_;
+ my %j = reverse(%{$self->groups});
+ return exists $j{$id} ? 1 : 0;
}
sub can_see_user {
@@ -405,7 +394,7 @@ sub can_see_bug {
LEFT JOIN bug_group_map
ON bugs.bug_id = bug_group_map.bug_id
AND bug_group_map.group_ID NOT IN(" .
- join(',',(-1, values(%{$self->groups}))) .
+ $self->groups_as_string .
") WHERE bugs.bug_id = ?
AND creation_ts IS NOT NULL " .
$dbh->sql_group_by('bugs.bug_id', 'reporter, ' .
@@ -445,7 +434,7 @@ sub get_selectable_products {
CONTROLMAPMANDATORY . " ";
}
$query .= "AND group_id NOT IN(" .
- join(',', (-1,values(%{Bugzilla->user->groups}))) . ") " .
+ $self->groups_as_string . ") " .
"WHERE group_id IS NULL ORDER BY name";
my $dbh = Bugzilla->dbh;
my $sth = $dbh->prepare($query);
@@ -501,8 +490,8 @@ sub visible_groups_as_string {
return join(', ', @{$self->visible_groups_inherited()});
}
-sub derive_groups {
- my ($self, $already_locked) = @_;
+sub derive_regexp_groups {
+ my ($self) = @_;
my $id = $self->id;
return unless $id;
@@ -511,71 +500,32 @@ sub derive_groups {
my $sth;
- $dbh->bz_lock_tables('profiles WRITE', 'user_group_map WRITE',
- 'group_group_map READ',
- 'groups READ') unless $already_locked;
-
# avoid races, we are only up to date as of the BEGINNING of this process
my $time = $dbh->selectrow_array("SELECT NOW()");
- # first remove any old derived stuff for this user
- $dbh->do(q{DELETE FROM user_group_map
- WHERE user_id = ?
- AND grant_type != ?},
- undef,
- $id,
- GRANT_DIRECT);
-
- my %groupidsadded = ();
# add derived records for any matching regexps
- $sth = $dbh->prepare("SELECT id, userregexp FROM groups WHERE userregexp != ''");
- $sth->execute;
-
- my $group_insert;
- while (my $row = $sth->fetch) {
- if ($self->{login} =~ m/$row->[1]/i) {
- $group_insert ||= $dbh->prepare(q{INSERT INTO user_group_map
- (user_id, group_id, isbless, grant_type)
- VALUES (?, ?, 0, ?)});
- $groupidsadded{$row->[0]} = 1;
- $group_insert->execute($id, $row->[0], GRANT_REGEXP);
- }
- }
-
- # Get a list of the groups of which the user is a member.
- my %groupidschecked = ();
-
- my @groupidstocheck = @{$dbh->selectcol_arrayref(q{SELECT group_id
- FROM user_group_map
- WHERE user_id=?},
- undef,
- $id)};
-
- # Each group needs to be checked for inherited memberships once.
- my $group_sth;
- while (@groupidstocheck) {
- my $group = shift @groupidstocheck;
- if (!defined($groupidschecked{"$group"})) {
- $groupidschecked{"$group"} = 1;
- $group_sth ||= $dbh->prepare(q{SELECT grantor_id
- FROM group_group_map
- WHERE member_id=?
- AND grant_type = } .
- GROUP_MEMBERSHIP);
- $group_sth->execute($group);
- while (my ($groupid) = $group_sth->fetchrow_array) {
- if (!defined($groupidschecked{"$groupid"})) {
- push(@groupidstocheck,$groupid);
- }
- if (!$groupidsadded{$groupid}) {
- $groupidsadded{$groupid} = 1;
- $group_insert ||= $dbh->prepare(q{INSERT INTO user_group_map
- (user_id, group_id, isbless, grant_type)
- VALUES (?, ?, 0, ?)});
- $group_insert->execute($id, $groupid, GRANT_DERIVED);
- }
- }
+ $sth = $dbh->prepare("SELECT id, userregexp, user_group_map.group_id
+ FROM groups
+ LEFT JOIN user_group_map
+ ON groups.id = user_group_map.group_id
+ AND user_group_map.user_id = ?
+ AND user_group_map.grant_type = ?");
+ $sth->execute($id, GRANT_REGEXP);
+
+ my $group_insert = $dbh->prepare(q{INSERT INTO user_group_map
+ (user_id, group_id, isbless, grant_type)
+ VALUES (?, ?, 0, ?)});
+ my $group_delete = $dbh->prepare(q{DELETE FROM user_group_map
+ WHERE user_id = ?
+ AND group_id = ?
+ AND isbless = 0
+ AND grant_type = ?});
+ while (my ($group, $regexp, $present) = $sth->fetchrow_array()) {
+ if (($regexp ne '') && ($self->{login} =~ m/$regexp/i)) {
+ $group_insert->execute($id, $group, GRANT_REGEXP) unless $present;
+ } else {
+ $group_delete->execute($id, $group, GRANT_REGEXP) if $present;
}
}
@@ -585,7 +535,6 @@ sub derive_groups {
undef,
$time,
$id);
- $dbh->bz_unlock_tables() unless $already_locked;
}
sub product_responsibilities {
@@ -690,8 +639,8 @@ sub match {
$query .= "AND user_group_map.user_id = userid " .
"AND isbless = 0 " .
"AND group_id IN(" .
- join(', ', (-1, @{$user->visible_groups_inherited})) . ") " .
- "AND grant_type <> " . GRANT_DERIVED;
+ join(', ', (-1, @{$user->visible_groups_inherited})) .
+ ")";
}
$query .= " AND disabledtext = '' " if $exclude_disabled;
$query .= "ORDER BY namelength ";
@@ -743,8 +692,7 @@ sub match {
$query .= " AND user_group_map.user_id = userid" .
" AND isbless = 0" .
" AND group_id IN(" .
- join(', ', (-1, @{$user->visible_groups_inherited})) . ")" .
- " AND grant_type <> " . GRANT_DERIVED;
+ join(', ', (-1, @{$user->visible_groups_inherited})) . ")";
}
$query .= " AND disabledtext = ''" if $exclude_disabled;
$query .= " ORDER BY namelength";
@@ -1151,8 +1099,7 @@ sub get_userlist {
$query .= "LEFT JOIN user_group_map " .
"ON user_group_map.user_id = userid AND isbless = 0 " .
"AND group_id IN(" .
- join(', ', (-1, @{$self->visible_groups_inherited})) . ") " .
- "AND grant_type <> " . GRANT_DERIVED;
+ join(', ', (-1, @{$self->visible_groups_inherited})) . ")";
}
$query .= " WHERE disabledtext = '' ";
$query .= $dbh->sql_group_by('userid', 'login_name, realname');
@@ -1271,7 +1218,7 @@ sub login_to_id {
}
sub UserInGroup {
- return defined Bugzilla->user->groups->{$_[0]} ? 1 : 0;
+ return exists Bugzilla->user->groups->{$_[0]} ? 1 : 0;
}
1;
@@ -1346,10 +1293,6 @@ C<undef> if no matching user is found.
This routine should not be required in general; most scripts should be using
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
-routine's documentation for details.
-
=end undocumented
=item C<id>
@@ -1439,12 +1382,19 @@ are the names of the groups, whilst the values are the respective group ids.
(This is so that a set of all groupids for groups the user is in can be
obtained by C<values(%{$user-E<gt>groups})>.)
+=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". This is most often used
+within an SQL IN() function.
+
=item C<in_group>
-Determines whether or not a user is in the given group. 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.
+Determines whether or not a user is in the given group by name.
+
+=item C<in_group_id>
+
+Determines whether or not a user is in the given group by id.
=item C<bless_groups>
@@ -1464,7 +1414,7 @@ Returns 1 if the specified user account exists and is visible to the user,
Determines if the user can see the specified bug.
-=item C<derive_groups>
+=item C<derive_regexp_groups>
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
@@ -1508,23 +1458,6 @@ Returns a list of groups that the user is aware of.
Returns the result of C<visible_groups_direct> as a string (a comma-separated
list).
-=begin undocumented
-
-This routine takes an optional argument. If true, then this routine will not
-lock the tables, but will rely on the caller to have done so itsself.
-
-This is required because mysql will only execute a query if all of the tables
-are locked, or if none of them are, not a mixture. If the caller has already
-done some locking, then this routine would fail. Thus the caller needs to lock
-all the tables required by this method, and then C<derive_groups> won't do
-any locking.
-
-This is a really ugly solution, and when Bugzilla supports transactions
-instead of using the explicit table locking we were forced to do when thats
-all MySQL supported, this will go away.
-
-=end undocumented
-
=item C<product_responsibilities>
Retrieve user's product responsibilities as a list of hashes.