From aeb116bace23f66a86caf6d5868ea82dfb901e36 Mon Sep 17 00:00:00 2001
From: pennae <github@quasiparticle.net>
Date: Wed, 10 Aug 2022 01:54:20 +0200
Subject: don't allow users to edit devices of other users
while device ids should be impossible to guess (being as long as oauth
tokens), we should still guard against malicious activity if they should
ever leak.
---
...er_id_check_to_insert_or_update_device.down.sql | 4 ++
...user_id_check_to_insert_or_update_device.up.sql | 47 ++++++++++++++++++++++
tests/conftest.py | 11 +++++
tests/test_auth_device.py | 15 +++++++
4 files changed, 77 insertions(+)
create mode 100644 migrations/20220809225706_add_user_id_check_to_insert_or_update_device.down.sql
create mode 100644 migrations/20220809225706_add_user_id_check_to_insert_or_update_device.up.sql
diff --git a/migrations/20220809225706_add_user_id_check_to_insert_or_update_device.down.sql b/migrations/20220809225706_add_user_id_check_to_insert_or_update_device.down.sql
new file mode 100644
index 0000000..51d1743
--- /dev/null
+++ b/migrations/20220809225706_add_user_id_check_to_insert_or_update_device.down.sql
@@ -0,0 +1,4 @@
+drop function insert_or_update_device(device_id, user_id, text, text, device_push_info,
+ device_command[], jsonb, out device);
+alter function insert_or_update_device_1(device_id, user_id, text, text, device_push_info,
+ device_command[], jsonb, out device) rename to insert_or_update_device;
diff --git a/migrations/20220809225706_add_user_id_check_to_insert_or_update_device.up.sql b/migrations/20220809225706_add_user_id_check_to_insert_or_update_device.up.sql
new file mode 100644
index 0000000..06dbb94
--- /dev/null
+++ b/migrations/20220809225706_add_user_id_check_to_insert_or_update_device.up.sql
@@ -0,0 +1,47 @@
+alter function insert_or_update_device(device_id, user_id, text, text, device_push_info,
+ device_command[], jsonb, out device) rename to insert_or_update_device_1;
+
+create function insert_or_update_device(
+ p_device_id device_id,
+ p_user_id user_id,
+ p_name text,
+ p_type text,
+ p_push device_push_info,
+ p_available_commands device_command[],
+ p_location jsonb,
+ out result device)
+ returns setof device
+ language plpgsql
+ as $$
+ begin
+ update device
+ set name = coalesce(p_name, name),
+ type = coalesce(p_type, type),
+ push = coalesce(p_push, push),
+ available_commands = coalesce(p_available_commands, available_commands),
+ push_expired = push_expired and p_push is null,
+ location = coalesce(p_location, location)
+ where device_id = p_device_id and user_id = p_user_id
+ returning *
+ into result;
+
+ if not found then
+ begin
+ insert into device (device_id, user_id, name, type, push, push_expired,
+ available_commands, location)
+ values (p_device_id, p_user_id, p_name, p_type, p_push, p_push is null,
+ p_available_commands, coalesce(p_location, '{}'::jsonb))
+ returning *
+ into result;
+ exception
+ when unique_violation then
+ -- almost certainly updating another user's device, or we have
+ -- a broken RNG on the system. return nothing to signal either
+ -- error.
+ return;
+ end;
+ end if;
+
+ return next;
+ end
+ $$;
diff --git a/tests/conftest.py b/tests/conftest.py
index bf877e2..487eff9 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -55,6 +55,17 @@ def _account(client, primary, email, mail_server):
email1 = f"test.account-{os.urandom(8).hex()}@test-auth"
email2 = f"test.account2-{os.urandom(8).hex()}@test-auth"
+@pytest.fixture(scope="class")
+def account_plain(request, mail_server):
+ for a in _account(AuthClient(), True, email1, mail_server):
+ yield a
+ break
+@pytest.fixture(scope="class")
+def account2_plain(request, mail_server):
+ for a in _account(AuthClient(), True, email2, mail_server):
+ yield a
+ break
+
@pytest.fixture(params=[True, False], ids=["primary", "secondary"], scope="class")
def account(request, mail_server):
for a in _account(AuthClient(), request.param, email1, mail_server):
diff --git a/tests/test_auth_device.py b/tests/test_auth_device.py
index 5ec42f3..c978d87 100644
--- a/tests/test_auth_device.py
+++ b/tests/test_auth_device.py
@@ -192,6 +192,21 @@ def test_change(account_or_rt, populate_devices):
assert mdevs1[i1]['pushPublicKey'] or '' == mdevs2[i2]['pushPublicKey'] or ''
assert mdevs1[i1]['pushAuthKey'] or '' == mdevs2[i2]['pushAuthKey'] or ''
+def test_change_foreign(account_plain, account2_plain):
+ dev = account_plain.post_a("/account/device", device_data[0])
+ dev['name'] = 'foo'
+ del dev['isCurrentDevice']
+ del dev['lastAccessTime']
+ del dev['pushEndpointExpired']
+ with pytest.raises(ClientError) as e:
+ account2_plain.post_a("/account/device", dev)
+ assert e.value.details == {
+ 'code': 400,
+ 'errno': 123,
+ 'error': 'Bad Request',
+ 'message': 'unknown device'
+ }
+
def test_invoke_noauth(client):
body = {"target": "0" * 32, "command": "foo", "payload": {}, "ttl": 10}
with pytest.raises(ClientError) as e:
--
cgit v1.2.3