Hi all,
again, many thanks for the reviews and testing!
On 2/4/22 17:09, Laurenz Albe wrote:
> I also see no reason to split a small patch like this into three parts.
I've split it into the three unrelated parts (code, docs, tests) to ease
review, but I happily carry it as one patch too.
> In the attached, I dealt with the above and went over the comments.
> How do you like it?
That is really nice, I used it to base v6 on.
On 2/9/22 17:40, walther@technowledgy.de wrote:
> Ah, good find. In that case, I suggest to change the docs slightly to
> say that the schema will not be checked.
>
> In one place it's described as "it will cause all access to the
> underlying tables to be checked as ..." which is fine, I think. But in
> another place it's "access to tables, functions and *other objects*
> referenced in the view, ..." which is misleading
I removed the reference to "other objects" for now in v6.
>> I agree that the name "security_invoker" is suggestive of SECURITY
>> INVOKER
>> in CREATE FUNCTION, but the behavior is different.
>> Perhaps the solution is as simple as choosing a different name that does
>> not prompt this association, for example "permissions_invoker".
>
> Yes, given that there is not much that can be done about the
> functionality anymore, a different name would be better. This would also
> avoid the implicit "if security_invoker=false, the view behaves like
> SECURITY DEFINER" association, which is also clearly wrong. And this
> assumption is actually what made me think the chained views example was
> somehow off.
>
> I am not convinced "permissions_invoker" is much better, though. The
> difference between SECURITY INVOKER and SECURITY DEFINER is invoker vs
> definer... where, I think, we need something else to describe what we
> currently have and what the patch provides.
>
> Maybe we can look at it from the other perspective: Both ways of
> operating keep the CURRENT_USER the same, pretty much like what we
> understand "security invoker" should do. The difference, however, is the
> current default in which the permissions are checked with the view
> *owner*. Let's treat this difference as the thing that can be set:
> security_owner=true|false. Or run_as_owner=true|false.
>
> xxx_owner=true would be the default and xxx_owner=false could be set
> explicitly to get the behavior we are looking for in this patch?
I'm not sure if an option which is on by default would be best, IMHO. I
would rather have an off-by-default option, so that you explicitly have
to turn *on* that behavior rather than turning *off* the current.
[ Pretty much bike-shedding here, but if the agreement comes to one of
"xxx_owner" I won't mind it either. ]
My best suggestions is maybe something like run_as_invoker=t|f, but that
would probably raise the same "invoker vs definer" association.
I left it for now as-is.
>> I guess more documentation how this works would be a good idea.
>> [...] but since it exposes this
>> in new ways, it might as well clarify how all this works.
I tried to clarify this situation in the documentation in a concise
matter, I'd appreciate further feedback on that.
Thanks,
Christoph Heiss
