Prevent users from adding another user who shouldn't have access to a bug as assignee or CC member

RESOLVED FIXED in Bugzilla 2.22

Status

()

Bugzilla
Creating/Changing Bugs
P2
enhancement
RESOLVED FIXED
13 years ago
6 years ago

People

(Reporter: André Batosti, Assigned: Gabriel Sales de Oliveira)

Tracking

({selenium})

2.21
Bugzilla 2.22
selenium
Dependency tree / graph
Bug Flags:
approval +
testcase +

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050725 Firefox/1.0.6 (Ubuntu package 1.0.6)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050725 Firefox/1.0.6 (Ubuntu package 1.0.6)

an user can add another user can't have access to bug as assignee or in CC list.
perhaps this user will be able to see the bug after this.

Reproducible: Always
(Reporter)

Comment 1

13 years ago
Created attachment 197104 [details] [diff] [review]
batosti_v1
Attachment #197104 - Flags: review?(bugreport)

Comment 2

13 years ago
I'm marking this as an enhancment since it really adds a level of isolation of
users optionally.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.22
Version: unspecified → 2.21

Comment 3

13 years ago
You know, I think it's generally silly to do this -- you can just remove the "CC
List" checkbox from the bug, and then only the people who are actually in the
group can see the bug. Then, even if people are on the CC List, they won't get
any emails, because BugMail will check and see that they don't actually have
permissions.

I recommend WONTFIX or INVALID or WORKSFORME, depending on how you look at it. :-)
Severity: normal → enhancement

Comment 4

13 years ago
That doesn't do it.  For compartmentalized installations, it is important that
CClist and assignee still work, but users need to be protected from making
really big goofs.

Naturally, this type of operation is controlled by a param.
Summary: an user can add another user can't have access to bug as assignee or in CC list → Prevent users from adding another user who shouldn't have access to a bug as assignee or CC member

Comment 5

13 years ago
Comment on attachment 197104 [details] [diff] [review]
batosti_v1

>Index: process_bug.cgi

>+sub CanAddUserToBug {
>+    my ($prod_id, $id, $uid) = (@_);
>+    my $user = new Bugzilla::User($uid);
>+    unless ($user->can_edit_product_id($prod_id)) {
>+        ThrowUserError("invalid_user_group", { 'user' =>
>+        $cgi->param('assigned_to'), bug_id => $id });
>+    }
>+}

I don't think this routine should be in process_bug.cgi, but rather in Bug.pm.
I think post_bug.cgi would need this routine too?

Comment 6

13 years ago
I agree with Joel.  I have an IT company with many customers.  They report their
issues using Bugzilla and I'm concerned that Acme and Zoom can't send messages
to each other through this interface.  I'm also concerned that I am not going to
accidentally be able to add CC's from Zoom when a bug belongs to Acme.

To allow for the best of both situations, when a user's visibility expands
beyond the limitations of a particular group, I think it's appropriate to have
two CC boxes instead of one.  The first box would list associated CC's (meaning
those associated with the product), and the second box would list all CC's not
associated with the product.  This is especially true for installations that
have drop-down lists enabled.  I also feel the display of the second box should
be a param.

Comment 7

13 years ago
From my standpoint, we want to prevent the addition of any user who would
violate a CANEDIT restriction on a bug.  It is less a matter of keeping the user
from seing the bug as it is to keep the others who can see the bug from seeing
the user.

Imagine if Oracle's interest in Bugzilla were a highly sensitive bit if
commercial information and someone accidentally copied an Oracle person on a
Bugzilla bug.  Suddenly, it would be obvious to a whole bunch of people what
Oracle was and was not interested in.  In some enviroments (clealy including
Kevin's and mine - and I suspec a number of others), that is a very key issue.

Comment 8

13 years ago
Comment on attachment 197104 [details] [diff] [review]
batosti_v1

Cleanups suggested regarding usage of CanAddUserToBug() are valid suggestions. 
Really, the key ussue is ...  does this  user have any business being on any
bug in this product?   I think the test is can_edit_product where the product
may be the product of a particular bug or of a bug about to be posted.

We should also apply the same restriction to requests.

As a distinct bug, we should not let a product change carry inappropriate users
into a new product.
Attachment #197104 - Flags: review?(bugreport) → review-

Comment 9

13 years ago
If you really have different populations of users and products, why not having
separate installations of Bugzilla?

Comment 10

13 years ago
(In reply to comment #9)
> If you really have different populations of users and products, why not having
> separate installations of Bugzilla?

They all get to see some bugs in common.  Also, the internal team has to deal
with all of them.

Updated

13 years ago
Assignee: create-and-change → gabriel

Comment 11

13 years ago
One additional benefit of this is an implicit "documentation" of groups.

Looking at the list of users in Bugzilla for a given defect, I can easily bring
these users into a single room and get things rolling.

I'd much appreciate finding this feature in a future release of Bugzilla.
(Assignee)

Comment 12

13 years ago
Created attachment 199091 [details] [diff] [review]
v2-move can_add_user_to_bug to Bug.pm

can_add_user_to_bug moved to Bug.pm
Attachment #199091 - Flags: review?(bugreport)

Comment 13

13 years ago
Comment on attachment 199091 [details] [diff] [review]
v2-move can_add_user_to_bug to Bug.pm

Close... a few items...

1) defparms says the user must be able to see the bug.	Actually, the user must
be able to edit bugs in the product.

2) This should also cover qa contact if that is in use.

3) When the assignee is rejected, the error message format is munged up.  That
may only happen if realname is not set.  I suggest oyu just consistently use
the login_name for these messages.
Attachment #199091 - Flags: review?(bugreport) → review-
(Assignee)

Comment 14

13 years ago
Created attachment 199180 [details] [diff] [review]
v3- qa-contact added

Qa-contact added and paramaters description updated.
Attachment #199091 - Attachment is obsolete: true
Attachment #199180 - Flags: review?(bugreport)

Updated

13 years ago
Attachment #199180 - Flags: review?(bugreport) → review+

Updated

13 years ago
Flags: approval?
This got bitrotted by bug 46296.  defparams.pl no longer exists.
Flags: approval?
(Assignee)

Comment 16

13 years ago
Created attachment 199293 [details] [diff] [review]
v3-fix 

Unbitrotted version after bug 46296
Attachment #199180 - Attachment is obsolete: true
Attachment #199293 - Flags: review?(bugreport)

Updated

13 years ago
Attachment #199293 - Flags: review?(bugreport) → review+

Updated

13 years ago
Flags: approval?

Updated

13 years ago
Flags: approval?
(Assignee)

Comment 17

13 years ago
Created attachment 199308 [details] [diff] [review]
fix change can_edit_product_id to can_edit_product

Change can_edit_product_id() to can_edit_product()
Attachment #199293 - Attachment is obsolete: true

Updated

13 years ago
Attachment #199308 - Flags: review?(bugreport)

Comment 18

13 years ago
Comment on attachment 199308 [details] [diff] [review]
fix change can_edit_product_id to can_edit_product

When strictisolation is off, rather than permitting the CC entries, it just
silently ignores them.
Attachment #199308 - Flags: review?(bugreport) → review-

Comment 19

13 years ago
Comment on attachment 199308 [details] [diff] [review]
fix change can_edit_product_id to can_edit_product

Actually, there is another problem with this...

If I try to add 4 users to the CClist and 3 of them are OK and one is not, I
will get an error message, but the rest of the CC changes will have gone
through, but not the rest of the bug changes altogether.

The check for inappropriate CC members needs to happen at the validation stage
not at the actual insertion stage so that, if we are going to throw an error,
we don't let ANY of the changes happen.
(Assignee)

Comment 20

13 years ago
Created attachment 199830 [details] [diff] [review]
v4-fix previous mistakes

cc add is treat separate.
Attachment #199830 - Flags: review?(bugreport)

Comment 21

13 years ago
Comment on attachment 199830 [details] [diff] [review]
v4-fix previous mistakes

r=joel

Still need to do a similar protection for post_bug... but, that is another bug.
(needs to be done by 2.22)
Attachment #199830 - Flags: review?(bugreport) → review+

Updated

13 years ago
Flags: approval?
Flags: approval? → approval+

Comment 22

13 years ago
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.97; previous revision: 1.96
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.343; previous revision: 1.342
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.288; previous revision: 1.287
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.98; previous revision: 1.97
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.89; previous revision: 1.88
done
Checking in Bugzilla/Config/GroupSecurity.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/GroupSecurity.pm,v  <--  Grou
pSecurity.pm
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/admin/params/groupsecurity.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/groupsecurit
y.html.tmpl,v  <--  groupsecurity.html.tmpl
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tm
pl,v  <--  user-error.html.tmpl
new revision: 1.133; previous revision: 1.132
done
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Blocks: 312773

Updated

13 years ago
Blocks: 315332

Comment 23

12 years ago
*** Bug 328360 has been marked as a duplicate of this bug. ***

Updated

12 years ago
Flags: testcase?

Comment 24

7 years ago
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.2/
added t/test_strict_isolation.t
Committed revision 206.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.0/
added t/test_strict_isolation.t
Committed revision 195.
Flags: testcase? → testcase+

Comment 25

7 years ago
(In reply to Frédéric Buclin from comment #24)
> Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.2/
> added t/test_strict_isolation.t
> Committed revision 206.
> 
> Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.0/
> added t/test_strict_isolation.t
> Committed revision 195.

  Wow, this is awesome!! Thank you so much, LpSolit!! This is a huge deal. :-)

Updated

6 years ago
Keywords: selenium
You need to log in before you can comment on or make changes to this bug.