Last Comment Bug 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.
: When moving bugs from one product to another, I should be able to keep a secu...
Status: RESOLVED FIXED
: sec-want
Product: bugzilla.mozilla.org
Classification: Other
Component: Administration (show other bugs)
: Production
: x86_64 Linux
: -- normal with 1 vote (vote)
: ---
Assigned To: Byron Jones ‹:glob›
:
Mentors:
Depends on:
Blocks: 856952 869162
  Show dependency treegraph
 
Reported: 2013-01-02 06:59 PST by Benjamin Smedberg [:bsmedberg]
Modified: 2013-05-20 12:38 PDT (History)
7 users (show)
See Also:
Due Date:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (6.35 KB, patch)
2013-04-15 02:30 PDT, Byron Jones ‹:glob›
no flags Details | Diff | Review
patch v2 (10.72 KB, patch)
2013-04-23 11:12 PDT, Byron Jones ‹:glob›
no flags Details | Diff | Review
patch v3 (16.12 KB, patch)
2013-05-01 01:59 PDT, Byron Jones ‹:glob›
dkl: review+
Details | Diff | Review

Description Benjamin Smedberg [:bsmedberg] 2013-01-02 06:59:49 PST
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 Byron Jones ‹:glob› 2013-04-01 21:23:50 PDT
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] 2013-04-01 21:51:15 PDT
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 Byron Jones ‹:glob› 2013-04-15 02:30:08 PDT
Created attachment 737400 [details] [diff] [review]
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 2013-04-16 13:59:02 PDT
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] 2013-04-16 15:29:29 PDT
(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 Byron Jones ‹:glob› 2013-04-16 22:14:10 PDT
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 Byron Jones ‹:glob› 2013-04-17 09:37:20 PDT
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 Byron Jones ‹:glob› 2013-04-23 11:12:46 PDT
Created attachment 740936 [details] [diff] [review]
patch v2
Comment 9 Reed Loden [:reed] (use needinfo?) 2013-04-23 11:21:33 PDT
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] 2013-04-23 15:11:08 PDT
(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] (use needinfo?) 2013-04-23 15:23:41 PDT
(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] 2013-04-23 18:25:25 PDT
(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] 2013-04-24 09:12:18 PDT
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 Byron Jones ‹:glob› 2013-04-24 10:10:20 PDT
(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] 2013-04-24 14:42:37 PDT
Comment on attachment 740936 [details] [diff] [review]
patch v2

Removing r+ based on comment 14
Comment 16 Byron Jones ‹:glob› 2013-05-01 01:59:16 PDT
Created attachment 744058 [details] [diff] [review]
patch v3
Comment 17 David Lawrence [:dkl] 2013-05-02 13:50:39 PDT
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 Byron Jones ‹:glob› 2013-05-02 22:47:18 PDT
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 Byron Jones ‹:glob› 2013-05-02 22:47:50 PDT
Comment on attachment 744058 [details] [diff] [review]
patch v3

urgh, r? not r+ :)
Comment 20 David Lawrence [:dkl] 2013-05-03 09:14:33 PDT
Comment on attachment 744058 [details] [diff] [review]
patch v3

Ah. Missed that step. r=dkl
Comment 21 Byron Jones ‹:glob› 2013-05-05 23:32:30 PDT
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/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] 2013-05-20 12:38:48 PDT
Just want to say that this works great -- thanks!

Note You need to log in before you can comment on or make changes to this bug.