Stephen,
We, as a community, have gotten flak from time-to-time about the
> superuser role. We also tend to wish to avoid unnecessary
> optimization as it complicates the code base and makes folks reviewing
> the code wonder at the exceptions.
>
I have attached a patch for consideration/discussion that addresses these
items. I have based it off of current master (0c013e0). I have done some
cursory testing including check-world with success.
> My 2c about this function is that it should be completely removed
> and the place where it's checked replaced with just the
> 'has_rolreplication' call and error. It's only called in one
> place and it'd be a simple one-liner anyway. As for
> has_rolreplication, I don't understand why it's in miscinit.c when
> the rest of the has_* set is in acl.c.
>
With all the other pg_has_* functions being found in acl.c I agree that it
would seem odd that this (or any other related function) would be found
elsewhere. Since aclchk.c also has a couple of functions that follow the
pattern of "has_<priv>_privilege", I moved this function there, for now, as
well as modified it to include superuser checks as they were not previously
there. The only related function I found in acl.c was "has_rolinherit"
which is a static function. :-/ There is also a static function
"has_rolcatupdate" in aclchk.c. I would agree that acl.c (or aclchk.c?)
would be a more appropriate location for these functions, though in some of
the above cases, it might require exposing them (I'm not sure that I see
any harm in doing so).
Perhaps refactoring to consolidate all of these in either acl.c or aclchk.c
with the following pattern might be a step in the right direction?
* has_createrole_privilege
* has_bypassrls_privilege
* has_inherit_privilege
* has_catupdate_privilege
* has_replication_privilege
* has_???_privilege
In either case, I think following a "convention" on the naming of these
functions (unless there is semantic reason not to) would also help to
reduce any confusion or head scratching.
Removing these design patterns may also help to avoid ending up with
> more of them in the future as folks copy and/or crib off of what we've
> already done to implement their features...
>
I agree.
-Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
new file mode 100644
index 133143d..695a13c
*** a/src/backend/access/transam/xlogfuncs.c
--- b/src/backend/access/transam/xlogfuncs.c
***************
*** 27,32 ****
--- 27,33 ----
#include "miscadmin.h"
#include "replication/walreceiver.h"
#include "storage/smgr.h"
+ #include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/numeric.h"
#include "utils/guc.h"
*************** pg_start_backup(PG_FUNCTION_ARGS)
*** 54,60 ****
backupidstr = text_to_cstring(backupid);
! if (!superuser() && !has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser or replication role to run a backup")));
--- 55,61 ----
backupidstr = text_to_cstring(backupid);
! if (!has_replication_privilege(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser or replication role to run a backup")));
*************** pg_stop_backup(PG_FUNCTION_ARGS)
*** 82,88 ****
{
XLogRecPtr stoppoint;
! if (!superuser() && !has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
(errmsg("must be superuser or replication role to run a backup"))));
--- 83,89 ----
{
XLogRecPtr stoppoint;
! if (!has_replication_privilege(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
(errmsg("must be superuser or replication role to run a backup"))));
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
new file mode 100644
index d30612c..a386fe2
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
*************** has_createrole_privilege(Oid roleid)
*** 5080,5085 ****
--- 5080,5107 ----
return result;
}
+ /*
+ * Check whether specified role has REPLICATION priviledge (or is a superuser)
+ */
+ bool
+ has_replication_privilege(Oid roleid)
+ {
+ bool result = false;
+ HeapTuple utup;
+
+ /* Superusers bypass all permission checking. */
+ if (superuser_arg(roleid))
+ return true;
+
+ utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
+ if (HeapTupleIsValid(utup))
+ {
+ result = ((Form_pg_authid) GETSTRUCT(utup))->rolreplication;
+ ReleaseSysCache(utup);
+ }
+ return result;
+ }
+
bool
has_bypassrls_privilege(Oid roleid)
{
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
new file mode 100644
index c9a9baf..ed89b23
*** a/src/backend/commands/alter.c
--- b/src/backend/commands/alter.c
*************** AlterObjectOwner_internal(Relation rel,
*** 807,852 ****
bool *nulls;
bool *replaces;
! /* Superusers can bypass permission checks */
! if (!superuser())
{
! AclObjectKind aclkind = get_object_aclkind(classId);
! /* must be owner */
! if (!has_privs_of_role(GetUserId(), old_ownerId))
{
! char *objname;
! char namebuf[NAMEDATALEN];
!
! if (Anum_name != InvalidAttrNumber)
! {
! datum = heap_getattr(oldtup, Anum_name,
! RelationGetDescr(rel), &isnull);
! Assert(!isnull);
! objname = NameStr(*DatumGetName(datum));
! }
! else
! {
! snprintf(namebuf, sizeof(namebuf), "%u",
! HeapTupleGetOid(oldtup));
! objname = namebuf;
! }
! aclcheck_error(ACLCHECK_NOT_OWNER, aclkind, objname);
}
! /* Must be able to become new owner */
! check_is_member_of_role(GetUserId(), new_ownerId);
!
! /* New owner must have CREATE privilege on namespace */
! if (OidIsValid(namespaceId))
{
! AclResult aclresult;
!
! aclresult = pg_namespace_aclcheck(namespaceId, new_ownerId,
! ACL_CREATE);
! if (aclresult != ACLCHECK_OK)
! aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
! get_namespace_name(namespaceId));
}
}
/* Build a modified tuple */
--- 807,848 ----
bool *nulls;
bool *replaces;
! AclObjectKind aclkind = get_object_aclkind(classId);
!
! /* must be owner */
! if (!has_privs_of_role(GetUserId(), old_ownerId))
{
! char *objname;
! char namebuf[NAMEDATALEN];
! if (Anum_name != InvalidAttrNumber)
{
! datum = heap_getattr(oldtup, Anum_name,
! RelationGetDescr(rel), &isnull);
! Assert(!isnull);
! objname = NameStr(*DatumGetName(datum));
}
! else
{
! snprintf(namebuf, sizeof(namebuf), "%u",
! HeapTupleGetOid(oldtup));
! objname = namebuf;
}
+ aclcheck_error(ACLCHECK_NOT_OWNER, aclkind, objname);
+ }
+ /* Must be able to become new owner */
+ check_is_member_of_role(GetUserId(), new_ownerId);
+
+ /* New owner must have CREATE privilege on namespace */
+ if (OidIsValid(namespaceId))
+ {
+ AclResult aclresult;
+
+ aclresult = pg_namespace_aclcheck(namespaceId, new_ownerId,
+ ACL_CREATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
+ get_namespace_name(namespaceId));
}
/* Build a modified tuple */
diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
new file mode 100644
index ab4ed6c..aad6ae4
*** a/src/backend/commands/foreigncmds.c
--- b/src/backend/commands/foreigncmds.c
*************** AlterForeignServerOwner_internal(Relatio
*** 332,361 ****
if (form->srvowner != newOwnerId)
{
! /* Superusers can always do it */
! if (!superuser())
! {
! Oid srvId;
! AclResult aclresult;
! srvId = HeapTupleGetOid(tup);
! /* Must be owner */
! if (!pg_foreign_server_ownercheck(srvId, GetUserId()))
! aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_FOREIGN_SERVER,
! NameStr(form->srvname));
! /* Must be able to become new owner */
! check_is_member_of_role(GetUserId(), newOwnerId);
! /* New owner must have USAGE privilege on foreign-data wrapper */
! aclresult = pg_foreign_data_wrapper_aclcheck(form->srvfdw, newOwnerId, ACL_USAGE);
! if (aclresult != ACLCHECK_OK)
! {
! ForeignDataWrapper *fdw = GetForeignDataWrapper(form->srvfdw);
! aclcheck_error(aclresult, ACL_KIND_FDW, fdw->fdwname);
! }
}
form->srvowner = newOwnerId;
--- 332,359 ----
if (form->srvowner != newOwnerId)
{
! Oid srvId;
! AclResult aclresult;
! srvId = HeapTupleGetOid(tup);
! /* Must be owner */
! if (!pg_foreign_server_ownercheck(srvId, GetUserId()))
! aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_FOREIGN_SERVER,
! NameStr(form->srvname));
! /* Must be able to become new owner */
! check_is_member_of_role(GetUserId(), newOwnerId);
! /* New owner must have USAGE privilege on foreign-data wrapper */
! aclresult = pg_foreign_data_wrapper_aclcheck(form->srvfdw, newOwnerId,
! ACL_USAGE);
! if (aclresult != ACLCHECK_OK)
! {
! ForeignDataWrapper *fdw = GetForeignDataWrapper(form->srvfdw);
!
! aclcheck_error(aclresult, ACL_KIND_FDW, fdw->fdwname);
}
form->srvowner = newOwnerId;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
new file mode 100644
index cb16c53..b3d864c
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** ATExecChangeOwner(Oid relationOid, Oid n
*** 8624,8650 ****
/* skip permission checks when recursing to index or toast table */
if (!recursing)
{
! /* Superusers can always do it */
! if (!superuser())
! {
! Oid namespaceOid = tuple_class->relnamespace;
! AclResult aclresult;
! /* Otherwise, must be owner of the existing object */
! if (!pg_class_ownercheck(relationOid, GetUserId()))
! aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
! RelationGetRelationName(target_rel));
! /* Must be able to become new owner */
! check_is_member_of_role(GetUserId(), newOwnerId);
! /* New owner must have CREATE privilege on namespace */
! aclresult = pg_namespace_aclcheck(namespaceOid, newOwnerId,
! ACL_CREATE);
! if (aclresult != ACLCHECK_OK)
! aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
! get_namespace_name(namespaceOid));
! }
}
memset(repl_null, false, sizeof(repl_null));
--- 8624,8646 ----
/* skip permission checks when recursing to index or toast table */
if (!recursing)
{
! Oid namespaceOid = tuple_class->relnamespace;
! AclResult aclresult;
! /* Must be owner of the existing object */
! if (!pg_class_ownercheck(relationOid, GetUserId()))
! aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
! RelationGetRelationName(target_rel));
! /* Must be able to become new owner */
! check_is_member_of_role(GetUserId(), newOwnerId);
! /* New owner must have CREATE privilege on namespace */
! aclresult = pg_namespace_aclcheck(namespaceOid, newOwnerId,
! ACL_CREATE);
! if (aclresult != ACLCHECK_OK)
! aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
! get_namespace_name(namespaceOid));
}
memset(repl_null, false, sizeof(repl_null));
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
new file mode 100644
index ad364ef..c154d21
*** a/src/backend/commands/typecmds.c
--- b/src/backend/commands/typecmds.c
*************** AlterTypeOwner(List *names, Oid newOwner
*** 3301,3324 ****
*/
if (typTup->typowner != newOwnerId)
{
! /* Superusers can always do it */
! if (!superuser())
! {
! /* Otherwise, must be owner of the existing object */
! if (!pg_type_ownercheck(HeapTupleGetOid(tup), GetUserId()))
! aclcheck_error_type(ACLCHECK_NOT_OWNER, HeapTupleGetOid(tup));
! /* Must be able to become new owner */
! check_is_member_of_role(GetUserId(), newOwnerId);
! /* New owner must have CREATE privilege on namespace */
! aclresult = pg_namespace_aclcheck(typTup->typnamespace,
! newOwnerId,
! ACL_CREATE);
! if (aclresult != ACLCHECK_OK)
! aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
! get_namespace_name(typTup->typnamespace));
! }
/*
* If it's a composite type, invoke ATExecChangeOwner so that we fix
--- 3301,3320 ----
*/
if (typTup->typowner != newOwnerId)
{
! /* Must be owner of the existing object */
! if (!pg_type_ownercheck(HeapTupleGetOid(tup), GetUserId()))
! aclcheck_error_type(ACLCHECK_NOT_OWNER, HeapTupleGetOid(tup));
! /* Must be able to become new owner */
! check_is_member_of_role(GetUserId(), newOwnerId);
! /* New owner must have CREATE privilege on namespace */
! aclresult = pg_namespace_aclcheck(typTup->typnamespace,
! newOwnerId,
! ACL_CREATE);
! if (aclresult != ACLCHECK_OK)
! aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
! get_namespace_name(typTup->typnamespace));
/*
* If it's a composite type, invoke ATExecChangeOwner so that we fix
diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
new file mode 100644
index 3a5ec2f..a7417c9
*** a/src/backend/replication/logical/logicalfuncs.c
--- b/src/backend/replication/logical/logicalfuncs.c
***************
*** 27,32 ****
--- 27,33 ----
#include "mb/pg_wchar.h"
+ #include "utils/acl.h"
#include "utils/array.h"
#include "utils/builtins.h"
#include "utils/inval.h"
*************** XLogRead(char *buf, TimeLineID tli, XLog
*** 200,214 ****
}
}
- static void
- check_permissions(void)
- {
- if (!superuser() && !has_rolreplication(GetUserId()))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- (errmsg("must be superuser or replication role to use replication slots"))));
- }
-
/*
* read_page callback for logical decoding contexts.
*
--- 201,206 ----
*************** pg_logical_slot_get_changes_guts(Functio
*** 322,328 ****
if (get_call_result_type(fcinfo, NULL, &p->tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
! check_permissions();
CheckLogicalDecodingRequirements();
--- 314,323 ----
if (get_call_result_type(fcinfo, NULL, &p->tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
! if (!has_replication_privilege(GetUserId()))
! ereport(ERROR,
! (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("must be superuser or replication role to use replication slots")));
CheckLogicalDecodingRequirements();
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
new file mode 100644
index bd4701f..ebae1a2
*** a/src/backend/replication/slotfuncs.c
--- b/src/backend/replication/slotfuncs.c
***************
*** 20,37 ****
#include "replication/slot.h"
#include "replication/logical.h"
#include "replication/logicalfuncs.h"
#include "utils/builtins.h"
#include "utils/pg_lsn.h"
- static void
- check_permissions(void)
- {
- if (!superuser() && !has_rolreplication(GetUserId()))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- (errmsg("must be superuser or replication role to use replication slots"))));
- }
-
/*
* SQL function for creating a new physical (streaming replication)
* replication slot.
--- 20,29 ----
#include "replication/slot.h"
#include "replication/logical.h"
#include "replication/logicalfuncs.h"
+ #include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/pg_lsn.h"
/*
* SQL function for creating a new physical (streaming replication)
* replication slot.
*************** pg_create_physical_replication_slot(PG_F
*** 51,57 ****
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
! check_permissions();
CheckSlotRequirements();
--- 43,52 ----
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
! if (!has_replication_privilege(GetUserId()))
! ereport(ERROR,
! (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("must be superuser or replication role to use replication slots")));
CheckSlotRequirements();
*************** pg_create_logical_replication_slot(PG_FU
*** 94,100 ****
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
! check_permissions();
CheckLogicalDecodingRequirements();
--- 89,98 ----
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
! if (!has_replication_privilege(GetUserId()))
! ereport(ERROR,
! (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("must be superuser or replication role to use replication slots")));
CheckLogicalDecodingRequirements();
*************** pg_drop_replication_slot(PG_FUNCTION_ARG
*** 143,149 ****
{
Name name = PG_GETARG_NAME(0);
! check_permissions();
CheckSlotRequirements();
--- 141,150 ----
{
Name name = PG_GETARG_NAME(0);
! if (!has_replication_privilege(GetUserId()))
! ereport(ERROR,
! (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("must be superuser or replication role to use replication slots")));
CheckSlotRequirements();
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
new file mode 100644
index a703c67..2e41dc5
*** a/src/backend/utils/init/miscinit.c
--- b/src/backend/utils/init/miscinit.c
*************** SetUserIdAndContext(Oid userid, bool sec
*** 317,341 ****
SecurityRestrictionContext &= ~SECURITY_LOCAL_USERID_CHANGE;
}
-
- /*
- * Check whether specified role has explicit REPLICATION privilege
- */
- bool
- has_rolreplication(Oid roleid)
- {
- bool result = false;
- HeapTuple utup;
-
- utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
- if (HeapTupleIsValid(utup))
- {
- result = ((Form_pg_authid) GETSTRUCT(utup))->rolreplication;
- ReleaseSysCache(utup);
- }
- return result;
- }
-
/*
* Initialize user identity during normal backend startup
*/
--- 317,322 ----
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
new file mode 100644
index 6a6a445..28c25f4
*** a/src/backend/utils/init/postinit.c
--- b/src/backend/utils/init/postinit.c
*************** InitPostgres(const char *in_dbname, Oid
*** 761,767 ****
{
Assert(!bootstrap);
! if (!superuser() && !has_rolreplication(GetUserId()))
ereport(FATAL,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser or replication role to start walsender")));
--- 761,767 ----
{
Assert(!bootstrap);
! if (!has_replication_privilege(GetUserId()))
ereport(FATAL,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser or replication role to start walsender")));
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
new file mode 100644
index 2ba9885..011bd62
*** a/src/include/miscadmin.h
--- b/src/include/miscadmin.h
*************** extern void ValidatePgVersion(const char
*** 434,440 ****
extern void process_shared_preload_libraries(void);
extern void process_session_preload_libraries(void);
extern void pg_bindtextdomain(const char *domain);
- extern bool has_rolreplication(Oid roleid);
/* in access/transam/xlog.c */
extern bool BackupInProgress(void);
--- 434,439 ----
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
new file mode 100644
index a8e3164..1c52fdc
*** a/src/include/utils/acl.h
--- b/src/include/utils/acl.h
*************** extern bool pg_event_trigger_ownercheck(
*** 328,332 ****
--- 328,333 ----
extern bool pg_extension_ownercheck(Oid ext_oid, Oid roleid);
extern bool has_createrole_privilege(Oid roleid);
extern bool has_bypassrls_privilege(Oid roleid);
+ extern bool has_replication_privilege(Oid roleid);
#endif /* ACL_H */
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
new file mode 100644
index e4dedb0..b3b5cd0
*** a/src/test/regress/expected/foreign_data.out
--- b/src/test/regress/expected/foreign_data.out
*************** ERROR: must be owner of foreign server
*** 394,399 ****
--- 394,400 ----
ALTER SERVER s1 OWNER TO regress_test_role; -- ERROR
ERROR: must be owner of foreign server s1
RESET ROLE;
+ GRANT USAGE ON FOREIGN DATA WRAPPER foo TO regress_test_role;
ALTER SERVER s1 OWNER TO regress_test_role;
GRANT regress_test_role2 TO regress_test_role;
SET ROLE regress_test_role;
*************** GRANT USAGE ON FOREIGN DATA WRAPPER foo
*** 417,422 ****
--- 418,424 ----
SET ROLE regress_test_role;
ALTER SERVER s1 OWNER TO regress_test_indirect;
RESET ROLE;
+ REVOKE USAGE ON FOREIGN DATA WRAPPER foo FROM regress_test_role;
DROP ROLE regress_test_indirect; -- ERROR
ERROR: role "regress_test_indirect" cannot be dropped because some objects depend on it
DETAIL: owner of server s1
diff --git a/src/test/regress/sql/foreign_data.sql b/src/test/regress/sql/foreign_data.sql
new file mode 100644
index de9dbc8..91d51c9
*** a/src/test/regress/sql/foreign_data.sql
--- b/src/test/regress/sql/foreign_data.sql
*************** SET ROLE regress_test_role;
*** 164,169 ****
--- 164,170 ----
ALTER SERVER s1 VERSION '1.1'; -- ERROR
ALTER SERVER s1 OWNER TO regress_test_role; -- ERROR
RESET ROLE;
+ GRANT USAGE ON FOREIGN DATA WRAPPER foo TO regress_test_role;
ALTER SERVER s1 OWNER TO regress_test_role;
GRANT regress_test_role2 TO regress_test_role;
SET ROLE regress_test_role;
*************** GRANT USAGE ON FOREIGN DATA WRAPPER foo
*** 183,188 ****
--- 184,190 ----
SET ROLE regress_test_role;
ALTER SERVER s1 OWNER TO regress_test_indirect;
RESET ROLE;
+ REVOKE USAGE ON FOREIGN DATA WRAPPER foo FROM regress_test_role;
DROP ROLE regress_test_indirect; -- ERROR
\des+
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
