changeset 410:3f803d64a6ee

Do not rely on session_user for authorization Privileges are usually checked based on current_user, which can be changed using SET ROLE, while session_user is based on the actually logged in user and can only be changed by a superuser using SET SESSION AUTHORIZATION. Using session_user for authorization purposes prevents the expected behaviour of SET ROLE. current_user_country() does not need to be SECURITY DEFINER since a while, because there is no RLS policy affected by what is mentioned in the removed comment.
author Tom Gottfried <tom@intevation.de>
date Wed, 15 Aug 2018 16:39:00 +0200
parents cdd63547930a
children 21fb992b1f5a
files schema/manage_users.sql schema/manage_users_tests.sql schema/run_tests.sh
diffstat 3 files changed, 60 insertions(+), 25 deletions(-) [+]
line wrap: on
line diff
--- a/schema/manage_users.sql	Wed Aug 15 15:57:36 2018 +0200
+++ b/schema/manage_users.sql	Wed Aug 15 16:39:00 2018 +0200
@@ -24,19 +24,6 @@
     LANGUAGE plpgsql;
 
 
--- Security-definer function to get current users country, which allows to
--- restrict the view on user_profiles by country without infinite recursion
-CREATE FUNCTION users.current_user_country()
-    RETURNS internal.user_profiles.country%TYPE
-    AS $$
-        SELECT country FROM internal.user_profiles
-            WHERE username = session_user
-    $$
-    LANGUAGE SQL
-    SECURITY DEFINER
-    STABLE PARALLEL SAFE;
-
-
 CREATE OR REPLACE VIEW users.list_users WITH (security_barrier) AS
     SELECT
             r.rolname,
@@ -51,11 +38,23 @@
             JOIN pg_roles r ON a.roleid = r.oid
         WHERE p.username = current_user
             OR pg_has_role('waterway_admin', 'MEMBER')
-                AND p.country = users.current_user_country()
+                AND p.country = (
+                    SELECT country FROM internal.user_profiles
+                        WHERE username = current_user)
             OR pg_has_role('pw_reset', 'MEMBER')
             OR pg_has_role('sys_admin', 'MEMBER');
 
 
+CREATE OR REPLACE FUNCTION users.current_user_country()
+    RETURNS internal.user_profiles.country%TYPE
+    AS $$
+        SELECT country FROM users.list_users
+            WHERE username = current_user
+    $$
+    LANGUAGE SQL
+    STABLE PARALLEL SAFE;
+
+
 CREATE OR REPLACE FUNCTION internal.create_user() RETURNS trigger
 AS $$
 BEGIN
@@ -86,6 +85,31 @@
     EXECUTE PROCEDURE internal.create_user();
 
 
+-- Prevent roles other than sys_admin and pw_reset to update any user but
+-- themselves (affects waterway_admin)
+CREATE OR REPLACE FUNCTION internal.authorize_update_user() RETURNS trigger
+AS $$
+BEGIN
+    IF OLD.username <> current_user
+        AND NOT (pg_has_role('sys_admin', 'MEMBER')
+            OR pg_has_role('pw_reset', 'MEMBER'))
+    THEN
+        RETURN NULL;
+    ELSE
+        RETURN NEW;
+    END IF;
+END;
+$$
+    LANGUAGE plpgsql;
+
+-- Note that PostgreSQL fires triggers for the same event in alphabetical
+-- order! Make sure that authorization takes place before any other trigger
+-- is fired that might execute otherwise unauthorized statements!
+CREATE TRIGGER authorize_update_user INSTEAD OF UPDATE ON users.list_users
+    FOR EACH ROW
+    EXECUTE PROCEDURE internal.authorize_update_user();
+
+
 CREATE OR REPLACE FUNCTION internal.update_user() RETURNS trigger
 AS $$
 DECLARE
@@ -93,14 +117,6 @@
 BEGIN
     cur_username = OLD.username;
 
-    IF cur_username <> session_user
-        AND NOT (pg_has_role(session_user, 'sys_admin', 'MEMBER')
-            OR pg_has_role(session_user, 'pw_reset', 'MEMBER'))
-    THEN
-        -- Discard row. This is what WITH CHECK in an RLS policy would do.
-        RETURN NULL;
-    END IF;
-
     UPDATE internal.user_profiles p
         SET (username, country, map_extent, email_address)
         = (NEW.username, NEW.country, NEW.map_extent, NEW.email_address)
--- a/schema/manage_users_tests.sql	Wed Aug 15 15:57:36 2018 +0200
+++ b/schema/manage_users_tests.sql	Wed Aug 15 16:39:00 2018 +0200
@@ -138,6 +138,25 @@
     $$,
     'Waterway admin cannot update attributes of other users in country');
 
+-- The above test will pass even if the password is actually updated in case
+-- a trigger returns NULL after ALTER ROLE ... PASSWORD ... has been executed.
+RESET SESSION AUTHORIZATION;
+CREATE TEMP TABLE old_pw_hash AS
+    SELECT rolpassword FROM pg_authid WHERE rolname = 'test_user_at';
+SET SESSION AUTHORIZATION test_admin_at;
+UPDATE users.list_users
+    SET pw = 'test_user_at2!'
+    WHERE username = 'test_user_at';
+RESET SESSION AUTHORIZATION;
+SELECT set_eq($$
+    SELECT rolpassword FROM old_pw_hash
+    $$,
+    $$
+    SELECT rolpassword FROM pg_authid WHERE rolname = 'test_user_at'
+    $$,
+    'Waterway admin cannot update password of other users in country');
+
+
 SET SESSION AUTHORIZATION test_sys_admin1;
 
 SELECT lives_ok($$
@@ -223,8 +242,8 @@
 -- To compare passwords, we need to run the following tests as superuser
 RESET SESSION AUTHORIZATION;
 
-CREATE TEMP TABLE old_pw_hash AS
-    SELECT rolpassword FROM pg_authid WHERE rolname = 'test_user_at';
+UPDATE old_pw_hash SET rolpassword = (
+    SELECT rolpassword FROM pg_authid WHERE rolname = 'test_user_at');
 
 UPDATE users.list_users
     SET (rolname, username, pw, country, map_extent, email_address)
--- a/schema/run_tests.sh	Wed Aug 15 15:57:36 2018 +0200
+++ b/schema/run_tests.sh	Wed Aug 15 16:39:00 2018 +0200
@@ -16,7 +16,7 @@
     -c 'SET client_min_messages TO WARNING' \
     -c "DROP ROLE IF EXISTS $TEST_ROLES" \
     -f tap_tests_data.sql \
-    -c 'SELECT plan(44)' \
+    -c 'SELECT plan(45)' \
     -f auth_tests.sql \
     -f manage_users_tests.sql \
     -c 'SELECT * FROM finish()'