825886 - When moving bugs from one product to another, I should be able to keep a security bug private across groups that I'm not a member of.

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Administration, task)

Description

[Benjamin Smedberg]

I frequently end up in this situation: as a member of the client s-g, I can see website security bugs which are "misfiled" in client products with the client s-s flag. I want to move those bugs into another product which has a different security group, but I can't because I'm not a member of the target security group.

The current dance required for this is getting somebody from webtools to add the bug to the webtools s-s group, then move the bug, then remove it from the client s-s group. (I suppose that somebody who is in both groups could do this in one step, but I don't know who those people are.)

This is a subset of bug 504461 which might not have the same UI issues as that bug.

Comment 1

[:glob ✱]

after chatting with dveditz about this, one proposal is:

if (moving bug to diff product) 
   && (bug is in any groups)
   && (new product doesn't have any of the groups)
then
  show and check default security group
  allow user to place bug into that group when moving

Comment 2

[Daniel Veditz [:dveditz]]

This design preserves the ability to say "I want these bugs to be public" by unchecking that box, but it wouldn't happen accidentally or due to lack of privileges as it can now.

Comment 3

[:glob ✱]

Attachment: patch v1

here's an 'ignoring whitespace' patch for this (i've removed _all_ trailing whitespace from BMO/Extension.pm).

this adds a hook on the verify page which is used to insert the default sec group if it's missing.  if groups are being invalidated, the default sec group will be checked by default.

Comment 4

[Frédéric Buclin]

I don't know how group settings are configured in bmo, but this bug is invalid upstream. When you move a bug into another product, you can restrict the bug to groups you don't belong to. Looks like bmo uses Shown/NA instead of Shown/Shown. Would be better to fix group settings than to hack bmo.

Comment 5

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #4)
> I don't know how group settings are configured in bmo, but this bug is
> invalid upstream. When you move a bug into another product, you can restrict
> the bug to groups you don't belong to. Looks like bmo uses Shown/NA instead
> of Shown/Shown. Would be better to fix group settings than to hack bmo.

This is true and I just tried a test case locally to be sure. This has the side-effect of showing the product's security group as a checkbox on the new bug entry form in addition to the custom checkfox BMO has added for making a bug private. Both checkboxes are wired to the same group.

The product change verification page shows the new product's security group (if set to Shown/Shown) and can be checked if the user wants to keep the bug private.

So essentially doing the same as this patch, right?

dkl

Comment 6

[:glob ✱]

i'd assumed this was done in code for enter_bug for some reason (instead of taking the much easier shown/shown route).

i will do some archeology work, see if i can determine why that happened.

Comment 7

[:glob ✱]

it looks like the custom code which allows users to file bugs into the default security groups is ancient, and has simply been brought forward throughout the bmo upgrades.

this customisation is no longer required.

i'll work on a patch which removes it and fixes the duplication of group checkboxes on the enter_bug page.  once committed we can update the default security group's settings on all products from shown/na to shown/shown.

Comment 8

[:glob ✱]

Attachment: patch v2

Comment 9

[Reed Loden [:reed]]

Comment on attachment 740936 [details] [diff] [review]
patch v2

What about products that use multiple security groups depending on the issue (basically, custom forms that will pick a specific security group)? This seems like that would be broken...

Comment 10

[David Lawrence [:dkl]]

(In reply to Reed Loden [:reed] from comment #9)
> Comment on attachment 740936 [details] [diff] [review]
> patch v2
> 
> What about products that use multiple security groups depending on the issue
> (basically, custom forms that will pick a specific security group)? This
> seems like that would be broken...

Those forms normally do not show the full list of group checkboxes and just have the group names as hidden form fields. Those do not rely on the product security groups defined in the Data.pm file as far as I can tell.

dkl

Comment 11

[Reed Loden [:reed]]

(In reply to David Lawrence [:dkl] from comment #10)
> Those forms normally do not show the full list of group checkboxes and just
> have the group names as hidden form fields. Those do not rely on the product
> security groups defined in the Data.pm file as far as I can tell.

They do rely on the ability for *any user* to file a bug in the specified group, though (even if said user is not in said group). Just want to make sure that won't get broken.

Comment 12

[David Lawrence [:dkl]]

(In reply to Reed Loden [:reed] from comment #11)
> (In reply to David Lawrence [:dkl] from comment #10)
> > Those forms normally do not show the full list of group checkboxes and just
> > have the group names as hidden form fields. Those do not rely on the product
> > security groups defined in the Data.pm file as far as I can tell.
> 
> They do rely on the ability for *any user* to file a bug in the specified
> group, though (even if said user is not in said group). Just want to make
> sure that won't get broken.

Yep. That will be possible by setting the group to shown/shown which will need to be done for the default security group.

dkl

Comment 13

[David Lawrence [:dkl]]

Comment on attachment 740936 [details] [diff] [review]
patch v2

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

You will need to incorporate the create.html.tmpl changes into the custom reporting forms that access product.groups_available and do not have the group names as hard coded hidden fields.

Also I had to make some group changes (which we discussed in irc) to allow one or more custom forms to work such as form.moz.project.review. Examples:

mozilla.org :: mozilla-corporate-confidential :: shown/na -> shown/shown
Privacy :: mozilla-corporation-confidential :: shown/na -> shown/shown

This was using a non-mozilla test account which would otherwise we able to create bugs using the form. Also these groups are not part of %product_sec_groups but were originally part of %always_fileable. Glob assures (irc) that we will make sure the proper groups are all converted before code pushed to prod.

Otherwise works fine and solves the private bug moving from one product to another issue as described. r=dkl

::: extensions/BMO/Extension.pm
@@ +1127,5 @@
> +
> +sub _default_security_group_obj {
> +    my ($self) = @_;
> +    return unless my $group_name = $self->default_security_group;
> +    return Bugzilla::Group->new({ name => $group_name })

nit: 

    $self->{'default_security_group'} ||= Bugzilla::Group->new({ name => $group_name });
    return $self->{'default_security_group'};

Comment 14

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #13)
> You will need to incorporate the create.html.tmpl changes into the custom
> reporting forms that access product.groups_available and do not have the
> group names as hard coded hidden fields.

that'll be easy -- just the mozpr form does this.

> Also I had to make some group changes (which we discussed in irc) to allow
> one or more custom forms to work such as form.moz.project.review.

when doing this patch, i didn't consider custom forms which file bugs into currently always-fileable groups which aren't that product's default security group.

after auditing the form, this is reasonably frequent.


given we want to keep from showing these groups on the enter_bug page, i think the best way to fix this would be to retain the current always-filable-group list and code, but switch the default-security-group to shown/shown to address the product moving issue.

Comment 15

[David Lawrence [:dkl]]

Comment on attachment 740936 [details] [diff] [review]
patch v2

Removing r+ based on comment 14

Comment 16

[:glob ✱]

Attachment: patch v3

Comment 17

[David Lawrence [:dkl]]

Comment on attachment 744058 [details] [diff] [review]
patch v3

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

I was not able to get this to right off of the bat for two reasons:

1. I had to add the extra code to _check_default_product_security_group to add the group object to $optional_group_controls. This allowed the default security group checkbox and name to be displayed and checked by default.

2. You also need to add $product->group_always_settable to Bugzilla::Bug::add_group otherwise I got the error that I could not move the bug into the new secure group since I was not a member of the group even though it as displayed and checked in the verification page. Bugzilla::Bug::set_product calls add_group separately to _check_groups which does it's own checks separately.

dkl

::: extensions/BMO/Extension.pm
@@ +1152,5 @@
> +    if (@$invalid_groups) {
> +        my $cgi = Bugzilla->cgi;
> +        my @groups = $cgi->param('groups');
> +        push @groups, $group->name unless grep { $_ eq $group->name } @groups;
> +        $cgi->param('groups', @groups);

In order to get this part to work properly I had to add the following as well:

        $cgi->param('groups', @groups);
+       push(@$optional_group_controls, { group => $group });

::: extensions/BMO/lib/Data.pm
@@ +395,5 @@
>  
>  # Mapping of products to their security bits
>  our %product_sec_groups = (
>      "addons.mozilla.org"           => 'client-services-security',
> +    "Air Mozilla"                  => 'mozilla-corporation-confidential',

Separate bug?

Comment 18

[:glob ✱]

Comment on attachment 744058 [details] [diff] [review]
patch v3

(In reply to David Lawrence [:dkl] from comment #17)
> I was not able to get this to right off of the bat for two reasons:

sounds like you didn't set the default security group for the products to shown/shown.

> +    "Air Mozilla"                  => 'mozilla-corporation-confidential',

i detected this issue while auditing the custom fields/groups for this.  can split into a separate bug, but figured while i was in that neck of the woods.. :)

Comment 19

[:glob ✱]

Comment on attachment 744058 [details] [diff] [review]
patch v3

urgh, r? not r+ :)

Comment 20

[David Lawrence [:dkl]]

Comment on attachment 744058 [details] [diff] [review]
patch v3

Ah. Missed that step. r=dkl

Comment 21

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
modified Bugzilla/Bug.pm
modified extensions/BMO/Extension.pm
modified extensions/BMO/lib/Data.pm
modified extensions/BMO/template/en/default/bug/create/create-bootgecko-partner.html.tmpl
modified extensions/BMO/template/en/default/bug/create/create-mdn.html.tmpl
modified extensions/BMO/template/en/default/bug/create/create-mozpr.html.tmpl
modified extensions/BMO/template/en/default/hook/bug/create/create-form.html.tmpl
missing extensions/BMO/template/en/default/hook/bug/create/create-guided-form.html.tmpl
deleted extensions/BMO/template/en/default/hook/bug/create/create-guided-form.html.tmpl
modified template/en/default/bug/create/create.html.tmpl
modified template/en/default/bug/process/verify-new-product.html.tmpl
Committed revision 8772.

Comment 22

[Daniel Veditz [:dveditz]]

Just want to say that this works great -- thanks!