465606 - If a user doesn't have access to change target milestone, it gets set back to default.

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Rob Siklos [:robzilla]]

In check_can_change_field() in Bug.pm, I added some code near the top so that only users with "editcomponents" privs on a product can change the target milestone:

    if ($field eq 'target_milestone') {
        $$PrivilegesRequired = 3;
        return $user->in_group('editcomponents', $self->{'product_id'})
    }

Now, if a user who does not have this privilege saves any changes to a bug, the target milestone for that bug gets reset back to the default value (---).

I'm not sure if this is a regression or if it's always been like this, but I think I'm doing something supported, and the behaviour seems obviously broken (to me).

Comment 1

[Max Kanat-Alexander]

I'm assuming this is 3.2rc2, yes?

Comment 2

[Frédéric Buclin]

I can reproduce the problem. I have a bug with TM = 1.3, and doing nothing besides clicking "Commit", I get:

You tried to change the Target Milestone field from ---  to 1.3  , but only a user with the required permissions may change that field. 


As I don't try to edit the TM, and it was already set to 1.3, I shouldn't get this error.

Comment 3

[Frédéric Buclin]

About the testcase flag, we will only implement a script if it's doable without altering check_can_change_field().

Comment 4

[Frédéric Buclin]

First, the good news: the problem is specific to milestones. Other fields are not affected by this bug.

Now the bad news: the error comes from set_product(). This method has the following test:

    if ($product_changed && Bugzilla->usage_mode == USAGE_MODE_BROWSER) {

As you don't edit the product, $product_changed is false and so we jump to:

    else {
        ....
        if ($self->check_can_change_field('target_milestone', 0, 1)) {
            $self->set_target_milestone($tm_name);
        }
        else {
            # Have to set this directly to bypass the validators.
            $self->{target_milestone} = $product->default_milestone;
        }
    }

So the last check is wrong. If you don't edit the milestone AND you don't have privs to edit it anyway, it falls back to the default milestone instead of first making sure the current milestone is valid (which is probably the case as the bug is still in the same product).

Note that you can only trigger this problem if you edit check_can_change_field() to use different privs to edit the product and the target milestone, which explains why our Selenium scripts do not catch this issue -> severity 'normal' rather than 'major'. Still a blocker, though.

Comment 5

[Frédéric Buclin]

Attachment: patch, v1

If the product changed and you are not allowed to choose the milestone, use the default one. In all other cases, check the given milestone passed by the user. If the user is not allowed to select it, 'undef' is passed to set_product() which automatically sets $tm_name to the current milestone, meaning that this test will pass, as desired. So all 8 combinations (web/API + product changed/unchanged + milestone edition allowed/forbidden) work as expected now.

Comment 6

[Max Kanat-Alexander]

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

That looks right to me.

Comment 7

[Frédéric Buclin]

tip:

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

3.2rc2:

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