Re: [HACKERS] LOCK TABLE Permissions
All,
* Tom Lane (t...@sss.pgh.pa.us) wrote:
Stephen Frost sfr...@snowman.net writes:
if (lockmode == AccessShareLock)
aclresult = pg_class_aclcheck(reloid, GetUserId(),
ACL_SELECT);
+ else if (lockmode == RowExclusiveLock)
+ aclresult = pg_class_aclcheck(reloid, GetUserId(),
+ACL_INSERT | ACL_UPDATE | ACL_DELETE |
ACL_TRUNCATE);
else
aclresult = pg_class_aclcheck(reloid, GetUserId(),
ACL_UPDATE | ACL_DELETE |
ACL_TRUNCATE);
Perhaps it would be better to refactor with a local variable for the
aclmask and just one instance of the pg_class_aclcheck call. Also, I'm
pretty sure that the documentation work needed is more extensive
than the actual patch ;-). Otherwise, I don't see a problem with this.
Now for a blast from the past... This came up again on IRC recently and
reminded me that I ran into the same issue a couple years back. Updated
patch includes the refactoring suggested and includes documentation.
Not going to be back-patched, as discussed with Robert.
Barring objections, I'll push this later today.
Thanks!
Stephen
From d2b0fbc9fd8c0783f870fef3c901f7f8891c6e90 Mon Sep 17 00:00:00 2001
From: Stephen Frost sfr...@snowman.net
Date: Mon, 11 May 2015 09:14:49 -0400
Subject: [PATCH] Allow LOCK TABLE .. ROW EXCLUSIVE MODE with INSERT
INSERT acquires RowExclusiveLock during normal operation and therefore
it makes sense to allow LOCK TABLE .. ROW EXCLUSIVE MODE to be executed
by users who have INSERT rights on a table (even if they don't have
UPDATE or DELETE).
Not back-patching this as it's a behavior change which, strictly
speaking, loosens security restrictions.
Per discussion with Tom and Robert (circa 2013).
---
doc/src/sgml/ref/lock.sgml | 8 +---
src/backend/commands/lockcmds.c | 12
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
index 913afe7..b946eab 100644
--- a/doc/src/sgml/ref/lock.sgml
+++ b/doc/src/sgml/ref/lock.sgml
@@ -161,9 +161,11 @@ LOCK [ TABLE ] [ ONLY ] replaceable class=PARAMETERname/replaceable [ * ]
para
literalLOCK TABLE ... IN ACCESS SHARE MODE/ requires literalSELECT/
-privileges on the target table. All other forms of commandLOCK/
-require table-level literalUPDATE/, literalDELETE/, or
-literalTRUNCATE/ privileges.
+privileges on the target table. literalLOCK TABLE ... IN ROW EXCLUSIVE
+MODE/ requires literalINSERT/, literalUPDATE/, literalDELETE/,
+or literalTRUNCATE/ privileges on the target table. All other forms of
+commandLOCK/ require table-level literalUPDATE/, literalDELETE/,
+or literalTRUNCATE/ privileges.
/para
para
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index bdec2ff..a167082 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -169,13 +169,17 @@ static AclResult
LockTableAclCheck(Oid reloid, LOCKMODE lockmode)
{
AclResult aclresult;
+ AclMode aclmask;
/* Verify adequate privilege */
if (lockmode == AccessShareLock)
- aclresult = pg_class_aclcheck(reloid, GetUserId(),
- ACL_SELECT);
+ aclmask = ACL_SELECT;
+ else if (lockmode == RowExclusiveLock)
+ aclmask = ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE;
else
- aclresult = pg_class_aclcheck(reloid, GetUserId(),
- ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE);
+ aclmask = ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE;
+
+ aclresult = pg_class_aclcheck(reloid, GetUserId(), aclmask);
+
return aclresult;
}
--
1.9.1
signature.asc
Description: Digital signature
Re: [HACKERS] LOCK TABLE Permissions
On Mon, May 11, 2015 at 10:32 PM, Stephen Frost sfr...@snowman.net wrote: Now for a blast from the past... This came up again on IRC recently and reminded me that I ran into the same issue a couple years back. Updated patch includes the refactoring suggested and includes documentation. Barring objections, I'll push this later today. Small suggestion: a test case in src/test/isolation? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LOCK TABLE Permissions
* Michael Paquier (michael.paqu...@gmail.com) wrote: On Mon, May 11, 2015 at 10:32 PM, Stephen Frost sfr...@snowman.net wrote: Now for a blast from the past... This came up again on IRC recently and reminded me that I ran into the same issue a couple years back. Updated patch includes the refactoring suggested and includes documentation. Barring objections, I'll push this later today. Small suggestion: a test case in src/test/isolation? This is entirely a permissions-related change and src/test/isolation is for testing concurrent behavior, not about testing permissions. I'm not saying that we shouldn't have more tests there, but it'd not be appropriate for this particular patch. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] LOCK TABLE Permissions
All, * Stephen Frost (sfr...@snowman.net) wrote: * Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: if (lockmode == AccessShareLock) aclresult = pg_class_aclcheck(reloid, GetUserId(), ACL_SELECT); + else if (lockmode == RowExclusiveLock) + aclresult = pg_class_aclcheck(reloid, GetUserId(), +ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE); else aclresult = pg_class_aclcheck(reloid, GetUserId(), ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE); Perhaps it would be better to refactor with a local variable for the aclmask and just one instance of the pg_class_aclcheck call. Also, I'm pretty sure that the documentation work needed is more extensive than the actual patch ;-). Otherwise, I don't see a problem with this. Now for a blast from the past... This came up again on IRC recently and reminded me that I ran into the same issue a couple years back. Updated patch includes the refactoring suggested and includes documentation. Not going to be back-patched, as discussed with Robert. Barring objections, I'll push this later today. Done, finally. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] LOCK TABLE Permissions
On Tue, May 12, 2015 at 4:01 AM, Stephen Frost sfr...@snowman.net wrote: * Michael Paquier (michael.paqu...@gmail.com) wrote: On Mon, May 11, 2015 at 10:32 PM, Stephen Frost sfr...@snowman.net wrote: Now for a blast from the past... This came up again on IRC recently and reminded me that I ran into the same issue a couple years back. Updated patch includes the refactoring suggested and includes documentation. Barring objections, I'll push this later today. Small suggestion: a test case in src/test/isolation? This is entirely a permissions-related change and src/test/isolation is for testing concurrent behavior, not about testing permissions. I'm not saying that we shouldn't have more tests there, but it'd not be appropriate for this particular patch. Perhaps. Note that we could have tests of this type though in lock.sql: create role foo login; create table aa (a int); grant insert on aa TO foo; \c postgres foo begin; insert into aa values (1); lock table aa in row exclusive mode; -- should pass Just mentioning it for the sake of the archives, I cannot work on that for now. Regards, -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LOCK TABLE Permissions
* Michael Paquier (michael.paqu...@gmail.com) wrote: On Tue, May 12, 2015 at 4:01 AM, Stephen Frost sfr...@snowman.net wrote: * Michael Paquier (michael.paqu...@gmail.com) wrote: On Mon, May 11, 2015 at 10:32 PM, Stephen Frost sfr...@snowman.net wrote: Now for a blast from the past... This came up again on IRC recently and reminded me that I ran into the same issue a couple years back. Updated patch includes the refactoring suggested and includes documentation. Barring objections, I'll push this later today. Small suggestion: a test case in src/test/isolation? This is entirely a permissions-related change and src/test/isolation is for testing concurrent behavior, not about testing permissions. I'm not saying that we shouldn't have more tests there, but it'd not be appropriate for this particular patch. Perhaps. Note that we could have tests of this type though in lock.sql: create role foo login; create table aa (a int); grant insert on aa TO foo; \c postgres foo begin; insert into aa values (1); lock table aa in row exclusive mode; -- should pass Yeah, it might not be bad to have tests for all the different lock types and make sure that the permissions match up. I'd probably put those tests into 'permissions.sql' instead though. Just mentioning it for the sake of the archives, I cannot work on that for now. Ditto. I'm trying to work through the postgres_fdw UPDATE push-down patch now.. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] LOCK TABLE Permissions
On Tue, May 12, 2015 at 4:53 AM, Stephen Frost wrote: Yeah, it might not be bad to have tests for all the different lock types and make sure that the permissions match up. I'd probably put those tests into 'permissions.sql' instead though. You mean privileges.sql, right? I just wrote a patch with that. I'll create a new thread with the patch. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LOCK TABLE Permissions
Stephen Frost sfr...@snowman.net writes: if (lockmode == AccessShareLock) aclresult = pg_class_aclcheck(reloid, GetUserId(), ACL_SELECT); + else if (lockmode == RowExclusiveLock) + aclresult = pg_class_aclcheck(reloid, GetUserId(), +ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE); else aclresult = pg_class_aclcheck(reloid, GetUserId(), ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE); Perhaps it would be better to refactor with a local variable for the aclmask and just one instance of the pg_class_aclcheck call. Also, I'm pretty sure that the documentation work needed is more extensive than the actual patch ;-). Otherwise, I don't see a problem with this. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LOCK TABLE Permissions
On Fri, Jul 19, 2013 at 12:33 PM, Tom Lane t...@sss.pgh.pa.us wrote: Stephen Frost sfr...@snowman.net writes: if (lockmode == AccessShareLock) aclresult = pg_class_aclcheck(reloid, GetUserId(), ACL_SELECT); + else if (lockmode == RowExclusiveLock) + aclresult = pg_class_aclcheck(reloid, GetUserId(), +ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE); else aclresult = pg_class_aclcheck(reloid, GetUserId(), ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE); Perhaps it would be better to refactor with a local variable for the aclmask and just one instance of the pg_class_aclcheck call. Also, I'm pretty sure that the documentation work needed is more extensive than the actual patch ;-). Otherwise, I don't see a problem with this. I don't really care one way or the other whether we change this in master, but I think back-patching changes that loosen security restrictions is a poor idea. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
