Re: A little RLS oversight?

On 07/26/2015 07:19 AM, Dean Rasheed wrote:
> 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.

fair enough

> 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? ]

Hmmm, I can see that now but find it surprising. Seems like an odd
interface, and probably deserves some explanation in the function
comments. Maybe Stephen or someone else can weigh in on this...

> 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).

Thanks for that. I'll add the docs.

Joe