From b90a806ed3837cdc28b2ecb7c44326b3da3a6956 Mon Sep 17 00:00:00 2001 From: Eugenia Grieff Date: Fri, 9 May 2025 18:28:34 +0200 Subject: [PATCH 1/3] Skip field authorization for work items The field authorization is not necessary because this is already handled by WorkItems::Finder --- app/finders/issues/issue_types_filter.rb | 22 ++++ app/finders/issues_finder.rb | 12 +- .../work_items/linked_items_resolver.rb | 14 ++- app/graphql/resolvers/work_items_resolver.rb | 9 ++ app/graphql/types/work_item_type.rb | 2 + .../types/work_items/linked_item_type.rb | 4 +- .../types/work_items/widgets/labels_type.rb | 1 + .../work_items/widgets/milestone_type.rb | 1 + .../skip_work_items_field_authorization.yml | 10 ++ .../finders/ee/issues/issue_types_filter.rb | 58 ++++++++++ .../ee/work_items/work_items_finder_spec.rb | 105 ++++++++++++++++++ ee/spec/finders/issues_finder_spec.rb | 104 +++++++++++++++++ .../api/graphql/group/work_items_spec.rb | 39 ++++++- .../work_items/work_items_finder_spec.rb | 4 +- .../api/graphql/project/work_items_spec.rb | 33 ++++++ 15 files changed, 406 insertions(+), 12 deletions(-) create mode 100644 app/finders/issues/issue_types_filter.rb create mode 100644 config/feature_flags/gitlab_com_derisk/skip_work_items_field_authorization.yml create mode 100644 ee/app/finders/ee/issues/issue_types_filter.rb diff --git a/app/finders/issues/issue_types_filter.rb b/app/finders/issues/issue_types_filter.rb new file mode 100644 index 00000000000000..27c3c6f398066f --- /dev/null +++ b/app/finders/issues/issue_types_filter.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Issues # rubocop:disable Gitlab/BoundedContexts -- existing Finders modules/classes are not bounded + class IssueTypesFilter < Issuables::BaseFilter + def filter(issues) + by_issue_types(issues) + end + + def by_issue_types(issues) + return issues if param_types.blank? + return issues.model.none unless (WorkItems::Type.base_types.keys & param_types).sort == param_types.sort + + issues.with_issue_type(param_types) + end + + def param_types + Array.wrap(params[:issue_types]).map(&:to_s) + end + end +end # rubocop:enable Gitlab/BoundedContexts + +Issues::IssueTypesFilter.prepend_mod diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index 0d1799fc9c6c4b..addb14cebf0aa5 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -113,12 +113,16 @@ def by_due_date(items) end def by_issue_types(items) - issue_type_params = Array(params[:issue_types]).map(&:to_s) - return items if issue_type_params.blank? - return klass.none unless (WorkItems::Type.base_types.keys & issue_type_params).sort == issue_type_params.sort + types_filter.filter(items) + end - items.with_issue_type(params[:issue_types]) + def types_filter + Issues::IssueTypesFilter.new( + params: original_params, + parent: params.parent + ) end + strong_memoize_attr :types_filter def by_service_desk(items) return items unless params[:author_username] == "support-bot" diff --git a/app/graphql/resolvers/work_items/linked_items_resolver.rb b/app/graphql/resolvers/work_items/linked_items_resolver.rb index dd0c119a2900eb..bc2c7b4f25a94d 100644 --- a/app/graphql/resolvers/work_items/linked_items_resolver.rb +++ b/app/graphql/resolvers/work_items/linked_items_resolver.rb @@ -49,12 +49,24 @@ def linked_items_grouped_by_source(linked_items, item_ids) linked_items.each_with_object({}) do |item, result| # Find the ID of the item that this item links to target_id = [item.issue_link_source_id, item.issue_link_target_id].find { |id| id != item.id } - next unless item_ids.include?(target_id) + # Skip items that don't link to our target or that the user can't read + # We optimize authorization for items with the same properties to avoid redundant checks + next unless item_ids.include?(target_id) && can_read_linked_item?(item) result[target_id] ||= [] result[target_id] << item end end + + def can_read_linked_item?(item) + if work_item.work_item_type_id == item.work_item_type_id && + work_item.resource_parent == item.resource_parent && + work_item.confidential == item.confidential + true + else + current_user.can?(:read_work_item, item) + end + end end end end diff --git a/app/graphql/resolvers/work_items_resolver.rb b/app/graphql/resolvers/work_items_resolver.rb index 0e42a2d3cbfe8e..9e51a61ad86cf7 100644 --- a/app/graphql/resolvers/work_items_resolver.rb +++ b/app/graphql/resolvers/work_items_resolver.rb @@ -22,6 +22,11 @@ class WorkItemsResolver < BaseResolver def resolve_with_lookahead(**args) return WorkItem.none if resource_parent.nil? + # Adding skip_type_authorization in the resolver while it is conditionally enabled. + # It can be moved to the field definition once the feature flag is removed + # Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/548096 + context.scoped_set!(:skip_type_authorization, [:read_work_item]) if skip_field_authorization? + finder = choose_finder(args) items = Gitlab::Graphql::Loaders::IssuableLoader @@ -75,6 +80,10 @@ def resource_parent obj.respond_to?(:sync) ? obj.sync : obj end end + + def skip_field_authorization? + Feature.enabled?(:skip_work_items_field_authorization, resource_parent&.root_ancestor, type: :gitlab_com_derisk) + end end end diff --git a/app/graphql/types/work_item_type.rb b/app/graphql/types/work_item_type.rb index 668f2d166a9378..49b6c97d5aa178 100644 --- a/app/graphql/types/work_item_type.rb +++ b/app/graphql/types/work_item_type.rb @@ -38,9 +38,11 @@ class WorkItemType < BaseObject description: 'Lock version of the work item. Incremented each time the work item is updated.' field :namespace, Types::NamespaceType, null: true, description: 'Namespace the work item belongs to.', + skip_type_authorization: [:read_namespace], experiment: { milestone: '15.10' } field :project, Types::ProjectType, null: true, description: 'Project the work item belongs to.', + skip_type_authorization: [:read_project], experiment: { milestone: '15.3' } field :state, WorkItemStateEnum, null: false, description: 'State of the work item.' diff --git a/app/graphql/types/work_items/linked_item_type.rb b/app/graphql/types/work_items/linked_item_type.rb index 01d2fd8efac2e6..b0a1ded0741022 100644 --- a/app/graphql/types/work_items/linked_item_type.rb +++ b/app/graphql/types/work_items/linked_item_type.rb @@ -2,11 +2,10 @@ module Types module WorkItems + # rubocop:disable Graphql/AuthorizeTypes -- authorized by resolver class LinkedItemType < BaseObject graphql_name 'LinkedWorkItemType' - authorize :read_work_item - field :link_created_at, ::Types::TimeType, description: 'Timestamp the link was created.', null: false, method: :issue_link_created_at @@ -30,5 +29,6 @@ def work_item object end end + # rubocop:enable Graphql/AuthorizeTypes end end diff --git a/app/graphql/types/work_items/widgets/labels_type.rb b/app/graphql/types/work_items/widgets/labels_type.rb index 5a56548c39e1a8..dcfbccf55512d5 100644 --- a/app/graphql/types/work_items/widgets/labels_type.rb +++ b/app/graphql/types/work_items/widgets/labels_type.rb @@ -15,6 +15,7 @@ class LabelsType < BaseObject field :labels, ::Types::LabelType.connection_type, null: true, description: 'Labels assigned to the work item.', + skip_type_authorization: [:read_label], resolver: Resolvers::BulkLabelsResolver field :allows_scoped_labels, GraphQL::Types::Boolean, diff --git a/app/graphql/types/work_items/widgets/milestone_type.rb b/app/graphql/types/work_items/widgets/milestone_type.rb index 0b3ac182a3830c..e3a1d0eb263377 100644 --- a/app/graphql/types/work_items/widgets/milestone_type.rb +++ b/app/graphql/types/work_items/widgets/milestone_type.rb @@ -14,6 +14,7 @@ class MilestoneType < BaseObject field :milestone, ::Types::MilestoneType, + skip_type_authorization: [:read_milestone], null: true, description: 'Milestone of the work item.' end diff --git a/config/feature_flags/gitlab_com_derisk/skip_work_items_field_authorization.yml b/config/feature_flags/gitlab_com_derisk/skip_work_items_field_authorization.yml new file mode 100644 index 00000000000000..3995989f585b23 --- /dev/null +++ b/config/feature_flags/gitlab_com_derisk/skip_work_items_field_authorization.yml @@ -0,0 +1,10 @@ +--- +name: skip_work_items_field_authorization +description: +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/542421 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/190860 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/548096 +milestone: '18.1' +group: group::product planning +type: gitlab_com_derisk +default_enabled: false diff --git a/ee/app/finders/ee/issues/issue_types_filter.rb b/ee/app/finders/ee/issues/issue_types_filter.rb new file mode 100644 index 00000000000000..15f129f53c0c9d --- /dev/null +++ b/ee/app/finders/ee/issues/issue_types_filter.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +module EE + module Issues # rubocop:disable Gitlab/BoundedContexts -- FOSS finder is not bounded to a context + module IssueTypesFilter + extend ::Gitlab::Utils::Override + + private + + override :by_issue_types + def by_issue_types(issues) + return super unless authorize_issue_types? + return super if param_types.any? && param_types.exclude?('epic') + + if param_types.any? && (::WorkItems::Type.base_types.keys & param_types).sort != param_types.sort + return issues.model.none + end + + return without_epic_type(issues) unless epic_type_allowed? + # In namespace-level searches remove project-level epics if they're disabled for projects + return without_epic_type_in_projects(issues) unless epic_type_allowed_in_group_projects? + + param_types.any? ? issues.with_issue_type(param_types) : issues + end + + def authorize_issue_types? + ::Feature.enabled?(:skip_work_items_field_authorization, parent&.root_ancestor, type: :gitlab_com_derisk) + end + + def epic_type_allowed? + return false unless parent&.licensed_feature_available?(:epics) + return false if parent.is_a?(Project) && !parent.project_epics_enabled? + + true + end + + def epic_type_allowed_in_group_projects? + return true unless parent.is_a?(Group) + + parent.project_epics_enabled? + end + + def without_epic_type_in_projects(issues) + project_epics = issues.with_issue_type('epic').project_level + + filtered_issues = param_types.any? ? issues.with_issue_type(param_types) : issues + filtered_issues.id_not_in(project_epics) + end + + def without_epic_type(issues) + return issues.without_issue_type('epic') if param_types.blank? + + allowed_param_types = param_types.excluding('epic') + allowed_param_types.any? ? issues.with_issue_type(allowed_param_types) : issues.model.none + end + end + end # rubocop:enable Gitlab/BoundedContexts +end diff --git a/ee/spec/finders/ee/work_items/work_items_finder_spec.rb b/ee/spec/finders/ee/work_items/work_items_finder_spec.rb index 0b01d417fab992..983dceaef7a0b1 100644 --- a/ee/spec/finders/ee/work_items/work_items_finder_spec.rb +++ b/ee/spec/finders/ee/work_items/work_items_finder_spec.rb @@ -87,6 +87,7 @@ before do group.add_reporter(user) + stub_licensed_features(epics: true) epic_work_item1.labels << label3 epic_work_item2.labels << label5 @@ -170,5 +171,109 @@ it_behaves_like 'filter by unified emoji association' end + + describe 'filtering by issue_types' do + let_it_be(:current_user) { create(:user) } + let_it_be(:group) { create(:group, developers: [current_user]) } + let_it_be(:subgroup) { create(:group, developers: [current_user], parent: group) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:subgroup_project) { create(:project, group: subgroup) } + + let_it_be(:task) { create(:work_item, :task, author: current_user, project: project) } + let_it_be(:project_issue) { create(:work_item, author: current_user, project: project) } + let_it_be(:group_issue) { create(:work_item, author: current_user, namespace: group) } + let_it_be(:project_epic) { create(:work_item, :epic, author: current_user, project: project) } + let_it_be(:group_epic) { create(:work_item, :epic, author: current_user, namespace: group) } + let_it_be(:subgroup_epic) { create(:work_item, :epic, author: current_user, namespace: subgroup) } + let_it_be(:subproject_epic) { create(:work_item, :epic, author: current_user, project: project) } + + let(:params) { {} } + + subject(:items) { described_class.new(current_user, params.reverse_merge(scope: 'all', state: 'opened')).execute } + + context 'with group param' do + context 'with include_descendants as false' do + let(:params) { { group_id: group, include_descendants: false } } + + context 'when epics feature is available' do + before do + stub_licensed_features(epics: true) + end + + it { is_expected.to contain_exactly(group_issue, group_epic) } + end + + context 'when epics feature is not available' do + before do + stub_licensed_features(epics: false) + end + + it { is_expected.to be_empty } + + context 'when issue_types param is epic' do + let(:params) { { group_id: group, include_descendants: false, issue_types: 'epic' } } + + it { is_expected.to be_empty } + end + end + end + + context 'with include_descendants as true' do + let(:params) { { group_id: group, include_descendants: true } } + + context 'when epics feature is available' do + before do + stub_licensed_features(epics: true) + end + + it 'returns allowed types' do + is_expected.to contain_exactly( + task, + project_issue, + project_epic, + group_issue, + group_epic, + subgroup_epic, + subproject_epic + ) + end + + context 'when project_work_item_epics feature flag is disabled' do + before do + stub_feature_flags(project_work_item_epics: false) + end + + it { is_expected.to contain_exactly(task, project_issue, group_issue, group_epic, subgroup_epic) } + + context 'and issue_types param is epic' do + let(:params) { { group_id: group, include_descendants: true, issue_types: 'epic' } } + + it { is_expected.to contain_exactly(group_epic, subgroup_epic) } + end + + context 'and issue_types param includes epic' do + let(:params) { { group_id: group, include_descendants: true, issue_types: %w[epic task] } } + + it { is_expected.to contain_exactly(task, group_epic, subgroup_epic) } + end + end + end + + context 'when epics feature is not available' do + before do + stub_licensed_features(epics: false) + end + + it { is_expected.to contain_exactly(task, project_issue) } + + context 'and issue_types param is epic' do + let(:params) { { group_id: group, include_descendants: true, issue_types: 'epic' } } + + it { is_expected.to be_empty } + end + end + end + end + end end end diff --git a/ee/spec/finders/issues_finder_spec.rb b/ee/spec/finders/issues_finder_spec.rb index a1bdff19188439..76c7f81114303a 100644 --- a/ee/spec/finders/issues_finder_spec.rb +++ b/ee/spec/finders/issues_finder_spec.rb @@ -587,4 +587,108 @@ it_behaves_like 'a filtered collection' # by status id end end + + describe 'filtering by issue_types' do + let_it_be(:current_user) { create(:user) } + let_it_be(:group) { create(:group, developers: [current_user]) } + let_it_be(:project) { create(:project, group: group) } + + let_it_be(:task) { create(:issue, :task, author: current_user, project: project) } + let_it_be(:issue) { create(:issue, author: current_user, project: project) } + let_it_be(:project_epic) { create(:issue, :epic, author: current_user, project: project) } + + let(:params) { {} } + + subject { described_class.new(current_user, params.reverse_merge(scope: 'all', state: 'opened')).execute } + + context 'when issue_types param is not present' do + let(:params) { { project_id: project } } + + context 'when epics feature is available' do + before do + stub_licensed_features(epics: true) + end + + it { is_expected.to contain_exactly(task, issue, project_epic) } + + context 'and project_work_item_epics feature flag is disabled' do + before do + stub_feature_flags(project_work_item_epics: false) + end + + it { is_expected.to contain_exactly(task, issue) } + end + end + + context 'when epics feature is not available' do + before do + stub_licensed_features(epics: false) + end + + it { is_expected.to contain_exactly(task, issue) } + end + end + + context 'when issue_types param includes epic' do + let(:params) { { project_id: project, issue_types: %w[task epic] } } + + context 'when epics feature is available' do + before do + stub_licensed_features(epics: true) + end + + it { is_expected.to contain_exactly(task, project_epic) } + + context 'and project_work_item_epics feature flag is disabled' do + before do + stub_feature_flags(project_work_item_epics: false) + end + + it { is_expected.to contain_exactly(task) } + end + end + + context 'when epics feature is not available' do + before do + stub_licensed_features(epics: false) + end + + it { is_expected.to contain_exactly(task) } + end + end + + context 'when issue_types param is epic' do + let(:params) { { project_id: project, issue_types: 'epic' } } + + context 'when epics feature is available' do + before do + stub_licensed_features(epics: true) + end + + it { is_expected.to contain_exactly(project_epic) } + + context 'and project_work_item_epics feature flag is disabled' do + before do + stub_feature_flags(project_work_item_epics: false) + end + + it { is_expected.to be_empty } + end + end + + context 'when epics feature is not available' do + before do + stub_licensed_features(epics: false) + end + + it { is_expected.to be_empty } + end + end + + context 'when issue_types param includes an invalid type' do + let(:params) { { project_id: project, issue_types: %w[foo issue epic] } } + + it { is_expected.to be_empty } + end + end end diff --git a/ee/spec/requests/api/graphql/group/work_items_spec.rb b/ee/spec/requests/api/graphql/group/work_items_spec.rb index 90162b357419ca..dc2dc5c0375ffb 100644 --- a/ee/spec/requests/api/graphql/group/work_items_spec.rb +++ b/ee/spec/requests/api/graphql/group/work_items_spec.rb @@ -620,7 +620,7 @@ stub_licensed_features(epics: true) end - it 'returns grou work items' do + it 'returns group work items' do post_graphql(query, current_user: current_user) work_items = graphql_data_at(:group, :workItems, :nodes) @@ -769,7 +769,7 @@ it 'does not return disabled work item types' do data = execute['data'][container_name]['workItems'] - expect(data['count']).to eq(20) + expect(data['count']).to eq(15) expect(data['edges'].count).to eq(15) expect(data['edges'].map { |node| node.dig('node', 'id') }).to match_array( issuables.flat_map(&:to_gid).map(&:to_s) @@ -794,7 +794,7 @@ it 'does not return licensed work items' do data = execute['data'][container_name]['workItems'] - expect(data['count']).to eq(10) + expect(data['count']).to eq(5) expect(data['edges'].count).to eq(5) expect(data['edges'].map { |node| node.dig('node', 'id') }).to match_array( project_issues.flat_map(&:to_gid).map(&:to_s) @@ -804,6 +804,39 @@ end end + context 'when skipping authorization' do + shared_examples 'request with skipped abilities' do |abilities = []| + it 'authorizes objects as expected' do + expect_any_instance_of(Gitlab::Graphql::Authorize::ObjectAuthorization) do |authorization| + expect(authorization).to receive(:ok).with( + group.work_items.first, + current_user, + scope_validator: nil, + skip_abilities: abilities + ) + end + + post_graphql(query, current_user: current_user) + end + end + + context 'when skip_work_items_field_authorization feature flag is enabled' do + before do + stub_feature_flags(skip_work_items_field_authorization: true) + end + + it_behaves_like 'request with skipped abilities', [:read_work_item] + end + + context 'when skip_work_items_field_authorization feature flag is disabled' do + before do + stub_feature_flags(skip_work_items_field_authorization: false) + end + + it_behaves_like 'request with skipped abilities', [] + end + end + def query(params = item_filter_params) graphql_query_for( 'group', diff --git a/spec/finders/work_items/work_items_finder_spec.rb b/spec/finders/work_items/work_items_finder_spec.rb index ab97d24483e963..8346b6b373d40b 100644 --- a/spec/finders/work_items/work_items_finder_spec.rb +++ b/spec/finders/work_items/work_items_finder_spec.rb @@ -16,8 +16,8 @@ context 'with start and end date filtering' do include_context '{Issues|WorkItems}Finder#execute context', :work_item - let_it_be(:work_item1) { create(:work_item, :epic, project: project1) } - let_it_be(:work_item2) { create(:work_item, :epic, project: project1) } + let_it_be(:work_item1) { create(:work_item, :issue, project: project1) } + let_it_be(:work_item2) { create(:work_item, :issue, project: project1) } let(:scope) { 'all' } let(:params) { { start_date: '2020-08-12', end_date: '2020-08-14', project_id: project1.id } } diff --git a/spec/requests/api/graphql/project/work_items_spec.rb b/spec/requests/api/graphql/project/work_items_spec.rb index cce9bdc549c516..368550bf1ddc1e 100644 --- a/spec/requests/api/graphql/project/work_items_spec.rb +++ b/spec/requests/api/graphql/project/work_items_spec.rb @@ -1082,6 +1082,39 @@ def pagination_query(params) end end + context 'when skipping authorization' do + shared_examples 'request with skipped abilities' do |abilities = []| + it 'authorizes objects as expected' do + expect_any_instance_of(Gitlab::Graphql::Authorize::ObjectAuthorization) do |authorization| + expect(authorization).to receive(:ok).with( + project.work_items.first, + current_user, + scope_validator: nil, + skip_abilities: abilities + ) + end + + post_graphql(query, current_user: current_user) + end + end + + context 'when skip_work_items_field_authorization feature flag is enabled' do + before do + stub_feature_flags(skip_work_items_field_authorization: true) + end + + it_behaves_like 'request with skipped abilities', [:read_work_item] + end + + context 'when skip_work_items_field_authorization feature flag is disabled' do + before do + stub_feature_flags(skip_work_items_field_authorization: false) + end + + it_behaves_like 'request with skipped abilities', [] + end + end + def item_ids graphql_dig_at(items_data, :id) end -- GitLab From 4d05be7fa7500b3361eccc9d49f645608bc85f90 Mon Sep 17 00:00:00 2001 From: Eugenia Grieff Date: Mon, 16 Jun 2025 14:13:37 +0200 Subject: [PATCH 2/3] Apply review suggestions - Remove redundant safe navigator - Dry issue_types_filter.rb - Improve readability --- .../resolvers/work_items/linked_items_resolver.rb | 12 +++++------- app/graphql/resolvers/work_items_resolver.rb | 2 +- .../skip_work_items_field_authorization.yml | 2 +- ee/app/finders/ee/issues/issue_types_filter.rb | 8 ++++++-- 4 files changed, 13 insertions(+), 11 deletions(-) diff --git a/app/graphql/resolvers/work_items/linked_items_resolver.rb b/app/graphql/resolvers/work_items/linked_items_resolver.rb index bc2c7b4f25a94d..4e3417a2586c06 100644 --- a/app/graphql/resolvers/work_items/linked_items_resolver.rb +++ b/app/graphql/resolvers/work_items/linked_items_resolver.rb @@ -59,13 +59,11 @@ def linked_items_grouped_by_source(linked_items, item_ids) end def can_read_linked_item?(item) - if work_item.work_item_type_id == item.work_item_type_id && - work_item.resource_parent == item.resource_parent && - work_item.confidential == item.confidential - true - else - current_user.can?(:read_work_item, item) - end + return true if work_item.work_item_type_id == item.work_item_type_id && + work_item.resource_parent == item.resource_parent && + work_item.confidential == item.confidential + + current_user.can?(:read_work_item, item) end end end diff --git a/app/graphql/resolvers/work_items_resolver.rb b/app/graphql/resolvers/work_items_resolver.rb index 9e51a61ad86cf7..2eecfb2de15449 100644 --- a/app/graphql/resolvers/work_items_resolver.rb +++ b/app/graphql/resolvers/work_items_resolver.rb @@ -82,7 +82,7 @@ def resource_parent end def skip_field_authorization? - Feature.enabled?(:skip_work_items_field_authorization, resource_parent&.root_ancestor, type: :gitlab_com_derisk) + Feature.enabled?(:skip_work_items_field_authorization, resource_parent.root_ancestor, type: :gitlab_com_derisk) end end end diff --git a/config/feature_flags/gitlab_com_derisk/skip_work_items_field_authorization.yml b/config/feature_flags/gitlab_com_derisk/skip_work_items_field_authorization.yml index 3995989f585b23..e4409eabb6c484 100644 --- a/config/feature_flags/gitlab_com_derisk/skip_work_items_field_authorization.yml +++ b/config/feature_flags/gitlab_com_derisk/skip_work_items_field_authorization.yml @@ -4,7 +4,7 @@ description: feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/542421 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/190860 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/548096 -milestone: '18.1' +milestone: '18.2' group: group::product planning type: gitlab_com_derisk default_enabled: false diff --git a/ee/app/finders/ee/issues/issue_types_filter.rb b/ee/app/finders/ee/issues/issue_types_filter.rb index 15f129f53c0c9d..50a6fc22e3a391 100644 --- a/ee/app/finders/ee/issues/issue_types_filter.rb +++ b/ee/app/finders/ee/issues/issue_types_filter.rb @@ -20,7 +20,7 @@ def by_issue_types(issues) # In namespace-level searches remove project-level epics if they're disabled for projects return without_epic_type_in_projects(issues) unless epic_type_allowed_in_group_projects? - param_types.any? ? issues.with_issue_type(param_types) : issues + filter_by_param_types(issues) end def authorize_issue_types? @@ -43,7 +43,7 @@ def epic_type_allowed_in_group_projects? def without_epic_type_in_projects(issues) project_epics = issues.with_issue_type('epic').project_level - filtered_issues = param_types.any? ? issues.with_issue_type(param_types) : issues + filtered_issues = filter_by_param_types(issues) filtered_issues.id_not_in(project_epics) end @@ -53,6 +53,10 @@ def without_epic_type(issues) allowed_param_types = param_types.excluding('epic') allowed_param_types.any? ? issues.with_issue_type(allowed_param_types) : issues.model.none end + + def filter_by_param_types(issues) + param_types.any? ? issues.with_issue_type(param_types) : issues + end end end # rubocop:enable Gitlab/BoundedContexts end -- GitLab From 8b6115382282705c75eb050aeee6136d72ab7732 Mon Sep 17 00:00:00 2001 From: Eugenia Grieff Date: Tue, 17 Jun 2025 12:40:55 +0200 Subject: [PATCH 3/3] Apply review suggestions - Rename feature flag to improve readability - Dry IssueTypeFilter --- app/finders/issues/issue_types_filter.rb | 8 +++++++- app/graphql/resolvers/work_items_resolver.rb | 2 +- ...zation.yml => authorize_issue_types_in_finder.yml} | 4 ++-- ee/app/finders/ee/issues/issue_types_filter.rb | 11 ++++------- ee/spec/requests/api/graphql/group/work_items_spec.rb | 8 ++++---- spec/requests/api/graphql/project/work_items_spec.rb | 8 ++++---- 6 files changed, 22 insertions(+), 19 deletions(-) rename config/feature_flags/gitlab_com_derisk/{skip_work_items_field_authorization.yml => authorize_issue_types_in_finder.yml} (73%) diff --git a/app/finders/issues/issue_types_filter.rb b/app/finders/issues/issue_types_filter.rb index 27c3c6f398066f..ad5b54e4c50c02 100644 --- a/app/finders/issues/issue_types_filter.rb +++ b/app/finders/issues/issue_types_filter.rb @@ -6,13 +6,19 @@ def filter(issues) by_issue_types(issues) end + private + def by_issue_types(issues) return issues if param_types.blank? - return issues.model.none unless (WorkItems::Type.base_types.keys & param_types).sort == param_types.sort + return issues.model.none unless valid_param_types? issues.with_issue_type(param_types) end + def valid_param_types? + (::WorkItems::Type.base_types.keys & param_types).sort == param_types.sort + end + def param_types Array.wrap(params[:issue_types]).map(&:to_s) end diff --git a/app/graphql/resolvers/work_items_resolver.rb b/app/graphql/resolvers/work_items_resolver.rb index 2eecfb2de15449..6dbae0c5fe6217 100644 --- a/app/graphql/resolvers/work_items_resolver.rb +++ b/app/graphql/resolvers/work_items_resolver.rb @@ -82,7 +82,7 @@ def resource_parent end def skip_field_authorization? - Feature.enabled?(:skip_work_items_field_authorization, resource_parent.root_ancestor, type: :gitlab_com_derisk) + Feature.enabled?(:authorize_issue_types_in_finder, resource_parent.root_ancestor, type: :gitlab_com_derisk) end end end diff --git a/config/feature_flags/gitlab_com_derisk/skip_work_items_field_authorization.yml b/config/feature_flags/gitlab_com_derisk/authorize_issue_types_in_finder.yml similarity index 73% rename from config/feature_flags/gitlab_com_derisk/skip_work_items_field_authorization.yml rename to config/feature_flags/gitlab_com_derisk/authorize_issue_types_in_finder.yml index e4409eabb6c484..22484f0aa0e32a 100644 --- a/config/feature_flags/gitlab_com_derisk/skip_work_items_field_authorization.yml +++ b/config/feature_flags/gitlab_com_derisk/authorize_issue_types_in_finder.yml @@ -1,6 +1,6 @@ --- -name: skip_work_items_field_authorization -description: +name: authorize_issue_types_in_finder +description: Enabled handling work items authorization solely in IssuesFinder feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/542421 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/190860 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/548096 diff --git a/ee/app/finders/ee/issues/issue_types_filter.rb b/ee/app/finders/ee/issues/issue_types_filter.rb index 50a6fc22e3a391..8ffb4e7867a96d 100644 --- a/ee/app/finders/ee/issues/issue_types_filter.rb +++ b/ee/app/finders/ee/issues/issue_types_filter.rb @@ -10,21 +10,18 @@ module IssueTypesFilter override :by_issue_types def by_issue_types(issues) return super unless authorize_issue_types? - return super if param_types.any? && param_types.exclude?('epic') - - if param_types.any? && (::WorkItems::Type.base_types.keys & param_types).sort != param_types.sort - return issues.model.none - end + return issues.model.none unless valid_param_types? return without_epic_type(issues) unless epic_type_allowed? - # In namespace-level searches remove project-level epics if they're disabled for projects return without_epic_type_in_projects(issues) unless epic_type_allowed_in_group_projects? filter_by_param_types(issues) end def authorize_issue_types? - ::Feature.enabled?(:skip_work_items_field_authorization, parent&.root_ancestor, type: :gitlab_com_derisk) + return false if param_types.any? && param_types.exclude?('epic') + + ::Feature.enabled?(:authorize_issue_types_in_finder, parent&.root_ancestor, type: :gitlab_com_derisk) end def epic_type_allowed? diff --git a/ee/spec/requests/api/graphql/group/work_items_spec.rb b/ee/spec/requests/api/graphql/group/work_items_spec.rb index dc2dc5c0375ffb..c7acaee0c8389b 100644 --- a/ee/spec/requests/api/graphql/group/work_items_spec.rb +++ b/ee/spec/requests/api/graphql/group/work_items_spec.rb @@ -820,17 +820,17 @@ end end - context 'when skip_work_items_field_authorization feature flag is enabled' do + context 'when authorize_issue_types_in_finder feature flag is enabled' do before do - stub_feature_flags(skip_work_items_field_authorization: true) + stub_feature_flags(authorize_issue_types_in_finder: true) end it_behaves_like 'request with skipped abilities', [:read_work_item] end - context 'when skip_work_items_field_authorization feature flag is disabled' do + context 'when authorize_issue_types_in_finder feature flag is disabled' do before do - stub_feature_flags(skip_work_items_field_authorization: false) + stub_feature_flags(authorize_issue_types_in_finder: false) end it_behaves_like 'request with skipped abilities', [] diff --git a/spec/requests/api/graphql/project/work_items_spec.rb b/spec/requests/api/graphql/project/work_items_spec.rb index 368550bf1ddc1e..dc17b0ba21ebc5 100644 --- a/spec/requests/api/graphql/project/work_items_spec.rb +++ b/spec/requests/api/graphql/project/work_items_spec.rb @@ -1098,17 +1098,17 @@ def pagination_query(params) end end - context 'when skip_work_items_field_authorization feature flag is enabled' do + context 'when authorize_issue_types_in_finder feature flag is enabled' do before do - stub_feature_flags(skip_work_items_field_authorization: true) + stub_feature_flags(authorize_issue_types_in_finder: true) end it_behaves_like 'request with skipped abilities', [:read_work_item] end - context 'when skip_work_items_field_authorization feature flag is disabled' do + context 'when authorize_issue_types_in_finder feature flag is disabled' do before do - stub_feature_flags(skip_work_items_field_authorization: false) + stub_feature_flags(authorize_issue_types_in_finder: false) end it_behaves_like 'request with skipped abilities', [] -- GitLab