Mercurial > gemma
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()'