Mercurial > gemma

changeset 307:750a9c9cd965

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
Use SQL UPDATE to update users This implies it's not a database error anymore to try to update a non-existent user. Thus, handle this as a HTTP-404 in the backend, which is in line with what GET does. Using UPDATE here will allow to GRANT column-wise privileges. The password has become part of the view to be updatable as well.
author Tom Gottfried <tom@intevation.de>
date Wed, 01 Aug 2018 15:49:38 +0200
parents 70592a18c5c6
children e964c617265e
files controllers/user.go schema/auth.sql schema/manage_users.sql schema/manage_users_tests.sql schema/run_tests.sh
diffstat 5 files changed, 107 insertions(+), 86 deletions(-) [+]
line wrap: on
line diff
--- a/controllers/user.go	Wed Aug 01 15:18:26 2018 +0200
+++ b/controllers/user.go	Wed Aug 01 15:49:38 2018 +0200
@@ -15,9 +15,14 @@
 	createUserExtentSQL = `SELECT sys_admin.create_user($1, $2, $3, $4,
   ST_MakeBox2D(ST_Point($5, $6), ST_Point($7, $8)), $9)`
 
-	updateUserSQL       = `SELECT sys_admin.update_user($1, $2, $3, $4, $5, NULL, $6)`
-	updateUserExtentSQL = `SELECT sys_admin.update_user($1, $2, $3, $4, $5,
-  ST_MakeBox2D(ST_Point($6, $7), ST_Point($8, $9)), $10)`
+	updateUserSQL = `UPDATE users.list_users
+  SET (rolname, username, pw, country, map_extent, email_address)
+  = ($2, $3, $4, $5, NULL, $6)
+  WHERE username = $1`
+	updateUserExtentSQL = `UPDATE users.list_users
+  SET (rolname, username, pw, country, map_extent, email_address)
+  = ($2, $3, $4, $5, ST_MakeBox2D(ST_Point($6, $7), ST_Point($8, $9)), $10)
+  WHERE username = $1`
 
 	deleteUserSQL = `SELECT sys_admin.delete_user($1)`
 
@@ -80,9 +85,10 @@
 	}
 
 	newUser := input.(*User)
+	var res sql.Result
 
 	if newUser.Extent == nil {
-		_, err = db.Exec(
+		res, err = db.Exec(
 			updateUserSQL,
 			user,
 			newUser.Role,
@@ -92,7 +98,7 @@
 			newUser.Email,
 		)
 	} else {
-		_, err = db.Exec(
+		res, err = db.Exec(
 			updateUserExtentSQL,
 			user,
 			newUser.Role,
@@ -105,6 +111,13 @@
 		)
 	}
 
+	if n, _ := res.RowsAffected(); n == 0 {
+		err = JSONError{
+			Code:    http.StatusNotFound,
+			Message: fmt.Sprintf("Cannot find user %s.", user),
+		}
+		return
+	}
 	if err != nil {
 		return
 	}
--- a/schema/auth.sql	Wed Aug 01 15:18:26 2018 +0200
+++ b/schema/auth.sql	Wed Aug 01 15:49:38 2018 +0200
@@ -25,7 +25,7 @@
 -- Extended privileges for sys_admin
 --
 GRANT INSERT, UPDATE, DELETE
-    ON users.responsibility_areas TO sys_admin;
+    ON users.list_users, users.responsibility_areas TO sys_admin;
 GRANT USAGE ON SCHEMA sys_admin TO sys_admin;
 GRANT SELECT ON ALL TABLES IN SCHEMA sys_admin TO sys_admin;
 GRANT UPDATE ON sys_admin.system_config TO sys_admin;
--- a/schema/manage_users.sql	Wed Aug 01 15:18:26 2018 +0200
+++ b/schema/manage_users.sql	Wed Aug 01 15:49:38 2018 +0200
@@ -38,7 +38,13 @@
 
 
 CREATE OR REPLACE VIEW users.list_users WITH (security_barrier) AS
-    SELECT r.rolname, p.*
+    SELECT
+            r.rolname,
+            p.username,
+            CAST('' AS varchar) AS pw,
+            p.country,
+            p.map_extent,
+            p.email_address
         FROM internal.user_profiles p
             JOIN pg_roles u ON p.username = u.rolname
             JOIN pg_auth_members a ON u.oid = a.member
@@ -79,61 +85,52 @@
     SECURITY DEFINER;
 
 
-CREATE OR REPLACE FUNCTION sys_admin.update_user(
-       username internal.user_profiles.username%TYPE,
-       new_userrole varchar,
-       new_username internal.user_profiles.username%TYPE,
-       new_pw varchar,
-       new_country internal.user_profiles.country%TYPE,
-       new_map_extent internal.user_profiles.map_extent%TYPE,
-       new_email_address internal.user_profiles.email_address%TYPE
-    )
-    RETURNS void
+CREATE OR REPLACE FUNCTION internal.update_user() RETURNS trigger
 AS $$
 DECLARE
-    cur_username name;
-    cur_userrole name;
+    cur_username varchar;
 BEGIN
-    cur_username = username;
+    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)
+        = (NEW.username, NEW.country, NEW.map_extent, NEW.email_address)
         WHERE p.username = cur_username;
 
-    IF new_username <> cur_username
+    IF NEW.username <> cur_username
     THEN
         EXECUTE format(
-            'ALTER ROLE %I RENAME TO %I', username, new_username);
-        cur_username = new_username;
+            'ALTER ROLE %I RENAME TO %I', cur_username, NEW.username);
+        cur_username = NEW.username;
     END IF;
 
-    cur_userrole = rolname FROM pg_roles r
-        JOIN pg_auth_members a ON r.oid = a.roleid
-        WHERE member = (
-            SELECT oid FROM pg_roles WHERE rolname = cur_username);
-    IF new_userrole <> cur_userrole
+    IF NEW.rolname <> OLD.rolname
     THEN
         EXECUTE format(
-            'REVOKE %I FROM %I', cur_userrole, cur_username);
+            'REVOKE %I FROM %I', OLD.rolname, cur_username);
+        EXECUTE format(
+            'GRANT %I TO %I', NEW.rolname, cur_username);
     END IF;
-    -- GRANT new_userrole unconditionally to ensure it's an error to upgrade
-    -- a non-existent cur_username (GRANTing a role twice is not an error)
-    EXECUTE format(
-        'GRANT %I TO %I', new_userrole, cur_username);
 
-    IF new_pw IS NOT NULL AND new_pw <> ''
+    IF NEW.pw IS NOT NULL AND NEW.pw <> ''
     THEN
         EXECUTE format(
             'ALTER ROLE %I PASSWORD %L',
             cur_username,
-            users.check_password(new_pw));
+            users.check_password(NEW.pw));
     END IF;
+
+    -- Do not leak new password
+    NEW.pw = '';
+    RETURN NEW;
 END;
 $$
     LANGUAGE plpgsql
     SECURITY DEFINER;
 
+CREATE TRIGGER update_user INSTEAD OF UPDATE ON users.list_users FOR EACH ROW
+    EXECUTE PROCEDURE internal.update_user();
+
 
 CREATE OR REPLACE FUNCTION sys_admin.delete_user(
        username internal.user_profiles.username%TYPE
--- a/schema/manage_users_tests.sql	Wed Aug 01 15:18:26 2018 +0200
+++ b/schema/manage_users_tests.sql	Wed Aug 01 15:49:38 2018 +0200
@@ -105,74 +105,79 @@
 SELECT lives_ok($$
     SELECT sys_admin.create_user(
         'waterway_user', 'test2', 'secret1$', 'AT', NULL, 'test2');
-    SELECT sys_admin.update_user('test2',
-        'waterway_user', 'test2_new', 'new_secret1$', 'AT',
-        (SELECT map_extent FROM users.list_users
-            WHERE username = 'test_user_at'), 'test5')
+    UPDATE users.list_users
+        SET (rolname, username, pw, country, map_extent, email_address)
+            = ('waterway_user', 'test2_new', 'new_secret1$', 'AT',
+                (SELECT map_extent FROM users.list_users
+                    WHERE username = 'test_user_at'), 'test5')
+        WHERE username = 'test2'
     $$,
     'Existing user can be updated');
 
 SELECT throws_ok($$
-    SELECT sys_admin.update_user('test_non_existent',
-        'waterway_user', 'test_non_existent', '', 'AT',
-        (SELECT map_extent FROM users.list_users
-            WHERE username = 'test_user_at'), 'test5')
-    $$,
-    42704, NULL,
-    'Non-existent user cannot be updated');
-
-SELECT throws_ok($$
-    SELECT sys_admin.update_user(CAST(current_user AS varchar),
-        'waterway_user', 'test_new_name', 'secret1$', 'AT',
-        (SELECT map_extent FROM users.list_users
-            WHERE username = 'test_user_at'), 'test6')
+    UPDATE users.list_users
+        SET (rolname, username, pw, country, map_extent, email_address)
+            = ('waterway_user', 'test_new_name', 'secret1$', 'AT',
+                (SELECT map_extent FROM users.list_users
+                    WHERE username = 'test_user_at'), 'test6')
+        WHERE username = CAST(current_user AS varchar)
     $$,
     '0A000', NULL,
     'Name of current user cannot be altered');
 
 SELECT throws_ok($$
-    SELECT sys_admin.update_user('test_user_at',
-        'invalid', 'test2', 'secret1$', 'AT',
-        (SELECT map_extent FROM users.list_users
-            WHERE username = 'test_user_at'), 'test2')
+    UPDATE users.list_users
+        SET (rolname, username, pw, country, map_extent, email_address)
+            = ('invalid', 'test2', 'secret1$', 'AT',
+                (SELECT map_extent FROM users.list_users
+                    WHERE username = 'test_user_at'), 'test2')
+        WHERE username = 'test_user_at'
     $$,
     42704, NULL,
     'Valid role name has to be provided');
 
 SELECT throws_ok($$
-    SELECT sys_admin.update_user('test_user_at',
-        'waterway_user', NULL, 'secret1$', 'AT',
-        (SELECT map_extent FROM users.list_users
-            WHERE username = 'test_user_at'), 'test3')
+    UPDATE users.list_users
+        SET (rolname, username, pw, country, map_extent, email_address)
+            = ('waterway_user', NULL, 'secret1$', 'AT',
+                (SELECT map_extent FROM users.list_users
+                    WHERE username = 'test_user_at'), 'test3')
+        WHERE username = 'test_user_at'
     $$,
     23502, NULL,
     'New username is mandatory');
 -- Though other arguments are mandatory, too, there are no explicit tests
 
 SELECT throws_ok($$
-    SELECT sys_admin.update_user('test_user_at',
-        'waterway_user', 'waterway_user', 'secret1$', 'AT',
-        (SELECT map_extent FROM users.list_users
-            WHERE username = 'test_user_at'), 'test4')
+    UPDATE users.list_users
+        SET (rolname, username, pw, country, map_extent, email_address)
+            = ('waterway_user', 'waterway_user', 'secret1$', 'AT',
+                (SELECT map_extent FROM users.list_users
+                    WHERE username = 'test_user_at'), 'test4')
+        WHERE username = 'test_user_at'
     $$,
     42710, NULL,
     'Reserved role names cannot be used as username');
 
 SELECT throws_ok($$
-    SELECT sys_admin.update_user('test_user_at',
-        'waterway_user', 'test_user_ro', 'secret1$', 'AT',
-        (SELECT map_extent FROM users.list_users
-            WHERE username = 'test_user_at'), 'test4')
+    UPDATE users.list_users
+        SET (rolname, username, pw, country, map_extent, email_address)
+            = ('waterway_user', 'test_user_ro', 'secret1$', 'AT',
+                (SELECT map_extent FROM users.list_users
+                    WHERE username = 'test_user_at'), 'test4')
+        WHERE username = 'test_user_at'
     $$,
     23505, NULL,
     'No duplicate user name is allowed');
 
 -- Test password policy (only one rule to ensure it's also used on update)
 SELECT throws_ok($$
-    SELECT sys_admin.update_user('test_user_at',
-        'waterway_user', 'test_user_at', 'secret', 'AT',
-        (SELECT map_extent FROM users.list_users
-            WHERE username = 'test_user_at'), 'test4')
+    UPDATE users.list_users
+        SET (rolname, username, pw, country, map_extent, email_address)
+            = ('waterway_user', 'test_user_at', 'secret', 'AT',
+                (SELECT map_extent FROM users.list_users
+                    WHERE username = 'test_user_at'), 'test4')
+        WHERE username = 'test_user_at'
     $$,
     '28P01', NULL,
     'Non-compliant password is not accepted');
@@ -183,10 +188,12 @@
 CREATE TEMP TABLE old_pw_hash AS
     SELECT rolpassword FROM pg_authid WHERE rolname = 'test_user_at';
 
-SELECT sys_admin.update_user('test_user_at',
-    'waterway_user', 'test_user_at', NULL, 'AT',
-    (SELECT map_extent FROM internal.user_profiles
-        WHERE username = 'test_user_at'), 'xxx');
+UPDATE users.list_users
+    SET (rolname, username, pw, country, map_extent, email_address)
+        = ('waterway_user', 'test_user_at', NULL, 'AT',
+            (SELECT map_extent FROM internal.user_profiles
+                WHERE username = 'test_user_at'), 'xxx')
+    WHERE username = 'test_user_at';
 SELECT set_eq($$
     SELECT rolpassword FROM old_pw_hash
     $$,
@@ -195,10 +202,12 @@
     $$,
     'Giving NULL password does not change password');
 
-SELECT sys_admin.update_user('test_user_at',
-    'waterway_user', 'test_user_at', '', 'AT',
-    (SELECT map_extent FROM internal.user_profiles
-        WHERE username = 'test_user_at'), 'xxx');
+UPDATE users.list_users
+    SET (rolname, username, pw, country, map_extent, email_address)
+        = ('waterway_user', 'test_user_at', '', 'AT',
+            (SELECT map_extent FROM internal.user_profiles
+                WHERE username = 'test_user_at'), 'xxx')
+    WHERE username = 'test_user_at';
 SELECT set_eq($$
     SELECT rolpassword FROM old_pw_hash
     $$,
@@ -207,10 +216,12 @@
     $$,
     'Giving empty string as password does not change password');
 
-SELECT sys_admin.update_user('test_user_at',
-    'waterway_user', 'test_user_at', 'new_pw1$', 'AT',
-    (SELECT map_extent FROM internal.user_profiles
-        WHERE username = 'test_user_at'), 'xxx');
+UPDATE users.list_users
+    SET (rolname, username, pw, country, map_extent, email_address)
+        = ('waterway_user', 'test_user_at', 'new_pw1$', 'AT',
+            (SELECT map_extent FROM internal.user_profiles
+                WHERE username = 'test_user_at'), 'xxx')
+    WHERE username = 'test_user_at';
 SELECT set_ne($$
     SELECT rolpassword FROM old_pw_hash
     $$,
--- a/schema/run_tests.sh	Wed Aug 01 15:18:26 2018 +0200
+++ b/schema/run_tests.sh	Wed Aug 01 15:49:38 2018 +0200
@@ -15,7 +15,7 @@
 psql -qXv ON_ERROR_STOP= -v -d gemma_test \
     -c "DROP ROLE IF EXISTS $TEST_ROLES" \
     -f tap_tests_data.sql \
-    -c 'SELECT plan(40)' \
+    -c 'SELECT plan(39)' \
     -f auth_tests.sql \
     -f manage_users_tests.sql \
     -c 'SELECT * FROM finish()'