Bug 285555 is dealing with one case where we use HAVING <alias>, but there are other two cases, in request.cgi and Search.pm. We need to fix them as well, either by re-structuring the query to avoid using HAVING, or by repeating the whole expression in the HAVING clause.

I have removed the HAVING clause from request.cgi, the WHERE clause I created there should have the same function. For Search.pm, there are two occurences - flags and percent_complete. flag matching could be probably converted not to use HAVING, but I think there is no way to convert the percent_complete (without subselects), so I left both to use HAVING, just removed the alias. The patch actually contains one other small fix, missing braces aroung text concatenation, if that's a problem, I will remove it and raise a new bug for it.

One more note: I have done only limited testing on Postgres, I plan to do more testing tonight on MySQL.

OK, I have tested this on MySQL and simplified it even more. I have also removed the fix for the braces, I will raise a separate bug for that.

Comment on attachment 177601 [details] [diff] [review] V1.1 This patch won't apply to my tip install: [myk@myk bztip]$ cvs diff -u request.cgi Bugzilla/Search.pm [myk@myk bztip]$ patch -p0 --dry-run < dbcompat-having-20050316-2.patch patching file request.cgi patching file Bugzilla/Search.pm Hunk #2 FAILED at 811. 1 out of 2 hunks FAILED -- saving rejects to file Bugzilla/Search.pm.rej >- # Select columns that help us weed out secure bugs to which the user >- # should not have access. >- " COUNT(DISTINCT ugmap.group_id) AS cntuseringroups, >- COUNT(DISTINCT bgmap.group_id) AS cntbugingroups, >- ((COUNT(DISTINCT ccmap.who) > 0 AND cclist_accessible = 1) >- OR ((bugs.reporter = $::userid) AND bugs.reporter_accessible = 1) >- OR bugs.assigned_to = $::userid ) AS canseeanyway It's not this bug, but shouldn't QA contact be part of this? >+ # only records the use has access to Nit: use -> user >+ $query .= " AND ((bgmap.group_id IS NULL AND ugmap.group_id IS NULL) OR >+ (bgmap.group_id = ugmap.group_id) OR >+ ((ccmap.who IS NOT NULL AND cclist_accessible = 1) OR >+ (bugs.reporter = $::userid AND bugs.reporter_accessible = 1) OR >+ bugs.assigned_to = $::userid)) "; Won't this retrieve a record if the bug is in one of the groups the user belongs to even if it's also in other groups the user doesn't belong to (and thus shouldn't be visible to that user)?

(In reply to comment #4) > This patch won't apply to my tip install: Yes, the bitrot is fast these days ;-). I will fix that. > It's not this bug, but shouldn't QA contact be part of this? > Well, I am not feeling competent to answer this question. But as you say, it's a different bug. File it and we can discuss it :-). But don't make it specific to requests.cgi, similar checks are done all over the code (User.pm comes to my mind). > >+ # only records the use has access to > > Nit: use -> user Thanks, I will fix that. > Won't this retrieve a record if the bug is in one of the groups the user > belongs to even if it's also in other groups the user doesn't belong to (and > thus shouldn't be visible to that user)? Eh, you are right. I tought it's enough to be member of one of the groups to have access, but I have read the doco now ;-). That means we can't get away from HAVING, or at least I can't see any other way. So I will revert back to HAVING, but fix it for ANSI SQL. Thanks for the comments.

(In reply to comment #5) > Eh, you are right. I tought it's enough to be member of one of the groups to > have access, but I have read the doco now ;-). > That means we can't get away from HAVING, or at least I can't see any other way. > So I will revert back to HAVING, but fix it for ANSI SQL. > Yes you can get away from HAVING. You get each user's list of groups from the User object which already exists anyway. Then, you LEFT JOIN bgmap ON bgmap.bug_id = $bugid AND bgmap.group_id NOT IN (LIST) .... WHERE bgmap.group_id IS NULL Look at Search.pm to see how this is done (near line 1340)

(In reply to comment #7) > Yes you can get away from HAVING. You get each user's list of groups from the > User object which already exists anyway. Then, you LEFT JOIN bgmap ON > bgmap.bug_id = $bugid AND bgmap.group_id NOT IN (LIST) .... WHERE > bgmap.group_id IS NULL > > Look at Search.pm to see how this is done (near line 1340) > Now, that's an idea. I knew I can fix it using second select or subselect, but I didn't want to do that for obvious reasons, but you are right that the list of groups the user is in is already available, so this should be simple. Thanks, I will look at that.

New version. I have incorporated the approach suggested by Joel, which even enabled to get rid of one join :-). I have also added the qacontact test you mentioned, but if you would prefer to deal with it in a separate bug, let me know and I will dissect it :-). BTW the first part, the 'white space' patch in Search.pm, fixes cr-lf line ending which I have introduced with different patch made on windows, so if you don't mind, leave it there.

Comment on attachment 178177 [details] [diff] [review] V3 > I have also added the qacontact test you > mentioned, but if you would prefer to deal with it in a separate bug, let me > know and I will dissect it :-). Let's deal with it in a separate bug. I've filed bug 288557 on it. >+ # Weed out bug the user does not have access to >+ " AND ((bug_group_map.group_id IS NULL) OR bug_group_map -> bgmap or the query dies with an "unknown table" error on MySQL (since you've aliased it, so it's only known by its alias). Otherwise this looks good, and a nice improvement over use of the having now that we have the User object available to us. But because this is a change to security code, I'd like to have a second set of eyes on it, so please request second review from another experienced reviewer.

(In reply to comment #10) I'd like to have a second set of eyes on it, so please request > second review from another experienced reviewer. > Aside from what myk already pointed out, your logic looks good. I'll re-check it when you have the final version.

Fixed review points.

Comment on attachment 179647 [details] [diff] [review] V3.1 + AND bgmap.group_id NOT IN (" . + join(', ', values(%{Bugzilla->user->groups})) . ") This fails if the user is logged out (no groups). Join a -1 in as well and you're set.

(In reply to comment #13) > (From update of attachment 179647 [details] [diff] [review] [edit]) > > + AND bgmap.group_id NOT IN (" . > + join(', ', values(%{Bugzilla->user->groups})) . ") > > This fails if the user is logged out (no groups). Join a -1 in as well and > you're set. > Ahhh, good catch, thanks. I have actually excluded that condition from the query completely, it seems to be more obvious in the code. I have also adjusted the formating a bit.

Comment on attachment 179655 [details] [diff] [review] V3.2 >+ if (%{Bugzilla->user->groups}) { >+ $query .= " >+ AND bgmap.group_id NOT IN (" . >+ join(', ', values(%{Bugzilla->user->groups})) . ")"; >+ } I believe this will still fail if the user is logged in but does not belong to any groups. You would need to do: if (%{Bugzilla->user->groups} && scalar(values(%{Bugzilla->user->groups})) > 0) { ... }

Yeah, the -1 trick is.... join(',', (-1, values(%{Bugzilla->user->groups})))

(In reply to comment #15) > (From update of attachment 179655 [details] [diff] [review] [edit]) > >+ if (%{Bugzilla->user->groups}) { > >+ $query .= " > >+ AND bgmap.group_id NOT IN (" . > >+ join(', ', values(%{Bugzilla->user->groups})) . ")"; > >+ } > > I believe this will still fail if the user is logged in but does not belong to > any groups. You would need to do: > > if (%{Bugzilla->user->groups} > && scalar(values(%{Bugzilla->user->groups})) > 0) { > ... > } > Well, ok, I will change it to the -1 trick. But someone should check all the other occurences of this code, it is used at least on three places in globals.pl (maybe somewhere else). How's that that it does not cause problems there?

Fixed the group membership test as suggested, no other changes.

Comment on attachment 179795 [details] [diff] [review] V3.3 r=joel

Comment on attachment 179795 [details] [diff] [review] V3.3 looks good, r=myk

Checking in request.cgi; /cvsroot/mozilla/webtools/bugzilla/request.cgi,v <-- request.cgi new revision: 1.22; previous revision: 1.21 done Checking in Bugzilla/Search.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v <-- Search.pm new revision: 1.94; previous revision: 1.93 done