From bd55f598e801fe8d13cb2c8c41e3c218d8f388ad Mon Sep 17 00:00:00 2001 From: Mark Chao <mchao@gitlab.com> Date: Thu, 28 Mar 2019 13:07:50 +0000 Subject: [PATCH] Extract params filtering logic Appends hidden groups if remove_hidden_groups is not true --- .../javascripts/approvals/components/app.vue | 2 +- .../components/approvers_list_item.vue | 14 ++- .../components/hidden_groups_item.vue | 31 +++++ .../mr_edit/mr_rules_hidden_inputs.vue | 9 ++ .../approvals/components/rule_form.vue | 16 ++- .../assets/javascripts/approvals/constants.js | 1 + .../assets/javascripts/approvals/mappers.js | 2 + .../merge_requests/application_controller.rb | 17 ++- ee/app/finders/approval_rules/group_finder.rb | 27 +++++ ee/app/presenters/approval_rule_presenter.rb | 12 +- .../params_filtering_service.rb | 110 ++++++++++++++++++ .../services/approval_rules/update_service.rb | 14 ++- .../ee/merge_requests/base_service.rb | 28 +---- .../10356-private-group-handling.yml | 5 + ee/lib/api/project_approval_rules.rb | 1 + ee/lib/ee/api/entities.rb | 1 + .../user_sets_approval_rules_spec.rb | 53 +++++++++ .../approval_rules/group_finder_spec.rb | 42 +++++++ .../public_api/v4/project_approval_rule.json | 1 + .../components/approvers_list_item_spec.js | 25 +++- .../components/hidden_groups_item_spec.js | 27 +++++ .../mr_edit/mr_rules_hidden_inputs_spec.js | 16 +++ .../approvals/components/rule_form_spec.js | 88 +++++++++++++- .../approval_rule_presenter_spec.rb | 53 ++++++++- .../params_filtering_service_spec.rb | 91 +++++++++++++++ .../approval_rules/update_service_spec.rb | 55 +++++++++ .../ee/merge_requests/base_service_spec.rb | 83 ++++--------- locale/gitlab.pot | 6 + 28 files changed, 723 insertions(+), 107 deletions(-) create mode 100644 ee/app/assets/javascripts/approvals/components/hidden_groups_item.vue create mode 100644 ee/app/finders/approval_rules/group_finder.rb create mode 100644 ee/app/services/approval_rules/params_filtering_service.rb create mode 100644 ee/changelogs/unreleased/10356-private-group-handling.yml create mode 100644 ee/spec/features/merge_request/user_sets_approval_rules_spec.rb create mode 100644 ee/spec/finders/approval_rules/group_finder_spec.rb create mode 100644 ee/spec/javascripts/approvals/components/hidden_groups_item_spec.js create mode 100644 ee/spec/services/approval_rules/params_filtering_service_spec.rb diff --git a/ee/app/assets/javascripts/approvals/components/app.vue b/ee/app/assets/javascripts/approvals/components/app.vue index d76487e3cd41e..0b934d9cbfbeb 100644 --- a/ee/app/assets/javascripts/approvals/components/app.vue +++ b/ee/app/assets/javascripts/approvals/components/app.vue @@ -38,7 +38,7 @@ export default { </script> <template> - <div> + <div class="js-approval-rules"> <gl-loading-icon v-if="!hasLoaded" :size="2" /> <template v-else> <div class="border-bottom"> diff --git a/ee/app/assets/javascripts/approvals/components/approvers_list_item.vue b/ee/app/assets/javascripts/approvals/components/approvers_list_item.vue index 1ad19127ab0e9..c50cf175eb753 100644 --- a/ee/app/assets/javascripts/approvals/components/approvers_list_item.vue +++ b/ee/app/assets/javascripts/approvals/components/approvers_list_item.vue @@ -2,15 +2,17 @@ import { GlButton } from '@gitlab/ui'; import Icon from '~/vue_shared/components/icon.vue'; import Avatar from '~/vue_shared/components/project_avatar/default.vue'; -import { TYPE_USER, TYPE_GROUP } from '../constants'; +import HiddenGroupsItem from './hidden_groups_item.vue'; +import { TYPE_USER, TYPE_GROUP, TYPE_HIDDEN_GROUPS } from '../constants'; -const types = [TYPE_USER, TYPE_GROUP]; +const types = [TYPE_USER, TYPE_GROUP, TYPE_HIDDEN_GROUPS]; export default { components: { GlButton, Icon, Avatar, + HiddenGroupsItem, }, props: { approver: { @@ -23,6 +25,9 @@ export default { isGroup() { return this.approver.type === TYPE_GROUP; }, + isHiddenGroups() { + return this.approver.type === TYPE_HIDDEN_GROUPS; + }, displayName() { return this.isGroup ? this.approver.full_path : this.approver.name; }, @@ -34,7 +39,10 @@ export default { <transition name="fade"> <li class="settings-flex-row"> <div class="px-3 d-flex align-items-center"> - <avatar :project="approver" :size="24" /><span>{{ displayName }}</span> + <hidden-groups-item v-if="isHiddenGroups" /> + <template v-else> + <avatar :project="approver" :size="24" /><span>{{ displayName }}</span> + </template> <gl-button variant="none" class="ml-auto" @click="$emit('remove', approver)"> <icon name="remove" :aria-label="__('Remove')" /> </gl-button> diff --git a/ee/app/assets/javascripts/approvals/components/hidden_groups_item.vue b/ee/app/assets/javascripts/approvals/components/hidden_groups_item.vue new file mode 100644 index 0000000000000..2a55a5b1004ae --- /dev/null +++ b/ee/app/assets/javascripts/approvals/components/hidden_groups_item.vue @@ -0,0 +1,31 @@ +<script> +import { GlTooltipDirective, GlButton } from '@gitlab/ui'; +import Icon from '~/vue_shared/components/icon.vue'; + +export default { + components: { + GlButton, + Icon, + }, + directives: { + GlTooltip: GlTooltipDirective, + }, +}; +</script> + +<template> + <div class="d-flex align-items-center"> + <div class="square s24 d-flex-center mr-2 text-tertiary"> + <icon name="folder-o" :size="16" /> + </div> + <span>{{ __('Private group(s)') }}</span> + <gl-button + v-gl-tooltip + :title="__('One or more groups that you don\'t have access to.')" + variant="blank" + class="ml-1 text-secondary" + > + <icon name="question-o" :size="16" /> + </gl-button> + </div> +</template> diff --git a/ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs.vue b/ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs.vue index 0e7a9a61f1110..5e69193dcbc40 100644 --- a/ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs.vue +++ b/ee/app/assets/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs.vue @@ -8,6 +8,8 @@ const INPUT_APPROVALS_REQUIRED = 'merge_request[approval_rules_attributes][][app const INPUT_USER_IDS = 'merge_request[approval_rules_attributes][][user_ids][]'; const INPUT_GROUP_IDS = 'merge_request[approval_rules_attributes][][group_ids][]'; const INPUT_DELETE = 'merge_request[approval_rules_attributes][][_destroy]'; +const INPUT_REMOVE_HIDDEN_GROUPS = + 'merge_request[approval_rules_attributes][][remove_hidden_groups]'; const INPUT_FALLBACK_APPROVALS_REQUIRED = 'merge_request[approvals_before_merge]'; export default { @@ -26,6 +28,7 @@ export default { INPUT_USER_IDS, INPUT_GROUP_IDS, INPUT_DELETE, + INPUT_REMOVE_HIDDEN_GROUPS, INPUT_FALLBACK_APPROVALS_REQUIRED, }; </script> @@ -82,6 +85,12 @@ export default { :name="$options.INPUT_GROUP_IDS" type="hidden" /> + <input + v-if="rule.removeHiddenGroups" + value="true" + :name="$options.INPUT_REMOVE_HIDDEN_GROUPS" + type="hidden" + /> </div> </div> </template> diff --git a/ee/app/assets/javascripts/approvals/components/rule_form.vue b/ee/app/assets/javascripts/approvals/components/rule_form.vue index 9ff9566b9f20e..660a12ffd4d13 100644 --- a/ee/app/assets/javascripts/approvals/components/rule_form.vue +++ b/ee/app/assets/javascripts/approvals/components/rule_form.vue @@ -5,7 +5,7 @@ import { GlButton } from '@gitlab/ui'; import { sprintf, __ } from '~/locale'; import ApproversList from './approvers_list.vue'; import ApproversSelect from './approvers_select.vue'; -import { TYPE_USER, TYPE_GROUP } from '../constants'; +import { TYPE_USER, TYPE_GROUP, TYPE_HIDDEN_GROUPS } from '../constants'; const DEFAULT_NAME = 'Default'; @@ -31,6 +31,7 @@ export default { approversToAdd: [], showValidation: false, isFallback: false, + containsHiddenGroups: false, ...this.getInitialData(), }; }, @@ -102,6 +103,9 @@ export default { this.settings.allowMultiRule && this.isFallback && !this.name && !this.approvers.length ); }, + removeHiddenGroups() { + return this.containsHiddenGroups && !this.approversByType[TYPE_HIDDEN_GROUPS]; + }, submissionData() { return { id: this.initRule && this.initRule.id, @@ -111,6 +115,7 @@ export default { groups: this.groupIds, userRecords: this.users, groupRecords: this.groups, + removeHiddenGroups: this.removeHiddenGroups, }; }, }, @@ -191,6 +196,8 @@ export default { }; } + const { containsHiddenGroups = false, removeHiddenGroups = false } = this.initRule; + const users = this.initRule.users.map(x => ({ ...x, type: TYPE_USER })); const groups = this.initRule.groups.map(x => ({ ...x, type: TYPE_GROUP })); @@ -198,7 +205,12 @@ export default { name: this.initRule.name || '', approvalsRequired: this.initRule.approvalsRequired || 0, minApprovalsRequired: this.initRule.minApprovalsRequired || 0, - approvers: groups.concat(users), + containsHiddenGroups, + approvers: groups + .concat(users) + .concat( + containsHiddenGroups && !removeHiddenGroups ? [{ type: TYPE_HIDDEN_GROUPS }] : [], + ), }; }, }, diff --git a/ee/app/assets/javascripts/approvals/constants.js b/ee/app/assets/javascripts/approvals/constants.js index bd8532e23d6c5..d5f7c5eb016c8 100644 --- a/ee/app/assets/javascripts/approvals/constants.js +++ b/ee/app/assets/javascripts/approvals/constants.js @@ -1,5 +1,6 @@ export const TYPE_USER = 'user'; export const TYPE_GROUP = 'group'; +export const TYPE_HIDDEN_GROUPS = 'hidden_groups'; export const RULE_TYPE_FALLBACK = 'fallback'; export const RULE_TYPE_REGULAR = 'regular'; diff --git a/ee/app/assets/javascripts/approvals/mappers.js b/ee/app/assets/javascripts/approvals/mappers.js index 27b9c6ed04352..3978fdb62481f 100644 --- a/ee/app/assets/javascripts/approvals/mappers.js +++ b/ee/app/assets/javascripts/approvals/mappers.js @@ -6,6 +6,7 @@ export const mapApprovalRuleRequest = req => ({ approvals_required: req.approvalsRequired, users: req.users, groups: req.groups, + remove_hidden_groups: req.removeHiddenGroups, }); export const mapApprovalFallbackRuleRequest = req => ({ @@ -19,6 +20,7 @@ export const mapApprovalRuleResponse = res => ({ approvalsRequired: res.approvals_required, minApprovalsRequired: res.source_rule ? res.source_rule.approvals_required : 0, approvers: res.approvers, + containsHiddenGroups: res.contains_hidden_groups, users: res.users, groups: res.groups, }); diff --git a/ee/app/controllers/ee/projects/merge_requests/application_controller.rb b/ee/app/controllers/ee/projects/merge_requests/application_controller.rb index 347478ba52655..d148c9f0099fd 100644 --- a/ee/app/controllers/ee/projects/merge_requests/application_controller.rb +++ b/ee/app/controllers/ee/projects/merge_requests/application_controller.rb @@ -14,7 +14,7 @@ def merge_request_params def merge_request_params_attributes attrs = super.push( - { approval_rules_attributes: [:id, :name, { user_ids: [] }, { group_ids: [] }, :approvals_required, :approval_project_rule_id, :_destroy] }, + approval_rule_attributes, :approvals_before_merge, :approver_group_ids, :approver_ids @@ -23,6 +23,21 @@ def merge_request_params_attributes attrs end + def approval_rule_attributes + { + approval_rules_attributes: [ + :id, + :name, + { user_ids: [] }, + { group_ids: [] }, + :approvals_required, + :approval_project_rule_id, + :remove_hidden_groups, + :_destroy + ] + } + end + # If the number of approvals is not greater than the project default, set to # the project default, so that we fall back to the project default. And # still allow overriding rules defined at the project level but not allow diff --git a/ee/app/finders/approval_rules/group_finder.rb b/ee/app/finders/approval_rules/group_finder.rb new file mode 100644 index 0000000000000..a1dfab6b09747 --- /dev/null +++ b/ee/app/finders/approval_rules/group_finder.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +# For caching group related queries relative to current_user +module ApprovalRules + class GroupFinder + attr_reader :rule, :current_user + + def initialize(rule, user) + @rule = rule + @current_user = user + end + + def visible_groups + @visible_groups ||= rule.groups.public_or_visible_to_user(current_user) + end + + # rubocop: disable CodeReuse/ActiveRecord + def hidden_groups + @hidden_groups ||= rule.groups.where.not(id: visible_groups.map(&:id)) + end + + def contains_hidden_groups? + hidden_groups.loaded? ? hidden_groups.present? : hidden_groups.exists? + end + # rubocop: enable CodeReuse/ActiveRecord + end +end diff --git a/ee/app/presenters/approval_rule_presenter.rb b/ee/app/presenters/approval_rule_presenter.rb index b7ca2bdf2118b..fda2b3e299d65 100644 --- a/ee/app/presenters/approval_rule_presenter.rb +++ b/ee/app/presenters/approval_rule_presenter.rb @@ -2,6 +2,16 @@ class ApprovalRulePresenter < Gitlab::View::Presenter::Delegated def groups - super.public_or_visible_to_user(current_user) + group_query_service.visible_groups + end + + def contains_hidden_groups? + group_query_service.contains_hidden_groups? + end + + private + + def group_query_service + @group_query_service ||= ApprovalRules::GroupFinder.new(@subject, current_user) end end diff --git a/ee/app/services/approval_rules/params_filtering_service.rb b/ee/app/services/approval_rules/params_filtering_service.rb new file mode 100644 index 0000000000000..7bf79874eb371 --- /dev/null +++ b/ee/app/services/approval_rules/params_filtering_service.rb @@ -0,0 +1,110 @@ +# frozen_string_literal: true + +# @params target [MergeRequest, Project] +# @params params [Hash] for updating or creating target +# @params user [User] current user +# +# Returns a copy of `params` with rules modified, +# filtering rule users and groups based on accessibility from user +module ApprovalRules + class ParamsFilteringService + include Gitlab::Utils::StrongMemoize + + attr_reader :target, :params, :current_user, :rules, :visible_group_ids, :visible_user_ids + + def initialize(target, user, params) + @target = target + @current_user = user + @params = params.with_indifferent_access + + batch_load_visible_user_and_group_ids + end + + def execute + return params unless params.key?(:approval_rules_attributes) + + params[:approval_rules_attributes].each do |rule_attributes| + handle_rule(rule_attributes) + end + + params + end + + private + + def handle_rule(rule_attributes) + if rule_attributes.key?(:group_ids) + provided_group_ids = rule_attributes[:group_ids].map(&:to_i) + rule_attributes[:group_ids] = provided_group_ids & visible_group_ids + + append_hidden_groups(rule_attributes) + end + + if rule_attributes.key?(:user_ids) + provided_user_ids = rule_attributes[:user_ids].map(&:to_i) + rule_attributes[:user_ids] = provided_user_ids & visible_user_ids + end + end + + # Append hidden groups to params to prevent them from being removed, + # as hidden group ids are never passed to/back from clients for security reasons. + def append_hidden_groups(rule_attributes) + keep_hidden_groups = !Gitlab::Utils.to_boolean(rule_attributes.delete(:remove_hidden_groups)) + + return unless keep_hidden_groups + return unless rule_attributes.key?(:group_ids) + + hidden_group_sourcing_rule = find_hidden_group_sourcing_rule(rule_attributes) + + return unless hidden_group_sourcing_rule + + rule_attributes[:group_ids].concat( + ::ApprovalRules::GroupFinder.new(hidden_group_sourcing_rule, current_user).hidden_groups.map(&:id) + ) + end + + # Allow ruby-level filtering with only 2 queries + def batch_load_visible_user_and_group_ids + return unless params.key?(:approval_rules_attributes) + + # rubocop: disable CodeReuse/ActiveRecord + @visible_group_ids = params[:approval_rules_attributes].flat_map { |hash| hash[:group_ids] } + if @visible_group_ids.present? + @visible_group_ids = ::Group.id_in(@visible_group_ids).public_or_visible_to_user(current_user).pluck(:id) + end + + @visible_user_ids = params[:approval_rules_attributes].flat_map { |hash| hash[:user_ids] } + if @visible_user_ids.present? + @visible_user_ids = project.members_among(::User.id_in(@visible_user_ids)).pluck(:id) + end + # rubocop: enable CodeReuse/ActiveRecord + end + + def project + if target.is_a?(Project) + target + else + target.target_project + end + end + + def updating? + strong_memoize(:updating) { @target.persisted? } + end + + def find_hidden_group_sourcing_rule(rule_attributes) + rule_id = updating? ? rule_attributes[:id] : rule_attributes[:approval_project_rule_id] + + return if rule_id.blank? + + hidden_group_sourcing_rules[rule_id.to_i] + end + + def hidden_group_sourcing_rules + strong_memoize(:hidden_group_sourcing_rules) do + source = updating? ? target : project + source.approval_rules.includes(:groups).index_by(&:id) # rubocop: disable CodeReuse/ActiveRecord + end + end + end +end diff --git a/ee/app/services/approval_rules/update_service.rb b/ee/app/services/approval_rules/update_service.rb index b72236f8a55ef..8df9c39687fbe 100644 --- a/ee/app/services/approval_rules/update_service.rb +++ b/ee/app/services/approval_rules/update_service.rb @@ -2,11 +2,23 @@ module ApprovalRules class UpdateService < ::ApprovalRules::BaseService - attr_reader :rule + attr_reader :rule, :keep_existing_hidden_groups def initialize(approval_rule, user, params) @rule = approval_rule + @keep_existing_hidden_groups = !Gitlab::Utils.to_boolean(params.delete(:remove_hidden_groups)) + super(@rule.project, user, params) end + + def filter_eligible_groups! + super + + # Append hidden groups unless removal is explicitly stated, + # as hidden group ids are never passed to/back from clients for security reasons. + if params[:groups] && keep_existing_hidden_groups + params[:groups] += GroupFinder.new(rule, current_user).hidden_groups + end + end end end diff --git a/ee/app/services/ee/merge_requests/base_service.rb b/ee/app/services/ee/merge_requests/base_service.rb index e7715a1eab96a..3e9b7a7abea1c 100644 --- a/ee/app/services/ee/merge_requests/base_service.rb +++ b/ee/app/services/ee/merge_requests/base_service.rb @@ -12,36 +12,10 @@ def filter_params(merge_request) params.delete(:approver_group_ids) end - filter_approval_rule_groups_and_users(merge_request) + self.params = ApprovalRules::ParamsFilteringService.new(merge_request, current_user, params).execute super end - - def filter_approval_rule_groups_and_users(merge_request) - return unless params.key?(:approval_rules_attributes) - - # For efficiency, we avoid repeated check per rule for eligibility of users and groups - # but instead consolidate all ids so eligibility can be checked in one go. - group_ids = params[:approval_rules_attributes].flat_map { |hash| hash[:group_ids] } - user_ids = params[:approval_rules_attributes].flat_map { |hash| hash[:user_ids] } - - # rubocop: disable CodeReuse/ActiveRecord - group_ids = ::Group.id_in(group_ids).public_or_visible_to_user(current_user).pluck(:id) unless group_ids.empty? - user_ids = merge_request.project.members_among(::User.id_in(user_ids)).pluck(:id) unless user_ids.empty? - # rubocop: enable CodeReuse/ActiveRecord - - params[:approval_rules_attributes].each do |rule_attributes| - if rule_attributes.key?(:group_ids) - provided_group_ids = rule_attributes[:group_ids].map(&:to_i) - rule_attributes[:group_ids] = provided_group_ids & group_ids - end - - if rule_attributes.key?(:user_ids) - provided_user_ids = rule_attributes[:user_ids].map(&:to_i) - rule_attributes[:user_ids] = provided_user_ids & user_ids - end - end - end end end end diff --git a/ee/changelogs/unreleased/10356-private-group-handling.yml b/ee/changelogs/unreleased/10356-private-group-handling.yml new file mode 100644 index 0000000000000..f6fdf67b4991f --- /dev/null +++ b/ee/changelogs/unreleased/10356-private-group-handling.yml @@ -0,0 +1,5 @@ +--- +title: Fix project approval rule with only private group being considered as approved when override is allowed +merge_request: 10356 +author: +type: fixed diff --git a/ee/lib/api/project_approval_rules.rb b/ee/lib/api/project_approval_rules.rb index 29d297a8777c2..a3ceade854630 100644 --- a/ee/lib/api/project_approval_rules.rb +++ b/ee/lib/api/project_approval_rules.rb @@ -56,6 +56,7 @@ class ProjectApprovalRules < ::Grape::API optional :approvals_required, type: Integer, desc: 'The number of required approvals for this rule' optional :users, as: :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule' optional :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule' + optional :remove_hidden_groups, type: Boolean, desc: 'Whether hidden groups should be removed' end put do authorize! :admin_project, user_project diff --git a/ee/lib/ee/api/entities.rb b/ee/lib/ee/api/entities.rb index 2ba4df06c2e35..c275204c2303f 100644 --- a/ee/lib/ee/api/entities.rb +++ b/ee/lib/ee/api/entities.rb @@ -280,6 +280,7 @@ def initialize(object, options = {}) expose :approvals_required expose :users, using: ::API::Entities::UserBasic expose :groups, using: ::API::Entities::Group + expose :contains_hidden_groups?, as: :contains_hidden_groups end class MergeRequestApprovalRule < ApprovalRule diff --git a/ee/spec/features/merge_request/user_sets_approval_rules_spec.rb b/ee/spec/features/merge_request/user_sets_approval_rules_spec.rb new file mode 100644 index 0000000000000..9169a5ae41622 --- /dev/null +++ b/ee/spec/features/merge_request/user_sets_approval_rules_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe 'Merge request > User sets approval rules', :js do + let(:approver) { create(:user) } + let(:author) { create(:user) } + let(:project) { create(:project, :public, :repository) } + + before do + stub_licensed_features(multiple_approval_rules: true) + + [approver, author].each do |member| + project.add_maintainer(member) + end + end + + context "with project approval rules" do + let!(:regular_rule) { create(:approval_project_rule, project: project, users: [approver], name: 'Regular Rule') } + + context "with a private group rule" do + let!(:private_group) { create(:group, :private) } + let!(:private_rule) { create(:approval_project_rule, project: project, groups: [private_group], name: 'Private Rule') } + let!(:rules) { [regular_rule, private_rule] } + + before do + private_group.add_developer(approver) + + sign_in(author) + visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: 'feature' }) + wait_for_requests + end + + it "shows approval rules" do + names = page.all('.js-approval-rules table .js-name') + rules.each.with_index do |rule, idx| + expect(names[idx]).to have_text(rule.name) + end + end + + it "persists hidden groups that author has no access to when creating MR" do + click_on("Submit merge request") + wait_for_requests + + click_on("View eligible approvers") + wait_for_requests + + tr = page.find(:css, 'tr', text: private_rule.name) + expect(tr).to have_selector('.js-approvers a.user-avatar-link') + end + end + end +end diff --git a/ee/spec/finders/approval_rules/group_finder_spec.rb b/ee/spec/finders/approval_rules/group_finder_spec.rb new file mode 100644 index 0000000000000..7a136867163b9 --- /dev/null +++ b/ee/spec/finders/approval_rules/group_finder_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ApprovalRules::GroupFinder do + let(:rule) { create(:approval_project_rule) } + let(:user) { create(:user) } + + let(:public_group) { create(:group, name: 'public_group') } + let(:private_inaccessible_group) { create(:group, :private, name: 'private_inaccessible_group') } + let(:private_accessible_group) { create(:group, :private, name: 'private_accessible_group') } + + subject { described_class.new(rule, user) } + + before do + private_accessible_group.add_owner(user) + end + + context 'when with inaccessible groups' do + before do + rule.groups = [public_group, private_inaccessible_group, private_accessible_group] + end + + it 'returns groups' do + expect(subject.visible_groups).to contain_exactly(public_group, private_accessible_group) + expect(subject.hidden_groups).to contain_exactly(private_inaccessible_group) + expect(subject.contains_hidden_groups?).to eq(true) + end + end + + context 'when without inaccessible groups' do + before do + rule.groups = [public_group, private_accessible_group] + end + + it 'returns groups' do + expect(subject.visible_groups).to contain_exactly(public_group, private_accessible_group) + expect(subject.hidden_groups).to be_empty + expect(subject.contains_hidden_groups?).to eq(false) + end + end +end diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json index c8510433db16f..226c692a2f3ee 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/project_approval_rule.json @@ -4,6 +4,7 @@ "id": { "type": "integer" }, "name": { "type": "string" }, "approvals_required": { "type": "integer" }, + "contains_hidden_groups": { "type": "boolean" }, "rule_type": { "type": "tring" }, "approvers": { "type": "array", diff --git a/ee/spec/javascripts/approvals/components/approvers_list_item_spec.js b/ee/spec/javascripts/approvals/components/approvers_list_item_spec.js index 33bf799c912c0..7d0e4ca966a53 100644 --- a/ee/spec/javascripts/approvals/components/approvers_list_item_spec.js +++ b/ee/spec/javascripts/approvals/components/approvers_list_item_spec.js @@ -1,8 +1,9 @@ import { shallowMount, createLocalVue } from '@vue/test-utils'; import { GlButton } from '@gitlab/ui'; import Avatar from '~/vue_shared/components/project_avatar/default.vue'; -import { TYPE_USER, TYPE_GROUP } from 'ee/approvals/constants'; +import { TYPE_USER, TYPE_GROUP, TYPE_HIDDEN_GROUPS } from 'ee/approvals/constants'; import ApproversListItem from 'ee/approvals/components/approvers_list_item.vue'; +import HiddenGroupsItem from 'ee/approvals/components/hidden_groups_item.vue'; const localVue = createLocalVue(); const TEST_USER = { @@ -69,5 +70,27 @@ describe('Approvals ApproversListItem', () => { expect(wrapper.text()).toContain(TEST_GROUP.full_path); expect(wrapper.text()).not.toContain(TEST_GROUP.name); }); + + it('does not render hidden-groups-item', () => { + expect(wrapper.find(HiddenGroupsItem).exists()).toBe(false); + }); + }); + + describe('when hidden groups', () => { + beforeEach(() => { + factory({ + propsData: { + approver: { type: TYPE_HIDDEN_GROUPS }, + }, + }); + }); + + it('renders hidden-groups-item', () => { + expect(wrapper.find(HiddenGroupsItem).exists()).toBe(true); + }); + + it('does not render avatar', () => { + expect(wrapper.find(Avatar).exists()).toBe(false); + }); }); }); diff --git a/ee/spec/javascripts/approvals/components/hidden_groups_item_spec.js b/ee/spec/javascripts/approvals/components/hidden_groups_item_spec.js new file mode 100644 index 0000000000000..41b17d02217ee --- /dev/null +++ b/ee/spec/javascripts/approvals/components/hidden_groups_item_spec.js @@ -0,0 +1,27 @@ +import { shallowMount, createLocalVue } from '@vue/test-utils'; +import HiddenGroupsItem from 'ee/approvals/components/hidden_groups_item.vue'; + +const localVue = createLocalVue(); + +describe('Approvals HiddenGroupsItem', () => { + let wrapper; + + const factory = (options = {}) => { + wrapper = shallowMount(localVue.extend(HiddenGroupsItem), { + ...options, + localVue, + sync: false, + }); + }; + + afterEach(() => { + wrapper.destroy(); + wrapper = null; + }); + + it('renders successfully', () => { + factory(); + + expect(wrapper.exists()).toBe(true); + }); +}); diff --git a/ee/spec/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs_spec.js b/ee/spec/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs_spec.js index 75fd2e8815c0a..2b5e1d1c1127f 100644 --- a/ee/spec/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs_spec.js +++ b/ee/spec/javascripts/approvals/components/mr_edit/mr_rules_hidden_inputs_spec.js @@ -16,6 +16,7 @@ const { INPUT_USER_IDS, INPUT_GROUP_IDS, INPUT_DELETE, + INPUT_REMOVE_HIDDEN_GROUPS, INPUT_FALLBACK_APPROVALS_REQUIRED, } = MRRulesHiddenInputs; const TEST_USERS = [{ id: 1 }, { id: 10 }]; @@ -184,6 +185,21 @@ describe('EE Approvlas MRRulesHiddenInputs', () => { }); }); }); + + describe('with remove hidden groups', () => { + beforeEach(() => { + rule.removeHiddenGroups = true; + }); + + it('renders input to remove hidden groups', () => { + factory(); + + expect(findHiddenInputs()).toContain({ + name: INPUT_REMOVE_HIDDEN_GROUPS, + value: 'true', + }); + }); + }); }); }); }); diff --git a/ee/spec/javascripts/approvals/components/rule_form_spec.js b/ee/spec/javascripts/approvals/components/rule_form_spec.js index 67ee1fecb7f81..e5cce74d45e2e 100644 --- a/ee/spec/javascripts/approvals/components/rule_form_spec.js +++ b/ee/spec/javascripts/approvals/components/rule_form_spec.js @@ -4,8 +4,9 @@ import { GlButton } from '@gitlab/ui'; import { createStoreOptions } from 'ee/approvals/stores'; import projectSettingsModule from 'ee/approvals/stores/modules/project_settings'; import ApproversSelect from 'ee/approvals/components/approvers_select.vue'; +import ApproversList from 'ee/approvals/components/approvers_list.vue'; import RuleForm from 'ee/approvals/components/rule_form.vue'; -import { TYPE_USER, TYPE_GROUP } from 'ee/approvals/constants'; +import { TYPE_USER, TYPE_GROUP, TYPE_HIDDEN_GROUPS } from 'ee/approvals/constants'; const TEST_PROJECT_ID = '7'; const TEST_RULE = { @@ -25,6 +26,8 @@ const TEST_FALLBACK_RULE = { const localVue = createLocalVue(); localVue.use(Vuex); +const addType = type => x => Object.assign(x, { type }); + describe('EE Approvals RuleForm', () => { let wrapper; let store; @@ -48,6 +51,7 @@ describe('EE Approvals RuleForm', () => { const findApprovalsRequiredValidation = () => findValidation(findApprovalsRequiredInput(), false); const findApproversSelect = () => wrapper.find(ApproversSelect); const findApproversValidation = () => findValidation(findApproversSelect(), true); + const findApproversList = () => wrapper.find(ApproversList); const findValidations = () => [ findNameValidation(), findApprovalsRequiredValidation(), @@ -156,6 +160,7 @@ describe('EE Approvals RuleForm', () => { groups, userRecords, groupRecords, + removeHiddenGroups: false, }; findNameInput().setValue(expected.name); @@ -192,6 +197,15 @@ describe('EE Approvals RuleForm', () => { }); }); + it('shows approvers', () => { + const list = findApproversList(); + + expect(list.props('value')).toEqual([ + ...TEST_RULE.groups.map(addType(TYPE_GROUP)), + ...TEST_RULE.users.map(addType(TYPE_USER)), + ]); + }); + it('on submit, puts rule', () => { const userRecords = TEST_RULE.users.map(x => ({ ...x, type: TYPE_USER })); const groupRecords = TEST_RULE.groups.map(x => ({ ...x, type: TYPE_GROUP })); @@ -204,6 +218,7 @@ describe('EE Approvals RuleForm', () => { groups, userRecords, groupRecords, + removeHiddenGroups: false, }; wrapper.vm.submit(); @@ -298,6 +313,77 @@ describe('EE Approvals RuleForm', () => { }); }); }); + + describe('with hidden groups rule', () => { + beforeEach(() => { + createComponent({ + initRule: { + ...TEST_RULE, + containsHiddenGroups: true, + }, + }); + }); + + it('shows approvers and hidden group', () => { + const list = findApproversList(); + + expect(list.props('value')).toEqual([ + ...TEST_RULE.groups.map(addType(TYPE_GROUP)), + ...TEST_RULE.users.map(addType(TYPE_USER)), + { type: TYPE_HIDDEN_GROUPS }, + ]); + }); + + it('on submit, does not remove hidden groups', () => { + wrapper.vm.submit(); + + expect(actions.putRule).toHaveBeenCalledWith( + jasmine.anything(), + jasmine.objectContaining({ + removeHiddenGroups: false, + }), + undefined, + ); + }); + + describe('and hidden groups removed', () => { + beforeEach(() => { + wrapper.vm.approvers = wrapper.vm.approvers.filter(x => x.type !== TYPE_HIDDEN_GROUPS); + }); + + it('on submit, removes hidden groups', () => { + wrapper.vm.submit(); + + expect(actions.putRule).toHaveBeenCalledWith( + jasmine.anything(), + jasmine.objectContaining({ + removeHiddenGroups: true, + }), + undefined, + ); + }); + }); + }); + + describe('with removed hidden groups rule', () => { + beforeEach(() => { + createComponent({ + initRule: { + ...TEST_RULE, + containsHiddenGroups: true, + removeHiddenGroups: true, + }, + }); + }); + + it('does not add hidden groups in approvers', () => { + expect( + findApproversList() + .props('value') + .every(x => x.type !== TYPE_HIDDEN_GROUPS), + ).toBe(true); + }); + }); }); describe('when allow only single rule', () => { diff --git a/ee/spec/presenters/approval_rule_presenter_spec.rb b/ee/spec/presenters/approval_rule_presenter_spec.rb index 87fdb3220e94e..fd5d3525b5be1 100644 --- a/ee/spec/presenters/approval_rule_presenter_spec.rb +++ b/ee/spec/presenters/approval_rule_presenter_spec.rb @@ -3,13 +3,13 @@ require 'spec_helper' describe ApprovalRulePresenter do - describe '#groups' do - set(:user) { create(:user) } - set(:public_group) { create(:group) } - set(:private_group) { create(:group, :private) } - let(:groups) { [public_group, private_group] } - subject { described_class.new(rule, current_user: user) } + set(:user) { create(:user) } + set(:public_group) { create(:group) } + set(:private_group) { create(:group, :private) } + let(:groups) { [public_group, private_group] } + subject { described_class.new(rule, current_user: user) } + describe '#groups' do shared_examples 'filtering private group' do context 'when user has no access to private group' do it 'excludes private group' do @@ -49,4 +49,45 @@ end end end + + describe '#contains_hidden_groups?' do + shared_examples 'detecting hidden group' do + context 'when user has no access to private group' do + it 'excludes private group' do + expect(subject.contains_hidden_groups?).to eq(true) + end + end + + context 'when user has access to private group' do + it 'includes private group' do + private_group.add_owner(user) + + expect(subject.contains_hidden_groups?).to eq(false) + end + end + end + + context 'project rule' do + let(:rule) { create(:approval_project_rule, groups: groups) } + + it_behaves_like 'detecting hidden group' + end + + context 'wrapped approval rule' do + let(:rule) do + mr_rule = create(:approval_merge_request_rule, groups: groups) + ApprovalWrappedRule.new(mr_rule.merge_request, mr_rule) + end + + it_behaves_like 'detecting hidden group' + end + + context 'fallback rule' do + let(:rule) { ApprovalMergeRequestFallback.new(create(:merge_request)) } + + it 'contains no groups without raising an error' do + expect(subject.contains_hidden_groups?).to eq(false) + end + end + end end diff --git a/ee/spec/services/approval_rules/params_filtering_service_spec.rb b/ee/spec/services/approval_rules/params_filtering_service_spec.rb new file mode 100644 index 0000000000000..10d33d6d6a044 --- /dev/null +++ b/ee/spec/services/approval_rules/params_filtering_service_spec.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ApprovalRules::ParamsFilteringService do + let(:service) { described_class.new(merge_request, user, params) } + let(:project_member) { create(:user) } + let(:outsider) { create(:user) } + let(:accessible_group) { create(:group, :private) } + let(:inaccessible_group) { create(:group, :private) } + let(:project) { create(:project, :repository) } + let(:user) { create(:user) } + + describe '#execute' do + shared_examples_for(:assigning_users_and_groups) do + before do + project.add_maintainer(user) + project.add_reporter(project_member) + + accessible_group.add_developer(user) + end + + it 'only assigns eligible users and groups' do + params = service.execute + + rule1 = params[:approval_rules_attributes].first + + expect(rule1[:user_ids]).to contain_exactly(project_member.id) + + rule2 = params[:approval_rules_attributes].last + expected_group_ids = expected_groups.map(&:id) + + expect(rule2[:user_ids]).to be_empty + expect(rule2[:group_ids]).to contain_exactly(*expected_group_ids) + end + end + + context 'create' do + it_behaves_like :assigning_users_and_groups do + let(:merge_request) { build(:merge_request, target_project: project, source_project: project) } + let(:params) do + { + title: 'Awesome merge_request', + description: 'please fix', + source_branch: 'feature', + target_branch: 'master', + force_remove_source_branch: '1', + approval_rules_attributes: [ + { name: 'foo', user_ids: [project_member.id, outsider.id] }, + { name: 'bar', user_ids: [outsider.id], group_ids: [accessible_group.id, inaccessible_group.id] } + ] + } + end + let(:expected_groups) { [accessible_group] } + end + end + + context 'update' do + let(:merge_request) { create(:merge_request, target_project: project, source_project: project)} + let(:existing_private_group) { create(:group, :private) } + let!(:rule1) { create(:approval_merge_request_rule, merge_request: merge_request, users: [create(:user)]) } + let!(:rule2) { create(:approval_merge_request_rule, merge_request: merge_request, groups: [existing_private_group]) } + + it_behaves_like :assigning_users_and_groups do + let(:params) do + { + approval_rules_attributes: [ + { id: rule1.id, name: 'foo', user_ids: [project_member.id, outsider.id] }, + { id: rule2.id, name: 'bar', user_ids: [outsider.id], group_ids: [accessible_group.id, inaccessible_group.id] } + ] + } + end + let(:expected_groups) { [accessible_group, existing_private_group] } + end + + context 'with remove_hidden_groups being true' do + it_behaves_like :assigning_users_and_groups do + let(:params) do + { + approval_rules_attributes: [ + { id: rule1.id, name: 'foo', user_ids: [project_member.id, outsider.id] }, + { id: rule2.id, name: 'bar', user_ids: [outsider.id], group_ids: [accessible_group.id, inaccessible_group.id], remove_hidden_groups: true } + ] + } + end + let(:expected_groups) { [accessible_group] } + end + end + end + end +end diff --git a/ee/spec/services/approval_rules/update_service_spec.rb b/ee/spec/services/approval_rules/update_service_spec.rb index 6cacd2997edda..f00b4d2e0025c 100644 --- a/ee/spec/services/approval_rules/update_service_spec.rb +++ b/ee/spec/services/approval_rules/update_service_spec.rb @@ -54,6 +54,61 @@ end end + context 'when existing groups are inaccessible to user' do + let(:private_accessible_group) { create(:group, :private) } + let(:private_inaccessible_group) { create(:group, :private) } + let(:new_group) { create(:group) } + + before do + approval_rule.groups = [private_accessible_group, private_inaccessible_group] + private_accessible_group.add_guest user + end + + context 'when remove_hidden_groups is false' do + it 'preserves inaccessible groups' do + result = described_class.new(approval_rule, user, { + remove_hidden_groups: false, + group_ids: [new_group.id] + }).execute + + expect(result[:status]).to eq(:success) + + rule = result[:rule] + + expect(rule.groups).to contain_exactly(private_inaccessible_group, new_group) + end + end + + context 'when remove_hidden_groups is not specified' do + it 'removes inaccessible groups' do + result = described_class.new(approval_rule, user, { + group_ids: [new_group.id] + }).execute + + expect(result[:status]).to eq(:success) + + rule = result[:rule] + + expect(rule.groups).to contain_exactly(private_inaccessible_group, new_group) + end + end + + context 'when remove_hidden_groups is true' do + it 'removes inaccessible groups' do + result = described_class.new(approval_rule, user, { + remove_hidden_groups: true, + group_ids: [new_group.id] + }).execute + + expect(result[:status]).to eq(:success) + + rule = result[:rule] + + expect(rule.groups).to contain_exactly(new_group) + end + end + end + context 'when validation fails' do it 'returns error message' do result = described_class.new(approval_rule, user, { diff --git a/ee/spec/services/ee/merge_requests/base_service_spec.rb b/ee/spec/services/ee/merge_requests/base_service_spec.rb index 2dd5a7372a08a..35731bd383b7a 100644 --- a/ee/spec/services/ee/merge_requests/base_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/base_service_spec.rb @@ -3,77 +3,34 @@ require 'spec_helper' describe MergeRequests::BaseService do - include ProjectForksHelper - - let(:project_member) { create(:user) } - let(:outsider) { create(:user) } - let(:accessible_group) { create(:group, :private) } - let(:inaccessible_group) { create(:group, :private) } + subject { MergeRequests::CreateService.new(project, project.owner, params) } let(:project) { create(:project, :repository) } - let(:user) { create(:user) } + let(:params_filtering_service) { double(:params_filtering_service) } + let(:params) do + { + title: 'Awesome merge_request', + description: 'please fix', + source_branch: 'feature', + target_branch: 'master' + } + end describe '#filter_params' do context 'filter users and groups' do - shared_examples_for(:assigning_users_and_groups) do - before do - project.add_maintainer(user) - project.add_reporter(project_member) - - accessible_group.add_developer(user) - - allow(service).to receive(:execute_hooks) - end - - it 'only assigns eligible users and groups' do - merge_request = subject - - rule1 = merge_request.approval_rules.regular.first - - expect(rule1.users).to contain_exactly(*project_member) - - rule2 = merge_request.approval_rules.regular.last - - expect(rule2.users).to be_empty - expect(rule2.groups).to contain_exactly(*accessible_group) - end - end - - context 'create' do - it_behaves_like :assigning_users_and_groups do - let(:service) { MergeRequests::CreateService.new(project, user, opts) } - let(:opts) do - { - title: 'Awesome merge_request', - description: 'please fix', - source_branch: 'feature', - target_branch: 'master', - force_remove_source_branch: '1', - approval_rules_attributes: [ - { name: 'foo', user_ids: [project_member.id, outsider.id] }, - { name: 'bar', user_ids: [outsider.id], group_ids: [accessible_group.id, inaccessible_group.id] } - ] - } - end - subject { service.execute } - end + before do + allow(subject).to receive(:execute_hooks) end - context 'update' do - let(:merge_request) { create(:merge_request, target_project: project, source_project: project)} + it 'calls ParamsFilteringService' do + expect(ApprovalRules::ParamsFilteringService).to receive(:new).with( + an_instance_of(MergeRequest), + project.owner, + params + ).and_return(params_filtering_service) + expect(params_filtering_service).to receive(:execute).and_return(params) - it_behaves_like :assigning_users_and_groups do - let(:service) { MergeRequests::UpdateService.new(project, user, opts) } - let(:opts) do - { - approval_rules_attributes: [ - { name: 'foo', user_ids: [project_member.id, outsider.id] }, - { name: 'bar', user_ids: [outsider.id], group_ids: [accessible_group.id, inaccessible_group.id] } - ] - } - end - subject { service.execute(merge_request) } - end + subject.execute end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 6a8d2bcea6001..c15bf573ea3a6 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -7326,6 +7326,9 @@ msgid_plural "%d more items" msgstr[0] "" msgstr[1] "" +msgid "One or more groups that you don't have access to." +msgstr "" + msgid "One or more of your Bitbucket projects cannot be imported into GitLab directly because they use Subversion or Mercurial for version control, rather than Git." msgstr "" @@ -7875,6 +7878,9 @@ msgstr "" msgid "Private - The group and its projects can only be viewed by members." msgstr "" +msgid "Private group(s)" +msgstr "" + msgid "Private projects can be created in your personal namespace with:" msgstr "" -- GitLab