Обсуждение: LOCK TABLE Permissions

Greetings,
 We've run into a curious case and I'd like to solicit feedback regarding a possible change to the access rights
requiredto acquire locks on a relation.  Specifically, we have a process which normally INSERTs into a table and
anotherprocess which Exclusive locks that same table in order to syncronize other processing.  We then ran into a case
wherewe didn't actually want to INSERT but still wanted to have the syncronization happen.  Unfortunately, we don't
allowLOCK TABLE to acquire RowExclusive unless you have UPDATE, DELETE, or TRUNCATE privileges.
 
 My first impression is that the current code was just overly simplistic regarding what level of permissions are
requiredfor a given lock type and that it wasn't intentional to deny processes which have INSERT privileges from
acquiringRowExclusive (as they can do so anyway using an actual INSERT).  Therefore, I'd like to propose the below
simple3-line patch to correct this.
 
 Thoughts?  Objections to back-patching?
     Thanks,
    Stephen

diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
new file mode 100644
index 49950d7..60f54c5 100644
*** a/src/backend/commands/lockcmds.c
--- b/src/backend/commands/lockcmds.c
*************** LockTableAclCheck(Oid reloid, LOCKMODE l
*** 174,179 ****
--- 174,182 ----   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);
 

Stephen Frost <sfrost@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

On Fri, Jul 19, 2013 at 12:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Stephen Frost <sfrost@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

All,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@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

On Mon, May 11, 2015 at 10:32 PM, Stephen Frost <sfrost@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

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Mon, May 11, 2015 at 10:32 PM, Stephen Frost <sfrost@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

All,

* Stephen Frost (sfrost@snowman.net) wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > Stephen Frost <sfrost@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

On Tue, May 12, 2015 at 4:01 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Michael Paquier (michael.paquier@gmail.com) wrote:
>> On Mon, May 11, 2015 at 10:32 PM, Stephen Frost <sfrost@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

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Tue, May 12, 2015 at 4:01 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > * Michael Paquier (michael.paquier@gmail.com) wrote:
> >> On Mon, May 11, 2015 at 10:32 PM, Stephen Frost <sfrost@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

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

* Michael Paquier (michael.paquier@gmail.com) wrote:
> 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.

Er, yes.
Thanks!
    Stephen