Merge branch 'security-idor-ff-user-list' into 'master'

Forbid Setting a gitlabUserList Strategy to a List From Another Project

See merge request gitlab-org/security/gitlab!1044

app/controllers/projects/feature_flags_controller.rb

@@ -77,7 +77,7 @@ class Projects::FeatureFlagsController < Projects::ApplicationController
       end
     else
       respond_to do |format|
-        format.json { render_error_json(result[:message]) }
+        format.json { render_error_json(result[:message], result[:http_status]) }
       end
     end
   end
@@ -167,8 +167,8 @@ class Projects::FeatureFlagsController < Projects::ApplicationController
     render json: feature_flag_json(feature_flag), status: :ok
   end
 
-  def render_error_json(messages)
+  def render_error_json(messages, status = :bad_request)
     render json: { message: messages },
-           status: :bad_request
+           status: status
   end
 end

app/models/operations/feature_flags/user_list.rb

@@ -28,6 +28,11 @@ module Operations
         fuzzy_search(query, [:name], use_minimum_char_limit: false)
       end
 
+      def self.belongs_to?(project_id, user_list_ids)
+        uniq_ids = user_list_ids.uniq
+        where(id: uniq_ids, project_id: project_id).count == uniq_ids.count
+      end
+
       private
 
       def ensure_no_associated_strategies

app/services/feature_flags/update_service.rb

@@ -10,6 +10,7 @@ module FeatureFlags
 
     def execute(feature_flag)
       return error('Access Denied', 403) unless can_update?(feature_flag)
+      return error('Not Found', 404) unless valid_user_list_ids?(feature_flag, user_list_ids(params))
 
       ActiveRecord::Base.transaction do
         feature_flag.assign_attributes(params)
@@ -87,5 +88,15 @@ module FeatureFlags
     def can_update?(feature_flag)
       Ability.allowed?(current_user, :update_feature_flag, feature_flag)
     end
+
+    def user_list_ids(params)
+      params.fetch(:strategies_attributes, [])
+        .select { |s| s[:user_list_id].present? }
+        .map { |s| s[:user_list_id] }
+    end
+
+    def valid_user_list_ids?(feature_flag, user_list_ids)
+      user_list_ids.empty? || ::Operations::FeatureFlags::UserList.belongs_to?(feature_flag.project_id, user_list_ids)
+    end
   end
 end

changelogs/unreleased/security-idor-ff-user-list.yml

+---
+title: Forbid setting a gitlabUserList strategy to a list from another project
+merge_request:
+author:
+type: security

spec/controllers/projects/feature_flags_controller_spec.rb

@@ -1511,6 +1511,40 @@ RSpec.describe Projects::FeatureFlagsController do
         expect(response).to have_gitlab_http_status(:not_found)
       end
 
+      it 'returns not found when trying to update a gitlabUserList strategy with a user list from another project' do
+        user_list = create(:operations_feature_flag_user_list, project: project, name: 'My List', user_xids: 'user1,user2')
+        strategy = create(:operations_strategy, feature_flag: new_version_flag, name: 'gitlabUserList', parameters: {}, user_list: user_list)
+        other_project = create(:project)
+        other_user_list = create(:operations_feature_flag_user_list, project: other_project, name: 'Other List', user_xids: 'some,one')
+
+        put_request(new_version_flag, strategies_attributes: [{
+          id: strategy.id,
+          user_list_id: other_user_list.id
+        }])
+
+        expect(response).to have_gitlab_http_status(:not_found)
+        expect(strategy.reload.user_list).to eq(user_list)
+      end
+
+      it 'allows setting multiple gitlabUserList strategies to the same user list' do
+        user_list_a = create(:operations_feature_flag_user_list, project: project, name: 'My List A', user_xids: 'user1,user2')
+        user_list_b = create(:operations_feature_flag_user_list, project: project, name: 'My List B', user_xids: 'user3,user4')
+        strategy_a = create(:operations_strategy, feature_flag: new_version_flag, name: 'gitlabUserList', parameters: {}, user_list: user_list_a)
+        strategy_b = create(:operations_strategy, feature_flag: new_version_flag, name: 'gitlabUserList', parameters: {}, user_list: user_list_a)
+
+        put_request(new_version_flag, strategies_attributes: [{
+          id: strategy_a.id,
+          user_list_id: user_list_b.id
+        }, {
+          id: strategy_b.id,
+          user_list_id: user_list_b.id
+        }])
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(strategy_a.reload.user_list).to eq(user_list_b)
+        expect(strategy_b.reload.user_list).to eq(user_list_b)
+      end
+
       it 'updates an existing strategy' do
         strategy = create(:operations_strategy, feature_flag: new_version_flag, name: 'default', parameters: {})