46002 - reports.cgi ignores group restrictions with usebuggroups on

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P3)

Description

[Joe Robins]

I noticed the other day that reports.cgi doesn't check for the usebuggroups 
parameter, and thus doesn't restrict access to reports for products by bug group 
if it's on.  I consider this a bug, and so I've corrected it on my local 
installation.

If people don't have permission to access the bugs of a product, they shouldn't 
be able to get reports on that product either, right?

Comment 1

[Dave Miller [:justdave]]

I'll take this.  Since you said you've corrected it on your site, I assume you 
can post a patch?

Comment 2

[Joe Robins]

Attachment: Patch to make reports.cgi respect usebuggroups parameter

Comment 3

[Dave Miller [:justdave]]

Question....  since you're the one that invented the usebuggroups and 
usebuggroupsentry params, what specifically is each supposed to restrict?  when I 
added a similar patch to this to query.cgi to keep it out of the product 
selection box on the query screen, I used the 'usebuggroupsentry' param.  Is that 
what I should have used?  Judging by this patch, I should have used the 
'usebuggroups' one...

Also, I don't like the confirm_login() in there.  Now anyone that doesn't have an 
account can't get reports, not even for publicly visible products.  (I have some 
of each on my system).  Better to do the quietly_check_login() to see if they are 
logged in or not, and just pretend they don't have access if they're not logged 
in.  Anyone who wants to run a report on a restricted product will know they need 
to log in first to see it.

Comment 4

[Joe Robins]

1) I hadn't looked at your patch to query.cgi, so I didn't realize that you were 
doing it that way.  (Can you let me know the bug that that's attached to?  I'd 
like to look at it, and probably add it to my installation...)  The intended use 
was for "usebuggroups" to be used for querying and viewing bugs, and 
"usebuggroupsentry" to be for entering new bugs.  The reasoning was that you 
might want to have people testing something who can enter new bugs, but you 
wouldn't want them to be querying other bugs.  If you turned on "usebuggroups" 
but left "usebuggroupsentry" off, then they could enter bugs against a product 
without having permissions for that product.  (It occurs to me now that this 
would also prevent them from returning to the bug that they themselves had 
entered, and that might be a bad thing.  Maybe I should create a new bug for 
that and look into it...)

So in the case of query.cgi, and in the reports.cgi patch I checked in, it would 
seem to me that "usebuggroups" would be the parameter you'd want, not 
"usebuggroupsentry".

2) You're right...  That thought hadn't occured to me, but using 
quietly_check_login() would be just fine.

However, in looking at that a bit, I realized that my patch is flawed;  I forgot 
to deal with the "-All-" case, so if a user doesn't have permissions to any 
groups, they can still see full stats by selecting the "-All-" option.  I'll fix 
this case, and switch the login mechanism as well, and I'll post a new patch 
when that's done.

Comment 5

[Dave Miller [:justdave]]

The query.cgi patch is in bug 45586.  If you're updating from CVS, it's already 
been checked in, so just do a 'cvs update' and you'll have it.

I don't see any product-specific information in any of the current reports, so I 
don't think "-All-" will be much of a concern.  It gives you an accurate picture 
of the workload a given engineer has, without having to know exactly what he's 
working on.  If the same engineer is responsible for "A", which is public, and 
"B", which is private, and he has 2 open bugs in "A", and 50 in "B", and the user 
runs a report on -All- that is restricted to what he can see, it looks like that 
engineer only has 2 bugs.  Now this user will try to claim the engineer is being 
lazy because he won't look at his 2 bugs....   :-)

See bug 39816 for a proposed solution to your problem with someone not being able 
to see bugs they entered if they didn't have access to that product.

Comment 6

[Joe Robins]

Yeah, you've got a point.  And since they won't be able to actually view the 
bugs if they click on them, all they see are bug IDs, so that's not really a 
problem.

So the only change needed to the patch is to change the login mechanism called.  
Should I put a new patch up, or can whoever checks it in change the patch 
themselves before doing it?

Comment 7

[Dave Miller [:justdave]]

That's straight-forward enough, I think I can fix it when I check it in.

Comment 8

[Dave Miller [:justdave]]

Fixed and checked in.

Comment 9

[Dave Miller [:justdave]]

In search of accurate queries....  (sorry for the spam)

Comment 10

[Dave Miller [:justdave]]

Moving closed bugs to Bugzilla product