Cannot change status and reassign at the same time if the assignee/QA contact doesn't have editbugs privs

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
Creating/Changing Bugs
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Nick Z Liu, Assigned: Frédéric Buclin)

Tracking

Bugzilla 3.2
Bug Flags:
approval +
approval3.2 +
blocking3.2 +
testcase ?

Details

Attachments

(1 attachment, 1 obsolete attachment)

2.12 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0; Maxthon; InfoPath.1)
Build Identifier: bugzilla-3.2rc1

The user without any privileges want to change status and reassign at the same time. If do these two together, it failed. If change status first and then reassign, it's ok.

Reproducible: Always

Steps to Reproduce:
1. User A report a bug and assign to B
2. B change status to resolved and reassign to C for verifying
Actual Results:  
Show "You tried to change the Status field from RESOLVED to ASSIGNED , but only the assignee or reporter of the bug, or a user with the required permissions may change that field. " with red background.

Expected Results:  
It should be ok

It looks like the right of change assignee field was checked first and if it passed, the new value of assignee will take effect. Then the user is no longer the owner of this bug and can't change any other field again.
(Assignee)

Comment 1

9 years ago
Yes, I can reproduce the bug.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking3.2?
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: --- → Bugzilla 3.2
Version: unspecified → 3.2
(Assignee)

Comment 2

9 years ago
There are two ways to fix the problem:

1) Either force process_bug.cgi to set the assignee and QA contact as the very last steps before committing changes, i.e by moving these lines much later in the code:

push(@set_fields, 'assigned_to') if !$cgi->param('set_default_assignee');
push(@set_fields, 'qa_contact')  if !$cgi->param('set_default_qa_contact');

    $b->reset_assigned_to if $cgi->param('set_default_assignee');
    $b->reset_qa_contact  if $cgi->param('set_default_qa_contact');

As soon as Bug->set_all() is fully implemented, this won't be an issue anymore if we keep this order in mind.

2) Or let Bug->check_can_change_field look at the old assignee and QA contact, i.e. that $bug->set_assigned_to and $bug->set_qa_contact should move the old assignee/QA contact into $self->{'old_assigned_to'} and $self->{'old_qa_contact'} so that check_can_change_field() can do

if ($self->{'assigned_to'} == $user->id
   || $self->{'old_assigned_to'} && $self->{'old_assigned_to'} == $user->id)

instead of the current if ($self->{'assigned_to'} == $user->id) check. Same for the QA contact.

Max, which solution do you prefer?
Summary: Cannot change status and reassign at the same time → Cannot change status and reassign at the same time if the assignee/QA contact doesn't have editbugs privs
(Assignee)

Comment 3

9 years ago
OK, after thinking about this a little bit more, I think we should go with 1) for two reasons. First because set_all() will fix this problem for us (again, if we keep this order in mind), and secondly because if for some unknown reason the bug object remains in live for a long time, the old assignee/QA contact could perform some unexpected actions because check_can_change_field() would still see $self->{'old_assigned_to'} and $self->{'old_qa_contact'}, and accept changed made by them despite their privileges should now be revoked (to prevent that, $bug->update should delete these two fields for us when it's done, but that's probably less safe than 1) anyway).

Comment 4

9 years ago
No, as I recall, set_assignee and set_qa_contact have to be exactly where they are.

I think the best thing is to fix check_can_change_field to look at both, which will work no matter where the setters are.
Flags: blocking3.2? → blocking3.2+

Comment 5

9 years ago
Also, what you can do to fix the problem you mentioned is delete old_qa_contact and old_assignee when you do update(). I think having those around would actually help anyhow, because then we wouldn't have to convert logins to ids when doing LogActivityEntry, we could just pull the data out of the old objects.

They should of course be _old_assignee and _old_qa_contact (with the leading underscores), because they're internal variables only.
(Assignee)

Comment 6

9 years ago
(In reply to comment #4)
> No, as I recall, set_assignee and set_qa_contact have to be exactly where they
> are.

I really doubt there was a reason for them. There was one for products, but I think that's all.

Comment 7

9 years ago
No, I'm pretty sure there's a reason. In any case, the check_can_change_field option is a better option because it allows the methods to be more flexible when being used in other places, too.
(Assignee)

Updated

9 years ago
Assignee: create-and-change → LpSolit
Status: NEW → ASSIGNED
(Assignee)

Comment 8

9 years ago
Created attachment 345216 [details] [diff] [review]
patch, v1
Attachment #345216 - Flags: review?(mkanat)

Updated

9 years ago
Attachment #345216 - Flags: review?(mkanat) → review+

Comment 9

9 years ago
Comment on attachment 345216 [details] [diff] [review]
patch, v1

This looks fine, but just store the whole object. Then we can re-use it in update()'s LogActivityEntry sections if we want to.

Updated

9 years ago
Flags: approval3.2+
Flags: approval+
(Assignee)

Comment 10

9 years ago
(In reply to comment #9)
> (From update of attachment 345216 [details] [diff] [review])
> This looks fine, but just store the whole object.

I would prefer not to. ->{assigned_to} contains an ID, so I prefer ->{_old_assigned_to} to also contain an ID, for consistency. You can implement ->{_old_assigned_to_obj} later if you want, which would be consistent with the existing ->{assigned_to_obj}.
(Assignee)

Comment 11

9 years ago
Created attachment 345392 [details] [diff] [review]
patch, v1.1

Now store the user object instead of the user ID.
Attachment #345216 - Attachment is obsolete: true
Attachment #345392 - Flags: review?(mkanat)

Updated

9 years ago
Attachment #345392 - Flags: review?(mkanat) → review-

Comment 12

9 years ago
Comment on attachment 345392 [details] [diff] [review]
patch, v1.1

This is fine, except it's possible that assigned_to_obj is undef at the point when you delete it, right?
(Assignee)

Comment 13

9 years ago
Comment on attachment 345392 [details] [diff] [review]
patch, v1.1

Ah yes, you are right. In this case, it doesn't make sense to force a call to $bug->assigned_to to get the user object. I will check in my first patch and let someone else store the user object in a separate bug when it's needed.
Attachment #345392 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #345216 - Attachment is obsolete: false
(Assignee)

Comment 14

9 years ago
tip:

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

3.2rc1:

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.241.2.17; previous revision: 1.241.2.16
done
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Flags: testcase?
You need to log in before you can comment on or make changes to this bug.