[PATCH] Remove obscure permission checks in FindConversion()

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()?

The FindConversion() returns the OID of conversion for the given name and
namespace, or InvalidOid if not found or user does not have ACL_EXECUTE
privilege.
It is called from DropConversionsCommand(), RenameConversion(),
AlterConversionOwner() and CommentConversion(), to obtain OID of the target
conversion to be modified by DDL statement.

On the other hand, FindDefaultConversionProc() does not apply such kind of
permission checks, though it is called from SetClientEncoding().
The conversion procedure is implicitly called when user communicates to
the server backend, so it seems to me quite natural if FindDefaultConversionProc()
checks user's ACL_EXECUTE privilege.

But it is checked when we lookup the target conversion on DDL statement.

It's unclear for me what is the intension of this check.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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?

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.

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.

regards, tom lane

Tom Lane wrote:
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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 <kaigai(at)ak(dot)jp(dot)nec(dot)com>

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<kaigai(at)ak(dot)jp(dot)nec(dot)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 <kaigai(at)ak(dot)jp(dot)nec(dot)com>