753635 - Can't add / remove a local see also if you cannot edit the other bug

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Simon Green]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120424151611

Steps to reproduce:

Red Hat Bugzilla is configured so only selected people have the editbugs group privilege.


Actual results:

When creating a local 'see also' link (i.e. to the same bugzilla), Bugzilla creates a reciprocal 'see also' link in the other bugzilla.

If a reporter (who has the ability to edit their own bug) adds a 'See also' value to a local bug that they did not create, an error will occur because the user does not have the ability to modify the other bug.


Expected results:

The reciprocal link should not be created.

Comment 1

[Simon Green]

Attachment: v1 patch

This is against the current trunk. The same patch will work with 4.2 (with line offsets).

It checks that the user has the privilege to update the other bug. The 'and $removed_bug_url->ref_bug_url' was added because it is now possible for the reciprocal link to be removed (by the reporter for example) before this 'see also' link is removed.

Comment 2

[:glob ✱]

i asked for some clarification from simon on irc..

glob: This is when editing two existing bugs. Joe Bloggs created bug 6, and wants to create a 'see also' link to bug 5. Joe doesn't have the correct access to modify bug 5. What should happen, is the see also link should be created on bug 6, but no receprocal link created on bug 5.

Comment 3

[Frédéric Buclin]

How do we do this for dependencies? We should be consistent.

Comment 4

[Simon Green]

(In reply to Frédéric Buclin from comment #3)
> How do we do this for dependencies? We should be consistent.

For dependencies, you can create a dependency to a bug that you do not have editbugs privilege on (but not to one that you cannot view). If we were to do the same for the See Also field, that would require us to escalate the user to create the See Also link pointing the other way, and I would assume that is not ideal.

An alternate option to do it directly in SQL, but I assume that would be frowned upon.

Can you let me know what you think is the best solution?

  -- simon

Comment 5

[Frédéric Buclin]

OK, let's do it this way:

1) if the user cannot see the other bug, reject the change entirely
2) if the user can see the other bug but cannot edit it, just add the reference on the currently edited bug (i.e. there will be no reciprocal link)
3) if the user can see and edit both bugs, add the reciprocal reference to both bugs.

Comment 6

[Simon Green]

(In reply to Frédéric Buclin from comment #5)
> OK, let's do it this way:

Based on this, my patch is still valid, and does as expected in this case. Have asked for a additional review on it.

Comment 7

[:glob ✱]

Comment on attachment 622622 [details] [diff] [review]
v1 patch

>=== modified file 'Bugzilla/Bug.pm'
>--- Bugzilla/Bug.pm	2012-03-27 22:27:41 +0000
>+++ Bugzilla/Bug.pm	2012-05-10 03:36:20 +0000
>@@ -2841,7 +2841,8 @@
>         # ref bug id for sending changes email.
>         my $ref_bug = delete $field_values->{ref_bug};
>         if ($class->isa('Bugzilla::BugUrl::Bugzilla::Local')
>-            and !$skip_recursion)
>+            and !$skip_recursion
>+            and $ref_bug->check_can_change_field('see_also', '', $self->id, \$privs))
>         {
>             $ref_bug->add_see_also($self->id, 'skip_recursion');
>             push @{ $self->{_update_ref_bugs} }, $ref_bug;
>@@ -2873,12 +2874,15 @@
>     # we need to notify changes for that bug too.
>     $removed_bug_url = $removed_bug_url->[0];
>     if (!$skip_recursion and $removed_bug_url
>-        and $removed_bug_url->isa('Bugzilla::BugUrl::Bugzilla::Local'))
>+        and $removed_bug_url->isa('Bugzilla::BugUrl::Bugzilla::Local')
>+        and $removed_bug_url->ref_bug_url)
>     {
>         my $ref_bug
>             = Bugzilla::Bug->check($removed_bug_url->ref_bug_url->bug_id);
> 
>-        if (Bugzilla->user->can_edit_product($ref_bug->product_id)) {
>+        if (Bugzilla->user->can_edit_product($ref_bug->product_id)
>+            and $ref_bug->check_can_change_field('see_also', $self->id, '', \$privs))
>+        {
>             my $self_url = $removed_bug_url->local_uri($self->id);
>             $ref_bug->remove_see_also($self_url, 'skip_recursion');
>             push @{ $self->{_update_ref_bugs} }, $ref_bug;
>

Comment 8

[:glob ✱]

sorry, i don't know what happened to my review comment :(

with your patch, this:

> if the user can see the other bug but cannot edit it, just add the reference on
> the currently edited bug (i.e. there will be no reciprocal link)

results in a link being added to the referenced bug as well as the currently edited bug.

Comment 9

[Simon Green]

Comment on attachment 622622 [details] [diff] [review]
v1 patch

okay. I'm going to need some help on this, since I cannot replicate it myself (on my test instance of Bugzilla trunk).

From the patch:
          if ($class->isa('Bugzilla::BugUrl::Bugzilla::Local')
-            and !$skip_recursion)
+            and !$skip_recursion
+            and $ref_bug->check_can_change_field('see_also', '', $self->id, \$privs))
         {
             $ref_bug->add_see_also($self->id, 'skip_recursion');

So the change I am making is that I am not updating the see_also value on the referenced bug if you cannot edit the referenced bug. If you tried and do so now, you would get a red error screen saying that you do not have permission to change the see_also value.

Assuming that the above patch is correct, it makes me wonder what is different between our test setups that means you are able to edit the see also value on a bug you do not have the ability to edit (which in theory could be a security issue, and definitely not something that upstream 4.2 support currently). Are you absolutely sure you don't have the editbugs privilege on the referenced bug (or be the reporter, assignee or qe contact)? I know I asked this in IRC, but I cannot see a problem with my code.

Comment 10

[:glob ✱]

Comment on attachment 622622 [details] [diff] [review]
v1 patch

r=glob

sorry simon, i don't know where i went wrong with my testing last time, but i just redid my tests and this looks good.

Comment 11

[Simon Green]

Asking for a4.2? because it is a bug in 4.2 (and earlier versions) that I think should be fixed.

Comment 12

[Frédéric Buclin]

Just to be sure, does it work with the CANEDIT bit of group settings correctly?

Comment 13

[Simon Green]

(In reply to Frédéric Buclin from comment #12)
> Just to be sure, does it work with the CANEDIT bit of group settings
> correctly?

Yes. I have tested it on my upstream-based installation. I'm not sure if you want glob to test it too.

  -- simon

Comment 14

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Bug.pm
Committed revision 8418.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified Bugzilla/Bug.pm
Committed revision 8412.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified Bugzilla/Bug.pm
Committed revision 8147.