288557 - show in the request queue those requests tied to secure bugs for which the user is the QA contact

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

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

request.cgi lines 81-87 exclude from the queue any requests tied to bugs the
user doesn't have access to, but we don't check if the user is the QA contact
for any of the bugs, even though QA contacts are allowed to see their secure
bugs.  We should show requests tied to secure bugs for which the user is the QA
contact.

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

Comment 2

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

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

>-    
>-    my $attach_join_clause = "flags.attach_id = attachments.attach_id";
>+
>+    my $attach_join_clause = "";
>     if (Param("insidergroup") && !UserInGroup(Param("insidergroup"))) {
>-        $attach_join_clause .= " AND attachments.isprivate < 1";
>+        $attach_join_clause = " AND attachments.isprivate < 1 ";

What's the purpose of this change?  It makes the variable name
"attach_join_clause" inaccurate, since it now contains only part of the clause.


>-           LEFT JOIN products
>+          INNER JOIN products
>                   ON bugs.product_id = products.id
>-           LEFT JOIN components
>+          INNER JOIN components

What is the purpose of these changes?


>-                  ON ccmap.who = $::userid
>-                 AND ccmap.bug_id = bugs.bug_id
>-    " .
>+                  ON ccmap.bug_id = bugs.bug_id
>+                 AND ccmap.who = $userid" .

Nit: it'd be good to keep some whitespace in between the end of this condition
and its closing quotation mark.  Then, if someone later inserts an additional
conditional, here, we don't run the risk of that change missing the lack of
whitespace and creating a bug.


>+                 (bugs.qa_contact IS NOT NULL AND bugs.qa_contact = $userid))";

Isn't bugs.qa_contact guaranteed to != $userid when bugs.qa_contact IS NULL?

Comment 3

[Frédéric Buclin]

(In reply to comment #2)

> >-    my $attach_join_clause = "flags.attach_id = attachments.attach_id";
> >+
> >+    my $attach_join_clause = "";
> >     if (Param("insidergroup") && !UserInGroup(Param("insidergroup"))) {
> >-        $attach_join_clause .= " AND attachments.isprivate < 1";
> >+        $attach_join_clause = " AND attachments.isprivate < 1 ";
> 
> What's the purpose of this change?  It makes the variable name
> "attach_join_clause" inaccurate, since it now contains only part of the clause.

The first part of the clause appears in all cases. I much prefer to see it in
the SQL statement directly. This way, I avoid to scroll to see what it means.


> >-           LEFT JOIN products
> >+          INNER JOIN products
> >                   ON bugs.product_id = products.id
> >-           LEFT JOIN components
> >+          INNER JOIN components
> 
> What is the purpose of these changes?

In case of data corruption, especially on product and component deletion, some
references may be invalid (known bug in 2.18). In this case, I don't want the
bug to become visible. The INNER JOIN ensures that the bug is in a valid product
and component.


> >+                 (bugs.qa_contact IS NOT NULL AND bugs.qa_contact = $userid))";
> 
> Isn't bugs.qa_contact guaranteed to != $userid when bugs.qa_contact IS NULL?

I thought about that too and I think you are right. But I really wanted to make
sure that a non-authenticated user ($userid = 0) cannot see bugs because of some
unknown reason, which could be DB-related.

Comment 4

[Frédéric Buclin]

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

>+                 (bugs.reporter = $userid AND bugs.reporter_accessible = 1) OR
>+                 (bugs.assigned_to = $userid) OR
>+                 (bugs.qa_contact IS NOT NULL AND bugs.qa_contact = $userid))";


Hum... We shouldn't look at the QA contact if Param(useqacontact) is turned
off.

Comment 5

[Frédéric Buclin]

Attachment: patch, v2

This patch doesn't let the QA contact see requests if 'useqacontact' is off.

Comment 6

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

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

Looks good; r=myk

Comment 7

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

Valuable, low-risk polish fix.

Comment 8

[Frédéric Buclin]

Attachment: backport for 2.20, v1

2.20 still uses $::userid instead of $userid. Except this point, the patch is the same as for the tip.

Comment 9

[Frédéric Buclin]

tip:

Checking in request.cgi;
/cvsroot/mozilla/webtools/bugzilla/request.cgi,v  <--  request.cgi
new revision: 1.29; previous revision: 1.28
done

2.20:

Checking in request.cgi;
/cvsroot/mozilla/webtools/bugzilla/request.cgi,v  <--  request.cgi
new revision: 1.23.2.1; previous revision: 1.23
done