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

I tried to implement based on the idea to call object-access-hook with
flag; that
informs extensions contexts of objects being removed.
Because I missed DROP_RESTRICT and DROP_CASCADE are enum type,
this patch added performInternalDeletion() instead of OR-masked DROP_INTERNAL.
All its difference from performDeletion() is a flag (OAT_DROP_FLAGS_INTERNAL)
shall be delivered to extension module. I replaced several performDeletion() by
performInternalDeletion() that clean-up objects due to internal stuff.

How about this approach?

Thanks,

2012/1/21 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2012/1/19 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> 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?
>>
> Yes. This approach requires to continue code revising on sepgsql-side
> also for each major release.
> I think the approach (1) raise an issue similar to what we discussed
> when sepgsql implemented create permission; we have to know
> details of extension module to determine whether the hook should
> be called, or not. My preference is (2) that is more reasonable.
>
>> 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...
>>
> I think the case (1) should also call the hook but with a flag that indicate
> database internal stuff, because make_new_heap() calls OAT_POST_CREATE
> hook, thus, we need to give extensions a chance to cleanup, if it did
> something on this timing.
>
> I'd like to propose to utilize DropBehavior argument of performDeletion()
> to inform dependency.c its invoked context.
> If we have a new flag DROP_INTERNAL that can be OR-masked with
> existing DROP_RESTRICT or DROP_CASCADE, it can be used to
> inform the current context, then, it shall be used to the flag to the hook.
> It seems to me this approach is minimum invasive.
>
> In addition, we may have a case when extension want to know whether
> the deletion is cascaded, or explicitly specified by users. If they want to
> implement same security model on this hook.
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>