457642 - Users with editbugs privs cannot edit a duplicate anymore if it points to a bug you cannot see

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

I'm trying to CC myself on bug 457543. It's a public bug, but it's marked as a duplicate of a security sensitive bug. When I submit the form, I get the "Duplicate Warning" page that asks me to confirm whether or not the reporter should be added to the other bug's CC list. I'm not doing anything but CCing myself, so it shouldn't be prompting me.

Comment 1

[Frédéric Buclin]

This is a pretty severe regression. I didn't investigate yet which bug regressed this. If you belong to the group the public bug is pointing to, you can ignore the message and CC yourself or do any other change. *But* if you don't belong to that group (which is my case in the example given in comment 0) and you have editbugs privs, you are no longer allowed to edit the public bug *at all*. I just tried CC'ing myself to bug 457543, and b.m.o didn't let me do this.

I can reproduce the bug upstream so this is not due to a local customization. Investigating!

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

The problem was that $self->check($dup_id) was called in all cases, even if the dupe was already here. I changed set_dup_id() to only validate the bug ID, and do nothing if the dupe ID matches $self->dup_id.

I also removed the 2nd argument 'dup_id' passed to check(), which has no meaning for this method.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 340945 [details] [diff] [review]
patch, v1

>Index: Bugzilla/Bug.pm
>-    my $dupe_of_bug = $self->check($dupe_of, 'dup_id');
>+    my $dupe_of_bug = $self->check($dupe_of);

  That argument does have meaning--it makes the error message clearer.

>@@ -1807,12 +1807,17 @@ sub _clear_dup_id { $_[0]->{dup_id} = un
> sub set_dup_id {
>     my ($self, $dup_id) = @_;
>     my $old = $self->dup_id || 0;
>+    # We call new() rather than check() to avoid group restriction
>+    # checks. If the user cannot see the dupe but the dupe was already
>+    # set, that's fine.
>+    my $dupe_of = new Bugzilla::Bug($dup_id);
>+    return if $old == $dupe_of->id;

  This sort of thing should be in the checker, not in set.

Comment 4

[Frédéric Buclin]

(In reply to comment #3)
>   This sort of thing should be in the checker, not in set.

I don't see why. This code was already in set_dup_id(). All I did was to move $dupe_of before return. You still have to return at this point in set_dup_id(), so the code cannot go into _check_dup_id().

Comment 5

[Max Kanat-Alexander]

Well, first off, new() doesn't do any trimming, so spaces will confuse it, in which case your code would die. (In fact, any invalid $dupe_of would now cause the setter to die.)

Also, it's really just more logical to have it in the checker, since we shouldn't be doing any checks if the dupe_of hasn't changed.

Comment 6

[Frédéric Buclin]

Attachment: patch, v2

To avoid creating the same bug object three times in a row, I altered check() a bit to accept either a bug ID or a bug object. If you pass an ID, its behavior is unchanged; if you pass an object, it doesn't try to recreate it and only do visibility checks on it. Also, if the 2nd argument 'dupe_id' is passed, now check() behaves the same way as 'dependson' and 'blocks', i.e. it returns before doing visibility checks, so that we have a chance to test the bug existence separately from visibility checks while still complaining if the bug doesn't exist. And finally, _check_dup_id() returns early if it detects that the dupe is unchanged.

I tested it and it's working fine.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 341114 [details] [diff] [review]
patch, v2

Okay, I have an idea. Instead of letting check take a Bug object, let's create a new Bugzilla::Bug method called $bug->check_is_visible(), which just does the can_see_bug code at the end of check(). Then we can re-use that method inside of the dup_id checker.

Comment 8

[Frédéric Buclin]

Attachment: patch, v3

Implement $bug->check_is_visible.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 343337 [details] [diff] [review]
patch, v3

Okay, this looks great. But why did you remove the "|| 0" at the bottom of the patch, there? Won't that cause undef warnings?

Comment 10

[Frédéric Buclin]

(In reply to comment #9)
> Okay, this looks great. But why did you remove the "|| 0" at the bottom of the
> patch, there? Won't that cause undef warnings?

I removed || 0 because if you come that far in the code, this means the dupe ID exists and is valid. So it can never be undefined. If it's undefined at this point, this means set_dup() didn't do its job correctly as it should throw an error that the bug ID is missing.

Comment 11

[Frédéric Buclin]

Pff, my patch doesn't apply cleanly on 3.2. Here is the checkin for tip:

tip:

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.265; previous revision: 1.264
done

Comment 12

[Frédéric Buclin]

Attachment: patch for 3.2, v1

Here is the backport for 3.2rc1. The idea is exactly the same, except ValidateBugID doesn't return a bug object. So I have to pass the bug ID to check_is_visible() instead of having it as a method.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 343386 [details] [diff] [review]
patch for 3.2, v1

Sure, looks great.

Comment 14

[Frédéric Buclin]

3.2rc1:

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.241.2.15; previous revision: 1.241.2.14
done

Comment 15

[Frédéric Buclin]

Attachment: Selenium script

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.2/
added t/test_dependencies.t
Committed revision 219.

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.0/
added t/test_dependencies.t
Committed revision 206.