Last Comment Bug 788098 - Queries involving group substitution crash when usevisibilitygroups is enabled
: Queries involving group substitution crash when usevisibilitygroups is enabled
Status: RESOLVED FIXED
: regression
Product: Bugzilla
Classification: Server Software
Component: Query/Bug List (show other bugs)
: 4.2
: All All
: -- normal (vote)
: Bugzilla 4.2
Assigned To: Frédéric Buclin
: default-qa
Mentors:
Depends on: bz-search-args
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-03 23:26 PDT by shirphy
Modified: 2012-10-04 08:49 PDT (History)
2 users (show)
LpSolit: approval+
LpSolit: approval4.4+
LpSolit: blocking4.4+
LpSolit: approval4.2+
LpSolit: blocking4.2.4+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (2.35 KB, patch)
2012-09-04 03:52 PDT, Frédéric Buclin
no flags Details | Diff | Review
patch for 4.4 and trunk, v1.1 (2.35 KB, patch)
2012-10-03 12:11 PDT, Frédéric Buclin
dkl: review+
Details | Diff | Review
patch for 4.2, v1 (2.35 KB, patch)
2012-10-03 12:18 PDT, Frédéric Buclin
dkl: review+
Details | Diff | Review

Description shirphy 2012-09-03 23:26:01 PDT
User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.2; Trident/4.0; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729)

Steps to reproduce:

I want to query all bug submitted by a group's members, and I use the advanced query, and set the Reporter is equal to %group.groupleader%.


Actual results:

I get a error:
Can't call method "id" without a package or object reference at Bugzilla/Group.pm line 192, <DATA> line 558.
For help, please send mail to the webmaster (shirphy74@126.com), giving this error message and the time and date of the error.

But if I set usevisibilitygroups:Off, then query is oK.


Expected results:

According to the doc of bugzilla user guider, I should have get the query list.
Comment 1 Frédéric Buclin 2012-09-04 02:57:04 PDT
Works fine in 4.0.8, but fails in 4.5. I guess the rewrite of Search.pm regressed this. Thanks for catching that!
Comment 2 Frédéric Buclin 2012-09-04 03:17:29 PDT
It's indeed a regression due to bug 574556. This code in Bugzilla::Group->check_members_are_visible() is wrong:

 my $is_visible = grep { $_->id == $_ } @{ $user->visible_groups_inherited };

This should be { $self->id == $_ }.
Comment 3 Frédéric Buclin 2012-09-04 03:52:26 PDT
Created attachment 658043 [details] [diff] [review]
patch, v1

There were several bugs related to check_members_are_visible() besides the code in this method itself: first, we were disclosing if you were able to see members of the group before making sure you were allowed to know about the existence of the group. So CVE-2011-2979 was not fully fixed by bug 674497. Secondly, the regexp used before calling _contact_exact_group() was incomplete and didn't match the one used in this method, which could lead to empty group names (e.g. if you missed the trailing %).
Comment 4 shirphy 2012-09-04 22:58:33 PDT
Thanks very much
It's worked.
Comment 5 Byron Jones ‹:glob› 2012-09-05 00:58:21 PDT
reopening; we need to get this fix into bugzilla itself, not just your installation :)
Comment 6 shirphy 2012-09-05 01:30:36 PDT
I got a new error when I set the query in Advanced Query with the following
[Assignee] [is equal to] [%group.APPS%]
The Error is:
The group you specified, APPS, is not valid here.

But if I modify the APPS group with the setting:
Groups That Are a Member of This Group
("Users in X are automatically in APPS")
admin
(Means admin users are automatically in APPS)

Then the Query can run and get the result list.
It seems any group must contain admin group?

Since admin group is also in the APPS group, there will be a lot of 
Bugs not in the group APPS but in the admin group. 
So I have to add another additional query limit:
[Assignee] [is not equal to] [admin]
Then the result is what I want. 

So my question is if can I just use a query limit ([Assignee] [is equal to] [%group.APPS%])to get the result?

Thank you very much.
Comment 7 Frédéric Buclin 2012-09-05 03:47:16 PDT
(In reply to shirphy from comment #6)
> So my question is if can I just use a query limit ([Assignee] [is equal to]
> [%group.APPS%])to get the result?

This is not the right place to ask for help. This bug-tracker is to track bugs only. For help, please visit http://www.bugzilla.org/support.
Comment 8 David Lawrence [:dkl] 2012-09-07 08:54:02 PDT
Comment on attachment 658043 [details] [diff] [review]
patch, v1

Review of attachment 658043 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Group.pm
@@ +173,2 @@
>      my $self = shift;
>      my $user = Bugzilla->user;

Just noticed when looking at this method, it would be nice if this would take $user as a parameter so it could be used outside of the context of the current user:

my ($self, $user) = @_;
$user ||= Bugzilla->user;

but another bug for that.

::: Bugzilla/Search.pm
@@ +2102,5 @@
>      my ($self, $args) = @_;
>      my $value = $args->{value};
>      my $user = $self->_user;
> +
> +    if ($value =~ /^\%group\.(?:[^%]+)%$/) {

Nit: Could this be simply /^%group\.[^%]+%$/ without the grouping parens? I don't see the need.

@@ +2124,5 @@
>      $value =~ /\%group\.([^%]+)%/;
> +    my $group_name = $1;
> +    my $group = Bugzilla::Group->check({ name => $group_name, _error => 'invalid_group_name' });
> +    # Pass $group_name instead of $group->name to the error message
> +    # to not leak the existence of the group.

This part needs explaining to me. Group->check will already complain if the group does not exist. So passing $group_name to ThrowUserError for $user->in_group in order to not give away the existence of the group seems redundant and unnecessary. So why not just pass $group->name as before?

I agree with the other change of moving in_group above check_members_are_visible().
Comment 9 Frédéric Buclin 2012-09-07 09:36:24 PDT
(In reply to David Lawrence [:dkl] from comment #8)
> Nit: Could this be simply /^%group\.[^%]+%$/ without the grouping parens?

Ah, sure, good point! :)


> This part needs explaining to me. Group->check will already complain if the
> group does not exist. So passing $group_name to ThrowUserError for
> $user->in_group in order to not give away the existence of the group seems
> redundant and unnecessary. So why not just pass $group->name as before?

Because if you pass "secret_group" and the group name is "Secret_Group" and we pass $group->name to the error message, then we would leak that the group exists, because the case of the output would be different than the input string. If the group doesn't exist OR the user doesn't belong to this group, then we don't want to leak the existence of the group, which is why we must display what the user typed.
Comment 10 David Lawrence [:dkl] 2012-10-03 12:01:15 PDT
Ok r=dkl for bugzilla/trunk but patch doesn't apply cleanly to bugzilla/4.2 so holding off:

patching file Bugzilla/Group.pm
Hunk #1 succeeded at 189 (offset 16 lines).
patching file Bugzilla/Search.pm
Hunk #1 succeeded at 2050 (offset -38 lines).
Hunk #2 FAILED at 2106.
1 out of 2 hunks FAILED -- saving rejects to file Bugzilla/Search.pm.rej

dkl
Comment 11 Frédéric Buclin 2012-10-03 12:11:22 PDT
Created attachment 667590 [details] [diff] [review]
patch for 4.4 and trunk, v1.1

I fixed the regexp to remove the useless (?: ).
Comment 12 Frédéric Buclin 2012-10-03 12:18:31 PDT
Created attachment 667594 [details] [diff] [review]
patch for 4.2, v1

Same patch as for trunk/4.4. The only difference is that a line had some extra trailing whitespaces which were removed in 4.4.
Comment 13 David Lawrence [:dkl] 2012-10-03 12:25:23 PDT
Comment on attachment 667590 [details] [diff] [review]
patch for 4.4 and trunk, v1.1

r=dkl from previous review
Comment 14 David Lawrence [:dkl] 2012-10-03 12:30:57 PDT
Comment on attachment 667594 [details] [diff] [review]
patch for 4.2, v1

r=dkl for 4.2 as well
Comment 15 Frédéric Buclin 2012-10-04 08:49:02 PDT
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Group.pm
modified Bugzilla/Search.pm
Committed revision 8411.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.4/
modified Bugzilla/Group.pm
modified Bugzilla/Search.pm
Committed revision 8406.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.2/
modified Bugzilla/Group.pm
modified Bugzilla/Search.pm
Committed revision 8144.

Note You need to log in before you can comment on or make changes to this bug.