28849 - Users should not be able to unCC anyone other than themselves if they can't edit bugs

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement, P3)

Description

[Dave Miller [:justdave]]

Users without the capability to edit a bug should not be able to remove other 
users from the CC list, but should still be able to remove themselves.  A good 
way to solve this would be add a checkbox for "Email me when this bug changes".  
If the user has the capability to edit bugs, then they should still get the 
existing textbox like we do currently, as well, but if the user doesn't have that 
capability, or if they haven't logged in, then the CC list can be included in a 
hidden field instead of a textbox, so that it still gets submitted with the 
update, but the users don't have the opportunity to change it.  (This also helps 
to protect the privacy of people who want themselves CC'd on a bug).  Then the 
cgi can look at that checkbox and add or remove that person's name as necessary 
when processing it.  (It would also have to be preset if their name was in the CC 
list when they viewed it, if they were logged in at the time).

Comment 1

[Jesse Ruderman]

This bug is similar to bug 7345.

Comment 2

[Terry Weissman]

[e-mail] is the new owner of Bugzilla and Bonsai.  (For details,
see my posting in netscape.public.mozilla.webtools,
news://news.mozilla.org/38F5D90D.F40E8C1A%40geocast.com .)

Comment 3

[Ben Bucksch (:BenB)]

See also bug 34488.

Comment 4

[Stephan Niemz]

Adding default QA contact to all open Webtools/Bugzilla bugs lacking one.
Sorry for the spam.

Comment 5

[GavinS]

On a sort of related note, if a user doesn't have permissions to edit things on
a bug, then shouldn't all the fields the user can't edit be made read-only?

e.g. the keywords field sits there tempting me to add things on all bugs, but I
can only make changes to it on bugs I created.

Comment 6

[Zach Lipton [:zach]]

moving

Comment 7

[Dave Miller [:justdave]]

correcting version field lost in product move

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Taking.  Very similar to bug 7710 which is also mine.

Comment 9

[Christian Reis]

Gavin: that is not the gist of this bug, but is worthy of a proper bug itself.
If you could file another one with your summary, it would be great.

Are there UI proposals for this bug to compare with my proposal on bug 34488?

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

->2.18

Comment 11

[GavinS]

Christian: Re my comment#5 and your reply in comment#9. I've only just seen
this, as I wasn't CC'd - doh!

Bug#95923 is about not letting the user change fields they aren't allowed to.

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

Sigh.  No time to work on this anymore...  ->Default owner.

Comment 13

[Dave Miller [:justdave]]

Enhancements which don't currently have patches on them which are targetted at
2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. 
Consideration will be taken for moving items back to 2.18 on a case-by-case
basis (but is unlikely for enhancements)

Comment 14

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 15

[Frédéric Buclin]

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

Comment 16

[alanjstr]

Dave -
The whiteboard says blocker will fix and the blocker is done.  So can this be closed?

Comment 17

[Olav Vitters]

No.

Comment 18

[Dave Miller [:justdave]]

yeah, what ended up getting implemented in bug 95923 isn't quite was was being asked for here.  That ended up blocking all changes, and only for the product restrictions (i.e. the canedit bit on a product). With that change, users can no longer CC themselves, even, on a bug in a product in which they don't have canedit.

Users should probably still be able to CC/unCC themselves from a bug, even without canedit privs, as that's our notification method. Likewise, random users who haven't gotten editbugs yet probably shouldn't be able to CC/unCC anyone other than themselves, which they CAN do now if the product doesn't have canedit restrictions.

Comment 22

[Frédéric Buclin]

I think being able to CC people is fine. But unCC'ing is probably not unless you have editbugs privs.

Comment 23

[:glob ✱]

Attachment: patch for trunk

Comment 24

[:glob ✱]

Attachment: patch for 4.0

Comment 25

[:glob ✱]

Attachment: patch for 3.6

Comment 26

[Frédéric Buclin]

Comment on attachment 527271 [details] [diff] [review]
patch for 3.6

No need to attach 3 times the exact same patch. :)

Comment 27

[Frédéric Buclin]

Comment on attachment 527269 [details] [diff] [review]
patch for trunk

>=== modified file 'Bugzilla/Bug.pm'

>+    if (!$currentUser->in_group('editbugs', $self->{'product_id'})) {
>+        if ($user->id != $currentUser->id) {

You could combine both conditions into a single one. Also, please use $self->product_id instead of the internal value directly.



>=== modified file 'template/en/default/global/user-error.html.tmpl'

>+  [% ELSIF error == "cc_remove_denied" %]
>+    [% title = "Access Denied" %]

Maybe "Change Denied" would be more appropriate?


Also, you have to enforce this restriction in the UI. For powerless users, I think we should make the CC list read-only, and the label of the checkbox should be changed from "Remove selected CCs" to "Remove me from the CC list".

Comment 28

[:glob ✱]

Attachment: patch v2

Comment 29

[Max Kanat-Alexander]

Comment on attachment 527296 [details] [diff] [review]
patch v2

Hmm. Perhaps this should be implemented via check_can_change_field instead? Also, I'm suspicious of your UI logic, as it doesn't check all the ways to user can edit the bug.

Comment 30

[:glob ✱]

(In reply to comment #29)
> Also, I'm suspicious of your UI logic, as it doesn't check all the ways to user
> can edit the bug.

i tested it as anon, a user without editbugs and a user with editbugs.  what did i miss?

Comment 31

[Frédéric Buclin]

(In reply to comment #29)
> Hmm. Perhaps this should be implemented via check_can_change_field instead?

The problem is that you can always edit the CC field, as you can add/remove yourself.

Comment 32

[Frédéric Buclin]

Comment on attachment 527296 [details] [diff] [review]
patch v2

>=== modified file 'template/en/default/bug/edit.html.tmpl'

>+          [% UNLESS user.id && !user.in_group('editbugs', bug.product.id) && !bug.cc.contains(user.email) %]
>             (<a href="#" id="cc_edit_area_showhide">[% IF user.id %]edit[% ELSE %]show[% END %]</a>)
>+          [% END %]

This totally prevents us from displaying the CC list. Instead of your UNLESS block, it should rather be:

 [% IF user.id AND bug.user.canedit %]edit[% ELSE %]show[% END %]


>-          [% IF user.id %]
>+          [% IF user.id && user.in_group('editbugs', bug.product.id) %]

This change should be reverted. We don't want to prevent users from adding other users as they may legitimately have to do it. And we never saw abuse this way, isn't it? ;)


>-            <select id="cc" name="cc" multiple="multiple" size="5">
>+            <select id="cc" multiple="multiple" size="5"
>+             name="cc[% IF !user.in_group('editbugs', bug.product.id) %]_select[% END %]">

Shouldn't we omit the name entirely, in this case? cc_select is not understood by process_bug.cgi. Also, it should be bug.user.canedit instead.


>+            [% IF user.id && !user.in_group('editbugs', bug.product.id) -%]
>+            <input type="hidden" name="cc" value="[% user.email FILTER html %]">
>+            [% END %]

Same here.


>-              [%%]<label for="removecc">Remove selected CCs</label>
>+              [%-%]<label for="removecc">
>+              [% IF user.in_group('editbugs', bug.product.id) %]
>+              [%-%]Remove selected CCs
>+              [% ELSE %]
>+              [%-%]Remove me from the CC list
>+              [% END %]
>+              </label>

Please fix the indentation. Also, I'm not sure we need all these [%-%].

Comment 33

[Frédéric Buclin]

Comment on attachment 527296 [details] [diff] [review]
patch v2

>=== modified file 'template/en/default/bug/edit.html.tmpl'

>             [% IF user.id %]
>               <br>
>               <input type="checkbox" id="removecc" name="removecc">
>-              [%%]<label for="removecc">Remove selected CCs</label>
>+              [%-%]<label for="removecc">
>+              [% IF user.in_group('editbugs', bug.product.id) %]
>+              [%-%]Remove selected CCs
>+              [% ELSE %]
>+              [%-%]Remove me from the CC list
>+              [% END %]
>+              </label>
>               <br>
>             [% END %]

One more thing. If the user isn't in the CC list and has no privs, this checkbox shouldn't be displayed at all. So the first IF condition should be changed to

 [% IF user.id AND (bug.user.canedit OR bug.cc.contains(user.login)) %]

Comment 34

[:glob ✱]

Attachment: patch v3

thanks for the review; this revision address the review points.

Comment 35

[Frédéric Buclin]

Comment on attachment 527506 [details] [diff] [review]
patch v3

>=== modified file 'template/en/default/bug/edit.html.tmpl'

>-            (<a href="#" id="cc_edit_area_showhide">[% IF user.id %]edit[% ELSE %]show[% END %]</a>)
>+            (<a href="#" id="cc_edit_area_showhide">
>+            [% IF user.id AND (bug.user.canedit OR bug.cc.contains(user.login)) %]edit
>+            [% ELSE %]show[% END %]</a>)

I just realized that we don't want this change as a logged in user can always add people to the list. So technically, "edit" is correct. Please revert this change on checkin.


>+            [% IF user.id && !bug.user.canedit %]
>+            <input type="hidden" name="cc" value="[% user.email FILTER html %]">
>+            [% END %]

Please fix the indentation. Also, we must pass user.login, not user.email, else this will break process_bug.cgi if emailsuffix is in use. And as in the code above it, it must be FILTER email FILTER html.


>+                [% IF (bug.user.canedit) %]

Useless parens.


Otherwise looks good. Next shoot will be the last one. :)

Comment 36

[:glob ✱]

Attachment: patch v4

Comment 37

[Frédéric Buclin]

Comment on attachment 528531 [details] [diff] [review]
patch v4

>=== modified file 'Bugzilla/Bug.pm'

>+    my $currentUser = Bugzilla->user;
>+    if (!$currentUser->in_group('editbugs', $self->product_id)
>+        && $user->id != $currentUser->id)

Now that we use bug.user.canedit everywhere in the template, we have to use it here to. So we want
  if (!$self->user->{canedit} && $user->id != Bugzilla->user->id) here.



>=== modified file 'template/en/default/bug/edit.html.tmpl'

>+              <input type="hidden" name="cc" value="[% user.login FILTER html %]">

You forgot FILTER email: [% user.login FILTER email FILTER html %].

Comment 38

[:glob ✱]

Attachment: patch v5

Comment 39

[Frédéric Buclin]

Comment on attachment 529948 [details] [diff] [review]
patch v5

>=== modified file 'Bugzilla/Bug.pm'

>+    if (!$self->user->{'canedit'} && $user->id != $currentUser->id)
>+    {

Nit: { comes at the end of the previous line.



>=== modified file 'template/en/default/bug/edit.html.tmpl'

>-            <select id="cc" name="cc" multiple="multiple" size="5">
>-              [% FOREACH c = bug.cc %]
>-                <option value="[% c FILTER email FILTER html %]">
>-                  [% c FILTER email FILTER html %]</option>
>-              [% END %]
>+            <select id="cc" multiple="multiple" size="5"
>+            [% IF bug.user.canedit %]name="cc"[% END %]>
>+            [% FOREACH c = bug.cc %]
>+              <option value="[% c FILTER email FILTER html %]">
>+                [% c FILTER email FILTER html %]</option>
>+            [% END %]
>             </select>

There is no reason to change the indentation of the FOREACH block. It was correct as it was; the change isn't. Please revert this on checkin.

Otherwise looks good and works fine. r=LpSolit

Comment 40

[:glob ✱]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Bug.pm
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/global/user-error.html.tmpl
Committed revision 7811.

Comment 41

[Mark A. Hershberger]

Updated the patch in Bug #704753

Comment 42

[Frédéric Buclin]

Added to relnotes in bug 713346.