Merge pull request #285 from fablabbcn/bugfix/only-show-private-devic…

…es-to-authorized-users

Ensure /users/:id endpoint only lists private devices when the requesting user is authorized to see them

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

app/views/v0/users/_user.jbuilder

@@ -7,7 +7,6 @@ json.(user,
   :profile_picture,
   :url,
   :location,
-  :joined_at,
   :updated_at
 )
 
@@ -21,29 +20,16 @@ else
   json.merge! legacy_api_key: '[FILTERED]'
 end
 
-json.devices user.devices 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
+json.devices user.devices.filter { |d|
+  !d.is_private? || current_user == user || current_user&.is_admin?
+}.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

spec/requests/v0/users_spec.rb

@@ -42,52 +42,106 @@
       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)
+    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 city" do
-        json = api_get "users?q[city_eq]=Barcelona"
-        expect(response.status).to eq(200)
-      end
+      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
 
-      it "allows searching by country code" do
-        json = api_get "users?q[country_code_eq]=es"
-        expect(response.status).to eq(200)
+        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
 
-      it "allows searching by id" do
-        json = api_get "users?q[id_eq]=1"
-        expect(response.status).to eq(200)
-      end
+      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
 
-      it "allows searching by username" do
-        json = api_get "users?q[username_eq]=mistertim"
-        expect(response.status).to eq(200)
+        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
 
-      it "allows searching by uuid" do
-        json = api_get "users?q[uuid_eq]=1"
-        expect(response.status).to eq(200)
-      end
+      context "when the request is authenticated as a different citizen user" do
+        let(:requesting_token) {
+          requesting_user = create :user
+          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 "allows searching by created_at" do
-        json = api_get "users?q[created_at_eq]=1"
-        expect(response.status).to eq(200)
+        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
 
-      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
+        let(:requesting_token) {
+          requesting_user = create :user, role_mask: 3
+          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
 
-      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
+        let(:requesting_token) {
+          requesting_user = create :user, role_mask: 5
+          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
@@ -161,6 +215,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