286160 - possible invalid flag types when moving a bug to a different product

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Frédéric Buclin]

When a reporter without editbugs privs move his bug to a different product where
some of the flags set in this bug do not apply, these flags are not removed as
process_bug.cgi checks if the user has rights to edit flags. This way, it is
possible to have flag types in some products where they do not apply!

Do we really want this behavior? What if the grant group is defined for these
flag types and the reporter is not in it? What takes precedence? See also bug
170019.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

My initial reaction would be to deny the move since the user does not have
privileges to do everything that the move entails.

Comment 2

[Frédéric Buclin]

OK, the culprit in process_bug.cgi is here:

    # Set and update flags.
    if ($UserInEditGroupSet) {
        die $UserInEditGroupSet;
        my $target = Bugzilla::Flag::GetTarget($id);
        Bugzilla::Flag::process($target, $timestamp, $cgi);
    }

$UserInEditGroupSet is set to -1 by default and is only changed by
CheckCanChangeField:

    if ($UserInEditGroupSet < 0) {
        $UserInEditGroupSet = UserInGroup("editbugs");
    }

So if a user does not change any field except flags, $UserInEditGroupSet = -1
and this user can change flags (if validate() agrees) even without any privs.
*But* if a reporter without privs changes a field he is allowed to change, then
$UserInEditGroupSet = 0 and flag changes won't be considered. How bad!

The fix is trivial: allow any user to change flags as long as
Flag(Type)::validate() says you can.

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

Comment 4

[Max Kanat-Alexander]

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

OK. Conversations with LpSolit make this seem to me like the right thing to do.
Having the check be atomic inside process() sounds much better, anyhow.

Comment 5

[Frédéric Buclin]

Attachment: 2.18 backport, v1

Comment 6

[Max Kanat-Alexander]

Comment on attachment 182745 [details] [diff] [review]
2.18 backport, v1

That looks the same to me. Your security code is also in 2.18, right?

Comment 7

[Frédéric Buclin]

Tip:

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.253; previous revision: 1.252
done


2.18:

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.205.2.18; previous revision: 1.205.2.17
done