several bugs in process_bug.cgi when 'strict_isolation' is on

RESOLVED FIXED in Bugzilla 2.22

Status

()

Bugzilla
Creating/Changing Bugs
--
major
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Frédéric Buclin, Assigned: Joel Peshkin)

Tracking

2.21
Bugzilla 2.22
Dependency tree / graph
Bug Flags:
approval +
blocking2.22 +

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

12 years ago
When you move a bug to a component with no default QA contact and you also reassign the bug to the default assignee and QA contact, you get an error that '' is not able to edit the bug. This is due to:

    if ($cgi->param('qa_contact') && Param("strict_isolation")) { 
        Bugzilla::Bug::can_add_user_to_bug(
            $bug_obj->{'product_id'}, $id, $qacontact);
    }

where $qacontact is undefined when the component has no default QA contact. $cgi->param('qa_contact') is the content of the "QA contact" field and should not be considered (is unreliable) when reassigning to default assignee and QA contact of the component.
(Reporter)

Comment 1

12 years ago
process_bug.cgi is full of bugs due to the strict_isolation param. Morphing the bug summary to fix them all here instead of opening one bug per problem detected.

/me blames joel
Status: NEW → ASSIGNED
Summary: process_bug.cgi returns an error when 'strict_isolation' is on and you move the bug to a component with no default QA contact → several bugs in process_bug.cgi when 'strict_isolation' is on
(Reporter)

Comment 2

12 years ago
Culprits are bug 309681 and bug 312787.

I can bypass most checks related to strict_isolation quite easily, involving letting process_bug.cgi thinking I'm someone else!! o_O

Requesting blocking due to that...
Flags: blocking2.22?
(Assignee)

Comment 3

12 years ago
Comment #2 makes no sense to me. Where strict_isolation applies to cclist memebers, assignees, and qacontacts, this is to keep people who do have the right to edit a bug from adding OTHER people who should not be on that bug.

  Please clarify.
(Reporter)

Comment 4

12 years ago
(In reply to comment #3)
> Comment #2 makes no sense to me. Where strict_isolation applies to cclist
> memebers, assignees, and qacontacts, this is to keep people who do have the
> right to edit a bug from adding OTHER people who should not be on that bug.

I understand that.

line 172:

            if (Param("strict_isolation")) {
                my $deltabug = new Bugzilla::Bug($id, $user);
                if (!$user->can_edit_product($deltabug->{'product_id'})) {
                    $vars->{'field'} = $field;
                    ThrowUserError("illegal_change_deps", $vars);
                }
            }

Here, you pass $user as a second argument to Bugzilla::Bug->new, but this method expect to receive the user ID, not a user object. Now Bugzilla::Bug behaves as if you were logged out. This is a problem if the bug is restricted to some groups.


line 1018:

        $qacontact = DBNameToIdAndCheck($name) if ($name ne "");
        if (Param("strict_isolation")) {
                my $product_id = get_product_id($cgi->param('product'));
                Bugzilla::Bug::can_add_user_to_bug(
                    $product_id, $cgi->param('id'), $qacontact);
        }

When changing several bugs at once, you can easily bypass this check by leaving the product field as "--dont-change--". As there is no group restriction on this unexisting product, it will not prevent you from adding users to the bug.
In all cases, this is not the right place for this check.


line 1085:

        if (defined $cgi->param('assigned_to')) { 
            my $uid = DBNameToIdAndCheck($cgi->param('assigned_to'));
            if (Param("strict_isolation")) {
                my $product_id = get_product_id($cgi->param('product'));
                Bugzilla::Bug::can_add_user_to_bug(
                    $product_id, $cgi->param('id'), $uid);
            }
        } elsif (!defined $cgi->param('assigned_to')
            || trim($cgi->param('assigned_to')) eq "") {
            ThrowUserError("reassign_to_empty");
        }
        $assignee = DBNameToIdAndCheck(trim($cgi->param('assigned_to')));

Same remark as above: you can easily bypass this check by changing several bugs at once as $cgi->param('product') doesn't contain the product name when this field is left untouched.
Moreover, you call DBNameToIdAndCheck() twice, which doesn't make sense, really!
And finally, note that you interverted the checks. Now if no assignee is given, you will get the standard error from DBNameToIdAndCheck() instead of calling ThrowUserError("reassign_to_empty").


line 1400:

    if ($cgi->param('assigned_to') && Param("strict_isolation")) { 
        my $uid = DBNameToIdAndCheck($cgi->param('assigned_to'));
        Bugzilla::Bug::can_add_user_to_bug(
            $bug_obj->{'product_id'}, $id, $uid);
    }
    if ($cgi->param('qa_contact') && Param("strict_isolation")) { 
        Bugzilla::Bug::can_add_user_to_bug(
            $bug_obj->{'product_id'}, $id, $qacontact);
    }

Not only the assignee has already been checked earlier, but you are referring to the wrong fields (including the QA contact, see comment 0) when reassigning the bug to the default assignee and QA contact. This cause unexpected errors.
Moreover, when moving the bug into another product/component where the actual assignee and/or QA contact are not allowed to edit, the next time you try to edit the bug, you will get an error telling you that the assignee and/or QA contact are not allowed to edit the bug, even if these fields are left untouched. A user who is allowed to edit the product but hasn't editbugs privs cannot change the bug *at all* anymore.
Another point, having a product where the actual or default assignee and/or QA contact are not allowed to edit the product (you could encounter this situation, even if this doesn't make sense to do so. editproducts.cgi doesn't prevent you from doing so), not you cannot reassign the bug to default assignee and QA contact anymore, but you cannot leave these fields untouched. If the assignee and/or QA contact are already in place, they should stay in place.

Note that checks done around line 1400 are the only ones which make sense, as you now know the product the bug is in (but you should also care about where the bug is moving into, else the next time you want to edit the bug, you will get an error). The checks at line 1018 and 1085 should be removed completely!


line 1626:

        if (Param("strict_isolation")) {
            foreach my $pid (keys %cc_add) {
                my $user = Bugzilla::User->new($pid);
                if (!$user->can_edit_product($bug_obj->{'product_id'})) {
                    push (@blocked_cc, $cc_add{$pid});
                }
            }
            if (scalar(@blocked_cc)) {
                my $blocked_cc = join(", ", @blocked_cc);
                ThrowUserError("invalid_user_group", 
                    {'user' => $blocked_cc , bug_id => $id });
            }
        }

What happens if you are moving a bug into a product these users cannot edit? The next time you will try to edit the bug, an error will be thrown.


This is a big r- for the patches in bug 309681 and bug 312787. The right fix so far is to back out these two patches and rewrite a correct patch.

justdave, is that fine for you?
(Reporter)

Updated

12 years ago
Depends on: 309681, 312787
(Reporter)

Comment 5

12 years ago
(13:59:12) myk: LpSolit: i'm just wondering whether a patch by joel that fixes the bugs you found would be preferable to backing out the buggy patches that have already made it in
(14:03:06) LpSolit: myk: oh ok, other patches have been submitted since then which prevent us from backing them out :-/
(14:04:43) LpSolit: note that all these checkins are post-2.21.1, so no release contains this broken param so far
Flags: blocking2.22? → blocking2.22+
(Reporter)

Updated

12 years ago
Blocks: 315330
(Assignee)

Comment 6

12 years ago
Created attachment 202805 [details] [diff] [review]
Partial patch - still need to prevalidate mass changes
(Assignee)

Comment 7

12 years ago
OK... here we go....

process_bug figures out which product(s) the bug(s) is/are being changed to OR is/are remaining in.  It does this while building a query before ever processing a bug.

If the product(s) is/are not changing, then any new assignee or CC member or QA contact gets checked against the union of the entry groups for all of those products. (We can assume that default owners and qa contacts are permitted to see bugs in the product for which they are assigned!!)  This can be done before starting to process the updates for individual bugs.

When the product is changed, we know this before entering the loop to process individual bugs.  We can obtain a list of all of the assignees (if that is not changing), qa_contacts (if that is not changing), and existing cclist members for all of the bugs being moved and check them against the entry groups for the product.

This means that you cannot move a bug to a new product if it has a cc member that is not permitted to go along with it even if you try to remover that member at the same time.  You can always remove the offending member first and then move the bug if you want.

(Assignee)

Comment 8

12 years ago
Created attachment 202863 [details] [diff] [review]
The fix as described in the previous comments
Assignee: LpSolit → bugreport
Attachment #202805 - Attachment is obsolete: true
Attachment #202863 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #202863 - Flags: review?(LpSolit)
(Reporter)

Comment 9

12 years ago
Comment on attachment 202863 [details] [diff] [review]
The fix as described in the previous comments

>Index: process_bug.cgi

>@@ -170,7 +170,7 @@
>             if (Param("strict_isolation")) {
>+                my $deltabug = new Bugzilla::Bug($id, $user->id);
>                 if (!$user->can_edit_product($deltabug->{'product_id'})) {

Nit partially related to this bug: if (!CheckCanChangeField($field, $bug->bug_id, 0, 1)) written right before this block should not be called for all bugs in the dependency list as it only affects the current bug. Could you please fix this issue here or open a new bug and mark it as depending on this one and CC me on it?


>@@ -862,14 +863,19 @@
> } else {
>-    SendSQL("SELECT DISTINCT product_id FROM bugs WHERE bug_id IN (" .
>-            join(',', @idlist) . ") " . $dbh->sql_limit(2));
>-    $prod_id = FetchOneColumn();
>-    $prod_id = undef if (FetchOneColumn());

>+    @newprod_ids = @{$dbh->selectcol_arrayref("SELECT DISTINCT product_id
>+                             FROM bugs 
>+                             WHERE bug_id IN (" .
>+                             join(',', @idlist) . 
>+                             ")")};

Nit: please fix the indentation (align SELECT, FROM and WHERE).


>+    $prod_id = undef if (scalar(@newprod_ids) > 1);

This change is wrong. If there is only one product returned, $prod_id should be set to this product ID. Here, $prod_id is never defined.


>+print STDERR "product ids: " . join(',',@newprod_ids) . "\n";

bye bye! (debug stuff)


>@@ -1006,6 +1012,8 @@
> 
> my $assignee;
> my $qacontact;
>+my $qacontact_checked = 0;
>+my $assignee_checked = 0;

I still don't understand why you split these validations all over the code. Having a central place would make much more sense.


>@@ -1083,15 +1099,23 @@
>         if (defined $cgi->param('assigned_to')) { 

Please restore this test to its initial state (read my previous review)! It was:
if (defined $cgi->param('assigned_to') && trim($cgi->param('assigned_to')) ne "")


>             my $uid = DBNameToIdAndCheck($cgi->param('assigned_to'));

Again, I asked to remove this duplicated call to DBNameToIdAndCheck(). $uid should be $assignee and will be used in $::query.


>         } elsif (!defined $cgi->param('assigned_to')
>             || trim($cgi->param('assigned_to')) eq "") {

With the change above, this simply reduced to "ELSE".


>         $assignee = DBNameToIdAndCheck(trim($cgi->param('assigned_to')));

As I said, $assignee must be defined in the IF block.


>@@ -1288,6 +1312,66 @@
>+my @blocked_cc = ();
>+if (Param("strict_isolation")) {

Nit: @blocked_cc is only used in this IF block. You should move this declaration in the IF block to make this clear.


>+    foreach my $pid (keys %cc_add) {
>+        my $user = Bugzilla::User->new($pid);
>+        foreach my $product_id (@newprod_ids) {
>+            if (!$user->can_edit_product($product_id)) {
>+                push (@blocked_cc, $cc_add{$pid});
>+            }
>+        }

As soon as a user in added to @blocked_cc, it doesn't make sense to check the remaining products for this user (this will also avoid duplicated logins).


>+if (defined($prod_id) && Param("strict_isolation")) {
>+    my $sth_cc = $dbh->prepare("SELECT who
>+                                FROM cc
>+                                WHERE bug_id = ?");
>+    my $sth_bug = $dbh->prepare("SELECT assigned_to, qa_contact
>+                                 FROM bugs
>+                                 WHERE bug_id = ?");

You should build a $bug object and then use its methods to access the assignee, QA contact and the CC list.


>+    my $prod_name = get_product_name($prod_id);

Nit: in the same way, you should build a product object instead of calling this soon deprecated function.


>+    foreach my $id (@idlist) {
>+        $sth_cc->execute($id);
>+        while (my ($pid) = $sth_cc->fetchrow_array) {

As you are going to process several bugs which may contain several times the same users, you should cache user objects to avoid to recreate them when we already have them. See how editusers.cgi works ($user_objects{$pid} ||= Bugzilla::User->new(foo)).


>+            my $user = Bugzilla::User->new($pid);
>+            if (!$user->can_edit_product($prod_id)) {
>+                    ThrowUserError('invalid_user_group',
>+                                      {'user'   => $user->login,
>+                                       'bug_id' => $id,
>+                                       'product' => $prod_name});
>+            }

Above, you processed the whole CC list before throwing an error. But here, you throw one immediately. Be consistent. Note that it can be pretty painful to get only one name, eventually remove this user, resubmit again to discover that there is a second user who is not allowed to edit one product, remove this second user, etc..


>+        if (!$assignee_checked) {
>+            my $user = Bugzilla::User->new($assignee);
>+            if (!$user->can_edit_product($prod_id)) {

Nit: what happens if this assignee is the default one for the product we are moving into?


>+        if (!$qacontact_checked && $qacontact) {
>+            my $user = Bugzilla::User->new($qacontact);
>+            if (!$user->can_edit_product($prod_id)) {

Nit: same question for the QA contact. Also, you should only complain if Param('useqacontact') is on (you know, guys such as timeless love to hack the URL and report uninteresting bugs about that).


>@@ -1541,7 +1616,9 @@
>         }
>     }
>     $query .= " where bug_id = $id";
>
>+    my (@added, @removed) = ();

Please leave this line of code at its original place, see below. These declarations have nothing to do here.


>@@ -1618,22 +1695,6 @@
>         while (MoreSQLData()) {
>             $oncc{FetchOneColumn()} = 1;
>         }
>-
>-        my (@added, @removed, @blocked_cc) = ();

Here is its original place.



>Index: template/en/default/global/user-error.html.tmpl

>   [% ELSIF error == "invalid_user_group" %]
>     [% title = "Invalid User Group" %]
>+    User '[% user FILTER html %]' is not able to edit the

As said in my previous review, several users may be passed at once.
Attachment #202863 - Flags: review?(LpSolit)
Attachment #202863 - Flags: review?
Attachment #202863 - Flags: review-
(Assignee)

Comment 10

12 years ago
Created attachment 203224 [details] [diff] [review]
v2


Took care of everything nitless except....

>>>+    $prod_id = undef if (scalar(@newprod_ids) > 1);
>>
>>This change is wrong. If there is only one product returned, $prod_id should be
>>set to this product ID. Here, $prod_id is never defined.

Actually, this is correct.  I added a comment.

>>You should build a $bug object and then use its methods to access the 
>> assignee, QA contact and the CC list.

Unfortunately, the bug object returns login names instead of ids and that would muck up the user object caching.
Attachment #202863 - Attachment is obsolete: true
Attachment #203224 - Flags: review?(LpSolit)
(Reporter)

Comment 11

12 years ago
Comment on attachment 203224 [details] [diff] [review]
v2

>Index: process_bug.cgi

>@@ -164,19 +164,19 @@

>+        if (!CheckCanChangeField($field, $bug->bug_id, 0, 1)) {
>+            $vars->{'privs'} = $PrivilegesRequired;
>+            $vars->{'field'} = $field;
>+            ThrowUserError("illegal_change", $vars);
>+        }

You have to do this check only if $added or $removed is not null. Doing this check in all cases will prevent most users to edit a bug which has dependencies on it.


>@@ -862,12 +863,16 @@

>+    # $prod_id is only defined if all bugs are in a single product.
>+    $prod_id = undef if (scalar(@newprod_ids) > 1);

First of all, your comment is wrong. If all bugs are in the same product (i.e. even with product = --dont_change--), $prod_id should be set.
Secondly, $prod_id = undef if (scalar(@newprod_ids) > 1) is useless as $prod_id is already undefined, and you offer no alternative if scalar(@newprod_ids) == 1.
Thirdly, if you want $prod_id to be defined only if the user explicitly sets $cgi->('product'), you prevent to move bugs into a different component of the same product when the user leave the product field as --dont_change--.


>@@ -1017,10 +1029,19 @@
>     if ($name ne $cgi->param('dontchange')) {
>         $qacontact = DBNameToIdAndCheck($name) if ($name ne "");
>         if (Param("strict_isolation")) {

Only check the QA contact if $name ne ""! if ($qacontact && Param('strict_isolation')) is the right test to do.


>@@ -1290,6 +1319,73 @@

>+        my $user = $usercache{$pid};
>+        foreach my $product_id (@newprod_ids) {
>+            if (!$user->can_edit_product($product_id)) {
>+                push (@blocked_cc, $cc_add{$pid});

Write push(@blocked_cc, $user->login) for consistency.


>+        if (!$qacontact_checked && $qacontact) {
>+            my $user = Bugzilla::User->new($qacontact);

First check if $usercache{$qacontact} exists.



>Index: template/en/default/global/user-error.html.tmpl

>+    not able to edit the
>+    [% IF product %]
>+      '[% product FILTER html %]'
>+    [% END %]
>+     
>+    [% field_descs.product FILTER html %]

Remove  . If you need a whitespace here, write [%+ field_descs.product FILTER html %] instead.
Attachment #203224 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 12

12 years ago
Created attachment 203332 [details] [diff] [review]
v3

OK... this should do it.
Attachment #203224 - Attachment is obsolete: true
Attachment #203332 - Flags: review?(LpSolit)
(Reporter)

Comment 13

12 years ago
Comment on attachment 203332 [details] [diff] [review]
v3

>Index: process_bug.cgi

>@@ -1016,11 +1032,20 @@
>+                        ThrowUserError('invalid_user_group',
>+                                          {'user'   => $qa_user->login,
>+                                           'product' => $product_name,
>+                                           'bug_id' => $cgi->param('id') });

You should write "scalar $cgi->param('id')" in case several bugs are changed at once, in which case this param is undefined. Else the param list is shifted to the left starting from this parameter (don't ask me why). As it's the last param of the list, it doesn't hurt, but it will if we ever add a forth param.

Moreover, passing the 'user' variable is a very bad idea as templates use it to represent the current user ('user' is assumed to be Bugzilla->user in templates). This breaks the footer as user.queries user.in_group('foo') are invalid. As this parameter can contain several users, I suggest to rename it as 'users'. I tried, it works. ;)


>@@ -1082,18 +1107,26 @@
>+                        ThrowUserError('invalid_user_group',
>+                                          {'user'   => $assign_user->login,
>+                                           'product' => $product_name,
>+                                           'bug_id' => $cgi->param('id') });

Same comments here about 'user' and $cgi->param('id').


>@@ -1290,6 +1323,74 @@
>+    foreach my $pid (keys %cc_add) {
>+        $usercache{$pid} ||= Bugzilla::User->new($pid);
>+        my $user = $usercache{$pid};

Nit: you shouldn't use $user which is already set to Bugzilla->user. Maybe $cc_user.


>+    if (scalar(@blocked_cc)) {
>+        ThrowUserError("invalid_user_group", 
>+            {'user' => \@blocked_cc});
>+    }

Nit: you could pass the bug_id (if only one bug is changed) and the product name too (if all bugs are in the same product or only one bug is changed). In the worst case, they are undefined, but the templates know how to behave in this case. ;)

And my comment about 'user' still applies here too.


>+        while (my ($pid) = $sth_cc->fetchrow_array) {
>+            $usercache{$pid} ||= Bugzilla::User->new($pid);
>+            my $user = $usercache{$pid};

Nit: here also, write $cc_user to avoid overwriting $user.


>+        if (!$assignee_checked) {
>+            $usercache{$assignee} ||= Bugzilla::User->new($assignee);
>+            my $user = $usercache{$assignee};

Nit: $assign_user


>+        if (!$qacontact_checked && $qacontact) {
>+            $usercache{$qacontact} ||= Bugzilla::User->new($qacontact);
>+            my $user = $usercache{$qacontact};

Nit: $qa_user


r- to make sure you fix my comments. ;)
Attachment #203332 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 14

12 years ago
Created attachment 203616 [details] [diff] [review]
v4 - nits fixed
Attachment #203332 - Attachment is obsolete: true
Attachment #203616 - Flags: review?(LpSolit)
(Reporter)

Comment 15

12 years ago
Comment on attachment 203616 [details] [diff] [review]
v4 - nits fixed

r=LpSolit

One remark though: when strict_isolation is turned on *after* that some unauthorized users are added to some bugs, you are almost locking your system as process_bug.cgi will detect these users and will complain. The workaround I found is to remove these users during a mass-change, leaving the product field as --dont_change--.

Maybe should we open a separate bug (because this one has to land first; it fixes 99% of the problems we have so far) where process_bug.cgi looks whether we are trying to remove an unauthorized user, and if yes, don't complain that this user is in the bug and that he shouldn't be.
Attachment #203616 - Flags: review?(LpSolit) → review+
(Reporter)

Updated

12 years ago
Flags: approval?
(Reporter)

Comment 16

12 years ago
(In reply to comment #15)
> The workaround I
> found is to remove these users during a mass-change, leaving the product field
> as --dont_change--.

Hum... maybe this is a good thing. This will force you to leave bugs in their original product, remove unauthorized users, and then you can move bugs into a new product. Else if process_bug.cgi allows you to remove an unauthorized user AND to move the bug into a "secret" (or hidden to the user) product at once, then this user will receive an email that he has been removed from, say, the CC list, and will also get the name of this "secret" product, as both changes have been made at the same time.

And if I understand your paranoid 'strict_isolation' parameter correctly, that's the kind of information you wouldn't want to display. So from this point of view, it's not a bug.
Flags: approval? → approval+
(Assignee)

Comment 17

12 years ago

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.297; previous revision: 1.296
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.101; previous revision: 1.100
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.141; previous revision: 1.140
done

Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.