From: KaiGai Kohei <kaigai@ak.jp.nec.com>
To: Stephen Frost <sfrost@snowman.net>
Cc: KaiGai Kohei <kaigai@kaigai.gr.jp>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: [PATCH] Reworks for Access Control facilities (r2311)
Date: Wed, 30 Sep 2009 14:32:36 +0900
Message-ID: <4AC2ED74.2030409@ak.jp.nec.com> (raw)
In-Reply-To: <4AC2BDD0.7050906@ak.jp.nec.com>
References: <20090929105431.GO17756@tamriel.snowman.net>
<4AC1EA9E.3080907@kaigai.gr.jp>
<20090929173049.GP17756@tamriel.snowman.net>
<4AC2BDD0.7050906@ak.jp.nec.com>
>>>>> 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.ushttp://archives.postgresql.org/message-id/4ABC136A.90903@ak.jp.nec.com
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
Attachments:
[text/x-patch] remove-unneeded-checks.patch (3.7K, ../4AC2ED74.2030409@ak.jp.nec.com/2-remove-unneeded-checks.patch)
download | inline diff:
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);
}
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: kaigai@ak.jp.nec.com, sfrost@snowman.net, kaigai@kaigai.gr.jp, robertmhaas@gmail.com
Subject: Re: [PATCH] Reworks for Access Control facilities (r2311)
In-Reply-To: <4AC2ED74.2030409@ak.jp.nec.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox