Re: A little RLS oversight?

On 25 July 2015 at 19:12, Joe Conway <joe.conway@crunchydata.com> wrote:
> On 07/22/2015 02:17 PM, Dean Rasheed wrote:
>> Hmm, I think it probably ought to do more, based on whether or not RLS
>> is being bypassed or in force-mode -- see the first few checks in
>> get_row_security_policies(). Perhaps a new SQL-callable function
>> exposing those checks and calling check_enable_rls(). It's probably
>> still worth including the "c.relrowsecurity = false" check in SQL to
>> save calling the function for the majority of tables that don't have
>> RLS.
>
> Please see the attached patch and let me know what you think. I believe
> the only thing lacking is documentation for the two new user visible
> functions. Comments?
>

I'm not convinced about exporting convert_table_name() from acl.c,
particularly with such a non-descriptive name. It's only a couple of
lines of code, so I think they may as well just be included directly
in the new function, as seems to be common practice elsewhere.

As it stands, passing checkAsUser = GetUserId() to check_enable_rls()
will cause it to skip the check for row_security = OFF, and so it may
falsely report that RLS is active when the user has bypassed it. To
avoid that row_security_active() needs to pass checkAsUser =
InvalidOid to check_enable_rls().

[ Actually there seem to be a few other callers of check_enable_rls()
that suffer the same problem. I don't really understand the reasoning
behind that check in check_enable_rls() - if the current user has the
BYPASSRLS attribute and row_security = OFF, shouldn't RLS be disabled
on every table no matter how it is accessed? ]

I think it would be better if the security context check were part of
this new function too. Right now that can't make any difference, since
only the RI triggers set SECURITY_ROW_LEVEL_DISABLED and this function
cannot be called in that code path, but it's possible that in the
future other code might set SECURITY_ROW_LEVEL_DISABLED, so moving the
check down guarantees that check_enable_rls()/row_security_active()
always accurately return the RLS status for the table.

While I was looking at it, I spotted a couple of other things to tidy
up in existing related code:

* The comment for GetUserIdAndSecContext() needed updating for the new RLS bit.

* Calling GetUserIdAndSecContext() and then throwing away the user_id
returned seems ugly. There is already a code style precedent for
checking the other bits of SecurityRestrictionContext, so I've added a
similar bit-testing function for SECURITY_ROW_LEVEL_DISABLED which
makes this code a bit neater.

Attached is an updated patch (still needs some docs for the functions).

Regards,
Dean