Bug 408671

Hi,

is this still an issue with bugzilla 3.2? Looking under "Restrict Group Visibility:" -> "Current Groups: (edit)"...clicking the edit link reveals the security group checkbox for me. YMMV

Kevin, have you checked with BZ account with less privileges?  E.g. with non-RH account, there's no group selection at all.  Given that BZ should not allow anyone with access to the bug to uncheck security group, I think that group checkbox is only visible to members of security and admin BZ groups.

This is a bug and should be considered for fixing. Bugzilla is not recognizing that the "Security" group is set to be shown even for non-members in the edit bug template. The new bug template does this properly so we can try to replicate it's behaviour for the fix.

Created attachment 326085 [details]
v1 to enable assigning bug to security group for non security group members

The attached patch will view the security group in the group section of the bug report at the bug edit time even if the user is not a member of that group, and it will enable them to associate a bug to that group, but will not enable them to remove that group membership from a bug, so non members of security group can add bugs to the security group but they can not remove bugs from the security groups.

Noura

please note that the patch is at bz-web2 for your testing.

Comment on attachment 326085 [details]
v1 to enable assigning bug to security group for non security group members

I personally think we should keep this feature generic to all 'othercontrol' type groups. Currently upstream code only allows for 'othercontrol' groups to be altered when a product is being changed or a new bug is created.
I think we should allow any of those groups to be checked if they are not currently checked, but then not allowed unchecked except by real members. 

A different issue that this feature needs to address is the we still need to add 'security-responce' and 'Security' keyword if the security group is checked on a current bug. So maybe have that code in Bugzilla::Bug::update() that checks for all groups that are set, then looks at keywords and cc list to make sure the proper data is present.

Dave

Created attachment 326193 [details]
Sample patch on allowing othercontrol groups in show_bug.cgi to be set (v1)

Hey Noura. Here is a quick patch using some of what you did except to make it generic to all 'othercontrol' style groups instead of specific to security group. I commented out the part in add_group() that blocks othercontrol group changes except for product changes since we would not be able to do that anymore. The code in remove_group() can stay since we would only allow actual group members to uncheck the othercontrol groups. Also I don't have any code for adding the security cc and keyword data to the bug if the group is checked so that would still need to be added. What do you think or do you think we should just stick to looking for group id 71?

Dave

Created attachment 326272 [details]
v3 for security bugs update

Hey Dave, You patch looks good and works perfect for me, also i have added to it code that will update cc list and keyword with security information when the security group gets selected.

Noura

Comment on attachment 326272 [details]
v3 for security bugs update

>Index: Bugzilla/Bug.pm
>+    #if (!Bugzilla->user->in_group($group->name)) {
>+    #    my $controls = $self->product_obj->group_controls->{$group->id};
>+    #    if (!$self->{_old_product_name}
>+    #        || $controls->{othercontrol} == CONTROLMAPNA)
>+    #    {
>+    #        ThrowUserError('group_change_denied',
>+    #                       { bug => $self, group_id => $group->id });
>+    #    }
>+    #}

Need to add "# REDHAT EXTENSION " tags around this with a short explanation
of why we removed the functionality.
 
>Index: process_bug.cgi
>+    # REDHAT EXTENSION START 408671
>+    # Allow extensions to perform security bug data updates
>+    Bugzilla::Hook::process( 'update_security_bug_data',
>+                             { bug => $bug } );
>+    # REDHAT EXTENSION END 408671
>+

Use the current update_bug_data instead of creating a new one. For this we will need to move the current
hook up before the $bug->update($timestamp). For example:

Index: process_bug.cgi
===================================================================
RCS file: /cvs/qa/rh_bugzilla_3/process_bug.cgi,v
retrieving revision 1.29
diff -u -r1.29 process_bug.cgi
--- process_bug.cgi 28 Nov 2008 05:20:18 -0000  1.29
+++ process_bug.cgi 9 Dec 2008 17:25:00 -0000
@@ -586,6 +586,13 @@
     $dbh->bz_start_transaction();

     my $timestamp = $dbh->selectrow_array(q{SELECT NOW()});
+
+    # REDHAT EXTENSION START 406161 408671
+    # Allow extensions to perform data updates
+    Bugzilla::Hook::process( 'update_bug_data',
+        { bug => $bug, timestamp => $timestamp } );
+    # REDHAT EXTENSION END 406161 408671
+
     my $changes = $bug->update($timestamp);

     my %notify_deps;
@@ -631,12 +638,6 @@
     # Set and update flags.
     Bugzilla::Flag->process($bug, undef, $timestamp, $vars);

-    # REDHAT EXTENSION START 406161
-    # Allow extensions to perform data updates
-    Bugzilla::Hook::process( 'update_bug_data',
-        { bug => $bug, timestamp => $timestamp } );
-    # REDHAT EXTENSION END 406161
-
     $dbh->bz_commit_transaction();

Also, I am not sure that we needed to created a new extension directory called 
'bug_security'. I think we can just move extensions/bug_security/code/update_secuity_bug_data.pl
to extensions/redhat_fields/code/update_bug_data.pl instead. I have tested the above change
on bz-web2-test and moving the update_bug_data still allows the other extensions to work
such as external_bugs.

>Index: extensions/bug_security/code/update_security_bug_data.pl
>+foreach my $group (@{$bug->product_obj->groups_valid}) {
>+    my $gid = $group->id;
>+    if ($gid == 71 && 
>+        (defined $cgi->param("bit-$gid") || 
>+         defined $cgi->param("defined_bit-$gid")
>+        ) && 
>+        ($cgi->param("bit-$gid") == 1)){
>+
>+            $bug->add_cc('security-response-team');
>+            $bug->modify_keywords('Security', 'add');
>+
>+        }
>+}

Nit-pick: move the { to the line below in the if {} block and line them up.

    if ($gid == 71 
        && (defined $cgi->param("bit-$gid") || 
            defined $cgi->param("defined_bit-$gid")) 
        && ($cgi->param("bit-$gid") == 1))
    {

        $bug->add_cc('security-response-team');
        $bug->modify_keywords('Security', 'add');

    }

Created attachment 326434 [details]
v4 for adding code to update bug security data

Hey Dave, I have also tried using the same extension to the one we have for external bug and it worked fine. 

Thanks,
Noura

Comment on attachment 326434 [details]
v4 for adding code to update bug security data

Looks good Noura.

Thanks for the review Dave, committed to cvs should be live next bugzilla update which happens every Thursday.

Noura

Created attachment 326901 [details]
Patch to check that the security group is being newly added before updating cc and keywords

Noura please take a look at this patch that fixes the issue thoger mentioned. This now checks to see that the security checkbox is being added to a bug that it was not previously enabled before adding the cc and keyword. This should allow
the security team to remove themselves from CC if the want.

Thanks
Dave

Comment on attachment 326901 [details]
Patch to check that the security group is being newly added before updating cc and keywords

looks good Dave.

Noura