archtechx/tenancy
1
0
Fork 0
mirror of https://github.com/archtechx/tenancy.git synced 2025-12-12 19:04:02 +00:00
Code Activity

[4.x] TableRLSManager refactor, comment constraints (#1354)

Browse source 
* Add option to provide constraint information in column comment

* Fix code style (php-cs-fixer)

* Correct comment functionality, add comment constraint exception

* Simplify and clarify comment-related TableRLSManager code

* Make path skipping logic more explicit

* Correct terminology, add test for throwing exceptions

* Fix code style (php-cs-fixer)

* Improve comments

* Refactor TableRLSManagerr (dynamic programming, deal with recursive relationships, determine shortest paths while generating the paths)

* Fix code style (php-cs-fixer)

* Improve TableRLSManager comments

* Test uncovered edge cases

* Improve code for determining the shortest path

* Improve readability

* Fix code style (php-cs-fixer)

* Update the tree terminology

* Use consistent shortest path terminology

* Improve comment

* Improve method name

* Simplify and clarify core shortest path generation test

* Clarify and simplify tests, add comments

* Delete excessive test

* Test data separation with comment constraints

* Use tenant id instead of getTenantKey()

* Make higher-level code clearer, improve comments

* Improve comments, delete excessive methods, make methods more concise, position helper methods more appropriately

* Fix code style (php-cs-fixer)

* Add a "single source of truth" for path array format, make lower-level code more concise, improve comments

* Fix code style (php-cs-fixer)

* Correct terminology and comments in TableRLSManager

* Correct terminology in table manager test file

* Improve comments and method name

* Fix typo

* bump php memory limit when running tests

* Delete findShortestPath, merge the code into shortestPathToTenantsTabke

* Minor shortestPathToTenantsTable improvement

* Improve docblocks,as discussed

* Move RLSCommentConstraintException to src/RLS/Exceptions

* Fully cover shouldSkipPathLeadingThrough in tests

* test improvements

* tests: add comment to clarify the chosen path

* formatting

* Fix typo

* Use `===` instead of `Str::is()`

* Refactor constraint formatting in TableRLSManager

* Fix code style (php-cs-fixer)

* Update key names of the formatted constraints

* Rename shouldSkipPathLeadingThrough() to shouldSkipPathLeadingThroughConstraint()

* misc improvements

* code improvements

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Samuel Štancl <samuel@archte.ch>
This commit is contained in:
lukinovec 2025-07-03 21:12:04 +02:00 • committed by GitHub
parent d1f12f594d
commit 4ead17a56b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 777 additions and 355 deletions
Download patch file Download diff file Expand all files Collapse all files

470
tests/RLS/TableManagerTest.php
View file

@ -15,11 +15,13 @@ use Stancl\Tenancy\Listeners\BootstrapTenancy;
use Illuminate\Database\Eloquent\Relations\HasMany;
use Stancl\Tenancy\Listeners\RevertToCentralContext;
use Illuminate\Database\Eloquent\Relations\BelongsTo;
use Illuminate\Database\Schema\ForeignIdColumnDefinition;
use Stancl\Tenancy\Commands\CreateUserWithRLSPolicies;
use Stancl\Tenancy\RLS\PolicyManagers\TableRLSManager;
use Stancl\Tenancy\Bootstrappers\PostgresRLSBootstrapper;
use Stancl\Tenancy\Database\Exceptions\RecursiveRelationshipException;
use function Stancl\Tenancy\Tests\pest;
use Stancl\Tenancy\RLS\Exceptions\RLSCommentConstraintException;
beforeEach(function () {
CreateUserWithRLSPolicies::$forceRls = true;
@ -164,15 +166,22 @@ test('correct rls policies get created with the correct hash using table manager
}
});
test('queries are correctly scoped using RLS', function (bool $forceRls) {
test('queries are correctly scoped using RLS', function (
bool $forceRls,
bool $commentConstraint,
) {
CreateUserWithRLSPolicies::$forceRls = $forceRls;
// 3-levels deep relationship
Schema::create('notes', function (Blueprint $table) {
Schema::create('notes', function (Blueprint $table) use ($commentConstraint) {
$table->id();
$table->string('text')->default('foo');
// no rls comment needed, $scopeByDefault is set to true
$table->foreignId('comment_id')->onUpdate('cascade')->onDelete('cascade')->constrained('comments');
if ($commentConstraint) {
$table->foreignId('comment_id')->comment('rls comments.id');
} else {
$table->foreignId('comment_id')->constrained('comments');
}
$table->timestamps();
});
@ -188,9 +197,9 @@ test('queries are correctly scoped using RLS', function (bool $forceRls) {
$post1 = Post::create([
'text' => 'first post',
'tenant_id' => $tenant1->getTenantKey(),
'author_id' => Author::create(['name' => 'author1', 'tenant_id' => $tenant1->getTenantKey()])->id,
'category_id' => Category::create(['name' => 'category1', 'tenant_id' => $tenant1->getTenantKey()])->id,
'tenant_id' => $tenant1->id,
'author_id' => Author::create(['name' => 'author1', 'tenant_id' => $tenant1->id])->id,
'category_id' => Category::create(['name' => 'category1', 'tenant_id' => $tenant1->id])->id,
]);
$post1Comment = Comment::create(['text' => 'first comment', 'post_id' => $post1->id]);
@ -201,9 +210,9 @@ test('queries are correctly scoped using RLS', function (bool $forceRls) {
$post2 = Post::create([
'text' => 'second post',
'tenant_id' => $tenant2->getTenantKey(),
'author_id' => Author::create(['name' => 'author2', 'tenant_id' => $tenant2->getTenantKey()])->id,
'category_id' => Category::create(['name' => 'category2', 'tenant_id' => $tenant2->getTenantKey()])->id
'tenant_id' => $tenant2->id,
'author_id' => Author::create(['name' => 'author2', 'tenant_id' => $tenant2->id])->id,
'category_id' => Category::create(['name' => 'category2', 'tenant_id' => $tenant2->id])->id
]);
$post2Comment = Comment::create(['text' => 'second comment', 'post_id' => $post2->id]);
@ -319,7 +328,7 @@ test('queries are correctly scoped using RLS', function (bool $forceRls) {
expect(Note::count())->toBe(1);
// Directly inserting records to other tenant's tables should fail (insufficient privilege error – new row violates row-level security policy)
expect(fn () => DB::statement("INSERT INTO posts (text, author_id, category_id, tenant_id) VALUES ('third post', 1, 1, '{$tenant1->getTenantKey()}')"))
expect(fn () => DB::statement("INSERT INTO posts (text, author_id, category_id, tenant_id) VALUES ('third post', 1, 1, '{$tenant1->id}')"))
->toThrow(QueryException::class);
expect(fn () => DB::statement("INSERT INTO comments (text, post_id) VALUES ('third comment', {$post1->id})"))
@ -327,95 +336,90 @@ test('queries are correctly scoped using RLS', function (bool $forceRls) {
expect(fn () => DB::statement("INSERT INTO notes (text, comment_id) VALUES ('baz', {$post1Comment->id})"))
->toThrow(QueryException::class);
})->with([true, false]);
})->with(['forceRls is true' => true, 'forceRls is false' => false])
->with(['comment constraint' => true, 'foreign key constraint' => false]);
test('table rls manager generates relationship trees with tables related to the tenants table', function (bool $scopeByDefault) {
test('table rls manager generates shortest paths that lead to the tenants table correctly', function (bool $scopeByDefault) {
TableRLSManager::$scopeByDefault = $scopeByDefault;
// Only related to the tenants table through nullable columns (directly through tenant_id and indirectly through post_id)
Schema::create('ratings', function (Blueprint $table) {
$table->id();
$table->foreignId('post_id')->nullable()->comment('rls')->constrained();
// No 'rls' comment – should get excluded from path generation when using explicit scoping
$table->string('tenant_id')->nullable();
$table->foreign('tenant_id')->references('id')->on('tenants')->onUpdate('cascade')->onDelete('cascade');
$table->timestamps();
});
/** @var TableRLSManager $manager */
$manager = app(TableRLSManager::class);
$expectedTrees = [
$expectedShortestPaths = [
'authors' => [
// Directly related to tenants
'tenant_id' => [
[
[
'foreignKey' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignId' => 'id',
'nullable' => false,
]
],
],
],
'comments' => [
// Tree starting from the post_id foreign key
'post_id' => [
[
[
'foreignKey' => 'post_id',
'foreignTable' => 'posts',
'foreignId' => 'id',
'nullable' => false,
],
[
'foreignKey' => 'author_id',
'foreignTable' => 'authors',
'foreignId' => 'id',
'nullable' => false,
],
[
'foreignKey' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignId' => 'id',
'nullable' => false,
],
],
[
[
'foreignKey' => 'post_id',
'foreignTable' => 'posts',
'foreignId' => 'id',
'nullable' => false,
],
[
'foreignKey' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignId' => 'id',
'nullable' => true,
],
],
[
'localColumn' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignColumn' => 'id',
],
],
'posts' => [
// Category tree gets excluded because the category table is related to the tenant table
// only through a column with the 'no-rls' comment
'author_id' => [
[
[
'foreignKey' => 'author_id',
'foreignTable' => 'authors',
'foreignId' => 'id',
'nullable' => false,
],
[
'foreignKey' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignId' => 'id',
'nullable' => false,
]
],
[
'localColumn' => 'author_id',
'foreignTable' => 'authors',
'foreignColumn' => 'id',
],
'tenant_id' => [
[
[
'foreignKey' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignId' => 'id',
'nullable' => true,
]
]
[
'localColumn' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignColumn' => 'id',
],
],
'comments' => [
[
'localColumn' => 'post_id',
'foreignTable' => 'posts',
'foreignColumn' => 'id',
],
[
'localColumn' => 'author_id',
'foreignTable' => 'authors',
'foreignColumn' => 'id',
],
[
'localColumn' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignColumn' => 'id',
],
],
// When scoping by default is enabled (implicit scoping),
// the shortest path from the ratings table leads directly through tenant_id.
// When scoping by default is disabled (explicit scoping),
// the shortest path leads through post_id.
'ratings' => $scopeByDefault ? [
[
'localColumn' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignColumn' => 'id',
],
] : [
[
'localColumn' => 'post_id',
'foreignTable' => 'posts',
'foreignColumn' => 'id',
],
[
'localColumn' => 'author_id',
'foreignTable' => 'authors',
'foreignColumn' => 'id',
],
[
'localColumn' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignColumn' => 'id',
],
],
// Articles table is ignored because it's not related to the tenant table in any way
@ -423,123 +427,52 @@ test('table rls manager generates relationship trees with tables related to the
// Categories table is ignored because of the 'no-rls' comment on the tenant_id column
];
expect($manager->generateTrees())->toEqual($expectedTrees);
$expectedShortestPaths = [
'authors' => [
[
'foreignKey' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignId' => 'id',
],
],
'posts' => [
[
'foreignKey' => 'author_id',
'foreignTable' => 'authors',
'foreignId' => 'id',
],
[
'foreignKey' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignId' => 'id',
],
],
'comments' => [
[
'foreignKey' => 'post_id',
'foreignTable' => 'posts',
'foreignId' => 'id',
],
[
'foreignKey' => 'author_id',
'foreignTable' => 'authors',
'foreignId' => 'id',
],
[
'foreignKey' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignId' => 'id',
],
],
];
expect($manager->shortestPaths())->toEqual($expectedShortestPaths);
// Only related to the tenants table through nullable columns – tenant_id and indirectly through post_id
Schema::create('ratings', function (Blueprint $table) {
$table->id();
$table->integer('stars')->default(0);
$table->unsignedBigInteger('post_id')->nullable()->comment('rls');
$table->foreign('post_id')->references('id')->on('posts')->onUpdate('cascade')->onDelete('cascade');
// No 'rls' comment – should get excluded from full trees when using explicit scoping
$table->string('tenant_id')->nullable();
$table->foreign('tenant_id')->references('id')->on('tenants')->onUpdate('cascade')->onDelete('cascade');
$table->timestamps();
});
// The shortest paths should include a path for the ratings table
// That leads through tenant_id – when scoping by default is enabled, that's the shortest path
// When scoping by default is disabled, the shortest path leads through post_id
// This behavior is handled by the manager's generateTrees() method, which is called by shortestPaths()
$shortestPaths = $manager->shortestPaths();
$expectedShortestPath = $scopeByDefault ? [
[
'foreignKey' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignId' => 'id',
],
] : [
[
'foreignKey' => 'post_id',
'foreignTable' => 'posts',
'foreignId' => 'id',
],
[
'foreignKey' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignId' => 'id',
],
];
expect($shortestPaths['ratings'])->toBe($expectedShortestPath);
// Add non-nullable comment_id foreign key
// Add non-nullable comment_id comment constraint
Schema::table('ratings', function (Blueprint $table) {
$table->foreignId('comment_id')->comment('rls')->constrained('comments')->onUpdate('cascade')->onDelete('cascade');
$table->string('comment_id')->comment('rls comments.id');
// Nullable constraint with a non-RLS comment.
// Skipped when scopeByDefault is false,
// not ignored when scopeByDefault is true, but still,
// not preferred since comment_id is valid and non-nullable.
$table->foreignId('author_id')->nullable()->comment('random comment')->constrained('authors');
});
// Non-nullable paths are preferred over nullable paths
// The shortest paths should include a path for the ratings table
// That leads through comment_id instead of tenant_id
$shortestPaths = $manager->shortestPaths();
expect($shortestPaths['ratings'])->toBe([
$expectedShortestPaths['ratings'] = [
[
'foreignKey' => 'comment_id',
'localColumn' => 'comment_id',
'foreignTable' => 'comments',
'foreignId' => 'id',
'foreignColumn' => 'id',
],
[
'foreignKey' => 'post_id',
'localColumn' => 'post_id',
'foreignTable' => 'posts',
'foreignId' => 'id',
'foreignColumn' => 'id',
],
[
'foreignKey' => 'author_id',
// Importantly, the best path goes through authors
// since ratings -> posts is nullable, as well as
// posts -> tenants directly (without going through
// authors first).
'localColumn' => 'author_id',
'foreignTable' => 'authors',
'foreignId' => 'id',
'foreignColumn' => 'id',
],
[
'foreignKey' => 'tenant_id',
'localColumn' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignId' => 'id',
'foreignColumn' => 'id',
],
]);
];
// The shortest paths should now include a path for the ratings table
// that leads through comment_id instead of tenant_id since comment_id
// is not nullable (and therefore preferable) unlike path_id or tenant_id
// even if the latter paths are shorter.
expect($manager->shortestPaths())->toEqual($expectedShortestPaths);
})->with([true, false]);
// https://github.com/archtechx/tenancy/pull/1293
@ -576,7 +509,7 @@ test('forceRls prevents even the table owner from querying his own tables if he
// Create RLS policy for the orders table
pest()->artisan('tenants:rls');
$tenant1->run(fn () => Order::create(['name' => 'order1', 'tenant_id' => $tenant1->getTenantKey()]));
$tenant1->run(fn () => Order::create(['name' => 'order1', 'tenant_id' => $tenant1->id]));
// We are still using the 'administrator' user - owner of the orders table
@ -623,7 +556,7 @@ test('users with BYPASSRLS privilege can bypass RLS regardless of forceRls setti
// Create RLS policy for the orders table
pest()->artisan('tenants:rls');
$tenant1->run(fn () => Order::create(['name' => 'order1', 'tenant_id' => $tenant1->getTenantKey()]));
$tenant1->run(fn () => Order::create(['name' => 'order1', 'tenant_id' => $tenant1->id]));
// We are still using the 'administrator' user
@ -680,38 +613,38 @@ test('table rls manager generates queries correctly', function() {
$paths = [
'primaries' => [
[
'foreignKey' => 'tenant_id',
'localColumn' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignId' => 'id',
'foreignColumn' => 'id',
],
],
'secondaries' => [
[
'foreignKey' => 'primary_id',
'localColumn' => 'primary_id',
'foreignTable' => 'primaries',
'foreignId' => 'id',
'foreignColumn' => 'id',
],
[
'foreignKey' => 'tenant_id',
'localColumn' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignId' => 'id',
'foreignColumn' => 'id',
],
],
'foo' => [
[
'foreignKey' => 'secondary_id',
'localColumn' => 'secondary_id',
'foreignTable' => 'secondaries',
'foreignId' => 'id',
'foreignColumn' => 'id',
],
[
'foreignKey' => 'primary_id',
'localColumn' => 'primary_id',
'foreignTable' => 'primaries',
'foreignId' => 'id',
'foreignColumn' => 'id',
],
[
'foreignKey' => 'tenant_id',
'localColumn' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignId' => 'id',
'foreignColumn' => 'id',
],
],
];
@ -747,18 +680,61 @@ test('table rls manager generates queries correctly', function() {
);
});
test('table manager throws an exception when encountering a recursive relationship', function() {
test('table manager throws an exception when the only available paths lead through recursive relationships', function (bool $useCommentConstraints) {
// We test recursive relations using both foreign key constraints and comment constraints
$makeConstraint = function (ForeignIdColumnDefinition $relation, $table, $column) use ($useCommentConstraints) {
if ($useCommentConstraints) {
$relation->comment("rls $table.$column");
} else {
$relation->constrained($table, $column);
}
};
Schema::create('recursive_posts', function (Blueprint $table) {
$table->id();
$table->foreignId('highlighted_comment_id')->nullable()->comment('rls')->constrained('comments');
});
Schema::table('comments', function (Blueprint $table) {
$table->foreignId('recursive_post_id')->comment('rls')->constrained('recursive_posts');
Schema::create('recursive_comments', function (Blueprint $table) {
$table->id();
});
expect(fn () => app(TableRLSManager::class)->generateTrees())->toThrow(RecursiveRelationshipException::class);
});
Schema::table('recursive_posts', function (Blueprint $table) use ($makeConstraint) {
$makeConstraint($table->foreignId('highlighted_comment_id')->nullable(), 'recursive_comments', 'id');
});
Schema::table('recursive_comments', function (Blueprint $table) use ($makeConstraint) {
$makeConstraint($table->foreignId('recursive_post_id'), 'recursive_posts', 'id');
});
expect(fn () => app(TableRLSManager::class)->shortestPaths())->toThrow(RecursiveRelationshipException::class);
Schema::table('recursive_comments', function (Blueprint $table) use ($makeConstraint, $useCommentConstraints) {
// Add another recursive relationship to demonstrate a more complex case
$makeConstraint($table->foreignId('related_post_id'), 'recursive_posts', 'id');
// Add a foreign key to the current table (= self-referencing constraint)
$makeConstraint($table->foreignId('parent_comment_id'), 'recursive_comments', 'id');
// Add tenant_id to break the recursion - RecursiveRelationshipException should not be thrown
// We cannot use $makeConstraint() here since tenant_id is a string column
if ($useCommentConstraints) {
$table->string('tenant_id')->comment('rls tenants.id');
} else {
$table->string('tenant_id')->comment('rls')->nullable();
$table->foreign('tenant_id')->references('id')->on('tenants')->onUpdate('cascade')->onDelete('cascade');
}
});
// Doesn't throw an exception anymore
$shortestPaths = app(TableRLSManager::class)->shortestPaths();
// Generated paths include both the recursive_posts and the recursive_comments tables
// because they actually lead to the tenants table now.
//
// recursive_comments has a direct path to tenants, recursive_posts has a path
// to tenants through recursive_comments
expect(array_keys($shortestPaths))->toContain('recursive_posts', 'recursive_comments');
})->with([true, false]);
test('table manager ignores recursive relationship if the foreign key responsible for the recursion has no-rls comment', function() {
Schema::create('recursive_posts', function (Blueprint $table) {
@ -766,13 +742,91 @@ test('table manager ignores recursive relationship if the foreign key responsibl
$table->foreignId('highlighted_comment_id')->nullable()->comment('no-rls')->constrained('comments');
});
// Add a foreign key constraint to the comments table to introduce a recursive relationship
// Note that the comments table still has the post_id foreign key that leads to the tenants table
Schema::table('comments', function (Blueprint $table) {
$table->foreignId('recursive_post_id')->comment('rls')->constrained('recursive_posts');
});
expect(fn () => app(TableRLSManager::class)->generateTrees())->not()->toThrow(RecursiveRelationshipException::class);
// No exception thrown because
// the highlighted_comment_id foreign key has a no-rls comment
$shortestPaths = app(TableRLSManager::class)->shortestPaths();
expect(array_keys($shortestPaths))
->toContain('posts', 'comments')
// Shortest paths do not include the recursive_posts table
// because it has a 'no-rls' comment on its only foreign key
->not()->toContain('recursive_posts');
});
test('table manager can generate paths leading through comment constraint columns', function() {
// Drop extra tables created in beforeEach
Schema::dropIfExists('reactions');
Schema::dropIfExists('comments');
Schema::dropIfExists('posts');
Schema::dropIfExists('authors');
Schema::create('non_constrained_users', function (Blueprint $table) {
$table->id();
$table->string('tenant_id')->comment('rls tenants.id'); // Comment constraint
});
Schema::create('non_constrained_posts', function (Blueprint $table) {
$table->id();
$table->foreignId('author_id')->comment('rls non_constrained_users.id'); // Comment constraint
});
/** @var TableRLSManager $manager */
$manager = app(TableRLSManager::class);
$expectedPaths = [
'non_constrained_posts' => [
[
'localColumn' => 'author_id',
'foreignTable' => 'non_constrained_users',
'foreignColumn' => 'id',
],
[
'localColumn' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignColumn' => 'id',
],
],
'non_constrained_users' => [
[
'localColumn' => 'tenant_id',
'foreignTable' => 'tenants',
'foreignColumn' => 'id',
],
],
];
expect($manager->shortestPaths())->toEqual($expectedPaths);
});
test('table manager throws an exception when comment constraint is incorrect', function(string $comment, string $exceptionMessage) {
Schema::create('non_constrained_users', function (Blueprint $table) use ($comment) {
$table->id();
$table->string('tenant_id')->comment($comment); // Invalid comment constraint
});
/** @var TableRLSManager $manager */
$manager = app(TableRLSManager::class);
expect(fn () => $manager->shortestPaths())->toThrow(
RLSCommentConstraintException::class,
$exceptionMessage
);
})->with([
['rls ', 'Malformed comment constraint on non_constrained_users'], // Missing table.column
['rls tenants', 'Malformed comment constraint on non_constrained_users'], // Missing column part
['rls tenants.', 'Malformed comment constraint on non_constrained_users'], // Missing column part
['rls .id', 'Malformed comment constraint on non_constrained_users'], // Missing table part
['rls tenants.foreign.id', 'Malformed comment constraint on non_constrained_users'], // Too many parts
['rls nonexistent-table.id', 'references non-existent table'],
['rls tenants.nonexistent-column', 'references non-existent column'],
]);
function createPostgresUser(string $username, string $password = 'password', bool $bypassRls = false): array
{
try {