[4.x] RLS without central user (role) having bypassrls

[JustinElst]

Original issue

Description

I am using a managed Postgres solution where the admin role does not have and can not get BYPASSRLS.
That leads to the behaviour that when using the 'central connection' i don't have access to tenant data (including 'tennant_user').
That makes the management of tenants impossible.

So I thought of a solution giving the tenancy.rls.user.username (database tenant_user) the rsl policies already implemented by the package.
AND giving the central user a policy with full access:

CREATE POLICY {$table}_central_rls_policy ON {$table} AS PERMISSIVE TO {$centralUserName} USING (true);

This worked for me :)

Why this should be added

I have implemented this in the CreateUserWithRLSPolicies command and 'TableRLSManager' class.
This could use some more refactoring and possibly detecting if central user has BYPASSRLS permissions and ommiting the central policy.
But this is to give you an idea of what I did with this.
What do you think?

Original @stancl reply:

Thanks for the detailed writeup! I have some plans for refactoring the RLS feature, so I think I could take a look at this alongside that. This seems like an implementation detail so there shouldn't be any breaking changes.

Would the ideal behavior be basically checking whether the central user has BYPASSRLS perms when creating the policies, and if it doesn't, append the policies with the AS PERMISSIVE TO ...?

Will just need to find the right spot for this, to not run the check multiple times ideally but also to make this work well with the hashing logic.

[stancl]

Thanks for the PR! Can you revert the formatting changes?

[stancl]

Ah nevermind, I see that there's an extra loop now.

[stancl]

Currently in the process of testing an alternative implementation for this btw, so no need to bother fixing phpstan for now.

[stancl]

Could you provide some concrete reproduction steps for the bug first? ALTER ROLE (central user) NOBYPASSRLS in our tests doesn't seem to do it.

[JustinElst]

The user that is used as the central user in tenancy should not have the superuser or bypassrls.

Could you try with:

ALTER USER *central user* NOBYPASSRLS

Or just create a new user:

create user *central user*
    createdb
    createrole;

Some possible steps to follow:

1. create central_user (see above)
2. setup database grants grant connect, create, temporary on database *yourdatabase* to *central user*;
3. setup schema grants grant create, usage on schema *yourschema* to *central user*;
4. setup tenancy with that user
5. run 'artisan tenants:rls'
6. check if central user can see all resources

Without this pull request the app will throw a error or just not select the data, because the user cannot access the data without rls bypass.

You can check the policies by running select * from pg_policies.

[stancl]

Thanks, we managed to reproduce the issue with those steps. I have a few questions though:

1. What would be a case where the central user has enough permissions to create users and create RLS policies, but not bypass them? If you can grant the former two, I imagine you could just use BYPASSRLS? The only case where I see this not being possible is if you're using RLS yourself for other things and explicitly don't want to use BYPASSRLS.
2. Instead of making two separate policies, we could just remove FORCE ROW LEVEL SECURITY which should still enforce RLS for the tenant user, but not the central user as it's the table owner.

[JustinElst]

Thanks for checking!

I'm using a Scaleway managed Postgresql database and it has limitations:

• users with an admin role have CREATEROLE and CREATEDB privileges.
• users do NOT have SUPERUSER nor REPLICATION privileges.

They have made it so that i can create users from their admin-ui but im unable to grant the users BYPASSRLS. Maybe i'm stupid in not being able to figure out how to do it with their system and limitations but there you go.
If i have a look at the Postgres docs they tell me that i am right in thinking i cant grant BYPASSRLS without super-user.

You must be a superuser to create a new role having the BYPASSRLS attribute.

I'm not sure about the 'FORCE ROW LEVEL SECURITY' removal. How can i test this on my system?

[stancl]

Should be enough to just remove 'FORCE ROW LEVEL SECURITY' from all places in the code.

[JustinElst]

I tested it a bit, and everything seems fine, so that's great! But it does seem less secure, although it probably isn't. It depends on the users that have access to the database. With the other setup all users have to adhere to RLS.
Aside; To remove the force from existing tables this works:

ALTER TABLE *table* NO FORCE ROW LEVEL SECURITY

[stancl]

I think we can just make that part of the RLS statements configurable then? Seems better than maintaining two sets of policies.

[JustinElst]

Off topic, RLS macros
I have been using/creating some macros because I needed to finetune some RLS for tables, they might be useful for your re-implementation of this feature.

use Illuminate\Database\Query\Grammars\PostgresGrammar;
// Add RLS Policy macros
Blueprint::macro('policy', function (string $policy, array $options = []) {
    $prefix = $this->prefix;
    $table = $this->table;

    $this->addCommand(
        'policy',
        compact('prefix', 'table', 'policy', 'options')
    );
});

PostgresGrammar::macro('compilePolicy', function (Blueprint $blueprint, Fluent $command) {
    $prefix = $command->prefix;
    $table = $command->table;
    $policy = $command->policy;
    $options = $command->options;

    $modifiers = [];
    foreach ($options as $key => $value) {
        $modifiers[] = match (true) {
            'as' === $key => "as {$value}",
            'for' === $key => "for {$value}",
            'to' === $key => "to {$value}",
            'using' === $key => "using ({$value})",
            'check' === $key => "with check ({$value})",
            default => throw_if(true, message: "Unknown option '{$key}'."),
        };
    }
    $sqlModifiers = implode(' ', $modifiers);

    return [
        sprintf('CREATE POLICY %s ON %s%s %s', $policy, $prefix, $table, $sqlModifiers),
    ];
});

Blueprint::macro('dropPolicy', function (string $policy) {
    $prefix = $this->prefix;
    $table = $this->table;

    $this->addCommand(
        'dropPolicy',
        compact('prefix', 'table', 'policy')
    );
});

PostgresGrammar::macro('compileDropPolicy', function (Blueprint $blueprint, Fluent $command) {
    $prefix = $command->prefix;
    $table = $command->table;
    $policy = $command->policy;

    return [
        sprintf('DROP POLICY IF EXISTS %s ON %s%s', $policy, $prefix, $table),
    ];
});

Usage

Schema::table('table', function (Blueprint $table) {
    $table->policy(
        '*policy_name*',
        [
            'for' => "INSERT",
            'to' => config('tenancy.rls.user.username'),
            'check' => "*whatever to check*"
        ]
    );
});

[JustinElst]

I think we can just make that part of the RLS statements configurable then? Seems better than maintaining two sets of policies.

Seems about right.

[stancl]

Can you provide some more context for the finetuning? What exactly do you need to customize?

[JustinElst]

Yeah sure, I have one table that contains 'system' and tenant created rows.
All tenants should be able to interact with the system rows. So I run:

Schema::table('table', function (Blueprint $table) {
    $table->policy(
        'table_system_rows_should_be_permissive',
        [
            'as' =>"PERMISSIVE",
            'to' =>config('tenancy.rls.user.username'),
            'using' =>"created_by_type::text = 'system'" // ENUM
        ]
    );
});

And in another instance I wanted to use a function to determine if a user should or should not be able to view nodes in a tree where the root node is set in a tree_owners table:

$table->policy(
    'tree_nodes_tenant_from_root_node_select',
    [
        'for' => "SELECT",
        'to' => config('tenancy.rls.user.username'),
        'using' => "COALESCE(tree_id,find_root_tree(tree_nodes))::bpchar IN ( SELECT tree_owner.id
  FROM tree_owner
  WHERE ((tree_owner.tenant_id)::text = current_setting('my.current_tenant'::text)) OR created_by_type::text = 'system')"
    ]
);

Not very efficient but does the job in my case

[stancl]

Hmm could you expand on the use case in a bit more depth? Normally to interact with central data you could just do tenancy()->central(...) from the tenant context. Do you need to be able to interact with some central data from within otherwise tenant queries, or what's the reason for wanting the policies customized like that? Struggling to see the use case here so more context would help if we are to adjust the package for this.