276553 - process_bug should yell if you try to add a bug to a group that isn't legal for that product

Status: RESOLVED WORKSFORME

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Dave Miller [:justdave]]

Scenario:

I just created a new update-security group for the Update product so they can
add people to it that don't have a reason to see Bugzilla/Bonsai stuff.

I did a query on all bugs in the Update product with webtools-security set on
them, and did "change several bugs at once".  I checked all, and told it to
remove webtools-security and add update-security.  Clicked submit.

Watched several people who weren't in the security groups get mailed about the
bug changes.  Went "huh?"

Discovered I had forgotten to add update-security as a legal group on the Update
product.  This just momentarily exposed 15 security bugs to people via watches.
 process_bug should have rejected the change since the group wasn't legal (and
it probably wouldn't have hurt anything to have pared down the group list on
change-multiple to only include the legal ones for the product, since every bug
in the list was in the same product).

Comment 1

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1

UI solution -- don't display group addition/removal options if they might be
invalid.

Having process_bug.cgi complain doesn't work because it does its work step by
step -- at the time we're looking at the bugs the keywords have already
changed, for example. Here we go again, wanting database transaction support :)

Comment 2

[Joel Peshkin]

How does this handle product changes or lists of bugs that span multiple products?

Comment 3

[Marc Schumann [:Wurblzap]]

(In reply to comment #2)
> How does this handle product changes or lists of bugs that span multiple
> products?

This doesn't change process_bug.cgi, and as such, it does not alter current
product change behaviour.

It displays the remove possibility for a group only if all bugs may be moved out
of the group.
It displays the add possibility for a group only if all bugs may be moved into
the group.
If both is not possible, the group is not displayed at all.

Comment 4

[Joel Peshkin]

Comment on attachment 188416 [details] [diff] [review]
Patch 1

r=joel
This either needs a 2xr or it needs plenty of time for	testing before it gets
to a major release.

Comment 5

[Frédéric Buclin]

Comment on attachment 188416 [details] [diff] [review]
Patch 1

>Index: buglist.cgi

>+    my $sth = $dbh->prepare(qq{
>+          SELECT MIN(COALESCE(gcm.membercontrol, ?)) AS mincontrol,
>+                 MAX(COALESCE(gcm.membercontrol, ?)) AS maxcontrol
>+            FROM products
>+       LEFT JOIN group_control_map AS gcm
>+              ON gcm.product_id = products.id
>+             AND gcm.group_id = ?
>+           WHERE products.id IN ($bugproductidlist) }
>+       .$dbh->sql_group_by('COALESCE(gcm.group_id, ?)'));

I see no reason to do a 'GROUP BY group_id' because you already limit the query
to a given group ID: 'AND gcm.group_id = ?'.


>+    foreach $group (@$groups) {
>+        $sth->execute(CONTROLMAPNA, CONTROLMAPNA, $$group{'id'}, $$group{'id'});
>+        my $row = $sth->fetchrow_hashref();
>+        $$group{'mincontrol'} = $$row{'mincontrol'};
>+        $$group{'maxcontrol'} = $$row{'maxcontrol'};
>+    }

I think you can write: $group = $sth->fetchrow_hashref(); as $group is already
a hash.


> DefineColumn("short_desc"        , "bugs.short_desc"            , "Summary"          );
> DefineColumn("status_whiteboard" , "bugs.status_whiteboard"     , "Whiteboard"       );
> DefineColumn("component"         , "map_components.name"        , "Component"        );
>+DefineColumn("product_id"        , "bugs.product_id"            , "Product ID"       );

I see no reason to duplicate the code. Product names are unique, so you should
use them to pass to the routine above.


>     # The groups to which the user belongs.
>-    $vars->{'groups'} = GetGroupsByUserId($::userid);
>+    $vars->{'groups'} = GetGroupsByUserIdAndProducts($::userid,
>+                                                     $bugproductids);

You should pass a list of product names; they are unique.



>Index: template/en/default/list/edit-multiple.html.tmpl

>+      [% IF    (group.mincontrol == constants.CONTROLMAPNA ||
>+                group.mincontrol == constants.CONTROLMAPMANDATORY)
>+            && (group.maxcontrol == constants.CONTROLMAPMANDATORY ||
>+                group.maxcontrol == constants.CONTROLMAPNA) %]
>+        [% foundmandatory = 1 %]
>+        [% foundna = 1 %]
>       [% ELSE %]

I disagree! If group.maxcontrol == constants.CONTROLMAPNA, [% foundmandatory =
1 %] is incorrect.
If group.mincontrol == constants.CONTROLMAPMANDATORY, [% foundna = 1 %] is
incorrect too.

I didn't review the rest of the file yet.

Comment 6

[Marc Schumann [:Wurblzap]]

(In reply to comment #2)
> How does this handle product changes or lists of bugs that span multiple
> products?

Hmmmmmmmmm.... Thinking a little more, I'm not sure whether doing it the way of
attachment 188416 [details] [diff] [review] is any good. You may move bugs into any product, so it should
be possible to move bugs into groups of the new product. Otherwise, you might be
forced to temporarily make bugs visible: if only groups of all current products
are shown, then the new product's group you want to move the bugs into along
with the product switch might not be visible.

:(

I've no idea short of making bug/process/verify-new-product.html.tmpl give a
comprehensive group selection.

Comment 7

[Frédéric Buclin]

*** Bug 305790 has been marked as a duplicate of this bug. ***

Comment 8

[Marc Schumann [:Wurblzap]]

It seems I won't be able to come up with a comprehensive solution in any
sensible timeframe :(

Comment 9

[Dave Miller [:justdave]]

This is important to fix and we should still try to do something soon (and
backporting to the branches is okay) but I'm not going to hold releases for it.

Comment 10

[Frédéric Buclin]

(In reply to comment #6)
> You may move bugs into any product, so it should
> be possible to move bugs into groups of the new product. Otherwise, you might be
> forced to temporarily make bugs visible

No worry about moving bugs. Available groups for the new product are displayed in the intermediate page, and you are then free to select the ones you want. So there is no risk to have some bugs becoming public without your consent.

This means this bug only needs to focus on the case where bugs remain in their respective product.

Comment 11

[Frédéric Buclin]

To make things even clearer for users, we could use some JS to hide the group table if the product field is not set to "do not change" and display a text which would explain that groups can be set on the next page (a.k.a intermediate page a.k.a confirmation page). And if the user set back the product field to "do not change", then we redisplay the group table again. So there should be no confusion about this.

Comment 12

[Frédéric Buclin]

The Bugzilla 3.0 branch is now locked to security bugs and dataloss fixes only. This bug doesn't fit into one of these two categories and is retargetted to 3.2 as part of a mass-change. To catch bugmails related to this mass-change, use lts081207 in your email client filter.

Comment 13

[Max Kanat-Alexander]

This is working as of about 3.6, I believe. (It might have been fixed in 3.4 though.)