All,
> It was brought up for discussion- see
>
> http://www.postgresql.org/message-id/[email protected]
>
> Specifically:
> ---------------
> One curious item to note is that the
> current if(!superuser()) {} block approach has masked an inconsistency
> in at least the FDW code which required a change to the regression
> tests- namely, we normally force FDW owners to have USAGE rights on
> the FDW, but that was being bypassed when a superuser makes the call.
> I seriously doubt that was intentional. I won't push back if someone
> really wants it to stay that way though.
> ---------------
>
> No one mentioned any concerns with it (and three people replied), so I'm
> inclined to think it's an agreeable change. Adam probably didn't
> mention it with this patch simply because it had already been brought
> up.
Yes, this is correct, I was under the impression that this has already been
addressed. Also, I thought it is a relatively benign change and perhaps
even one for the better. With that said, I'll certainly leave it as is if
that is the consensus.
> > Which makes me wonder whether the other changes are indeed without
> > effect or just not covered by tests.
> >
> > > * has_privilege-cleanup_11-5-2014.patch
> >
> > I don't really see the merit of this patch. A "cleanup" patch that adds
> > more code than it removes isn't really a cleanup. If you want to make
> > the APIs consistent, then let's make a patch for that. If you want to
> > make the error messages consistent, then let's make a patch for that.
>
Fair enough. I think we all agree that fixing the superuser shortcuts are
a relevant and welcome change (at least that is the sense I get). So, how
about for the time being, we table the "consistent API and error messages"
and focus solely on the shortcuts? If that is favorable, then I have
attached a patch to address those changes.
> There is other work going on replacing these role attributes with
> > something more general, so maybe this cleanup should be done as part of
> > that other work.
>
I agree and given the work that has already been done toward that effort I
think that would perhaps be the better approach to addressing them.
Perhaps 'cleanup' is just an overloaded term. The patch is to make the
> APIs and the error messages consistent. I'm amazed at how much
> discussion and work is going into these exceptionally minor changes
> which have been already broken out of the larger and far more
> interesting work (imv anyway). Having two patches, one to simply move
> the checks around and then another to make the error messages in those
> checks consistent, which will naturally end up depending on each other,
> strikes me as overkill, but we can certainly do it.
>
Agreed, but will certainly do whatever is necessary to keep these changes
moving forward. Though, I think the attached patch mitigates any need to
break these changes out further.
Andres raised a concern about the specific error message wording (which
> was intended to just make it more consistent with the rest of our
> permission-check error messages..), are there any other opinions on the
> wording? Would be great to get feedback on that..
>
Agreed, I would certainly be willing to move these changes forward as a
separate effort, but without more specific recommendations beyond what has
already been discussed and proposed I'm at a bit of a loss on where to take
it. I'm all ears on this one and would certainly appreciate any feedback
Thanks,
Adam
--
Adam Brightwell - [email protected]
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..510caf6
*** a/src/backend/access/transam/xlogfuncs.c
--- b/src/backend/access/transam/xlogfuncs.c
*************** 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")));
--- 54,60 ----
backupidstr = text_to_cstring(backupid);
! if (!has_rolreplication(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"))));
--- 82,88 ----
{
XLogRecPtr stoppoint;
! if (!has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
(errmsg("must be superuser or replication role to run a backup"))));
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
new file mode 100644
index c9a9baf..3723066
*** a/src/backend/commands/alter.c
--- b/src/backend/commands/alter.c
*************** AlterObjectOwner_internal(Relation rel,
*** 806,852 ****
Datum *values;
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 */
--- 806,847 ----
Datum *values;
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 5629455..c0d4d4e
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** ATExecChangeOwner(Oid relationOid, Oid n
*** 8618,8644 ****
/* 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));
--- 8618,8640 ----
/* 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 cbb65f8..b5904a8
*** a/src/backend/commands/typecmds.c
--- b/src/backend/commands/typecmds.c
*************** AlterTypeOwner(List *names, Oid newOwner
*** 3348,3371 ****
*/
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
--- 3348,3367 ----
*/
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 1977f09..1941c42
*** a/src/backend/replication/logical/logicalfuncs.c
--- b/src/backend/replication/logical/logicalfuncs.c
*************** XLogRead(char *buf, TimeLineID tli, XLog
*** 205,211 ****
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"))));
--- 205,211 ----
static void
check_permissions(void)
{
! if (!has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
(errmsg("must be superuser or replication role to use replication slots"))));
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
new file mode 100644
index bd4701f..7211161
*** a/src/backend/replication/slotfuncs.c
--- b/src/backend/replication/slotfuncs.c
***************
*** 26,32 ****
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"))));
--- 26,32 ----
static void
check_permissions(void)
{
! if (!has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
(errmsg("must be superuser or replication role to use replication slots"))));
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
new file mode 100644
index 8fccb4c..679396d
*** a/src/backend/utils/init/miscinit.c
--- b/src/backend/utils/init/miscinit.c
*************** has_rolreplication(Oid roleid)
*** 337,342 ****
--- 337,346 ----
bool result = false;
HeapTuple utup;
+ /* Superusers bypass all permission checking. */
+ if (superuser_arg(roleid))
+ return true;
+
utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
if (HeapTupleIsValid(utup))
{
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
new file mode 100644
index c348034..ec0ac6a
*** a/src/backend/utils/init/postinit.c
--- b/src/backend/utils/init/postinit.c
*************** InitPostgres(const char *in_dbname, Oid
*** 762,768 ****
{
Assert(!bootstrap);
! if (!superuser() && !has_rolreplication(GetUserId()))
ereport(FATAL,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser or replication role to start walsender")));
--- 762,768 ----
{
Assert(!bootstrap);
! if (!has_rolreplication(GetUserId()))
ereport(FATAL,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser or replication role to start walsender")));
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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
