Message-ID: <4AC41DB7.5070706@ak.jp.nec.com>
Date: Thu, 01 Oct 2009 12:10:47 +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> <4AC2ED74.2030409@ak.jp.nec.com>
	<20090930122347.GT17756@tamriel.snowman.net>
In-Reply-To: <20090930122347.GT17756@tamriel.snowman.net>
Content-Type: text/plain; charset=ISO-2022-JP
Content-Transfer-Encoding: 7bit

Stephen Frost wrote:
> KaiGai,
> 
> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>> 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.  To make sure it gets picked up, you might respond to Tom's
> message above with this same email.  Just a thought.

The following message was my reply.
  http://archives.postgresql.org/pgsql-hackers/2009-08/msg01420.php


Now I'm removing something with behavior change such as above patch,
and eliminating comments to be discussed in other thread.
I would like to quote them not to forget them away.

* ACL_CREATE checks on renaming type

When we rename an existing type, it checks ownership of the target
type, but ACL_CREATE on the namespace in which the type is stored,
although it is checked on renaming any other database objects stored
within a certain namespace, such as tables, functions and so on.

Is it an intended behavior?


* Ownership checks on REASSIGN OWNED BY statement

When we use the REASSIGN OWNER BY statement, it tries to change
ownership of the database objects which are owned by the target
role. It internally calls routine to change the ownership such
as AlterFunctionOwner_oid().
The routine depends on the class of objects, and they have a code
to check privilege to change ownership when it is actually changed
as follows:
But the code path corresponding to relations and types does not
have such kind of checks. IMO, similar check should be deployed.

-- at the AlterFunctionOwner_internal()

    if (procForm->proowner != newOwnerId)
    {
        Datum       repl_val[Natts_pg_proc];
        bool        repl_null[Natts_pg_proc];
        bool        repl_repl[Natts_pg_proc];
        Acl        *newAcl;
        Datum       aclDatum;
        bool        isNull;
        HeapTuple   newtuple;

        /* Superusers can always do it */
        if (!superuser())
        {
            /* Otherwise, must be owner of the existing object */
            if (!pg_proc_ownercheck(procOid, GetUserId()))
                aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PROC,
                               NameStr(procForm->proname));

            /* Must be able to become new owner */
            check_is_member_of_role(GetUserId(), newOwnerId);

            /* New owner must have CREATE privilege on namespace */
            aclresult = pg_namespace_aclcheck(procForm->pronamespace,
                                              newOwnerId,
                                              ACL_CREATE);
            if (aclresult != ACLCHECK_OK)
                aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
                               get_namespace_name(procForm->pronamespace));
        }
               :

-- 
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>