From 41746459fadfdd134f061d09dbce06b2985165b8 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 3 Mar 2022 12:20:45 +1100 Subject: [PATCH] Restrict allow_charges attribute to read-only We want people to use the UI to change this attribute. --- app/controllers/api/v1/base_controller.rb | 9 +++++ .../concerns/raising_parameters.rb | 23 +++++++++++ app/json_schemas/customer_schema.rb | 7 ++++ config/locales/en.yml | 1 + .../concerns/raising_parameters_spec.rb | 40 +++++++++++++++++++ spec/requests/api/v1/customers_spec.rb | 26 ++++++++++-- swagger/v1/swagger.yaml | 9 ----- 7 files changed, 102 insertions(+), 13 deletions(-) create mode 100644 app/controllers/concerns/raising_parameters.rb create mode 100644 spec/controllers/concerns/raising_parameters_spec.rb diff --git a/app/controllers/api/v1/base_controller.rb b/app/controllers/api/v1/base_controller.rb index aca1c09e55..4402873e69 100644 --- a/app/controllers/api/v1/base_controller.rb +++ b/app/controllers/api/v1/base_controller.rb @@ -7,6 +7,7 @@ module Api include RequestTimeouts include Pagy::Backend include JsonApiPagination + include RaisingParameters check_authorization @@ -18,6 +19,7 @@ module Api rescue_from CanCan::AccessDenied, with: :unauthorized rescue_from ActiveRecord::RecordNotFound, with: :not_found rescue_from Pagy::VariableError, with: :invalid_pagination + rescue_from ActionController::UnpermittedParameters, with: :unpermitted_parameters private @@ -60,6 +62,13 @@ module Api json: json_api_error(exception.message) end + def unpermitted_parameters(error) + message = I18n.t(:unpermitted_parameters, params: error.params.join(", "), scope: :api) + + render status: :unprocessable_entity, + json: json_api_error(message) + end + def invalid_resource!(resource = nil) render status: :unprocessable_entity, json: json_api_invalid( diff --git a/app/controllers/concerns/raising_parameters.rb b/app/controllers/concerns/raising_parameters.rb new file mode 100644 index 0000000000..d3f11c90c2 --- /dev/null +++ b/app/controllers/concerns/raising_parameters.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +# The API uses strict parameter checking. +# +# We want to raise errors when unused or unpermitted parameters are given +# to the API. You then know straight away when a parameter isn't used. +module RaisingParameters + extend ActiveSupport::Concern + + # ActionController manages this config on a per-class basis. The subclass + # enables us to raise errors only here and not in the rest of the app. + class Parameters < ActionController::Parameters + def self.action_on_unpermitted_parameters + :raise + end + end + + # We override the params method so that we always use the strict parameters. + # We could rename this method if we need access to the orginal as well. + def params + Parameters.new(super.to_unsafe_hash) + end +end diff --git a/app/json_schemas/customer_schema.rb b/app/json_schemas/customer_schema.rb index 8da8b67e2b..3281d12d90 100644 --- a/app/json_schemas/customer_schema.rb +++ b/app/json_schemas/customer_schema.rb @@ -21,6 +21,13 @@ class CustomerSchema < JsonApiSchema [:enterprise_id, :email] end + def self.writable_attributes + attributes.except( + :id, + :allow_charges, + ) + end + def self.relationships [:enterprise] end diff --git a/config/locales/en.yml b/config/locales/en.yml index 7bee8133c7..56dc356d3b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1412,6 +1412,7 @@ en: unknown_error: "Something went wrong. Our team has been notified." invalid_api_key: "Invalid API key (%{key}) specified." unauthorized: "You are not authorized to perform that action." + unpermitted_parameters: "Parameters not allowed in this request: %{params}" invalid_resource: "Invalid resource. Please fix errors and try again." resource_not_found: "The resource you were looking for could not be found." enterprise_logo: diff --git a/spec/controllers/concerns/raising_parameters_spec.rb b/spec/controllers/concerns/raising_parameters_spec.rb new file mode 100644 index 0000000000..cf330221b7 --- /dev/null +++ b/spec/controllers/concerns/raising_parameters_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe RaisingParameters do + describe "Parameters" do + let(:params) do + RaisingParameters::Parameters.new( + controller: "example", + action: "update", + data: { + id: "unique", + admin: true, + } + ) + end + + it "raises an error when a parameter is not permitted" do + expect { + params.require(:data).permit(:id) + }.to raise_error( + ActionController::UnpermittedParameters + ) + end + + it "raises no error when all parameters are permitted" do + expect { + params.require(:data).permit(:id, :admin) + }.to_not raise_error + end + + it "doesn't change standard parameter objects" do + original_params = ActionController::Parameters.new(one: 1, two: 2) + + expect { + original_params.permit(:one) + }.to_not raise_error + end + end +end diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index 7c8519a4b0..2167d1c953 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -101,7 +101,7 @@ describe "Customers", type: :request do parameter name: :customer, in: :body, schema: { type: :object, - properties: CustomerSchema.attributes.except(:id), + properties: CustomerSchema.writable_attributes, required: CustomerSchema.required_attributes } @@ -114,7 +114,26 @@ describe "Customers", type: :request do end schema "$ref": "#/components/schemas/resources/customer" - run_test! + run_test! do + expect(json_response[:data][:attributes]).to include( + allow_charges: false + ) + end + end + + response "422", "Unpermitted parameter" do + param(:customer) do + { + email: "test@example.com", + enterprise_id: enterprise1.id.to_s, + allow_charges: true, + } + end + schema "$ref": "#/components/schemas/error_response" + + run_test! do + expect(json_error_detail).to eq "Parameters not allowed in this request: allow_charges" + end end response "422", "Unprocessable entity" do @@ -187,7 +206,7 @@ describe "Customers", type: :request do parameter name: :customer, in: :body, schema: { type: :object, - properties: CustomerSchema.attributes, + properties: CustomerSchema.writable_attributes, required: CustomerSchema.required_attributes } @@ -195,7 +214,6 @@ describe "Customers", type: :request do param(:id) { customer1.id } param(:customer) do { - id: customer1.id.to_s, email: "test@example.com", enterprise_id: enterprise1.id.to_s } diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index aec3ce9759..207673ebf0 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -279,9 +279,6 @@ paths: email: type: string example: alice@example.com - allow_charges: - type: boolean - example: false required: - enterprise_id - email @@ -344,9 +341,6 @@ paths: schema: type: object properties: - id: - type: integer - example: 1 enterprise_id: type: integer example: 2 @@ -365,9 +359,6 @@ paths: email: type: string example: alice@example.com - allow_charges: - type: boolean - example: false required: - enterprise_id - email