Extending grant insert on tables to sequences

On Thu, May 22, 2008 at 1:18 PM, Jaime Casanova <systemguards@gmail.com> wrote:

Hi,

The idea of this patch is to avoid the need to make explicit grants on
sequences owned by tables.

I've noted that the patch i attached is an older version that doesn't
compile because of a typo...
Re-attaching right patch and fix documentation to indicate the new behaviour...

we need an user visible message to indicate this implicit grant on the
sequences?

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 087171157

Jaime Casanova escribi�:

On Thu, May 22, 2008 at 1:18 PM, Jaime Casanova <systemguards@gmail.com> wrote:

Hi,

The idea of this patch is to avoid the need to make explicit grants on
sequences owned by tables.

I've noted that the patch i attached is an older version that doesn't
compile because of a typo...
Re-attaching right patch and fix documentation to indicate the new behaviour...

Please add the patch to the commitfest page,

http://wiki.postgresql.org/wiki/CommitFest:July

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Sat, May 24, 2008 at 12:09 AM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

Please add the patch to the commitfest page,

Ah! I forgot we have a new process now... patch added to the commitfest page...

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 087171157

On Saturday 24 May 2008 01:19:05 Jaime Casanova wrote:

On Sat, May 24, 2008 at 12:09 AM, Alvaro Herrera

<alvherre@commandprompt.com> wrote:

Please add the patch to the commitfest page,

Ah! I forgot we have a new process now... patch added to the commitfest
page...

What's the use case for extending SELECT on table to SELECT on sequence ?

--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL

On 5/29/08, Robert Treat <xzilla@users.sourceforge.net> wrote:

On Saturday 24 May 2008 01:19:05 Jaime Casanova wrote:

On Sat, May 24, 2008 at 12:09 AM, Alvaro Herrera

<alvherre@commandprompt.com> wrote:

Please add the patch to the commitfest page,

Ah! I forgot we have a new process now... patch added to the commitfest
page...

What's the use case for extending SELECT on table to SELECT on sequence ?

Just to be consistent

--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 087171157

Jaime Casanova escribi�:

On Thu, May 22, 2008 at 1:18 PM, Jaime Casanova <systemguards@gmail.com> wrote:

Hi,

The idea of this patch is to avoid the need to make explicit grants on
sequences owned by tables.

I've noted that the patch i attached is an older version that doesn't
compile because of a typo...
Re-attaching right patch and fix documentation to indicate the new behaviour...

I had a look at this patch and it looks good. The only thing that's not
clear to me is whether we have agreed we want this to be the default
behavior?

A quibble:

+ 		foreach(cell, istmt.objects)
+ 		{
+ 			[...]
+ 
+ 			istmt_seq.objects = getOwnedSequences(lfirst_oid(cell));
+ 			if (istmt_seq.objects != NIL)
+ 			{
+ 				if (istmt.privileges & (ACL_INSERT)) 
+ 					istmt_seq.privileges |= ACL_USAGE;
+ 				else if (istmt.privileges & (ACL_UPDATE)) 
+ 					istmt_seq.privileges |= ACL_UPDATE;
+ 				else if (istmt.privileges & (ACL_SELECT)) 
+ 					istmt_seq.privileges |= ACL_SELECT;
+ 
+ 				ExecGrantStmt_oids(&istmt_seq);
+ 			}

Wouldn't it be clearer to build a list with all the sequences owned by
the tables in istmt.objects, and then call ExecGrantStmt_oids() a single
time with the big list?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 7/8/08, Alvaro Herrera <alvherre@commandprompt.com> wrote:

Jaime Casanova escribió:

On Thu, May 22, 2008 at 1:18 PM, Jaime Casanova <systemguards@gmail.com> wrote:

Hi,

The idea of this patch is to avoid the need to make explicit grants on
sequences owned by tables.

I've noted that the patch i attached is an older version that doesn't
compile because of a typo...
Re-attaching right patch and fix documentation to indicate the new behaviour...

I had a look at this patch and it looks good. The only thing that's not
clear to me is whether we have agreed we want this to be the default
behavior?

mmm... i don't remember from where i took the equivalences...
i will review if there is any concensus in that...
anyway now i when people should speak about it...

Wouldn't it be clearer to build a list with all the sequences owned by
the tables in istmt.objects, and then call ExecGrantStmt_oids() a single
time with the big list?

at night i will see the code for this...

--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 87171157

At 2008-07-08 09:32:44 -0400, alvherre@commandprompt.com wrote:

The idea of this patch is to avoid the need to make explicit
grants on sequences owned by tables. [...]

I had a look at this patch and it looks good. The only thing that's
not clear to me is whether we have agreed we want this to be the
default behavior?

For what it's worth, I quite like the idea.

(I looked at the patch, and it looks good to me too.)

Wouldn't it be clearer to build a list with all the sequences owned by
the tables in istmt.objects, and then call ExecGrantStmt_oids() a
single time with the big list?

i.e., to hoist most of the istmt_seq initialisation out of the loop,
right? Yes, that makes sense.

-- ams

Abhijit Menon-Sen escribi�:

At 2008-07-08 09:32:44 -0400, alvherre@commandprompt.com wrote:

Wouldn't it be clearer to build a list with all the sequences owned by
the tables in istmt.objects, and then call ExecGrantStmt_oids() a
single time with the big list?

i.e., to hoist most of the istmt_seq initialisation out of the loop,
right? Yes, that makes sense.

No, actually I meant having a lone "list = lappend(list, newseq);" in
the loop, so that ExecGrantStmt_oids is called only once. The
initialization is done only once too, of course.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

At 2008-07-09 15:11:25 -0400, alvherre@commandprompt.com wrote:

No, actually I meant having a lone "list = lappend(list, newseq);" in
the loop, so that ExecGrantStmt_oids is called only once.

Yes, I understand what you meant. I just phrased my agreement poorly.
Here's a more precise phrasing. ;-)

(I agree with Robert Treat that there seems to be no point granting
SELECT on the sequence. I don't *particularly* care about it, but I
tend towards wanting to drop that bit. This patch reflects that.)

Jaime: please feel free to use or ignore this, as you wish.

-- ams

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 15f5af0..8664203 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -361,6 +361,41 @@ ExecuteGrantStmt(GrantStmt *stmt)
 	}

 	ExecGrantStmt_oids(&istmt);
+
+	/* If INSERT or UPDATE privileges are being granted or revoked on a
+	 * relation, this extends the operation to include any sequences
+	 * owned by the relation.
+	 */
+
+	if (istmt.objtype == ACL_OBJECT_RELATION &&
+		(istmt.privileges & (ACL_INSERT | ACL_UPDATE)))
+	{
+		InternalGrant istmt_seq;
+
+		istmt_seq.is_grant = istmt.is_grant;
+		istmt_seq.objtype = ACL_OBJECT_SEQUENCE;
+		istmt_seq.grantees = istmt.grantees;
+		istmt_seq.grant_option = istmt.grant_option;
+		istmt_seq.behavior = istmt.behavior;
+		istmt_seq.all_privs = false;
+
+		istmt_seq.privileges = ACL_NO_RIGHTS;
+		if (istmt.privileges & ACL_INSERT)
+			istmt_seq.privileges |= ACL_USAGE;
+		if (istmt.privileges & ACL_UPDATE)
+			istmt_seq.privileges |= ACL_UPDATE;
+
+		istmt_seq.objects = NIL;
+		foreach (cell, istmt.objects)
+		{
+			istmt_seq.objects =
+				list_concat(istmt_seq.objects,
+							getOwnedSequences(lfirst_oid(cell)));
+		}
+
+		if (istmt_seq.objects != NIL)
+			ExecGrantStmt_oids(&istmt_seq);
+	}
 }

/*

On Wed, Jul 9, 2008 at 10:11 PM, Abhijit Menon-Sen <ams@oryx.com> wrote:

At 2008-07-09 15:11:25 -0400, alvherre@commandprompt.com wrote:

No, actually I meant having a lone "list = lappend(list, newseq);" in
the loop, so that ExecGrantStmt_oids is called only once.

Yes, I understand what you meant. I just phrased my agreement poorly.
Here's a more precise phrasing. ;-)

(I agree with Robert Treat that there seems to be no point granting
SELECT on the sequence. I don't *particularly* care about it, but I
tend towards wanting to drop that bit. This patch reflects that.)

Hi,
sorry for the delay i was busy...

attached is a new version of the patch, it implements Alvaro's
suggestion and fix a bug i found (it wasn't managing GRANT ALL) :(

About the SELECT issue, AFAIU Robert doesn't complaint he just asked
what is the use case... if people think it should be removed ok, but
OTOH: why? i don't think that affects anything...

--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 87171157

At 2008-07-11 11:57:37 -0500, jcasanov@systemguards.com.ec wrote:

attached is a new version of the patch, it implements Alvaro's
suggestion and fix a bug i found (it wasn't managing GRANT ALL) :(

Looks good to me.

About the SELECT issue, AFAIU Robert doesn't complaint he just asked
what is the use case... if people think it should be removed ok, but
OTOH: why? i don't think that affects anything...

As I said, I don't feel too strongly about it.

<para>
! Granting permission on a table automatically extend
! permissions to any sequences owned by the table, including
! sequences tied to <type>SERIAL</> columns.
</para>

Should be "Granting permissions on a table automatically extends those
permissions to...".

+ 	if ((istmt.objtype == ACL_OBJECT_RELATION) && (istmt.all_privs ||  
+ 		(istmt.privileges & (ACL_INSERT | ACL_UPDATE | ACL_SELECT)))) 
+ 	{

The parentheses around the first comparison can go away, and also the
ones around the ACL_* here:

+ 			if (istmt.privileges & (ACL_INSERT)) 
+ 				istmt_seq.privileges |= ACL_USAGE;
+ 			if (istmt.privileges & (ACL_UPDATE)) 
+ 				istmt_seq.privileges |= ACL_UPDATE;
+ 			if (istmt.privileges & (ACL_SELECT)) 
+ 				istmt_seq.privileges |= ACL_SELECT;

-- ams

On Sat, Jul 12, 2008 at 6:30 AM, Abhijit Menon-Sen <ams@oryx.com> wrote:

<para>
! Granting permission on a table automatically extend
! permissions to any sequences owned by the table, including
! sequences tied to <type>SERIAL</> columns.
</para>

Should be "Granting permissions on a table automatically extends those
permissions to...".

what about "extends them to..."

+     if ((istmt.objtype == ACL_OBJECT_RELATION) && (istmt.all_privs ||
+             (istmt.privileges & (ACL_INSERT | ACL_UPDATE | ACL_SELECT))))
+     {

The parentheses around the first comparison can go away, and also the
ones around the ACL_* here:

ok

--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 87171157

At 2008-07-12 14:32:03 -0500, jcasanov@systemguards.com.ec wrote:

Should be "Granting permissions on a table automatically extends
those permissions to...".

what about "extends them to..."

Yes, sounds fine, thanks.

But I notice that nobody else has commented on whether they want this
feature or not. Does anyone particularly dislike the idea?

-- ams

"Jaime Casanova" <jcasanov@systemguards.com.ec> writes:

+ 		if (istmt.all_privs)
+ 			istmt_seq.all_privs = true;
+ 		else
+ 		{
+ 			if (istmt.privileges & ACL_INSERT) 
+ 				istmt_seq.privileges |= ACL_USAGE;
+ 			if (istmt.privileges & ACL_UPDATE) 
+ 				istmt_seq.privileges |= ACL_UPDATE;
+ 			if (istmt.privileges & ACL_SELECT) 
+ 				istmt_seq.privileges |= ACL_SELECT;
+ 		}

This definition of the derived rights seems pretty arbitrary and
unprincipled. If you can't explain it precisely in a few words in the
documentation (and I notice you didn't even attempt to explain it at
all), then you probably need to think harder.

In particular, I don't see why having UPDATE on the parent table should
grant the right to use setval() on the sequence. If you had INSERT and
DELETE as well, implying the right to make arbitrary changes in the
parent table, then maybe setval() would be sensible to allow --- but
this code can't really tell that, since it doesn't have a global view
of the privileges previously granted.

What I think makes sense is just to have parent INSERT privilege lead to
USAGE on the sequence (thus granting nextval/currval rights, which are
what you'd typically need in association with trying to do inserts).
I don't see any need to automatically grant more than that. Selects and
updates on the parent table don't need to touch the sequence, so why are
those privileges granting anything?

regards, tom lane

Abhijit Menon-Sen <ams@oryx.com> writes:

But I notice that nobody else has commented on whether they want this
feature or not. Does anyone particularly dislike the idea?

I think it's probably reasonable as long as we keep the implicitly
granted rights as narrow as possible. INSERT on the parent table
would normally be hard to use correctly if you can't nextval() the
sequence, so automatically allowing nextval() seems pretty reasonable.
I think the case for granting anything more than that is weak ---
even without considering backwards-compatibility arguments.

A fairly important practical problem is whether this will keep pg_dump
from correctly reproducing the state of a database. Assume that someone
did revoke the implicitly-granted rights on the sequence --- would a
dump and reload correctly preserve that state? It'd depend on the order
in which pg_dump issued the GRANTs, and I'm not at all sure pg_dump
could be relied on to get that right. (Even if we fixed it to account
for the issue today, what of older dump scripts?)

Another issue is the interaction with the planned column-level GRANT
feature. AFAICS, the obvious-sounding rule that usage of the sequence
should be granted consequent to granting INSERT on the owning column
would be exactly backwards. It's when you have *not* got INSERT on
that column that you *must* rely on the default for it, and hence you'd
better have the ability to do nextval() or your alleged insert
privileges on other columns are worthless. So it seems that sequence
usage should be granted if any column INSERT is granted, and revoked
only when all column INSERT privileges are revoked --- and that latter
rule is going to be hard to implement with this type of patch, because
it doesn't know what column privileges are going to remain.

I thought for a bit about abandoning the proposed implementation and
instead having nextval/currval check at runtime: IOW, if the check for
ACL_USAGE on the sequence fails, look to see if the sequence is "owned"
and if so look to see if the user has ACL_INSERT on the parent table.
(This seems a bit slow but maybe it wouldn't be a problem, or maybe we
could arrange to cache the lookup results.)  This would avoid the
"action at a distance" behavior in GRANT and thereby cure both of
the problems mentioned above.  However, it would mean that it'd be
impossible to grant INSERT without effectively granting sequence USAGE
--- revoking USAGE on the sequence wouldn't stop anything.  Plus, \z on
the sequence would fail to tell you about those implicitly held rights.
So I'm not sure I like this way any better.

regards, tom lane

Sorry for the delay in the answer but i was busy with 2 projects and a talk...

On Sat, Jul 12, 2008 at 3:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think it's probably reasonable as long as we keep the implicitly
granted rights as narrow as possible. INSERT on the parent table
would normally be hard to use correctly if you can't nextval() the
sequence, so automatically allowing nextval() seems pretty reasonable.
I think the case for granting anything more than that is weak ---
even without considering backwards-compatibility arguments.

ok. at least two more reviewers make questions against the SELECT permission...
my point was that if keep INSERT and UPDATE permissions then keep
SELECT as well... but let *only* INSERT's seems enough to me...

A fairly important practical problem is whether this will keep pg_dump
from correctly reproducing the state of a database. Assume that someone
did revoke the implicitly-granted rights on the sequence --- would a
dump and reload correctly preserve that state? It'd depend on the order
in which pg_dump issued the GRANTs, and I'm not at all sure pg_dump
could be relied on to get that right. (Even if we fixed it to account
for the issue today, what of older dump scripts?)

good point! a simple test make me think that yes, i will try some
complex cases to be sure (actually i think it should be a problem
here)

Another issue is the interaction with the planned column-level GRANT
feature.

Although that is a feature we want, is a WIP one... do we stop patches
because it can conflict with a project we don't know will be applied
soon?

In any case, i will review that patch to see where we are on that and
to try make those two compatible...

I thought for a bit about abandoning the proposed implementation and
instead having nextval/currval check at runtime: IOW, if the check for
ACL_USAGE on the sequence fails, look to see if the sequence is "owned"
and if so look to see if the user has ACL_INSERT on the parent table.
(This seems a bit slow but maybe it wouldn't be a problem, or maybe we
could arrange to cache the lookup results.)  This would avoid the
"action at a distance" behavior in GRANT and thereby cure both of
the problems mentioned above.  However, it would mean that it'd be
impossible to grant INSERT without effectively granting sequence USAGE
--- revoking USAGE on the sequence wouldn't stop anything.  Plus, \z on
the sequence would fail to tell you about those implicitly held rights.

seems like a hackish... do we want this? comments?

i will work on this patch for the next days...

--
regards,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Guayaquil - Ecuador
Cel. (593) 87171157

"Jaime Casanova" <jcasanov@systemguards.com.ec> writes:

Another issue is the interaction with the planned column-level GRANT
feature.

Although that is a feature we want, is a WIP one... do we stop patches
because it can conflict with a project we don't know will be applied
soon?

Well, considering that that one is implementing a feature required by
SQL spec, your feature will lose any tug-of-war ;-). So yeah, you
ought to consider how to make yours play nice when (not if) that
happens.

regards, tom lane

Added to September commit fest.

---------------------------------------------------------------------------

Abhijit Menon-Sen wrote:

At 2008-07-09 15:11:25 -0400, alvherre@commandprompt.com wrote:

No, actually I meant having a lone "list = lappend(list, newseq);" in
the loop, so that ExecGrantStmt_oids is called only once.

Yes, I understand what you meant. I just phrased my agreement poorly.
Here's a more precise phrasing. ;-)

(I agree with Robert Treat that there seems to be no point granting
SELECT on the sequence. I don't *particularly* care about it, but I
tend towards wanting to drop that bit. This patch reflects that.)

Jaime: please feel free to use or ignore this, as you wish.

-- ams

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 15f5af0..8664203 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -361,6 +361,41 @@ ExecuteGrantStmt(GrantStmt *stmt)
}

ExecGrantStmt_oids(&istmt);
+
+	/* If INSERT or UPDATE privileges are being granted or revoked on a
+	 * relation, this extends the operation to include any sequences
+	 * owned by the relation.
+	 */
+
+	if (istmt.objtype == ACL_OBJECT_RELATION &&
+		(istmt.privileges & (ACL_INSERT | ACL_UPDATE)))
+	{
+		InternalGrant istmt_seq;
+
+		istmt_seq.is_grant = istmt.is_grant;
+		istmt_seq.objtype = ACL_OBJECT_SEQUENCE;
+		istmt_seq.grantees = istmt.grantees;
+		istmt_seq.grant_option = istmt.grant_option;
+		istmt_seq.behavior = istmt.behavior;
+		istmt_seq.all_privs = false;
+
+		istmt_seq.privileges = ACL_NO_RIGHTS;
+		if (istmt.privileges & ACL_INSERT)
+			istmt_seq.privileges |= ACL_USAGE;
+		if (istmt.privileges & ACL_UPDATE)
+			istmt_seq.privileges |= ACL_UPDATE;
+
+		istmt_seq.objects = NIL;
+		foreach (cell, istmt.objects)
+		{
+			istmt_seq.objects =
+				list_concat(istmt_seq.objects,
+							getOwnedSequences(lfirst_oid(cell)));
+		}
+
+		if (istmt_seq.objects != NIL)
+			ExecGrantStmt_oids(&istmt_seq);
+	}
}

/*

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +