Mercurial > gemma

changeset 4723:baabc2b2f094

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
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;