From 2057e1e5ae469d305144d9e5f8a842d915d489f9 Mon Sep 17 00:00:00 2001 From: lukinovec Date: Thu, 5 Jun 2025 05:06:05 +0200 Subject: [PATCH] [4.x] Make forcing RLS configurable (#1293) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add `$forceRls` static property to tenants:rls * Set `$forceRls` in tests where scoping is tested, add non-superuser, non-bypassrls table owner test * Move DROP TABLE statement * Remove try/catch * Put DROP OWNED BY into try/catch * Static property cleanup in afterEach * Make with() matrix syntax more clear by using with() multiple times * Fix typo, improve comment * Move and update force RLS comment * Add test for `$forceRls = false`, refactor BYPASSRLS test * Update link in test comment * Add a dataset for `$forceRls` in the table owner test, fix BYPASSRLS test * Correct PR link comment * minor fixes * Add test that makes the bypassrls/forceRls behavior clear * Delete redundant test * cleanup * Update tests/RLS/TableManagerTest.php Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Samuel Štancl Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/Commands/CreateUserWithRLSPolicies.php | 28 ++-- tests/RLS/PolicyTest.php | 14 +- tests/RLS/TableManagerTest.php | 146 ++++++++++++++++++++- tests/RLS/TraitManagerTest.php | 13 +- 4 files changed, 181 insertions(+), 20 deletions(-) diff --git a/src/Commands/CreateUserWithRLSPolicies.php b/src/Commands/CreateUserWithRLSPolicies.php index aa171d58..420df935 100644 --- a/src/Commands/CreateUserWithRLSPolicies.php +++ b/src/Commands/CreateUserWithRLSPolicies.php @@ -22,6 +22,23 @@ class CreateUserWithRLSPolicies extends Command protected $description = "Creates RLS policies for all tables related to the tenant table. Also creates the RLS user if it doesn't exist yet"; + /** + * Force, rather than just enable, the created RLS policies. + * + * By default, table owners bypass RLS policies. When this is enabled, + * they also need the BYPASSRLS permission. If your setup lets you create + * a user with BYPASSRLS, you may prefer leaving this on for additional + * safety. Otherwise, if you can't use BYPASSRLS, you can set this to false + * and depend on the behavior of table owners bypassing RLS automatically. + * + * This setting generally doesn't affect behavior at all with "default" + * setups, however if you have a more custom setup, with additional users + * involved (e.g. central connection user not being the same user that + * creates tables, or the created "RLS user" creating some tables) you + * should take care with how you configure this. + */ + public static bool $forceRls = true; + public function handle(PermissionControlledPostgreSQLSchemaManager $manager): int { $username = config('tenancy.rls.user.username'); @@ -49,14 +66,9 @@ class CreateUserWithRLSPolicies extends Command // Enable RLS scoping on the table (without this, queries won't be scoped using RLS) DB::statement("ALTER TABLE {$table} ENABLE ROW LEVEL SECURITY"); - /** - * Force RLS scoping on the table, so that the table owner users - * don't bypass the scoping – table owners bypass RLS by default. - * - * E.g. when using a custom implementation where you create tables as the RLS user, - * the queries won't be scoped for the RLS user unless we force the RLS scoping using this query. - */ - DB::statement("ALTER TABLE {$table} FORCE ROW LEVEL SECURITY"); + if (static::$forceRls) { + DB::statement("ALTER TABLE {$table} FORCE ROW LEVEL SECURITY"); + } } /** diff --git a/tests/RLS/PolicyTest.php b/tests/RLS/PolicyTest.php index 7278776a..ee9bf5cc 100644 --- a/tests/RLS/PolicyTest.php +++ b/tests/RLS/PolicyTest.php @@ -20,6 +20,7 @@ use Stancl\Tenancy\Bootstrappers\PostgresRLSBootstrapper; use function Stancl\Tenancy\Tests\pest; beforeEach(function () { + CreateUserWithRLSPolicies::$forceRls = true; TraitRLSManager::$excludedModels = [Article::class]; TraitRLSManager::$modelDirectories = [__DIR__ . '/Etc']; @@ -79,6 +80,10 @@ beforeEach(function () { }); }); +afterEach(function () { + CreateUserWithRLSPolicies::$forceRls = true; +}); + // Regression test for https://github.com/archtechx/tenancy/pull/1280 test('rls command doesnt fail when a view is in the database', function (string $manager) { DB::statement(" @@ -184,7 +189,9 @@ test('rls command recreates policies if the force option is passed', function (s TraitRLSManager::class, ]); -test('queries will stop working when the tenant session variable is not set', function(string $manager) { +test('queries will stop working when the tenant session variable is not set', function(string $manager, bool $forceRls) { + CreateUserWithRLSPolicies::$forceRls = $forceRls; + config(['tenancy.rls.manager' => $manager]); $sessionVariableName = config('tenancy.rls.session_variable_name'); @@ -216,7 +223,4 @@ test('queries will stop working when the tenant session variable is not set', fu INSERT INTO posts (text, tenant_id, author_id) VALUES ('post2', ?, ?) SQL, [$tenant->id, $authorId]))->toThrow(QueryException::class); -})->with([ - TableRLSManager::class, - TraitRLSManager::class, -]); +})->with([TableRLSManager::class, TraitRLSManager::class])->with([true, false]); diff --git a/tests/RLS/TableManagerTest.php b/tests/RLS/TableManagerTest.php index fd9c6f44..c0520a1d 100644 --- a/tests/RLS/TableManagerTest.php +++ b/tests/RLS/TableManagerTest.php @@ -22,6 +22,7 @@ use Stancl\Tenancy\Database\Exceptions\RecursiveRelationshipException; use function Stancl\Tenancy\Tests\pest; beforeEach(function () { + CreateUserWithRLSPolicies::$forceRls = true; TableRLSManager::$scopeByDefault = true; Event::listen(TenancyInitialized::class, BootstrapTenancy::class); @@ -107,6 +108,10 @@ beforeEach(function () { }); }); +afterEach(function () { + CreateUserWithRLSPolicies::$forceRls = true; +}); + test('correct rls policies get created with the correct hash using table manager', function() { $manager = app(config('tenancy.rls.manager')); @@ -159,7 +164,9 @@ test('correct rls policies get created with the correct hash using table manager } }); -test('queries are correctly scoped using RLS', function() { +test('queries are correctly scoped using RLS', function (bool $forceRls) { + CreateUserWithRLSPolicies::$forceRls = $forceRls; + // 3-levels deep relationship Schema::create('notes', function (Blueprint $table) { $table->id(); @@ -320,7 +327,7 @@ test('queries are correctly scoped using RLS', function() { expect(fn () => DB::statement("INSERT INTO notes (text, comment_id) VALUES ('baz', {$post1Comment->id})")) ->toThrow(QueryException::class); -}); +})->with([true, false]); test('table rls manager generates relationship trees with tables related to the tenants table', function (bool $scopeByDefault) { TableRLSManager::$scopeByDefault = $scopeByDefault; @@ -535,6 +542,109 @@ test('table rls manager generates relationship trees with tables related to the ]); })->with([true, false]); +// https://github.com/archtechx/tenancy/pull/1293 +test('forceRls prevents even the table owner from querying his own tables if he doesnt have a BYPASSRLS permission', function (bool $forceRls) { + CreateUserWithRLSPolicies::$forceRls = $forceRls; + + // Drop all tables created in beforeEach + DB::statement("DROP TABLE authors, categories, posts, comments, reactions, articles;"); + + // Create a new user so we have full control over the permissions. + // We explicitly set bypassRls to false. + [$username, $password] = createPostgresUser('administrator', bypassRls: false); + + config(['database.connections.central' => array_merge(config('database.connections.pgsql'), [ + 'username' => $username, + 'password' => $password, + ])]); + + DB::reconnect(); + + // This table is owned by the newly created 'administrator' user + Schema::create('orders', function (Blueprint $table) { + $table->id(); + $table->string('name'); + + $table->string('tenant_id')->comment('rls'); + $table->foreign('tenant_id')->references('id')->on('tenants')->onUpdate('cascade')->onDelete('cascade'); + + $table->timestamps(); + }); + + $tenant1 = Tenant::create(); + + // Create RLS policy for the orders table + pest()->artisan('tenants:rls'); + + $tenant1->run(fn () => Order::create(['name' => 'order1', 'tenant_id' => $tenant1->getTenantKey()])); + + // We are still using the 'administrator' user - owner of the orders table + + if ($forceRls) { + // RLS is forced, so by default, not even the table owner should be able to query the table protected by the RLS policy. + // The RLS policy is not being bypassed, 'unrecognized configuration parameter' means + // that the my.current_tenant session variable isn't set -- the RLS policy is *still* being enforced. + expect(fn () => Order::first())->toThrow(QueryException::class, 'unrecognized configuration parameter "my.current_tenant"'); + } else { + // RLS is not forced, so the table owner should be able to query the table, bypassing the RLS policy + expect(Order::first())->not()->toBeNull(); + } +})->with([true, false]); + +test('users with BYPASSRLS privilege can bypass RLS regardless of forceRls setting', function (bool $forceRls, bool $bypassRls) { + CreateUserWithRLSPolicies::$forceRls = $forceRls; + + // Drop all tables created in beforeEach + DB::statement("DROP TABLE authors, categories, posts, comments, reactions, articles;"); + + // Create a new user so we have control over his BYPASSRLS permission + // and use that as the new central connection user + [$username, $password] = createPostgresUser('administrator', 'password', $bypassRls); + + config(['database.connections.central' => array_merge(config('database.connections.pgsql'), [ + 'username' => $username, + 'password' => $password, + ])]); + + DB::reconnect(); + + Schema::create('orders', function (Blueprint $table) { + $table->id(); + $table->string('name'); + + $table->string('tenant_id')->comment('rls'); + $table->foreign('tenant_id')->references('id')->on('tenants')->onUpdate('cascade')->onDelete('cascade'); + + $table->timestamps(); + }); + + $tenant1 = Tenant::create(); + + // Create RLS policy for the orders table + pest()->artisan('tenants:rls'); + + $tenant1->run(fn () => Order::create(['name' => 'order1', 'tenant_id' => $tenant1->getTenantKey()])); + + // We are still using the 'administrator' user + + if ($bypassRls) { + // Users with BYPASSRLS can always query tables regardless of forceRls setting + expect(Order::count())->toBe(1); + expect(Order::first()->name)->toBe('order1'); + } else { + // Users without BYPASSRLS are subject to RLS policies even if they're table owners when forceRls is true + // OR they can bypass as table owners (when forceRls=false) + if ($forceRls) { + // Even table owners need session variable -- this means RLS was NOT bypassed + expect(fn () => Order::first())->toThrow(QueryException::class, 'unrecognized configuration parameter "my.current_tenant"'); + } else { + // Table owners can bypass RLS automatically when forceRls is false + expect(Order::count())->toBe(1); + expect(Order::first()->name)->toBe('order1'); + } + } +})->with([true, false])->with([true, false]); + test('table rls manager generates queries correctly', function() { expect(array_values(app(TableRLSManager::class)->generateQueries()))->toEqualCanonicalizing([ << app(TableRLSManager::class)->generateTrees())->not()->toThrow(RecursiveRelationshipException::class); }); +function createPostgresUser(string $username, string $password = 'password', bool $bypassRls = false): array +{ + try { + DB::statement("DROP OWNED BY {$username};"); + } catch (\Throwable) {} + + DB::statement("DROP USER IF EXISTS {$username};"); + + DB::statement("CREATE USER {$username} WITH ENCRYPTED PASSWORD '{$password}'"); + DB::statement("ALTER USER {$username} CREATEDB"); + DB::statement("ALTER USER {$username} CREATEROLE"); + + // Grant BYPASSRLS privilege if requested + if ($bypassRls) { + DB::statement("ALTER USER {$username} BYPASSRLS"); + } + + // Grant privileges to the new central user + DB::statement("GRANT ALL PRIVILEGES ON DATABASE main to {$username}"); + DB::statement("GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA public TO {$username}"); + DB::statement("GRANT ALL ON SCHEMA public TO {$username}"); + DB::statement("ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT ALL PRIVILEGES ON TABLES TO {$username}"); + DB::statement("GRANT ALL PRIVILEGES ON ALL SEQUENCES IN SCHEMA public TO {$username}"); + + return [$username, $password]; +} + class Post extends Model { protected $guarded = []; @@ -715,3 +852,8 @@ class Author extends Model { protected $guarded = []; } + +class Order extends Model +{ + protected $guarded = []; +} diff --git a/tests/RLS/TraitManagerTest.php b/tests/RLS/TraitManagerTest.php index af2f6f84..b88b147b 100644 --- a/tests/RLS/TraitManagerTest.php +++ b/tests/RLS/TraitManagerTest.php @@ -28,6 +28,7 @@ use Stancl\Tenancy\Database\Concerns\BelongsToPrimaryModel; use function Stancl\Tenancy\Tests\pest; beforeEach(function () { + CreateUserWithRLSPolicies::$forceRls = true; TraitRLSManager::$implicitRLS = true; TraitRLSManager::$modelDirectories = [__DIR__ . '/Etc']; TraitRLSManager::$excludedModels = [Article::class]; @@ -78,6 +79,10 @@ beforeEach(function () { }); }); +afterEach(function () { + CreateUserWithRLSPolicies::$forceRls = true; +}); + test('correct rls policies get created with the correct hash using trait manager', function () { $manager = app(TraitRLSManager::class); @@ -149,7 +154,8 @@ test('global scope is not applied when using rls with single db traits', functio expect(NonRLSComment::make()->hasGlobalScope(ParentModelScope::class))->toBeFalse(); }); -test('queries are correctly scoped using RLS with trait rls manager', function (bool $implicitRLS) { +test('queries are correctly scoped using RLS with trait rls manager', function (bool $implicitRLS, bool $forceRls) { + CreateUserWithRLSPolicies::$forceRls = $forceRls; TraitRLSManager::$implicitRLS = $implicitRLS; $postModel = $implicitRLS ? NonRLSPost::class : Post::class; @@ -263,10 +269,7 @@ test('queries are correctly scoped using RLS with trait rls manager', function ( expect(fn () => DB::statement("INSERT INTO comments (text, post_id) VALUES ('third comment', {$post1->id})")) ->toThrow(QueryException::class); -})->with([ - true, - false -]); +})->with([true, false])->with([true, false]); test('trait rls manager generates queries correctly', function() { /** @var TraitRLSManager $manager */