[Patch Review] TRUNCATE Permission

Hello all,

Robert Haas submitted the TRUNCATE permissions patch for the September
commit fest:
http://archives.postgresql.org/message-id/603c8f070807261444s133fb281sf34d069ab5b4c0b@mail.gmail.com

I had some extra time tonight, so I downloaded, installed and reviewed this
patch.
Here are my findings:

1. I found the patch style to be consistent with the surrounding code.
2. The patch provides documentation updates and regression test updates.
3. The patch applies (with some fuzz) to the latest GIT tree.

Three issues I found with the patch via code reading and verified via
testing:

1. File: src/backend/catalog/aclchk.c:
Function: pg_class_aclmask():

I believe the ACL_TRUNCATE trigger should be added to this check and mask.

/*
* Deny anyone permission to update a system catalog unless
* pg_authid.rolcatupdate is set. (This is to let superusers
protect
* themselves from themselves.) Also allow it if
allowSystemTableMods.
*
* As of 7.4 we have some updatable system views; those shouldn't be
* protected in this way. Assume the view rules can take care of
* themselves. ACL_USAGE is if we ever have system sequences.
*/
if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_USAGE)) &&
IsSystemClass(classForm) &&
classForm->relkind != RELKIND_VIEW &&
!has_rolcatupdate(roleid) &&
!allowSystemTableMods)
{
#ifdef ACLDEBUG
elog(DEBUG2, "permission denied for system catalog update");
#endif
mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_USAGE);
}

Here is where it is visible via the psql interface:

template1=# select rolname, rolcatupdate from pg_authid;
rolname | rolcatupdate
---------+--------------
rbrad | t
(1 row)

template1=# select has_table_privilege('pg_class', 'delete');
has_table_privilege
---------------------
t
(1 row)

template1=# select has_table_privilege('pg_class', 'truncate');
has_table_privilege
---------------------
t
(1 row)

template1=# update pg_authid set rolcatupdate = false;
UPDATE 1

template1=# select has_table_privilege('pg_class', 'delete');
has_table_privilege
---------------------
f
(1 row)

template1=# select has_table_privilege('pg_class', 'truncate');
has_table_privilege
---------------------
t
(1 row)

I do not believe this is a huge issue since truncate is prohibited on the
system catalogs
by the truncate_check_rel().

template1=# truncate pg_authid;
ERROR: permission denied: "pg_authid" is a system catalog

2. The src/test/regress/sql/privileges.sql regression test has tests for
the has_table_privilege() function. It looks like all the other
permissions
are tested in this function, but there is not a test for the TRUNCATE
privilege.

3. I believe you missed a documentation update in the ddl.sgml file:

There are several different privileges: <literal>SELECT</>,
<literal>INSERT</>, <literal>UPDATE</>, <literal>DELETE</>,
<literal>REFERENCES</>, <literal>TRIGGER</>,
<literal>CREATE</>, <literal>CONNECT</>, <literal>TEMPORARY</>,
<literal>EXECUTE</>, and <literal>USAGE</>.

I played around with the patch for an hour or so tonight and I did not
observer any other unusual behaviors.

Hopefully this review is useful. It is my first patch review for a
commit-fest.
I will update the wiki with a link to this email message for my review.

Thanks,

- Ryan