286360 - ANSI SQL does not allow aliases to be used in HAVING clause

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Tomas Kopal]

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.

Comment 1

[Tomas Kopal]

Attachment: V1

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.

Comment 2

[Tomas Kopal]

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

Comment 3

[Tomas Kopal]

Attachment: V1.1

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 4

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

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)?

Comment 5

[Tomas Kopal]

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

Comment 6

[Tomas Kopal]

Attachment: V2

Comment 7

[Joel Peshkin]

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

Comment 8

[Tomas Kopal]

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

Comment 9

[Tomas Kopal]

Attachment: V3

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 10

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

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.

Comment 11

[Joel Peshkin]

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

Comment 12

[Tomas Kopal]

Attachment: V3.1

Fixed review points.

Comment 13

[Joel Peshkin]

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.

Comment 14

[Tomas Kopal]

Attachment: V3.2

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

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

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) {
   ...
}

Comment 16

[Joel Peshkin]

Yeah, the -1 trick is....

join(',', (-1, values(%{Bugzilla->user->groups})))

Comment 17

[Tomas Kopal]

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

Comment 18

[Tomas Kopal]

Attachment: V3.3

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

Comment 19

[Joel Peshkin]

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

r=joel

Comment 20

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

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

looks good, r=myk

Comment 21

[Max Kanat-Alexander]

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