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

Status: RESOLVED FIXED

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

Description

[André Batosti]

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

Comment 1

[André Batosti]

Attachment: batosti_v1

Comment 2

[Joel Peshkin]

I'm marking this as an enhancment since it really adds a level of isolation of
users optionally.

Comment 3

[Max Kanat-Alexander]

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. :-)

Comment 4

[Joel Peshkin]

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.

Comment 5

[Frédéric Buclin]

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

[Kevin Benton]

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

[Joel Peshkin]

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

[Joel Peshkin]

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.

Comment 9

[Frédéric Buclin]

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

Comment 10

[Joel Peshkin]

(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.

Comment 11

[Stefan Hoffmeister]

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.

Comment 12

[Gabriel Sales de Oliveira]

Attachment: v2-move can_add_user_to_bug to Bug.pm

can_add_user_to_bug moved to Bug.pm

Comment 13

[Joel Peshkin]

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.

Comment 14

[Gabriel Sales de Oliveira]

Attachment: v3- qa-contact added

Qa-contact added and paramaters description updated.

Comment 15

[Dave Miller [:justdave]]

This got bitrotted by bug 46296.  defparams.pl no longer exists.

Comment 16

[Gabriel Sales de Oliveira]

Attachment: v3-fix

Unbitrotted version after bug 46296

Comment 17

[Gabriel Sales de Oliveira]

Attachment: fix change can_edit_product_id to can_edit_product

Change can_edit_product_id() to can_edit_product()

Comment 18

[Joel Peshkin]

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.

Comment 19

[Joel Peshkin]

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.

Comment 20

[Gabriel Sales de Oliveira]

Attachment: v4-fix previous mistakes

cc add is treat separate.

Comment 21

[Joel Peshkin]

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)

Comment 22

[Joel Peshkin]

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

Comment 23

[Frédéric Buclin]

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

Comment 24

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.2/
added t/test_strict_isolation.t
Committed revision 206.

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.0/
added t/test_strict_isolation.t
Committed revision 195.

Comment 25

[Max Kanat-Alexander]

(In reply to Frédéric Buclin from comment #24)
> Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.2/
> added t/test_strict_isolation.t
> Committed revision 206.
> 
> Committing to: bzr+ssh://[e-mail]/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. :-)