266579 - Users without privs can confirm bugs by assigning to themselves first, without having canconfirm privs

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Frédéric Buclin]

Any reporter is able to create a bug and assign it to itself. As the owner, it
is then able to confirm that bug. It can then reassign the bug to the default
owner and you get the same result as if the user had canconfirm privs!

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

Look if the user has canconfirm or editbugs privs. If not, take the default
assignee, without any warn.

Comment 2

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

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

I think we want users to be able to assign to themselves, we just don't want
them to be able to confirm afterwards.

Comment 3

[Frédéric Buclin]

(In reply to comment #2)
> (From update of attachment 163772 [details] [diff] [review])
> I think we want users to be able to assign to themselves, we just don't want
> them to be able to confirm afterwards.
> 

What about a user (with no priv at all) having two accounts? You create a bug
and assign it to your second account. As this bug is not yours (well, Bugzilla
does know it is), you are free to confirm your bug.

Another possibility is to first assign the bug to you when creating a new bug
and then reassign it to your second account, so that you can again confirm.

Either we definitely forbid a user with no priv to assign a bug to anybody
except the default assignee, as suggested by my patch, or we need to remove the
capacity for owners and QA contacts to confirm bugs if they haven't canconfirm
privs. But what myk and justdave told me on IRC is that they don't want this
last solution.

So what? If a user wants to fix a bug he reported, he is free to submit a patch
to this bug which is assigned to the default assignee and request for review.
This assignee, or any user with editbugs privs, can then reassign this bug to
the reporter if there is a good reason to do so.

Comment 4

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

>Either we definitely forbid a user with no priv to assign a bug to anybody
>except the default assignee, as suggested by my patch, or we need to remove the
>capacity for owners and QA contacts to confirm bugs if they haven't canconfirm
>privs. But what myk and justdave told me on IRC is that they don't want this
>last solution.

I don't know what Dave told you, but what I told you on IRC is that I *do* want
the last solution.  Users without canconfirm should not be able to confirm bugs,
even if they are the assignee or QA contact for them.

Comment 5

[Frédéric Buclin]

Attachment: patch, v2: need canconfirm privs to confirm bugs, even for owners and QA people

Following myk's last comment, owners and QA people need canconfirm privs to
confirm bugs.

Comment 6

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

Comment on attachment 163818 [details] [diff] [review]
patch, v2: need canconfirm privs to confirm bugs, even for owners and QA people

>+    if (($field eq "canconfirm") || (($field eq "bug_status") && 
>+        ($oldvalue eq $::unconfirmedstate) && IsOpenedState($newvalue))) {

When hacking old code, please update its style per the Bugzilla style guide by
breaking long lines with multiple conditionals into one line per conditional,
breaking before boolean operators, and lining up the opening curly bracket with
"if" keyword, i.e.:

    if (($field eq "canconfirm")
	|| (($field eq "bug_status")
	    && ($oldvalue eq $::unconfirmedstate)
	    && IsOpenedState($newvalue)))
    {


Nit: remove unnecessary parentheses around '$field eq "canconfirm"', '$field eq
"bug_status"', and '$oldvalue eq $::unconfirmedstate'.


>+        if ($UserInCanConfirmGroupSet) {
>             return 1;
>+        } else {
>+        # This restriction also applies to owners and QA people without privs
>+            return 0;

Since you're returning a boolean, and $UserInCanConfirmGroupset is a boolean,
just return its value, i.e.:

return $UserInCanConfirmGroupSet;

Comment 7

[Dave Miller [:justdave]]

(In reply to comment #4)
> I don't know what Dave told you, but what I told you on IRC is that I *do* want
> the last solution.  Users without canconfirm should not be able to confirm bugs,
> even if they are the assignee or QA contact for them.

I believe I said the same...

Comment 8

[Frédéric Buclin]

Attachment: Better patch + hide useless UI

justdave, myk, sorry! You were right, you never told me to let owners without
canconfirm privs to confirm bugs. I misread a comment or two (I read my log
file again). ;)

This patch is more complete than the previous one and hides unnecessary UI for
users without canconfirm privs. I also clean up CheckCanChangeField a bit which
allows me to give a better error message to the user.

Not sure if everybody will like this...

Comment 9

[Frédéric Buclin]

Comment on attachment 164336 [details] [diff] [review]
Better patch + hide useless UI

asking gerv for review as he knows CheckCanChangeField by heart!

Comment 10

[Frédéric Buclin]

Comment on attachment 164336 [details] [diff] [review]
Better patch + hide useless UI

requesting kiko's review as gerv seems to be away for a long time.

kiko, first real step to clean process_bug.cgi. :)

Comment 11

[Frédéric Buclin]

I think it is worth considering for 2.20. Do we want it for 2.18 too? (I guess not)

Comment 12

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

Comment on attachment 164336 [details] [diff] [review]
Better patch + hide useless UI

>Index: mozilla/webtools/bugzilla/process_bug.cgi

>+    # If the user isn't allowed to change the field, we must give him an
>+    # adequate reason. The error message depends on $PrivilegesRequired:
>+    # $PrivilegesRequired = 0 : need to be either the reporter, the owner,
>+    # or an empowered user;
>+    # $PrivilegesRequired = 1 : need to be either the owner or an
>+    # empowered user;
>+    # $PrivilegesRequired = 2 : need to be an empowered user.

I think it would make sense to reserve 0 (zero) for the meaning "no privileges
required", even if we never check for that value, since zero evaluates to
"false" in boolean context, thus giving the variable some additional
intuitiveness: if set to zero, no privileges are required; if set to a positive
value, some privileges are required.


>+    # If we haven't returned by this point and the user isn't the
>+    # reporter of the bug, then it doesn't have the necessary
>+    # permissions to change this field.

Nit: it -> he/she/they


>+    $PrivilegesRequired = 0;
>+    return 0 unless ($reporterid eq $whoid);
>+
>     # The reporter's a more complicated case...
>-    if ($reporterid eq $whoid) {
  ...
>+    $PrivilegesRequired = 1;

This change doesn't make much sense to me. It makes the way we check reporter
privileges inconsistent with how we check assignee and QA contact privileges. 
Currently we do:

if user is the assignee... permission granted
if user is the QA contact... permission granted
if user is the reporter... permission granted or denied depending on further
checks
otherwise... permission denied

Now we're doing:

if the user is the assignee... permission granted
if the user is the QA contact... permission granted
if the user isn't the reporter... permission denied
otherwise... permission granted or denied depending on further checks

Of these two approaches, the first one makes much more intuitive sense, since
each role-related check uses the same kind of conditional, and we make a
decision for each role before we make one for a user not in any role.  Why was
this change made?


>Index: mozilla/webtools/bugzilla/Bugzilla/Bug.pm

>+    my $poweruser = !Bugzilla->user->id
>+                    || Bugzilla->user->in_group("editbugs");
>+    my $privileged = $poweruser

Nit: "poweruser" doesn't describe the state it captures, which is actually a
situation in which we don't know whether the user has privileges or not. 
something like "unknown" or "privileges_unknown" would be better here.


>Index: mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl

>+    but only [% IF privs < 2 %] the owner [% IF !privs %] or 
>+    submitter [% END %] of the [% terms.bug %], or [% END %] a 
>+    sufficiently empowered user[% IF privs < 2 %],[% END %] may 
>+    change that field.

Nit: this code is difficult to parse.  Perhaps there's a way to write it that
makes more immediate sense upon first inspection?

Comment 13

[Frédéric Buclin]

Attachment: Better patch + hide useless UI, v2

This bug is blocked by bug 193125 (Reassign bug forces attempt to change to NEW
even when not specified). This is because we explicitly remove the ability to
the owner and the QA contact to confirm bugs if they haven't canconfirm privs.

More precisely, we remove the ability to ANY user without canconfirm or
editbugs privs to confirm bugs. But what Bugzilla does when someone tries to
reassign an unconfirmed bug is to (virtually) change the status to "NEW" so
that the reporter without privs cannot reassign his bugs, as expected, but this
trick now also prevents the owner and the QA contact without privs from
reassigning their bugs, which is not wanted.

process_bug.cgi really needs some cleanup here, and this starts with bug
193125!

Comment 14

[Frédéric Buclin]

Comment on attachment 166050 [details] [diff] [review]
Better patch + hide useless UI, v2

oops! Wrong patch! Sorry!

Comment 15

[Frédéric Buclin]

Attachment: Better patch + hide useless UI, v2

My comment 13 is still valid, now with the correct patch. Sorry for spam.

Comment 16

[Frédéric Buclin]

Comment on attachment 166053 [details] [diff] [review]
Better patch + hide useless UI, v2

This patch can be reviewed as is, but bug 193125 should be fixed first.

Comment 17

[Frédéric Buclin]

Comment on attachment 166053 [details] [diff] [review]
Better patch + hide useless UI, v2

I finally decided to write a single patch for both this bug and bug 193125,
making this patch obsolete.

The new patch is written and first tests are good. I'll wait a day or two
before submitting.

Comment 18

[Frédéric Buclin]

Attachment: Complete patch, v1

Successfully tested on my installation. checksetup.pl and runtests.sh are both
happy. I'm also!

This patch does 3 main things:
1) Disallow users without canconfirm privs to confirm bugs, even if they are
the assignee or QA contact.

2) Correctly handles reassignment. Now the bug status is not used anymore to
determine if a user is allowed to reassign or not, which is the reason why bug
193125 happens! That means this patch also solve bug 193125. I couldn't
dissociate both patches for subtle reasons, so I decided to write a single one.
The subtle reason is that removing the ability to the assignee and QA contact
to confirm bugs also prevents them to reassign unconfirmed bugs. That the
reason I need to consider both bugs in a single patch.

3) Alter the UI so that *only* authorized actions are available, including
confirming and reassigning bugs. Removing useless UI will also decrease the
number of error messages a user can get!

Comment 19

[Frédéric Buclin]

Comment on attachment 167127 [details] [diff] [review]
Complete patch, v1

myk, this patch will correct what I call a "security hole"!

Comment 20

[Frédéric Buclin]

Comment on attachment 167127 [details] [diff] [review]
Complete patch, v1

myk asked me to find a second reviewer because of potential changes to the
security stuff. So asking peshkin and jouni as well.

Comment 21

[Jouni Heikniemi]

Comment on attachment 167127 [details] [diff] [review]
Complete patch, v1

It strikes me as odd that the reporter cannot reassign the bug, even if he
initially had the permission to set the assignee in post_bug. This is counter
to the logic we have for the priority field for example. Also, the ability of
marking bugs fixed but not reassigning them is illogical, and perhaps will
cause reporters to INVALIDate and re-submit bugs with the accidentally
wrongly-set assignee. I can't find the grounds for this decision in this bug;
could someone please explain?

Btw, right now it looks like I'm not going to give an r+ for this patch even if
it satisfied me completely. I can test it and say when I'm happy, but I don't
know (well actually, remember) process_bug.cgi's security validation logic well
enough to bless a security-related patch of this magnitude. If you're in a
hurry, you might want to push the other reviewers for this. Of course, if we'll
end up going over several iterations, I'll probably be forced to revitalize my
knowledge anyway ;-) 


>Index: mozilla/webtools/bugzilla/process_bug.cgi
>===================================================================

>+    # If the user isn't allowed to change some field, we must tell
>+    # him who can, based on $PrivilegesRequired.

Nit: "change _a_ field"

Nit: I'd replace "based on $PrivilegedRequired" by something more verbose.
Perhaps: "We store the required permission set into the $PrivilegedRequired
variable, from where the information gets pulled to the error template."

>+        # The reporter may not:
>+        # - reassign bugs (if bugs are not assigned to him)

Nit: Perhaps "unless the bugs are assigned to him; in that case we will have
already returned 1 above when checking for the assignee field.


>Index: mozilla/webtools/bugzilla/Bugzilla/Bug.pm
>===================================================================

>+    my $unknown = !Bugzilla->user->id
>+                  || Bugzilla->user->in_group("editbugs");

... What on earth are the semantics of this variable?


>Index: mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl
>===================================================================

>+      [% IF privs < 2 %] or submitter [% END %]

"reporter" is more like our standard terminology.

Comment 22

[Frédéric Buclin]

(In reply to comment #21)
> (From update of attachment 167127 [details] [diff] [review])
> It strikes me as odd that the reporter cannot reassign the bug, even if he
> initially had the permission to set the assignee in post_bug. This is counter
> to the logic we have for the priority field for example.

Following my discussion with myk on IRC about the question of reassignment, here
is his answer:

From IRC:
<myk> the reporter may have some rights that users in general don't have, but i
don't think that should include being able to decide who gets assigned to a bug


And I totally agree with him. Changing the priority is nothing serious, but
changing the assignee of a bug has other implications. One is described in bug
122480: if a bug is private and as the reporter you reassign this bug, the
actual assignee may be thrown out of the bug and cannot access it anymore. Bad!
Another example is that (maybe) you don't want the reporter to change something
as soon as the bug is assigned to the appropriate hacker, see e.g. bug 90619.
Another point is described in bug 231594 about who is allowed to reassign bugs.

jouni, I'm sure you don't want me to reassign your bugs if you are working on
them. I have editbugs privs, so I could do that. *But* I have editbugs privs
*because* myk trusted me and has seen my work. That also means I won't reassign
your bugs!! ;) I don't think reporters should have such privileges "by default".
Should they?


Also, the ability of
> marking bugs fixed but not reassigning them is illogical, and perhaps will
> cause reporters to INVALIDate and re-submit bugs with the accidentally
> wrongly-set assignee. I can't find the grounds for this decision in this bug;
> could someone please explain?

We let reporters to close there own bugs because some users *first* submit bugs,
*then* realize they are wrong (or less frequently because the problem is already
solved in an updated version) and we let them to close the bug. Bug 90619 goes
in this direction, that is to forbid reporters to close there own bugs as soon
as they are confirmed (well, this is actually only a request I made). This is
another question and please comment there about that question.

Moreover, I think if you submit a bug, it's because you would like to see it
being fixed. There is no reason to see the reporter to close his own bugs if
they aren't fixed!

Well, that's only my point of view and justdave or myk could have another one.

> 
> Btw, right now it looks like I'm not going to give an r+ for this patch even if
> it satisfied me completely. I can test it and say when I'm happy, but I don't
> know (well actually, remember) process_bug.cgi's security validation logic well
> enough to bless a security-related patch of this magnitude.

Bad! Who are the experts about security then? myk and peshkin only? Is your
review- only because you aren't confident enough about security stuff or because
my patch doesn't do the job correctly (except your nits)?


If you're in a
> hurry, you might want to push the other reviewers for this. Of course, if we'll
> end up going over several iterations, I'll probably be forced to revitalize my
> knowledge anyway ;-) 

Hummm....


> >Index: mozilla/webtools/bugzilla/Bugzilla/Bug.pm
> >===================================================================
> 
> >+    my $unknown = !Bugzilla->user->id
> >+                  || Bugzilla->user->in_group("editbugs");
> 
> ... What on earth are the semantics of this variable?

Please read myk's comment 12! That's his idea! :)



> >Index: mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl
> >===================================================================
> 
> >+      [% IF privs < 2 %] or submitter [% END %]
> 
> "reporter" is more like our standard terminology.
> 

"Submitter" is already here for a long time. I consider this point as a nit! ;)

Comment 23

[Jouni Heikniemi]

> jouni, I'm sure you don't want me to reassign your bugs if you are working on
> them. I have editbugs privs, so I could do that. *But* I have editbugs privs
> *because* myk trusted me and has seen my work. That also means I won't 
> reassign your bugs!! ;) 

I'm almost sure you didn't honestly understand my comment as taking a stance in
that issue. Materially, I agree that being the bug reporter shouldn't
automatically grant you excessive power over the bug. From an system integrity /
usability standpoint, I find it odd that the user can choose the assignee at one
point (enter_bug/post_bug) but not another (show_bug/process_bug). 

If the change to reporters reassignment ability is made, we should re-evaluate
the role of the assignee input field on enter_bug just as well. So far I haven't
seen a stance taken on that issue. Myk being the UI czar here, we'll naturally
be taking his decision - but I'd much like to see it worded out here. I'm fine
with removing the assignee field from enter_bug for non-editbugs users, but it's
naturally a considerable change in the way Bugzilla operates by default.

> Moreover, I think if you submit a bug, it's because you would like to see it
> being fixed. There is no reason to see the reporter to close his own bugs if
> they aren't fixed!

You could defend the ability to reassign with similar arguments - based on the
thought that doing illegit reassignments would only hamper the proper fixing of
the bug. You can't row in two directions here; either you trust the reporter or
you don't. This doesn't mean the combination of "no reassignment but still can
resolve" wouldn't be possible - it's just that the argumentation behind this
sort of changes needs to be solid and withstand later observation. 

> Is your review- only because you aren't confident enough about security 
> stuff or because my patch doesn't do the job correctly (except your nits)?

It's because I'm not convinced the current solution produces a working and
understandable relation between enter_bug and show_bug. The rest of my comments
are mostly nits, and if everybody else thinks the reporter/reassignment dilemma
is fine as per your patch, I'll remove my r-.

> > >+    my $unknown = !Bugzilla->user->id
> > >+                  || Bugzilla->user->in_group("editbugs");
> > ... What on earth are the semantics of this variable?
> Please read myk's comment 12! That's his idea! :)

Even if the idea came from Duke of Hell himself, it's not readable.
"unknown_privileges" is better, but either of them needs a descriptive comment
to explain the concept of "If we don't know who the user is, we assume he can do
everything" (another assumption of Bugzilla I'd be ready to get rid of).

> > >+      [% IF privs < 2 %] or submitter [% END %]
> > "reporter" is more like our standard terminology.
> "Submitter" is already here for a long time. I consider this point 
> as a nit! ;)

History isn't a particularly good defense for bad wording choices - unless the
change for the better would result in unacceptable confusion or lack of
uniformity in a given context.

Comment 24

[Dave Miller [:justdave]]

(In reply to comment #23)
> "If we don't know who the user is, we assume he can do
> everything" (another assumption of Bugzilla I'd be ready to get rid of).

Hear hear! :)

Comment 25

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

>If the change to reporters reassignment ability is made, we should re-evaluate
>the role of the assignee input field on enter_bug just as well.

Agreed, and this is an important issue raised by the fix for this bug.  The
solution is, as you noted, to forbid reporters from assigning bugs upon entry
that they can't reassign afterwards, and to grant both assignment and
reassignment privileges to users who need either, since there's no functional
difference between the two that would compel us to grant privileges to each
separately.

>Even if the idea came from Duke of Hell himself

You rang? ;-)

Comment 26

[Frédéric Buclin]

Attachment: Complete patch, v2

Now prevents the user without editbugs privs from assigning the newly created
bug in enter_bug.cgi, as requested by myk. The "assign to" field is disabled
for these users. Hope the patch is complete now.

Comment 27

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

Comment on attachment 168257 [details] [diff] [review]
Complete patch, v2

># him who can. We store the required permission set into the
># $PrivilegesRequired variable, from where the information
># gets pulled to the error template.

Nit: from where the information gets pulled -> which gets passed


># The reporter's a more complicated case...

Nit: -> The reporter is a more complicated case...


>-               ThrowUserError("still_unresolved_bugs", 
>+               ThrowUserError("still_unresolved_bugs",

Are there any actual changes here, or is this just whitespace?


>  [% IF bug.isopened && bug.bug_status != "ASSIGNED" && bug.user.canedit %]
>    [% IF !bug.isunconfirmed || bug.user.canconfirm %]

Nit: combine these into a single conditional.


>$::FORM{'qa_contact'} = $id;
...
>$::FORM{'assigned_to'} = $newid;
...
>$::FORM{'qa_contact'} = $qacontact;

We should avoid hacking form field values in general,
preferring instead to create new variables containing
the modified values.  That way a programmer who modifies
a different part of this file in the future can be sure
that the value of FORM keys is whatever the user entered
and not possibly something that another part of the code
has modified.

What are these being used for?

Except for that last issue, this patch looks good.

Comment 28

[Dave Miller [:justdave]]

I'll accept this prior to 2.20 branching, but I won't block the release on it. 
If this doesn't make it in by the time we branch, I'll still go on the trunk,
and not the 2.20 branch.

Comment 29

[Frédéric Buclin]

Attachment: Complete patch, v3

Now, I don't modify any $::FORM{} variable anymore.

All my tests run as expected...

Comment 30

[Dave Miller [:justdave]]

Comment on attachment 168455 [details] [diff] [review]
Complete patch, v3

>Index: mozilla/webtools/bugzilla/process_bug.cgi

>@@ -628,19 +628,20 @@
>-        $::FORM{'resolution'} = $str; # Used later by CheckCanChangeField

You can't remove this or it'll break any rules that apply to resolutions in
CheckCanChangeField.   Remember that some resolutions are set by a knob action
and don't actually use the resolution form field (duplicate, moved), so we need
to fake it for the benefit of CheckCanChangeField.

>@@ -872,6 +861,26 @@
>+if (defined $::FORM{'qa_contact'}
>+    && $::FORM{'knob'} ne "reassignbycomponent")
>+{
>+    $qacontact = 0;
>+    my $name = trim($::FORM{'qa_contact'});
>+    # The QA contact cannot be deleted from show_bug.cgi for a single bug!

Why not?  This is a regression if you do this.	We specifically fixed this so
you could not too long ago, people won't be happy if it gets broken again.

Also, just as an FYI, the way you're handling $PrivilegesRequired is not
mod_perl safe (see bug 173629), but that can be cleaned up later, as there's a
hell of a lot of other stuff in process_bug that isn't mod_perl safe either ;)

Nothing else in here stands out to me at the moment.  I haven't tested it, just
took a quick look to see if there were any gotchas that stood out.  I
appreciate your level of commenting, it helps make the huge mess of process_bug
a bit more readable ;)

Comment 31

[Dave Miller [:justdave]]

(In reply to comment #28)
> I'll accept this prior to 2.20 branching, but I won't block the release on it. 
> If this doesn't make it in by the time we branch, I'll still go on the trunk,
> and not the 2.20 branch.

Hmm, and I hadn't actually looked at the pending patches yet when I said that. 
The sheer enormity of the patch and the particular pieces of code it touches
makes me really nervous about taking this for 2.20.  If we want this in 2.20
we'll need to make sure it gets set up on landfill and have someone do very
*thorough* testing on submitting bug changes with both show_bug and the
change-multiple screen.  If we wait till we branch, I'd be willing to check it
in anyway and catch regressions, although some thorough testing would still be
nice even then. :)  But as we're mostly volunteers, getting someone to do that
kind of testing isn't always easy.

Comment 32

[Dave Miller [:justdave]]

(In reply to comment #30)
> >+    # The QA contact cannot be deleted from show_bug.cgi for a single bug!
> Why not?  This is a regression if you do this. We specifically fixed this so
> you could not too long ago, people won't be happy if it gets broken again.

OK, I stand corrected, the previous fix for this was only for "reassign to
default" if the default was empty.  So this is fine.

Comment 33

[Frédéric Buclin]

Attachment: Complete patch, v3.1

restore $::FORM{'resolution'} as requested by justdave. No other modifications!

Comment 34

[Frédéric Buclin]

Comment on attachment 168481 [details] [diff] [review]
Complete patch, v3.1

Asking justdave for review as I don't know when joel will come back.

Comment 35

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

Comment on attachment 168481 [details] [diff] [review]
Complete patch, v3.1

>-    }
>-    elsif (trim($oldvalue) eq trim($newvalue)) {
>+    } elsif (trim($oldvalue) eq trim($newvalue)) {
...
>-    } elsif (($field eq "estimated_time" || $field eq "remaining_time") &&
>-             $oldvalue == $newvalue) {
>+    } elsif (($field eq "estimated_time" || $field eq "remaining_time")
>+             && $oldvalue == $newvalue)

Nit: per the style guide, keep/put elsif clauses on their own line, i.e.:

    }
    elsif (trim($oldvalue) eq trim($newvalue)) {
...
    }
    elsif (($field eq "estimated_time" || $field eq "remaining_time")
	   && $oldvalue == $newvalue)


>+        $qacontact = DBNameToIdAndCheck($name) if ($name ne "");

Nit: you don't need parentheses around conditionals appended to statements,
i.e. the following works just as well (and is simpler and easier to read):

+	 $qacontact = DBNameToIdAndCheck($name) if $name ne "";


Otherwise this looks good.  But I concur with Dave--this should wait until we
branch.

Comment 36

[Frédéric Buclin]

Comment on attachment 168481 [details] [diff] [review]
Complete patch, v3.1

joel, if you have time to review this one, I would appreciate. :)

Comment 37

[Vlad Dascalu]

Targeting bug to 2.20 since the 2.20 feature freeze was canceled.

Comment 38

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

Cancelling approval request due to pending review request.

Comment 39

[Dave Miller [:justdave]]

Since we have a few months until we're branching now, I'm not so nervous about
this anymore.

Comment 40

[Frédéric Buclin]

Attachment: Complete patch, v3.1.1

Forwarding r+. The previous patch is bitrotten due to my other patch in bug
274456 (see knob.html.tmpl).

Comment 41

[Shane H. W. Travis]

Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.100; previous revision: 1.99
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.96; previous revision: 1.95
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.225; previous revision: 1.224
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.49; previous revision: 1.48
done
Checking in template/en/default/bug/knob.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/knob.html.tmpl,v  <--
  knob.html.tmpl
new revision: 1.10; previous revision: 1.9
done
Checking in template/en/default/bug/create/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create.html.tm
pl,v  <--  create.html.tmpl
new revision: 1.39; previous revision: 1.38
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-
error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.84; previous revision: 1.83
done
Checking in template/en/default/global/userselect.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/userselect.html.tm
pl,v  <--  userselect.html.tmpl
new revision: 1.3; previous revision: 1.2
done

Comment 42

[Frédéric Buclin]

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

Comment 43

[Stephen Lee]

Comment on attachment 171931 [details] [diff] [review]
Complete patch, v3.1.1

+    # The reporter is a more complicated case...
+    if ($reporterid == $whoid) {
+	 $PrivilegesRequired = 2;

[ various conditions that could result in a return 0 ]

+	 # Allow the reporter to change anything else.
	 return 1;
     }
-  
+
     # If we haven't returned by this point, then the user doesn't have the
     # necessary permissions to change this field.
+    $PrivilegesRequired = 1;
     return 0;
 }


If I'm reading this right:

Random user tries to change (e.g.) target_milestone. The if($reporterid ==
$whoid) section is skipped.

Random user would see a message saying the reporter, owner or an empowered user
can change it.

Random user happens to know the reporter quite well, so asks the reporter to
make that change for him.

When the reporter tries to change it, the if($reporterid == $whoid) section is
executed.

This time the message only says that the owner or an empowered user can change
it...


The message should be consistent about who can make the change. In other words,
we need to check whether you would be able to make the change if you were the
reporter, even if you are not the reporter.

To do that, the above could become:

+    # The reporter is a more complicated case...
+	 $PrivilegesRequired = 2;

[ various conditions that could result in a return 0 ]

+    if ($reporterid == $whoid) {
+	 # Allow the reporter to change anything else.
	 return 1;
     }
-  
+
     # If we haven't returned by this point, then the user doesn't have the
     # necessary permissions to change this field.
+    $PrivilegesRequired = 1;
     return 0;
 }

Comment 44

[Frédéric Buclin]

Indeed, I thought about that too. The point is that I didn't want to test what
the reporter is allowed to do if the current user is *not* the reporter (as the
result remains the same for him, that is: action forbidden).

What you suggest here is to test all cases and only at the end decide to return
0 or 1. Yes, why not (it makes sense ;) ). My idea was that the error message
which is now displayed is "better" than the old one as the only case where it
lies is when you are not the reporter *and* the reporter would not be allowed to
do what you are trying to do.

Well ok, you convinced me.... ;)

Comment 45

[Frédéric Buclin]

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

Comment 46

[Frédéric Buclin]

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