Fujii-san,
Thanks for taking a look.
On Fri, Jan 10, 2020 at 10:29 AM Fujii Masao <masao.fu...@gmail.com> wrote:
> On Tue, Jan 7, 2020 at 5:15 PM Amit Langote <amitlangot...@gmail.com> wrote:
> > I tend to agree that TRUNCATE's permission model for inheritance
> > should be consistent with that for the other commands. How about the
> > attached patch toward that end?
>
> Thanks for the patch!
>
> The patch basically looks good to me.
>
> +GRANT SELECT (f1, fz), UPDATE (fz) ON atestc TO regress_priv_user2;
> +REVOKE TRUNCATE ON atestc FROM regress_priv_user2;
>
> These seem not to be necessary for the test.
You're right. Removed in the attached updated patch.
> BTW, I found that LOCK TABLE on the parent table checks the permission
> of its child tables. This also needs to be fixed (as a separate patch)?
Commit ac33c7e2c13 and a past discussion ([1], [2], resp.) appear to
disagree with that position, but I would like to agree with you
because the behavior you suggest would be consistent with other
commands. So, I'm attaching a patch for that too, although it would
be better to hear more opinions before accepting it.
Thanks,
Amit
[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ac33c7e2c13
[2]
https://www.postgresql.org/message-id/flat/34d269d40905121340h535ef652kbf8f054811e42e39%40mail.gmail.com
From fd763965648865062cdcb47c5029a50efb645119 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlangot...@gmail.com>
Date: Wed, 22 Jan 2020 15:39:39 +0900
Subject: [PATCH 1/2] Don't check child's TRUNCATE privilege when truncated
recursively
---
src/backend/commands/tablecmds.c | 30 +++++++++++++++++++++++-------
src/test/regress/expected/privileges.out | 21 +++++++++++++++++++++
src/test/regress/sql/privileges.sql | 14 ++++++++++++++
3 files changed, 58 insertions(+), 7 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7c23968f2d..9b86692e87 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -304,6 +304,7 @@ struct DropRelationCallbackState
((child_is_partition) ? DEPENDENCY_AUTO : DEPENDENCY_NORMAL)
static void truncate_check_rel(Oid relid, Form_pg_class reltuple);
+static void truncate_check_perms(Oid relid, Form_pg_class reltuple);
static void truncate_check_activity(Relation rel);
static void RangeVarCallbackForTruncate(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
@@ -1616,6 +1617,10 @@ ExecuteTruncate(TruncateStmt *stmt)
}
truncate_check_rel(RelationGetRelid(rel),
rel->rd_rel);
+ /*
+ * We skip checking the child's permission,
because we
+ * already checked the parent's.
+ */
truncate_check_activity(rel);
rels = lappend(rels, rel);
@@ -1701,6 +1706,7 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids,
List *relids_logged,
(errmsg("truncate cascades to
table \"%s\"",
RelationGetRelationName(rel))));
truncate_check_rel(relid, rel->rd_rel);
+ truncate_check_perms(relid, rel->rd_rel);
truncate_check_activity(rel);
rels = lappend(rels, rel);
relids = lappend_oid(relids, relid);
@@ -1951,7 +1957,6 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids,
List *relids_logged,
static void
truncate_check_rel(Oid relid, Form_pg_class reltuple)
{
- AclResult aclresult;
char *relname = NameStr(reltuple->relname);
/*
@@ -1965,12 +1970,6 @@ truncate_check_rel(Oid relid, Form_pg_class reltuple)
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not a table", relname)));
- /* Permissions checks */
- aclresult = pg_class_aclcheck(relid, GetUserId(), ACL_TRUNCATE);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult,
get_relkind_objtype(reltuple->relkind),
- relname);
-
if (!allowSystemTableMods && IsSystemClass(relid, reltuple))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -1980,6 +1979,22 @@ truncate_check_rel(Oid relid, Form_pg_class reltuple)
InvokeObjectTruncateHook(relid);
}
+/*
+ * Check that current user has the permission to truncate given relation.
+ */
+static void
+truncate_check_perms(Oid relid, Form_pg_class reltuple)
+{
+ char *relname = NameStr(reltuple->relname);
+ AclResult aclresult;
+
+ /* Permissions checks */
+ aclresult = pg_class_aclcheck(relid, GetUserId(), ACL_TRUNCATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult,
get_relkind_objtype(reltuple->relkind),
+ relname);
+}
+
/*
* Set of extra sanity checks to check if a given relation is safe to
* truncate. This is split with truncate_check_rel() as
@@ -15287,6 +15302,7 @@ RangeVarCallbackForTruncate(const RangeVar *relation,
elog(ERROR, "cache lookup failed for relation %u", relId);
truncate_check_rel(relId, (Form_pg_class) GETSTRUCT(tuple));
+ truncate_check_perms(relId, (Form_pg_class) GETSTRUCT(tuple));
ReleaseSysCache(tuple);
}
diff --git a/src/test/regress/expected/privileges.out
b/src/test/regress/expected/privileges.out
index 0ddbd8e89f..bc8e198097 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -695,6 +695,27 @@ SELECT tableoid FROM atestp2; -- ok
----------
(0 rows)
+-- child's permissions do not apply when operating on parent
+SET SESSION AUTHORIZATION regress_priv_user1;
+REVOKE ALL ON atestc FROM regress_priv_user2;
+GRANT ALL ON atestp1 TO regress_priv_user2;
+SET SESSION AUTHORIZATION regress_priv_user2;
+SELECT f2 FROM atestp1; -- ok
+ f2
+----
+(0 rows)
+
+SELECT f2 FROM atestc; -- fail
+ERROR: permission denied for table atestc
+DELETE FROM atestp1; -- ok
+DELETE FROM atestc; -- fail
+ERROR: permission denied for table atestc
+UPDATE atestp1 SET f1 = 1; -- ok
+UPDATE atestc SET f1 = 1; -- fail
+ERROR: permission denied for table atestc
+TRUNCATE atestp1; -- ok
+TRUNCATE atestc; -- fail
+ERROR: permission denied for table atestc
-- privileges on functions, languages
-- switch to superuser
\c -
diff --git a/src/test/regress/sql/privileges.sql
b/src/test/regress/sql/privileges.sql
index f15d1f3773..dfe2603fe2 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -446,6 +446,20 @@ SELECT fy FROM atestp2; -- ok
SELECT atestp2 FROM atestp2; -- ok
SELECT tableoid FROM atestp2; -- ok
+-- child's permissions do not apply when operating on parent
+SET SESSION AUTHORIZATION regress_priv_user1;
+REVOKE ALL ON atestc FROM regress_priv_user2;
+GRANT ALL ON atestp1 TO regress_priv_user2;
+SET SESSION AUTHORIZATION regress_priv_user2;
+SELECT f2 FROM atestp1; -- ok
+SELECT f2 FROM atestc; -- fail
+DELETE FROM atestp1; -- ok
+DELETE FROM atestc; -- fail
+UPDATE atestp1 SET f1 = 1; -- ok
+UPDATE atestc SET f1 = 1; -- fail
+TRUNCATE atestp1; -- ok
+TRUNCATE atestc; -- fail
+
-- privileges on functions, languages
-- switch to superuser
--
2.16.5
From 87963d50b0416be397d0da9789c0c8efce733fe3 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlangot...@gmail.com>
Date: Wed, 22 Jan 2020 15:43:41 +0900
Subject: [PATCH 2/2] Don't check child's LOCK privilege when locked
recursively
---
src/backend/commands/lockcmds.c | 33 +++++++++++---------------------
src/test/regress/expected/lock.out | 8 ++------
src/test/regress/expected/privileges.out | 7 +++++++
src/test/regress/sql/lock.sql | 7 ++-----
src/test/regress/sql/privileges.sql | 6 ++++++
5 files changed, 28 insertions(+), 33 deletions(-)
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 329ab849c0..d8cafc42bb 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -28,7 +28,7 @@
#include "utils/lsyscache.h"
#include "utils/syscache.h"
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid
userid);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
Oid oldrelid, void *arg);
@@ -59,7 +59,7 @@ LockTableCommand(LockStmt *lockstmt)
if (get_rel_relkind(reloid) == RELKIND_VIEW)
LockViewRecurse(reloid, lockstmt->mode,
lockstmt->nowait, NIL);
else if (recurse)
- LockTableRecurse(reloid, lockstmt->mode,
lockstmt->nowait, GetUserId());
+ LockTableRecurse(reloid, lockstmt->mode,
lockstmt->nowait);
}
}
@@ -108,35 +108,26 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid
relid, Oid oldrelid,
/*
* Apply LOCK TABLE recursively over an inheritance tree
*
- * We use find_inheritance_children not find_all_inheritors to avoid taking
- * locks far in advance of checking privileges. This means we'll visit
- * multiply-inheriting children more than once, but that's no problem.
+ * This doesn't check permission to perform LOCK TABLE on the child tables,
+ * because getting here means that the user has permission to lock the
+ * parent which is enough.
*/
static void
-LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid)
+LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
{
List *children;
ListCell *lc;
- children = find_inheritance_children(reloid, NoLock);
+ children = find_all_inheritors(reloid, NoLock, NULL);
foreach(lc, children)
{
Oid childreloid = lfirst_oid(lc);
- AclResult aclresult;
- /* Check permissions before acquiring the lock. */
- aclresult = LockTableAclCheck(childreloid, lockmode, userid);
- if (aclresult != ACLCHECK_OK)
- {
- char *relname = get_rel_name(childreloid);
-
- if (!relname)
- continue; /* child concurrently
dropped, just skip it */
- aclcheck_error(aclresult,
get_relkind_objtype(get_rel_relkind(childreloid)), relname);
- }
+ /* Parent already locked. */
+ if (childreloid == reloid)
+ continue;
- /* We have enough rights to lock the relation; do so. */
if (!nowait)
LockRelationOid(childreloid, lockmode);
else if (!ConditionalLockRelationOid(childreloid, lockmode))
@@ -162,8 +153,6 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool
nowait, Oid userid)
UnlockRelationOid(childreloid, lockmode);
continue;
}
-
- LockTableRecurse(childreloid, lockmode, nowait, userid);
}
}
@@ -241,7 +230,7 @@ LockViewRecurse_walker(Node *node, LockViewRecurse_context
*context)
if (relkind == RELKIND_VIEW)
LockViewRecurse(relid, context->lockmode,
context->nowait, context->ancestor_views);
else if (rte->inh)
- LockTableRecurse(relid, context->lockmode,
context->nowait, context->viewowner);
+ LockTableRecurse(relid, context->lockmode,
context->nowait);
}
return query_tree_walker(query,
diff --git a/src/test/regress/expected/lock.out
b/src/test/regress/expected/lock.out
index 185fd2f879..5f20b846c9 100644
--- a/src/test/regress/expected/lock.out
+++ b/src/test/regress/expected/lock.out
@@ -138,16 +138,12 @@ CREATE TABLE lock_tbl3 () INHERITS (lock_tbl2);
BEGIN TRANSACTION;
LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE;
ROLLBACK;
--- Verify that we can't lock a child table just because we have permission
--- on the parent, but that we can lock the parent only.
+-- Verify that we can lock children too as long as we have permission
+-- on the parent.
GRANT UPDATE ON TABLE lock_tbl1 TO regress_rol_lock1;
SET ROLE regress_rol_lock1;
BEGIN;
LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE;
-ERROR: permission denied for table lock_tbl2
-ROLLBACK;
-BEGIN;
-LOCK TABLE ONLY lock_tbl1;
ROLLBACK;
RESET ROLE;
--
diff --git a/src/test/regress/expected/privileges.out
b/src/test/regress/expected/privileges.out
index bc8e198097..43d50d0c54 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -716,6 +716,13 @@ ERROR: permission denied for table atestc
TRUNCATE atestp1; -- ok
TRUNCATE atestc; -- fail
ERROR: permission denied for table atestc
+BEGIN;
+LOCK atestp1;
+END;
+BEGIN;
+LOCK atestc;
+ERROR: permission denied for table atestc
+END;
-- privileges on functions, languages
-- switch to superuser
\c -
diff --git a/src/test/regress/sql/lock.sql b/src/test/regress/sql/lock.sql
index 26a7e59a13..22db0d3690 100644
--- a/src/test/regress/sql/lock.sql
+++ b/src/test/regress/sql/lock.sql
@@ -101,16 +101,13 @@ BEGIN TRANSACTION;
LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE;
ROLLBACK;
--- Verify that we can't lock a child table just because we have permission
--- on the parent, but that we can lock the parent only.
+-- Verify that we can lock children too as long as we have permission
+-- on the parent.
GRANT UPDATE ON TABLE lock_tbl1 TO regress_rol_lock1;
SET ROLE regress_rol_lock1;
BEGIN;
LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE;
ROLLBACK;
-BEGIN;
-LOCK TABLE ONLY lock_tbl1;
-ROLLBACK;
RESET ROLE;
--
diff --git a/src/test/regress/sql/privileges.sql
b/src/test/regress/sql/privileges.sql
index dfe2603fe2..2ba69617dc 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -459,6 +459,12 @@ UPDATE atestp1 SET f1 = 1; -- ok
UPDATE atestc SET f1 = 1; -- fail
TRUNCATE atestp1; -- ok
TRUNCATE atestc; -- fail
+BEGIN;
+LOCK atestp1;
+END;
+BEGIN;
+LOCK atestc;
+END;
-- privileges on functions, languages
--
2.16.5
