* Stephen Frost (sfr...@snowman.net) wrote: > * Robert Haas (robertmh...@gmail.com) wrote: > > On Tue, Apr 26, 2016 at 7:39 PM, Robert Haas <robertmh...@gmail.com> wrote: > > > On Mon, Apr 25, 2016 at 6:55 PM, Stephen Frost <sfr...@snowman.net> wrote: > > >> Based on our discussion at PGConf.US and the comments up-thread from > > >> Tom, I'll work up a patch to remove those checks around SET ROLE and > > >> friends which were trying to prevent default roles from possibly being > > >> made to own objects. > > >> > > >> Should the checks, which have been included since nearly the start of > > >> this version of the patch, to prevent users from GRANT'ing other rights > > >> to the default roles remain? Or should those also be removed? I > > >> *think* pg_dump/pg_upgrade would be fine with rights being added, and if > > >> we aren't preventing ownership of objects then we aren't going to be > > >> able to remove such roles in any case. > > > > > > It'd be good to test that that works. If it does, I think we may as > > > well allow it. > > > > > >> Of course, with these default roles, users can't REVOKE the rights which > > >> are granted to them as that happens in C code, outside of the GRANT > > >> system. > > > > > > I think you mean that they can't revoke the special magic rights, but > > > they could revoke any additional privileges which were granted. > > > > > >> Working up a patch to remove these checks should be pretty quickly done > > >> (iirc, I've actually got an independent patch around from when I added > > >> them, just need to find it and then go through the committed patches to > > >> make sure I take care of everything), but would like to make sure that > > >> we're now all on the same page and that *all* of these checks should be > > >> removed, making default roles just exactly like "regular" roles, except > > >> that they're created at initdb time and have "special" rights provided > > >> by C-level code checks. > > > > > > That's what I'm thinking. I would welcome other views. > > > > Ping! > > Thanks. I'm planning to post a patch tomorrow to remove these checks.
Apologies about not getting to this yesterday, was pretty busy finding pre-existing issues in pg_dump. Attached is a patch which removes the various special checks that I had added to prevent using default roles like regular roles. As noted in the commit message, users are still prevented from creating roles in the "pg_" namespace and from ALTER'ing those roles, but otherwise they're very much like regular roles. I've adjusted the regression tests accordingly, but I'm going to do more testing to make sure that pg_dump handles them correctly (and will be adding cases to my pg_dump TAP test suite to ensure that they stay working) over the next day or so. Barring objections or concerns, I'll push this sometime tomorrow (probably after I get back to DC). Thanks! Stephen
From 4fae6e77eddc86360381e44f35d4da4a495cbdc1 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfr...@snowman.net>
Date: Thu, 5 May 2016 09:52:26 -0400
Subject: [PATCH] Remove various special checks around default roles
Default roles really should be like regular roles, for the most part.
This removes a number of checks that were trying to make default roles
extra special by not allowing them to be used as regular roles.
We still prevent users from creating roles in the "pg_" namespace or
from altering roles which exist in that namespace via ALTER ROLE, as
we can't preserve such changes, but otherwise the roles are very much
like regular roles.
Based on discussion with Robert and Tom.
---
src/backend/catalog/aclchk.c | 7 -------
src/backend/commands/alter.c | 3 ---
src/backend/commands/foreigncmds.c | 13 -------------
src/backend/commands/policy.c | 5 -----
src/backend/commands/schemacmds.c | 4 ----
src/backend/commands/tablecmds.c | 2 --
src/backend/commands/tablespace.c | 4 ----
src/backend/commands/user.c | 11 -----------
src/backend/commands/variable.c | 7 -------
src/test/regress/expected/rolenames.out | 18 +++++-------------
src/test/regress/sql/rolenames.sql | 10 +++++-----
11 files changed, 10 insertions(+), 74 deletions(-)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 7d656d5..d074e85 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -423,9 +423,6 @@ ExecuteGrantStmt(GrantStmt *stmt)
grantee_uid = ACL_ID_PUBLIC;
break;
default:
- if (!IsBootstrapProcessingMode())
- check_rolespec_name((Node *) grantee,
- "Cannot GRANT or REVOKE privileges to or from a reserved role.");
grantee_uid = get_rolespec_oid((Node *) grantee, false);
break;
}
@@ -921,8 +918,6 @@ ExecAlterDefaultPrivilegesStmt(AlterDefaultPrivilegesStmt *stmt)
grantee_uid = ACL_ID_PUBLIC;
break;
default:
- check_rolespec_name((Node *) grantee,
- "Cannot GRANT or REVOKE default privileges to or from a reserved role.");
grantee_uid = get_rolespec_oid((Node *) grantee, false);
break;
}
@@ -1013,8 +1008,6 @@ ExecAlterDefaultPrivilegesStmt(AlterDefaultPrivilegesStmt *stmt)
{
RoleSpec *rolespec = lfirst(rolecell);
- check_rolespec_name((Node *) rolespec,
- "Cannot alter default privileges for reserved role.");
iacls.roleid = get_rolespec_oid((Node *) rolespec, false);
/*
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index 47a5c50..4b08cb8 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -747,9 +747,6 @@ ExecAlterOwnerStmt(AlterOwnerStmt *stmt)
{
Oid newowner = get_rolespec_oid(stmt->newowner, false);
- check_rolespec_name(stmt->newowner,
- "Cannot make reserved roles owners of objects.");
-
switch (stmt->objectType)
{
case OBJECT_DATABASE:
diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
index 88cefb7..804bab2 100644
--- a/src/backend/commands/foreigncmds.c
+++ b/src/backend/commands/foreigncmds.c
@@ -1148,10 +1148,6 @@ CreateUserMapping(CreateUserMappingStmt *stmt)
else
useId = get_rolespec_oid(stmt->user, false);
- /* Additional check to protect reserved role names */
- check_rolespec_name(stmt->user,
- "Cannot specify reserved role as mapping user.");
-
/* Check that the server exists. */
srv = GetForeignServerByName(stmt->servername, false);
@@ -1252,10 +1248,6 @@ AlterUserMapping(AlterUserMappingStmt *stmt)
else
useId = get_rolespec_oid(stmt->user, false);
- /* Additional check to protect reserved role names */
- check_rolespec_name(stmt->user,
- "Cannot alter reserved role mapping user.");
-
srv = GetForeignServerByName(stmt->servername, false);
umId = GetSysCacheOid2(USERMAPPINGUSERSERVER,
@@ -1345,11 +1337,6 @@ RemoveUserMapping(DropUserMappingStmt *stmt)
else
{
useId = get_rolespec_oid(stmt->user, stmt->missing_ok);
-
- /* Additional check to protect reserved role names */
- check_rolespec_name(stmt->user,
- "Cannot remove reserved role mapping user.");
-
if (!OidIsValid(useId))
{
/*
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 146b36c..93d15e4 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -176,13 +176,8 @@ policy_role_list_to_array(List *roles, int *num_roles)
return role_oids;
}
else
- {
- /* Additional check to protect reserved role names */
- check_rolespec_name((Node *) spec,
- "Cannot specify reserved role as policy target");
role_oids[i++] =
ObjectIdGetDatum(get_rolespec_oid((Node *) spec, false));
- }
}
return role_oids;
diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c
index dea3299..a60ceb8 100644
--- a/src/backend/commands/schemacmds.c
+++ b/src/backend/commands/schemacmds.c
@@ -65,10 +65,6 @@ CreateSchemaCommand(CreateSchemaStmt *stmt, const char *queryString)
else
owner_uid = saved_uid;
- /* Additional check to protect reserved role names */
- check_rolespec_name(stmt->authrole,
- "Cannot specify reserved role as owner.");
-
/* fill schema name with the user name if not specified */
if (!schemaName)
{
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 45a5144..86e9814 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3566,8 +3566,6 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
(List *) cmd->def, lockmode);
break;
case AT_ChangeOwner: /* ALTER OWNER */
- check_rolespec_name(cmd->newowner,
- "Cannot specify reserved role as owner.");
ATExecChangeOwner(RelationGetRelid(rel),
get_rolespec_oid(cmd->newowner, false),
false, lockmode);
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index fe7f253..7902d43 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -256,10 +256,6 @@ CreateTableSpace(CreateTableSpaceStmt *stmt)
else
ownerId = GetUserId();
- /* Additional check to protect reserved role names */
- check_rolespec_name(stmt->owner,
- "Cannot specify reserved role as owner.");
-
/* Unix-ify the offered path, and strip any trailing slashes */
location = pstrdup(stmt->location);
canonicalize_path(location);
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index cc3d564..f0ac636 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -1262,18 +1262,10 @@ GrantRole(GrantRoleStmt *stmt)
ListCell *item;
if (stmt->grantor)
- {
- check_rolespec_name(stmt->grantor,
- "Cannot specify reserved role as grantor.");
grantor = get_rolespec_oid(stmt->grantor, false);
- }
else
grantor = GetUserId();
- foreach(item, stmt->grantee_roles)
- check_rolespec_name(lfirst(item),
- "Cannot GRANT roles to a reserved role.");
-
grantee_ids = roleSpecsToIds(stmt->grantee_roles);
/* AccessShareLock is enough since we aren't modifying pg_authid */
@@ -1364,9 +1356,6 @@ ReassignOwnedObjects(ReassignOwnedStmt *stmt)
errmsg("permission denied to reassign objects")));
}
- check_rolespec_name(stmt->newrole,
- "Cannot specify reserved role as owner.");
-
/* Must have privileges on the receiving side too */
newrole = get_rolespec_oid(stmt->newrole, false);
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 05e59a6..f801faa 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -794,10 +794,6 @@ check_session_authorization(char **newval, void **extra, GucSource source)
return false;
}
- /* Do not allow setting role to a reserved role. */
- if (strncmp(*newval, "pg_", 3) == 0)
- return false;
-
/* Look up the username */
roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(*newval));
if (!HeapTupleIsValid(roleTup))
@@ -858,9 +854,6 @@ check_role(char **newval, void **extra, GucSource source)
roleid = InvalidOid;
is_superuser = false;
}
- /* Do not allow setting role to a reserved role. */
- else if (strncmp(*newval, "pg_", 3) == 0)
- return false;
else
{
if (!IsTransactionState())
diff --git a/src/test/regress/expected/rolenames.out b/src/test/regress/expected/rolenames.out
index 15a97ab..a1f0394 100644
--- a/src/test/regress/expected/rolenames.out
+++ b/src/test/regress/expected/rolenames.out
@@ -816,19 +816,11 @@ LINE 1: DROP USER MAPPING IF EXISTS FOR CURRENT_ROLE SERVER sv9;
DROP USER MAPPING IF EXISTS FOR nonexistent SERVER sv9; -- error
NOTICE: role "nonexistent" does not exist, skipping
-- GRANT/REVOKE
-GRANT testrol0 TO pg_abc; -- error
-ERROR: role "pg_abc" is reserved
-DETAIL: Cannot GRANT roles to a reserved role.
-GRANT pg_abc TO pg_abcdef; -- error
-ERROR: role "pg_abcdef" is reserved
-DETAIL: Cannot GRANT roles to a reserved role.
-SET ROLE pg_testrole; -- error
-ERROR: invalid value for parameter "role": "pg_testrole"
-SET ROLE pg_signal_backend; --error
-ERROR: invalid value for parameter "role": "pg_signal_backend"
-CREATE SCHEMA test_schema AUTHORIZATION pg_signal_backend; --error
-ERROR: role "pg_signal_backend" is reserved
-DETAIL: Cannot specify reserved role as owner.
+GRANT testrol0 TO pg_signal_backend; -- success
+SET ROLE pg_signal_backend; --success
+RESET ROLE;
+CREATE SCHEMA test_schema AUTHORIZATION pg_signal_backend; --success
+SET ROLE testrol2;
UPDATE pg_proc SET proacl = null WHERE proname LIKE 'testagg_';
SELECT proname, proacl FROM pg_proc WHERE proname LIKE 'testagg_';
proname | proacl
diff --git a/src/test/regress/sql/rolenames.sql b/src/test/regress/sql/rolenames.sql
index b58a163..6c831b8 100644
--- a/src/test/regress/sql/rolenames.sql
+++ b/src/test/regress/sql/rolenames.sql
@@ -381,12 +381,12 @@ DROP USER MAPPING IF EXISTS FOR CURRENT_ROLE SERVER sv9; --error
DROP USER MAPPING IF EXISTS FOR nonexistent SERVER sv9; -- error
-- GRANT/REVOKE
-GRANT testrol0 TO pg_abc; -- error
-GRANT pg_abc TO pg_abcdef; -- error
+GRANT testrol0 TO pg_signal_backend; -- success
-SET ROLE pg_testrole; -- error
-SET ROLE pg_signal_backend; --error
-CREATE SCHEMA test_schema AUTHORIZATION pg_signal_backend; --error
+SET ROLE pg_signal_backend; --success
+RESET ROLE;
+CREATE SCHEMA test_schema AUTHORIZATION pg_signal_backend; --success
+SET ROLE testrol2;
UPDATE pg_proc SET proacl = null WHERE proname LIKE 'testagg_';
SELECT proname, proacl FROM pg_proc WHERE proname LIKE 'testagg_';
--
2.5.0
signature.asc
Description: Digital signature
