Re: [PATCH] Add reloption for views to enable RLS - Mailing list pgsql-hackers

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