pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #15833: defining a comment on a domain constraint fails with wrong OID
11+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2019-06-12 ceac4505d3 fa5f3a4bcc 56a932533a ff1a25601e 13f4b462db Fix handling of COMMENT for domain constraints
* BUG #15833: defining a comment on a domain constraint fails with wrong OID
@ 2019-06-05 10:50 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 11+ messages in thread
From: PG Bug reporting form @ 2019-06-05 10:50 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: clemens@ladisch.de
The following bug has been logged on the website:
Bug reference: 15833
Logged by: Clemens Ladisch
Email address: clemens@ladisch.de
PostgreSQL version: 10.8
Operating system: Windows 64-bit
Description:
postgres=> CREATE DOMAIN ddd AS text CONSTRAINT ccc CHECK (TRUE);
CREATE DOMAIN
postgres=> COMMENT ON CONSTRAINT ccc ON DOMAIN ddd IS 'test';
ERROR: 42704: type with OID 444275 does not exist
LOCATION: pg_type_ownercheck, aclchk.c:4585
And there is indeed no type with that OID:
postgres=> SELECT 'type', oid FROM pg_type WHERE typname = 'ddd' UNION ALL
SELECT 'constraint', oid FROM pg_constraint WHERE conname = 'ccc';
?column? | oid
------------+--------
type | 444274
constraint | 444275
(2 rows)
Same with Postgres 9.6 on SQLFiddle:
<http://www.sqlfiddle.com/#!17/a63b6/2;
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #15833: defining a comment on a domain constraint fails with wrong OID
@ 2019-06-05 18:15 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 11+ messages in thread
From: Alvaro Herrera @ 2019-06-05 18:15 UTC (permalink / raw)
To: clemens@ladisch.de; pgsql-bugs@lists.postgresql.org
On 2019-Jun-05, PG Bug reporting form wrote:
> postgres=> CREATE DOMAIN ddd AS text CONSTRAINT ccc CHECK (TRUE);
> CREATE DOMAIN
> postgres=> COMMENT ON CONSTRAINT ccc ON DOMAIN ddd IS 'test';
> ERROR: 42704: type with OID 444275 does not exist
> LOCATION: pg_type_ownercheck, aclchk.c:4585
Confirmed. It works for superusers, which explains why the existing
regression tests pass -- and that's because check_object_ownership()
(which is handing the OBJECT_DOMCONSTRAINT case wrongly) is bypassed for
superusers. Annoyingly, get_object_address does not return the type's
OID, only the domain's.
I'm surprised that this has been broken for so long with no reports ...
I broke it in 7eca575d1c28 (December 2014).
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #15833: defining a comment on a domain constraint fails with wrong OID
@ 2019-06-07 05:42 Michael Paquier <michael@paquier.xyz>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 11+ messages in thread
From: Michael Paquier @ 2019-06-07 05:42 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: clemens@ladisch.de; pgsql-bugs@lists.postgresql.org
On Wed, Jun 05, 2019 at 02:15:02PM -0400, Alvaro Herrera wrote:
> Confirmed. It works for superusers, which explains why the existing
> regression tests pass -- and that's because check_object_ownership()
> (which is handing the OBJECT_DOMCONSTRAINT case wrongly) is bypassed for
> superusers. Annoyingly, get_object_address does not return the type's
> OID, only the domain's.
Well, it wouldn't be a problem to do a syscache lookup and then use
the type from contypid, no? It seems to me that it would be more
consistent to just add a pg_domain_constraint_ownercheck() in aclchk.c
as all the syscache lookups happen their for all the other objects
types. What do you think?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #15833: defining a comment on a domain constraint fails with wrong OID
@ 2019-06-10 06:28 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 11+ messages in thread
From: Michael Paquier @ 2019-06-10 06:28 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: clemens@ladisch.de; pgsql-bugs@lists.postgresql.org
On Fri, Jun 07, 2019 at 02:42:33PM +0900, Michael Paquier wrote:
> Well, it wouldn't be a problem to do a syscache lookup and then use
> the type from contypid, no? It seems to me that it would be more
> consistent to just add a pg_domain_constraint_ownercheck() in aclchk.c
> as all the syscache lookups happen their for all the other objects
> types. What do you think?
Attached is what I have in mind. There are already tests at the
bottom of constraints.source checking for comments on both table and
domain constraints, so my proposal is to run them with a dedicated
role. What do you think?
--
Michael
Attachments:
dom-constraint-comments-v1.patch (text/x-diff, 4.9K) download
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 2797af35c3..a155b76276 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -33,6 +33,7 @@
#include "catalog/pg_authid.h"
#include "catalog/pg_cast.h"
#include "catalog/pg_collation.h"
+#include "catalog/pg_constraint.h"
#include "catalog/pg_conversion.h"
#include "catalog/pg_database.h"
#include "catalog/pg_default_acl.h"
@@ -4800,6 +4801,38 @@ pg_type_ownercheck(Oid type_oid, Oid roleid)
return has_privs_of_role(roleid, ownerId);
}
+/*
+ * Ownership check for a domain constraint (specified by OID).
+ *
+ * Note that this relies on the ownership of the associated type.
+ */
+bool
+pg_domain_constraint_ownercheck(Oid con_oid, Oid roleid)
+{
+ HeapTuple tuple;
+ Oid typeId;
+
+ /* Superusers bypass all permission checking. */
+ if (superuser_arg(roleid))
+ return true;
+
+ tuple = SearchSysCache1(CONSTROID, ObjectIdGetDatum(con_oid));
+ if (!HeapTupleIsValid(tuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("constraint with OID %u does not exist", con_oid)));
+
+ typeId = ((Form_pg_constraint) GETSTRUCT(tuple))->contypid;
+
+ ReleaseSysCache(tuple);
+
+ /*
+ * Fallback to type ownership check in this case as this is what domain
+ * constraints rely on.
+ */
+ return pg_type_ownercheck(typeId, roleid);
+}
+
/*
* Ownership check for an operator (specified by OID).
*/
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 2235e5626f..427f954629 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2295,10 +2295,13 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address,
case OBJECT_TYPE:
case OBJECT_DOMAIN:
case OBJECT_ATTRIBUTE:
- case OBJECT_DOMCONSTRAINT:
if (!pg_type_ownercheck(address.objectId, roleid))
aclcheck_error_type(ACLCHECK_NOT_OWNER, address.objectId);
break;
+ case OBJECT_DOMCONSTRAINT:
+ if (!pg_domain_constraint_ownercheck(address.objectId, roleid))
+ aclcheck_error_type(ACLCHECK_NOT_OWNER, address.objectId);
+ break;
case OBJECT_AGGREGATE:
case OBJECT_FUNCTION:
case OBJECT_PROCEDURE:
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index f4c160ee72..9adfd72c09 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -287,6 +287,7 @@ extern void removeExtObjInitPriv(Oid objoid, Oid classoid);
/* ownercheck routines just return true (owner) or false (not) */
extern bool pg_class_ownercheck(Oid class_oid, Oid roleid);
extern bool pg_type_ownercheck(Oid type_oid, Oid roleid);
+extern bool pg_domain_constraint_ownercheck(Oid type_oid, Oid roleid);
extern bool pg_oper_ownercheck(Oid oper_oid, Oid roleid);
extern bool pg_proc_ownercheck(Oid proc_oid, Oid roleid);
extern bool pg_language_ownercheck(Oid lan_oid, Oid roleid);
diff --git a/src/test/regress/input/constraints.source b/src/test/regress/input/constraints.source
index 0abf95577e..111e65475f 100644
--- a/src/test/regress/input/constraints.source
+++ b/src/test/regress/input/constraints.source
@@ -518,6 +518,10 @@ ALTER TABLE deferred_excl ADD EXCLUDE (f1 WITH =);
DROP TABLE deferred_excl;
-- Comments
+-- Setup a low-level role to enforce non-superuser checks.
+CREATE ROLE regress_constraint_comments;
+SET SESSION AUTHORIZATION regress_constraint_comments;
+
CREATE TABLE constraint_comments_tbl (a int CONSTRAINT the_constraint CHECK (a > 0));
CREATE DOMAIN constraint_comments_dom AS int CONSTRAINT the_constraint CHECK (value > 0);
@@ -537,3 +541,5 @@ COMMENT ON CONSTRAINT the_constraint ON DOMAIN constraint_comments_dom IS NULL;
DROP TABLE constraint_comments_tbl;
DROP DOMAIN constraint_comments_dom;
+RESET SESSION AUTHORIZATION;
+DROP ROLE regress_constraint_comments;
diff --git a/src/test/regress/output/constraints.source b/src/test/regress/output/constraints.source
index 465d6da0e0..6313a4d71e 100644
--- a/src/test/regress/output/constraints.source
+++ b/src/test/regress/output/constraints.source
@@ -704,6 +704,9 @@ ERROR: could not create exclusion constraint "deferred_excl_f1_excl"
DETAIL: Key (f1)=(3) conflicts with key (f1)=(3).
DROP TABLE deferred_excl;
-- Comments
+-- Setup a low-level role to enforce non-superuser checks.
+CREATE ROLE regress_constraint_comments;
+SET SESSION AUTHORIZATION regress_constraint_comments;
CREATE TABLE constraint_comments_tbl (a int CONSTRAINT the_constraint CHECK (a > 0));
CREATE DOMAIN constraint_comments_dom AS int CONSTRAINT the_constraint CHECK (value > 0);
COMMENT ON CONSTRAINT the_constraint ON constraint_comments_tbl IS 'yes, the comment';
@@ -722,3 +725,5 @@ COMMENT ON CONSTRAINT the_constraint ON constraint_comments_tbl IS NULL;
COMMENT ON CONSTRAINT the_constraint ON DOMAIN constraint_comments_dom IS NULL;
DROP TABLE constraint_comments_tbl;
DROP DOMAIN constraint_comments_dom;
+RESET SESSION AUTHORIZATION;
+DROP ROLE regress_constraint_comments;
* Re: BUG #15833: defining a comment on a domain constraint fails with wrong OID
@ 2019-06-10 10:25 Daniel Gustafsson <daniel@yesql.se>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 11+ messages in thread
From: Daniel Gustafsson @ 2019-06-10 10:25 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; clemens@ladisch.de, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
> On 10 Jun 2019, at 08:28, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Jun 07, 2019 at 02:42:33PM +0900, Michael Paquier wrote:
>> Well, it wouldn't be a problem to do a syscache lookup and then use
>> the type from contypid, no? It seems to me that it would be more
>> consistent to just add a pg_domain_constraint_ownercheck() in aclchk.c
>> as all the syscache lookups happen their for all the other objects
>> types. What do you think?
>
> Attached is what I have in mind. There are already tests at the
> bottom of constraints.source checking for comments on both table and
> domain constraints, so my proposal is to run them with a dedicated
> role. What do you think?
+1 on the approach of the patch, it seems like the simplest approach. A
comment on the check_object_ownership() diff though:
+ if (!pg_domain_constraint_ownercheck(address.objectId, roleid))
+ aclcheck_error_type(ACLCHECK_NOT_OWNER, address.objectId);
This doesn’t work for the errorcase as the address.objectId is the wrong Oid
here as well, the contypid extracted in pg_domain_constraint_ownercheck() is
required. I’ve hacked up your patch to pass it back and that seems to work,
and also added a test for the errorpath. Another option would be to provide a
new aclcheck_error_domain_constraint(), not sure which is best.
cheers ./daniel
Attachments:
dom-constraint-comments-v2.patch (application/octet-stream, 6.8K) download
From 69854257d54962ebc2fef1abcab9edd4b9921fd4 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Mon, 10 Jun 2019 11:47:35 +0200
Subject: [PATCH] dom-constraint-comments-v2.patch
https://postgr.es/m/0190610062855.GE2199@paquier.xyz
---
src/backend/catalog/aclchk.c | 34 ++++++++++++++++++++++++++++++
src/backend/catalog/objectaddress.c | 9 +++++++-
src/include/utils/acl.h | 1 +
src/test/regress/input/constraints.source | 15 +++++++++++++
src/test/regress/output/constraints.source | 14 ++++++++++++
5 files changed, 72 insertions(+), 1 deletion(-)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 2797af35c3..54753a6499 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -33,6 +33,7 @@
#include "catalog/pg_authid.h"
#include "catalog/pg_cast.h"
#include "catalog/pg_collation.h"
+#include "catalog/pg_constraint.h"
#include "catalog/pg_conversion.h"
#include "catalog/pg_database.h"
#include "catalog/pg_default_acl.h"
@@ -4800,6 +4801,39 @@ pg_type_ownercheck(Oid type_oid, Oid roleid)
return has_privs_of_role(roleid, ownerId);
}
+/*
+ * Ownership check for a domain constraint (specified by OID).
+ *
+ * Note that this relies on the ownership of the associated type.
+ */
+bool
+pg_domain_constraint_ownercheck(Oid con_oid, Oid roleid, Oid *contypid)
+{
+ HeapTuple tuple;
+ Oid typeId;
+
+ /* Superusers bypass all permission checking. */
+ if (superuser_arg(roleid))
+ return true;
+
+ tuple = SearchSysCache1(CONSTROID, ObjectIdGetDatum(con_oid));
+ if (!HeapTupleIsValid(tuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("constraint with OID %u does not exist", con_oid)));
+
+ typeId = ((Form_pg_constraint) GETSTRUCT(tuple))->contypid;
+ *contypid = typeId;
+
+ ReleaseSysCache(tuple);
+
+ /*
+ * Fallback to type ownership check in this case as this is what domain
+ * constraints rely on.
+ */
+ return pg_type_ownercheck(typeId, roleid);
+}
+
/*
* Ownership check for an operator (specified by OID).
*/
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 2235e5626f..b932fd462b 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2295,10 +2295,17 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address,
case OBJECT_TYPE:
case OBJECT_DOMAIN:
case OBJECT_ATTRIBUTE:
- case OBJECT_DOMCONSTRAINT:
if (!pg_type_ownercheck(address.objectId, roleid))
aclcheck_error_type(ACLCHECK_NOT_OWNER, address.objectId);
break;
+ case OBJECT_DOMCONSTRAINT:
+ {
+ Oid contypid;
+
+ if (!pg_domain_constraint_ownercheck(address.objectId, roleid, &contypid))
+ aclcheck_error_type(ACLCHECK_NOT_OWNER, contypid);
+ }
+ break;
case OBJECT_AGGREGATE:
case OBJECT_FUNCTION:
case OBJECT_PROCEDURE:
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index f4c160ee72..d8ff0cb515 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -287,6 +287,7 @@ extern void removeExtObjInitPriv(Oid objoid, Oid classoid);
/* ownercheck routines just return true (owner) or false (not) */
extern bool pg_class_ownercheck(Oid class_oid, Oid roleid);
extern bool pg_type_ownercheck(Oid type_oid, Oid roleid);
+extern bool pg_domain_constraint_ownercheck(Oid type_oid, Oid roleid, Oid *contypid);
extern bool pg_oper_ownercheck(Oid oper_oid, Oid roleid);
extern bool pg_proc_ownercheck(Oid proc_oid, Oid roleid);
extern bool pg_language_ownercheck(Oid lan_oid, Oid roleid);
diff --git a/src/test/regress/input/constraints.source b/src/test/regress/input/constraints.source
index 0abf95577e..0d4c5b882d 100644
--- a/src/test/regress/input/constraints.source
+++ b/src/test/regress/input/constraints.source
@@ -518,6 +518,10 @@ ALTER TABLE deferred_excl ADD EXCLUDE (f1 WITH =);
DROP TABLE deferred_excl;
-- Comments
+-- Setup a low-level role to enforce non-superuser checks.
+CREATE ROLE regress_constraint_comments;
+SET SESSION AUTHORIZATION regress_constraint_comments;
+
CREATE TABLE constraint_comments_tbl (a int CONSTRAINT the_constraint CHECK (a > 0));
CREATE DOMAIN constraint_comments_dom AS int CONSTRAINT the_constraint CHECK (value > 0);
@@ -535,5 +539,16 @@ COMMENT ON CONSTRAINT the_constraint ON DOMAIN no_comments_dom IS 'another bad c
COMMENT ON CONSTRAINT the_constraint ON constraint_comments_tbl IS NULL;
COMMENT ON CONSTRAINT the_constraint ON DOMAIN constraint_comments_dom IS NULL;
+-- unathorized user
+RESET SESSION AUTHORIZATION;
+CREATE ROLE regress_constraint_comments_noaccess;
+SET SESSION AUTHORIZATION regress_constraint_comments_noaccess;
+COMMENT ON CONSTRAINT the_constraint ON constraint_comments_tbl IS 'no, the comment';
+COMMENT ON CONSTRAINT the_constraint ON DOMAIN constraint_comments_dom IS 'no, another comment';
+RESET SESSION AUTHORIZATION;
+
DROP TABLE constraint_comments_tbl;
DROP DOMAIN constraint_comments_dom;
+
+DROP ROLE regress_constraint_comments;
+DROP ROLE regress_constraint_comments_noaccess;
diff --git a/src/test/regress/output/constraints.source b/src/test/regress/output/constraints.source
index 465d6da0e0..be151b7d60 100644
--- a/src/test/regress/output/constraints.source
+++ b/src/test/regress/output/constraints.source
@@ -704,6 +704,9 @@ ERROR: could not create exclusion constraint "deferred_excl_f1_excl"
DETAIL: Key (f1)=(3) conflicts with key (f1)=(3).
DROP TABLE deferred_excl;
-- Comments
+-- Setup a low-level role to enforce non-superuser checks.
+CREATE ROLE regress_constraint_comments;
+SET SESSION AUTHORIZATION regress_constraint_comments;
CREATE TABLE constraint_comments_tbl (a int CONSTRAINT the_constraint CHECK (a > 0));
CREATE DOMAIN constraint_comments_dom AS int CONSTRAINT the_constraint CHECK (value > 0);
COMMENT ON CONSTRAINT the_constraint ON constraint_comments_tbl IS 'yes, the comment';
@@ -720,5 +723,16 @@ COMMENT ON CONSTRAINT the_constraint ON DOMAIN no_comments_dom IS 'another bad c
ERROR: type "no_comments_dom" does not exist
COMMENT ON CONSTRAINT the_constraint ON constraint_comments_tbl IS NULL;
COMMENT ON CONSTRAINT the_constraint ON DOMAIN constraint_comments_dom IS NULL;
+-- unathorized user
+RESET SESSION AUTHORIZATION;
+CREATE ROLE regress_constraint_comments_noaccess;
+SET SESSION AUTHORIZATION regress_constraint_comments_noaccess;
+COMMENT ON CONSTRAINT the_constraint ON constraint_comments_tbl IS 'no, the comment';
+ERROR: must be owner of relation constraint_comments_tbl
+COMMENT ON CONSTRAINT the_constraint ON DOMAIN constraint_comments_dom IS 'no, another comment';
+ERROR: must be owner of type constraint_comments_dom
+RESET SESSION AUTHORIZATION;
DROP TABLE constraint_comments_tbl;
DROP DOMAIN constraint_comments_dom;
+DROP ROLE regress_constraint_comments;
+DROP ROLE regress_constraint_comments_noaccess;
--
2.14.1.145.gb3622a4ee
=
* Re: BUG #15833: defining a comment on a domain constraint fails with wrong OID
@ 2019-06-10 12:55 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 11+ messages in thread
From: Alvaro Herrera @ 2019-06-10 12:55 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Michael Paquier <michael@paquier.xyz>; clemens@ladisch.de, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On 2019-Jun-10, Daniel Gustafsson wrote:
> +1 on the approach of the patch, it seems like the simplest approach. A
> comment on the check_object_ownership() diff though:
>
> + if (!pg_domain_constraint_ownercheck(address.objectId, roleid))
> + aclcheck_error_type(ACLCHECK_NOT_OWNER, address.objectId);
>
> This doesn’t work for the errorcase as the address.objectId is the wrong Oid
> here as well, the contypid extracted in pg_domain_constraint_ownercheck() is
> required. I’ve hacked up your patch to pass it back and that seems to work,
-1 on this approach. Having this ownercheck function return the owning
object ID seems way too strange. I'd rather not have the new ownercheck
function, and instead do a syscache search to obtain the type OID in
check_object_ownership, then do pg_type_ownercheck. I'm not even sure
that pg_domain_constraint_ownercheck makes a lot of sense in itself,
since it's never the constraint that requires an owner check.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #15833: defining a comment on a domain constraint fails with wrong OID
@ 2019-06-11 04:55 Michael Paquier <michael@paquier.xyz>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 11+ messages in thread
From: Michael Paquier @ 2019-06-11 04:55 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Daniel Gustafsson <daniel@yesql.se>; clemens@ladisch.de, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On Mon, Jun 10, 2019 at 08:55:27AM -0400, Alvaro Herrera wrote:
> -1 on this approach. Having this ownercheck function return the owning
> object ID seems way too strange. I'd rather not have the new ownercheck
> function, and instead do a syscache search to obtain the type OID in
> check_object_ownership, then do pg_type_ownercheck. I'm not even sure
> that pg_domain_constraint_ownercheck makes a lot of sense in itself,
> since it's never the constraint that requires an owner check.
I can see your point, yes perhaps I overdid it. What do you think
about the attached instead? This moves the syscache lookup directly
into check_object_ownership() as you suggest.
--
Michael
Attachments:
dom-constraint-comments-v3.patch (text/x-diff, 4.5K) download
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 2235e5626f..2207daa5ed 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2295,10 +2295,35 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address,
case OBJECT_TYPE:
case OBJECT_DOMAIN:
case OBJECT_ATTRIBUTE:
- case OBJECT_DOMCONSTRAINT:
if (!pg_type_ownercheck(address.objectId, roleid))
aclcheck_error_type(ACLCHECK_NOT_OWNER, address.objectId);
break;
+ case OBJECT_DOMCONSTRAINT:
+ {
+ HeapTuple tuple;
+ Oid contypid;
+ Oid conoid;
+
+ conoid = address.objectId;
+
+ tuple = SearchSysCache1(CONSTROID, ObjectIdGetDatum(conoid));
+ if (!HeapTupleIsValid(tuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("constraint with OID %u does not exist", conoid)));
+
+ contypid = ((Form_pg_constraint) GETSTRUCT(tuple))->contypid;
+
+ ReleaseSysCache(tuple);
+
+ /*
+ * Fallback to type ownership check in this case as this is
+ * what domain constraints rely on.
+ */
+ if (!pg_type_ownercheck(contypid, roleid))
+ aclcheck_error_type(ACLCHECK_NOT_OWNER, contypid);
+ }
+ break;
case OBJECT_AGGREGATE:
case OBJECT_FUNCTION:
case OBJECT_PROCEDURE:
diff --git a/src/test/regress/input/constraints.source b/src/test/regress/input/constraints.source
index 0abf95577e..0d4c5b882d 100644
--- a/src/test/regress/input/constraints.source
+++ b/src/test/regress/input/constraints.source
@@ -518,6 +518,10 @@ ALTER TABLE deferred_excl ADD EXCLUDE (f1 WITH =);
DROP TABLE deferred_excl;
-- Comments
+-- Setup a low-level role to enforce non-superuser checks.
+CREATE ROLE regress_constraint_comments;
+SET SESSION AUTHORIZATION regress_constraint_comments;
+
CREATE TABLE constraint_comments_tbl (a int CONSTRAINT the_constraint CHECK (a > 0));
CREATE DOMAIN constraint_comments_dom AS int CONSTRAINT the_constraint CHECK (value > 0);
@@ -535,5 +539,16 @@ COMMENT ON CONSTRAINT the_constraint ON DOMAIN no_comments_dom IS 'another bad c
COMMENT ON CONSTRAINT the_constraint ON constraint_comments_tbl IS NULL;
COMMENT ON CONSTRAINT the_constraint ON DOMAIN constraint_comments_dom IS NULL;
+-- unathorized user
+RESET SESSION AUTHORIZATION;
+CREATE ROLE regress_constraint_comments_noaccess;
+SET SESSION AUTHORIZATION regress_constraint_comments_noaccess;
+COMMENT ON CONSTRAINT the_constraint ON constraint_comments_tbl IS 'no, the comment';
+COMMENT ON CONSTRAINT the_constraint ON DOMAIN constraint_comments_dom IS 'no, another comment';
+RESET SESSION AUTHORIZATION;
+
DROP TABLE constraint_comments_tbl;
DROP DOMAIN constraint_comments_dom;
+
+DROP ROLE regress_constraint_comments;
+DROP ROLE regress_constraint_comments_noaccess;
diff --git a/src/test/regress/output/constraints.source b/src/test/regress/output/constraints.source
index 465d6da0e0..be151b7d60 100644
--- a/src/test/regress/output/constraints.source
+++ b/src/test/regress/output/constraints.source
@@ -704,6 +704,9 @@ ERROR: could not create exclusion constraint "deferred_excl_f1_excl"
DETAIL: Key (f1)=(3) conflicts with key (f1)=(3).
DROP TABLE deferred_excl;
-- Comments
+-- Setup a low-level role to enforce non-superuser checks.
+CREATE ROLE regress_constraint_comments;
+SET SESSION AUTHORIZATION regress_constraint_comments;
CREATE TABLE constraint_comments_tbl (a int CONSTRAINT the_constraint CHECK (a > 0));
CREATE DOMAIN constraint_comments_dom AS int CONSTRAINT the_constraint CHECK (value > 0);
COMMENT ON CONSTRAINT the_constraint ON constraint_comments_tbl IS 'yes, the comment';
@@ -720,5 +723,16 @@ COMMENT ON CONSTRAINT the_constraint ON DOMAIN no_comments_dom IS 'another bad c
ERROR: type "no_comments_dom" does not exist
COMMENT ON CONSTRAINT the_constraint ON constraint_comments_tbl IS NULL;
COMMENT ON CONSTRAINT the_constraint ON DOMAIN constraint_comments_dom IS NULL;
+-- unathorized user
+RESET SESSION AUTHORIZATION;
+CREATE ROLE regress_constraint_comments_noaccess;
+SET SESSION AUTHORIZATION regress_constraint_comments_noaccess;
+COMMENT ON CONSTRAINT the_constraint ON constraint_comments_tbl IS 'no, the comment';
+ERROR: must be owner of relation constraint_comments_tbl
+COMMENT ON CONSTRAINT the_constraint ON DOMAIN constraint_comments_dom IS 'no, another comment';
+ERROR: must be owner of type constraint_comments_dom
+RESET SESSION AUTHORIZATION;
DROP TABLE constraint_comments_tbl;
DROP DOMAIN constraint_comments_dom;
+DROP ROLE regress_constraint_comments;
+DROP ROLE regress_constraint_comments_noaccess;
* Re: BUG #15833: defining a comment on a domain constraint fails with wrong OID
@ 2019-06-11 13:32 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 11+ messages in thread
From: Alvaro Herrera @ 2019-06-11 13:32 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Daniel Gustafsson <daniel@yesql.se>; clemens@ladisch.de, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On 2019-Jun-11, Michael Paquier wrote:
> On Mon, Jun 10, 2019 at 08:55:27AM -0400, Alvaro Herrera wrote:
> > -1 on this approach. Having this ownercheck function return the owning
> > object ID seems way too strange. I'd rather not have the new ownercheck
> > function, and instead do a syscache search to obtain the type OID in
> > check_object_ownership, then do pg_type_ownercheck. I'm not even sure
> > that pg_domain_constraint_ownercheck makes a lot of sense in itself,
> > since it's never the constraint that requires an owner check.
>
> I can see your point, yes perhaps I overdid it. What do you think
> about the attached instead? This moves the syscache lookup directly
> into check_object_ownership() as you suggest.
Yeah, looks better. I think the error message should be a normal elog()
cache failure, though ... at least in the COMMENT case, the obj-does-not-
exist message is supposed to be thrown by get_object_address(), before
check_object_ownership is called.
As a matter of style, I would get rid of the 'conoid' variable and just
use address.objectId where needed.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #15833: defining a comment on a domain constraint fails with wrong OID
@ 2019-06-12 02:36 Michael Paquier <michael@paquier.xyz>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 2 replies; 11+ messages in thread
From: Michael Paquier @ 2019-06-12 02:36 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Daniel Gustafsson <daniel@yesql.se>; clemens@ladisch.de, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On Tue, Jun 11, 2019 at 09:32:55AM -0400, Alvaro Herrera wrote:
> Yeah, looks better. I think the error message should be a normal elog()
> cache failure, though ... at least in the COMMENT case, the obj-does-not-
> exist message is supposed to be thrown by get_object_address(), before
> check_object_ownership is called.
>
> As a matter of style, I would get rid of the 'conoid' variable and just
> use address.objectId where needed.
OK. I have included both your comments, and committed the patch down
to 9.5 where it applies. Thanks for the feedback!
s/unathorized/unauthorized/ by the way.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #15833: defining a comment on a domain constraint fails with wrong OID
@ 2019-06-12 04:51 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 11+ messages in thread
From: Alvaro Herrera @ 2019-06-12 04:51 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Daniel Gustafsson <daniel@yesql.se>; clemens@ladisch.de, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On 2019-Jun-12, Michael Paquier wrote:
> On Tue, Jun 11, 2019 at 09:32:55AM -0400, Alvaro Herrera wrote:
> > Yeah, looks better. I think the error message should be a normal elog()
> > cache failure, though ... at least in the COMMENT case, the obj-does-not-
> > exist message is supposed to be thrown by get_object_address(), before
> > check_object_ownership is called.
> >
> > As a matter of style, I would get rid of the 'conoid' variable and just
> > use address.objectId where needed.
>
> OK. I have included both your comments, and committed the patch down
> to 9.5 where it applies. Thanks for the feedback!
Thanks for fixing :-)
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #15833: defining a comment on a domain constraint fails with wrong OID
@ 2019-06-12 05:40 Daniel Gustafsson <daniel@yesql.se>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 11+ messages in thread
From: Daniel Gustafsson @ 2019-06-12 05:40 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; clemens@ladisch.de, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
> On 12 Jun 2019, at 04:36, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Jun 11, 2019 at 09:32:55AM -0400, Alvaro Herrera wrote:
>> Yeah, looks better. I think the error message should be a normal elog()
>> cache failure, though ... at least in the COMMENT case, the obj-does-not-
>> exist message is supposed to be thrown by get_object_address(), before
>> check_object_ownership is called.
>>
>> As a matter of style, I would get rid of the 'conoid' variable and just
>> use address.objectId where needed.
>
> OK. I have included both your comments, and committed the patch down
> to 9.5 where it applies. Thanks for the feedback!
Thanks!
> s/unathorized/unauthorized/ by the way.
Ugh, sorry about that.
cheers ./daniel
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2019-06-12 05:40 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-06-05 10:50 BUG #15833: defining a comment on a domain constraint fails with wrong OID PG Bug reporting form <noreply@postgresql.org>
2019-06-05 18:15 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-06-07 05:42 ` Michael Paquier <michael@paquier.xyz>
2019-06-10 06:28 ` Michael Paquier <michael@paquier.xyz>
2019-06-10 10:25 ` Daniel Gustafsson <daniel@yesql.se>
2019-06-10 12:55 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-06-11 04:55 ` Michael Paquier <michael@paquier.xyz>
2019-06-11 13:32 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-06-12 02:36 ` Michael Paquier <michael@paquier.xyz>
2019-06-12 04:51 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2019-06-12 05:40 ` Daniel Gustafsson <daniel@yesql.se>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox