From a6163445a3b08922a54c3bef6a55ec15d3ccd169 Mon Sep 17 00:00:00 2001 From: Tim Cowlishaw Date: Sat, 25 Nov 2023 07:00:49 +0100 Subject: [PATCH 1/2] Ensure /users/:id endpoint only lists private devices when the requesting user is authorized to see them --- app/views/v0/users/_user.jbuilder | 4 +- spec/requests/v0/users_spec.rb | 130 +++++++++++++++++++++--------- 2 files changed, 97 insertions(+), 37 deletions(-) diff --git a/app/views/v0/users/_user.jbuilder b/app/views/v0/users/_user.jbuilder index 621144e1..7fda587d 100644 --- a/app/views/v0/users/_user.jbuilder +++ b/app/views/v0/users/_user.jbuilder @@ -21,7 +21,9 @@ else json.merge! legacy_api_key: '[FILTERED]' end -json.devices user.devices do |device| +json.devices user.devices.filter { |d| + !d.is_private? || current_user == user || current_user&.is_admin? +} do |device| json.id device.id json.uuid device.uuid json.is_private device.is_private diff --git a/spec/requests/v0/users_spec.rb b/spec/requests/v0/users_spec.rb index dedf8e37..d7f558ae 100644 --- a/spec/requests/v0/users_spec.rb +++ b/spec/requests/v0/users_spec.rb @@ -42,52 +42,62 @@ expect(j['email']).to eq(user.email) end - describe "smoke tests for ransack" do - it "does not allow searching by first name" do - 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 - json = api_get "users?q[city_eq]=Barcelona" - expect(response.status).to eq(200) + describe "device privacy" do + before do + @private_device = create(:device, owner: user, is_private: true) + @public_device = create(:device, owner: user, is_private: false) 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) + context "when the request is not authenticated" do + it "only returns public devices" do + j = api_get "users/testguy" + expect(j["devices"].map { |d| d["id"] }).to include(@public_device.id) + expect(j["devices"].map { |d| d["id"] }).not_to include(@private_device.id) + end end - it "allows searching by uuid" do - json = api_get "users?q[uuid_eq]=1" - expect(response.status).to eq(200) + context "when the request is authenticated as the user being requested" do + it "returns all devices" do + j = api_get "users/testguy?access_token=#{token.token}" + expect(j["devices"].map { |d| d["id"] }).to include(@public_device.id) + expect(j["devices"].map { |d| d["id"] }).to include(@private_device.id) + end end - it "allows searching by created_at" do - json = api_get "users?q[created_at_eq]=1" - expect(response.status).to eq(200) + context "when the request is authenticated as a different citizen user" do + it "only returns public devices" do + requesting_user = create :user + requesting_token = create :access_token, + application: application, + resource_owner_id: requesting_user.id + j = api_get "users/testguy?access_token=#{requesting_token.token}" + expect(j["devices"].map { |d| d["id"] }).to include(@public_device.id) + expect(j["devices"].map { |d| d["id"] }).not_to include(@private_device.id) + end end - it "allows searching by updated_at" do - json = api_get "users?q[updated_at_eq]=1" - expect(response.status).to eq(200) + context "when the request is authenticated as a different researcher user" do + it "only returns public devices" do + requesting_user = create :user, role_mask: 3 + requesting_token = create :access_token, + application: application, + resource_owner_id: requesting_user.id + j = api_get "users/testguy?access_token=#{requesting_token.token}" + expect(j["devices"].map { |d| d["id"] }).to include(@public_device.id) + expect(j["devices"].map { |d| d["id"] }).not_to include(@private_device.id) + end end - it "does not allow searching on disallowed parameters" do - json = api_get "users?q[disallowed_eq]=1" - expect(response.status).to eq(400) - expect(json["status"]).to eq(400) + context "when the request is authenticated as a different admin user" do + it "returns all devices" do + requesting_user = create :user, role_mask: 5 + requesting_token = create :access_token, + application: application, + resource_owner_id: requesting_user.id + j = api_get "users/testguy?access_token=#{requesting_token.token}" + expect(j["devices"].map { |d| d["id"] }).to include(@public_device.id) + expect(j["devices"].map { |d| d["id"] }).to include(@private_device.id) + end end end end @@ -161,6 +171,54 @@ end end + describe "smoke tests for ransack" do + it "does not allow searching by first name" do + 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 + 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 + json = api_get "users?q[disallowed_eq]=1" + expect(response.status).to eq(400) + expect(json["status"]).to eq(400) + end + end end describe "POST /users" do From af96f84c959fb56236e6878d95a58482f627820b Mon Sep 17 00:00:00 2001 From: Tim Cowlishaw Date: Fri, 1 Dec 2023 09:51:10 +0100 Subject: [PATCH 2/2] Include user tags and location in user device responses --- app/views/v0/devices/_device.jbuilder | 9 +++-- app/views/v0/users/_user.jbuilder | 34 +++++----------- spec/requests/v0/users_spec.rb | 56 ++++++++++++++++++++++++--- 3 files changed, 64 insertions(+), 35 deletions(-) diff --git a/app/views/v0/devices/_device.jbuilder b/app/views/v0/devices/_device.jbuilder index 224ff593..c1a59582 100644 --- a/app/views/v0/devices/_device.jbuilder +++ b/app/views/v0/devices/_device.jbuilder @@ -1,3 +1,6 @@ +with_owner = true unless local_assigns.has_key?(:with_owner) +with_data = true unless local_assigns.has_key?(:with_data) + json.( device, :id, @@ -23,7 +26,7 @@ else json.merge! mac_address: '[FILTERED]' end -if device.owner +if with_owner && device.owner json.owner do json.id device.owner.id json.uuid device.owner.uuid @@ -37,11 +40,9 @@ if device.owner json.location device.owner.location json.device_ids device.owner.cached_device_ids end -else - json.merge! owner: nil end -json.data device.formatted_data +json.data device.formatted_data if with_data if device.kit json.kit device.kit, :id, :uuid, :slug, :name, :description, :created_at, :updated_at diff --git a/app/views/v0/users/_user.jbuilder b/app/views/v0/users/_user.jbuilder index 7fda587d..d3f54d33 100644 --- a/app/views/v0/users/_user.jbuilder +++ b/app/views/v0/users/_user.jbuilder @@ -7,7 +7,6 @@ json.(user, :profile_picture, :url, :location, - :joined_at, :updated_at ) @@ -23,29 +22,14 @@ end json.devices user.devices.filter { |d| !d.is_private? || current_user == user || current_user&.is_admin? -} do |device| - json.id device.id - json.uuid device.uuid - json.is_private device.is_private - - if current_user and (current_user.is_admin? or (device.owner_id and current_user.id == device.owner_id)) - json.mac_address device.mac_address - else - json.mac_address '[FILTERED]' - if device.is_private? - next - end +}.map do |device| + json.partial! "devices/device", device: device, with_data: false, with_owner: false + json.merge!(kit_id: device.kit_id) + if current_user == user || current_user&.is_admin? + json.merge!( + location: device.location, + latitude: device.latitude, + longitude: device.longitude, + ) end - - json.name device.name.present? ? device.name : nil - json.description device.description.present? ? device.description : nil - json.location device.location - json.latitude device.latitude - json.longitude device.longitude - json.kit_id device.kit_id - json.state device.state - json.system_tags device.system_tags - json.last_reading_at device.last_reading_at - json.added_at device.added_at.utc.iso8601 - json.updated_at device.updated_at.utc.iso8601 end diff --git a/spec/requests/v0/users_spec.rb b/spec/requests/v0/users_spec.rb index d7f558ae..65f0f576 100644 --- a/spec/requests/v0/users_spec.rb +++ b/spec/requests/v0/users_spec.rb @@ -54,6 +54,13 @@ expect(j["devices"].map { |d| d["id"] }).to include(@public_device.id) expect(j["devices"].map { |d| d["id"] }).not_to include(@private_device.id) end + + it "does not include the device locations" do + j = api_get "users/testguy" + expect(j["devices"].map { |d| d["location"]}.compact).to be_empty + expect(j["devices"].map { |d| d["latitude"]}.compact).to be_empty + expect(j["devices"].map { |d| d["longitude"]}.compact).to be_empty + end end context "when the request is authenticated as the user being requested" do @@ -62,42 +69,79 @@ expect(j["devices"].map { |d| d["id"] }).to include(@public_device.id) expect(j["devices"].map { |d| d["id"] }).to include(@private_device.id) end + + it "includes the device locations" do + j = api_get "users/testguy?access_token=#{token.token}" + expect(j["devices"].map { |d| d["location"]}.compact).not_to be_empty + expect(j["devices"].map { |d| d["latitude"]}.compact).not_to be_empty + expect(j["devices"].map { |d| d["longitude"]}.compact).not_to be_empty + end end context "when the request is authenticated as a different citizen user" do - it "only returns public devices" do + let(:requesting_token) { requesting_user = create :user - requesting_token = create :access_token, + create :access_token, application: application, resource_owner_id: requesting_user.id + } + + it "only returns public devices" do j = api_get "users/testguy?access_token=#{requesting_token.token}" expect(j["devices"].map { |d| d["id"] }).to include(@public_device.id) expect(j["devices"].map { |d| d["id"] }).not_to include(@private_device.id) end + + it "does not include the device locations" do + j = api_get "users/testguy?access_token=#{requesting_token.token}" + expect(j["devices"].map { |d| d["location"]}.compact).to be_empty + expect(j["devices"].map { |d| d["latitude"]}.compact).to be_empty + expect(j["devices"].map { |d| d["longitude"]}.compact).to be_empty + end end context "when the request is authenticated as a different researcher user" do - it "only returns public devices" do + let(:requesting_token) { requesting_user = create :user, role_mask: 3 - requesting_token = create :access_token, + create :access_token, application: application, resource_owner_id: requesting_user.id + } + + it "only returns public devices" do j = api_get "users/testguy?access_token=#{requesting_token.token}" expect(j["devices"].map { |d| d["id"] }).to include(@public_device.id) expect(j["devices"].map { |d| d["id"] }).not_to include(@private_device.id) end + + it "does not include the device locations" do + j = api_get "users/testguy?access_token=#{requesting_token.token}" + expect(j["devices"].map { |d| d["location"]}.compact).to be_empty + expect(j["devices"].map { |d| d["latitude"]}.compact).to be_empty + expect(j["devices"].map { |d| d["longitude"]}.compact).to be_empty + end end context "when the request is authenticated as a different admin user" do - it "returns all devices" do + let(:requesting_token) { requesting_user = create :user, role_mask: 5 - requesting_token = create :access_token, + create :access_token, application: application, resource_owner_id: requesting_user.id + } + + it "returns all devices" do j = api_get "users/testguy?access_token=#{requesting_token.token}" expect(j["devices"].map { |d| d["id"] }).to include(@public_device.id) expect(j["devices"].map { |d| d["id"] }).to include(@private_device.id) end + + it "includes the device locations" do + j = api_get "users/testguy?access_token=#{requesting_token.token}" + expect(j["devices"].map { |d| d["location"]}.compact).not_to be_empty + expect(j["devices"].map { |d| d["latitude"]}.compact).not_to be_empty + expect(j["devices"].map { |d| d["longitude"]}.compact).not_to be_empty + end end end end