Re: [v9.2] sepgsql's DROP Permission checks

On Thu, Jan 19, 2012 at 3:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2012/1/19 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Wed, Jan 18, 2012 at 9:50 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> In sepgsql side, it determines a case to apply permission checks
>>> according to the contextual information; that is same technique
>>> when we implemented create permission.
>>> Thus, it could checks db_xxx:{drop} permission correctly.
>>
>> Why do we need the contextual information in this case?  Why
>> can't/shouldn't the decision be made solely on the basis of what
>> object is targeted?
>>
> Several code-paths to remove a particular objects are not appropriate
> to apply permission checks. For example...
>
> [1] Cleanup of temporary objects
> on_shmem_eixt() registers RemoveTempRelationsCallback(), then
> it eventually calls deleteWhatDependsOn() that have an invocation
> of deleteOneObject().
> This code path is just an internal cleanup process, not related to
> permission of the client.
>
> [2] Cleanup of transient table at VACUUM FULL/CLUSTER command
> rebuild_relation() creates a temporary table with make_new_heap(),
> then it copies the contents of original table according to the order of
> index, and calls finish_heap_swap() that swaps relation files and
> removes the temporary table using performDeletion().
> This code actually create and drop a table, however, it is quite
> internal design and not related to permission of the client.
>
> So, I think sepgsql should only applied to permission checks
> object deletion invoked by user's operations, such as DropStmt.

I agree with that theory, but isn't this method of implementing that a
pretty horrible kludge? For example, if you'd implemented it this way
for 9.1, the recent drop-statement refactoring would have broken it.
Or if, in the future, we add another type of statement that can drop
things, this code will still compile just fine but will no longer work
correctly. ISTM that we need a way to either (1) not call the hook at
all unless the operation is user-initiated, or (2) call the hook, but
pass a flag indicating what sort of operation this is?

Let's imagine another possible use of this hook: we want to emit some
kind of log message every time a database object gets dropped. I
think that's a plausible use-case, and in that case what we'd want is:
(1) VACUUM FULL or CLUSTER shouldn't call the hook at all, (2) cleanup
of temporary objects should probably call the hook, but ideally with a
flag to indicate that it's an internal (DB-initiated) operation, and
(3) user activity should definitely call the hook.

I'm not sure how we can cleanly get that behavior, but ISTM that's
what we want...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company