From 4c6f48737861121479a7d44f62af42b859f7ef32 Mon Sep 17 00:00:00 2001 From: Viktor Smari Date: Sat, 17 Aug 2019 16:36:10 +0200 Subject: [PATCH 01/13] WIP: role_mask 3 is now a 'superuser' --- app/models/user.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/models/user.rb b/app/models/user.rb index 4bde34de..559bf0b0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -97,7 +97,12 @@ def is_admin? role == 'admin' end + def is_superuser? + role == 'superuser' + end + def role + role_mask == 3 ? 'superuser' : nil role_mask < 5 ? 'citizen' : 'admin' end From 53cf63edc36777966012d4928f4a8a742d9a2a6d Mon Sep 17 00:00:00 2001 From: Viktor Smari Date: Mon, 19 Aug 2019 12:10:40 +0200 Subject: [PATCH 02/13] Private devices only visible to owners & admins --- app/policies/device_policy.rb | 6 +++++- db/migrate/20190819084816_add_is_private_to_devices.rb | 5 +++++ db/schema.rb | 3 ++- 3 files changed, 12 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20190819084816_add_is_private_to_devices.rb diff --git a/app/policies/device_policy.rb b/app/policies/device_policy.rb index 642affc4..b4e0f687 100644 --- a/app/policies/device_policy.rb +++ b/app/policies/device_policy.rb @@ -1,7 +1,11 @@ class DevicePolicy < ApplicationPolicy def show? - true + if record.is_private? + update? + else + true + end end def update? diff --git a/db/migrate/20190819084816_add_is_private_to_devices.rb b/db/migrate/20190819084816_add_is_private_to_devices.rb new file mode 100644 index 00000000..09663943 --- /dev/null +++ b/db/migrate/20190819084816_add_is_private_to_devices.rb @@ -0,0 +1,5 @@ +class AddIsPrivateToDevices < ActiveRecord::Migration[5.2] + def change + add_column :devices, :is_private, :boolean, default: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 61b95735..2bcad7bb 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2019_02_22_130041) do +ActiveRecord::Schema.define(version: 2019_08_19_084816) do # These are extensions that must be enabled in order to support this database enable_extension "adminpack" @@ -93,6 +93,7 @@ t.datetime "notify_low_battery_timestamp", default: "2019-01-16 16:19:35" t.boolean "notify_low_battery", default: false t.boolean "notify_stopped_publishing", default: false + t.boolean "is_private", default: false t.index ["device_token"], name: "index_devices_on_device_token", unique: true t.index ["geohash"], name: "index_devices_on_geohash" t.index ["kit_id"], name: "index_devices_on_kit_id" From 9b23d77523b39a0722faf081941fa7875c52bc5f Mon Sep 17 00:00:00 2001 From: Viktor Smari Date: Mon, 19 Aug 2019 16:38:32 +0200 Subject: [PATCH 03/13] Use policy_scope for Device + tests Add is_private to /world_map and /device --- app/controllers/v0/devices_controller.rb | 7 ++- app/policies/device_policy.rb | 23 +++++++++ app/views/v0/devices/_device.jbuilder | 1 + spec/requests/v0/devices_spec.rb | 61 +++++++++++++++++++++++- 4 files changed, 89 insertions(+), 3 deletions(-) diff --git a/app/controllers/v0/devices_controller.rb b/app/controllers/v0/devices_controller.rb index ff896ee6..4fe32eb0 100644 --- a/app/controllers/v0/devices_controller.rb +++ b/app/controllers/v0/devices_controller.rb @@ -14,11 +14,13 @@ def show def index if params[:with_tags] - @q = Device.with_user_tags(params[:with_tags]) + @q = policy_scope(Device) + .with_user_tags(params[:with_tags]) .includes(:owner,:tags, kit: [:sensors, :components]) .ransack(params[:q]) else - @q = Device.includes(:owner, :tags, kit: [:components, :sensors]) + @q = policy_scope(Device) + .includes(:owner, :tags, kit: [:components, :sensors]) .ransack(params[:q]) end @@ -82,6 +84,7 @@ def fresh_world_map longitude: device.longitude, city: device.city, country_code: device.country_code, + is_private: device.is_private, kit_id: device.kit_id, state: device.state, system_tags: device.system_tags, diff --git a/app/policies/device_policy.rb b/app/policies/device_policy.rb index b4e0f687..291f2d7d 100644 --- a/app/policies/device_policy.rb +++ b/app/policies/device_policy.rb @@ -1,4 +1,27 @@ class DevicePolicy < ApplicationPolicy + class Scope + attr_reader :user, :scope + + def initialize(user, scope) + @user = user + @scope = scope + end + + def resolve + if user + if user.is_admin? + # Admins get everything + scope + else + # Non admins should get all non_private + the Devices they own + scope.where(is_private: false).or(scope.where(owner_id: user.id)) + end + else + # not logged in + scope.where(is_private: false) + end + end + end def show? if record.is_private? diff --git a/app/views/v0/devices/_device.jbuilder b/app/views/v0/devices/_device.jbuilder index 6a653c6a..d381127d 100644 --- a/app/views/v0/devices/_device.jbuilder +++ b/app/views/v0/devices/_device.jbuilder @@ -7,6 +7,7 @@ json.(device, :hardware_info, :system_tags, :user_tags, + :is_private, :notify_low_battery, :notify_stopped_publishing, :last_reading_at, diff --git a/spec/requests/v0/devices_spec.rb b/spec/requests/v0/devices_spec.rb index 99165f23..a13c94cf 100644 --- a/spec/requests/v0/devices_spec.rb +++ b/spec/requests/v0/devices_spec.rb @@ -4,6 +4,8 @@ let(:application) { create :application } let(:user) { create :user } + let(:user2) { create :user } + let(:admin) { create :user, is_admin: true } let(:token) { create :access_token, application: application, resource_owner_id: user.id } let(:device) { create(:device) } let(:admin) { create :admin } @@ -24,7 +26,7 @@ # expect(json[0]['name']).to eq(first.name) # expect(json[1]['name']).to eq(second.name) expect(json[0].keys).to eq(%w(id uuid name description state - hardware_info system_tags user_tags notify_low_battery notify_stopped_publishing last_reading_at added_at updated_at mac_address owner data kit)) + hardware_info system_tags user_tags is_private notify_low_battery notify_stopped_publishing last_reading_at added_at updated_at mac_address owner data kit)) end describe "world map" do @@ -87,6 +89,21 @@ expect(response.status).to eq(404) end + it 'does not show a private device' do + device = create(:device, owner: user, is_private: true) + j = api_get "devices/#{device.id}" + expect(j['id']).to eq("forbidden") + expect(response.status).to eq(403) + end + + it 'shows a non_private device' do + device = create(:device, owner: user, is_private: false) + j = api_get "devices/#{device.id}" + expect(j['id']).to eq(device.id) + expect(response.status).to eq(200) + end + + describe "mac_address" do it "filters mac address from guests" do @@ -140,6 +157,48 @@ end + describe "GET /devices" do + it 'does not show private devices' do + device = create(:device, owner: user, is_private: false) + device1 = create(:device, owner: user, is_private: true) + device2 = create(:device, owner: user, is_private: true) + + expect(Device.count).to eq(3) + j = api_get "devices/" + expect(j.count).to eq(1) + expect(response.status).to eq(200) + expect(j[0]['id']).to eq(device.id) + end + + skip 'logged in user can see his devices, even though they are private' do + # TODO: how to use current_user? + # if current_user is 'user' then he would only see 2 devices, because + # device2 is owned by user2 + device1 = create(:device, owner: user, is_private: false) + device2 = create(:device, owner: user, is_private: true) + device3 = create(:device, owner: user2, is_private: true) + + expect(Device.count).to eq(3) + j = api_get "devices/" + expect(j[0]['id']).to eq(device1.id) + expect(response.status).to eq(200) + expect(j.count).to eq(2) + end + + skip 'admin can see ALL devices' do + # TODO: if admin was current_user / logged in, he should see 3 devices + device1 = create(:device, owner: user, is_private: false) + device2 = create(:device, owner: user, is_private: true) + device3 = create(:device, owner: user2, is_private: true) + + expect(Device.count).to eq(3) + j = api_get "devices/" + expect(response.status).to eq(200) + expect(j[0]['id']).to eq(device1.id) + expect(j.count).to eq(3) + end + end + describe "POST /devices" do it "creates a device" do From a9da0346510e5981401aa7f71c15f5e790c9508f Mon Sep 17 00:00:00 2001 From: Viktor Smari Date: Mon, 19 Aug 2019 16:54:26 +0200 Subject: [PATCH 04/13] Test: admin can see all devices --- spec/requests/v0/devices_spec.rb | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/spec/requests/v0/devices_spec.rb b/spec/requests/v0/devices_spec.rb index a13c94cf..96f2f3ad 100644 --- a/spec/requests/v0/devices_spec.rb +++ b/spec/requests/v0/devices_spec.rb @@ -5,7 +5,6 @@ let(:application) { create :application } let(:user) { create :user } let(:user2) { create :user } - let(:admin) { create :user, is_admin: true } let(:token) { create :access_token, application: application, resource_owner_id: user.id } let(:device) { create(:device) } let(:admin) { create :admin } @@ -170,29 +169,27 @@ expect(j[0]['id']).to eq(device.id) end - skip 'logged in user can see his devices, even though they are private' do - # TODO: how to use current_user? - # if current_user is 'user' then he would only see 2 devices, because - # device2 is owned by user2 + it 'logged in user can see his devices, even though they are private' do device1 = create(:device, owner: user, is_private: false) device2 = create(:device, owner: user, is_private: true) device3 = create(:device, owner: user2, is_private: true) expect(Device.count).to eq(3) - j = api_get "devices/" + j = api_get "devices/", { access_token: token.token } expect(j[0]['id']).to eq(device1.id) expect(response.status).to eq(200) expect(j.count).to eq(2) end - skip 'admin can see ALL devices' do - # TODO: if admin was current_user / logged in, he should see 3 devices + it 'admin can see ALL devices' do device1 = create(:device, owner: user, is_private: false) device2 = create(:device, owner: user, is_private: true) device3 = create(:device, owner: user2, is_private: true) expect(Device.count).to eq(3) - j = api_get "devices/" + + j = api_get "devices/", {access_token: admin_token.token} + expect(response.status).to eq(200) expect(j[0]['id']).to eq(device1.id) expect(j.count).to eq(3) From d2c30fd1bf23b863a13efb6a9890ca348a0a0ad1 Mon Sep 17 00:00:00 2001 From: Viktor Smari Date: Tue, 20 Aug 2019 15:26:10 +0200 Subject: [PATCH 05/13] /metrics: Add private device counter --- app/controllers/v0/static_controller.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/controllers/v0/static_controller.rb b/app/controllers/v0/static_controller.rb index 586f24fd..52004655 100644 --- a/app/controllers/v0/static_controller.rb +++ b/app/controllers/v0/static_controller.rb @@ -29,6 +29,7 @@ def metrics render json: { devices: { total: Device.count, + private: Device.where(is_private: true).count, online: { now: Device.where('last_recorded_at > ?', 10.minutes.ago).count, last_hour: Device.where('last_recorded_at > ?', 1.hour.ago).count, From f82520d6d1c60a70410a82c0e45aeb5311af9c49 Mon Sep 17 00:00:00 2001 From: Viktor Smari Date: Wed, 21 Aug 2019 09:05:29 +0200 Subject: [PATCH 06/13] Sort tests --- spec/requests/v0/devices_spec.rb | 83 ++++++++++++++++---------------- 1 file changed, 42 insertions(+), 41 deletions(-) diff --git a/spec/requests/v0/devices_spec.rb b/spec/requests/v0/devices_spec.rb index 96f2f3ad..fefd9edc 100644 --- a/spec/requests/v0/devices_spec.rb +++ b/spec/requests/v0/devices_spec.rb @@ -28,6 +28,48 @@ hardware_info system_tags user_tags is_private notify_low_battery notify_stopped_publishing last_reading_at added_at updated_at mac_address owner data kit)) end + describe "when not logged in" do + it 'does not show private devices' do + device = create(:device, owner: user, is_private: false) + device1 = create(:device, owner: user, is_private: true) + device2 = create(:device, owner: user, is_private: true) + + expect(Device.count).to eq(3) + j = api_get "devices/" + expect(j.count).to eq(1) + expect(response.status).to eq(200) + expect(j[0]['id']).to eq(device.id) + end + end + + describe "when logged in as a normal user" do + it 'shows the user his devices, even though they are private' do + device1 = create(:device, owner: user, is_private: false) + device2 = create(:device, owner: user, is_private: true) + device3 = create(:device, owner: user2, is_private: true) + + expect(Device.count).to eq(3) + j = api_get "devices/", { access_token: token.token } + expect(j[0]['id']).to eq(device1.id) + expect(response.status).to eq(200) + expect(j.count).to eq(2) + end + end + + describe "when logged in as an admin" do + it 'allows admin to see ALL devices' do + device1 = create(:device, owner: user, is_private: false) + device2 = create(:device, owner: user, is_private: true) + device3 = create(:device, owner: user2, is_private: true) + + expect(Device.count).to eq(3) + j = api_get "devices/", {access_token: admin_token.token} + expect(response.status).to eq(200) + expect(j[0]['id']).to eq(device1.id) + expect(j.count).to eq(3) + end + end + describe "world map" do it "returns all devices" do first = create(:device, data: { "": Time.now }) @@ -156,48 +198,7 @@ end - describe "GET /devices" do - it 'does not show private devices' do - device = create(:device, owner: user, is_private: false) - device1 = create(:device, owner: user, is_private: true) - device2 = create(:device, owner: user, is_private: true) - - expect(Device.count).to eq(3) - j = api_get "devices/" - expect(j.count).to eq(1) - expect(response.status).to eq(200) - expect(j[0]['id']).to eq(device.id) - end - - it 'logged in user can see his devices, even though they are private' do - device1 = create(:device, owner: user, is_private: false) - device2 = create(:device, owner: user, is_private: true) - device3 = create(:device, owner: user2, is_private: true) - - expect(Device.count).to eq(3) - j = api_get "devices/", { access_token: token.token } - expect(j[0]['id']).to eq(device1.id) - expect(response.status).to eq(200) - expect(j.count).to eq(2) - end - - it 'admin can see ALL devices' do - device1 = create(:device, owner: user, is_private: false) - device2 = create(:device, owner: user, is_private: true) - device3 = create(:device, owner: user2, is_private: true) - - expect(Device.count).to eq(3) - - j = api_get "devices/", {access_token: admin_token.token} - - expect(response.status).to eq(200) - expect(j[0]['id']).to eq(device1.id) - expect(j.count).to eq(3) - end - end - describe "POST /devices" do - it "creates a device" do api_post 'devices', { access_token: token.token, From 9481656cd2a592f1a27272d8d5d85d94cf9e0da9 Mon Sep 17 00:00:00 2001 From: Viktor Smari Date: Fri, 23 Aug 2019 13:13:25 +0200 Subject: [PATCH 07/13] Allow is_private param --- app/controllers/v0/devices_controller.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/controllers/v0/devices_controller.rb b/app/controllers/v0/devices_controller.rb index 4fe32eb0..d5dd8b05 100644 --- a/app/controllers/v0/devices_controller.rb +++ b/app/controllers/v0/devices_controller.rb @@ -135,6 +135,7 @@ def device_params :longitude, :elevation, :device_token, + :is_private, :notify_low_battery, :notify_stopped_publishing, :exposure, From 0b8d1394e7d06a591ee8083c6077f0650832b2ba Mon Sep 17 00:00:00 2001 From: Viktor Smari Date: Mon, 26 Aug 2019 11:35:55 +0200 Subject: [PATCH 08/13] Only superusers+ can update is_private attribute --- app/controllers/v0/devices_controller.rb | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/app/controllers/v0/devices_controller.rb b/app/controllers/v0/devices_controller.rb index d5dd8b05..58c18d1f 100644 --- a/app/controllers/v0/devices_controller.rb +++ b/app/controllers/v0/devices_controller.rb @@ -127,7 +127,7 @@ def world_map private def device_params - params.permit( + params_to_permit = [ :name, :description, :mac_address, @@ -135,13 +135,21 @@ def device_params :longitude, :elevation, :device_token, - :is_private, :notify_low_battery, :notify_stopped_publishing, :exposure, :meta, :kit_id, :user_tags + ] + + # Superusers + Admins can update is_private + if current_user.role_mask >= 3 + params_to_permit.push(:is_private) + end + + params.permit( + params_to_permit ) end From a47903cdad02ca4fc11f39159fbe1180b1c19f5f Mon Sep 17 00:00:00 2001 From: Viktor Smari Date: Mon, 26 Aug 2019 11:55:15 +0200 Subject: [PATCH 09/13] Test: normal user cannot patch is_private --- spec/requests/v0/devices_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/spec/requests/v0/devices_spec.rb b/spec/requests/v0/devices_spec.rb index fefd9edc..6dbc1c09 100644 --- a/spec/requests/v0/devices_spec.rb +++ b/spec/requests/v0/devices_spec.rb @@ -176,6 +176,12 @@ let!(:device) { create :device, owner: user } + it "cannot update a device is_private attribute" do + api_put "devices/#{device.id}", { is_private: true, access_token: token.token } + expect(response.status).to eq(200) + expect(Device.find(device.id).is_private).to eq(false) + end + it "updates a device" do api_put "devices/#{device.id}", { name: 'new name', access_token: token.token } expect(response.status).to eq(200) From 4c18f650c59f672781886008da53dbfacfc43ef2 Mon Sep 17 00:00:00 2001 From: Viktor Smari Date: Mon, 26 Aug 2019 11:59:31 +0200 Subject: [PATCH 10/13] Test: user with role_mask can change is_private --- spec/requests/v0/devices_spec.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/spec/requests/v0/devices_spec.rb b/spec/requests/v0/devices_spec.rb index 6dbc1c09..d3fc6971 100644 --- a/spec/requests/v0/devices_spec.rb +++ b/spec/requests/v0/devices_spec.rb @@ -182,6 +182,13 @@ expect(Device.find(device.id).is_private).to eq(false) end + it "can update a device is_private attribute when user has role" do + user.update role_mask: 3 + api_put "devices/#{device.id}", { is_private: true, access_token: token.token } + expect(response.status).to eq(200) + expect(Device.find(device.id).is_private).to eq(true) + end + it "updates a device" do api_put "devices/#{device.id}", { name: 'new name', access_token: token.token } expect(response.status).to eq(200) From e740dc1f15bcebe017c09b140c317fc4849c9d01 Mon Sep 17 00:00:00 2001 From: Viktor Smari Date: Mon, 26 Aug 2019 15:34:36 +0200 Subject: [PATCH 11/13] Superuser renamed to researcher with role_mask = 2 --- app/controllers/v0/devices_controller.rb | 4 ++-- app/models/user.rb | 6 +----- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/app/controllers/v0/devices_controller.rb b/app/controllers/v0/devices_controller.rb index 58c18d1f..4712ec16 100644 --- a/app/controllers/v0/devices_controller.rb +++ b/app/controllers/v0/devices_controller.rb @@ -143,8 +143,8 @@ def device_params :user_tags ] - # Superusers + Admins can update is_private - if current_user.role_mask >= 3 + # Researchers + Admins can update is_private + if current_user.role_mask >= 2 params_to_permit.push(:is_private) end diff --git a/app/models/user.rb b/app/models/user.rb index 559bf0b0..ea0d3e81 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -97,12 +97,8 @@ def is_admin? role == 'admin' end - def is_superuser? - role == 'superuser' - end - def role - role_mask == 3 ? 'superuser' : nil + role_mask == 2 ? 'researcher' : nil role_mask < 5 ? 'citizen' : 'admin' end From 8b674c9526410edf3bea1f808005025157a99a04 Mon Sep 17 00:00:00 2001 From: Viktor Smari Date: Mon, 26 Aug 2019 15:40:47 +0200 Subject: [PATCH 12/13] role method bugfix --- app/models/user.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index ea0d3e81..b7c88740 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -98,7 +98,7 @@ def is_admin? end def role - role_mask == 2 ? 'researcher' : nil + return 'researcher' if role_mask == 2 role_mask < 5 ? 'citizen' : 'admin' end From 20423e9ba329bfd410e05fd3b4260363a5210ef8 Mon Sep 17 00:00:00 2001 From: Viktor Smari Date: Mon, 26 Aug 2019 16:40:12 +0200 Subject: [PATCH 13/13] Seeds file also seeds private devices --- db/seeds.rb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/db/seeds.rb b/db/seeds.rb index 9d81a29f..02b16daa 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -26,7 +26,7 @@ 3.times do Kit.create( name: "Kit #{Faker::Educator.campus}", - description: Faker::Lorem.sentence(5), + description: Faker::Lorem.sentence, slug: 'sck:1,1', sensor_map: {"temp": 12, "hum": 13, "light": 14} ) @@ -89,10 +89,11 @@ #device has many sensors through components #has_many :components, as: :board -5.times do + +10.times do Device.create( { - owner: User.first, + owner: User.all.sample, name: Faker::Address.city, city: Faker::Address.city, country_code: Faker::Address.country_code, @@ -102,6 +103,7 @@ latitude: 42.385, longitude: 2.173, device_token: Faker::Crypto.sha1[0,6], + is_private: [true, false].sample, notify_low_battery: [true, false].sample, notify_low_battery_timestamp: Time.now, notify_stopped_publishing: [true, false].sample, @@ -128,6 +130,7 @@ # belongs_to :sensor # Kit and Device have many Components, as: :board + Component.create( board: Kit.first, sensor: Sensor.find(12) ) @@ -181,7 +184,8 @@ report: {"random_property":"random_result"}, ) -Device.find(1).update_attributes( +d = Device.first +d.update!( hardware_info: { "id": 1, "uuid": "7d45fead-defd-4482-bc6a-a1b711879e2d",