bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED in Bugzilla 4.2

Status

()

Bugzilla
Creating/Changing Bugs
P3
enhancement
RESOLVED FIXED
19 years ago
7 years ago

People

(Reporter: justdave, Assigned: glob)

Tracking

(Blocks: 1 bug)

unspecified
Bugzilla 4.2
Dependency tree / graph
Bug Flags:
approval +
testcase ?

Details

(Whiteboard: [people:cc] [permissions:edit][wanted-bmo])

Attachments

(1 attachment, 6 obsolete attachments)

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

19 years ago
This bug is similar to bug 7345.

Comment 2

19 years ago
tara@tequilarista.org 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 .)
Assignee: terry → tara

Comment 3

18 years ago
See also bug 34488.

Comment 4

18 years ago
Adding default QA contact to all open Webtools/Bugzilla bugs lacking one.
Sorry for the spam.
QA Contact: matty
Target Milestone: --- → Bugzilla 2.16
Priority: P3 → P1

Comment 5

17 years ago
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

17 years ago
moving
Assignee: tara → myk
Component: Bugzilla → Creating/Changing Bugs
Product: Webtools → Bugzilla
Version: other → 2.10
correcting version field lost in product move
Version: 2.10 → unspecified

Updated

17 years ago
Whiteboard: [people:cc]

Updated

17 years ago
Whiteboard: [people:cc] → [people:cc] [permissions:edit]
Taking.  Very similar to bug 7710 which is also mine.
Assignee: myk → caillon
Status: NEW → ASSIGNED
Priority: P1 → P3

Comment 9

17 years ago
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?
->2.18
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18

Comment 11

17 years ago
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.
Sigh.  No time to work on this anymore...  ->Default owner.
Assignee: caillon → myk
Status: ASSIGNED → NEW
Depends on: 95923
Whiteboard: [people:cc] [permissions:edit] → [people:cc] [permissions:edit] [blocker will fix]
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)
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
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.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22

Comment 15

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

Updated

13 years ago
Assignee: myk → create-and-change
QA Contact: mattyt-bugzilla → default-qa
Target Milestone: Bugzilla 2.22 → ---

Comment 16

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

Comment 17

12 years ago
No.
Whiteboard: [people:cc] [permissions:edit] [blocker will fix] → [people:cc] [permissions:edit]
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.
Summary: Add checkbox for "email me" instead of CC list if user can't edit bugs. → Users should not be able to CC/unCC anyone other than themselves if they can't edit bugs

Updated

8 years ago
Duplicate of this bug: 458142

Updated

8 years ago
Duplicate of this bug: 595735
(Assignee)

Updated

7 years ago
Whiteboard: [people:cc] [permissions:edit] → [people:cc] [permissions:edit][wanted-bmo]
(Assignee)

Updated

7 years ago
Duplicate of this bug: 651491

Comment 22

7 years ago
I think being able to CC people is fine. But unCC'ing is probably not unless you have editbugs privs.
Assignee: create-and-change → LpSolit
Target Milestone: --- → Bugzilla 5.0
(Assignee)

Updated

7 years ago
Assignee: LpSolit → glob
(Assignee)

Comment 23

7 years ago
Created attachment 527269 [details] [diff] [review]
patch for trunk
Attachment #527269 - Flags: review?(LpSolit)
(Assignee)

Comment 24

7 years ago
Created attachment 527270 [details] [diff] [review]
patch for 4.0
(Assignee)

Comment 25

7 years ago
Created attachment 527271 [details] [diff] [review]
patch for 3.6
(Assignee)

Updated

7 years ago
Summary: Users should not be able to CC/unCC anyone other than themselves if they can't edit bugs → Users should not be able to unCC anyone other than themselves if they can't edit bugs

Comment 26

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

No need to attach 3 times the exact same patch. :)
Attachment #527271 - Attachment is obsolete: true

Updated

7 years ago
Attachment #527270 - Attachment is obsolete: true

Comment 27

7 years ago
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".
Attachment #527269 - Flags: review?(LpSolit) → review-

Updated

7 years ago
Status: NEW → ASSIGNED
Target Milestone: Bugzilla 5.0 → Bugzilla 4.2
(Assignee)

Comment 28

7 years ago
Created attachment 527296 [details] [diff] [review]
patch v2
Attachment #527269 - Attachment is obsolete: true
Attachment #527296 - Flags: review?(LpSolit)

Comment 29

7 years ago
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.
(Assignee)

Comment 30

7 years ago
(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

7 years ago
(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

7 years ago
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 [%-%].
Attachment #527296 - Flags: review?(LpSolit) → review-

Comment 33

7 years ago
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)) %]

Updated

7 years ago
Blocks: 540044
(Assignee)

Comment 34

7 years ago
Created attachment 527506 [details] [diff] [review]
patch v3

thanks for the review; this revision address the review points.
Attachment #527296 - Attachment is obsolete: true
Attachment #527506 - Flags: review?(LpSolit)

Comment 35

7 years ago
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. :)
Attachment #527506 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 36

7 years ago
Created attachment 528531 [details] [diff] [review]
patch v4
Attachment #527506 - Attachment is obsolete: true
Attachment #528531 - Flags: review?(LpSolit)

Comment 37

7 years ago
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 %].
Attachment #528531 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 38

7 years ago
Created attachment 529948 [details] [diff] [review]
patch v5
Attachment #528531 - Attachment is obsolete: true
Attachment #529948 - Flags: review?(LpSolit)

Comment 39

7 years ago
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
Attachment #529948 - Flags: review?(LpSolit) → review+

Updated

7 years ago
Flags: approval+
(Assignee)

Comment 40

7 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Keywords: relnote

Updated

7 years ago
Flags: testcase?

Comment 41

7 years ago
Updated the patch in Bug #704753

Comment 42

7 years ago
Added to relnotes in bug 713346.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.