Mercurial > gemma
changeset 4723:baabc2b2f094
Avoid creating user profiles without matching role
The INSTEAD OF triggers on users.list_users did that already, but
profile data coming e.g. via restoring a dump had been added also
if there was no matching database role in the cluster.
This also unifies the errors occuring on creation of users with existing
role names that differed between roles with and without profile before.
Note this is no referential integrity. A dropped role still leaves an
orphaned profile behind.
author | Tom Gottfried <tom@intevation.de> |
---|---|
date | Thu, 17 Oct 2019 18:56:59 +0200 |
parents | 462d8f71da62 |
children | 89abbf65292c |
files | pkg/pgxutils/errors.go schema/gemma.sql schema/manage_users.sql schema/manage_users_tests.sql schema/run_tests.sh schema/updates/1307/01.improve_rolename_check.sql |
diffstat | 6 files changed, 113 insertions(+), 35 deletions(-) [+] |
line wrap: on
line diff
--- a/pkg/pgxutils/errors.go Thu Oct 17 16:36:58 2019 +0200 +++ b/pkg/pgxutils/errors.go Thu Oct 17 18:56:59 2019 +0200 @@ -27,6 +27,7 @@ uniqueViolation = "23505" checkViolation = "23514" violatesRowLevelSecurity = "42501" + duplicateObject = "42710" noDataFound = "P0002" ) @@ -59,6 +60,9 @@ c = http.StatusInternalServerError + // Most recent line from stacktrace contains failed statement + recent := strings.SplitN(err.Where, "\n", 1)[0] + switch err.Code { case notNullViolation: switch err.SchemaName { @@ -94,16 +98,6 @@ } case uniqueViolation: switch err.SchemaName { - case "internal": - switch err.TableName { - case "user_profiles": - switch err.ConstraintName { - case "user_profiles_pkey": - m = "A user with that name already exists" - c = http.StatusConflict - return - } - } case "users": switch err.TableName { case "stretches": @@ -138,9 +132,14 @@ } } } + case duplicateObject: + switch { + case strings.Contains(recent, "CREATE ROLE"): + m = "A user with that name already exists" + c = http.StatusConflict + return + } case noDataFound: - // Most recent line from stacktrace contains name of failed function - recent := strings.SplitN(err.Where, "\n", 1)[0] switch { case strings.Contains(recent, "isrsrange_points"): m = "No distance mark found for at least one given ISRS Location Code"
--- a/schema/gemma.sql Thu Oct 17 16:36:58 2019 +0200 +++ b/schema/gemma.sql Thu Oct 17 18:56:59 2019 +0200 @@ -350,7 +350,9 @@ CREATE SCHEMA internal -- Profile data are only accessible via the view users.list_users. CREATE TABLE user_profiles ( - username varchar PRIMARY KEY CHECK(octet_length(username) <= 63), + username varchar PRIMARY KEY + CHECK(octet_length(username) <= 63) + CHECK(to_regrole(quote_ident(username)) IS NOT NULL), -- keep username length compatible with role identifier country char(2) NOT NULL REFERENCES countries, map_extent box2d NOT NULL,
--- a/schema/manage_users.sql Thu Oct 17 16:36:58 2019 +0200 +++ b/schema/manage_users.sql Thu Oct 17 18:56:59 2019 +0200 @@ -99,14 +99,20 @@ JOIN users.stretch_countries stc ON stc.stretch_id = st.id WHERE stc.country = NEW.country; END IF; + + IF NEW.username IS NOT NULL + -- otherwise let the constraint on user_profiles speak + THEN + EXECUTE format( + 'CREATE ROLE %I IN ROLE %I LOGIN PASSWORD %L', + NEW.username, + NEW.rolname, + internal.check_password(NEW.pw)); + END IF; + INSERT INTO internal.user_profiles ( username, country, map_extent, email_address) VALUES (NEW.username, NEW.country, NEW.map_extent, NEW.email_address); - EXECUTE format( - 'CREATE ROLE %I IN ROLE %I LOGIN PASSWORD %L', - NEW.username, - NEW.rolname, - internal.check_password(NEW.pw)); -- Do not leak new password NEW.pw = ''; @@ -167,11 +173,6 @@ BEGIN cur_username = OLD.username; - UPDATE internal.user_profiles p - SET (username, country, map_extent, email_address) - = (NEW.username, NEW.country, NEW.map_extent, NEW.email_address) - WHERE p.username = cur_username; - IF NEW.username <> cur_username THEN EXECUTE format( @@ -179,6 +180,11 @@ cur_username = NEW.username; END IF; + UPDATE internal.user_profiles p + SET (username, country, map_extent, email_address) + = (NEW.username, NEW.country, NEW.map_extent, NEW.email_address) + WHERE p.username = cur_username; + IF NEW.rolname <> OLD.rolname THEN EXECUTE format(
--- a/schema/manage_users_tests.sql Thu Oct 17 16:36:58 2019 +0200 +++ b/schema/manage_users_tests.sql Thu Oct 17 18:56:59 2019 +0200 @@ -103,17 +103,10 @@ SELECT throws_ok($$ INSERT INTO users.list_users VALUES ( - 'waterway_user', 'waterway_user', 'secret1$', 'AT', NULL, 'test4') + 'waterway_user', 'test_user_at', 'secret1$', 'AT', NULL, 'test4') $$, 42710, NULL, - 'Reserved role names cannot be used as username'); - -SELECT throws_ok($$ - INSERT INTO users.list_users VALUES ( - 'waterway_user', 'test_user_at', 'secret1$', 'AT', NULL, 'test4') - $$, - 23505, NULL, - 'No duplicate user name is allowed'); + 'No existing role name is allowed'); SELECT throws_ok($$ INSERT INTO users.list_users VALUES ( @@ -271,8 +264,8 @@ WHERE username = 'test_user_at'), 'test4') WHERE username = 'test_user_at' $$, - 23505, NULL, - 'No duplicate user name is allowed'); + 42710, NULL, + 'No existing role name is allowed'); -- Test password policy (only one rule to ensure it's also used on update) SELECT throws_ok($$
--- a/schema/run_tests.sh Thu Oct 17 16:36:58 2019 +0200 +++ b/schema/run_tests.sh Thu Oct 17 18:56:59 2019 +0200 @@ -80,7 +80,7 @@ -c 'SET client_min_messages TO WARNING' \ -c "DROP ROLE IF EXISTS $TEST_ROLES" \ -f "$BASEDIR"/tap_tests_data.sql \ - -c "SELECT plan(78 + ( + -c "SELECT plan(77 + ( SELECT count(*)::int FROM information_schema.tables WHERE table_schema = 'waterway'))" \
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/schema/updates/1307/01.improve_rolename_check.sql Thu Oct 17 18:56:59 2019 +0200 @@ -0,0 +1,78 @@ +ALTER TABLE internal.user_profiles + ADD CHECK(to_regrole(quote_ident(username)) IS NOT NULL); + +CREATE OR REPLACE FUNCTION internal.create_user() RETURNS trigger +AS $$ +BEGIN + IF NEW.map_extent IS NULL + THEN + NEW.map_extent = ST_Extent(CAST(area AS geometry)) + FROM users.stretches st + JOIN users.stretch_countries stc ON stc.stretch_id = st.id + WHERE stc.country = NEW.country; + END IF; + + IF NEW.username IS NOT NULL + -- otherwise let the constraint on user_profiles speak + THEN + EXECUTE format( + 'CREATE ROLE %I IN ROLE %I LOGIN PASSWORD %L', + NEW.username, + NEW.rolname, + internal.check_password(NEW.pw)); + END IF; + + INSERT INTO internal.user_profiles ( + username, country, map_extent, email_address) + VALUES (NEW.username, NEW.country, NEW.map_extent, NEW.email_address); + + -- Do not leak new password + NEW.pw = ''; + RETURN NEW; +END; +$$ + LANGUAGE plpgsql + SECURITY DEFINER; + +CREATE OR REPLACE FUNCTION internal.update_user() RETURNS trigger +AS $$ +DECLARE + cur_username varchar; +BEGIN + cur_username = OLD.username; + + IF NEW.username <> cur_username + THEN + EXECUTE format( + 'ALTER ROLE %I RENAME TO %I', cur_username, NEW.username); + cur_username = NEW.username; + END IF; + + UPDATE internal.user_profiles p + SET (username, country, map_extent, email_address) + = (NEW.username, NEW.country, NEW.map_extent, NEW.email_address) + WHERE p.username = cur_username; + + IF NEW.rolname <> OLD.rolname + THEN + EXECUTE format( + 'REVOKE %I FROM %I', OLD.rolname, cur_username); + EXECUTE format( + 'GRANT %I TO %I', NEW.rolname, cur_username); + END IF; + + IF NEW.pw IS NOT NULL AND NEW.pw <> '' + THEN + EXECUTE format( + 'ALTER ROLE %I PASSWORD %L', + cur_username, + internal.check_password(NEW.pw)); + END IF; + + -- Do not leak new password + NEW.pw = ''; + RETURN NEW; +END; +$$ + LANGUAGE plpgsql + SECURITY DEFINER;