306265 - implement User::can_see_product()

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Frédéric Buclin]

Routines in Product.pm such as get_products_by_classification(),
get_all_products() and check_product() do not check whether the user is allowed
to access products they return, allowing users with unsufficient privileges to
access these products, see bug 271596, especially justdave's comments (bug
271596 comment 2 and bug 271596 comment 4). Actually, this only affects
edit*.cgi pages for users with editcomponents privs but we can easily imagine
that the use of Product.pm will in the near future extend to process_bug.cgi and
show_bug.cgi, for instance.

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

Comment 2

[Frédéric Buclin]

hum... I think 'use Bugzilla::User' inside Product.pm is useless as the user
object is obtained from Bugzilla->user. I can remove it on checkin if it's the
only one nit.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 194119 [details] [diff] [review]
patch, v1

No, this needs to be done only some of the time. We don't want to *always*
limit it, we want to create new functions that specifically only return
products the user can see.

And yeah, you don't need to add the "use Bugzilla::User".

Comment 4

[Frédéric Buclin]

(In reply to comment #3)
> (From update of attachment 194119 [details] [diff] [review] [edit])
> No, this needs to be done only some of the time. We don't want to *always*
> limit it, we want to create new functions that specifically only return
> products the user can see.


Huh?? Can you give me at least one case where we want to override your
privileges? Products which you cannot access due to restrictions should *never*
be returned. Look at how and where Product.pm is used and you will notice that
I'm correct. These restrictions are the expected default behavior.

Comment 5

[Max Kanat-Alexander]

Well, just think of any case where Bugzilla would need to iterate over the
actual entire list of products.

It's true, there might not be that many of those cases, though.

My real concern is that the subroutines are becoming confusing to the
unknowledgeable admin who wants to customize Bugzilla. That is, if the
subroutine is called "get_all_products" -- it should return ALL products. The
other modules, such as Bugzilla::Component and Bugzilla::Classification, return
ALL components and classifications.

Perhaps instead make a Bugzilla::User method for getting the list of products
that the user can access (something like get_selectable_classifications), and
use that.

Comment 6

[Frédéric Buclin]

yes, you are right. I was thinking about that too after my posting: Product.pm &
co should be user-inspecific. As we already have User::get_selectable_products()
and User::get_selectable_classifications(), as well as User::can_see_user() and
User::can_see_bug(), I think User::can_see_product() would make sense too, and
then all pages should make sure the user is allowed to access the given product
thanks to this method. Updating the summary accordingly.

Comment 7

[Frédéric Buclin]

Attachment: patch, v2

User::can_see_product() checks whether the product name passed to the routine
is one of the product names returned by User::get_selectable_products().

Comment 8

[Christian Reis]

Comment on attachment 194928 [details] [diff] [review]
patch, v2

Looks okay, if this is the way you want to implement it (IOW, if you are okay
with depending on selectable_products, and not implement a specific SQL query
for this. I think the SQL query could even be nicely factored out of
get_selectable_products, but...)

Comment 9

[Frédéric Buclin]

Yes, that the way I want it. As soon as $user->get_selectable_products has been
called, all future requests about products won't involve additional SQL queries.
This way of doing it is similar to what $user->groups and $user->in_group do.

Comment 10

[Frédéric Buclin]

Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.79; previous revision: 1.78
done