401965 - Move groups updating from process_bug.cgi to Bugzilla::Bug

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

This is the next step of the process_bug migration. We're getting close to the end, here.

We have to move the updating of groups from process_bug to Bugzilla::Bug. This should be a big simplification of things in general.

Comment 1

[Max Kanat-Alexander]

Attachment: WIP

This is a work in progress.

After writing this I realized that the errors have to avoid showing the group name. Darn.

It includes modifications to Bugzilla::Util that I didn't end up using, but would be generally extremely useful for Bugzilla.

Comment 2

[Max Kanat-Alexander]

Attachment: v1

This hugely simplifies group processing in general, and should make it much easier to adjust how the groups system works (though a few other changes would need to be made to make that totally easy).

I had to use should_set and add "defined_" hidden values for reporter_accessible and cclist_accessible, because they were getting set to 0 on a product change, with this version of the code.

I was surprised to discover that users with OtherControl can't actually change the group controls of the bug after it's been filed. (I've never had only OtherControl in a Bugzilla, so I didn't know.)

I've tested this a fair amount, and it has worked in all of my tests.

Comment 3

[Frédéric Buclin]

(In reply to comment #2)
> I was surprised to discover that users with OtherControl can't actually change
> the group controls of the bug after it's been filed.

Yup, that's something I enforced in another patch. We agreed that we would hate to have the reporter make a bug public thinking the issue was fixed before a new release was effectively available (think about e.g. Firefox). So as soon as the bug has been filed and restricted to a group, only members of the group can make it public.

Comment 4

[Max Kanat-Alexander]

(In reply to comment #3)
> So as soon as
> the bug has been filed and restricted to a group, only members of the group can
> make it public.

  Okay. Well, whoever made the change didn't fix the docs on editproducts.cgi's Group Controls page.

Comment 5

[Frédéric Buclin]

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

>Index: Bugzilla/Bug.pm

>+        foreach my $gid (@$removed_gr) {
>+            $dbh->do('DELETE FROM bug_group_map
>+                       WHERE bug_id = ? AND group_id = ?', undef, $self->id, $gid);
>+        }

You could put all this into a single DELETE using group_id IN (...).


>+        foreach my $gid (@$added_gr) {
>+            $dbh->do('INSERT INTO bug_group_map (bug_id, group_id) VALUES (?,?)',
>+                     undef, $self->id, $gid);
>+        }

In many places, we first prepare() the SQL call, then call execute() in the loop. I suggest to do the same here (and no, it doesn't make the code less readable).


>+            if (!grep($group->id eq $_->id, @{$product->groups_valid})) {

Use == as we are comparing numbers.


>+sub add_group {

>+    # Now that we know we're in the group, we have to make sure that bugs

... or that we are moving the bug in another product!


>+    # in this product can actually be restricted to this group.
>+    my $valid = $self->product_obj->groups_valid;

As said above, maybe you are only moving the bug in another product, meaning it's possible that you are not in the group you try to add. Valid groups depend on groups you belong to as you could have Shown/NA or Default/NA and so you have to pass the user object to groups_valid() to extract correct data.


>+    grep($group->id eq $_->id, @$valid)

Here too, use == .


>+sub remove_group {

>+        if ($id == $group->id && $item->{membercontrol} == CONTROLMAPMANDATORY) {

This check is not enough. */MANDATORY is a valid combinations and so you have to make sure othermember is not MANDATORY too if the user doesn't belong to the group.


>+    # You can *always* remove a group that is not valid for this product, so we
>+    # don't do any other checks if that's the case. (set_product does this.)
>+    if (!grep($_->id == $group->id, @{$self->product_obj->groups_valid})) {

Valid for whom? (see above)


>+        # Otherwise, you just can't remove a group if you're not a member of it.

Wrong, you may be moving the bug in another product and remove an optional group.



>Index: Bugzilla/Product.pm

>+sub groups_mandatory_for {

>+        "SELECT group_id FROM group_control_map
>+          WHERE (membercontrol = " . CONTROLMAPMANDATORY .
>+              "  AND group_id IN ($groups))

Remove |AND group_id IN ($groups)| as membercontrol = CONTROLMAPMANDATORY implies othercontrol = CONTROLMAPMANDATORY and so we don't care whether the user belongs to these groups or not.



>+sub groups_valid {
>+    my ($self) = @_;

You need the user object/ID too to return the correct list, see above.


>+    return $self->{groups_valid} if defined $self->{groups_valid};

Are we sure ->{groups_valid} is correctly updated when moving the bug in another product?


>+               AND (membercontrol != 0 OR othercontrol != 0)",

Write CONTROLMAPNA instead of 0.


>+=item C<groups_mandatory_for>

>+Tells you what groups are mandatory for a user filing a bug in this product
>+(or moving an existing bug into this product).

Why is this description specific to bug creation and moving the bug? You can always define groups_mandatory_for() even without moving an existing bug.



>Index: template/en/default/global/user-error.html.tmpl

>+  [% ELSIF error == "group_change_denied" %]
>+    [% title = "Cannot Add/Remove That Group" %]
>+    You tried to add or remove group id [% group_id FILTER html %]

I'm not a big fan of these internal IDs. Is there really no way to disclose the group name?


>+    from [% terms.bug %] id [% bug.id FILTER html %], but you are not

Remove |id|. We never say bug id 401965.


>+  [% ELSIF error == "group_invalid_removal" %]
>+    You tried to remove [% terms.bug %] [%+ bug.id FILTER html %]
>+    from group id [% group_id FILTER html %], but [% terms.bugs %] in the 
>+    '[% product.name FILTER html %]' product can not be removed from that
>+    group.

If you can play with this bug, you already know groups it's restricted to (AFAIK), so displaying the group name here is not a security problem.


Note that I didn't test your patch yet. Waiting for an updated version.

Comment 6

[timeless]

re comment 3, "we agreed" is interesting since it violates the mozilla.org policy.

http://www.mozilla.org/projects/security/security-bugs-policy.html#disclosure

The original reporter of a security bug may decide when that bug report will be made public; disclosure is done by clearing the bug's "Security-Sensitive" flag, after which the bug will revert to being an ordinary bug.

In the future, please don't make decisions for Firefox.

The behavior in Bugzilla was requested and designed to satisfy this requirement.
Needless to say, it no longer satisfies the requirement and is actually a problem.

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> In the future, please don't make decisions for Firefox.

  In the future, please don't assume that we're writing code only for Firefox when we're discussing things in the Bugzilla product, not in the bmo component.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #5)
> >+  [% ELSIF error == "group_change_denied" %]
> >+    [% title = "Cannot Add/Remove That Group" %]
> >+    You tried to add or remove group id [% group_id FILTER html %]
> 
> I'm not a big fan of these internal IDs. Is there really no way to disclose the
> group name?

  It's not so much that there's no way. I was under the impression that many installations don't want to expose group names to users who aren't members of them.

> If you can play with this bug, you already know groups it's restricted to
> (AFAIK), so displaying the group name here is not a security problem.

  There are (or could be) other interfaces to Bugzilla that would allow you to guess group ids and try to remove them.

Comment 9

[Max Kanat-Alexander]

Attachment: v2

Okay, I fixed everything and tested it pretty thoroughly.

Instead of changing groups_valid, I put the checks directly into add_group and remove_group, as Perl.

Comment 10

[Frédéric Buclin]

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

>Index: Bugzilla/Bug.pm

>+sub add_group {

>+        my $controls = $group->product_controls($self->product_obj);

You really don't want nor need product_controls(). You can write:

  my $controls = $self->product_obj->group_controls->{$group->id};

You use exactly the same fields but in a different order and use a method which already exists and is in use elsewhere. Moreover, this would fix comments I make below in Group.pm.


You can refactor this code a bit:

>+    my $ok;
>+    # OtherControl people can add groups only during a product change,
>+    # and only when the group is not NA for them.
>+    if (!Bugzilla->user->in_group($group->name)) {
>+        my $controls = $group->product_controls($self->product_obj);
>+        if ($self->{_old_product_name}
>+            && $controls->{othercontrol} != CONTROLMAPNA)
>+        {
>+            $ok = 1;
>+        }
>+    }
>+    # Since we're in this group and know it's valid, we're OK to add it.
>+    else {
>+        $ok = 1;
>+    }
>+
>+    if (!$ok) {
>+        ThrowUserError('group_change_denied',
>+                       { bug => $self, group_id => $group->id });
>+    }

Replace all this by:

# OtherControl people can add groups only during a product change,
# and only when the group is not NA for them.
if (!Bugzilla->user->in_group($group->name)) {
    my $controls = $self->product_obj->group_controls->{$group->id};
    if (!$self->{_old_product_name}
        || $controls->{othercontrol} == CONTROLMAPNA)
    {
        ThrowUserError('group_change_denied',
                       { bug => $self, group_id => $group->id });
    }
}

Much simpler, isn't it? ;)


>+sub remove_group {

>+        my $controls = $group->product_controls($self->product_obj);

Same comment as above about $group->product_controls being useless.


>+        my $ok;

Same comment as above about the refactoring.



>Index: Bugzilla/Group.pm

>+sub product_controls {
>+    my ($self, $product) = @_;
>+    my $prod_controls = $product->group_controls;

As said in Bug.pm, you really don't need it. Please remove it.


>+=item C<product_controls>

>+C<$product> (Optional) - A Bugzilla::Product object. If specified, limits
>+the returned data to a particular product.

That's wrong. $product is mandatory else $product->group_controls above will crash. Rather than fixing the description, remove the method. ;)


I did a few tests and your patch seems to work great. Nevertheless, I r- it as I think the refactoring suggested above and the removal of the useless method in Group.pm are worth enough to request an updated patch (and I fear that if I r+ it, you will ignore my comments ;) ).

Comment 11

[Max Kanat-Alexander]

Attachment: v3

Okay, here's the changes.

Comment 12

[Frédéric Buclin]

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

Seems to work fine. r=LpSolit

Comment 13

[Max Kanat-Alexander]

Yay! :-)

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.396; previous revision: 1.395
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.224; previous revision: 1.223
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.25; previous revision: 1.24
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.109; previous revision: 1.108
done
Checking in template/en/default/bug/process/verify-new-product.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/verify-new-product.html.tmpl,v  <--  verify-new-product.html.tmpl
new revision: 1.25; previous revision: 1.24
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.238; previous revision: 1.237
done