Merge branch 'reference-feature-flags-from-issues-be' into 'master'

Backend For Referencing Feature Flags from Issues

See merge request gitlab-org/gitlab!52144

app/models/ability.rb

@@ -54,6 +54,14 @@ class Ability
       end
     end
 
+    def feature_flags_readable_by_user(feature_flags, user = nil, filters: {})
+      feature_flags = apply_filters_if_needed(feature_flags, user, filters)
+
+      DeclarativePolicy.user_scope do
+        feature_flags.select { |flag| allowed?(user, :read_feature_flag, flag) }
+      end
+    end
+
     def allowed?(user, ability, subject = :global, opts = {})
       if subject.is_a?(Hash)
         opts = subject

ee/app/controllers/projects/issue_feature_flags_controller.rb

+# frozen_string_literal: true
+
+module Projects
+  class IssueFeatureFlagsController < Projects::ApplicationController
+    include IssuableLinks
+
+    before_action :authorize_admin_feature_flags_issue_links!
+
+    feature_category :feature_flags
+
+    private
+
+    def list_service
+      ::IssueFeatureFlags::ListService.new(issue, current_user)
+    end
+
+    def link
+      @link ||= ::FeatureFlagIssue.find(params[:id])
+    end
+
+    def issue
+      project.issues.find_by_iid(params[:issue_id])
+    end
+  end
+end

ee/app/models/ee/issue.rb

@@ -218,6 +218,20 @@ module EE
       SQL
     end
 
+    def related_feature_flags(current_user, preload: nil)
+      feature_flags = ::Operations::FeatureFlag
+        .select('operations_feature_flags.*, operations_feature_flags_issues.id AS link_id')
+        .joins(:feature_flag_issues)
+        .where(operations_feature_flags_issues: { issue_id: id })
+        .order('operations_feature_flags_issues.id ASC')
+        .includes(preload)
+
+      cross_project_filter = -> (feature_flags) { feature_flags.where(project: project) }
+      Ability.feature_flags_readable_by_user(feature_flags,
+        current_user,
+        filters: { read_cross_project: cross_project_filter })
+    end
+
     override :relocation_target
     def relocation_target
       super || promoted_to_epic

ee/app/serializers/issues/linked_issue_feature_flag_entity.rb

+# frozen_string_literal: true
+
+module Issues
+  class LinkedIssueFeatureFlagEntity < Grape::Entity
+    include RequestAwareEntity
+
+    expose :id, :name, :iid
+
+    expose :active
+
+    expose :path do |link|
+      project_feature_flag_path(link.project, link.iid)
+    end
+
+    expose :reference do |link|
+      link.to_reference(issuable.project)
+    end
+
+    expose :link_type do |_issue|
+      'relates_to'
+    end
+
+    def issuable
+      request.issuable
+    end
+  end
+end

ee/app/serializers/issues/linked_issue_feature_flag_serializer.rb

+# frozen_string_literal: true
+
+module Issues
+  class LinkedIssueFeatureFlagSerializer < BaseSerializer
+    entity LinkedIssueFeatureFlagEntity
+  end
+end

ee/app/services/issue_feature_flags/list_service.rb

+# frozen_string_literal: true
+
+module IssueFeatureFlags
+  class ListService < IssuableLinks::ListService
+    extend ::Gitlab::Utils::Override
+
+    private
+
+    def child_issuables
+      issuable.related_feature_flags(current_user, preload: preload_for_collection)
+    end
+
+    override :serializer
+    def serializer
+      Issues::LinkedIssueFeatureFlagSerializer
+    end
+
+    override :preload_for_collection
+    def preload_for_collection
+      [{ project: :namespace }]
+    end
+  end
+end

ee/config/routes/issues.rb

@@ -5,4 +5,5 @@ resources :issues, only: [], constraints: { id: /\d+/ } do
     get '/descriptions/:version_id/diff', action: :description_diff, as: :description_diff
     delete '/descriptions/:version_id', action: :delete_description_version, as: :delete_description_version
   end
+  resources :issue_feature_flags, only: [:index, :show], as: 'feature_flags', path: 'feature_flags'
 end

ee/spec/models/issue_spec.rb

@@ -1039,6 +1039,44 @@ RSpec.describe Issue do
     end
   end
 
+  describe '#related_feature_flags' do
+    let_it_be(:user) { create(:user) }
+
+    let_it_be(:authorized_project) { create(:project) }
+    let_it_be(:authorized_project2) { create(:project) }
+    let_it_be(:unauthorized_project) { create(:project) }
+
+    let_it_be(:issue) { create(:issue, project: authorized_project) }
+
+    let_it_be(:authorized_feature_flag) { create(:operations_feature_flag, project: authorized_project) }
+    let_it_be(:authorized_feature_flag_b) { create(:operations_feature_flag, project: authorized_project2) }
+
+    let_it_be(:unauthorized_feature_flag) { create(:operations_feature_flag, project: unauthorized_project) }
+
+    let_it_be(:issue_link_a) { create(:feature_flag_issue, issue: issue, feature_flag: authorized_feature_flag) }
+    let_it_be(:issue_link_b) { create(:feature_flag_issue, issue: issue, feature_flag: unauthorized_feature_flag) }
+    let_it_be(:issue_link_c) { create(:feature_flag_issue, issue: issue, feature_flag: authorized_feature_flag_b) }
+
+    before_all do
+      authorized_project.add_developer(user)
+      authorized_project2.add_developer(user)
+    end
+
+    it 'returns only authorized related feature flags for a given user' do
+      expect(issue.related_feature_flags(user)).to contain_exactly(authorized_feature_flag, authorized_feature_flag_b)
+    end
+
+    describe 'when a user cannot read cross project' do
+      it 'only returns feature_flags within the same project' do
+        expect(Ability).to receive(:allowed?).with(user, :read_feature_flag, authorized_feature_flag).and_return(true)
+        expect(Ability).to receive(:allowed?).with(user, :read_cross_project).and_return(false)
+
+        expect(issue.related_feature_flags(user))
+          .to contain_exactly(authorized_feature_flag)
+      end
+    end
+  end
+
   describe '.with_issue_type' do
     let_it_be(:project) { create(:project) }
     let_it_be(:issue) { create(:issue, project: project) }

ee/spec/requests/projects/issue_feature_flags_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Projects::IssueFeatureFlagsController do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:developer) { create(:user) }
+  let_it_be(:reporter) { create(:user) }
+
+  before_all do
+    project.add_developer(developer)
+    project.add_reporter(reporter)
+  end
+
+  before do
+    stub_licensed_features(feature_flags_related_issues: true)
+  end
+
+  describe 'GET #index' do
+    def setup
+      feature_flag = create(:operations_feature_flag, project: project)
+      issue = create(:issue, project: project)
+      link = create(:feature_flag_issue, feature_flag: feature_flag, issue: issue)
+
+      [feature_flag, issue, link]
+    end
+
+    def get_request(project, issue)
+      get project_issue_feature_flags_path(project, issue, format: :json)
+    end
+
+    it 'returns linked feature flags' do
+      feature_flag, issue = setup
+      sign_in(developer)
+
+      get_request(project, issue)
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response).to match([a_hash_including({
+        'id' => feature_flag.id
+      })])
+    end
+
+    it 'does not return linked feature flags for a reporter' do
+      _, issue, _ = setup
+      sign_in(reporter)
+
+      get_request(project, issue)
+
+      expect(response).to have_gitlab_http_status(:not_found)
+    end
+
+    it 'orders by feature_flag_issue id' do
+      issue = create(:issue, project: project)
+      feature_flag_a = create(:operations_feature_flag, project: project)
+      feature_flag_b = create(:operations_feature_flag, project: project)
+      create(:feature_flag_issue, feature_flag: feature_flag_b, issue: issue)
+      create(:feature_flag_issue, feature_flag: feature_flag_a, issue: issue)
+      sign_in(developer)
+
+      get_request(project, issue)
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response.map { |feature_flag| feature_flag['id'] }).to eq([feature_flag_b.id, feature_flag_a.id])
+    end
+
+    it 'does not make N+1 queries' do
+      feature_flag, _, _ = setup
+      sign_in(developer)
+
+      control_count = ActiveRecord::QueryRecorder.new { get_request(project, feature_flag) }.count
+
+      issue_b = create(:issue, project: project)
+      issue_c = create(:issue, project: project)
+      create(:feature_flag_issue, feature_flag: feature_flag, issue: issue_b)
+      create(:feature_flag_issue, feature_flag: feature_flag, issue: issue_c)
+
+      expect { get_request(project, feature_flag) }.not_to exceed_query_limit(control_count)
+    end
+
+    context 'when feature flag related issues feature is unlicensed' do
+      before do
+        stub_licensed_features(feature_flags_related_issues: false)
+      end
+
+      it 'returns not found' do
+        feature_flag, _, _ = setup
+        sign_in(developer)
+
+        get_request(project, feature_flag)
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+  end
+end

ee/spec/serializers/issues/linked_issue_feature_flag_entity_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Issues::LinkedIssueFeatureFlagEntity do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:developer) { create(:user) }
+
+  before_all do
+    project.add_developer(developer)
+  end
+
+  describe '#as_json' do
+    it 'returns json' do
+      issue = create(:issue, project: project)
+      feature_flag = create(:operations_feature_flag, project: project)
+      link = create(:feature_flag_issue, feature_flag: feature_flag, issue: issue)
+      allow(issue).to receive(:link_id).and_return(link.id)
+      request = double('request')
+      allow(request).to receive(:current_user).and_return(developer)
+      allow(request).to receive(:issuable).and_return(issue)
+      entity = described_class.new(feature_flag, request: request, current_user: developer)
+
+      expect(entity.as_json.slice(:link_type, :path, :reference)).to eq({
+        link_type: 'relates_to',
+        path: "/#{project.full_path}/-/feature_flags/#{feature_flag.iid}",
+        reference: "[feature_flag:#{feature_flag.iid}]"
+      })
+    end
+  end
+end

ee/spec/services/issue_feature_flags/list_service_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe IssueFeatureFlags::ListService do
+  let(:user) { create(:user) }
+  let(:project) { create(:project_empty_repo, :private) }
+  let(:issue) { create(:issue, project: project) }
+  let(:feature_flag) { create(:operations_feature_flag, project: project) }
+
+  describe '#execute' do
+    subject { described_class.new(issue, user).execute }
+
+    let(:feature_flag_b) { create(:operations_feature_flag, project: project) }
+    let(:feature_flag_c) { create(:operations_feature_flag, project: project) }
+    let(:feature_flag_d) { create(:operations_feature_flag, project: project) }
+
+    before do
+      create(:feature_flag_issue, feature_flag: feature_flag_b, issue: issue)
+      create(:feature_flag_issue, feature_flag: feature_flag_c, issue: issue)
+      create(:feature_flag_issue, feature_flag: feature_flag_d, issue: issue)
+      create(:feature_flag_issue, feature_flag: feature_flag, issue: issue)
+    end
+
+    context 'when user can see feature flags' do
+      before do
+        project.add_developer(user)
+      end
+
+      it 'ensures no N+1 queries are made' do
+        control_count = ActiveRecord::QueryRecorder.new { described_class.new(issue, user).execute }.count
+
+        expect { described_class.new(issue, user).execute }.not_to exceed_query_limit(control_count)
+      end
+
+      it 'returns related feature flags' do
+        expect(subject.size).to eq(4)
+
+        expect(subject).to include(include(id: feature_flag.id,
+                                           name: feature_flag.name,
+                                           path: "/#{project.full_path}/-/feature_flags/#{feature_flag.iid}"))
+        expect(subject).to include(include(id: feature_flag_b.id,
+                                           name: feature_flag_b.name,
+                                           path: "/#{project.full_path}/-/feature_flags/#{feature_flag_b.iid}"))
+        expect(subject).to include(include(id: feature_flag_c.id,
+                                           name: feature_flag_c.name,
+                                           path: "/#{project.full_path}/-/feature_flags/#{feature_flag_c.iid}"))
+        expect(subject).to include(include(id: feature_flag_d.id,
+                                           name: feature_flag_d.name,
+                                           path: "/#{project.full_path}/-/feature_flags/#{feature_flag_d.iid}"))
+      end
+    end
+
+    context 'when user can not see feature flags' do
+      it 'returns nothing' do
+        expect(subject.size).to eq(0)
+      end
+    end
+  end
+end

spec/models/ability_spec.rb

@@ -328,6 +328,69 @@ RSpec.describe Ability do
     end
   end
 
+  describe '.feature_flags_readable_by_user' do
+    context 'without a user' do
+      it 'returns no feature flags' do
+        feature_flag_1 = build(:operations_feature_flag)
+        feature_flag_2 = build(:operations_feature_flag, project: build(:project, :public))
+
+        feature_flags = described_class
+            .feature_flags_readable_by_user([feature_flag_1, feature_flag_2])
+
+        expect(feature_flags).to eq([])
+      end
+    end
+
+    context 'with a user' do
+      let(:user) { create(:user) }
+      let(:project) { create(:project) }
+      let(:feature_flag) { create(:operations_feature_flag, project: project) }
+      let(:cross_project) { create(:project) }
+      let(:cross_project_feature_flag) { create(:operations_feature_flag, project: cross_project) }
+
+      let(:other_feature_flag) { create(:operations_feature_flag) }
+      let(:all_feature_flags) do
+        [feature_flag, cross_project_feature_flag, other_feature_flag]
+      end
+
+      subject(:readable_feature_flags) do
+        described_class.feature_flags_readable_by_user(all_feature_flags, user)
+      end
+
+      before do
+        project.add_developer(user)
+        cross_project.add_developer(user)
+      end
+
+      it 'returns feature flags visible to the user' do
+        expect(readable_feature_flags).to contain_exactly(feature_flag, cross_project_feature_flag)
+      end
+
+      context 'when a user cannot read cross project and a filter is passed' do
+        before do
+          allow(described_class).to receive(:allowed?).and_call_original
+          expect(described_class).to receive(:allowed?).with(user, :read_cross_project) { false }
+        end
+
+        subject(:readable_feature_flags) do
+          read_cross_project_filter = -> (feature_flags) do
+            feature_flags.select { |flag| flag.project == project }
+          end
+          described_class.feature_flags_readable_by_user(
+            all_feature_flags, user,
+            filters: { read_cross_project: read_cross_project_filter }
+          )
+        end
+
+        it 'returns only feature flags of the specified project without checking access on others' do
+          expect(described_class).not_to receive(:allowed?).with(user, :read_feature_flag, cross_project_feature_flag)
+
+          expect(readable_feature_flags).to contain_exactly(feature_flag)
+        end
+      end
+    end
+  end
+
   describe '.project_disabled_features_rules' do
     let(:project) { create(:project, :wiki_disabled) }