diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm index 9590d1884e..5247e51f06 100644 --- a/Bugzilla/Search.pm +++ b/Bugzilla/Search.pm @@ -1458,6 +1458,20 @@ sub _standard_joins { extra => ['security_cc.who = ' . $user->id], }; push @joins, $security_cc_join; + + # Triage owners can see all bugs in their component, but only if they are + # also a member of the mozilla-employee-confidential group. + if ($user->is_employee_confidential) { + my $security_triage_join = { + table => 'components', + as => 'security_triage', + from => 'bugs.component_id', + to => 'id', + join => 'LEFT', + extra => ['security_triage.triage_owner_id = ' . $user->id], + }; + push @joins, $security_triage_join; + } } return @joins; @@ -1538,6 +1552,12 @@ sub _standard_where { if (Bugzilla->params->{'useqacontact'}) { push @involved, ("bugs.qa_contact = $userid"); } + + # This must stay in sync with the security_triage join in _standard_joins, + # which is only present for confidential-group members. + if ($self->_user->is_employee_confidential) { + push @involved, ('security_triage.triage_owner_id IS NOT NULL'); + } $term .= ' OR (' . join(') OR (', @involved) . ')'; } diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm index fb57db004a..5387c88d55 100644 --- a/Bugzilla/User.pm +++ b/Bugzilla/User.pm @@ -1548,12 +1548,14 @@ sub visible_bugs { # same result for bug_group_map.bug_id (so DISTINCT filters # out duplicate rows). "SELECT DISTINCT bugs.bug_id, reporter, assigned_to, qa_contact, - reporter_accessible, cclist_accessible, cc.who, - bug_group_map.bug_id + components.triage_owner_id, reporter_accessible, + cclist_accessible, cc.who, bug_group_map.bug_id FROM bugs LEFT JOIN cc ON cc.bug_id = bugs.bug_id AND cc.who = $user_id + LEFT JOIN components + ON bugs.component_id = components.id LEFT JOIN bug_group_map ON bugs.bug_id = bug_group_map.bug_id AND bug_group_map.group_id NOT IN (" @@ -1567,13 +1569,20 @@ sub visible_bugs { $sth->execute(@check_ids); my $use_qa_contact = Bugzilla->params->{'useqacontact'}; + + # Triage owners can see all bugs in their component, but only if they are + # also a member of the mozilla-employee-confidential group. + my $use_triage_owner = $self->is_employee_confidential; while (my $row = $sth->fetchrow_arrayref) { - my ($bug_id, $reporter, $owner, $qacontact, $reporter_access, $cclist_access, - $isoncclist, $missinggroup) - = @$row; + my ( + $bug_id, $reporter, $owner, + $qacontact, $triage_owner, $reporter_access, + $cclist_access, $isoncclist, $missinggroup + ) = @$row; $visible_cache->{$bug_id} ||= ((($reporter == $user_id) && $reporter_access) - || ($use_qa_contact && $qacontact && ($qacontact == $user_id)) + || ($use_qa_contact && $qacontact && ($qacontact == $user_id)) + || ($use_triage_owner && $triage_owner && ($triage_owner == $user_id)) || ($owner == $user_id) || ($isoncclist && $cclist_access) || !$missinggroup) ? 1 : 0; @@ -2580,6 +2589,16 @@ sub is_insider { return $self->{'is_insider'}; } +sub is_employee_confidential { + my $self = shift; + + if (!defined $self->{'is_employee_confidential'}) { + $self->{'is_employee_confidential'} + = $self->in_group('mozilla-employee-confidential') ? 1 : 0; + } + return $self->{'is_employee_confidential'}; +} + sub is_global_watcher { my $self = shift; @@ -3482,6 +3501,10 @@ for flag mail. Returns true if the user can access private comments and attachments, i.e. if the 'insidergroup' parameter is set and the user belongs to this group. +=item C + +Returns true if the user belongs to the 'mozilla-employee-confidential' group. + =item C Returns true if the user is a global watcher, diff --git a/extensions/BugModal/template/en/default/bug_modal/groups.html.tmpl b/extensions/BugModal/template/en/default/bug_modal/groups.html.tmpl index 48a28b3664..ad18dd3c21 100644 --- a/extensions/BugModal/template/en/default/bug_modal/groups.html.tmpl +++ b/extensions/BugModal/template/en/default/bug_modal/groups.html.tmpl @@ -108,8 +108,9 @@ [% " disabled=\"disabled\"" UNLESS user_can_edit_accessible %]> - The assignee [% IF (Param('useqacontact')) %]and QA contact[% END %] - can always see [% terms.abug %], and this section does not take effect + The assignee[% IF (Param('useqacontact')) %], QA contact,[% END %] + and triage owner (when a member of mozilla-employee-confidential) can + always see [% terms.abug %], and this section does not take effect unless the [% terms.bug %] is restricted to at least one group. [% END %] diff --git a/t/bmo/triage-owner-security-visibility.t b/t/bmo/triage-owner-security-visibility.t new file mode 100644 index 0000000000..c6852a3559 --- /dev/null +++ b/t/bmo/triage-owner-security-visibility.t @@ -0,0 +1,290 @@ +#!/usr/bin/env perl +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. +# +# This Source Code Form is "Incompatible With Secondary Licenses", as +# defined by the Mozilla Public License, v. 2.0. + +# Regression test for Bug 2065387: a component's triage owner may see +# group-restricted bugs in that component, but only while they are also a +# member of the mozilla-employee-confidential group. +# +# The rule is implemented twice and the two copies must stay synchronized: +# +# * Bugzilla::User::visible_bugs - direct bug access (show_bug, REST get) +# * Bugzilla::Search - the security_triage join in +# _standard_joins plus the matching term +# in _standard_where +# +# Every case below is therefore asserted through both paths. If the join is +# ever added without the WHERE term (or vice versa) the search assertions +# break, and if either path drops the group check the negative assertions +# break. + +use 5.10.1; +use strict; +use warnings; +use lib qw(. lib local/lib/perl5); +use Test::More; + +use Bugzilla; +use Bugzilla::Bug; +use Bugzilla::Component; +use Bugzilla::Constants; +use Bugzilla::Group; +use Bugzilla::Product; +use Bugzilla::Search; +use Bugzilla::User; +BEGIN { Bugzilla->extensions } + +Bugzilla->usage_mode(USAGE_MODE_TEST); +Bugzilla->error_mode(ERROR_MODE_DIE); + +my $dbh = Bugzilla->dbh; +my $pid = $$; + +my $confidential + = Bugzilla::Group->new({name => 'mozilla-employee-confidential'}); +plan skip_all => 'mozilla-employee-confidential group required' + unless $confidential; + +my $admin = Bugzilla::User->check({id => 1}); +Bugzilla->set_user($admin); + +# The helpers below grant and revoke group membership through set_groups(), +# which requires the acting user to have bless rights on the group. +plan skip_all => 'admin cannot bless ' . $confidential->name + unless $admin->can_bless($confidential->id); + +my ($product) = grep { @{$_->versions} } Bugzilla::Product->get_all; +plan skip_all => 'Need a product with at least one version' unless $product; + +############################################################################### +# Helpers +############################################################################### + +# set_groups() checks Bugzilla->user->can_bless(), which caches its group list +# on the user object. The fixtures create a group after that cache would have +# been filled, so re-read the admin before each change. +sub as_admin { + $admin = reload($admin); + Bugzilla->set_user($admin); +} + +# set_groups() stashes the change and update() applies it, writes the audit and +# profiles_activity rows, and invalidates the memcached group list. The admin +# group is granted bless on every group at creation time, so the admin may make +# both of these changes. +sub add_to_group { + my ($user, $group) = @_; + as_admin(); + my $target = reload($user); + $target->set_groups({add => [$group->name]}); + $target->update(); +} + +sub remove_from_group { + my ($user, $group) = @_; + as_admin(); + my $target = reload($user); + $target->set_groups({remove => [$group->name]}); + $target->update(); +} + +sub test_user { + my ($login) = @_; + my $user = Bugzilla::User->new({name => $login}); + return $user if $user; + return Bugzilla::User->create({ + login_name => $login, + realname => $login, + cryptpassword => 'triage-owner-test-passw0rd!', + disabledtext => '', + disable_mail => 1, + }); +} + +# Always re-read the user so neither the object cache, the per-object group +# list, nor the per-object _visible_bugs_cache can mask a permission change. +sub reload { + my ($user) = @_; + return Bugzilla::User->new({id => $user->id}); +} + +sub can_see { + my ($user, $bug_id) = @_; + return reload($user)->can_see_bug($bug_id) ? 1 : 0; +} + +sub search_finds { + my ($user, $bug_id) = @_; + my $searcher = reload($user); + Bugzilla->set_user($searcher); + my $search = Bugzilla::Search->new( + fields => ['bug_id'], + params => {f1 => 'bug_id', o1 => 'equals', v1 => $bug_id}, + user => $searcher, + ); + my $found = grep { $_->[0] == $bug_id } @{$search->data}; + Bugzilla->set_user($admin); + return $found ? 1 : 0; +} + +############################################################################### +# Fixtures +############################################################################### + +# The bug is restricted by a throwaway group that none of the triage owners +# belong to. Restricting it with mozilla-employee-confidential itself would let +# the triage owner in through ordinary group membership and prove nothing about +# the triage-owner rule. +my $sec_group = Bugzilla::Group->create({ + name => "test-triage-sec-$pid", + description => 'Temp security group for Bug 2065387 test', + isbuggroup => 1, +}); +$dbh->do( + 'INSERT IGNORE INTO group_control_map + (group_id, product_id, entry, membercontrol, othercontrol, canedit) + VALUES (?, ?, 0, 1, 0, 0)', undef, $sec_group->id, $product->id +); + +# The admin needs the group to be able to file the restricted bug. +add_to_group($admin, $sec_group); +as_admin(); + +my $owner_member = test_user("triage-member-$pid\@triage.test"); +my $owner_nonmember = test_user("triage-nonmember-$pid\@triage.test"); +my $owner_other = test_user("triage-other-$pid\@triage.test"); + +add_to_group($owner_member, $confidential); +add_to_group($owner_other, $confidential); + +# $owner_nonmember is deliberately left out of mozilla-employee-confidential. +remove_from_group($owner_nonmember, $confidential); + +# initialowner is the admin on both components so that a triage owner never +# picks up access as the default assignee instead. +my $comp_target = Bugzilla::Component->create({ + product => $product, + name => "TriageOwnerTarget-$pid", + description => 'Temp component for Bug 2065387 test', + initialowner => $admin->login, + team_name => 'Mozilla', + triage_owner_id => $owner_member->login, +}); +my $comp_other = Bugzilla::Component->create({ + product => $product, + name => "TriageOwnerOther-$pid", + description => 'Temp component for Bug 2065387 test', + initialowner => $admin->login, + team_name => 'Mozilla', + triage_owner_id => $owner_other->login, +}); + +my $bug = Bugzilla::Bug->create({ + short_desc => "Triage owner visibility - Bug 2065387 - $pid", + product => $product->name, + component => $comp_target->name, + bug_type => 'defect', + bug_severity => 'normal', + op_sys => 'Unspecified', + rep_platform => 'Unspecified', + version => $product->versions->[0]->name, + groups => [$sec_group->name], +}); + +############################################################################### +# Tests +############################################################################### + +ok( + (grep { $_->name eq $sec_group->name } @{$bug->groups_in}), + 'Test bug ' . $bug->id . ' is restricted to ' . $sec_group->name +); + +# --- Positive: triage owner who is in mozilla-employee-confidential --- + +ok(can_see($owner_member, $bug->id), + 'Triage owner in mozilla-employee-confidential can see the restricted bug'); +ok( + search_finds($owner_member, $bug->id), + 'Triage owner in mozilla-employee-confidential finds the restricted bug via search' +); + +# --- Negative: triage owner of a different component --- +# +# In mozilla-employee-confidential, but triage owner of the wrong component, +# so the join must not match. + +ok(!can_see($owner_other, $bug->id), + 'Triage owner of a different component cannot see the restricted bug'); +ok( + !search_finds($owner_other, $bug->id), + 'Triage owner of a different component does not find the restricted bug via search' +); + +# --- Negative: triage owner of the right component, not in the group --- + +$comp_target->set_triage_owner($owner_nonmember->login); +$comp_target->update(); + +ok( + !can_see($owner_nonmember, $bug->id), + 'Triage owner outside mozilla-employee-confidential cannot see the restricted bug' +); +ok( + !search_finds($owner_nonmember, $bug->id), + 'Triage owner outside mozilla-employee-confidential does not find the restricted bug via search' +); + +# --- Negative: same user and component, group membership revoked --- +# +# The strongest form of the check. Only the group membership changes between +# the passing assertions above and these, so nothing else can explain a pass. + +$comp_target->set_triage_owner($owner_member->login); +$comp_target->update(); +remove_from_group($owner_member, $confidential); + +ok(!can_see($owner_member, $bug->id), + 'Revoking mozilla-employee-confidential revokes the triage owner bug access'); +ok( + !search_finds($owner_member, $bug->id), + 'Revoking mozilla-employee-confidential removes the bug from triage owner search results' +); + +# Re-granting restores access, confirming the previous failures were caused by +# the group check and not by leftover state from set_triage_owner(). +add_to_group($owner_member, $confidential); + +ok( + can_see($owner_member, $bug->id), + 'Re-granting mozilla-employee-confidential restores the triage owner bug access' +); +ok( + search_finds($owner_member, $bug->id), + 'Re-granting mozilla-employee-confidential restores the bug in triage owner search results' +); + +############################################################################### +# Cleanup +############################################################################### + +Bugzilla->set_user($admin); +$bug->remove_from_db(); +$comp_target->remove_from_db(); +$comp_other->remove_from_db(); + +foreach my $user ($owner_member, $owner_nonmember, $owner_other) { + remove_from_group($user, $confidential); +} +remove_from_group($admin, $sec_group); + +$dbh->do('DELETE FROM group_control_map WHERE group_id = ?', + undef, $sec_group->id); +$dbh->do('DELETE FROM bug_group_map WHERE group_id = ?', undef, $sec_group->id); +$sec_group->remove_from_db(); + +done_testing();