Date: Mon, 10 Jun 2019 15:28:55 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: clemens@ladisch.de, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15833: defining a comment on a domain constraint fails with
 wrong OID
Message-ID: <20190610062855.GE2199@paquier.xyz>
References: <15833-808e11904835d26f@postgresql.org>
 <20190605181502.GA10753@alvherre.pgsql>
 <20190607054233.GG1736@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="nqkreNcslJAfgyzk"
Content-Disposition: inline
In-Reply-To: <20190607054233.GG1736@paquier.xyz>
Precedence: bulk


--nqkreNcslJAfgyzk
Content-Type: multipart/mixed; boundary="HCdXmnRlPgeNBad2"
Content-Disposition: inline


--HCdXmnRlPgeNBad2
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--HCdXmnRlPgeNBad2
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="dom-constraint-comments-v1.patch"
Content-Transfer-Encoding: quoted-printable

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);
 }
=20
+/*
+ * 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 =3D 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 =3D ((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/obje=
ctaddress.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 objty=
pe, 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 classo=
id);
 /* 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/i=
nput/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 =3D);
 DROP TABLE deferred_excl;
=20
 -- 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 CHEC=
K (a > 0));
 CREATE DOMAIN constraint_comments_dom AS int CONSTRAINT the_constraint CHE=
CK (value > 0);
=20
@@ -537,3 +541,5 @@ COMMENT ON CONSTRAINT the_constraint ON DOMAIN constrai=
nt_comments_dom IS NULL;
=20
 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)=3D(3) conflicts with key (f1)=3D(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 CHEC=
K (a > 0));
 CREATE DOMAIN constraint_comments_dom AS int CONSTRAINT the_constraint CHE=
CK (value > 0);
 COMMENT ON CONSTRAINT the_constraint ON constraint_comments_tbl IS 'yes, t=
he comment';
@@ -722,3 +725,5 @@ COMMENT ON CONSTRAINT the_constraint ON constraint_comm=
ents_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;

--HCdXmnRlPgeNBad2--

--nqkreNcslJAfgyzk
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlz9+KcACgkQnvQgOdby
QH1u5Q/+OrAFDD9KYP5lt1OEg0bullNlO/l+beoJj7vswqF10jXU79LHBkoAKvnB
qXWNddNWlEsSfpuA2uVLD2UUGsSM75/h/5zjUdCbFfdiPIdaFforV42bYNnacmWe
42pkXbaNwBsMyznGQSddOp987jb5efWHutGNPna6VFwpHfWA2zYCO7QxKyPnW+MY
9JNEFUVlSXbymdlnaiQLO0Bap8y6M6Sh5lD/ckvBGQQ5DqQZD2H7cR3EhO4Co2oB
sRInPE6212Qe4W5i2IugYC6UIn+9Q0SSMmOfnWFYhPvHYszZyS8TQhVnOpk3JsYl
KKAsBMvUMFaBd91F3W6Y2ANB6IL0Qv0i6ST4CA9WZ7aKcMsiY1bca8YEGKzY7nGg
rTS8g7rCXIkbDdsh+sgGi0xT6cndM/SSN11PSf6vzwojni1PNBG9IBxF/i9xtaEA
PRCnPBEMc68Qzg2oCr7+PkXMISUxevxe/fqTEnsdCehEflUsCZxnx0SFS5ZeMKMm
55tO90W6TRuIcMhsvJG1KANmeeocL7xvDxUtEZhhr+i8aw2eAEnxquvMCFmQUuGU
+kW20+/+kGusqGpF68+nf1Hlrq1YMIrOaSrqwe5fgn9/41L7ywYblRK0cQy3thtD
bYNkKQ35XYIBDcX3du4dh6pXamyOM2Nbju2PKE6XD0BR1n7w/IA=
=8O0h
-----END PGP SIGNATURE-----

--nqkreNcslJAfgyzk--