Subject: Re: New default role- 'pg_read_all_data'
To: pgsql-hackers@lists.postgresql.org
References: <20200828003023.GU29590@tamriel.snowman.net>
 <CABUevEy9QAnx5h_BmSk4j6K9AfS9=co15RUFda0kknMHO9NJOg@mail.gmail.com>
 <20200828123847.GV29590@tamriel.snowman.net>
 <CABUevEx5akfNeSrHsinCRWfXzSYFNRTnBc4vHKMgum8hLrr=yg@mail.gmail.com>
 <20200828132645.GZ29590@tamriel.snowman.net>
 <6a78c0d5-d010-f2b5-198f-b35eb152f3c4@darold.net>
 <20200828145219.GA29590@tamriel.snowman.net>
From: Gilles Darold <gilles@darold.net>
Message-ID: <1091e05d-66fa-b56c-9b60-dc1bde6cf9dc@darold.net>
Date: Fri, 28 Aug 2020 17:21:22 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200828145219.GA29590@tamriel.snowman.net>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

Le 28/08/2020 à 16:52, Stephen Frost a écrit :
> Greetings,
>
> * Gilles Darold (gilles@darold.net) wrote:
>> Le 28/08/2020 à 15:26, Stephen Frost a écrit :
>>> * Magnus Hagander (magnus@hagander.net) wrote:
>>>> On Fri, Aug 28, 2020 at 2:38 PM Stephen Frost <sfrost@snowman.net> wrote:
>>>>> * Magnus Hagander (magnus@hagander.net) wrote:
>>>>>> Without having actually looked at the code, definite +1 for this feature.
>>>>>> It's much requested...
>>>>> Thanks.
>>>>>
>>>>>> But, should we also have a pg_write_all_data to go along with it?
>>>>> Perhaps, but could certainly be a different patch, and it'd need to be
>>>>> better defined, it seems to me...  read_all is pretty straight-forward
>>>>> (the general goal being "make pg_dumpall/pg_dump work"), what would
>>>>> write mean?  INSERT?  DELETE?  TRUNCATE?  ALTER TABLE?  System catalogs?
>>>> Well, it's pg_write_all_*data*, so it certainly wouldn't be alter table or
>>>> system catalogs.
>>>>
>>>> I'd say insert/update/delete yes.
>>>>
>>>> TRUNCATE is always an outlier.Given it's generally classified as DDL, I
>>>> wouldn't include it.
>>> Alright, that seems like it'd be pretty easy.  We already have a check
>>> in pg_class_aclmask to disallow modification of system catalogs w/o
>>> being a superuser, so we should be alright to add a similar check for
>>> insert/update/delete just below where I added the SELECT check.
>>>
>>>>> Doesn't seem like you could just declare it to be 'allow pg_restore'
>>>>> either, as that might include creating untrusted functions, et al.
>>>> No definitely not. That wouldn't be the usecase at all :)
>>> Good. :)
>>>
>>>> (and fwiw to me the main use case for read_all_data also isn't pg_dump,
>>>> because most people using pg_dump are already db owner or higher in my
>>>> experience. But it is nice that it helps with that too)
>>> Glad to have confirmation that there's other use-cases this helps with.
>>>
>>> I'll post an updated patch with that added in a day or two.
>> Looking at this thread I was thinking about the FDW. Does role
>> pg_read_all_data will allow to execute pg_dump with --include-foreign-data
>> option? If this is the case how about priviledge on fdw and foreign server?
>> If this is the behavior we want I guess that the modification should be
>> applied to pg_foreign_data_wrapper_aclmask() and pg_foreign_server_aclmask()
>> too.
> Using an FDW will often also require having a user mapping and there's
> no way for that to be accomplished through only GRANT'ing a default
> role, so I don't think we should mix this specific role with the FDW
> permissions system.


I'm fine with that, perhaps it should be mentioned in the documentation 
that foreign tables are not covered by this role.

-- 
Gilles Darold