Right now, it is not possible to do a search on bugs based on group membership of people. I cannot, for example, search for bugs where the owner is in a specific group. This becomes trivially easy if I can rely on the user_group_map always has the regexp-based permissions up-to-date. All I then have to do is to flatten the group_group_map entries to get a list of groups that would convey membership in the group of interest and then select records where user_group_map.group_id IN(join(',',@those_flattened_groups)) [As usual, preventing the need to have the database support a complete implementation of perl regular expressions] The proposed change is as follows.... 1) user_group_map.isderived is replaced with user_group_map.grant_type which could have values EXPLICIT, REGEXP, or DERIVED 2) checksetup, on conversion of isderived, would ensure that every user is up-to-date with REGEXP-based groups 3) user->derive_groups would be called by editusers whenever a user's profile is changed 4) editgroups would go through all the users to update the regexp-based entries whenever a group regexp is changed.

>> I cannot, for example, search for bugs where the owner is in a >> specific group. If you would be able to do such a thing you could find out if the owner is in a specific group or not. This raises slightly a security issue. Right now it's not easy for a regular user to tell if someone belongs to a specific group or not.

I guess that is a basic design question we should ask ourselves.... do we care if a user asks for a list of bugs owned by any member who has security access so long as that user can only see those bugs that are not secured? I don't see a problem here, but it does mean that a user could figure out which team members are in a specific group.

OK, this can be done without conflicting with bug 190222

Comment on attachment 146076 [details] [diff] [review] First cut I've tested this on a 2.16 migration and a 2.17 update

Comment on attachment 146076 [details] [diff] [review] First cut Nice work. I only took a fast look at it and I'm fascinated by the groups code :) - $isderived = $isderived || 0; + $isderived = ($isderived > 0 ? 1 : 0); I'd prefer this to be called grant_type, even if you conflict with me on the editgroups.cgi thing. Let's spend some time solving some conflicts rather then having inconsistent terminology between CGI and the DB. + $isderived = ($isderived > 0 ? 1 : 0); my $checked = $member - $isderived; We talked about this in IRC. If the only reason for which we make $isderived=1 when it really is 2 is to have $checked calculated properly, then we probably need to change how $checked is computed in the first place. :-) Otherwise it looks good and I'll take a closer peak at version 2 :-)

OK, this changes GROUP_ constants to GRANT_ constants and cleans up editusers so that it has $isderived and $isregexp and indicates each appropriately.

Comment on attachment 146156 [details] [diff] [review] v2 +# if GRANT_DIRECT - record was explicitly granted +# if GRANT_DERIVED - record was derived from expanding a group hierarchy +# if GRANT_REGEXP - record was created by evaluating a regexp I'd probably prefer to have "- record" aligned all under each other, but that's a nit. +if (GetFieldDef("user_group_map", "isderived")) { ... +# Make sure groups get rederived +$dbh->do("UPDATE groups SET last_changed = NOW() WHERE name = 'admin'"); Are we sure we want to rederive the groups every time we run ./checksetup.pl? Shouldn't that be inside the if above? + $dbh->do("UPDATE user_group_map SET grant_type = " . + "IF(isderived, " . GRANT_DERIVED . ", " . + GRANT_DIRECT . ")"); + $dbh->do("DELETE FROM user_group_map + WHERE isbless = 0 AND grant_type != " . GRANT_DIRECT); I still don't understand why we delete the very same records that we just inserted one line above. If we want to keep only the ones where grant_type = GRANT_DIRECT, then probably the first update should be reflected to do the update only when isderived = 0. Looks good otherwise.

This will conflict with the bug 190222 templatization thing. We need a landing order established. :)

(In reply to comment #8) > > +if (GetFieldDef("user_group_map", "isderived")) { > ... > +# Make sure groups get rederived > +$dbh->do("UPDATE groups SET last_changed = NOW() WHERE name = 'admin'"); > > Are we sure we want to rederive the groups every time we run ./checksetup.pl? > Shouldn't that be inside the if above? > Actually, I am more inclined to treat this like forcing the recompilation of templates. It does no harm and it fixes anything strange someone might have done to the database. > + $dbh->do("UPDATE user_group_map SET grant_type = " . > + "IF(isderived, " . GRANT_DERIVED . ", " . > + GRANT_DIRECT . ")"); > + $dbh->do("DELETE FROM user_group_map > + WHERE isbless = 0 AND grant_type != " . GRANT_DIRECT); > > I still don't understand why we delete the very same records that we just > inserted one line above. If we want to keep only the ones where grant_type = > GRANT_DIRECT, then probably the first update should be reflected to do the > update only when isderived = 0. > The first line is not inserting any records. It is converting records. The second line is cleaning up the records that are now spurious. I'll add a comment to this on the next edit. On the landing order, it is probably appropriate for the templatization to land first.

Thanks for the explanations. I'll wait then for version 3 which will probably be based on the diff from the bug 190222 (which could be actually commited meanwhile), and with the comment and identation issues fixed, I'll take another look if it's needed.

This is blocked by a bug that got pushed to 2.20, so this goes with it.

No reason to leave this blocked. The dependency was originally created to accomodate a landing order which never came true.

Comment on attachment 146156 [details] [diff] [review] v2 I've tested it out and it works great. I have one nit to pick, though: ----- User is a member of any groups shown with grey bars and marked with brackets surrounding the membership checkbox as a result membership in another group or with '*' instead of the brackets indicating that membership is the result of a regular expression match. User can bless any group marked with brackets surrounding the bless checkbox as a result of membership in another group. ----- That seems really amazingly awkward. Might I suggest: ----- User is a member of any group shown with a check or grey bar. A grey bar indicates indirect membership, either derived from other groups (marked with square brackets), or via regular expression (marked with '*'). Square brackets around the bless checkbox indicate the ability to bless that group as a result of membership in another group. -----

Comment on attachment 149951 [details] [diff] [review] v3 - fixed language on editusers page Looks good to me.

Hmmm... concerning the security issues brought up in comment 1 and 2... how hard would it be to prevent people from searching with groups that they're not a member of? That's a question for the query implementation bug though. This patch in itself doesn't have any concerns, since it's only updating the data, not providing access to it.

checked in Will make sure that security is maintained in bug 244329

(In reply to comment #18) > Will make sure that security is maintained in bug 244329 > That should be bug#244239, I think

+sub RederiveRegexp ($$) ..... + while (my ($uid, $login) = $sth->fetchrow_array()) { + if ($login =~ m/$regexp/i) If the regexp is empty, meaning that no one should be added to the group, this code seems like it would add everyone. If $regexp is empty won't it match all $login values and thus adding everyone to the group.

argh!!! You're right.

actually, we'll track the fix under a distinct bug. bug 250080