>>>>> 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/[email protected]
>>>
>>> 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/[email protected]
http://archives.postgresql.org/message-id/[email protected]
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <[email protected]>
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);
}
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
