On Fri, Dec 3, 2021 at 9:53 AM Greg Nancarrow <gregn4...@gmail.com> wrote:
>
> On Fri, Dec 3, 2021 at 2:06 PM vignesh C <vignes...@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
From c75ce78efc80e86bcc62f8560a767cc73666a7d2 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignes...@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
