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

RESOLVED FIXED in Bugzilla 2.20

Status

()

RESOLVED FIXED
14 years ago
6 years ago

People

(Reporter: Tomas.Kopal, Assigned: Tomas.Kopal)

Tracking

unspecified
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

14 years ago
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.
(Assignee)

Comment 1

14 years ago
Created attachment 177582 [details] [diff] [review]
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.
(Assignee)

Updated

14 years ago
Assignee: general → Tomas.Kopal
Status: NEW → ASSIGNED
Attachment #177582 - Flags: review?(myk)
(Assignee)

Comment 2

14 years ago
One more note: I have done only limited testing on Postgres, I plan to do more
testing tonight on MySQL.
Target Milestone: --- → Bugzilla 2.20
(Assignee)

Comment 3

14 years ago
Created attachment 177601 [details] [diff] [review]
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.
Attachment #177582 - Attachment is obsolete: true
Attachment #177601 - Flags: review?(myk)
(Assignee)

Updated

14 years ago
Attachment #177582 - Flags: review?(myk)
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)?
Attachment #177601 - Flags: review?(myk) → review-
(Assignee)

Comment 5

14 years ago
(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.
(Assignee)

Comment 6

14 years ago
Created attachment 177826 [details] [diff] [review]
V2
Attachment #177601 - Attachment is obsolete: true
Attachment #177826 - Flags: review?(myk)

Comment 7

14 years ago
(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)

(Assignee)

Comment 8

14 years ago
(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.
(Assignee)

Updated

14 years ago
Attachment #177826 - Flags: review?(myk)
(Assignee)

Comment 9

14 years ago
Created attachment 178177 [details] [diff] [review]
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.
Attachment #177826 - Attachment is obsolete: true
Attachment #178177 - Flags: review?(myk)
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.
Attachment #178177 - Flags: review?(myk) → review-

Comment 11

14 years ago
(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.
(Assignee)

Comment 12

14 years ago
Created attachment 179647 [details] [diff] [review]
V3.1

Fixed review points.
Attachment #178177 - Attachment is obsolete: true
Attachment #179647 - Flags: review?(myk)
(Assignee)

Updated

14 years ago
Attachment #179647 - Flags: review?(bugreport)

Comment 13

14 years ago
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.
Attachment #179647 - Flags: review?(bugreport) → review-
(Assignee)

Comment 14

14 years ago
Created attachment 179655 [details] [diff] [review]
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.
Attachment #179647 - Attachment is obsolete: true
Attachment #179655 - Flags: review?(myk)
(Assignee)

Updated

14 years ago
Attachment #179647 - Flags: review?(myk)
(Assignee)

Updated

14 years ago
Attachment #179655 - Flags: review?(bugreport)
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) {
   ...
}
Attachment #179655 - Flags: review?(myk)
Attachment #179655 - Flags: review?(bugreport)
Attachment #179655 - Flags: review-

Comment 16

14 years ago
Yeah, the -1 trick is....

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

Comment 17

14 years ago
(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?
(Assignee)

Comment 18

14 years ago
Created attachment 179795 [details] [diff] [review]
V3.3

Fixed the group membership test as suggested, no other changes.
Attachment #179655 - Attachment is obsolete: true
Attachment #179795 - Flags: review?(myk)
(Assignee)

Updated

14 years ago
Attachment #179795 - Flags: review?(bugreport)

Comment 19

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

r=joel
Attachment #179795 - Flags: review?(bugreport) → review+
Comment on attachment 179795 [details] [diff] [review]
V3.3

looks good, r=myk
Attachment #179795 - Flags: review?(myk) → review+
(Assignee)

Updated

14 years ago
Flags: approval?
Flags: approval? → approval+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

6 years ago
Blocks: 179489
You need to log in before you can comment on or make changes to this bug.