Re: Strange permission problem regarding pg_settings

Tom Lane wrote:
> I suspect the fact that the pre-patch code made the "right" permissions
> check was really coincidental, and that the correct fix will not involve
> reversion of that patch but rather adding a facility somewhere to ensure
> that the original view gets properly permission-checked even if there's
> a DO INSTEAD NOTHING rule. However, before biting that bullet it'd
> probably be good to understand in detail what's happening in both the
> 7.3.2 and CVS-tip code. I have not looked at just why that patch
> changes this example's behavior.
>

I just started looking at this again. There is definitely an issue in
cvs tip:

create table t(f1 int, f2 text);
insert into t values(1, 'abc');
create view v as select * from t;
CREATE RULE v_upd AS ON UPDATE TO v DO INSTEAD
UPDATE t SET f1 = NEW.f1, f2 = NEW.f2 WHERE f1 = OLD.f1;
create user user1;

-- this fails; as it should, I think
\c - user1
update v set f2 = 'def' where f1 = 1;
ERROR: permission denied for relation v

-- so grant SELECT on the view
\c - postgres
grant select on v to public;

-- this should fail, but doesn't
\c - user1
update v set f2 = 'def' where f1 = 1;
UPDATE 1

On 7.3.2 that last section of the above script gives:

\c - user1
update v set f2 = 'def' where f1 = 1;
ERROR: v: permission denied

The comment associated with the change says this:

* Also, we must disable write-access checking in all the RT entries
* copied from the main query. This is safe since in fact the rule
* action won't write on them, and it's necessary because the rule
* action may have a different commandType than the main query, causing
* ExecCheckRTEPerms() to make an inappropriate check. The read-access
* checks can be left enabled, although they're probably redundant.
*/

So SELECT permissions get checked for user1, but write-access does not.
The underlying table should be checked for permissions based on the rule
owner per rewriteDefine.c around line 439 (line 387 in 7.3.2):

/*
* We want the rule's table references to be checked as though by the
* rule owner, not the user referencing the rule. Therefore, scan
* through the rule's rtables and set the checkAsUser field on all
* rtable entries.
*/

Since the rule owner in this case is also the creator of the table, the
UPDATE suceeds.

ISTM that we want the relations in the un-rewritten query checked based
on the basis of the user referencing the rule and for the modes used in
the un-rewritten query -- suggesting the change need be reverted. Then
we want the rule's table references checked based on rule owner and
actual operations performed. It looks like this part should be what's
happening.

I went back to the original complaint -- here is the example on a 7.3.2
installation:

regression=# create table table1 (test1 integer);
grant insert on table1 to pleb;
create rule test_rule as on insert to table1 do update table2 set test2
= 2 where test2 = 0;
\c - pleb;
insert into table1 values (1);
CREATE TABLE
regression=# create table table2 (test2 integer);
CREATE TABLE
regression=# create user pleb;
ERROR: CREATE USER: user name "pleb" already exists
regression=# grant insert on table1 to pleb;
GRANT
regression=# create rule test_rule as on insert to table1 do update
table2 set test2 = 2 where test2 = 0;
CREATE RULE
regression=# \c - pleb;
You are now connected as new user pleb.
regression=> insert into table1 values (1);
ERROR: table1: permission denied

A few NOTICES placed in ExecCheckRTEPerms() reveals this:

regression=> insert into table1 values (1);
NOTICE: relOid = 1245674
NOTICE: userid = 101
NOTICE: operation = CMD_INSERT
NOTICE: relOid = 1245674
NOTICE: userid = 101
NOTICE: operation = CMD_UPDATE
ERROR: table1: permission denied

regression=> select oid, relname from pg_class where relname like 'table%';
oid | relname
---------+---------
1245674 | table1
1245676 | table2
(2 rows)

It seems that second pass through ExecCheckRTEPerms() is not doing the
right thing. It ought to be checking table2 (not table1) for UPDATE as
userid == 1 (not 101), shouldn't it?

Any thoughts on where to look next?

Thanks,

Joe