This patch fixes a problem discussed earlier.
Now, FindConversion() which is only called from FindConversionByName()
checks ACL_EXECUTE permission on the conversion procedure matched.
If not allowed, it performs as if the given conversion does not exist
(InvalidOid shall be returned).
The FindConversionByName() is called from:
- CommentConversion()
- DropConversionsCommand()
- RenameConversion()
- AlterConversionOwner()
All of them also check ownership of the conversion eventually.
In addition, CreateConversionCommand() checks ACL_EXECUTE permission
on the conversion procedure when creation time, independently.
Also see the related discussion:
http://archives.postgresql.org/pgsql-hackers/2009-08/msg01361.php
http://archives.postgresql.org/pgsql-hackers/2009-08/msg01392.php
http://archives.postgresql.org/pgsql-hackers/2009-08/msg01420.php
Thanks,
(2009/08/20 8:50), KaiGai Kohei wrote:
> Tom Lane wrote:
>> KaiGai Kohei<kai...@ak.jp.nec.com> writes:
>>> When FindConversion() is called, it also checks current user's ACL_EXECUTE
>>> privilege on the conproc of the fetched conversion.
>>
>>> Why this check is applied on FindConversion(), instead of
>>> FindDefaultConversion()?
>>
>> Does seem pretty inconsistent, doesn't it?
>
> Anyway, I could not understand the intention of the checks, rather
> than inconsistency. So, I doubted it might be a bug, if it intended
> to provide a permission something like ACL_USAGE on conversion.
>
>> The original idea may have been to provide a substitute for a USAGE
>> ACL check on conversions, in which case it's not totally insane: if
>> you make a conversion default then you're implicitly granting it to
>> public. But there's no documentation about this.
>
> If ACL_EXECUTE checks is deployed on FindDefaultConversion(), it performs
> as if something like ACL_USAGE permission. However, FindConversion() is
> called from ALTER or DROP CONVERSION, so it controls DDL statements
> in addition to its ownership.
> Please note that this check (ACL_EXECUTE) is not applied when user choose
> a certain conversion.
>
>> Offhand I see no really good reason to have a usage check on
>> conversions, and would be happy with removing this one. The function
>> permission check at CREATE CONVERSION time ought to be sufficient.
>
> It seems to me reasonable.
>
> If user does not have ACL_EXECUTE privilege on the function used in
> the conversion, he does not alter or drop the conversion, even if he
> has ownership of the conversion. This behavior is hard to understand.
>
> Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kai...@ak.jp.nec.com>
*** base/src/include/catalog/pg_conversion_fn.h (revision 2486)
--- base/src/include/catalog/pg_conversion_fn.h (working copy)
***************
*** 19,25 ****
int32 conforencoding, int32 contoencoding,
Oid conproc, bool def);
extern void RemoveConversionById(Oid conversionOid);
- extern Oid FindConversion(const char *conname, Oid connamespace);
extern Oid FindDefaultConversion(Oid connamespace, int32 for_encoding, int32 to_encoding);
#endif /* PG_CONVERSION_FN_H */
--- 19,24 ----
*** base/src/backend/catalog/pg_conversion.c (revision 2486)
--- base/src/backend/catalog/pg_conversion.c (working copy)
***************
*** 209,246 ****
ReleaseSysCacheList(catlist);
return proc;
}
-
- /*
- * FindConversion
- *
- * Find conversion by namespace and conversion name.
- * Returns conversion OID.
- */
- 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;
- }
--- 209,211 ----
*** base/src/backend/catalog/namespace.c (revision 2486)
--- base/src/backend/catalog/namespace.c (working copy)
***************
*** 2836,2842 ****
{
/* use exact schema given */
namespaceId = LookupExplicitNamespace(schemaname);
! return FindConversion(conversion_name, namespaceId);
}
else
{
--- 2836,2845 ----
{
/* use exact schema given */
namespaceId = LookupExplicitNamespace(schemaname);
! return GetSysCacheOid(CONNAMENSP,
! PointerGetDatum(conversion_name),
! ObjectIdGetDatum(namespaceId),
! 0, 0);
}
else
{
***************
*** 2850,2856 ****
if (namespaceId == myTempNamespace)
continue; /* do not look in temp namespace */
! conoid = FindConversion(conversion_name, namespaceId);
if (OidIsValid(conoid))
return conoid;
}
--- 2853,2862 ----
if (namespaceId == myTempNamespace)
continue; /* do not look in temp namespace */
! conoid = GetSysCacheOid(CONNAMENSP,
! PointerGetDatum(conversion_name),
! ObjectIdGetDatum(namespaceId),
! 0, 0);
if (OidIsValid(conoid))
return conoid;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
