From 4a33aa773ca490aeb820004bb61e09d0cd9c63a9 Mon Sep 17 00:00:00 2001 From: Tim Cowlishaw Date: Tue, 26 Sep 2023 09:36:42 +0200 Subject: [PATCH 1/2] Add tests for advertised ransack parameters and fix regressions Our documentation advertises that users, devices, and sensors are searchable with ransack - this commit adds some tests that assert (in a very basic manner) that this will succeed, and fixes a few cases where we weren't actually supporting the behaviour we advertise. --- app/controllers/v0/application_controller.rb | 8 ++ app/controllers/v0/devices_controller.rb | 9 ++- app/controllers/v0/sensors_controller.rb | 6 +- app/controllers/v0/users_controller.rb | 4 +- app/models/device.rb | 19 +++-- app/models/postprocessing.rb | 4 + app/models/sensor.rb | 8 ++ app/models/tag.rb | 8 ++ app/models/user.rb | 2 +- config/initializers/ransack.rb | 5 ++ .../controllers/v0/devices_controller_spec.rb | 2 + spec/models/device_spec.rb | 6 +- spec/requests/v0/devices_spec.rb | 81 +++++++++++++++++++ spec/requests/v0/sensors_spec.rb | 55 +++++++++++++ spec/requests/v0/users_spec.rb | 48 +++++++++++ 15 files changed, 246 insertions(+), 19 deletions(-) create mode 100644 config/initializers/ransack.rb diff --git a/app/controllers/v0/application_controller.rb b/app/controllers/v0/application_controller.rb index ed823fe3..cd6273c0 100644 --- a/app/controllers/v0/application_controller.rb +++ b/app/controllers/v0/application_controller.rb @@ -29,6 +29,14 @@ def check_missing_params *params_list private + def raise_ransack_errors_as_bad_request(&block) + begin + block.call + rescue ArgumentError => e + raise ActionController::BadRequest.new(e.message) + end + end + def prepend_view_paths # is this still necessary? prepend_view_path "app/views/v0" diff --git a/app/controllers/v0/devices_controller.rb b/app/controllers/v0/devices_controller.rb index e3bb1c5c..02314b60 100644 --- a/app/controllers/v0/devices_controller.rb +++ b/app/controllers/v0/devices_controller.rb @@ -13,10 +13,11 @@ def show end def index - @q = policy_scope(Device) - .includes(:owner, :tags, kit: [:components, :sensors]) - .ransack(params[:q], auth_object: (current_user&.is_admin? ? :admin : nil)) - + raise_ransack_errors_as_bad_request do + @q = policy_scope(Device) + .includes(:owner, :tags, kit: [:components, :sensors]) + .ransack(params[:q], auth_object: (current_user&.is_admin? ? :admin : nil)) + end # We are here customly adding multiple tags into the Ransack query. # Ransack supports this, but how do we add multiple tag names in URL string? Which separator to use? # See Issue #186 https://github.com/fablabbcn/smartcitizen-api/issues/186 diff --git a/app/controllers/v0/sensors_controller.rb b/app/controllers/v0/sensors_controller.rb index 1c2685a6..45be712a 100644 --- a/app/controllers/v0/sensors_controller.rb +++ b/app/controllers/v0/sensors_controller.rb @@ -7,7 +7,11 @@ def show end def index - @sensors = Sensor.includes(:measurement, :tag_sensors).all + raise_ransack_errors_as_bad_request do + @q = Sensor.includes(:measurement, :tag_sensors).ransack(params[:q]) + end + @q.sorts = 'id asc' if @q.sorts.empty? + @sensors = @q.result(distinct: true) @sensors = paginate @sensors end diff --git a/app/controllers/v0/users_controller.rb b/app/controllers/v0/users_controller.rb index 83a729ea..89001084 100644 --- a/app/controllers/v0/users_controller.rb +++ b/app/controllers/v0/users_controller.rb @@ -9,7 +9,9 @@ def show end def index - @q = User.includes(:devices, :profile_picture_attachment).ransack(params[:q]) + raise_ransack_errors_as_bad_request do + @q = User.includes(:devices, :profile_picture_attachment).ransack(params[:q]) + end @q.sorts = 'id asc' if @q.sorts.empty? @users = @q.result(distinct: true) @users = paginate(@users) diff --git a/app/models/device.rb b/app/models/device.rb index 51c4460d..48a484d6 100644 --- a/app/models/device.rb +++ b/app/models/device.rb @@ -73,6 +73,15 @@ class Device < ActiveRecord::Base end end + def self.ransackable_attributes(auth_object = nil) + if auth_object == :admin + # admin can ransack on every attribute + self.authorizable_ransackable_attributes + else + ["id", "name", "description", "created_at", "updated_at", "last_recorded_at", "state","geohash", "uuid", "kit_id"] + end + end + def self.ransackable_associations(auth_object = nil) [ "components", "devices_tags", "kit", "owner", @@ -268,16 +277,6 @@ def remove_mac_address_for_newly_registered_device! update(old_mac_address: mac_address, mac_address: nil) end - def self.ransackable_attributes(auth_object = nil) - if auth_object == :admin - # admin can ransack on every attribute - super - else - # normal users can NOT ransack on device_token - column_names - ['device_token'] + _ransackers.keys - end - end - private def set_state diff --git a/app/models/postprocessing.rb b/app/models/postprocessing.rb index d42f1cee..aeda264d 100644 --- a/app/models/postprocessing.rb +++ b/app/models/postprocessing.rb @@ -4,4 +4,8 @@ class Postprocessing < ApplicationRecord def self.ransackable_attributes(auth_object = nil) ["blueprint_url", "created_at", "device_id", "forwarding_params", "hardware_url", "id", "latest_postprocessing", "meta", "updated_at"] end + + def self_ransackable_associations(auth_object = nil) + [] + end end diff --git a/app/models/sensor.rb b/app/models/sensor.rb index e9c1a595..10192832 100644 --- a/app/models/sensor.rb +++ b/app/models/sensor.rb @@ -18,6 +18,14 @@ class Sensor < ActiveRecord::Base has_ancestry validates_presence_of :name, :description#, :unit + def self.ransackable_attributes(auth_object = nil) + ["ancestry", "created_at", "description", "id", "measurement_id", "name", "unit", "updated_at", "uuid"] + end + + def self.ransackable_associations(auth_object = nil) + [] + end + def tags tag_sensors.map(&:name) end diff --git a/app/models/tag.rb b/app/models/tag.rb index 8003255e..7eb25f98 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -10,4 +10,12 @@ class Tag < ActiveRecord::Base extend FriendlyId friendly_id :name + + def self.ransackable_attributes(auth_object = nil) + ["created_at", "description", "id", "name", "updated_at", "uuid"] + end + + def ransackable_associations(auth_object = nil) + [] + end end diff --git a/app/models/user.rb b/app/models/user.rb index 8ddc2b38..1954ad79 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -42,7 +42,7 @@ class User < ActiveRecord::Base alias_attribute :joined_at, :created_at def self.ransackable_attributes(auth_object = nil) - [ "city", "country_code", "email", "id", "url", "username", "uuid", "created_at", "joined_at", "updated_at"] + [ "city", "country_code", "id", "username", "uuid", "created_at", "updated_at"] end def self.ransackable_associations(auth_object = nil) diff --git a/config/initializers/ransack.rb b/config/initializers/ransack.rb new file mode 100644 index 00000000..fed7e86e --- /dev/null +++ b/config/initializers/ransack.rb @@ -0,0 +1,5 @@ +Ransack.configure do |config| + # Raise errors if a query contains an unknown predicate or attribute. + # Default is true (do not raise error on unknown conditions). + config.ignore_unknown_conditions = false + end \ No newline at end of file diff --git a/spec/controllers/v0/devices_controller_spec.rb b/spec/controllers/v0/devices_controller_spec.rb index cf5af83e..f2613fdd 100644 --- a/spec/controllers/v0/devices_controller_spec.rb +++ b/spec/controllers/v0/devices_controller_spec.rb @@ -2,4 +2,6 @@ RSpec.describe V0::DevicesController do skip { is_expected.to permit(:name,:description,:mac_address,:latitude,:longitude,:elevation,:exposure,:meta,:kit_id,:user_tags).for(:create) } + + end diff --git a/spec/models/device_spec.rb b/spec/models/device_spec.rb index e7b59529..d0988b13 100644 --- a/spec/models/device_spec.rb +++ b/spec/models/device_spec.rb @@ -298,8 +298,10 @@ expect(Device.count).to eq(3) # Admins can ransack on device_token_contains expect(Device.ransack({device_token_cont: '123'}, auth_object: :admin).result.count).to eq(1) - # Normal users get all deviecs returned. They cannot ransack on device_token - expect(Device.ransack({device_token_cont: '123'}).result.count).to eq(3) + # Normal users cannot ransack on device_token + expect { + Device.ransack({device_token_cont: '123'}) + }.to raise_error(ArgumentError) end it 'does not validate uniqueness when nil' do diff --git a/spec/requests/v0/devices_spec.rb b/spec/requests/v0/devices_spec.rb index 230f1278..7617575a 100644 --- a/spec/requests/v0/devices_spec.rb +++ b/spec/requests/v0/devices_spec.rb @@ -114,11 +114,92 @@ expect(response.status).to eq(400) end + it "allows searching by id" do + json = api_get "devices?q[id_eq]=1" + expect(response.status).to eq(200) + end + + it "allows searching by name" do + json = api_get "devices?q[name_eq]=Name" + expect(response.status).to eq(200) + end + + it "allows searching by description" do + json = api_get "devices?q[description_eq]=Desc" + expect(response.status).to eq(200) + end + + it "allows searching by created_at" do + json = api_get "devices?q[created_at_lt]=2023-09-26" + expect(response.status).to eq(200) + end + + it "allows searching by updated_at" do + json = api_get "devices?q[updated_at_lt]=2023-09-26" + expect(response.status).to eq(200) + end + + it "allows searching by last_recorded_at" do + json = api_get "devices?q[last_recorded_at_lt]=2023-09-26" + expect(response.status).to eq(200) + end + + it "allows searching by state" do + json = api_get "devices?q[state_eq]=state" + expect(response.status).to eq(200) + end + + it "allows searching by geohash" do + json = api_get "devices?q[geohash_eq]=geohash" + expect(response.status).to eq(200) + end + + it "allows searching by uuid" do + json = api_get "devices?q[uuid_eq]=uuid" + expect(response.status).to eq(200) + end + + it "allows searching by owner id" do + json = api_get "devices?q[owner_id_eq]=1" + expect(response.status).to eq(200) + end + + it "allows searching by owner username" do + json = api_get "devices?q[owner_username_eq]=test" + expect(response.status).to eq(200) + end + + it "allows searching by tag name" do + json = api_get "devices?q[tags_name_eq]=test" + expect(response.status).to eq(200) + end + it "allows searching by presence of postprocessing" do json = api_get "devices?q[postprocessing_id_not_null]=1" expect(response.status).to eq(200) end + it "allows searching by postprocessing id" do + json = api_get "devices?q[postprocessing_id_eq]=1" + expect(response.status).to eq(200) + end + + it "allows searching by mac address by admins" do + json = api_get "devices?q[mac_address_eq]=00:00:00:00:00:00&access_token=#{admin_token.token}" + expect(response.status).to eq(200) + end + + it "does not allow searching by mac address by non-admins" do + expect { + api_get "devices?q[mac_address_eq]=00:00:00:00:00:00" + }.to raise_error(ActionController::BadRequest) + end + + it "does not allow searching on disallowed parameters" do + expect { + api_get "devices?q[disallowed_eq]=1" + }.to raise_error(ActionController::BadRequest) + end end end diff --git a/spec/requests/v0/sensors_spec.rb b/spec/requests/v0/sensors_spec.rb index 47c69f73..a136f734 100644 --- a/spec/requests/v0/sensors_spec.rb +++ b/spec/requests/v0/sensors_spec.rb @@ -26,6 +26,61 @@ expect(j[0]['name']).to eq('testing sensor') expect(response.status).to eq(200) end + + describe "smoke tests for ransack" do + + it "allows searching by ancestry" do + json = api_get "sensors?q[ancestry_eq]=1" + expect(response.status).to eq(200) + end + + it "allows searching by created_at" do + json = api_get "sensors?q[created_at_eq]=1" + expect(response.status).to eq(200) + end + + it "allows searching by description" do + json = api_get "sensors?q[description_eq]=1" + expect(response.status).to eq(200) + end + + it "allows searching by id" do + json = api_get "sensors?q[id_lt]=100" + expect(response.status).to eq(200) + end + + it "allows searching by measurement_id" do + json = api_get "sensors?q[measurement_id_eq]=1" + expect(response.status).to eq(200) + end + + it "allows searching by name" do + json = api_get "sensors?q[name_eq]=name" + expect(response.status).to eq(200) + end + + it "allows searching by unit" do + json = api_get "sensors?q[unit_eq]=ppm" + expect(response.status).to eq(200) + end + + it "allows searching by updated_at" do + json = api_get "sensors?q[updated_at_eq]=1" + expect(response.status).to eq(200) + end + + it "allows searching by uuid" do + json = api_get "sensors?q[uuid_eq]=1" + expect(response.status).to eq(200) + end + + it "does not allow searching on disallowed parameters" do + expect { + api_get "sensors?q[disallowed_eq]=1" + }.to raise_error(ActionController::BadRequest) + end + + end end describe "POST /sensors" do diff --git a/spec/requests/v0/users_spec.rb b/spec/requests/v0/users_spec.rb index 5c64110a..2cbb1e66 100644 --- a/spec/requests/v0/users_spec.rb +++ b/spec/requests/v0/users_spec.rb @@ -42,6 +42,54 @@ expect(j['email']).to eq(user.email) end + describe "smoke tests for ransack" do + it "does not allow searching by first name" do + expect { + api_get "users?q[first_name_eq]=Tim" + }.to raise_error(ActionController::BadRequest) + end + + it "allows searching by city" do + json = api_get "users?q[city_eq]=Barcelona" + expect(response.status).to eq(200) + end + + it "allows searching by country code" do + json = api_get "users?q[country_code_eq]=es" + expect(response.status).to eq(200) + end + + it "allows searching by id" do + json = api_get "users?q[id_eq]=1" + expect(response.status).to eq(200) + end + + it "allows searching by username" do + json = api_get "users?q[username_eq]=mistertim" + expect(response.status).to eq(200) + end + + it "allows searching by uuid" do + json = api_get "users?q[uuid_eq]=1" + expect(response.status).to eq(200) + end + + it "allows searching by created_at" do + json = api_get "users?q[created_at_eq]=1" + expect(response.status).to eq(200) + end + + it "allows searching by updated_at" do + json = api_get "users?q[updated_at_eq]=1" + expect(response.status).to eq(200) + end + + it "does not allow searching on disallowed parameters" do + expect { + api_get "users?q[disallowed_eq]=1" + }.to raise_error(ActionController::BadRequest) + end + end end # index From 800e6d50b8ed1ce6846fe7cb9914575e245ef19d Mon Sep 17 00:00:00 2001 From: Tim Cowlishaw Date: Thu, 26 Oct 2023 06:55:15 +0200 Subject: [PATCH 2/2] fix error responses so json returned in production --- app/controllers/v0/application_controller.rb | 2 +- app/controllers/v0/devices_controller.rb | 43 ++++++++++---------- app/controllers/v0/sensors_controller.rb | 6 +-- app/controllers/v0/users_controller.rb | 6 +-- spec/requests/v0/devices_spec.rb | 12 +++--- spec/requests/v0/sensors_spec.rb | 6 +-- spec/requests/v0/users_spec.rb | 12 +++--- 7 files changed, 43 insertions(+), 44 deletions(-) diff --git a/app/controllers/v0/application_controller.rb b/app/controllers/v0/application_controller.rb index cd6273c0..b1af5d0c 100644 --- a/app/controllers/v0/application_controller.rb +++ b/app/controllers/v0/application_controller.rb @@ -33,7 +33,7 @@ def raise_ransack_errors_as_bad_request(&block) begin block.call rescue ArgumentError => e - raise ActionController::BadRequest.new(e.message) + render json: { message: e.message, status: 400 }, status: 400 end end diff --git a/app/controllers/v0/devices_controller.rb b/app/controllers/v0/devices_controller.rb index 02314b60..07affcc4 100644 --- a/app/controllers/v0/devices_controller.rb +++ b/app/controllers/v0/devices_controller.rb @@ -17,31 +17,30 @@ def index @q = policy_scope(Device) .includes(:owner, :tags, kit: [:components, :sensors]) .ransack(params[:q], auth_object: (current_user&.is_admin? ? :admin : nil)) - end - # We are here customly adding multiple tags into the Ransack query. - # Ransack supports this, but how do we add multiple tag names in URL string? Which separator to use? - # See Issue #186 https://github.com/fablabbcn/smartcitizen-api/issues/186 - # If we figure it out, we can remove the next 3 lines, but remember to document in: - # https://developer.smartcitizen.me/#basic-searching - if params[:with_tags] - @q.tags_name_in = params[:with_tags].split('|') - end + # We are here customly adding multiple tags into the Ransack query. + # Ransack supports this, but how do we add multiple tag names in URL string? Which separator to use? + # See Issue #186 https://github.com/fablabbcn/smartcitizen-api/issues/186 + # If we figure it out, we can remove the next 3 lines, but remember to document in: + # https://developer.smartcitizen.me/#basic-searching + if params[:with_tags] + @q.tags_name_in = params[:with_tags].split('|') + end - @devices = @q.result(distinct: true) - - if params[:near] - if params[:near] =~ /\A(\-?\d+(\.\d+)?),\s*(\-?\d+(\.\d+)?)\z/ - @devices = @devices.near( - params[:near].split(','), (params[:within] || 1000)) - else - return render json: { id: "bad_request", - message: "Malformed near parameter", - url: 'https://developer.smartcitizen.me/#get-all-devices', - errors: nil }, status: :bad_request + @devices = @q.result(distinct: true) + + if params[:near] + if params[:near] =~ /\A(\-?\d+(\.\d+)?),\s*(\-?\d+(\.\d+)?)\z/ + @devices = @devices.near( + params[:near].split(','), (params[:within] || 1000)) + else + return render json: { id: "bad_request", + message: "Malformed near parameter", + url: 'https://developer.smartcitizen.me/#get-all-devices', + errors: nil }, status: :bad_request + end end + @devices = paginate(@devices) end - - @devices = paginate(@devices) end def update diff --git a/app/controllers/v0/sensors_controller.rb b/app/controllers/v0/sensors_controller.rb index 45be712a..cdfce631 100644 --- a/app/controllers/v0/sensors_controller.rb +++ b/app/controllers/v0/sensors_controller.rb @@ -9,10 +9,10 @@ def show def index raise_ransack_errors_as_bad_request do @q = Sensor.includes(:measurement, :tag_sensors).ransack(params[:q]) + @q.sorts = 'id asc' if @q.sorts.empty? + @sensors = @q.result(distinct: true) + @sensors = paginate @sensors end - @q.sorts = 'id asc' if @q.sorts.empty? - @sensors = @q.result(distinct: true) - @sensors = paginate @sensors end def create diff --git a/app/controllers/v0/users_controller.rb b/app/controllers/v0/users_controller.rb index 89001084..1ec0f11c 100644 --- a/app/controllers/v0/users_controller.rb +++ b/app/controllers/v0/users_controller.rb @@ -11,10 +11,10 @@ def show def index raise_ransack_errors_as_bad_request do @q = User.includes(:devices, :profile_picture_attachment).ransack(params[:q]) + @q.sorts = 'id asc' if @q.sorts.empty? + @users = @q.result(distinct: true) + @users = paginate(@users) end - @q.sorts = 'id asc' if @q.sorts.empty? - @users = @q.result(distinct: true) - @users = paginate(@users) end def create diff --git a/spec/requests/v0/devices_spec.rb b/spec/requests/v0/devices_spec.rb index 7617575a..aca25eba 100644 --- a/spec/requests/v0/devices_spec.rb +++ b/spec/requests/v0/devices_spec.rb @@ -190,15 +190,15 @@ end it "does not allow searching by mac address by non-admins" do - expect { - api_get "devices?q[mac_address_eq]=00:00:00:00:00:00" - }.to raise_error(ActionController::BadRequest) + json = api_get "devices?q[mac_address_eq]=00:00:00:00:00:00" + expect(response.status).to eq(400) + expect(json["status"]).to eq(400) end it "does not allow searching on disallowed parameters" do - expect { - api_get "devices?q[disallowed_eq]=1" - }.to raise_error(ActionController::BadRequest) + json = api_get "devices?q[disallowed_eq]=1" + expect(response.status).to eq(400) + expect(json["status"]).to eq(400) end end end diff --git a/spec/requests/v0/sensors_spec.rb b/spec/requests/v0/sensors_spec.rb index a136f734..b86811c0 100644 --- a/spec/requests/v0/sensors_spec.rb +++ b/spec/requests/v0/sensors_spec.rb @@ -75,9 +75,9 @@ end it "does not allow searching on disallowed parameters" do - expect { - api_get "sensors?q[disallowed_eq]=1" - }.to raise_error(ActionController::BadRequest) + json = api_get "sensors?q[disallowed_eq]=1" + expect(response.status).to eq(400) + expect(json["status"]).to eq(400) end end diff --git a/spec/requests/v0/users_spec.rb b/spec/requests/v0/users_spec.rb index 2cbb1e66..dedf8e37 100644 --- a/spec/requests/v0/users_spec.rb +++ b/spec/requests/v0/users_spec.rb @@ -44,9 +44,9 @@ describe "smoke tests for ransack" do it "does not allow searching by first name" do - expect { - api_get "users?q[first_name_eq]=Tim" - }.to raise_error(ActionController::BadRequest) + json = api_get "users?q[first_name_eq]=Tim" + expect(response.status).to eq(400) + expect(json["status"]).to eq(400) end it "allows searching by city" do @@ -85,9 +85,9 @@ end it "does not allow searching on disallowed parameters" do - expect { - api_get "users?q[disallowed_eq]=1" - }.to raise_error(ActionController::BadRequest) + json = api_get "users?q[disallowed_eq]=1" + expect(response.status).to eq(400) + expect(json["status"]).to eq(400) end end end