240325 - Keep regexp-based group permissions up-to-date

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P3)

Description

[Joel Peshkin]

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.

Comment 1

[Vlad Dascalu]

>> 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.

Comment 2

[Joel Peshkin]

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.

Comment 3

[Joel Peshkin]

Attachment: First cut

Comment 4

[Joel Peshkin]

OK, this can be done without conflicting with bug 190222

Comment 5

[Joel Peshkin]

Comment on attachment 146076 [details] [diff] [review]
First cut

I've tested this on a 2.16 migration and a 2.17 update

Comment 6

[Vlad Dascalu]

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 :-)

Comment 7

[Joel Peshkin]

Attachment: v2

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

Comment 8

[Vlad Dascalu]

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.

Comment 9

[Vlad Dascalu]

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

Comment 10

[Joel Peshkin]

(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.

Comment 11

[Vlad Dascalu]

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.

Comment 12

[Dave Miller [:justdave]]

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

Comment 13

[Joel Peshkin]

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

Comment 14

[Erik Stambaugh]

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 15

[Joel Peshkin]

Attachment: v3 - fixed language on editusers page

Comment 16

[Erik Stambaugh]

Comment on attachment 149951 [details] [diff] [review]
v3 - fixed language on editusers page

Looks good to me.

Comment 17

[Dave Miller [:justdave]]

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.

Comment 18

[Joel Peshkin]

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

Comment 19

[GavinS]

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

Comment 20

[Jon]

+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.

Comment 21

[Joel Peshkin]

argh!!! You're right.

Comment 22

[Joel Peshkin]

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