143313 - process_bug.cgi calls CheckCanChangeField() with --do_not_change-- values

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Bob Bowen]

This is not a problem with Mozilla's Bugzilla per se, but it is for those of us
who are customizing it (and maybe someday you folks would be in this situation).

I have added logic to CheckCanChangeField to restrict changing the contents of
certain fields (ones that I have added) to members of groups I have defined.
This worked great until someone tried to use the extended "Change Multiple bugs
at once" capability and they were caught by the membership checks even though
they weren't changing those fields.

This is because the logic for handling multiple bugs calls CheckCanChangeField
(line 842 in process_bug.cgi) with the form value even though the form value is
--do_not_change--. Of course, this differs from what is in the database, so it
falls through to my checks for membership and the change is refused.

I think CheckCanChangeField() (or the call to it) should check for
--do_not_change-- in the field value. I suspect the reason no one else has run
into this is because almost all the fields currently being tested in
CheckCanChangeField happen to not be ones which are set to --do_not_change-- on
the multiple bug form. (The one exception is QA contact, which has its own
special check).

So, thought I would share my experience in hopes it would save someone else
somewhere along the way. Hope this is understandable.

Comment 1

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

Bob, Do you have code fixing this bug or an implementation of your membership
checks that you're willing to contribute back?  I'm suspect some other
installations might also have a use for it.

Comment 2

[Bradley Baetz (:bbaetz)]

You need editbugs to do the changemultiple stuff, though. Have you changed that
on your system?

Comment 3

[Bob Bowen]

Attachment: patch to skip checks for values of --do_not_change--

Here's the fix that makes the most sense to me. It simply considers
--do_not_change-- as the field not changing (as it's not).

BTW, all my users have editbugs privileges. I have a script which runs nightly
and interrogates our LDAP server and automatically creates an entry for each
new user it finds.

Comment 4

[Bradley Baetz (:bbaetz)]

But if they have editbugs, then:

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

should be triggered.

If you've changed your bugzilla installation locally to need more checks for
certain fields, then isn't your patch a local change which you need?

With your patch, can't I change the summary of a bug to --do_not_change-- ?
Actually, maybe not - looking at code, I thikn we do the wrong thing if you try
to change a summary/url/etc to --do_not_change--

Comment 5

[Marc Schumann [:Wurblzap]]

*** Bug 284074 has been marked as a duplicate of this bug. ***

Comment 6

[Niclas]

Our system is not customized in any way. But we are using windows 2003 server
with iis and active state perl. 

I have not tried the patch, because it is targeted for bugzilla 2.15.

Comment 7

[Frédéric Buclin]

*** Bug 345480 has been marked as a duplicate of this bug. ***

Comment 8

[Johan Dahlin]

bug 345480 includes a patch updated for current HEAD

Comment 9

[Frédéric Buclin]

*** Bug 364335 has been marked as a duplicate of this bug. ***

Comment 10

[Johan Dahlin]

Attachment: patch from 364335

Comment 11

[Max Kanat-Alexander]

Comment on attachment 249116 [details] [diff] [review]
patch from 364335

$data can be undef, and $data->{dontchange} can be undef.

Comment 12

[Johan Dahlin]

Attachment: v2

Max: Is something like this what you had in mind?

Comment 13

[Frédéric Buclin]

Comment on attachment 249244 [details] [diff] [review]
v2

>+    } elsif (defined $dontchange && $newvalue eq $dontchange) {
>+        return 1;

As I already explained in b.e.c, this is a big security hole as almost anyone could delete data. $dontchange must be non-empty too.

Comment 14

[timeless]

Attachment: ensure dontchange isn't empty?

Comment 15

[Frédéric Buclin]

Max, unless I miss something, this patch will become useless as soon as bug->update is fully implemented, right? The single place to use $data is in process_bug.cgi, line 866 (assuming I have a not too hacked copy of this file).

Comment 16

[Max Kanat-Alexander]

Yeah, once bug->update is fully implemented, _check_can_change_field should never see --do-not-change--, I'm pretty sure.

Comment 17

[Frédéric Buclin]

OK, so we will wait for the full implementation of bug->update and see if $data went away. If yes, we can close this bug as WFM or WONTFIX.

Comment 18

[Frédéric Buclin]

Attachment: remove $data, v1

check_can_change_field() is no longer called with --do_not_change--

Comment 19

[Max Kanat-Alexander]

Comment on attachment 298051 [details] [diff] [review]
remove $data, v1

Looks good to me. :-)

Comment 20

[Frédéric Buclin]

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