Message-ID: <4AC2ED74.2030409@ak.jp.nec.com>
Date: Wed, 30 Sep 2009 14:32:36 +0900
From: KaiGai Kohei <kaigai@ak.jp.nec.com>
User-Agent: Thunderbird 2.0.0.6 (Windows/20070728)
MIME-Version: 1.0
To: Stephen Frost <sfrost@snowman.net>
CC: KaiGai Kohei <kaigai@kaigai.gr.jp>, Robert Haas <robertmhaas@gmail.com>,
	pgsql-hackers@postgresql.org
Subject: Re: [PATCH] Reworks for Access Control facilities (r2311)
References: <20090929105431.GO17756@tamriel.snowman.net>
	<4AC1EA9E.3080907@kaigai.gr.jp>
	<20090929173049.GP17756@tamriel.snowman.net>
	<4AC2BDD0.7050906@ak.jp.nec.com>
In-Reply-To: <4AC2BDD0.7050906@ak.jp.nec.com>
Content-Type: multipart/mixed; boundary="------------010401070004010201060007"

This is a multi-part message in MIME format.
--------------010401070004010201060007
Content-Type: text/plain; charset=ISO-2022-JP
Content-Transfer-Encoding: 7bit

>>>>> 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>

--------------010401070004010201060007
Content-Type: text/x-patch;
 name="remove-unneeded-checks.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="remove-unneeded-checks.patch"

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);
  }

--------------010401070004010201060007--