[Patch Review] TRUNCATE Permission - Mailing list pgsql-hackers

<div dir="ltr">Hello all,<br /><br />Robert Haas submitted the TRUNCATE permissions patch for the September commit
fest:<br/><a
href="http://archives.postgresql.org/message-id/603c8f070807261444s133fb281sf34d069ab5b4c0b@mail.gmail.com">http://archives.postgresql.org/message-id/603c8f070807261444s133fb281sf34d069ab5b4c0b@mail.gmail.com</a><br
/><br/>I had some extra time tonight, so I downloaded, installed and reviewed this patch. <br />Here are my
findings:<br/><br />1. I found the patch style to be consistent with the surrounding code.<br />2. The patch provides
documentationupdates and regression test updates.<br /> 3. The patch applies (with some fuzz) to the latest GIT
tree.<br/><br />Three issues I found with the patch via code reading and verified via testing:<br /><br />1. File:
src/backend/catalog/aclchk.c:<br />    Function: pg_class_aclmask():<br /><br />I believe the ACL_TRUNCATE trigger
shouldbe added to this check and mask.<br /><br />        /*<br />         * Deny anyone permission to update a system
catalogunless<br />         * pg_authid.rolcatupdate is set.       (This is to let superusers protect<br />          *
themselvesfrom themselves.)  Also allow it if allowSystemTableMods.<br />         *<br />         * As of 7.4 we have
someupdatable system views; those shouldn't be<br />         * protected in this way.  Assume the view rules can take
careof<br />          * themselves.  ACL_USAGE is if we ever have system sequences.<br />         */<br />        if
((mask& (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_USAGE)) &&<br />               
IsSystemClass(classForm)&&<br />                 classForm->relkind != RELKIND_VIEW &&<br
/>               !has_rolcatupdate(roleid) &&<br />                !allowSystemTableMods)<br />        {<br
/>#ifdefACLDEBUG<br />                elog(DEBUG2, "permission denied for system catalog update");<br /> #endif<br
/>               mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_USAGE);<br />        }<br /><br /><br />Here
iswhere it is visible via the psql interface:<br /><br />template1=# select rolname, rolcatupdate from pg_authid;<br />
 rolname| rolcatupdate <br />---------+--------------<br /> rbrad   | t<br />(1 row)<br /><br />template1=# select
has_table_privilege('pg_class','delete');<br /> has_table_privilege <br />---------------------<br /> t<br /> (1
row)<br/><br />template1=# select has_table_privilege('pg_class', 'truncate');<br /> has_table_privilege <br
/>---------------------<br/> t<br />(1 row)<br /><br />template1=# update pg_authid set rolcatupdate = false;<br />
UPDATE1<br /><br />template1=# select has_table_privilege('pg_class', 'delete');<br /> has_table_privilege <br
/>---------------------<br/> f<br />(1 row)<br /><br />template1=# select has_table_privilege('pg_class',
'truncate');<br/>  has_table_privilege <br />---------------------<br /> t<br />(1 row)<br /><br /><br />I do not
believethis is a huge issue since truncate is prohibited on the system catalogs<br />by the truncate_check_rel().<br
/><br/>template1=# truncate pg_authid;<br /> ERROR:  permission denied: "pg_authid" is a system catalog<br /><br /><br
/> 2. The src/test/regress/sql/privileges.sql regression test has tests for<br />      the has_table_privilege()
function. It looks like all the other permissions<br />       are tested in this function, but there is not a test for
theTRUNCATE<br />      privilege.<br /> <br /><br />  3.  I believe you missed a documentation update in the ddl.sgml
file:<br/><br />   There are several different privileges: <literal>SELECT</>,<br />   
<literal>INSERT</>,<literal>UPDATE</>, <literal>DELETE</>,<br />  
<literal>REFERENCES</>,<literal>TRIGGER</>,<br />   <literal>CREATE</>,
<literal>CONNECT</>,<literal>TEMPORARY</>,<br />    <literal>EXECUTE</>, and
<literal>USAGE</>.<br/><br /><br />I played around with the patch for an hour or so tonight and I did not
observerany other unusual behaviors.<br /><br />Hopefully this review is useful.  It is my first patch review for a
commit-fest.<br/> I will update the wiki with a link to this email message for my review.<br /><br />Thanks,<br /><br
/>-Ryan<br /><br /></div>