234855 - product field on edit-multiple includes products the user doesn't have access to

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

[Laran Evans]

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113

all products are configured with group restrictions (one group per product, each
user is in exactly 1 group).  current user searched for some bugs.  the list
came up, I clicked "change several bugs at once".  when the page loaded, all of
the products (including others to which the current user did not have access)
showed up in the dropdown.  Only those groups to which the user belongs should
show up.  In this case that would be one group.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Joel Peshkin]

Yep

Comment 2

[Dave Miller [:justdave]]

Does this affect 2.16 or did this get accidently broken by the groups rewrite?

Comment 3

[Dave Miller [:justdave]]

Not only does the product menu on edit-multiple include products that are
mandatory to groups you don't have access to, but it also include inactive
products (with disallownew set on them).  In theory you shouldn't be able to
move a bug to a product that's inactive either.

Comment 4

[Joel Peshkin]

Attachment: Bleah! - the patch

We should also do an audit of all the places where the fool ::legal_whatever
variables are used.

Comment 5

[Dave Miller [:justdave]]

Just confirmed this on the 2.16 branch.  It's a problem there as well.  Oof.

Comment 6

[Joel Peshkin]

hmm....

should we use EnterableProducts??

Comment 7

[Dave Miller [:justdave]]

Yeah, Enterable sounds good.  In theory, moving a bug to a different product
implies entry privs to put it there.

Comment 8

[Christian Reis]

Comment on attachment 142339 [details] [diff] [review]
Bleah! - the patch

This doesn't work. All the products are still returned when testing with a user
which is a member of *no* groups at all and product entry is restricted. I
think this is a bug in GetSelectableProducts...

Additional comments: none of the variables here  are in StudlyCaps, and
SelectableProduct is a bit confusing.

Comment 9

[Christian Reis]

It seems this bit of GetSelectableProducts is odd:

    if (defined Bugzilla->user && %{Bugzilla->user->groups}) {
        $query .= "AND group_id NOT IN(" .
                   join(',', values(%{Bugzilla->user->groups})) . ") ";
    }

can somebody tell me why we're checking if the group_id of the product is *not*
IN the user's list? Shouldn't it be *IN*?

Comment 10

[Christian Reis]

Attachment: kiko_v1: ugly but functional

Comment 11

[Christian Reis]

Attachment: kiko_v1: sigh. removed cruft.

So I added a GetEnterableProducts that nobody wants to look at, and use it in
buglist.cgi. Last patch had a chunk that wasn't relevant.

Comment 12

[Joel Peshkin]

Response to comment 9:
That's the way we make sure that something is restricted to no group of which
the user is not a member. 

We say....

SELECT thing FROM thing_table 
  LEFT JOIN thing_group_map 
    ON thing_table.id = thing_group_map.thing_id 
    AND thing_group_map.group_id NOT IN(your_grouplist)
  WHERE
    thing_group_map.thing_id IS NULL;

That says that we must find no (NULL) thing_group_map entry that your_grouplist
does not cover.


The original patch was finding selectable products rather than enterable
products.  It would offer you the same list as query.cgi would, closing the
security hole, but leaving you with a false expectation that you could actually
use some of those options.

The patch (kiko v1: sigh) looks reasonable...  I'll give it a better read when I
get a chance.

Comment 13

[Christian Reis]

We can use this lame-o function in duplicates.cgi too if the same issue exists
there.

Comment 14

[Joel Peshkin]

Comment on attachment 142792 [details] [diff] [review]
kiko_v1: sigh. removed cruft.

r=joel by inspection

Let's get a second r= as well and also make sure that someone else has tested
it.

Comment 15

[Christian Reis]

For the record, I did test this, and it did work for me at my local installation.

Comment 16

[Christian Reis]

(In reply to comment #12)
> That's the way we make sure that something is restricted to no group of which
> the user is not a member. 

That's quite possibly the most confusing phrase I've ever read, btw. :-)

Comment 17

[Joel Peshkin]

(In reply to comment #16)

> That's quite possibly the most confusing phrase I've ever read, btw. :-)

OK,  "You cannot see something if it requires a group of which you are not a
member.  You can see something if there is no reason why not." :-)

Gerv and I have been re-debating the question of shifting from AND groups to OR
groups.  If we could find a good way to migrate existing sites, this stuff would
all become much simpler.

Comment 18

[Joel Peshkin]

I'm not sure that we want to change this on the 2.16 branch.

The whole notion that there are products that are truly invisible to the
unauthorized is a new concept in 2.17

If a 2.16 installation were that sensitive, there are many other things that
would have to be changed as well.

Comment 19

[Dave Miller [:justdave]]

(In reply to comment #18)
> I'm not sure that we want to change this on the 2.16 branch.

We do, although it may or may not need the same logic in the fix.

> The whole notion that there are products that are truly invisible to the
> unauthorized is a new concept in 2.17

No it isn't.  The groups system in 2.16 had this as well.  If there was a group
name that matched a product name, only people in the group could see the product.

> If a 2.16 installation were that sensitive, there are many other things that
> would have to be changed as well.

Then those would also be security bugs and should be fixed.

Comment 20

[Christian Reis]

Should I be requesting approval here?

Comment 21

[Dave Miller [:justdave]]

Comment on attachment 142792 [details] [diff] [review]
kiko_v1: sigh. removed cruft.

Since this is the only place calling it, why not have GetEnterableProducts
return a reference to the array?  Then you can just assign the function call to
$vars->{'products'} instead of having to have a temporary assignment in between
so you can reference it...

r= justdave either way.

We still need a fix for this on the 2.16 branch as well.

Comment 22

[Joel Peshkin]

Attachment: 2.16-specific patch

Here's the 2.16-specific version

Comment 23

[Joel Peshkin]

Suggested description:

"A user performing a mass-change operation on a list of bugs would be presented
with a list of possible products including those products to which that user has
no access."

Comment 24

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

Comment on attachment 143589 [details] [diff] [review]
2.16-specific patch

Looks good, works.

Comment 25

[Joel Peshkin]

Proposed wording for release notes:
The form for mass-editing a list of bugs divulges the list of product names even
if some of those names are supposed to remain unknown to the user.

Comment 26

[Joel Peshkin]

checked in on both branches

Comment 27

[Dave Miller [:justdave]]

Clearing the security flag on disclosed bugs