Re: [PATCH] Reworks for Access Control facilities (r2311)

>>>>> I don't find the comment regarding what happened with FindConversion to
>>>>> be nearly descriptive enough.  Can you elaborate on why the check wasn't
>>>>> necessary and has now been removed?  If it really isn't needed, why have
>>>>> that function at all?
>>> http://archives.postgresql.org/message-id/26499.1250706473@sss.pgh.pa.us
>>>
>>> I'll add a comment about the reason why this check was simply eliminated.
>> Could these kind of changes be done as a separate patch?  Perhaps one
>> which should be applied first?  It's alot easier to review if we can
>> have:
>>
>> - patches to fix things in PG (such as the above)
>> - patch to add in ac_* model
>
> I think we can apply these kind of eliminations earlier or later.
> These checks might be redundant or unnecessary, but harmless.
> As far as the reworks patch does not touch them, it does not affect
> to our discussion.

The attached patch eliminates permission checks in FindConversion()
and EnableDisableRule(), because these are nonsense or redundant.

It is an separated issue from the ac_*() routines.
For now, we decided not to touch these stuffs in the access control
reworks patch. So, we can discuss about these fixes as a different
topic.

See the corresponding messages:
  http://archives.postgresql.org/message-id/26499.1250706473@sss.pgh.pa.us
  http://archives.postgresql.org/message-id/4ABC136A.90903@ak.jp.nec.com

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
Index: base/src/backend/rewrite/rewriteDefine.c
===================================================================
*** base/src/backend/rewrite/rewriteDefine.c    (revision 2336)
--- base/src/backend/rewrite/rewriteDefine.c    (working copy)
*************** EnableDisableRule(Relation rel, const ch
*** 671,677 ****
  {
      Relation    pg_rewrite_desc;
      Oid            owningRel = RelationGetRelid(rel);
-     Oid            eventRelationOid;
      HeapTuple    ruletup;
      bool        changed = false;

--- 671,676 ----
*************** EnableDisableRule(Relation rel, const ch
*** 690,702 ****
                          rulename, get_rel_name(owningRel))));

      /*
!      * Verify that the user has appropriate permissions.
       */
-     eventRelationOid = ((Form_pg_rewrite) GETSTRUCT(ruletup))->ev_class;
-     Assert(eventRelationOid == owningRel);
-     if (!pg_class_ownercheck(eventRelationOid, GetUserId()))
-         aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
-                        get_rel_name(eventRelationOid));

      /*
       * Change ev_enabled if it is different from the desired new state.
--- 689,704 ----
                          rulename, get_rel_name(owningRel))));

      /*
!      * At the prior release, we had a permission check here on
!      * a relation on which the given rule is configured.
!      * If user does not have ownership on the relation, it raises
!      * an error and aborts current transaction.
!      * But this check was redundant. ATExecCmd() is the only caller
!      * of EnableDisableRule(), and ATPrepCmd() already checks
!      * ownership of the target relation ATSimplePermissions().
!      *
!      * Therefore, we removed this permission check at v8.5.
       */

      /*
       * Change ev_enabled if it is different from the desired new state.
Index: base/src/backend/catalog/pg_conversion.c
===================================================================
*** base/src/backend/catalog/pg_conversion.c    (revision 2336)
--- base/src/backend/catalog/pg_conversion.c    (working copy)
***************
*** 24,30 ****
  #include "catalog/pg_proc.h"
  #include "mb/pg_wchar.h"
  #include "miscadmin.h"
- #include "utils/acl.h"
  #include "utils/builtins.h"
  #include "utils/fmgroids.h"
  #include "utils/rel.h"
--- 24,29 ----
*************** FindDefaultConversion(Oid name_space, in
*** 219,246 ****
  Oid
  FindConversion(const char *conname, Oid connamespace)
  {
!     HeapTuple    tuple;
!     Oid            procoid;
!     Oid            conoid;
!     AclResult    aclresult;
!
!     /* search pg_conversion by connamespace and conversion name */
!     tuple = SearchSysCache(CONNAMENSP,
!                            PointerGetDatum(conname),
!                            ObjectIdGetDatum(connamespace),
!                            0, 0);
!     if (!HeapTupleIsValid(tuple))
!         return InvalidOid;
!
!     procoid = ((Form_pg_conversion) GETSTRUCT(tuple))->conproc;
!     conoid = HeapTupleGetOid(tuple);
!
!     ReleaseSysCache(tuple);
!
!     /* Check we have execute rights for the function */
!     aclresult = pg_proc_aclcheck(procoid, GetUserId(), ACL_EXECUTE);
!     if (aclresult != ACLCHECK_OK)
!         return InvalidOid;
!
!     return conoid;
  }
--- 218,237 ----
  Oid
  FindConversion(const char *conname, Oid connamespace)
  {
!     /*
!      * At the prior release, we had a permission check here
!      * on the conversion function. If user does not have
!      * ACL_EXECUTE right on the function, the caller performs
!      * as if the required conversion is not exist.
!      * However, it is nonsense. FindConversion() is only called
!      * from the DDL code patch, such as ALTER CONVERSION, so
!      * we already apply enough checks on its creation time, and
!      * no interfaces are provided to change conversion function.
!      *
!      * Therefore, we removed this permission check at v8.5.
!      */
!     return GetSysCacheOid(CONNAMENSP,
!                           PointerGetDatum(conname),
!                           ObjectIdGetDatum(connamespace),
!                           0, 0);
  }