Merge branch 'introduce-feature-flag-api' into 'master'

Support Create/Read/Destroy operations in Feature Flag API

See merge request gitlab-org/gitlab!18198

changelogs/unreleased/introduce-feature-flag-api.yml

+---
+title: Support Create/Read/Destroy operations in Feature Flag API
+merge_request: 18198
+author:
+type: added

ee/app/finders/feature_flags_finder.rb

@@ -10,7 +10,7 @@ class FeatureFlagsFinder
     @params = params
   end
 
-  def execute
+  def execute(preload: true)
     unless Ability.allowed?(current_user, :read_feature_flag, project)
       return Operations::FeatureFlag.none
     end
@@ -19,6 +19,7 @@ class FeatureFlagsFinder
     items = by_scope(items)
     items = for_list(items)
 
+    items = items.preload_relations if preload
     items.ordered
   end
 

ee/app/serializers/feature_flag_serializer.rb

@@ -5,10 +5,6 @@ class FeatureFlagSerializer < BaseSerializer
   entity FeatureFlagEntity
 
   def represent(resource, opts = {})
-    if resource.is_a?(ActiveRecord::Relation)
-      resource = resource.preload_relations
-    end
-
     super(resource, opts)
   end
 end

ee/app/services/feature_flags/create_service.rb

@@ -3,6 +3,8 @@
 module FeatureFlags
   class CreateService < FeatureFlags::BaseService
     def execute
+      return error('Access Denied', 403) unless can_create?
+
       ActiveRecord::Base.transaction do
         feature_flag = project.operations_feature_flags.new(params)
 
@@ -11,7 +13,7 @@ module FeatureFlags
 
           success(feature_flag: feature_flag)
         else
-          error(feature_flag.errors.full_messages)
+          error(feature_flag.errors.full_messages, 400)
         end
       end
     end
@@ -28,5 +30,9 @@ module FeatureFlags
 
       message_parts.join(" ")
     end
+
+    def can_create?
+      Ability.allowed?(current_user, :create_feature_flag, project)
+    end
   end
 end

ee/app/services/feature_flags/destroy_service.rb

@@ -3,6 +3,14 @@
 module FeatureFlags
   class DestroyService < FeatureFlags::BaseService
     def execute(feature_flag)
+      destroy_feature_flag(feature_flag)
+    end
+
+    private
+
+    def destroy_feature_flag(feature_flag)
+      return error('Access Denied', 403) unless can_destroy?(feature_flag)
+
       ActiveRecord::Base.transaction do
         if feature_flag.destroy
           save_audit_event(audit_event(feature_flag))
@@ -14,10 +22,12 @@ module FeatureFlags
       end
     end
 
-    private
-
     def audit_message(feature_flag)
       "Deleted feature flag <strong>#{feature_flag.name}</strong>."
     end
+
+    def can_destroy?(feature_flag)
+      Ability.allowed?(current_user, :destroy_feature_flag, feature_flag)
+    end
   end
 end

ee/app/services/feature_flags/update_service.rb

@@ -9,6 +9,8 @@ module FeatureFlags
     }.freeze
 
     def execute(feature_flag)
+      return error('Access Denied', 403) unless can_update?(feature_flag)
+
       ActiveRecord::Base.transaction do
         feature_flag.assign_attributes(params)
 
@@ -71,5 +73,9 @@ module FeatureFlags
 
       message + '.'
     end
+
+    def can_update?(feature_flag)
+      Ability.allowed?(current_user, :update_feature_flag, feature_flag)
+    end
   end
 end

ee/lib/api/feature_flags.rb

+# frozen_string_literal: true
+
+module API
+  class FeatureFlags < Grape::API
+    include PaginationParams
+
+    FEATURE_FLAG_ENDPOINT_REQUIREMENTS = API::NAMESPACE_OR_PROJECT_REQUIREMENTS
+        .merge(name: API::NO_SLASH_URL_PART_REGEX)
+
+    before do
+      not_found! unless Feature.enabled?(:feature_flag_api, user_project)
+      authorize_read_feature_flags!
+    end
+
+    params do
+      requires :id, type: String, desc: 'The ID of a project'
+    end
+    resource 'projects/:id', requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
+      resource :feature_flags do
+        desc 'Get all feature flags of a project' do
+          success EE::API::Entities::FeatureFlag
+        end
+        params do
+          optional :scope, type: String, desc: 'The scope of feature flags',
+                                         values: %w[enabled disabled]
+          use :pagination
+        end
+        get do
+          feature_flags = ::FeatureFlagsFinder
+            .new(user_project, current_user, declared_params(include_missing: false))
+            .execute
+
+          present paginate(feature_flags), with: EE::API::Entities::FeatureFlag
+        end
+
+        desc 'Create a new feature flag' do
+          success EE::API::Entities::FeatureFlag
+        end
+        params do
+          requires :name, type: String, desc: 'The name of feature flag'
+          optional :description, type: String, desc: 'The description of the feature flag'
+          optional :scopes, type: Array do
+            requires :environment_scope, type: String, desc: 'The environment scope of the scope'
+            requires :active, type: Boolean, desc: 'Active/inactive of the scope'
+            requires :strategies, type: JSON, desc: 'The strategies of the scope'
+          end
+        end
+        post do
+          authorize_create_feature_flag!
+
+          param = declared_params(include_missing: false)
+          param[:scopes_attributes] = param.delete(:scopes) if param.key?(:scopes)
+
+          result = ::FeatureFlags::CreateService
+            .new(user_project, current_user, param)
+            .execute
+
+          if result[:status] == :success
+            present result[:feature_flag], with: EE::API::Entities::FeatureFlag
+          else
+            render_api_error!(result[:message], result[:http_status])
+          end
+        end
+      end
+
+      params do
+        requires :name, type: String, desc: 'The name of the feature flag'
+      end
+      resource 'feature_flags/:name', requirements: FEATURE_FLAG_ENDPOINT_REQUIREMENTS do
+        desc 'Get a feature flag of a project' do
+          success EE::API::Entities::FeatureFlag
+        end
+        get do
+          authorize_read_feature_flag!
+
+          present feature_flag, with: EE::API::Entities::FeatureFlag
+        end
+
+        desc 'Delete a feature flag' do
+          success EE::API::Entities::FeatureFlag
+        end
+        delete do
+          authorize_destroy_feature_flag!
+
+          result = ::FeatureFlags::DestroyService
+            .new(user_project, current_user, declared_params(include_missing: false))
+            .execute(feature_flag)
+
+          if result[:status] == :success
+            present result[:feature_flag], with: EE::API::Entities::FeatureFlag
+          else
+            render_api_error!(result[:message], result[:http_status])
+          end
+        end
+      end
+    end
+
+    helpers do
+      def authorize_read_feature_flags!
+        authorize! :read_feature_flag, user_project
+      end
+
+      def authorize_read_feature_flag!
+        authorize! :read_feature_flag, feature_flag
+      end
+
+      def authorize_create_feature_flag!
+        authorize! :create_feature_flag, user_project
+      end
+
+      def authorize_destroy_feature_flag!
+        authorize! :destroy_feature_flag, feature_flag
+      end
+
+      def feature_flag
+        @feature_flag ||=
+          user_project.operations_feature_flags.find_by_name!(params[:name])
+      end
+    end
+  end
+end

ee/lib/ee/api/api.rb

@@ -18,6 +18,7 @@ module EE
         mount ::API::EpicIssues
         mount ::API::EpicLinks
         mount ::API::Epics
+        mount ::API::FeatureFlags
         mount ::API::ContainerRegistryEvent
         mount ::API::Geo
         mount ::API::GeoNodes

ee/lib/ee/api/entities.rb

@@ -843,6 +843,23 @@ module EE
           Ability.allowed?(user, :read_project_security_dashboard, project)
         end
       end
+
+      class FeatureFlag < Grape::Entity
+        class Scope < Grape::Entity
+          expose :id
+          expose :active
+          expose :environment_scope
+          expose :strategies
+          expose :created_at
+          expose :updated_at
+        end
+
+        expose :name
+        expose :description
+        expose :created_at
+        expose :updated_at
+        expose :scopes, using: Scope
+      end
     end
   end
 end

ee/spec/finders/feature_flags_finder_spec.rb

@@ -20,15 +20,22 @@ describe FeatureFlagsFinder do
   end
 
   describe '#execute' do
-    subject { finder.execute }
+    subject { finder.execute(args) }
 
     let!(:feature_flag_1) { create(:operations_feature_flag, name: 'flag-a', project: project) }
     let!(:feature_flag_2) { create(:operations_feature_flag, name: 'flag-b', project: project) }
+    let(:args) { {} }
 
     it 'returns feature flags ordered by name' do
       is_expected.to eq([feature_flag_1, feature_flag_2])
     end
 
+    it 'preloads relations by default' do
+      expect(Operations::FeatureFlag).to receive(:preload_relations).and_call_original
+
+      subject
+    end
+
     context 'when user is a reporter' do
       let(:user) { reporter }
 
@@ -58,6 +65,16 @@ describe FeatureFlagsFinder do
       end
     end
 
+    context 'when preload option is false' do
+      let(:args) { { preload: false } }
+
+      it 'does not preload relations' do
+        expect(Operations::FeatureFlag).not_to receive(:preload_relations)
+
+        subject
+      end
+    end
+
     context 'when it is presented for list' do
       let!(:feature_flag_1) { create(:operations_feature_flag, project: project, active: false) }
       let!(:feature_flag_2) { create(:operations_feature_flag, project: project, active: false) }

ee/spec/fixtures/api/schemas/public_api/v4/feature_flag.json

+{
+  "type": "object",
+  "required": ["name"],
+  "properties": {
+    "name": { "type": "string" },
+    "description": { "type": ["string", "null"] },
+    "created_at": { "type": "date" },
+    "updated_at": { "type": "date" },
+    "scopes": { "type": "array", "items": { "$ref": "feature_flag_scope.json" } }
+  },
+  "additionalProperties": false
+}

ee/spec/fixtures/api/schemas/public_api/v4/feature_flag_scope.json

+{
+  "type": "object",
+  "required": [
+    "id",
+    "environment_scope",
+    "active"
+  ],
+  "properties": {
+    "id": { "type": "integer" },
+    "environment_scope": { "type": "string" },
+    "active": { "type": "boolean" },
+    "created_at": { "type": "date" },
+    "updated_at": { "type": "date" },
+    "strategies": { "type": "array", "items": { "$ref": "feature_flag_strategy.json" } }
+  },
+  "additionalProperties": false
+}

ee/spec/fixtures/api/schemas/public_api/v4/feature_flag_strategy.json

+{
+  "type": "object",
+  "required": [
+    "name"
+  ],
+  "properties": {
+    "name": { "type": "string" },
+    "parameters": {
+      "type": "object"
+    }
+  },
+  "additionalProperties": false
+}

ee/spec/fixtures/api/schemas/public_api/v4/feature_flags.json

+{
+  "type": "array",
+  "items": {
+    "type": "object",
+    "properties": {
+      "$ref": "./feature_flag.json"
+    }
+  }
+}

ee/spec/requests/api/feature_flags_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe API::FeatureFlags do
+  include FeatureFlagHelpers
+
+  let(:project) { create(:project, :repository) }
+  let(:developer) { create(:user) }
+  let(:reporter) { create(:user) }
+  let(:user) { developer }
+  let(:non_project_member) { create(:user) }
+
+  before do
+    stub_licensed_features(feature_flags: true)
+
+    project.add_developer(developer)
+    project.add_reporter(reporter)
+  end
+
+  shared_examples_for 'check user permission' do
+    context 'when user is reporter' do
+      let(:user) { reporter }
+
+      it 'forbids the request' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
+    end
+  end
+
+  shared_examples_for 'not found' do
+    it 'returns Not Found' do
+      subject
+
+      expect(response).to have_gitlab_http_status(:not_found)
+    end
+  end
+
+  describe 'GET /projects/:id/feature_flags' do
+    subject { get api("/projects/#{project.id}/feature_flags", user) }
+
+    context 'when there are two feature flags' do
+      let!(:feature_flag_1) do
+        create(:operations_feature_flag, project: project)
+      end
+
+      let!(:feature_flag_2) do
+        create(:operations_feature_flag, project: project)
+      end
+
+      it 'returns feature flags ordered by name' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('public_api/v4/feature_flags', dir: 'ee')
+        expect(json_response.count).to eq(2)
+        expect(json_response.first['name']).to eq(feature_flag_1.name)
+        expect(json_response.second['name']).to eq(feature_flag_2.name)
+      end
+
+      it 'does not have N+1 problem' do
+        control_count = ActiveRecord::QueryRecorder.new { subject }
+
+        create_list(:operations_feature_flag, 3, project: project)
+
+        expect { get api("/projects/#{project.id}/feature_flags", user) }
+          .not_to exceed_query_limit(control_count)
+      end
+
+      it_behaves_like 'check user permission'
+    end
+  end
+
+  describe 'POST /projects/:id/feature_flags' do
+    subject do
+      post api("/projects/#{project.id}/feature_flags", user), params: params
+    end
+
+    let(:params) do
+      {
+        name: 'awesome-feature',
+        scopes: [default_scope]
+      }
+    end
+
+    it 'creates a new feature flag' do
+      subject
+
+      expect(response).to have_gitlab_http_status(:created)
+      expect(response).to match_response_schema('public_api/v4/feature_flag', dir: 'ee')
+
+      feature_flag = project.operations_feature_flags.last
+      expect(feature_flag.name).to eq(params[:name])
+      expect(feature_flag.description).to eq(params[:description])
+    end
+
+    it_behaves_like 'check user permission'
+
+    context 'when no scopes passed in parameters' do
+      let(:params) { { name: 'awesome-feature' } }
+
+      it 'creates a new feature flag with active default scope' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:created)
+        feature_flag = project.operations_feature_flags.last
+        expect(feature_flag.default_scope).to be_active
+      end
+    end
+
+    context 'when there is a feature flag with the same name already' do
+      before do
+        create_flag(project, 'awesome-feature')
+      end
+
+      it 'fails to create a new feature flag' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+      end
+    end
+
+    context 'when create a feature flag with two scopes' do
+      let(:params) do
+        {
+          name: 'awesome-feature',
+          description: 'this is awesome',
+          scopes: [
+            default_scope,
+            scope_with_user_with_id
+          ]
+        }
+      end
+
+      let(:scope_with_user_with_id) do
+        {
+          environment_scope: 'production',
+          active: true,
+          strategies: [{
+            name: 'userWithId',
+            parameters: { userIds: 'user:1' }
+          }].to_json
+        }
+      end
+
+      it 'creates a new feature flag with two scopes' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:created)
+
+        feature_flag = project.operations_feature_flags.last
+        feature_flag.scopes.ordered.each_with_index do |scope, index|
+          expect(scope.environment_scope).to eq(params[:scopes][index][:environment_scope])
+          expect(scope.active).to eq(params[:scopes][index][:active])
+          expect(scope.strategies).to eq(JSON.parse(params[:scopes][index][:strategies]))
+        end
+      end
+    end
+
+    def default_scope
+      {
+        environment_scope: '*',
+        active: false,
+        strategies: [{ name: 'default', parameters: {} }].to_json
+      }
+    end
+  end
+
+  describe 'GET /projects/:id/feature_flags/:name' do
+    subject { get api("/projects/#{project.id}/feature_flags/#{feature_flag.name}", user) }
+
+    context 'when there is a feature flag' do
+      let!(:feature_flag) { create_flag(project, 'awesome-feature') }
+
+      it 'returns a feature flag entry' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('public_api/v4/feature_flag', dir: 'ee')
+        expect(json_response['name']).to eq(feature_flag.name)
+        expect(json_response['description']).to eq(feature_flag.description)
+      end
+
+      it_behaves_like 'check user permission'
+    end
+  end
+
+  describe 'DELETE /projects/:id/feature_flags/:name' do
+    subject do
+      delete api("/projects/#{project.id}/feature_flags/#{feature_flag.name}", user),
+             params: params
+    end
+
+    let!(:feature_flag) { create(:operations_feature_flag, project: project) }
+    let(:params) { {} }
+
+    it 'destroys the feature flag' do
+      expect { subject }.to change { Operations::FeatureFlag.count }.by(-1)
+
+      expect(response).to have_gitlab_http_status(:ok)
+    end
+  end
+end

ee/spec/services/feature_flags/create_service_spec.rb

@@ -4,7 +4,15 @@ require 'spec_helper'
 
 describe FeatureFlags::CreateService do
   let(:project) { create(:project) }
-  let(:user) { create(:user) }
+  let(:developer) { create(:user) }
+  let(:reporter) { create(:user) }
+  let(:user) { developer }
+
+  before do
+    stub_licensed_features(feature_flags: true)
+    project.add_developer(developer)
+    project.add_reporter(reporter)
+  end
 
   describe '#execute' do
     subject do
@@ -57,6 +65,15 @@ describe FeatureFlags::CreateService do
         expect { subject }.to change { AuditEvent.count }.by(1)
         expect(AuditEvent.last.present.action).to eq(expected_message)
       end
+
+      context 'when user is reporter' do
+        let(:user) { reporter }
+
+        it 'returns error status' do
+          expect(subject[:status]).to eq(:error)
+          expect(subject[:message]).to eq('Access Denied')
+        end
+      end
     end
   end
 end

ee/spec/services/feature_flags/destroy_service_spec.rb

@@ -3,13 +3,25 @@
 require 'spec_helper'
 
 describe FeatureFlags::DestroyService do
+  include FeatureFlagHelpers
+
   let(:project) { create(:project) }
-  let(:user) { create(:user) }
-  let!(:feature_flag) { create(:operations_feature_flag) }
+  let(:developer) { create(:user) }
+  let(:reporter) { create(:user) }
+  let(:user) { developer }
+  let!(:feature_flag) { create(:operations_feature_flag, project: project) }
+
+  before do
+    stub_licensed_features(feature_flags: true)
+    project.add_developer(developer)
+    project.add_reporter(reporter)
+  end
 
   describe '#execute' do
-    subject { described_class.new(project, user).execute(feature_flag) }
+    subject { described_class.new(project, user, params).execute(feature_flag) }
+
     let(:audit_event_message) { AuditEvent.last.present.action }
+    let(:params) { {} }
 
     it 'returns status success' do
       expect(subject[:status]).to eq(:success)
@@ -24,6 +36,15 @@ describe FeatureFlags::DestroyService do
       expect(audit_event_message).to eq("Deleted feature flag <strong>#{feature_flag.name}</strong>.")
     end
 
+    context 'when user is reporter' do
+      let(:user) { reporter }
+
+      it 'returns error status' do
+        expect(subject[:status]).to eq(:error)
+        expect(subject[:message]).to eq('Access Denied')
+      end
+    end
+
     context 'when feature flag can not be destroyed' do
       before do
         allow(feature_flag).to receive(:destroy).and_return(false)

ee/spec/services/feature_flags/update_service_spec.rb

@@ -4,8 +4,16 @@ require 'spec_helper'
 
 describe FeatureFlags::UpdateService do
   let(:project) { create(:project) }
-  let(:user) { create(:user) }
-  let(:feature_flag) { create(:operations_feature_flag) }
+  let(:developer) { create(:user) }
+  let(:reporter) { create(:user) }
+  let(:user) { developer }
+  let(:feature_flag) { create(:operations_feature_flag, project: project) }
+
+  before do
+    stub_licensed_features(feature_flags: true)
+    project.add_developer(developer)
+    project.add_reporter(reporter)
+  end
 
   describe '#execute' do
     subject { described_class.new(project, user, params).execute(feature_flag) }
@@ -45,6 +53,15 @@ describe FeatureFlags::UpdateService do
       end
     end
 
+    context 'when user is reporter' do
+      let(:user) { reporter }
+
+      it 'returns error status' do
+        expect(subject[:status]).to eq(:error)
+        expect(subject[:message]).to eq('Access Denied')
+      end
+    end
+
     context 'when nothing is changed' do
       let(:params) { {} }
 

ee/spec/support/helpers/feature_flag_helpers.rb

 # frozen_string_literal: true
 
 module FeatureFlagHelpers
-  def create_flag(project, name, active, description: nil)
+  def create_flag(project, name, active = true, description: nil)
     create(:operations_feature_flag, name: name, active: active,
                                      description: description, project: project)
   end
 
-  def create_scope(feature_flag, environment_scope, active, strategies = [{ name: "default", parameters: {} }])
+  def create_scope(feature_flag, environment_scope, active = true, strategies = [{ name: "default", parameters: {} }])
     create(:operations_feature_flag_scope,
       feature_flag: feature_flag,
       environment_scope: environment_scope,