Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.x] Make forcing RLS configurable #1293

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
21 changes: 13 additions & 8 deletions src/Commands/CreateUserWithRLSPolicies.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,16 @@ 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 RLS scoping on the tables, 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
* the `ALTER TABLE {$table} FORCE ROW LEVEL SECURITY` query in the `enableRLS` method.
*/
public static bool $forceRls = true;

public function handle(PermissionControlledPostgreSQLSchemaManager $manager): int
{
$username = config('tenancy.rls.user.username');
Expand Down Expand Up @@ -49,14 +59,9 @@ protected function enableRLS(string $table): void
// 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");
}
}

/**
Expand Down
14 changes: 9 additions & 5 deletions tests/RLS/PolicyTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
use Stancl\Tenancy\Bootstrappers\PostgresRLSBootstrapper;

beforeEach(function () {
CreateUserWithRLSPolicies::$forceRls = true;
TraitRLSManager::$excludedModels = [Article::class];
TraitRLSManager::$modelDirectories = [__DIR__ . '/Etc'];

Expand Down Expand Up @@ -78,6 +79,10 @@
});
});

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("
Expand Down Expand Up @@ -183,7 +188,9 @@
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');
Expand Down Expand Up @@ -215,7 +222,4 @@
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]);
131 changes: 129 additions & 2 deletions tests/RLS/TableManagerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
use Stancl\Tenancy\Database\Exceptions\RecursiveRelationshipException;

beforeEach(function () {
CreateUserWithRLSPolicies::$forceRls = true;
stancl marked this conversation as resolved.
Show resolved Hide resolved
TableRLSManager::$scopeByDefault = true;

Event::listen(TenancyInitialized::class, BootstrapTenancy::class);
Expand Down Expand Up @@ -106,6 +107,10 @@
});
});

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'));

Expand Down Expand Up @@ -158,7 +163,9 @@
}
});

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();
Expand Down Expand Up @@ -319,7 +326,7 @@

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;
Expand Down Expand Up @@ -534,6 +541,99 @@
]);
})->with([true, false]);

test('table owner sees all the records when forceRls is false while other users only see records scoped to them', function() {
CreateUserWithRLSPolicies::$forceRls = false;

// Drop all tables created in beforeEach
DB::statement("DROP TABLE authors, categories, posts, comments, reactions, articles;");

[$username, $password] = createPostgresUser('central_user');

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, $tenant2] = [Tenant::create(), Tenant::create()];

pest()->artisan('tenants:rls');

[$order1, $order2] = [
Order::create(['name' => 'order1', 'tenant_id' => $tenant1->getTenantKey()]),
Order::create(['name' => 'order2', 'tenant_id' => $tenant2->getTenantKey()]),
];

// The table owner should see all the records
expect(Order::all())->toHaveCount(2);
Comment on lines +578 to +579
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this test, add a dataset for $forceRls so the difference is actually observed. Assuming createPostgresUser() creates a user without BYPASSRLS, the change should be simple.


tenancy()->initialize($tenant1);

// The tenant users should only see their records
expect(Order::count())->toBe(1);
expect(Order::first()->name)->toBe($order1->name);

tenancy()->initialize($tenant2);

expect(Order::count())->toBe(1);
expect(Order::first()->name)->toBe($order2->name);
});

// https://github.com/archtechx/tenancy/pull/1288
test('user without BYPASSRLS can only query owned tables if forceRls is true', function(bool $forceRls) {
CreateUserWithRLSPolicies::$forceRls = $forceRls;

// Drop all tables created in beforeEach
DB::statement("DROP TABLE authors, categories, posts, comments, reactions, articles;");

[$username, $password] = createPostgresUser('administrator');

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');

Order::create(['name' => 'order1', 'tenant_id' => $tenant1->getTenantKey()]);

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.
expect(fn () => Order::first())->toThrow(QueryException::class, 'unrecognized configuration parameter');
} 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('table rls manager generates queries correctly', function() {
expect(array_values(app(TableRLSManager::class)->generateQueries()))->toEqualCanonicalizing([
<<<SQL
Expand Down Expand Up @@ -649,6 +749,28 @@
expect(fn () => app(TableRLSManager::class)->generateTrees())->toThrow(RecursiveRelationshipException::class);
});

function createPostgresUser(string $username, string $password = 'password'): array
{
try {
DB::statement("DROP OWNED BY {$username};");
} catch (\Throwable $th) {}

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 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 = [];
Expand Down Expand Up @@ -701,3 +823,8 @@ class Author extends Model
{
protected $guarded = [];
}

class Order extends Model
{
protected $guarded = [];
}
13 changes: 8 additions & 5 deletions tests/RLS/TraitManagerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
use Stancl\Tenancy\Database\Concerns\BelongsToPrimaryModel;

beforeEach(function () {
CreateUserWithRLSPolicies::$forceRls = true;
TraitRLSManager::$implicitRLS = true;
TraitRLSManager::$modelDirectories = [__DIR__ . '/Etc'];
TraitRLSManager::$excludedModels = [Article::class];
Expand Down Expand Up @@ -77,6 +78,10 @@
});
});

afterEach(function () {
CreateUserWithRLSPolicies::$forceRls = true;
});

test('correct rls policies get created with the correct hash using trait manager', function () {
$manager = app(TraitRLSManager::class);

Expand Down Expand Up @@ -148,7 +153,8 @@
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;
Expand Down Expand Up @@ -262,10 +268,7 @@

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 */
Expand Down
Loading