Add constraints for feature report routes

Rather than allowing any method on FeatureReportService to be called
(and potentially opening up a security hole), this commit adds some
metaprogramming machinery to manage a list of methods which are feature
reports and adds methods to allow that to be used safely, both to
constrain the parameters in the routes and to prevent arbitrary methods
from being called from the controller.

Author: lfdebrux

app/controllers/reports_controller.rb

@@ -22,10 +22,8 @@ def questions_with_answer_type
   def feature_report
     report = params[:report].underscore
 
-    raise ActionController::RoutingError unless Reports::FeatureReportService.method_defined? report
-
     forms = Reports::FormDocumentsService.live_form_documents
-    records = Reports::FeatureReportService.new(forms).send report
+    records = Reports::FeatureReportService.new(forms).report report
 
     if params[:format] == "csv"
       send_data Reports::CsvReportService.new(records).csv,

app/services/reports/feature_report_service.rb

@@ -1,11 +1,37 @@
 class Reports::FeatureReportService
+  def self.define_report(name, &block)
+    @reports ||= []
+    @reports << name
+
+    define_method(name, &block)
+  end
+
+  def self.matches_report?(name)
+    @reports.include? name.to_sym
+  end
+
+  class Constraint
+    def self.matches?(request)
+      name = request.params[:report].underscore
+      Reports::FeatureReportService.matches_report? name
+    end
+  end
+
+  private_class_method :define_report
+
   attr_reader :form_documents
 
   def initialize(form_documents)
     @form_documents = form_documents
   end
 
-  def report
+  def report(name = nil)
+    unless name.nil?
+      raise "'#{name}' is not a defined report" unless self.class.matches_report?(name)
+
+      return send(name)
+    end
+
     report = {
       total_forms: 0,
       forms_with_payment: 0,
@@ -54,27 +80,27 @@ def questions_with_answer_type(answer_type)
     end
   end
 
-  def questions_with_add_another_answer
+  define_report :questions_with_add_another_answer do
     form_documents.flat_map do |form|
       form["content"]["steps"]
         .select { |step| step["data"]["is_repeatable"] }
         .map { |step| questions_details(form, step) }
     end
   end
 
-  def forms_with_routes
+  define_report :forms_with_routes do
     form_documents
       .select { |form| Reports::FormDocumentsService.has_routes?(form) }
       .map { |form| form_with_routes_details(form) }
   end
 
-  def forms_with_payments
+  define_report :forms_with_payments do
     form_documents
       .select { |form| Reports::FormDocumentsService.has_payments?(form) }
       .map { |form| form_details(form) }
   end
 
-  def forms_with_csv_submission_enabled
+  define_report :forms_with_csv_submission_enabled do
     form_documents
       .select { |form| Reports::FormDocumentsService.has_csv_submission_enabled?(form) }
       .map { |form| form_details(form) }

config/routes.rb

@@ -196,7 +196,7 @@
     scope "/features" do
       get "/", to: "reports#features", as: :report_features
       get "/questions-with-answer-type/:answer_type", to: "reports#questions_with_answer_type", as: :report_questions_with_answer_type
-      get "/:report", to: "reports#feature_report", as: :feature_report
+      get "/:report", constraints: Reports::FeatureReportService::Constraint, to: "reports#feature_report", as: :feature_report
     end
 
     get "users", to: "reports#users", as: :report_users

spec/routing/reports_routing_spec.rb

@@ -42,6 +42,10 @@
       it "routes to #feature_report with forms_with_csv_submission_enabled and csv format" do
         expect(get: "/reports/features/forms-with-csv-submission-enabled.csv").to route_to("reports#feature_report", report: "forms-with-csv-submission-enabled", format: "csv")
       end
+
+      it "does not route to #feature_report if param does not match defined report" do
+        expect(get: "/reports/features/foobar").not_to route_to("reports#feature_report", report: "foobar")
+      end
     end
 
     describe "path helpers" do

spec/services/reports/feature_report_service_spec.rb

@@ -11,6 +11,32 @@
     GroupForm.create!(form_id: 4, group:)
   end
 
+  describe "Constraint" do
+    it "returns true if given the name of a defined report in slug format" do
+      expect(described_class::Constraint.matches?(OpenStruct.new(params: { report: "forms-with-routes" }))).to be true
+      expect(described_class::Constraint.matches?(OpenStruct.new(params: { report: "questions-with-add-another-answer" }))).to be true
+    end
+
+    it "returns false if names does not match a defined report" do
+      expect(described_class.matches_report?(OpenStruct.new(params: { report: "report" }))).to be false
+      expect(described_class.matches_report?(OpenStruct.new(params: { report: "inspect" }))).to be false
+      expect(described_class.matches_report?(OpenStruct.new(params: { report: "display" }))).to be false
+    end
+  end
+
+  describe ".matches_report?" do
+    it "returns true if given the name of a defined report" do
+      expect(described_class.matches_report?(:forms_with_routes)).to be true
+      expect(described_class.matches_report?(:questions_with_add_another_answer)).to be true
+    end
+
+    it "returns false if names does not match a defined report" do
+      expect(described_class.matches_report?(:report)).to be false
+      expect(described_class.matches_report?(:inspect)).to be false
+      expect(described_class.matches_report?(:display)).to be false
+    end
+  end
+
   describe "#report" do
     it "returns the feature report" do
       report = described_class.new(form_documents).report
@@ -44,6 +70,29 @@
         },
       })
     end
+
+    context "with the name of a feature report" do
+      it "returns that report" do
+        report = []
+        feature_report_service = described_class.new(form_documents)
+        allow(feature_report_service).to receive(:forms_with_routes).and_return report
+
+        expect(feature_report_service.report(:forms_with_routes)).to be report
+
+        expect(feature_report_service).to have_received(:forms_with_routes)
+      end
+    end
+
+    context "with the name of a method that is not a feature report" do
+      it "raises an error" do
+        feature_report_service = described_class.new(form_documents)
+        allow(feature_report_service).to receive(:display)
+
+        expect { feature_report_service.report(:display) }.to raise_error(/not a defined report/)
+
+        expect(feature_report_service).not_to have_received(:display)
+      end
+    end
   end
 
   describe "#questions" do