769134 - Bugzilla unintentionally removes groups when changing products with multiple bugs

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Simon Green]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0.1
Build ID: 20120616215613

Steps to reproduce:

1) Do a search that has private bugs
2) Click 'Change Several Bugs at Once' link
3) Select these bugs. and select a different Product. Note that all the add and remove group fields are unticked.
4) On the next page 'Verify New Product Details' select a new version, component, etc, and leave the 'verify bug group' check boxes unticked.
5) Press commit


Actual results:

All private groups from the bug are removed. This is despite us not selecting any groups to remove on the first change bug page.


Expected results:

No changes to the group privileges for the bug, assuming the new product offers the same group.

Comment 1

[Simon Green]

Attachment: First change bug page from 4.2 landfill showing problem

Comment 2

[Simon Green]

Attachment: Second change bug page 'Verify New Product Details' from 4.2 landfill showing problem

Comment 3

[Simon Green]

Before I submit a patch, I want to discuss the best solution. The add / remove this group check boxes on the first page use the 'defined_groups' and 'groups' HTML names. The second page also does this, but sets 'defined_groups' to all the bugs.

This means it has the same effect of all the 'Remove bugs from this group' options being ticked, and is clearly not what the user intended.

There are two possible solutions that I can see:
1) Do not show the groups at all on the second page.
2) Show the groups but do not set the 'defined_groups' values. This would give the user to select a group if the groups for the new product are different.

Thoughts?

Comment 4

[Simon Green]

I should also mention that this only happens when the following is true:

1) You are changing multiple bugs
2) You are changing the product of these bugs
3) It affects all selected bugs that have private groups.

It definitely occurs in 4.2 (which is what brc currently runs), but probably older versions too. I haven't tested trunk/4.3.1

  -- simon

Comment 5

[:glob ✱]

(In reply to Simon Green from comment #3)
> 2) Show the groups but do not set the 'defined_groups' values. This would
> give the user [the ability] to select a group if the groups for the new product are
> different.

i think this option makes sense.

Comment 6

[Simon Green]

(In reply to Byron Jones ‹:glob› from comment #5)
> (In reply to Simon Green from comment #3)
> > 2) Show the groups but do not set the 'defined_groups' values. This would
> > give the user [the ability] to select a group if the groups for the new product are
> > different.
> 
> i think this option makes sense.

Actually there is a problem with this option. If you select 'Remove from this group' on the first page, and select the same group on the second page (which is daft), that group won't be added to all bugs (but wouldn't be removed either.

Option 3 is to show the remove / add options on the second page. I'm starting to think that is the only way to really fix the problem. The selection on the second page would be copied over from the first page.

The reason why we need it is that the groups in the new product could be different from the groups for the current product(s).

Comment 7

[David Lawrence [:dkl]]

(In reply to Simon Green from comment #6)
> (In reply to Byron Jones ‹:glob› from comment #5)
> > (In reply to Simon Green from comment #3)
> > > 2) Show the groups but do not set the 'defined_groups' values. This would
> > > give the user [the ability] to select a group if the groups for the new product are
> > > different.
> > 
> > i think this option makes sense.
> 
> Actually there is a problem with this option. If you select 'Remove from
> this group' on the first page, and select the same group on the second page
> (which is daft), that group won't be added to all bugs (but wouldn't be
> removed either.
> 
> Option 3 is to show the remove / add options on the second page. I'm
> starting to think that is the only way to really fix the problem. The
> selection on the second page would be copied over from the first page.
> 
> The reason why we need it is that the groups in the new product could be
> different from the groups for the current product(s).

I agree with option 3. Code wise, process_bug.cgi and Bugzilla::Bug::set_all would need to be updated to recognize when multiple bugs are being updated and show a different group UI when changing product. On the interim page, if remove and add are not checked then the groups are not altered in any way. Similar to the first page.

dkl

Comment 8

[Simon Green]

(In reply to David Lawrence [:dkl] from comment #7)
> I agree with option 3. Code wise, process_bug.cgi and Bugzilla::Bug::set_all
> would need to be updated to recognize when multiple bugs are being updated
> and show a different group UI when changing product.

I don't think it needs to go this deep. In my patch, I've made a change in Bugzilla::Bug::_set_product (which is only called if Bugzilla is running in a browser). It checks for an id value. If it is set, it is assumed to be a single bug. If not set, it is a multiple bug.

> On the interim page, if
> remove and add are not checked then the groups are not altered in any way.
> Similar to the first page.

Agreed. Values from the first page will be carried forward to the second page (providing that group is available for the new product).

I have also checked that if we display a 3rd page (groups are set, but version, component, etc was not set) that my patch works correctly.

  -- simon

Comment 9

[Simon Green]

Attachment: v1 patch

Comment 10

[Simon Green]

Should mention that this patch was against trunk, but works on 4.2 as well (which is where I developed it)

  -- simon

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 637769 [details] [diff] [review]
v1 patch

Review of attachment 637769 [details] [diff] [review]:
-----------------------------------------------------------------

Functionally it works for me to solve the reported issue. The small changes could be fixed on commit.

r=dkl

::: Bugzilla/Bug.pm
@@ +2467,5 @@
>              $vars{'old_groups'} = Bugzilla::Group->new_from_list($gids);            
> +
> +            # Did we come here from editing multiple bugs? (affects how we
> +            # show optional group changes)
> +            $vars{multiple_bugs} = Bugzilla->cgi->param('id') ? 0 : 1;

You can just check for $params->{other_bugs} or @idlist instead of cgi->param('id').

            $vars{multiple_bugs} = @idlist ? 1 : 0;

::: template/en/default/bug/process/verify-new-product.html.tmpl
@@ +137,5 @@
>    [% IF optional_groups.size %]
>      <p>These groups are optional. You can decide to restrict [% terms.bugs %] to
>      one or more of the following groups:<br>
> +    [% IF multiple_bugs %]
> +      [% USE Bugzilla %]

No need as this is already setup for you to use cgi.param in global/hidden-fields.html.tmpl used earlier.

@@ +144,5 @@
> +            var other_checkbox = document.getElementById(id);
> +            if (myself.checked && other_checkbox) {
> +                other_checkbox.checked = false;
> +            }
> +        }

Now that this is in both edit-multiple.html.tmpl and here, we could look into moving this to an external js file such as js/util.js or similar and then include it in both places. I would be fine with that being a different bug as this is not alot of js code.

@@ +160,5 @@
> +          <td align="center">
> +            <input type="checkbox" name="defined_groups" 
> +                   id="defined_group_[% group.group.id %]"
> +                   value="[% group.group.name FILTER html %]"
> +                   [% IF Bugzilla.cgi.param("defined_groups").contains(group.group.name) %] checked="checked"[% END %]

s/Bugzilla.cgi/cgi/

@@ +167,5 @@
> +          <td align="center">
> +            <input type="checkbox" name="groups" 
> +                   id="group_[% group.group.id FILTER html %]"
> +                   value="[% group.group.name FILTER html %]"
> +                   [% IF Bugzilla.cgi.param("groups").contains(group.group.name) %] checked="checked"[% END %]

same

@@ +170,5 @@
> +                   value="[% group.group.name FILTER html %]"
> +                   [% IF Bugzilla.cgi.param("groups").contains(group.group.name) %] checked="checked"[% END %]
> +                   onchange="turn_off(this, 'defined_group_[% group.group.id %]')">
> +          </td>
> +

nit: remove blank lines

Comment 12

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #11)
> Functionally it works for me to solve the reported issue. The small changes
> could be fixed on commit.

There are many things to fix, even if they are small. Could an updated patch be uploaded before I approve it, please?

Comment 13

[Frédéric Buclin]

This is not a security bug, this patch is pretty invasive, and it touches group stuff, which can lead to security problems if there is a bug in the patch. So that's not something I want to take for an old stable branch. I will give a look at the patch before approving it for 4.2, to better understand what it does, just to be sure.

Comment 14

[Frédéric Buclin]

I'm not really happy with this fix, for at least two reasons:

- It requires JS to be turned on, and we still have many users with JS turned off, especially developers involved in security (so that they aren't affected by some malware attachments with embedded JS scripts).

- The UI is different if you move one single bug or several bugs at once, which is confusing, and inconsistent with mandatory and forbidden groups.

I would prefer a single checkbox in the intermediate page named "leave bugs in their respective groups if they still apply in the new product" and that's it. This mean this checkbox would apply to all optional groups at once, instead of per-group. I think this is enough in 99% of cases. And if we really want it to be per-product, then one additional checkbox per group would do it, taking its information from the first page. But maybe that's overkill (from a UI point of view).

Cancelling requests for approval, because the UI should be consistent and not depend on JS, for the reasons given above.

Comment 15

[Simon Green]

(In reply to Frédéric Buclin from comment #14)
> I'm not really happy with this fix, for at least two reasons:
> 
> - It requires JS to be turned on, and we still have many users with JS
> turned off, especially developers involved in security (so that they aren't
> affected by some malware attachments with embedded JS scripts).

It's the same javascript as on the first page, which unticks one check box if the other one in the same row is checked. If a user does not have JS and both columns are checked, then it would behave the same as if only the 'Add to this group' is checked.
 
> - The UI is different if you move one single bug or several bugs at once,
> which is confusing, and inconsistent with mandatory and forbidden groups.

It is the same UI as on the first page (when changing many bugs are being changed). I would argue the opposite. The way that it currently is, users have a false expectation that if they make no changes to the groups on either page that existing permissions would be retained.

If a group is mandatory or forbidden in the new product, this is reported the same way as if it was a single bug.

> I would prefer a single checkbox in the intermediate page named "leave bugs
> in their respective groups if they still apply in the new product" and
> that's it. This mean this checkbox would apply to all optional groups at
> once, instead of per-group. I think this is enough in 99% of cases. And if
> we really want it to be per-product, then one additional checkbox per group
> would do it, taking its information from the first page. But maybe that's
> overkill (from a UI point of view).

I'm reassigning this to the default asignee for the product. I'm happy with the way the original patch was written (with dkl's change), and brc will carry that change internally. I'll leave it for someone else to do it the way that upstream want it.

  -- simon

Comment 16

[Dave Miller [:justdave]]

(In reply to Frédéric Buclin from comment #13)
> This is not a security bug

I would differently on that, actually.  Removing groups from a bug that the user didn't ask to remove without warning them that they were about to do so is potentially a private data disclosure issue.

> this patch is pretty invasive, and it touches group stuff, which can lead
> to security problems if there is a bug in the patch.

This is also true. But I would argue that the problem needs to be fixed regardless because this is still a private data disclosure hole.  The only thing stopping me from suggesting we security release on this is the extreme unlikeliness of triggering it (that it took so long to be discovered when this UI has been around so long is a sign of that).

I suspect BRC has been using this for a while and can probably tell us if there's been side effects or not.  Assuming it's been working well for them, I'll approve for 4.2 as well.  I just did a once-over on the patch myself, and this doesn't look as invasive to me as what was suggested in the above comments.  The change being made is pretty straightforward when you compare it to the existing code for the same thing in the first page's code path.

Comment 17

[Dave Miller [:justdave]]

I guess BRC is already running 4.4, and the UI change might be confusing on such an old release.

Comment 18

[Simon Green]

Patch had to be modified to become runtests compliant.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/Bug.pm
modified template/en/default/bug/process/verify-new-product.html.tmpl
Committed revision 8756.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.4/                         
modified Bugzilla/Bug.pm
modified template/en/default/bug/process/verify-new-product.html.tmpl
Committed revision 8612.

Comment 19

[Frédéric Buclin]

(Please do not forget to set the target milestone when approving/committing stuff. Else it's unmanageable.)

Comment 20

[Frédéric Buclin]

Attachment: fixes on checkin

All fixes on checkin requested by dkl in comment 11 have been ignored:

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Bug.pm
modified template/en/default/bug/process/verify-new-product.html.tmpl
Committed revision 8770.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified Bugzilla/Bug.pm
modified template/en/default/bug/process/verify-new-product.html.tmpl
Committed revision 8618.

Comment 21

[Frédéric Buclin]

relnoted in 4.4.1.