Refactor whitelisting of filter params

app/controllers/concerns/issuable_collections.rb

@@ -81,36 +81,36 @@ module IssuableCollections
   end
   def issuable_finder_for(finder_class)
-    finder_class.new(current_user, filter_params)
+    finder_class.new(current_user, finder_options)
   end
   # rubocop:disable Gitlab/ModuleWithInstanceVariables
-  # rubocop: disable CodeReuse/ActiveRecord
-  def filter_params
-    set_sort_order_from_cookie
-    set_default_state
-    # Skip irrelevant Rails routing params
-    @filter_params = params.dup.except(:controller, :action, :namespace_id)
-    @filter_params[:sort] ||= default_sort_order
-    @sort = @filter_params[:sort]
+  def finder_options
+    params[:state] = default_state if params[:state].blank?
+    options = {
+      scope: params[:scope],
+      state: params[:state],
+      sort: set_sort_order_from_cookie || default_sort_order
+    }
+    # Used by view to highlight active option
+    @sort = options[:sort]
     if @project
-      @filter_params[:project_id] = @project.id
+      options[:project_id] = @project.id
     elsif @group
-      @filter_params[:group_id] = @group.id
-      @filter_params[:include_subgroups] = true
-      @filter_params[:use_cte_for_search] = true
+      options[:group_id] = @group.id
+      options[:include_subgroups] = true
+      options[:use_cte_for_search] = true
     end
-    @filter_params.permit(finder_type.valid_params)
+    params.permit(finder_type.valid_params).merge(options)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
   # rubocop:enable Gitlab/ModuleWithInstanceVariables
-  def set_default_state
-    params[:state] = 'opened' if params[:state].blank?
+  def default_state
+    'opened'
   end
   def set_sort_order_from_cookie
@@ -121,7 +121,7 @@ module IssuableCollections
     sort_value = update_cookie_value(sort_param)
     set_secure_cookie(remember_sorting_key, sort_value)
-    params[:sort] = sort_value
+    sort_value
   end
   def remember_sorting_key

app/controllers/concerns/merge_requests_action.rb

@@ -19,7 +19,7 @@ module MergeRequestsAction
       (MergeRequestsFinder if action_name == 'merge_requests')
   end
-  def filter_params
+  def finder_options
     super.merge(non_archived: true)
   end
 end

app/controllers/dashboard_controller.rb

@@ -4,17 +4,6 @@ class DashboardController < Dashboard::ApplicationController
   include IssuesAction
   include MergeRequestsAction
-  FILTER_PARAMS = [
-    # author_id and assignee_id are kept so old RSS links still work
-    :author_id,
-    :assignee_id,
-    :author_username,
-    :assignee_username,
-    :milestone_title,
-    :label_name,
-    :my_reaction_emoji
-  ].freeze
-
   before_action :event_filter, only: :activity
   before_action :projects, only: [:issues, :merge_requests]
   before_action :set_show_full_reference, only: [:issues, :merge_requests]
@@ -55,10 +44,13 @@ class DashboardController < Dashboard::ApplicationController
   end
   def check_filters_presence!
-    @no_filters_set = FILTER_PARAMS.none? { |k| params.key?(k) }
+    @no_filters_set = finder_type.scalar_params.none? { |k| params.key?(k) }
     return unless @no_filters_set
+    # Call to set selected `state` and `sort` options in view
+    finder_options
+
     respond_to do |format|
       format.html { render }
       format.atom { head :bad_request }

app/finders/issuable_finder.rb

@@ -14,7 +14,9 @@
 #     project_id: integer
 #     milestone_title: string
 #     author_id: integer
+#     author_username: string
 #     assignee_id: integer or 'None' or 'Any'
+#     assignee_username: string
 #     search: string
 #     label_name: string
 #     sort: string
@@ -49,25 +51,15 @@ class IssuableFinder
       assignee_username
       author_id
       author_username
-      authorized_only
-      group_id
-      iids
       label_name
       milestone_title
       my_reaction_emoji
-      non_archived
-      project_id
-      scope
       search
-      sort
-      state
-      include_subgroups
-      use_cte_for_search
     ]
   end
   def self.array_params
-    @array_params ||= { label_name: [], iids: [], assignee_username: [] }
+    @array_params ||= { label_name: [], assignee_username: [] }
   end
   def self.valid_params

app/helpers/application_helper.rb

@@ -173,19 +173,7 @@ module ApplicationHelper
     without = options.delete(:without)
     add_label = options.delete(:label)
-    exist_opts = {
-      state: params[:state],
-      scope: params[:scope],
-      milestone_title: params[:milestone_title],
-      assignee_username: params[:assignee_username],
-      author_username: params[:author_username],
-      search: params[:search],
-      label_name: params[:label_name],
-      my_reaction_emoji: params[:my_reaction_emoji],
-      wip: params[:wip]
-    }
-
-    options = exist_opts.merge(options)
+    options = request.query_parameters.merge(options)
     if without.present?
       without.each do |key|

spec/controllers/concerns/issuable_collections_spec.rb

@@ -60,7 +60,7 @@ describe IssuableCollections do
     end
   end
-  describe '#filter_params' do
+  describe '#finder_options' do
     let(:params) do
       {
         assignee_id: '1',
@@ -84,25 +84,20 @@ describe IssuableCollections do
       }
     end
-    it 'filters params' do
+    it 'only allows whitelisted params' do
       allow(controller).to receive(:cookies).and_return({})
-      filtered_params = controller.send(:filter_params)
-      expect(filtered_params).to eq({
+      finder_options = controller.send(:finder_options)
+      expect(finder_options).to eq({
         'assignee_id' => '1',
         'assignee_username' => 'user1',
         'author_id' => '2',
         'author_username' => 'user2',
-        'authorized_only' => 'true',
-        'due_date' => '2017-01-01',
-        'group_id' => '3',
-        'iids' => '4',
         'label_name' => 'foo',
         'milestone_title' => 'bar',
         'my_reaction_emoji' => 'thumbsup',
-        'non_archived' => 'true',
-        'project_id' => '5',
+        'due_date' => '2017-01-01',
         'scope' => 'all',
         'search' => 'baz',
         'sort' => 'priority',