The default bug view has changed. See this FAQ.

Queries involving group substitution crash when usevisibilitygroups is enabled

RESOLVED FIXED in Bugzilla 4.2

Status

()

Bugzilla
Query/Bug List
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: shirphy, Assigned: Frédéric Buclin)

Tracking

({regression})

Bugzilla 4.2
regression
Bug Flags:
approval +
approval4.4 +
blocking4.4 +
approval4.2 +
blocking4.2.4 +

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

5 years ago
Works fine in 4.0.8, but fails in 4.5. I guess the rewrite of Search.pm regressed this. Thanks for catching that!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking4.4+
Flags: blocking4.2.4+
Keywords: regression
Target Milestone: --- → Bugzilla 4.2
(Assignee)

Comment 2

5 years ago
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 == $_ }.
Depends on: 574556
(Assignee)

Comment 3

5 years ago
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 %).
Assignee: query-and-buglist → LpSolit
Status: NEW → ASSIGNED
Attachment #658043 - Flags: review?(dkl)
(Reporter)

Comment 4

5 years ago
Thanks very much
It's worked.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME
reopening; we need to get this fix into bugzilla itself, not just your installation :)
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Reporter)

Comment 6

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

Comment 7

5 years ago
(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.
Status: REOPENED → ASSIGNED
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().
(Assignee)

Comment 9

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

Comment 11

5 years ago
Created attachment 667590 [details] [diff] [review]
patch for 4.4 and trunk, v1.1

I fixed the regexp to remove the useless (?: ).
Attachment #658043 - Attachment is obsolete: true
Attachment #658043 - Flags: review?(dkl)
Attachment #667590 - Flags: review?(dkl)
(Assignee)

Comment 12

5 years ago
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.
Attachment #667594 - Flags: review?(dkl)
Comment on attachment 667590 [details] [diff] [review]
patch for 4.4 and trunk, v1.1

r=dkl from previous review
Attachment #667590 - Flags: review?(dkl) → review+
Comment on attachment 667594 [details] [diff] [review]
patch for 4.2, v1

r=dkl for 4.2 as well
Attachment #667594 - Flags: review?(dkl) → review+

Updated

5 years ago
Flags: approval?
Flags: approval4.4?
Flags: approval4.2?
(Assignee)

Updated

5 years ago
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval4.2?
Flags: approval4.2+
Flags: approval+
(Assignee)

Comment 15

5 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Summary: Group Query get a error when set usevisibilitygroups:on → Queries involving group substitution crash when usevisibilitygroups is enabled
You need to log in before you can comment on or make changes to this bug.