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.

RESOLVED FIXED

Status

()

bugzilla.mozilla.org
Administration
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Benjamin Smedberg, Assigned: glob)

Tracking

(Blocks: 1 bug, {sec-want})

Production
x86_64
Linux
sec-want
Dependency tree / graph

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
Keywords: sec-want
(Assignee)

Comment 1

4 years ago
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
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.
(Assignee)

Updated

4 years ago
Blocks: 856952
(Assignee)

Updated

4 years ago
Assignee: nobody → glob
(Assignee)

Comment 3

4 years ago
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.
Attachment #737400 - Flags: review?(dkl)

Comment 4

4 years ago
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.
(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
Flags: needinfo?(glob)
(Assignee)

Comment 6

4 years ago
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.
Flags: needinfo?(glob)
(Assignee)

Updated

4 years ago
Attachment #737400 - Attachment is obsolete: true
Attachment #737400 - Flags: review?(dkl)
(Assignee)

Comment 7

4 years ago
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.
(Assignee)

Comment 8

4 years ago
Created attachment 740936 [details] [diff] [review]
patch v2
Attachment #740936 - Flags: review?(dkl)
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...
(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
(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.
(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 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'};
Attachment #740936 - Flags: review?(dkl) → review+
(Assignee)

Comment 14

4 years ago
(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 on attachment 740936 [details] [diff] [review]
patch v2

Removing r+ based on comment 14
Attachment #740936 - Flags: review+
(Assignee)

Comment 16

4 years ago
Created attachment 744058 [details] [diff] [review]
patch v3
Attachment #740936 - Attachment is obsolete: true
Attachment #744058 - Flags: review?(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?
Attachment #744058 - Flags: review?(dkl) → review-
(Assignee)

Comment 18

4 years ago
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.. :)
Attachment #744058 - Flags: review- → review+
(Assignee)

Comment 19

4 years ago
Comment on attachment 744058 [details] [diff] [review]
patch v3

urgh, r? not r+ :)
Attachment #744058 - Flags: review+ → review?(dkl)
Comment on attachment 744058 [details] [diff] [review]
patch v3

Ah. Missed that step. r=dkl
Attachment #744058 - Flags: review?(dkl) → review+
(Assignee)

Comment 21

4 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Blocks: 869162
Just want to say that this works great -- thanks!
You need to log in before you can comment on or make changes to this bug.