Alter all tables in schema owner fix
Hi,
Currently while changing the owner of ALL TABLES IN SCHEMA
publication, it is not checked if the new owner has superuser
permission or not. Added a check to throw an error if the new owner
does not have superuser permission.
Attached patch has the changes for the same. Thoughts?
Regards,
Vignesh
Attachments:
v1-0001-Fix-for-new-owner-of-ALL-TABLES-IN-SCHEMA-publica.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Fix-for-new-owner-of-ALL-TABLES-IN-SCHEMA-publica.patchDownload
From d0d0d2d8944a66f28b239da5fb80c15415a016e6 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Thu, 2 Dec 2021 19:44:15 +0530
Subject: [PATCH v1] Fix for new owner of ALL TABLES IN SCHEMA publication
should be superuser.
Currently while changing the owner of ALL TABLES IN SCHEMA publication, it is
not checked if the new owner has superuser permission or not. Added a check to
throw an error if the new owner does not have super user permission.
---
src/backend/commands/publicationcmds.c | 38 +++++++++++++++++++++++
src/test/regress/expected/publication.out | 17 ++++++++++
src/test/regress/sql/publication.sql | 17 ++++++++++
3 files changed, 72 insertions(+)
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 7d4a0e95f6..7081919ffc 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -234,6 +234,37 @@ CheckObjSchemaNotAlreadyInPublication(List *rels, List *schemaidlist,
}
}
+/*
+ * Check if any schema is associated with the publication.
+ */
+static bool
+CheckSchemaPublication(Oid pubid)
+{
+ Relation pubschsrel;
+ ScanKeyData scankey;
+ SysScanDesc scan;
+ HeapTuple tup;
+ bool result = false;
+
+ pubschsrel = table_open(PublicationNamespaceRelationId, AccessShareLock);
+ ScanKeyInit(&scankey,
+ Anum_pg_publication_namespace_pnpubid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(pubid));
+
+ scan = systable_beginscan(pubschsrel,
+ PublicationNamespacePnnspidPnpubidIndexId,
+ true, NULL, 1, &scankey);
+ tup = systable_getnext(scan);
+ if (HeapTupleIsValid(tup))
+ result = true;
+
+ systable_endscan(scan);
+ table_close(pubschsrel, AccessShareLock);
+
+ return result;
+}
+
/*
* Create new publication.
*/
@@ -1192,6 +1223,13 @@ AlterPublicationOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
errmsg("permission denied to change owner of publication \"%s\"",
NameStr(form->pubname)),
errhint("The owner of a FOR ALL TABLES publication must be a superuser.")));
+
+ if (CheckSchemaPublication(form->oid) && !superuser_arg(newOwnerId))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to change owner of publication \"%s\"",
+ NameStr(form->pubname)),
+ errhint("The owner of a FOR ALL TABLES IN SCHEMA publication must be a superuser.")));
}
form->pubowner = newOwnerId;
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 1feb558968..4c74488113 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -373,6 +373,23 @@ ALTER PUBLICATION testpub2 ADD TABLE testpub_tbl1; -- ok
DROP PUBLICATION testpub2;
DROP PUBLICATION testpub3;
SET ROLE regress_publication_user;
+CREATE ROLE regress_publication_user3 LOGIN SUPERUSER;
+GRANT regress_publication_user2 TO regress_publication_user3;
+SET ROLE regress_publication_user3;
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test;
+RESET client_min_messages;
+SET ROLE regress_publication_user;
+ALTER ROLE regress_publication_user3 NOSUPERUSER;
+SET ROLE regress_publication_user3;
+-- fail - new owner must be superuser
+ALTER PUBLICATION testpub4 owner to regress_publication_user2; -- fail
+ERROR: permission denied to change owner of publication "testpub4"
+HINT: The owner of a FOR ALL TABLES IN SCHEMA publication must be a superuser.
+ALTER PUBLICATION testpub4 owner to regress_publication_user; -- ok
+SET ROLE regress_publication_user;
+DROP PUBLICATION testpub4;
+DROP ROLE regress_publication_user3;
REVOKE CREATE ON DATABASE regression FROM regress_publication_user2;
DROP TABLE testpub_parted;
DROP TABLE testpub_tbl1;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 8fa0435c32..880f0caa1d 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -218,6 +218,23 @@ DROP PUBLICATION testpub2;
DROP PUBLICATION testpub3;
SET ROLE regress_publication_user;
+CREATE ROLE regress_publication_user3 LOGIN SUPERUSER;
+GRANT regress_publication_user2 TO regress_publication_user3;
+SET ROLE regress_publication_user3;
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test;
+RESET client_min_messages;
+SET ROLE regress_publication_user;
+ALTER ROLE regress_publication_user3 NOSUPERUSER;
+SET ROLE regress_publication_user3;
+-- fail - new owner must be superuser
+ALTER PUBLICATION testpub4 owner to regress_publication_user2; -- fail
+ALTER PUBLICATION testpub4 owner to regress_publication_user; -- ok
+
+SET ROLE regress_publication_user;
+DROP PUBLICATION testpub4;
+DROP ROLE regress_publication_user3;
+
REVOKE CREATE ON DATABASE regression FROM regress_publication_user2;
DROP TABLE testpub_parted;
--
2.32.0
On Fri, Dec 3, 2021 at 2:06 PM vignesh C <vignesh21@gmail.com> wrote:
Currently while changing the owner of ALL TABLES IN SCHEMA
publication, it is not checked if the new owner has superuser
permission or not. Added a check to throw an error if the new owner
does not have superuser permission.
Attached patch has the changes for the same. Thoughts?
It looks OK to me, but just two things:
1) Isn't it better to name "CheckSchemaPublication" as
"IsSchemaPublication", since it has a boolean return and also
typically CheckXXX type functions normally do checking and error-out
if they find a problem.
2) Since superuser_arg() caches previous input arg (last_roleid) and
has a fast-exit, and has been called immediately before for the FOR
ALL TABLES case, it would be better to write:
+ if (CheckSchemaPublication(form->oid) && !superuser_arg(newOwnerId))
as:
+ if (!superuser_arg(newOwnerId) && IsSchemaPublication(form->oid))
Regards,
Greg Nancarrow
Fujitsu Australia
On 12/2/21, 7:07 PM, "vignesh C" <vignesh21@gmail.com> wrote:
Currently while changing the owner of ALL TABLES IN SCHEMA
publication, it is not checked if the new owner has superuser
permission or not. Added a check to throw an error if the new owner
does not have superuser permission.
Attached patch has the changes for the same. Thoughts?
Yeah, the documentation clearly states that "the new owner of a FOR
ALL TABLES or FOR ALL TABLES IN SCHEMA publication must be a
superuser" [0]https://www.postgresql.org/docs/devel/sql-alterpublication.html.
+/*
+ * Check if any schema is associated with the publication.
+ */
+static bool
+CheckSchemaPublication(Oid pubid)I don't think the name CheckSchemaPublication() accurately describes
what this function is doing. I would suggest something like
PublicationHasSchema() or PublicationContainsSchema(). Also, much of
this new function appears to be copied from GetPublicationSchemas().
Should we just use that instead?
+CREATE ROLE regress_publication_user3 LOGIN SUPERUSER;
+GRANT regress_publication_user2 TO regress_publication_user3;
+SET ROLE regress_publication_user3;
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test;
+RESET client_min_messages;
+SET ROLE regress_publication_user;
+ALTER ROLE regress_publication_user3 NOSUPERUSER;
+SET ROLE regress_publication_user3;I think this test setup can be simplified a bit:
CREATE ROLE regress_publication_user3 LOGIN;
GRANT regress_publication_user2 TO regress_publication_user3;
SET client_min_messages = 'ERROR';
CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test;
RESET client_min_messages;
ALTER PUBLICATION testpub4 OWNER TO regress_publication_user3;
SET ROLE regress_publication_user3;
Nathan
[0]: https://www.postgresql.org/docs/devel/sql-alterpublication.html
On Fri, Dec 3, 2021 at 9:58 AM Bossart, Nathan <bossartn@amazon.com> wrote:
On 12/2/21, 7:07 PM, "vignesh C" <vignesh21@gmail.com> wrote:
Currently while changing the owner of ALL TABLES IN SCHEMA
publication, it is not checked if the new owner has superuser
permission or not. Added a check to throw an error if the new owner
does not have superuser permission.
Attached patch has the changes for the same. Thoughts?Yeah, the documentation clearly states that "the new owner of a FOR
ALL TABLES or FOR ALL TABLES IN SCHEMA publication must be a
superuser" [0].+/* + * Check if any schema is associated with the publication. + */ +static bool +CheckSchemaPublication(Oid pubid)I don't think the name CheckSchemaPublication() accurately describes
what this function is doing. I would suggest something like
PublicationHasSchema() or PublicationContainsSchema(). Also, much of
this new function appears to be copied from GetPublicationSchemas().
Should we just use that instead?
I was thinking of changing it to IsSchemaPublication as suggested by
Greg unless you feel differently. I did not use GetPublicationSchemas
function because in our case we just need to check if there is any
schema publication, we don't need the schema list to be prepared in
this case. That is the reason I wrote a new function which just checks
if any schema is present or not for the publication. I'm planning to
use CheckSchemaPublication (renamed to IsSchemaPublication) so that
the list need not be prepared.
+CREATE ROLE regress_publication_user3 LOGIN SUPERUSER; +GRANT regress_publication_user2 TO regress_publication_user3; +SET ROLE regress_publication_user3; +SET client_min_messages = 'ERROR'; +CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test; +RESET client_min_messages; +SET ROLE regress_publication_user; +ALTER ROLE regress_publication_user3 NOSUPERUSER; +SET ROLE regress_publication_user3;I think this test setup can be simplified a bit:
CREATE ROLE regress_publication_user3 LOGIN;
GRANT regress_publication_user2 TO regress_publication_user3;
SET client_min_messages = 'ERROR';
CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test;
RESET client_min_messages;
ALTER PUBLICATION testpub4 OWNER TO regress_publication_user3;
SET ROLE regress_publication_user3;
I will make this change in the next version.
Regards,
VIgnesh
On Fri, Dec 3, 2021 at 9:53 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
On Fri, Dec 3, 2021 at 2:06 PM vignesh C <vignesh21@gmail.com> wrote:
Currently while changing the owner of ALL TABLES IN SCHEMA
publication, it is not checked if the new owner has superuser
permission or not. Added a check to throw an error if the new owner
does not have superuser permission.
Attached patch has the changes for the same. Thoughts?It looks OK to me, but just two things:
1) Isn't it better to name "CheckSchemaPublication" as
"IsSchemaPublication", since it has a boolean return and also
typically CheckXXX type functions normally do checking and error-out
if they find a problem.
Modified
2) Since superuser_arg() caches previous input arg (last_roleid) and
has a fast-exit, and has been called immediately before for the FOR
ALL TABLES case, it would be better to write:+ if (CheckSchemaPublication(form->oid) && !superuser_arg(newOwnerId))
as:
+ if (!superuser_arg(newOwnerId) && IsSchemaPublication(form->oid))
Modified
Thanks for the comments, the attached v2 patch has the changes for the same.
Regards,
Vignesh
Attachments:
v2-0001-Fix-for-new-owner-of-ALL-TABLES-IN-SCHEMA-publica.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Fix-for-new-owner-of-ALL-TABLES-IN-SCHEMA-publica.patchDownload
From c75ce78efc80e86bcc62f8560a767cc73666a7d2 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Thu, 2 Dec 2021 19:44:15 +0530
Subject: [PATCH v2] Fix for new owner of ALL TABLES IN SCHEMA publication
should be superuser.
Currently while changing the owner of ALL TABLES IN SCHEMA publication, it is
not checked if the new owner has superuser permission or not. Added a check to
throw an error if the new owner does not have super user permission.
---
src/backend/commands/publicationcmds.c | 39 +++++++++++++++++++++++
src/test/regress/expected/publication.out | 15 +++++++++
src/test/regress/sql/publication.sql | 15 +++++++++
3 files changed, 69 insertions(+)
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 7d4a0e95f6..e709ddb589 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -234,6 +234,38 @@ CheckObjSchemaNotAlreadyInPublication(List *rels, List *schemaidlist,
}
}
+/*
+ * Returns true if any schema is associated with the publication, false if no
+ * schema is associated with the publication.
+ */
+static bool
+IsSchemaPublication(Oid pubid)
+{
+ Relation pubschsrel;
+ ScanKeyData scankey;
+ SysScanDesc scan;
+ HeapTuple tup;
+ bool result = false;
+
+ pubschsrel = table_open(PublicationNamespaceRelationId, AccessShareLock);
+ ScanKeyInit(&scankey,
+ Anum_pg_publication_namespace_pnpubid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(pubid));
+
+ scan = systable_beginscan(pubschsrel,
+ PublicationNamespacePnnspidPnpubidIndexId,
+ true, NULL, 1, &scankey);
+ tup = systable_getnext(scan);
+ if (HeapTupleIsValid(tup))
+ result = true;
+
+ systable_endscan(scan);
+ table_close(pubschsrel, AccessShareLock);
+
+ return result;
+}
+
/*
* Create new publication.
*/
@@ -1192,6 +1224,13 @@ AlterPublicationOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
errmsg("permission denied to change owner of publication \"%s\"",
NameStr(form->pubname)),
errhint("The owner of a FOR ALL TABLES publication must be a superuser.")));
+
+ if (!superuser_arg(newOwnerId) && IsSchemaPublication(form->oid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to change owner of publication \"%s\"",
+ NameStr(form->pubname)),
+ errhint("The owner of a FOR ALL TABLES IN SCHEMA publication must be a superuser.")));
}
form->pubowner = newOwnerId;
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 1feb558968..b7df48e87c 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -373,6 +373,21 @@ ALTER PUBLICATION testpub2 ADD TABLE testpub_tbl1; -- ok
DROP PUBLICATION testpub2;
DROP PUBLICATION testpub3;
SET ROLE regress_publication_user;
+CREATE ROLE regress_publication_user3;
+GRANT regress_publication_user2 TO regress_publication_user3;
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test;
+RESET client_min_messages;
+ALTER PUBLICATION testpub4 OWNER TO regress_publication_user3;
+SET ROLE regress_publication_user3;
+-- fail - new owner must be superuser
+ALTER PUBLICATION testpub4 owner to regress_publication_user2; -- fail
+ERROR: permission denied to change owner of publication "testpub4"
+HINT: The owner of a FOR ALL TABLES IN SCHEMA publication must be a superuser.
+ALTER PUBLICATION testpub4 owner to regress_publication_user; -- ok
+SET ROLE regress_publication_user;
+DROP PUBLICATION testpub4;
+DROP ROLE regress_publication_user3;
REVOKE CREATE ON DATABASE regression FROM regress_publication_user2;
DROP TABLE testpub_parted;
DROP TABLE testpub_tbl1;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 8fa0435c32..d5628f0251 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -218,6 +218,21 @@ DROP PUBLICATION testpub2;
DROP PUBLICATION testpub3;
SET ROLE regress_publication_user;
+CREATE ROLE regress_publication_user3;
+GRANT regress_publication_user2 TO regress_publication_user3;
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test;
+RESET client_min_messages;
+ALTER PUBLICATION testpub4 OWNER TO regress_publication_user3;
+SET ROLE regress_publication_user3;
+-- fail - new owner must be superuser
+ALTER PUBLICATION testpub4 owner to regress_publication_user2; -- fail
+ALTER PUBLICATION testpub4 owner to regress_publication_user; -- ok
+
+SET ROLE regress_publication_user;
+DROP PUBLICATION testpub4;
+DROP ROLE regress_publication_user3;
+
REVOKE CREATE ON DATABASE regression FROM regress_publication_user2;
DROP TABLE testpub_parted;
--
2.32.0
On Friday, December 3, 2021 1:31 PM vignesh C <vignesh21@gmail.com> wrote:
On Fri, Dec 3, 2021 at 9:53 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
On Fri, Dec 3, 2021 at 2:06 PM vignesh C <vignesh21@gmail.com> wrote:
Currently while changing the owner of ALL TABLES IN SCHEMA
publication, it is not checked if the new owner has superuser
permission or not. Added a check to throw an error if the new owner
does not have superuser permission.
Attached patch has the changes for the same. Thoughts?It looks OK to me, but just two things:
1) Isn't it better to name "CheckSchemaPublication" as
"IsSchemaPublication", since it has a boolean return and also
typically CheckXXX type functions normally do checking and error-out
if they find a problem.Modified
2) Since superuser_arg() caches previous input arg (last_roleid) and
has a fast-exit, and has been called immediately before for the FOR
ALL TABLES case, it would be better to write:+ if (CheckSchemaPublication(form->oid) && !superuser_arg(newOwnerId))
as:
+ if (!superuser_arg(newOwnerId) && IsSchemaPublication(form->oid))
Modified
Thanks for the comments, the attached v2 patch has the changes for the same.
Thanks for your patch.
I tested it and it fixed this problem as expected. It also passed "make check-world".
Regards,
Tang
On 12/2/21, 11:57 PM, "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com> wrote:
Thanks for your patch.
I tested it and it fixed this problem as expected. It also passed "make check-world".
+1, the patch looks good to me, too. My only other suggestion would
be to move IsSchemaPublication() to pg_publication.c
Nathan
On Fri, Dec 03, 2021 at 05:20:35PM +0000, Bossart, Nathan wrote:
On 12/2/21, 11:57 PM, "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com> wrote:
Thanks for your patch.
I tested it and it fixed this problem as expected. It also passed "make check-world".+1, the patch looks good to me, too. My only other suggestion would
be to move IsSchemaPublication() to pg_publication.c
There is more to that, no? It seems to me that anything that opens
PublicationNamespaceRelationId should be in pg_publication.c, so that
would include RemovePublicationSchemaById(). If you do that,
GetSchemaPublicationRelations() could be local to pg_publication.c.
+ tup = systable_getnext(scan);
+ if (HeapTupleIsValid(tup))
+ result = true;
This can be written as just "result = HeapTupleIsValid(tup)". Anyway,
this code also means that once we drop the schema this publication
won't be considered anymore as a schema publication, meaning that it
also makes this code weaker to actual cache lookup failures? I find
the semantics around pg_publication_namespace is bit weird because of
that, and inconsistent with the existing
puballtables/pg_publication_rel.
--
Michael
On Mon, Dec 6, 2021 at 11:46 AM Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Dec 03, 2021 at 05:20:35PM +0000, Bossart, Nathan wrote:
On 12/2/21, 11:57 PM, "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com> wrote:
Thanks for your patch.
I tested it and it fixed this problem as expected. It also passed "make check-world".+1, the patch looks good to me, too. My only other suggestion would
be to move IsSchemaPublication() to pg_publication.cThere is more to that, no? It seems to me that anything that opens
PublicationNamespaceRelationId should be in pg_publication.c, so that
would include RemovePublicationSchemaById().
It is currently similar to RemovePublicationById,
RemovePublicationRelById, etc. which are also in publicationcmds.c.
If you do that,
GetSchemaPublicationRelations() could be local to pg_publication.c.+ tup = systable_getnext(scan);
+ if (HeapTupleIsValid(tup))
+ result = true;
This can be written as just "result = HeapTupleIsValid(tup)". Anyway,
this code also means that once we drop the schema this publication
won't be considered anymore as a schema publication, meaning that it
also makes this code weaker to actual cache lookup failures?
How, can you be a bit more specific?
I find
the semantics around pg_publication_namespace is bit weird because of
that, and inconsistent with the existing
puballtables/pg_publication_rel.
What do you mean by inconsistent with puballtables/pg_publication_rel?
--
With Regards,
Amit Kapila.
On Fri, Dec 3, 2021 at 10:50 PM Bossart, Nathan <bossartn@amazon.com> wrote:
On 12/2/21, 11:57 PM, "tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com> wrote:
Thanks for your patch.
I tested it and it fixed this problem as expected. It also passed "make check-world".+1, the patch looks good to me, too. My only other suggestion would
be to move IsSchemaPublication() to pg_publication.c
Thanks for your comments, I have made the changes. Additionally I have
renamed IsSchemaPublication to is_schema_publication for keeping the
naming similar around the code. The attached v3 patch has the changes
for the same.
Regards,
Vignesh
Attachments:
v3-0001-Fix-for-new-owner-of-ALL-TABLES-IN-SCHEMA-publica.patchtext/x-patch; charset=US-ASCII; name=v3-0001-Fix-for-new-owner-of-ALL-TABLES-IN-SCHEMA-publica.patchDownload
From cc4226ced0e535a324dde5b1ff0e48d6e0035e17 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Thu, 2 Dec 2021 19:44:15 +0530
Subject: [PATCH v3] Fix for new owner of ALL TABLES IN SCHEMA publication
should be superuser.
Currently while changing the owner of ALL TABLES IN SCHEMA publication, it is
not checked if the new owner has superuser permission or not. Added a check to
throw an error if the new owner does not have super user permission.
---
src/backend/catalog/pg_publication.c | 31 +++++++++++++++++++++++
src/backend/commands/publicationcmds.c | 7 +++++
src/include/catalog/pg_publication.h | 1 +
src/test/regress/expected/publication.out | 15 +++++++++++
src/test/regress/sql/publication.sql | 15 +++++++++++
5 files changed, 69 insertions(+)
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 63579b2f82..265bd538c3 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -193,6 +193,37 @@ is_publishable_relation(Relation rel)
return is_publishable_class(RelationGetRelid(rel), rel->rd_rel);
}
+/*
+ * Returns true if any schema is associated with the publication, false if no
+ * schema is associated with the publication.
+ */
+bool
+is_schema_publication(Oid pubid)
+{
+ Relation pubschsrel;
+ ScanKeyData scankey;
+ SysScanDesc scan;
+ HeapTuple tup;
+ bool result = false;
+
+ pubschsrel = table_open(PublicationNamespaceRelationId, AccessShareLock);
+ ScanKeyInit(&scankey,
+ Anum_pg_publication_namespace_pnpubid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(pubid));
+
+ scan = systable_beginscan(pubschsrel,
+ PublicationNamespacePnnspidPnpubidIndexId,
+ true, NULL, 1, &scankey);
+ tup = systable_getnext(scan);
+ if (HeapTupleIsValid(tup))
+ result = true;
+
+ systable_endscan(scan);
+ table_close(pubschsrel, AccessShareLock);
+
+ return result;
+}
/*
* SQL-callable variant of the above
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 7d4a0e95f6..404bb5d0c8 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1192,6 +1192,13 @@ AlterPublicationOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
errmsg("permission denied to change owner of publication \"%s\"",
NameStr(form->pubname)),
errhint("The owner of a FOR ALL TABLES publication must be a superuser.")));
+
+ if (!superuser_arg(newOwnerId) && is_schema_publication(form->oid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to change owner of publication \"%s\"",
+ NameStr(form->pubname)),
+ errhint("The owner of a FOR ALL TABLES IN SCHEMA publication must be a superuser.")));
}
form->pubowner = newOwnerId;
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index 1ae439e6f3..902f2f2f0d 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -122,6 +122,7 @@ extern List *GetPubPartitionOptionRelations(List *result,
Oid relid);
extern bool is_publishable_relation(Relation rel);
+extern bool is_schema_publication(Oid pubid);
extern ObjectAddress publication_add_relation(Oid pubid, PublicationRelInfo *targetrel,
bool if_not_exists);
extern ObjectAddress publication_add_schema(Oid pubid, Oid schemaid,
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 1feb558968..b7df48e87c 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -373,6 +373,21 @@ ALTER PUBLICATION testpub2 ADD TABLE testpub_tbl1; -- ok
DROP PUBLICATION testpub2;
DROP PUBLICATION testpub3;
SET ROLE regress_publication_user;
+CREATE ROLE regress_publication_user3;
+GRANT regress_publication_user2 TO regress_publication_user3;
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test;
+RESET client_min_messages;
+ALTER PUBLICATION testpub4 OWNER TO regress_publication_user3;
+SET ROLE regress_publication_user3;
+-- fail - new owner must be superuser
+ALTER PUBLICATION testpub4 owner to regress_publication_user2; -- fail
+ERROR: permission denied to change owner of publication "testpub4"
+HINT: The owner of a FOR ALL TABLES IN SCHEMA publication must be a superuser.
+ALTER PUBLICATION testpub4 owner to regress_publication_user; -- ok
+SET ROLE regress_publication_user;
+DROP PUBLICATION testpub4;
+DROP ROLE regress_publication_user3;
REVOKE CREATE ON DATABASE regression FROM regress_publication_user2;
DROP TABLE testpub_parted;
DROP TABLE testpub_tbl1;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 8fa0435c32..d5628f0251 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -218,6 +218,21 @@ DROP PUBLICATION testpub2;
DROP PUBLICATION testpub3;
SET ROLE regress_publication_user;
+CREATE ROLE regress_publication_user3;
+GRANT regress_publication_user2 TO regress_publication_user3;
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test;
+RESET client_min_messages;
+ALTER PUBLICATION testpub4 OWNER TO regress_publication_user3;
+SET ROLE regress_publication_user3;
+-- fail - new owner must be superuser
+ALTER PUBLICATION testpub4 owner to regress_publication_user2; -- fail
+ALTER PUBLICATION testpub4 owner to regress_publication_user; -- ok
+
+SET ROLE regress_publication_user;
+DROP PUBLICATION testpub4;
+DROP ROLE regress_publication_user3;
+
REVOKE CREATE ON DATABASE regression FROM regress_publication_user2;
DROP TABLE testpub_parted;
--
2.32.0
On Tue, Dec 7, 2021 at 8:21 AM vignesh C <vignesh21@gmail.com> wrote:
Thanks for your comments, I have made the changes. Additionally I have
renamed IsSchemaPublication to is_schema_publication for keeping the
naming similar around the code. The attached v3 patch has the changes
for the same.
Thanks, the patch looks mostly good to me. I have slightly modified it
to incorporate one of Michael's suggestions, ran pgindent, and
modified the commit message.
I am planning to push the attached tomorrow unless there are further
comments. Michael, do let me know if you have any questions or
objections about this?
--
With Regards,
Amit Kapila.
Attachments:
v4-0001-Fix-changing-the-ownership-of-ALL-TABLES-IN-SCHEM.patchapplication/octet-stream; name=v4-0001-Fix-changing-the-ownership-of-ALL-TABLES-IN-SCHEM.patchDownload
From f2f3e320c3e22c6246658d629055baa79615f87b Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Tue, 7 Dec 2021 14:55:10 +0530
Subject: [PATCH v4] Fix changing the ownership of ALL TABLES IN SCHEMA
publication.
Ensure that the new owner of ALL TABLES IN SCHEMA publication must be a
superuser. The same is already ensured during Create Publication.
Author: Vignesh C
Reviewed-by: Nathan Bossart, Greg Nancarrow, Michael Paquier, Haiying Tang
Discussion: https://postgr.es/m/CALDaNm0E5U-RqxFuFrkZrQeG7ae5trGa=xs=iRtPPHULtT4zOw@mail.gmail.com
---
src/backend/catalog/pg_publication.c | 30 ++++++++++++++++++++++++++++++
src/backend/commands/publicationcmds.c | 7 +++++++
src/include/catalog/pg_publication.h | 1 +
src/test/regress/expected/publication.out | 15 +++++++++++++++
src/test/regress/sql/publication.sql | 15 +++++++++++++++
5 files changed, 68 insertions(+)
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 63579b2..18ded46 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -193,6 +193,36 @@ is_publishable_relation(Relation rel)
return is_publishable_class(RelationGetRelid(rel), rel->rd_rel);
}
+/*
+ * Returns true if any schema is associated with the publication, false if no
+ * schema is associated with the publication.
+ */
+bool
+is_schema_publication(Oid pubid)
+{
+ Relation pubschsrel;
+ ScanKeyData scankey;
+ SysScanDesc scan;
+ HeapTuple tup;
+ bool result = false;
+
+ pubschsrel = table_open(PublicationNamespaceRelationId, AccessShareLock);
+ ScanKeyInit(&scankey,
+ Anum_pg_publication_namespace_pnpubid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(pubid));
+
+ scan = systable_beginscan(pubschsrel,
+ PublicationNamespacePnnspidPnpubidIndexId,
+ true, NULL, 1, &scankey);
+ tup = systable_getnext(scan);
+ result = HeapTupleIsValid(tup);
+
+ systable_endscan(scan);
+ table_close(pubschsrel, AccessShareLock);
+
+ return result;
+}
/*
* SQL-callable variant of the above
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 7d4a0e9..404bb5d 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1192,6 +1192,13 @@ AlterPublicationOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
errmsg("permission denied to change owner of publication \"%s\"",
NameStr(form->pubname)),
errhint("The owner of a FOR ALL TABLES publication must be a superuser.")));
+
+ if (!superuser_arg(newOwnerId) && is_schema_publication(form->oid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to change owner of publication \"%s\"",
+ NameStr(form->pubname)),
+ errhint("The owner of a FOR ALL TABLES IN SCHEMA publication must be a superuser.")));
}
form->pubowner = newOwnerId;
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index 1ae439e..902f2f2 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -122,6 +122,7 @@ extern List *GetPubPartitionOptionRelations(List *result,
Oid relid);
extern bool is_publishable_relation(Relation rel);
+extern bool is_schema_publication(Oid pubid);
extern ObjectAddress publication_add_relation(Oid pubid, PublicationRelInfo *targetrel,
bool if_not_exists);
extern ObjectAddress publication_add_schema(Oid pubid, Oid schemaid,
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 1feb558..b7df48e 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -373,6 +373,21 @@ ALTER PUBLICATION testpub2 ADD TABLE testpub_tbl1; -- ok
DROP PUBLICATION testpub2;
DROP PUBLICATION testpub3;
SET ROLE regress_publication_user;
+CREATE ROLE regress_publication_user3;
+GRANT regress_publication_user2 TO regress_publication_user3;
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test;
+RESET client_min_messages;
+ALTER PUBLICATION testpub4 OWNER TO regress_publication_user3;
+SET ROLE regress_publication_user3;
+-- fail - new owner must be superuser
+ALTER PUBLICATION testpub4 owner to regress_publication_user2; -- fail
+ERROR: permission denied to change owner of publication "testpub4"
+HINT: The owner of a FOR ALL TABLES IN SCHEMA publication must be a superuser.
+ALTER PUBLICATION testpub4 owner to regress_publication_user; -- ok
+SET ROLE regress_publication_user;
+DROP PUBLICATION testpub4;
+DROP ROLE regress_publication_user3;
REVOKE CREATE ON DATABASE regression FROM regress_publication_user2;
DROP TABLE testpub_parted;
DROP TABLE testpub_tbl1;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 8fa0435..d5628f0 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -218,6 +218,21 @@ DROP PUBLICATION testpub2;
DROP PUBLICATION testpub3;
SET ROLE regress_publication_user;
+CREATE ROLE regress_publication_user3;
+GRANT regress_publication_user2 TO regress_publication_user3;
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub4 FOR ALL TABLES IN SCHEMA pub_test;
+RESET client_min_messages;
+ALTER PUBLICATION testpub4 OWNER TO regress_publication_user3;
+SET ROLE regress_publication_user3;
+-- fail - new owner must be superuser
+ALTER PUBLICATION testpub4 owner to regress_publication_user2; -- fail
+ALTER PUBLICATION testpub4 owner to regress_publication_user; -- ok
+
+SET ROLE regress_publication_user;
+DROP PUBLICATION testpub4;
+DROP ROLE regress_publication_user3;
+
REVOKE CREATE ON DATABASE regression FROM regress_publication_user2;
DROP TABLE testpub_parted;
--
1.8.3.1
On Tue, Dec 7, 2021 at 9:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
Thanks, the patch looks mostly good to me. I have slightly modified it
to incorporate one of Michael's suggestions, ran pgindent, and
modified the commit message.
LGTM, except in the patch commit message I'd change "Create
Publication" to "CREATE PUBLICATION".
Regards,
Greg Nancarrow
Fujitsu Australia
On 12/7/21, 2:47 AM, "Greg Nancarrow" <gregn4422@gmail.com> wrote:
On Tue, Dec 7, 2021 at 9:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
Thanks, the patch looks mostly good to me. I have slightly modified it
to incorporate one of Michael's suggestions, ran pgindent, and
modified the commit message.LGTM, except in the patch commit message I'd change "Create
Publication" to "CREATE PUBLICATION".
LGTM, too.
Nathan
On Tue, Dec 7, 2021 at 11:20 PM Bossart, Nathan <bossartn@amazon.com> wrote:
On 12/7/21, 2:47 AM, "Greg Nancarrow" <gregn4422@gmail.com> wrote:
On Tue, Dec 7, 2021 at 9:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
Thanks, the patch looks mostly good to me. I have slightly modified it
to incorporate one of Michael's suggestions, ran pgindent, and
modified the commit message.LGTM, except in the patch commit message I'd change "Create
Publication" to "CREATE PUBLICATION".LGTM, too.
Pushed!
--
With Regards,
Amit Kapila.