Re: New patch for Column-level privileges

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> Please find attached an updated patch for column-level privileges
> which incorporates Alvaro's suggested changes and is updated to the
> latest CVS HEAD. Regression tests have been added as well as
> documentation (though this could probably be improved). Currently,
> column-level privileges are not honored when JOINs are involved (you
> must have the necessary table-level privileges, as you do today). It
> would really be great to have that working and mainly involves
> modifying the rewriter to add on to the appropriate range table column
> list entries the columns which are used in the joins and output from
> joins.

Here it is gzip'd since I'm afraid the last might not have made it
through due to size..

Stephen

On Fri, Jan 2, 2009 at 16:11, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Stephen Frost (sfrost(at)snowman(dot)net) wrote:
>> Please find attached an updated patch for column-level privileges
>> which incorporates Alvaro's suggested changes and is updated to the
>> latest CVS HEAD.

Hi!

This gives me
aclchk.c: In function 'ExecuteGrantStmt':
aclchk.c:276: warning: 'errormsg_col' may be used uninitialized in this function

Now it looks bogos but why not just move errmsg_col down to where we
actually use it? Or am I missing something?

*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
***************
*** 273,279 **** ExecuteGrantStmt(GrantStmt *stmt)
InternalGrant istmt;
ListCell *cell;
const char *errormsg;
- const char *errormsg_col;
AclMode all_privileges;

/*
--- 273,278 ----
***************
*** 324,330 **** ExecuteGrantStmt(GrantStmt *stmt)
case ACL_OBJECT_RELATION:
all_privileges = ACL_ALL_RIGHTS_RELATION | ACL_ALL_RIGHTS_SEQUENCE;
errormsg = gettext_noop("invalid privilege type %s for relation");
- errormsg_col = gettext_noop("invalid privilege type %s for column");
break;
case ACL_OBJECT_SEQUENCE:
all_privileges = ACL_ALL_RIGHTS_SEQUENCE;
--- 323,328 ----
***************
*** 362,368 **** ExecuteGrantStmt(GrantStmt *stmt)
/* keep compiler quiet */
all_privileges = ACL_NO_RIGHTS;
errormsg = NULL;
- errormsg_col = NULL;
elog(ERROR, "unrecognized GrantStmt.objtype: %d",
(int) stmt->objtype);
}
--- 360,365 ----
***************
*** 404,409 **** ExecuteGrantStmt(GrantStmt *stmt)
--- 401,408 ----
{
/* Column-level privileges given. */
ListCell *cell_obj;
+ const char *errormsg_col = gettext_noop("invalid privilege type
%s for column");
+

/* Must be of objtype ACL_OBJECT_RELATION for column
* level privileges */

Alex,

* Alex Hunsaker (badalex(at)gmail(dot)com) wrote:
> Now it looks bogos but why not just move errmsg_col down to where we
> actually use it? Or am I missing something?

Honestly, I think at this point is makes the most sense to just remove
it entirely, which I've done in the attached patch.

Thanks,

Stephen

Hello Stephen,

Stephen Frost wrote:
> ..in the attached patch.

Thanks, I've reviewed this patch again.

> Please find attached an updated patch for column-level privileges
> which incorporates Alvaro's suggested changes and is updated to the
> latest CVS HEAD.

Cool, applies cleanly and compiles without any error or warning on my
Debian box. Regression tests pass fine as well.

> Regression tests have been added as well as
> documentation (though this could probably be improved).

Sorry I didn't get around writing docu. Looks good so far. Some more
hints on its usage wouldn't hurt, especially because the error messages
aren't overly verbose ('permission denied..' doesn't tell you much).

> Currently,
> column-level privileges are not honored when JOINs are involved (you
> must have the necessary table-level privileges, as you do today). It
> would really be great to have that working and mainly involves
> modifying the rewriter to add on to the appropriate range table column
> list entries the columns which are used in the joins and output from
> joins.

Understood. Do you plan to work on that? Or do you think the patch
should go into 8.4 even without support for JOINs?

Experimenting with relation vs column level privileges, I've discovered
a strange behavior:

test=# GRANT SELECT ON test TO joe;
GRANT
test=# GRANT SELECT(id) ON test TO joe;
GRANT
test=# REVOKE SELECT ON test FROM joe;
ERROR: tuple already updated by self

That's odd. Maybe you need to increment the command counter in between
two updates of that tuple?

Also note, that I'm unsure about what to expect from this REVOKE. Is it
intended to remove the column level privilege as well or not?

Removing the column level privilege first, then the relation level one
works:

test=# REVOKE SELECT(id) ON test FROM joe;
REVOKE
test=# REVOKE SELECT ON test FROM joe;
REVOKE

I've also tested column level privileges on hidden attributes (xmin,
xmax, ctid, tableoid, cmin), which works fine for SELECTs. However, you
might want to filter out INSERT and UPDATE privileges on those, as those
don't make much sense:

test=# GRANT UPDATE(xmin) ON test TO joe;
GRANT
test=# GRANT INSERT(xmin) ON test TO joe;
GRANT

[ Note that user joe can INSERT or UPDATE tuples of relation test even
without those column level privileges, as long as he is allowed to
INSERT or UPDATE the affected non-hidden columns. ]

Some minor nit-picks: some lines exceed 80 columns, multi-line comments
don't follow coding standards.

BTW: how are long constant strings expected to be formatted? Are those
allowed to exceed 80 columns, or are they expected to be split like so
(i.e. for errmsg):

"Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed "
"do eiusmod tempor incididunt ut labore et dolore magna aliqua."

Nice work! I'd like to see this shipping with 8.4. The above mentioned
bugs (the "updated by self" and the hidden columns) should be easy
enough to fix, I think. Respecting columns level privileges for JOINs is
probably going to be more work, but is required as well, IMO.

Regards

Markus Wanner

Markus,

* Markus Wanner (markus(at)bluegap(dot)ch) wrote:
> Stephen Frost wrote:
> > ..in the attached patch.
>
> Thanks, I've reviewed this patch again.

Thanks!

> > Currently,
> > column-level privileges are not honored when JOINs are involved (you
> > must have the necessary table-level privileges, as you do today). It
> > would really be great to have that working and mainly involves
> > modifying the rewriter to add on to the appropriate range table column
> > list entries the columns which are used in the joins and output from
> > joins.
>
> Understood. Do you plan to work on that? Or do you think the patch
> should go into 8.4 even without support for JOINs?

I'm going to look into it but it's a bit complicated. I was hoping
someone who's more familiar with those parts would be able to look at
it.

> Experimenting with relation vs column level privileges, I've discovered
> a strange behavior:
>
> test=# GRANT SELECT ON test TO joe;
> GRANT
> test=# GRANT SELECT(id) ON test TO joe;
> GRANT
> test=# REVOKE SELECT ON test FROM joe;
> ERROR: tuple already updated by self
>
> That's odd. Maybe you need to increment the command counter in between
> two updates of that tuple?

I don't think that's the right approach, but I'll look into it. I ran
into a similiar issue though, and I don't believe it's too hard to fix
(the issue here is that the REVOKE needs to remove the column-level grant
as well). I'll try and look into it tonight or tomorrow.

> Also note, that I'm unsure about what to expect from this REVOKE. Is it
> intended to remove the column level privilege as well or not?

Yes, the SQL spec requires that a table-level REVOKE also revoke all
column-level grants as well.

> Removing the column level privilege first, then the relation level one
> works:
>
> test=# REVOKE SELECT(id) ON test FROM joe;
> REVOKE
> test=# REVOKE SELECT ON test FROM joe;
> REVOKE

This is essentially what should happen automagically.

> I've also tested column level privileges on hidden attributes (xmin,
> xmax, ctid, tableoid, cmin), which works fine for SELECTs. However, you
> might want to filter out INSERT and UPDATE privileges on those, as those
> don't make much sense:
>
> test=# GRANT UPDATE(xmin) ON test TO joe;
> GRANT
> test=# GRANT INSERT(xmin) ON test TO joe;
> GRANT

Hmm, ok, that's easy enough to fix.

> [ Note that user joe can INSERT or UPDATE tuples of relation test even
> without those column level privileges, as long as he is allowed to
> INSERT or UPDATE the affected non-hidden columns. ]

Right, that's correct. You don't need table-level permissions so long
as you have permissions on the columns you're trying to select/modify.

> Some minor nit-picks: some lines exceed 80 columns, multi-line comments
> don't follow coding standards.

Hrmpf. I'll go back and review the coding standards.. I don't recall
that 80 column was a fixed limit.

> BTW: how are long constant strings expected to be formatted? Are those
> allowed to exceed 80 columns, or are they expected to be split like so
> (i.e. for errmsg):
>
> "Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed "
> "do eiusmod tempor incididunt ut labore et dolore magna aliqua."

Honestly, I think I've seen both done. I can do it either way, of
course.

> Nice work! I'd like to see this shipping with 8.4. The above mentioned
> bugs (the "updated by self" and the hidden columns) should be easy
> enough to fix, I think. Respecting columns level privileges for JOINs is
> probably going to be more work, but is required as well, IMO.

Thanks for the review!

Stephen

Hello Stephen,

Stephen Frost wrote:
> I'm going to look into it but it's a bit complicated. I was hoping
> someone who's more familiar with those parts would be able to look at
> it.

Good to hear. I've just been asking, because it remained unclear to me.

> I don't think that's the right approach, but I'll look into it. I ran
> into a similiar issue though, and I don't believe it's too hard to fix
> (the issue here is that the REVOKE needs to remove the column-level grant
> as well). I'll try and look into it tonight or tomorrow.

Cool, because that's the biggest issue, IMO.

>> test=# GRANT UPDATE(xmin) ON test TO joe;
>> GRANT
>> test=# GRANT INSERT(xmin) ON test TO joe;
>> GRANT
>
> Hmm, ok, that's easy enough to fix.
>
>> [ Note that user joe can INSERT or UPDATE tuples of relation test even
>> without those column level privileges, as long as he is allowed to
>> INSERT or UPDATE the affected non-hidden columns. ]
>
> Right, that's correct. You don't need table-level permissions so long
> as you have permissions on the columns you're trying to select/modify.

I was trying to check, if these privileges on hidden columns have any
effect. So far I didn't encounter any, except for SELECT.

>> Some minor nit-picks: some lines exceed 80 columns, multi-line comments
>> don't follow coding standards.
>
> Hrmpf. I'll go back and review the coding standards.. I don't recall
> that 80 column was a fixed limit.

Hm.. sorry, looks like it's not mentioned in the docu. I'm pretty sure
pgindent strips lines to something below 80 columns, though. (And I'm
personally used to terminals with exactly 80 cols, so everything longer
than is not easy to my eyes, thus the complaint. Don't bother much).

>> BTW: how are long constant strings expected to be formatted? Are those
>> allowed to exceed 80 columns, or are they expected to be split like so
>> (i.e. for errmsg):
>>
>> "Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed "
>> "do eiusmod tempor incididunt ut labore et dolore magna aliqua."
>
> Honestly, I think I've seen both done.

Yeah, that's why I'm asking.

Regards

Markus Wanner

Markus Wanner <markus(at)bluegap(dot)ch> writes:
> Stephen Frost wrote:
>>> BTW: how are long constant strings expected to be formatted? Are those
>>> allowed to exceed 80 columns, or are they expected to be split like so
>>
>> Honestly, I think I've seen both done.

> Yeah, that's why I'm asking.

IMHO, the trouble with breaking up error strings like that is that it
can defeat attempts to grep the source for a particular error message.
(If you search for "foo bar baz", you won't find it if someone chose
to break the string between those words.) This isn't too harmful if
you get no hits, because you can try again with a substring --- but it
really sucks if some instances of the string are broken up differently
than others, because you might see only the wrong instances and come
to a mistaken conclusion about the possible sources of an error reported
from the field. So I tend not to like breaking up long strings at all,
and definitely look with disfavor on breaking them in random spots
rather than natural break points of the sentence.

Because of that, I tend not to worry about holding to the 80-column
window width when it comes to error message strings. Other than that
case, though, you should try to stay to less than 80 columns. If you
don't, pgindent will do it for you, and the end result will probably
be uglier than if you'd made the code fit manually.

regards, tom lane

Markus Wanner wrote:
> Hello Stephen,
>
> Stephen Frost wrote:
>> ..in the attached patch.
>
> Thanks, I've reviewed this patch again.
>
>> Please find attached an updated patch for column-level privileges
>> which incorporates Alvaro's suggested changes and is updated to the
>> latest CVS HEAD.
>
> Cool, applies cleanly and compiles without any error or warning on my
> Debian box. Regression tests pass fine as well.
>
>> Regression tests have been added as well as
>> documentation (though this could probably be improved).
>
> Sorry I didn't get around writing docu. Looks good so far. Some more
> hints on its usage wouldn't hurt, especially because the error messages
> aren't overly verbose ('permission denied..' doesn't tell you much).
>
>> Currently,
>> column-level privileges are not honored when JOINs are involved (you
>> must have the necessary table-level privileges, as you do today). It
>> would really be great to have that working and mainly involves
>> modifying the rewriter to add on to the appropriate range table column
>> list entries the columns which are used in the joins and output from
>> joins.
>
> Understood. Do you plan to work on that? Or do you think the patch
> should go into 8.4 even without support for JOINs?

I tried to check the latest Column-level privileges patch.

This trouble related to JOINs is come from inappropriate handling
of rte->cols_sel list, in my opinion.
The list is constructed at scanRTEForColumn() and expandRelation().
However, scanRTEForColumn() appends an appeared attribute number
even if given RangeTblEntry is RTE_JOIN, and expandRelation() appends
all the attribute number contained within the given relation.

I think your design is basically fine, so the issue is minor one.
But it is necessary to pick up carefully what columns are really
accessed.

Here is one proposition.
Is it possible to implement a walker function to pick up appeared
columns and to chain them on rte->cols_sel/cols_mod?
In this idea, columns in Query->targetList should be chained on
rte->cols_mod, and others should be chained on rte->cols_sel.

Here is an example to pick up all appeared relation:
http://code.google.com/p/sepgsql/source/browse/trunk/sepgsql/src/backend/security/rowacl/rowacl.c#30

If you don't have enough availability, I'll be able to do it within
a few days.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

>>> Currently,
>>> column-level privileges are not honored when JOINs are involved (you
>>> must have the necessary table-level privileges, as you do today). It
>>> would really be great to have that working and mainly involves
>>> modifying the rewriter to add on to the appropriate range table column
>>> list entries the columns which are used in the joins and output from
>>> joins.
>>
>> Understood. Do you plan to work on that? Or do you think the patch
>> should go into 8.4 even without support for JOINs?
>
> I tried to check the latest Column-level privileges patch.
>
> This trouble related to JOINs is come from inappropriate handling
> of rte->cols_sel list, in my opinion.
> The list is constructed at scanRTEForColumn() and expandRelation().
> However, scanRTEForColumn() appends an appeared attribute number
> even if given RangeTblEntry is RTE_JOIN, and expandRelation() appends
> all the attribute number contained within the given relation.
>
> I think your design is basically fine, so the issue is minor one.
> But it is necessary to pick up carefully what columns are really
> accessed.
>
> Here is one proposition.
> Is it possible to implement a walker function to pick up appeared
> columns and to chain them on rte->cols_sel/cols_mod?
> In this idea, columns in Query->targetList should be chained on
> rte->cols_mod, and others should be chained on rte->cols_sel.
>
> Here is an example to pick up all appeared relation:
> http://code.google.com/p/sepgsql/source/browse/trunk/sepgsql/src/backend/security/rowacl/rowacl.c#30
>
>
> If you don't have enough availability, I'll be able to do it within
> a few days.

The attached patch is a proof of the concept.
It walks on a given query tree to append accessed columns on
rte->cols_sel and rte->cols_mod.
When aliasvar of JOIN'ed relation is accesses, its source is
appended on the list.

This patch can be applied on the latest CVS HEAD with Stephen's
colprivs_wip.2009010201.diff.gz.

Any comment?

I strongly want the Column-level privileges to be get merged
as soon as possible, so I don't spare any possible assist
for his works.

Thanks,

---- example of the patched Column-level privileges ----
postgres=# CREATE TABLE t1 (a int, b text, c bool);
CREATE TABLE
postgres=# CREATE TABLE t2 (x int, y text, z bool);
CREATE TABLE
postgres=# GRANT SELECT(a,b) ON t1 TO ymj;
GRANT
postgres=# GRANT UPDATE(a,b) ON t1 TO ymj;
GRANT
postgres=# GRANT SELECT(x,y) ON t2 TO ymj;
GRANT
postgres=# INSERT INTO t1 VALUES (1, 'aaa', true), (2, 'bbb', null), (3, 'ccc', false);
INSERT 0 3
postgres=# INSERT INTO t2 VALUES (1, 'xxx', false), (2, 'yyy', null), (3, 'zzz', true);
INSERT 0 3
postgres=# \c - ymj
psql (8.4devel)
You are now connected to database "postgres" as user "ymj".
postgres=> SELECT * FROM t1;
NOTICE: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
NOTICE: pg_attribute_aclmask: t1.b required: 0002 allowed: 0002
NOTICE: pg_attribute_aclmask: t1.c required: 0002 allowed: 0000
ERROR: permission denied for relation t1
postgres=> SELECT a,b FROM t1;
NOTICE: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
NOTICE: pg_attribute_aclmask: t1.b required: 0002 allowed: 0002
a | b
---+-----
1 | aaa
2 | bbb
3 | ccc
(3 rows)

postgres=> SELECT x,y FROM t2;
NOTICE: pg_attribute_aclmask: t2.x required: 0002 allowed: 0002
NOTICE: pg_attribute_aclmask: t2.y required: 0002 allowed: 0002
x | y
---+-----
1 | xxx
2 | yyy
3 | zzz
(3 rows)

postgres=> SELECT b,y FROM t1 JOIN t2 ON a = x;
NOTICE: pg_attribute_aclmask: t1.b required: 0002 allowed: 0002
NOTICE: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
NOTICE: pg_attribute_aclmask: t2.y required: 0002 allowed: 0002
NOTICE: pg_attribute_aclmask: t2.x required: 0002 allowed: 0002
b | y
-----+-----
aaa | xxx
bbb | yyy
ccc | zzz
(3 rows)

postgres=> SELECT b,z FROM t1 JOIN t2 ON a = x;
NOTICE: pg_attribute_aclmask: t1.b required: 0002 allowed: 0002
NOTICE: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
NOTICE: pg_attribute_aclmask: t2.z required: 0002 allowed: 0000
ERROR: permission denied for relation t2
postgres=> UPDATE t1 SET a = 1;
NOTICE: pg_attribute_aclmask: t1.a required: 0004 allowed: 0004
UPDATE 3
postgres=> UPDATE t1 SET c = true;
NOTICE: pg_attribute_aclmask: t1.c required: 0004 allowed: 0000
ERROR: permission denied for relation t1
postgres=>

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Hi,

KaiGai Kohei wrote:
> The attached patch is a proof of the concept.

Awesome! I'll try to review during the day.

> I strongly want the Column-level privileges to be get merged
> as soon as possible, so I don't spare any possible assist
> for his works.

+1

Can you quickly comment on CLP vs. SE-Postgres. Are these dependent or
completely orthogonal? I didn't follow the SE-Postgres thread very closely.

Regards

Markus Wanner

Markus Wanner wrote:
> Hi,
>
> KaiGai Kohei wrote:
>> The attached patch is a proof of the concept.
>
> Awesome! I'll try to review during the day.
>
>> I strongly want the Column-level privileges to be get merged
>> as soon as possible, so I don't spare any possible assist
>> for his works.
>
> +1
>
> Can you quickly comment on CLP vs. SE-Postgres. Are these dependent or
> completely orthogonal? I didn't follow the SE-Postgres thread very closely.

These are completely orthogonal.
However, in previous discussion, we decided CLP is a prerequisite to
merge SE-PostgreSQL feature.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> Is it possible to implement a walker function to pick up appeared
>> columns and to chain them on rte->cols_sel/cols_mod?
>> In this idea, columns in Query->targetList should be chained on
>> rte->cols_mod, and others should be chained on rte->cols_sel.

This sounds like a reasonable approach to me, but as I mentioned before,
I'm not very familiar with the analyzer and company.

> The attached patch is a proof of the concept.

Excellent, I'll play around with it.

> Any comment?

I'm generally not a huge fan of recursion simply because it's often
overrated and overused and implements a limit based on stack depth which
can cause unexpected failures. Can we be confident that the recursion
added here doesn't add a new limit on the size/complexity of queries
which, if hit, will cause a stack overflow? I notice that we do use
recursion in some other places, but we also occationally have checks to
prevent recursing too far.

> I strongly want the Column-level privileges to be get merged
> as soon as possible, so I don't spare any possible assist
> for his works.

Thanks so much for your help! It's definitely appriciated. I'm going
to try and play with your patch today and probably add some additional
regression tests and make sure everything works as expected.

Thanks again!

Stephen

Stephen Frost wrote:
> KaiGai,
>
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>>> Is it possible to implement a walker function to pick up appeared
>>> columns and to chain them on rte->cols_sel/cols_mod?
>>> In this idea, columns in Query->targetList should be chained on
>>> rte->cols_mod, and others should be chained on rte->cols_sel.
>
> This sounds like a reasonable approach to me, but as I mentioned before,
> I'm not very familiar with the analyzer and company.
>
>> The attached patch is a proof of the concept.
>
> Excellent, I'll play around with it.
>
>> Any comment?
>
> I'm generally not a huge fan of recursion simply because it's often
> overrated and overused and implements a limit based on stack depth which
> can cause unexpected failures. Can we be confident that the recursion
> added here doesn't add a new limit on the size/complexity of queries
> which, if hit, will cause a stack overflow? I notice that we do use
> recursion in some other places, but we also occationally have checks to
> prevent recursing too far.

I think it does not give us any other new size/complexity limitation,
because we already have some of recursive walker functions which
consume stack more than applyColumnPrivsWalker() I proposed.

The applyColumnPrivsWalker() consumes its stack for a return address,
two pointers and an integer variables for each depth.
But, for example, existing check_ungrouped_columns_walker() consumes
it more.

In addition, please note that expression_tree_walker() invokes
check_stack_depth() to prevent unexpected stack overflow.

Thanks,

>> I strongly want the Column-level privileges to be get merged
>> as soon as possible, so I don't spare any possible assist
>> for his works.
>
> Thanks so much for your help! It's definitely appriciated. I'm going
> to try and play with your patch today and probably add some additional
> regression tests and make sure everything works as expected.
>
> Thanks again!
>
> Stephen

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> In addition, please note that expression_tree_walker() invokes
> check_stack_depth() to prevent unexpected stack overflow.

Ah, that's the part I was looking for and somehow overlooked. As long
as we're checking at some point then I worry alot less. I'm currently
working out some dependency kinks (which is the real source of the
ERROR: tuple updated by self that Markus ran into). I'm planning to
post a new patch tomorrow which includes your patch, some additional
regression tests, and the dependency fix.

Thanks,

Stephen

On Wed, Jan 7, 2009 at 1:46 AM, KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>
> The attached patch is a proof of the concept.
> It walks on a given query tree to append accessed columns on
> rte->cols_sel and rte->cols_mod.
> When aliasvar of JOIN'ed relation is accesses, its source is
> appended on the list.
>

for my test i created to tables:

CREATE TABLE t1 (col1 int primary key, col2 int);
CREATE TABLE t2 (col1 int references t1);

and a role:

CREATE ROLE rol1;

then i granted all cols in the table to the role:

GRANT SELECT (col1) ON t1 TO rol1;
GRANT SELECT (col2) ON t1 TO rol1;
GRANT SELECT (col1) ON t2 TO rol1;

prueba=> \dp
Access privileges
Schema | Name | Type | Access privileges | Column Access privileges
--------+------+-------+---------------------------+--------------------------
public | t1 | table | postgres=arwdDxt/postgres | col1
: postgres=arw/postgres
: rol1=r/postgres
: col2
: postgres=arw/postgres
: rol1=r/postgres
public | t2 | table | postgres=arwdDxt/postgres | col1
: postgres=arw/postgres
: rol1=r/postgres
(2 rows)

then i execute:

prueba=> select t1.* from t1, t2 where t1.col1 = t2.col1;
NOTICE: pg_attribute_aclmask: t1.col1 required: 0002 allowed: 0002
NOTICE: pg_attribute_aclmask: t1.col2 required: 0002 allowed: 0002
NOTICE: pg_attribute_aclmask: t1.col1 required: 0002 allowed: 0002
NOTICE: pg_attribute_aclmask: t2.col1 required: 0002 allowed: 0002
col1 | col2
------+------
(0 rows)

good, but if i doesn't include filter conditions:

prueba=> select t1.* from t1, t2;
NOTICE: pg_attribute_aclmask: t1.col1 required: 0002 allowed: 0002
NOTICE: pg_attribute_aclmask: t1.col2 required: 0002 allowed: 0002
ERROR: permission denied for relation t2

is this intended?

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

Jaime Casanova wrote:
> On Wed, Jan 7, 2009 at 1:46 AM, KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>> The attached patch is a proof of the concept.
>> It walks on a given query tree to append accessed columns on
>> rte->cols_sel and rte->cols_mod.
>> When aliasvar of JOIN'ed relation is accesses, its source is
>> appended on the list.
>>
>
> for my test i created to tables:
>
> CREATE TABLE t1 (col1 int primary key, col2 int);
> CREATE TABLE t2 (col1 int references t1);
>
> and a role:
>
> CREATE ROLE rol1;
>
> then i granted all cols in the table to the role:
>
> GRANT SELECT (col1) ON t1 TO rol1;
> GRANT SELECT (col2) ON t1 TO rol1;
> GRANT SELECT (col1) ON t2 TO rol1;
>
> prueba=> \dp
> Access privileges
> Schema | Name | Type | Access privileges | Column Access privileges
> --------+------+-------+---------------------------+--------------------------
> public | t1 | table | postgres=arwdDxt/postgres | col1
> : postgres=arw/postgres
> : rol1=r/postgres
> : col2
> : postgres=arw/postgres
> : rol1=r/postgres
> public | t2 | table | postgres=arwdDxt/postgres | col1
> : postgres=arw/postgres
> : rol1=r/postgres
> (2 rows)
>
>
> then i execute:
>
> prueba=> select t1.* from t1, t2 where t1.col1 = t2.col1;
> NOTICE: pg_attribute_aclmask: t1.col1 required: 0002 allowed: 0002
> NOTICE: pg_attribute_aclmask: t1.col2 required: 0002 allowed: 0002
> NOTICE: pg_attribute_aclmask: t1.col1 required: 0002 allowed: 0002
> NOTICE: pg_attribute_aclmask: t2.col1 required: 0002 allowed: 0002
> col1 | col2
> ------+------
> (0 rows)
>
> good, but if i doesn't include filter conditions:
>
> prueba=> select t1.* from t1, t2;
> NOTICE: pg_attribute_aclmask: t1.col1 required: 0002 allowed: 0002
> NOTICE: pg_attribute_aclmask: t1.col2 required: 0002 allowed: 0002
> ERROR: permission denied for relation t2
>
> is this intended?

Basically, I want to wait for Stephen's opinion.

However, it seem's to me it is a correct behavior.

ExecCheckRTEPerms() checks user's privileges on columns, when he does
not have required privileges on the table. When he has proper privileges
on all the appeared columns within the table, it is allowed.
But, when no columns are used on the table, it applies result of checks
on the table.

In this example, "rol1" does not have any privileges on relation "t1"
and "t2", but he can select "t1.col1", "t1.col2" and "t2.col1".
Since he does not have any privs on relations, column's privs are
checked in both of queries.

In the first query, he uses "col1" and "col2" for "t1" and "col1" for
"t2", and all of them are allowed to select. So, he got succeeded.
In the other query, he uses "col1" and "col2" for "t1" but no columns
for "t2", so the result of checks on relation "t2" is applied.

Stephen, could you indicate us what behavior is proper in this case?

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> ExecCheckRTEPerms() checks user's privileges on columns, when he does
> not have required privileges on the table. When he has proper privileges
> on all the appeared columns within the table, it is allowed.
> But, when no columns are used on the table, it applies result of checks
> on the table.

Surely the SQL spec tells us what to do here (and I cannot believe this
is it...)

regards, tom lane

Tom, et al,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> > ExecCheckRTEPerms() checks user's privileges on columns, when he does
> > not have required privileges on the table. When he has proper privileges
> > on all the appeared columns within the table, it is allowed.
> > But, when no columns are used on the table, it applies result of checks
> > on the table.
>
> Surely the SQL spec tells us what to do here (and I cannot believe this
> is it...)

Based on what I see in the SQL spec, we have to allow table references
like this when the user has SELECT rights on at least one column of the
table. If the column is referenced anywhere (SELECT clause, WHERE
clause, JOIN clause, through a NATURAL JOIN, etc) then the user must
have SELECT rights on the mentioned column(s).

I'm open to suggestions about how to handle this. My first thought
would be- add an entry to the cols_sel list for the RTE that is special
and indicates "any column", perhaps by using a '0' for the attrid, as is
done elsewhere. Then modify ExecCheckRTEPerms() to handle it.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> I'm open to suggestions about how to handle this. My first thought
> would be- add an entry to the cols_sel list for the RTE that is special
> and indicates "any column", perhaps by using a '0' for the attrid, as is
> done elsewhere. Then modify ExecCheckRTEPerms() to handle it.

Wouldn't it be sufficient to treat an empty cols_sel list that way?

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > I'm open to suggestions about how to handle this. My first thought
> > would be- add an entry to the cols_sel list for the RTE that is special
> > and indicates "any column", perhaps by using a '0' for the attrid, as is
> > done elsewhere. Then modify ExecCheckRTEPerms() to handle it.
>
> Wouldn't it be sufficient to treat an empty cols_sel list that way?

I've thought about it some, and yes, that sounds reasonable. I'll try
and implement it tonight and test it out.

Thanks!

Stephen

Stephen Frost wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>>> I'm open to suggestions about how to handle this. My first thought
>>> would be- add an entry to the cols_sel list for the RTE that is special
>>> and indicates "any column", perhaps by using a '0' for the attrid, as is
>>> done elsewhere. Then modify ExecCheckRTEPerms() to handle it.
>> Wouldn't it be sufficient to treat an empty cols_sel list that way?
>
> I've thought about it some, and yes, that sounds reasonable. I'll try
> and implement it tonight and test it out.

I also think it is a reasonable solution.

In addition, please note that the following case:

| postgres=# SELECT t1 FROM t1;
| t1
| ---------
| (1,aaa)
| (2,bbb)
| (3,ccc)

When we refer table-rowtype, analyzer handles its Var->varattno has
InvalidAttrNumber (=0). If SQL standard mention nothing, it is quite
natural to consider it refers whole of the user columns.

I think it can share the code to handle the above empty cols_sel cases.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> > I've thought about it some, and yes, that sounds reasonable. I'll try
> > and implement it tonight and test it out.

I've implemented this and it appears to work well.

> When we refer table-rowtype, analyzer handles its Var->varattno has
> InvalidAttrNumber (=0). If SQL standard mention nothing, it is quite
> natural to consider it refers whole of the user columns.

I've also updated the patch to handle this correctly.

I still have a dependency tracking issue that I'm trying to nail down.
I'll post an updated patch tonight regardless, however.

Thanks,

Stephen

KaiGai, Tom, all,

Attached is an updated patch which fixes Markus' dependency issue,
handles the other issues mentioned by KaiGai and Tom, has some
additional regressions tests to make sure all of that works, and has
been cleaned up to the 80-col goal (where it seemed to work well and
make sense..).

Please let me know if you have comments. Also, as is always the
case, testing by others is appreciated.

Thanks,

Stephen

On Thu, Jan 8, 2009 at 11:34 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
> Please let me know if you have comments.

a fast test seems to show that this documentation change is no longer
true ;) nice work!

--- 443,450 ----
</para>

<para>
! Currently, <productname>PostgreSQL</productname> does not recognize
! column-level SELECT privileges when a JOIN is involved.
One possible workaround is to create a view having just the desired
columns and then grant privileges to that view.
</para>
***************

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

Jamie, et al,

* Jaime Casanova (jcasanov(at)systemguards(dot)com(dot)ec) wrote:
> <para>
> ! Currently, <productname>PostgreSQL</productname> does not recognize
> ! column-level SELECT privileges when a JOIN is involved.
> One possible workaround is to create a view having just the desired
> columns and then grant privileges to that view.
> </para>

Remove this from the documentation, and:

- Other minor cleanups (thanks KaiGai)
- Added pg_dump support
- Added support for 'ALL(col1)' grant/revokes (helped with pg_dump)
- Added more regression tests
- Updated documentation accordingly

Please test, comment, etc.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> Please test, comment, etc.

A few random comments based on a very fast scan through the patch:

The RTE fields really ought to be bitmaps not integer lists. The
list representation is expensive to store, copy, etc. You could use
the same approach the HOT patch used, ie offset the indexes by
FirstLowInvalidHeapAttributeNumber (cf pull_varattnos).

Patch is desperately lacking comments surrounding the altered meaning
of ATTRIBUTE_TUPLE_SIZE, the fact that TupleDescs no longer contain
complete copies of pg_attribute rows, etc. It might be a good idea
to rename ATTRIBUTE_TUPLE_SIZE to ATTRIBUTE_TUPLE_FIXED_PART_SIZE
or something like that.

Don't like exporting allocacl() from acl.c nor having aclchk.c be so
intimate with the internal representation of ACLs. Seems like the
operations actually needed could be represented a bit more abstractly,
ie copy an ACL or merge two ACLs.

applyColumnPrivs is misnamed as it's not "applying" any privileges nor
indeed doing much of anything directly to do with privileges. It should
probably be named something more like findReferencedColumns. And what's
with the special exception for SortGroupClause!?

Actually, though, you probably shouldn't have applyColumnPrivsWalker at all.
It requires an additional traversal of the parse tree, and an additional RTE
search for each var, for no good reason. Can't we just mark the column
as referenced in make_var() and maybe a couple other places that already have
their hands on the RTE?

I don't see anything in transformDeleteStatement to handle the
fact that "DELETE ... WHERE foo = 42" requires select on foo.

In the gram.y changes, don't treat CREATE differently from the other
cases. You need a test and error in the statement execution code anyway
for the case of a privilege type inappropriately applied to columns, and
it's better to throw that very specific error message than the generic
"syntax error" that bison is going to throw for CREATE (column list).

The comment added to InsertPgAttributeTuple seems not to belong to it
(copy/paste error?)

What's the point of disallowing column privileges on a sequence? (aclcheck.c
line 800 or so) It's not nonsensical to want to restrict what someone can do
in "SELECT * FROM sequence".

pg_attribute_aclmask seems to need a rethink. I don't like the amount
of policy copied-and-pasted from pg_class_aclmask, nor the fact that
it will fail for system attributes (attnum < 0), nor the fact that it
insists on looping over the attributes even if the relation privileges
would provide what's needed. (Indeed, you shouldn't need that "merge
ACLs" operation at all -- you could be ORing a couple of bitmasks
instead, no?)

I don't find what you've done with no_priv_msg[] to be particularly
comfortable: if anyone ever tried to print an ACL_KIND_COLUMN error
with the regular code path (hardly unlikely) you'd get a core dump
due to the format wanting two %s arguments with only one supplied.
I think the best thing is for no_priv_msg[] to include just
+ gettext_noop("permission denied for column %s"),
and then make aclcheck_error_col() use its own error text rather than
pulling from the array.

Don't like naming of "Priv" node type, it's way too nonspecific.
Also, you need outfuncs.c support for it, see comment at head of
that file.

regards, tom lane

Tom, et al,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> A few random comments based on a very fast scan through the patch:

Thanks for the feedback!

> The RTE fields really ought to be bitmaps not integer lists. The
> list representation is expensive to store, copy, etc. You could use
> the same approach the HOT patch used, ie offset the indexes by
> FirstLowInvalidHeapAttributeNumber (cf pull_varattnos).

Done (was actually easier than I expected it to be).

> Patch is desperately lacking comments surrounding the altered meaning
> of ATTRIBUTE_TUPLE_SIZE, the fact that TupleDescs no longer contain
> complete copies of pg_attribute rows, etc. It might be a good idea
> to rename ATTRIBUTE_TUPLE_SIZE to ATTRIBUTE_TUPLE_FIXED_PART_SIZE
> or something like that.

Done.

> Don't like exporting allocacl() from acl.c nor having aclchk.c be so
> intimate with the internal representation of ACLs. Seems like the
> operations actually needed could be represented a bit more abstractly,
> ie copy an ACL or merge two ACLs.

Done.

> applyColumnPrivs is misnamed as it's not "applying" any privileges nor
> indeed doing much of anything directly to do with privileges. It should
> probably be named something more like findReferencedColumns. And what's
> with the special exception for SortGroupClause!?

I'm not sure what the story with SortGroupClause is.. Might just have
been a bit more difficult to do. KaiGai?

> Actually, though, you probably shouldn't have applyColumnPrivsWalker at all.
> It requires an additional traversal of the parse tree, and an additional RTE
> search for each var, for no good reason. Can't we just mark the column
> as referenced in make_var() and maybe a couple other places that already have
> their hands on the RTE?

I certainly like this idea and I'll look into it, but it might take me a
bit longer to find the appropriate places beyond make_var().

> I don't see anything in transformDeleteStatement to handle the
> fact that "DELETE ... WHERE foo = 42" requires select on foo.

I've fixed this (I believe).

> In the gram.y changes, don't treat CREATE differently from the other
> cases. You need a test and error in the statement execution code anyway
> for the case of a privilege type inappropriately applied to columns, and
> it's better to throw that very specific error message than the generic
> "syntax error" that bison is going to throw for CREATE (column list).

Done.

> The comment added to InsertPgAttributeTuple seems not to belong to it
> (copy/paste error?)

Fixed.

> What's the point of disallowing column privileges on a sequence? (aclcheck.c
> line 800 or so) It's not nonsensical to want to restrict what someone can do
> in "SELECT * FROM sequence".

I've removed the offending code but to be honest I'm a bit nervous that
there are other parts of the code that aren't expecting a sequence and
may have to be changed.

> pg_attribute_aclmask seems to need a rethink. I don't like the amount
> of policy copied-and-pasted from pg_class_aclmask, nor the fact that
> it will fail for system attributes (attnum < 0), nor the fact that it
> insists on looping over the attributes even if the relation privileges
> would provide what's needed. (Indeed, you shouldn't need that "merge
> ACLs" operation at all -- you could be ORing a couple of bitmasks
> instead, no?)

I'll have to think of the entry points for pg_attribute_aclmask. In
general, we shouldn't ever get to it if the relation privileges are
sufficient but I think it's exposed to the user at some level, where
we would be wrong to say a user doesn't have rights on the column
when they do have the appropriate table-level rights. Unfortunately,
aclmask() uses information beyond the bitmasks (who granted them,
specifically) so I don't think we can just OR the bitmasks.

With regard to looping through the attributes, I could split it up into
two functions, would that be better? A function that handles
all-attribute cases (either 'AND'd or 'OR'd together depending on what
the caller wants) could be added pretty easily and then
pg_attribute_aclmask could be reverted to just handling a single
attribute at a time (which would fix it for system attributes as
well..).

> I don't find what you've done with no_priv_msg[] to be particularly
> comfortable: if anyone ever tried to print an ACL_KIND_COLUMN error
> with the regular code path (hardly unlikely) you'd get a core dump
> due to the format wanting two %s arguments with only one supplied.
> I think the best thing is for no_priv_msg[] to include just
> + gettext_noop("permission denied for column %s"),
> and then make aclcheck_error_col() use its own error text rather than
> pulling from the array.

Done.

> Don't like naming of "Priv" node type, it's way too nonspecific.
> Also, you need outfuncs.c support for it, see comment at head of
> that file.

Done.

Updated patch attached.

Thanks!

Stephen

Tom, et al,

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> > applyColumnPrivs is misnamed as it's not "applying" any privileges nor
> > indeed doing much of anything directly to do with privileges. It should
> > probably be named something more like findReferencedColumns. And what's
> > with the special exception for SortGroupClause!?
>
> I'm not sure what the story with SortGroupClause is.. Might just have
> been a bit more difficult to do. KaiGai?

This should be resolved now, since..

> > Actually, though, you probably shouldn't have applyColumnPrivsWalker at all.
> > It requires an additional traversal of the parse tree, and an additional RTE
> > search for each var, for no good reason. Can't we just mark the column
> > as referenced in make_var() and maybe a couple other places that already have
> > their hands on the RTE?
>
> I certainly like this idea and I'll look into it, but it might take me a
> bit longer to find the appropriate places beyond make_var().

I've implemented and tested these suggested changes, and have removed
applyColumnPrivs, etc. It passes all the regression tests, which had a
variety of tests, so I'm reasonably happy with this.

> > pg_attribute_aclmask seems to need a rethink. I don't like the amount
> > of policy copied-and-pasted from pg_class_aclmask, nor the fact that
> > it will fail for system attributes (attnum < 0), nor the fact that it
> > insists on looping over the attributes even if the relation privileges
> > would provide what's needed. (Indeed, you shouldn't need that "merge
> > ACLs" operation at all -- you could be ORing a couple of bitmasks
> > instead, no?)
>
> I'll have to think of the entry points for pg_attribute_aclmask. In
> general, we shouldn't ever get to it if the relation privileges are
> sufficient but I think it's exposed to the user at some level, where
> we would be wrong to say a user doesn't have rights on the column
> when they do have the appropriate table-level rights. Unfortunately,
> aclmask() uses information beyond the bitmasks (who granted them,
> specifically) so I don't think we can just OR the bitmasks.
>
> With regard to looping through the attributes, I could split it up into
> two functions, would that be better? A function that handles
> all-attribute cases (either 'AND'd or 'OR'd together depending on what
> the caller wants) could be added pretty easily and then
> pg_attribute_aclmask could be reverted to just handling a single
> attribute at a time (which would fix it for system attributes as
> well..).

I've modified the code to handle system attributes but otherwise kept it
pretty much the same (with the loop and the special values to indicate
how to handle it). I looked at creating a seperate function and it
really seemed like it would be alot of code duplication.. It might
still be possible to refactor it if you'd really like. I'm open to
other thoughts or suggestions you have based on my comments above.

Updated patch attached.

Thanks!

Stephen

Stephen,

The revised patch can reproduce the original matter, as follows:
----------------
postgres=# CREATE TABLE t1 (a int, b text);
CREATE TABLE
postgres=# CREATE TABLE t2 (x int, y text);
CREATE TABLE
postgres=# GRANT select(a) on t1 to ymj;
GRANT
postgres=# GRANT select(x,y) on t2 TO ymj;
GRANT
postgres=# \c - ymj
psql (8.4devel)
You are now connected to database "postgres" as user "ymj".
postgres=> SELECT a, y FROM t1 JOIN t2 ON a = x;
NOTICE: make_var: attribute 1 added on rte(relid=16398, rtekind=0)
NOTICE: make_var: attribute 1 added on rte(relid=16404, rtekind=0)
NOTICE: make_var: attribute 1 added on rte(relid=0, rtekind=2)
NOTICE: make_var: attribute 4 added on rte(relid=0, rtekind=2)
NOTICE: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
NOTICE: pg_attribute_aclmask: t1.b required: 0002 allowed: 0000
ERROR: permission denied for relation t1
----------------

I think it is not an essentiality whether it walks on query tree, or not.
So, I also suppose putting this code on make_var().
However, it does not care the case when rte->relkind == RTE_JOIN, so
it requires column-level privileges on whole of columns including
unrefered ones.

My suggestiong is case separations.
If rte->relkind == RTE_RELATION, it can keep current behavior, as is.
If rte->relkind == RTE_JOIN, it need to find the source relation
recursively and marks required column. Please note that the source
relation can be RTE_JOIN or others.
Elsewhere, we don't need to care anymore.

Thanks,

Stephen Frost wrote:
> Tom, et al,
>
> * Stephen Frost (sfrost(at)snowman(dot)net) wrote:
>>> applyColumnPrivs is misnamed as it's not "applying" any privileges nor
>>> indeed doing much of anything directly to do with privileges. It should
>>> probably be named something more like findReferencedColumns. And what's
>>> with the special exception for SortGroupClause!?
>> I'm not sure what the story with SortGroupClause is.. Might just have
>> been a bit more difficult to do. KaiGai?
>
> This should be resolved now, since..
>
>>> Actually, though, you probably shouldn't have applyColumnPrivsWalker at all.
>>> It requires an additional traversal of the parse tree, and an additional RTE
>>> search for each var, for no good reason. Can't we just mark the column
>>> as referenced in make_var() and maybe a couple other places that already have
>>> their hands on the RTE?
>> I certainly like this idea and I'll look into it, but it might take me a
>> bit longer to find the appropriate places beyond make_var().
>
> I've implemented and tested these suggested changes, and have removed
> applyColumnPrivs, etc. It passes all the regression tests, which had a
> variety of tests, so I'm reasonably happy with this.
>
>>> pg_attribute_aclmask seems to need a rethink. I don't like the amount
>>> of policy copied-and-pasted from pg_class_aclmask, nor the fact that
>>> it will fail for system attributes (attnum < 0), nor the fact that it
>>> insists on looping over the attributes even if the relation privileges
>>> would provide what's needed. (Indeed, you shouldn't need that "merge
>>> ACLs" operation at all -- you could be ORing a couple of bitmasks
>>> instead, no?)
>> I'll have to think of the entry points for pg_attribute_aclmask. In
>> general, we shouldn't ever get to it if the relation privileges are
>> sufficient but I think it's exposed to the user at some level, where
>> we would be wrong to say a user doesn't have rights on the column
>> when they do have the appropriate table-level rights. Unfortunately,
>> aclmask() uses information beyond the bitmasks (who granted them,
>> specifically) so I don't think we can just OR the bitmasks.
>>
>> With regard to looping through the attributes, I could split it up into
>> two functions, would that be better? A function that handles
>> all-attribute cases (either 'AND'd or 'OR'd together depending on what
>> the caller wants) could be added pretty easily and then
>> pg_attribute_aclmask could be reverted to just handling a single
>> attribute at a time (which would fix it for system attributes as
>> well..).
>
> I've modified the code to handle system attributes but otherwise kept it
> pretty much the same (with the loop and the special values to indicate
> how to handle it). I looked at creating a seperate function and it
> really seemed like it would be alot of code duplication.. It might
> still be possible to refactor it if you'd really like. I'm open to
> other thoughts or suggestions you have based on my comments above.
>
> Updated patch attached.
>
> Thanks!
>
> Stephen

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

I can find a matter on JOIN.

Please make sure the following function invocation chain:

transformSelectStmt()
-> transformFromClause()
-> transformFromClauseItem()
-> expandRTE(), if JoinExpr
-> expandRelation(), if rte->rtekind == RTE_RELATION
-> expandTupleDesc()

expandTupleDesc() set rte->cols_sel for all the user columns.
It seems to me unexpected behavior.

Maybe, the point to mark rte->cols_sel has to be moved.
However, I wonder whether it is actually proper manner to put
a code depending its invocation context on existing parser.

I can understand Tom's concern, but does it make maintenance
difficulty in the future version?
For example, when a new future invokes expandRTE(), it also
polutes rte->cols_sel implicitly.
I reconsidered the previous walker implementation independent
from other parser codes is more simple and better.

Stephen, Tom, what is your opinion?

Thanks,

KaiGai Kohei wrote:
> Stephen,
>
> The revised patch can reproduce the original matter, as follows:
> ----------------
> postgres=# CREATE TABLE t1 (a int, b text);
> CREATE TABLE
> postgres=# CREATE TABLE t2 (x int, y text);
> CREATE TABLE
> postgres=# GRANT select(a) on t1 to ymj;
> GRANT
> postgres=# GRANT select(x,y) on t2 TO ymj;
> GRANT
> postgres=# \c - ymj
> psql (8.4devel)
> You are now connected to database "postgres" as user "ymj".
> postgres=> SELECT a, y FROM t1 JOIN t2 ON a = x;
> NOTICE: make_var: attribute 1 added on rte(relid=16398, rtekind=0)
> NOTICE: make_var: attribute 1 added on rte(relid=16404, rtekind=0)
> NOTICE: make_var: attribute 1 added on rte(relid=0, rtekind=2)
> NOTICE: make_var: attribute 4 added on rte(relid=0, rtekind=2)
> NOTICE: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
> NOTICE: pg_attribute_aclmask: t1.b required: 0002 allowed: 0000
> ERROR: permission denied for relation t1
> ----------------
>
> I think it is not an essentiality whether it walks on query tree, or not.
> So, I also suppose putting this code on make_var().
> However, it does not care the case when rte->relkind == RTE_JOIN, so
> it requires column-level privileges on whole of columns including
> unrefered ones.
>
> My suggestiong is case separations.
> If rte->relkind == RTE_RELATION, it can keep current behavior, as is.
> If rte->relkind == RTE_JOIN, it need to find the source relation
> recursively and marks required column. Please note that the source
> relation can be RTE_JOIN or others.
> Elsewhere, we don't need to care anymore.
>
> Thanks,
>
> Stephen Frost wrote:
>> Tom, et al,
>>
>> * Stephen Frost (sfrost(at)snowman(dot)net) wrote:
>>>> applyColumnPrivs is misnamed as it's not "applying" any privileges nor
>>>> indeed doing much of anything directly to do with privileges. It should
>>>> probably be named something more like findReferencedColumns. And what's
>>>> with the special exception for SortGroupClause!?
>>> I'm not sure what the story with SortGroupClause is.. Might just have
>>> been a bit more difficult to do. KaiGai?
>> This should be resolved now, since..
>>
>>>> Actually, though, you probably shouldn't have applyColumnPrivsWalker at all.
>>>> It requires an additional traversal of the parse tree, and an additional RTE
>>>> search for each var, for no good reason. Can't we just mark the column
>>>> as referenced in make_var() and maybe a couple other places that already have
>>>> their hands on the RTE?
>>> I certainly like this idea and I'll look into it, but it might take me a
>>> bit longer to find the appropriate places beyond make_var().
>> I've implemented and tested these suggested changes, and have removed
>> applyColumnPrivs, etc. It passes all the regression tests, which had a
>> variety of tests, so I'm reasonably happy with this.
>>
>>>> pg_attribute_aclmask seems to need a rethink. I don't like the amount
>>>> of policy copied-and-pasted from pg_class_aclmask, nor the fact that
>>>> it will fail for system attributes (attnum < 0), nor the fact that it
>>>> insists on looping over the attributes even if the relation privileges
>>>> would provide what's needed. (Indeed, you shouldn't need that "merge
>>>> ACLs" operation at all -- you could be ORing a couple of bitmasks
>>>> instead, no?)
>>> I'll have to think of the entry points for pg_attribute_aclmask. In
>>> general, we shouldn't ever get to it if the relation privileges are
>>> sufficient but I think it's exposed to the user at some level, where
>>> we would be wrong to say a user doesn't have rights on the column
>>> when they do have the appropriate table-level rights. Unfortunately,
>>> aclmask() uses information beyond the bitmasks (who granted them,
>>> specifically) so I don't think we can just OR the bitmasks.
>>>
>>> With regard to looping through the attributes, I could split it up into
>>> two functions, would that be better? A function that handles
>>> all-attribute cases (either 'AND'd or 'OR'd together depending on what
>>> the caller wants) could be added pretty easily and then
>>> pg_attribute_aclmask could be reverted to just handling a single
>>> attribute at a time (which would fix it for system attributes as
>>> well..).
>> I've modified the code to handle system attributes but otherwise kept it
>> pretty much the same (with the loop and the special values to indicate
>> how to handle it). I looked at creating a seperate function and it
>> really seemed like it would be alot of code duplication.. It might
>> still be possible to refactor it if you'd really like. I'm open to
>> other thoughts or suggestions you have based on my comments above.
>>
>> Updated patch attached.
>>
>> Thanks!
>>
>> Stephen
>
>

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> I reconsidered the previous walker implementation independent
> from other parser codes is more simple and better.

And slower, and equally subject to this bug, so I'm not convinced.

> Stephen, Tom, what is your opinion?

I'm thinking make_var is not the place to do this. The places that are
supposed to be taking care of permissions are the ones that do this:

/* Require read access --- see comments in setTargetTable() */
rte->requiredPerms |= ACL_SELECT;

It's possible that we've missed some --- in particular, right at the
moment I am not sure that whole-row Vars are handled properly.
And maybe we could refactor a little bit to save some code.
But those are basically the same places that ought to be adding
bits to the column bitmaps.

regards, tom lane

Tom Lane wrote:
> I'm thinking make_var is not the place to do this. The places that are
> supposed to be taking care of permissions are the ones that do this:
>
> /* Require read access --- see comments in setTargetTable() */
> rte->requiredPerms |= ACL_SELECT;

Indeed, it seems to me appropriate policy, because CLP feature is
a part of SQL standard access control model.

The rte->requiredPerms is set on the following places:

(1) transformLockingClause() and markQueryForLocking()
It adds ACL_SELECT_FOR_UPDATE for listed relations.
In this case, column-level priv feature checks ACL_UPDATE for each
columns on rte->cols_sel, doesn't it?
So, I think it is unnecessary to care about anything here.

(2) setTargetTable()
It is invoked from transformInsertStmt(), transformUpdateStmt() and
transformDeleteStmt(). I thnk it is a proper place to set rte->cols_mod,
but the caller does not initialize Query->targetList yet, when it is
invoked.
So, I think we need put a function call to set cols_mod on caller side
based on Query->resultRelation and Query->targetList.

(3) scanRTEForColumn()
Stephen's original one patched here, but it does not handle RTE_JOIN
well, so it made a matter on table-joins.
Please revert a code to mark rte->cols_sel, with proper handling for
RTE_JOIN cases.

(4) addRangeTableEntry()
Purpose of the function is to translate RangeVar node to RangeTblEntry
node listed on FROM clause and so on.
I think we don't need to add any column references here.
When the translated relation used for column-references, it is a case
that rte->cols_sel is empty.

(5) addRangeTableEntryForRelation()
It is similar to addRangeTableEntry().
I think we don't need to add something here.

(6) ExpandColumnRefStar() and ExpandAllTables()
They invoke expandRelAttrs() to handle "SELECT * FROM t;" case.
I think here is a proper point to mark refered columns.
Please note that here is no guarantee that given RangeTblEntry has
RTE_RELATION.

Thus, we need the following reworking in my opinion.
(a) Implement a function to set rte->cols_sel and rte->cols_mod correctly,
even if the given rte has RTE_JOIN.
(b) Put a function to set rte->cols_mod on the caller of setTargetTable().
(c) Put a function to set rte->cols_sel on scanRTEForColumn(), expandRelAttrs()
and transformWholeRowRef().

> It's possible that we've missed some --- in particular, right at the
> moment I am not sure that whole-row Vars are handled properly.

Is transformWholeRowRef() a proper place to handle it?
If given RangeTblEntry has RTE_JOIN, it has to resolve it and set
its source's cols_sel.

> And maybe we could refactor a little bit to save some code.
> But those are basically the same places that ought to be adding
> bits to the column bitmaps.

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

The attached patch is proof of the concept.

It can be applied on the latest CVS HEAD with colprivs_2009011001.diff.gz.
- It unpatches unnecessary updates at parser/parse_expr.c, parser/parse_node.c
and parser/parse_relation.c.
- It adds markColumnForSelectPriv() to mark proper rte->cols_sel for
the given Var node. It is invoked from scanRTEForColumn(), expandRelAttrs()
and transformWholeRowRef().
- The markColumnForSelectPriv() uses walker function internally, because
there is no guarantee all the entities within RangeTblEntry->joinaliasvars
are Var type node. However, it is used to walks on a single Var node, not
whole of Query tree, so I think its cost is small enough.

==== Results:
postgres=# CREATE TABLE t1 (a int, b text, c bool);
CREATE TABLE
postgres=# CREATE TABLE t2 (x int, y text, z bool);
CREATE TABLE
postgres=# GRANT SELECT (a,b) ON t1 TO ymj;
GRANT
postgres=# GRANT SELECT (x,y) ON t2 TO ymj;
GRANT
postgres=# \c - ymj
psql (8.4devel)
You are now connected to database "postgres" as user "ymj".
postgres=> SELECT a,b FROM t1;
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.b required: 0002 allowed: 0002
a | b
---+---
(0 rows)

postgres=> SELECT * FROM t1;
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.b required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.c required: 0002 allowed: 0000
ERROR: permission denied for relation t1

postgres=> SELECT a FROM t1 order by c;
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.c required: 0002 allowed: 0000
ERROR: permission denied for relation t1

postgres=> SELECT t1 FROM t1;
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.b required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.c required: 0002 allowed: 0000
ERROR: permission denied for relation t1

postgres=> SELECT 'abcd' FROM t1;
DEBUG: pg_attribute_aclmask: t1.tableoid required: 0002 allowed: 0000
DEBUG: pg_attribute_aclmask: t1.cmax required: 0002 allowed: 0000
DEBUG: pg_attribute_aclmask: t1.xmax required: 0002 allowed: 0000
DEBUG: pg_attribute_aclmask: t1.cmin required: 0002 allowed: 0000
DEBUG: pg_attribute_aclmask: t1.xmin required: 0002 allowed: 0000
DEBUG: pg_attribute_aclmask: t1.ctid required: 0002 allowed: 0000
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
?column?
----------
(0 rows)

postgres=> SELECT b,y FROM t1 JOIN t2 ON a=x;
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.b required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t2.x required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t2.y required: 0002 allowed: 0002
b | y
---+---
(0 rows)

postgres=> SELECT b, (SELECT y FROM t2 WHERE x=a) FROM t1;
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.b required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t2.x required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t2.y required: 0002 allowed: 0002
b | ?column?
---+----------
(0 rows)

Thanks,

KaiGai Kohei wrote:
> Tom Lane wrote:
>> I'm thinking make_var is not the place to do this. The places that are
>> supposed to be taking care of permissions are the ones that do this:
>>
>> /* Require read access --- see comments in setTargetTable() */
>> rte->requiredPerms |= ACL_SELECT;
>
> Indeed, it seems to me appropriate policy, because CLP feature is
> a part of SQL standard access control model.
>
>
> The rte->requiredPerms is set on the following places:
>
> (1) transformLockingClause() and markQueryForLocking()
> It adds ACL_SELECT_FOR_UPDATE for listed relations.
> In this case, column-level priv feature checks ACL_UPDATE for each
> columns on rte->cols_sel, doesn't it?
> So, I think it is unnecessary to care about anything here.
>
> (2) setTargetTable()
> It is invoked from transformInsertStmt(), transformUpdateStmt() and
> transformDeleteStmt(). I thnk it is a proper place to set rte->cols_mod,
> but the caller does not initialize Query->targetList yet, when it is
> invoked.
> So, I think we need put a function call to set cols_mod on caller side
> based on Query->resultRelation and Query->targetList.
>
> (3) scanRTEForColumn()
> Stephen's original one patched here, but it does not handle RTE_JOIN
> well, so it made a matter on table-joins.
> Please revert a code to mark rte->cols_sel, with proper handling for
> RTE_JOIN cases.
>
> (4) addRangeTableEntry()
> Purpose of the function is to translate RangeVar node to RangeTblEntry
> node listed on FROM clause and so on.
> I think we don't need to add any column references here.
> When the translated relation used for column-references, it is a case
> that rte->cols_sel is empty.
>
> (5) addRangeTableEntryForRelation()
> It is similar to addRangeTableEntry().
> I think we don't need to add something here.
>
> (6) ExpandColumnRefStar() and ExpandAllTables()
> They invoke expandRelAttrs() to handle "SELECT * FROM t;" case.
> I think here is a proper point to mark refered columns.
> Please note that here is no guarantee that given RangeTblEntry has
> RTE_RELATION.
>
> Thus, we need the following reworking in my opinion.
> (a) Implement a function to set rte->cols_sel and rte->cols_mod correctly,
> even if the given rte has RTE_JOIN.
> (b) Put a function to set rte->cols_mod on the caller of setTargetTable().
> (c) Put a function to set rte->cols_sel on scanRTEForColumn(), expandRelAttrs()
> and transformWholeRowRef().
>
>> It's possible that we've missed some --- in particular, right at the
>> moment I am not sure that whole-row Vars are handled properly.
>
> Is transformWholeRowRef() a proper place to handle it?
> If given RangeTblEntry has RTE_JOIN, it has to resolve it and set
> its source's cols_sel.
>
>> And maybe we could refactor a little bit to save some code.
>> But those are basically the same places that ought to be adding
>> bits to the column bitmaps.
>

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Tom, er al,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> I'm thinking make_var is not the place to do this. The places that are
> supposed to be taking care of permissions are the ones that do this:
>
> /* Require read access --- see comments in setTargetTable() */
> rte->requiredPerms |= ACL_SELECT;

Argh. That's what I had started out with, but I couldn't figure out how
to handle the JOIN case. I'm a bit mystified by what KaiGai found
though and havn't had a chance to look at it yet, but I thought I had
tested the JOIN cases and I had added them to the regression tests.
Guess I missed something.

> It's possible that we've missed some --- in particular, right at the
> moment I am not sure that whole-row Vars are handled properly.

I added specific regression test for whole-row Vars, so I'd be concerned
if something isn't working there.

> And maybe we could refactor a little bit to save some code.
> But those are basically the same places that ought to be adding
> bits to the column bitmaps.

I tend to agree, provided we can handle JOIN clauses sanely at those
places. I'll try and look at KaiGai's patch today and provide feedback.

Thanks,

Stephen

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> The attached patch is proof of the concept.

Thanks!

> It can be applied on the latest CVS HEAD with colprivs_2009011001.diff.gz.
> - It unpatches unnecessary updates at parser/parse_expr.c, parser/parse_node.c
> and parser/parse_relation.c.
> - It adds markColumnForSelectPriv() to mark proper rte->cols_sel for
> the given Var node. It is invoked from scanRTEForColumn(), expandRelAttrs()
> and transformWholeRowRef().

Ok.

> - The markColumnForSelectPriv() uses walker function internally, because
> there is no guarantee all the entities within RangeTblEntry->joinaliasvars
> are Var type node. However, it is used to walks on a single Var node, not
> whole of Query tree, so I think its cost is small enough.

If any of them aren't Vars, then wouldn't it be a subselect or similar,
in which case the non-join pieces would be handled through
scanRTEForColumn, etc? What I'm getting at here is- Are you sure that
the walk down the non-Var's in joinaliasvars is necessary? Have you
tried testing without it?

Other comments- you dropped the comments I had regarding the offsetting
when using the bitmap, and it would really be helpful if you could add
these cases to the regression tests so that myself and others working on
this patch and the code involved make sure to not break the semantics
which are required.

Thnaks!

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> - The markColumnForSelectPriv() uses walker function internally, because
>> there is no guarantee all the entities within RangeTblEntry->joinaliasvars
>> are Var type node.

> If any of them aren't Vars, then wouldn't it be a subselect or similar,

They're either Vars or COALESCEs of two Vars (the latter only for FULL
JOIN USING join columns). There are no other possibilities at parse
time. It might be best to use expression_tree_walker to avoid
explicitly handling the COALESCE case, but it's probably not going to be
any shorter.

You will need recursion of some type there, since it's entirely
possible for those Vars to point to another JOIN RTE.

BTW, another corner case that I'm not sure gets handled right is
that the join columns in JOIN USING or NATURAL JOIN need to be marked
as requiring ACL_SELECT. (Or so I'd expect anyway; I didn't run through
the SQL spec looking for chapter and verse on that.) I forget whether
we take any shortcuts in setting up the implied join condition
expressions, but if we do then some extra code might be needed. This
would be a good place for a regression test in any case.

regards, tom lane

Stephen Frost <sfrost(at)snowman(dot)net> writes:
>> It's possible that we've missed some --- in particular, right at the
>> moment I am not sure that whole-row Vars are handled properly.

> I added specific regression test for whole-row Vars, so I'd be concerned
> if something isn't working there.

What I see being tested is SELECT *, which is a different animal
entirely. As required by spec, SELECT * is expanded to a list of
ordinary variables at parse time and then it's not really a special
case anymore. A true whole-row variable only occurs when you have
something like

create function myfunc(mytable) ...

select myfunc(mytable.*) from mytable ...

This is different from the *-expansion case in that you get just
a single Var with attno 0 in the resulting parse tree. And the
reason for the different representation is that the semantics
are different. If the user later does ALTER TABLE ADD COLUMN,
the whole-row Var now implicitly includes that column too, whereas
existing views defined with SELECT * do not (read the spec).

Because of this action-at-a-distance in terms of what columns are
implicitly referenced, I think that the only feasible implementation is
to carry the "reference to column 0" notation in cols_sel right through
to execMain, and have execMain understand that as demanding select
rights on all currently existing non-dropped non-system columns.
I have not yet checked to see if that's what actually happens;
but it's certainly not being exercised in the added regression tests.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> What I see being tested is SELECT *, which is a different animal
> entirely. As required by spec, SELECT * is expanded to a list of
> ordinary variables at parse time and then it's not really a special
> case anymore. A true whole-row variable only occurs when you have
> something like
>
> create function myfunc(mytable) ...
>
> select myfunc(mytable.*) from mytable ...

Wouldn't this test cover those?

SELECT atest5 FROM atest5; -- fail

Which I added after KaiGai brought it up.

> This is different from the *-expansion case in that you get just
> a single Var with attno 0 in the resulting parse tree.

Right, that's part of the reason that pg_attribute_aclmask has the for
loop through all of the columns- because if an attno of 0 is passed in,
it has to check that the requestor has rights on *all* of the columns.

> Because of this action-at-a-distance in terms of what columns are
> implicitly referenced, I think that the only feasible implementation is
> to carry the "reference to column 0" notation in cols_sel right through
> to execMain, and have execMain understand that as demanding select
> rights on all currently existing non-dropped non-system columns.
> I have not yet checked to see if that's what actually happens;
> but it's certainly not being exercised in the added regression tests.

Right, that's what happens because execMain passes the zero attrno down
into pg_attribute_aclmask which then checks all columns. I could move
that looping logic up into execMain if you'd rather (along with the
special case for 'any column rights') but then I'd have to get the set
of columns which exist in the table referenced by the RTE in execMain.
Dunno if that'd be hard or not.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> What I see being tested is SELECT *, which is a different animal
>> entirely.

> Wouldn't this test cover those?
> SELECT atest5 FROM atest5; -- fail

Oh, I didn't see that. Still, this doesn't test whether the behavior
is correct with respect to ADD/DROP COLUMN --- if that were implemented
like SELECT * you'd not see any change in the regression result.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> >> What I see being tested is SELECT *, which is a different animal
> >> entirely.
>
> > Wouldn't this test cover those?
> > SELECT atest5 FROM atest5; -- fail
>
> Oh, I didn't see that. Still, this doesn't test whether the behavior
> is correct with respect to ADD/DROP COLUMN --- if that were implemented
> like SELECT * you'd not see any change in the regression result.

Hrm. If a column is added and you're not granted SELECT rights on it
(even if you have access to all the originals), it should start failing
for you. I can certainly add a test case to cover that though. Or is
that not the behavior you're expecting?

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> Oh, I didn't see that. Still, this doesn't test whether the behavior
>> is correct with respect to ADD/DROP COLUMN --- if that were implemented
>> like SELECT * you'd not see any change in the regression result.

> Hrm. If a column is added and you're not granted SELECT rights on it
> (even if you have access to all the originals), it should start failing
> for you. I can certainly add a test case to cover that though. Or is
> that not the behavior you're expecting?

Right, and conversely dropping the last non-granted column should make
it work.

regards, tom lane

Stephen Frost wrote:
> Tom, er al,
>
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> I'm thinking make_var is not the place to do this. The places that are
>> supposed to be taking care of permissions are the ones that do this:
>>
>> /* Require read access --- see comments in setTargetTable() */
>> rte->requiredPerms |= ACL_SELECT;
>
> Argh. That's what I had started out with, but I couldn't figure out how
> to handle the JOIN case. I'm a bit mystified by what KaiGai found
> though and havn't had a chance to look at it yet, but I thought I had
> tested the JOIN cases and I had added them to the regression tests.
> Guess I missed something.

It seems to me you didn't add "success cases" for JOINs.
The previous patch tries to check privilege for each columns within
JOIN'ed tables unexpectedly, so the test case always fails.

*** src/test/regress/sql/privileges.sql 4 Nov 2008 00:57:19 -0000 1.22
--- src/test/regress/sql/privileges.sql 10 Jan 2009 19:10:19 -0000
:
+ SET SESSION AUTHORIZATION regressuser4;
+ SELECT * FROM atest5; -- fail
+ SELECT one FROM atest5; -- ok
+ SELECT two FROM atest5; -- fail
+ SELECT atest5 FROM atest5; -- fail
+ SELECT 1 FROM atest5; -- ok
+ SELECT 1 FROM atest5 JOIN atest5 USING (two); -- fail
+ SELECT 1 FROM atest5 WHERE two = 2; -- fail
+ SELECT * FROM atest1, atest5; -- fail
+ SELECT atest1.* FROM atest1, atest5; -- ok
+ SELECT atest1.*,atest5.one FROM atest1, atest5; -- ok
+ SELECT atest1.*,atest5.one FROM atest1 JOIN atest5 ON (atest1.a = atest5.two); -- fail
+ SELECT one, two FROM atest5; -- fail
+
:

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> It seems to me you didn't add "success cases" for JOINs.
> The previous patch tries to check privilege for each columns within
> JOIN'ed tables unexpectedly, so the test case always fails.

Right, I realized that when I went back and looked at it. I'll add some
other tests.

Thanks!

Stephen

Tom Lane wrote:
> BTW, another corner case that I'm not sure gets handled right is
> that the join columns in JOIN USING or NATURAL JOIN need to be marked
> as requiring ACL_SELECT. (Or so I'd expect anyway; I didn't run through
> the SQL spec looking for chapter and verse on that.) I forget whether
> we take any shortcuts in setting up the implied join condition
> expressions, but if we do then some extra code might be needed. This
> would be a good place for a regression test in any case.

It indeed needs special care.

The attached patch put invocations of markColumnForSelectPriv()
at transformJoinUsingClause() to mark those columns are used.

-- Results:
postgres=# CREATE TABLE t1 (a int, b int, c int, x text);
CREATE TABLE
postgres=# CREATE TABLE t2 (a int, b int, c int, y text);
CREATE TABLE
postgres=# GRANT select(a,b,x) ON t1 TO ymj;
GRANT
postgres=# GRANT select(a,c,y) ON t2 TO ymj;
GRANT
postgres=# \c - ymj
psql (8.4devel)
You are now connected to database "postgres" as user "ymj".
postgres=> SELECT x, y FROM t1 NATURAL JOIN t2;
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.b required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.c required: 0002 allowed: 0000
ERROR: permission denied for relation t1

postgres=> SELECT x, y FROM t1 JOIN t2 USING (a,b);
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.b required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.x required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t2.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t2.b required: 0002 allowed: 0000
ERROR: permission denied for relation t2

postgres=> SELECT x, y FROM t1 JOIN t2 USING (a,c);
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.c required: 0002 allowed: 0000
ERROR: permission denied for relation t1

postgres=> SELECT x, y FROM t1 JOIN t2 USING (a);
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.x required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t2.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t2.y required: 0002 allowed: 0002
x | y
---+---
(0 rows)

postgres=> \c - kaigai
psql (8.4devel)
You are now connected to database "postgres" as user "kaigai".
postgres=# ALTER TABLE t1 DROP COLUMN c;
ALTER TABLE
postgres=# ALTER TABLE t2 DROP COLUMN b;
ALTER TABLE

postgres=# \c - ymj
psql (8.4devel)
You are now connected to database "postgres" as user "ymj".
postgres=> SELECT x, y FROM t1 NATURAL JOIN t2;
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t1.x required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t2.a required: 0002 allowed: 0002
DEBUG: pg_attribute_aclmask: t2.y required: 0002 allowed: 0002
x | y
---+---
(0 rows)

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> The attached patch put invocations of markColumnForSelectPriv()
> at transformJoinUsingClause() to mark those columns are used.

Thanks for the update!

Attached is a patch which:

- incorporates KaiGai's latest patches to deal with JOINs and
NATURAL JOINs

- adds regression tests following Tom's suggestion to check
whole-row vars in the face of column add/deletes

- adds regression tests for NATURAL JOIN and successful JOINs
with table sub-sets

- reworks pg_attribute_aclmask() to remove the looping component

- adds a new pg_attribute_aclcheck_all() to handle the ANY/ALL
needs of execMain and the looping

- removes special handling of system columns, they can still be
granted/revoked, but they won't be included in ANY/ALL tests and a
table-wide REVOKE won't affect them. After thinking about it for a
while, I felt this was the most sensible compromise between code
complexity, following the SQL spec, and user freedom.

- split out adding column revokes for table-level commands into a
add_col_revokes function to clean up ExecGrant_Relation a bit.

- when handling table-level revokes, skips over columns which do not
have an ACL defined, since it clearly has no effect except to force
creation of a default ACL that's just clutter.

Comments, testing, etc, most appreciated!

Thanks,

Stephen

Stephen, I could find a strange behavior unfortunatelly. :-)
(But it is obvious one, don't worry.)

postgres=# CREATE TABLE t1 (a int, b int, c int);
CREATE TABLE
postgres=# ALTER TABLE t1 DROP COLUMN c;
ALTER TABLE
postgres=# \c - ymj
psql (8.4devel)
You are now connected to database "postgres" as user "ymj".
postgres=> SELECT 1 FROM t1;
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0000
DEBUG: pg_attribute_aclmask: t1.b required: 0002 allowed: 0000
?column?
----------
(0 rows)

It is a case to be failed because 'ymj' has no privileges on t1
and its columns, but it does not raise any error.

In this case, t1 has three columns but one of them has already dropped.

Your pg_attribute_aclcheck_all() tries to check all the general columns
on the given relation, and it returns ACLCHECK_OK if one of them are
allowed and (how == ACLMASK_ANY).

+ AclResult
+ pg_attribute_aclcheck_all(Oid table_oid, Oid roleid, AclMode mode,
+ AclMaskHow how)
+ {
:
+ for (curr_att = 1; curr_att <= nattrs; curr_att++)
+ {
+ if (pg_attribute_aclmask(table_oid,
+ curr_att, roleid, mode, ACLMASK_ANY) != 0)
+ {
+ if (how == ACLMASK_ANY)
+ return ACLCHECK_OK;
+ }
+ else
+ {
+ if (how == ACLMASK_ALL)
+ return ACLCHECK_NO_PRIV;
+ }
+ }

But pg_attribute_aclmask() returns the required privileges set as is,
if target attribute has been already dropped, then pg_attribute_aclcheck_all()
considers it a column is allowed to select at least.

+ AclMode
+ pg_attribute_aclmask(Oid table_oid, AttrNumber att_number, Oid roleid,
+ AclMode mask, AclMaskHow how)
+ {
:
+ /* Skip dropped columns */
+ if (((Form_pg_attribute) GETSTRUCT(attTuple))->attisdropped)
+ {
+ /* if we have a detoasted copy, free it */
+ if (relacl && (Pointer) relacl != DatumGetPointer(relaclDatum))
+ pfree(relacl);
+
+ ReleaseSysCache(attTuple);
+ ReleaseSysCache(classTuple);
+
+ return mask;
+ }

I think pg_attribute_aclcheck_all() should skip dropped columns,
even if it need a finding-up system cache once more.
And, pg_attribute_aclmask() should raise an error for a request
to dropped column, as if it could not be found.

BTW, what is the official reviewer's opinion?
It seems to me most of the issues on column-level privileges are
resolved, so it is almost ready for getting merged.

Thanks,

Stephen Frost wrote:
> KaiGai,
>
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> The attached patch put invocations of markColumnForSelectPriv()
>> at transformJoinUsingClause() to mark those columns are used.
>
> Thanks for the update!
>
> Attached is a patch which:
>
> - incorporates KaiGai's latest patches to deal with JOINs and
> NATURAL JOINs
>
> - adds regression tests following Tom's suggestion to check
> whole-row vars in the face of column add/deletes
>
> - adds regression tests for NATURAL JOIN and successful JOINs
> with table sub-sets
>
> - reworks pg_attribute_aclmask() to remove the looping component
>
> - adds a new pg_attribute_aclcheck_all() to handle the ANY/ALL
> needs of execMain and the looping
>
> - removes special handling of system columns, they can still be
> granted/revoked, but they won't be included in ANY/ALL tests and a
> table-wide REVOKE won't affect them. After thinking about it for a
> while, I felt this was the most sensible compromise between code
> complexity, following the SQL spec, and user freedom.
>
> - split out adding column revokes for table-level commands into a
> add_col_revokes function to clean up ExecGrant_Relation a bit.
>
> - when handling table-level revokes, skips over columns which do not
> have an ACL defined, since it clearly has no effect except to force
> creation of a default ACL that's just clutter.
>
> Comments, testing, etc, most appreciated!
>
> Thanks,
>
> Stephen

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> Stephen, I could find a strange behavior unfortunatelly. :-)

Glad you're finding these, honestly. Better to find them and deal with
them now than after a release.

> It is a case to be failed because 'ymj' has no privileges on t1
> and its columns, but it does not raise any error.
>
> In this case, t1 has three columns but one of them has already dropped.
>
> Your pg_attribute_aclcheck_all() tries to check all the general columns
> on the given relation, and it returns ACLCHECK_OK if one of them are
> allowed and (how == ACLMASK_ANY).

Yeah, I'm not too suprised at that, honestly, it was awkward to deal
with dropped columns in pg_attribute_aclmask().

> I think pg_attribute_aclcheck_all() should skip dropped columns,
> even if it need a finding-up system cache once more.
> And, pg_attribute_aclmask() should raise an error for a request
> to dropped column, as if it could not be found.

I've implemented these changes, and updated the regression tests to
check for this. Updated patch is attached.

> BTW, what is the official reviewer's opinion?
> It seems to me most of the issues on column-level privileges are
> resolved, so it is almost ready for getting merged.

I tend to doubt Tom's had a chance to review it again yet, which is
fine, though I'm certainly hopeful the recent focus and our combined
efforts mean this patch can be included for 8.4. My personal opinion is
that it's ready for beta testing (which is kind of what this feels like
we're doing here) and barring any serious issues found during testing is
good to go for inclusion.

As for areas where there could still be some improvment, I'd love to
hear your thoughts and opinions (and others!) on how column-level
privileges are handled in ExecuteGrantStmt and into ExecGrant_Relation
and how we might improve it. I don't like the nested for() loops in
ExecuteGrantStmt, but I don't see any easy way to resolve that and keep
the SQL-required syntax. As for ExecGrant_Relation, it'd be nice if we
could shorten it somehow by either combining the relation and column
level handling into a single piece of code, or maybe refactoring it into
seperate functions which could be called from both pieces..

For both of these, I'm not sure it's really practical or would end up
being better than what's there, but that's what I'd look at next with
this patch and how it might be improved.

Thanks!

Stephen

>> BTW, what is the official reviewer's opinion?
>> It seems to me most of the issues on column-level privileges are
>> resolved, so it is almost ready for getting merged.
>
> I tend to doubt Tom's had a chance to review it again yet, which is
> fine, though I'm certainly hopeful the recent focus and our combined
> efforts mean this patch can be included for 8.4. My personal opinion is
> that it's ready for beta testing (which is kind of what this feels like
> we're doing here) and barring any serious issues found during testing is
> good to go for inclusion.
>
> As for areas where there could still be some improvment, I'd love to
> hear your thoughts and opinions (and others!) on how column-level
> privileges are handled in ExecuteGrantStmt and into ExecGrant_Relation
> and how we might improve it. I don't like the nested for() loops in
> ExecuteGrantStmt, but I don't see any easy way to resolve that and keep
> the SQL-required syntax.

What is your concern?
In my personal opinion, it is quite natural to apply nested-loop to
handle multiple columns within multiple tables.
At least, I don't have a smart idea to handle two-dimensional data
structure withou nested-loop.

> As for ExecGrant_Relation, it'd be nice if we
> could shorten it somehow by either combining the relation and column
> level handling into a single piece of code, or maybe refactoring it into
> seperate functions which could be called from both pieces..

It seems to me ExecGrant_Relation() is a bit larger than other ExecGrant_XXXX()s.
My preference is to clip out column-privilege part into ExecGrant_Attribute()
and invoke it for each given columns.
But, it is just my preference. Please ask it official commiters/reviewers.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On Thu, Jan 15, 2009 at 12:33 AM, KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>
> It seems to me ExecGrant_Relation() is a bit larger than other ExecGrant_XXXX()s.
> My preference is to clip out column-privilege part into ExecGrant_Attribute()
> and invoke it for each given columns.
> But, it is just my preference. Please ask it official commiters/reviewers.
>

while i'm not an official commiter/reviewer, it seems natural to me to
have an ExecGrant_Attribute() function.

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

Jaime,

* Jaime Casanova (jcasanov(at)systemguards(dot)com(dot)ec) wrote:
> while i'm not an official commiter/reviewer, it seems natural to me to
> have an ExecGrant_Attribute() function.

Thanks for the comment. I've gone ahead and done this now, and I do
think it improves the code overall and tells a better story.

Updated patch attached.

Thanks!

Stephen