On Monday, July 27, 2015, Dean Rasheed <
dean.a.rasheed@gmail.com> wrote:
On 27 July 2015 at 22:36, Joe Conway <mail@joeconway.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 07/27/2015 01:25 PM, Dean Rasheed wrote:
>> Looks good to me, except I'm not sure about those latest changes
>> because I don't understand the reasoning behind the logic in
>> check_enable_rls() when row_security is set to OFF.
>>
>> I would expect that if the current user has permission to bypass
>> RLS, and they have set row_security to OFF, then it should be off
>> for all tables that they have access to, regardless of how they
>> access those tables (directly or through a view). If it really is
>> intentional that RLS remains active when querying through a view
>> not owned by the table's owner, then the other calls to
>> check_enable_rls() should probably be left as they were, since the
>> table might have been updated through such a view and that code
>> can't really tell at that point.
>
> After looking again at those three call sites, I'm now on the fence.
> All three are in functions which are trying to avoid leaking info in
> error messages. If we leave the original coding, then the messages
> will remain the same for a given user regardless of the row_security
> setting, whereas if we change them as in my latest patch, the messages
> will be different depending on row_security for users with BYPASSRLS.
>
> I think if we do the former (original coding) there should probably be
> a comment added to indicate we are doing that intentionally.
>
> Thoughts?
>
I could go either way on that, but I don't think it's too serious to
worry about leaking information to users with BYPASSRLS.
AFK at the moment, but my thinking was that we should avoid having the error message change based on what a GUC is set to. I agree that there should be comments which explain that.
Thanks!
Stephen