From 4ead17a56ba09e1ebab1f03aa0291721f7193ea9 Mon Sep 17 00:00:00 2001 From: lukinovec Date: Thu, 3 Jul 2025 21:12:04 +0200 Subject: [PATCH] [4.x] TableRLSManager refactor, comment constraints (#1354) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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] Co-authored-by: Samuel Štancl --- .../RLSCommentConstraintException.php | 15 + src/RLS/PolicyManagers/TableRLSManager.php | 645 ++++++++++++++---- tests/RLS/TableManagerTest.php | 470 +++++++------ tests/TestCase.php | 2 + 4 files changed, 777 insertions(+), 355 deletions(-) create mode 100644 src/RLS/Exceptions/RLSCommentConstraintException.php diff --git a/src/RLS/Exceptions/RLSCommentConstraintException.php b/src/RLS/Exceptions/RLSCommentConstraintException.php new file mode 100644 index 00000000..7ca133f9 --- /dev/null +++ b/src/RLS/Exceptions/RLSCommentConstraintException.php @@ -0,0 +1,15 @@ +generateQueries(); + * + * // Generate the shortest path from table X to the tenants table. + * // Calls shortestPathToTenantsTable() recursively. + * // The paths will be returned in this format: + * // [ + * // 'foo_table' => [...$stepsLeadingToTenantsTable], + * // 'bar_table' => [ + * // [ + * // 'localColumn' => 'post_id', + * // 'foreignTable' => 'posts', + * // 'foreignColumn' => 'id' + * // ], + * // [ + * // 'localColumn' => 'tenant_id', + * // 'foreignTable' => 'tenants', + * // 'foreignColumn' => 'id' + * // ], + * // ], + * // This is used in the CreateUserWithRLSPolicies command. + * $shortestPath = app(TableRLSManager::class)->shortestPaths(); + * + * generateQueries() and shortestPaths() methods are the only public methods of this class. + * The rest of the methods are protected, and only used internally. + * To see how they're structured and how they work, you can check their annotations. + */ class TableRLSManager implements RLSPolicyManager { + /** + * When true, all valid constraints are considered while generating paths for RLS policies, + * unless explicitly marked with a 'no-rls' comment. + * + * When false, only columns explicitly marked with 'rls' or 'rls table.column' comments are considered. + */ public static bool $scopeByDefault = true; public function __construct( protected DatabaseManager $database ) {} - public function generateQueries(array $trees = []): array + /** + * Generate queries that will be executed by the tenants:rls command + * for creating RLS policies for all tables related to the tenants table + * or for a passed array of paths. + * + * The passed paths should be formatted like this: + * [ + * 'table_name' => [...$stepsLeadingToTenantsTable] + * ] + */ + public function generateQueries(array $paths = []): array { $queries = []; - foreach ($trees ?: $this->shortestPaths() as $table => $path) { + foreach ($paths ?: $this->shortestPaths() as $table => $path) { $queries[$table] = $this->generateQuery($table, $path); } @@ -28,185 +96,415 @@ class TableRLSManager implements RLSPolicyManager } /** - * Reduce trees to shortest paths (structured like ['table_foo' => $shortestPathForFoo, 'table_bar' => $shortestPathForBar]). + * Generate shortest paths from each table to the tenants table, + * structured like ['table_foo' => $shortestPathFromFoo, 'table_bar' => $shortestPathFromBar]. * * For example: * * 'posts' => [ * [ - * 'foreignKey' => 'tenant_id', + * 'localColumn' => 'tenant_id', * 'foreignTable' => 'tenants', - * 'foreignId' => 'id' + * 'foreignColumn' => 'id' * ], * ], * 'comments' => [ * [ - * 'foreignKey' => 'post_id', + * 'localColumn' => 'post_id', * 'foreignTable' => 'posts', - * 'foreignId' => 'id' + * 'foreignColumn' => 'id' * ], * [ - * 'foreignKey' => 'tenant_id', + * 'localColumn' => 'tenant_id', * 'foreignTable' => 'tenants', - * 'foreignId' => 'id' + * 'foreignColumn' => 'id' * ], * ], + * + * @throws RecursiveRelationshipException When tables have recursive relationships and no other valid paths + * @throws RLSCommentConstraintException When comment constraints are malformed */ - public function shortestPaths(array $trees = []): array + public function shortestPaths(): array { - $reducedTrees = []; + $shortestPaths = []; - foreach ($trees ?: $this->generateTrees() as $table => $tree) { - $reducedTrees[$table] = $this->findShortestPath($this->filterNonNullablePaths($tree) ?: $tree); + foreach ($this->getTableNames() as $tableName) { + // Generate the shortest path from table named $tableName to the tenants table + $shortestPath = $this->shortestPathToTenantsTable($tableName); + + if ($this->isValidPath($shortestPath)) { + // Format path steps to a more readable format (keep only the needed data) + $shortestPaths[$tableName] = array_map(fn (array $step) => [ + 'localColumn' => $step['localColumn'], + 'foreignTable' => $step['foreignTable'], + 'foreignColumn' => $step['foreignColumn'], + ], $shortestPath['steps']); + } + + // No valid path found. The shortest path either + // doesn't lead to the tenants table (ignore), + // or leads through a recursive relationship (throw an exception). + if ($shortestPath['recursive_relationship']) { + throw new RecursiveRelationshipException( + "Table '{$tableName}' has recursive relationships with no other valid paths to the tenants table." + ); + } } - return $reducedTrees; + return $shortestPaths; } /** - * Generate trees of paths that lead to the tenants table - * for the foreign keys of all tables – only the paths that lead to the tenants table are included. + * Create a path array with the given parameters. + * This method serves as a 'single source of truth' for the path array structure. * - * Also unset the 'comment' key from the retrieved path steps. + * The 'steps' key contains the path steps returned by shortestPaths(). + * The 'dead_end' and 'recursive_relationship' keys are just internal metadata. + * + * @param bool $deadEnd Whether the path is a dead end (no valid constraints leading to tenants table) + * @param bool $recursive Whether the path has recursive relationships + * @param array $steps Steps to the tenants table, each step being a formatted constraint */ - public function generateTrees(): array + protected function buildPath(bool $deadEnd = false, bool $recursive = false, array $steps = []): array { - $trees = []; - $builder = $this->database->getSchemaBuilder(); - - // We loop through each table in the database - foreach ($builder->getTableListing(schema: $this->database->getConfig('search_path')) as $table) { - // E.g. "public.table_name" -> "table_name" - $table = str($table)->afterLast('.')->toString(); - - // For each table, we get a list of all foreign key columns - $foreignKeys = collect($builder->getForeignKeys($table))->map(function ($foreign) use ($table) { - return $this->formatForeignKey($foreign, $table); - }); - - // We loop through each foreign key column and find - // all possible paths that lead to the tenants table - foreach ($foreignKeys as $foreign) { - $paths = []; - - $this->generatePaths($table, $foreign, $paths); - - foreach ($paths as &$path) { - foreach ($path as &$step) { - unset($step['comment']); - } - } - - if (count($paths)) { - $trees[$table][$foreign['foreignKey']] = $paths; - } - } - } - - return $trees; - } - - protected function generatePaths(string $table, array $foreign, array &$paths, array $currentPath = []): void - { - // If the foreign key has a comment of 'no-rls', we skip it - // Also skip the foreign key if implicit scoping is off and the foreign key has no comment - if ($foreign['comment'] === 'no-rls' || (! static::$scopeByDefault && $foreign['comment'] === null)) { - return; - } - - if (in_array($foreign['foreignTable'], array_column($currentPath, 'foreignTable'))) { - throw new RecursiveRelationshipException; - } - - $currentPath[] = $foreign; - - if ($foreign['foreignTable'] === tenancy()->model()->getTable()) { - $paths[] = $currentPath; - } else { - // If not, recursively generate paths for the foreign table - foreach ($this->database->getSchemaBuilder()->getForeignKeys($foreign['foreignTable']) as $nextConstraint) { - $this->generatePaths($table, $this->formatForeignKey($nextConstraint, $foreign['foreignTable']), $paths, $currentPath); - } - } - } - - /** Get tree's non-nullable paths. */ - protected function filterNonNullablePaths(array $tree): array - { - $nonNullablePaths = []; - - foreach ($tree as $foreignKey => $paths) { - foreach ($paths as $path) { - $pathIsNullable = false; - - foreach ($path as $step) { - if ($step['nullable']) { - $pathIsNullable = true; - break; - } - } - - if (! $pathIsNullable) { - $nonNullablePaths[$foreignKey][] = $path; - } - } - } - - return $nonNullablePaths; - } - - /** Find the shortest path in a tree and unset the 'nullable' key from the path steps. */ - protected function findShortestPath(array $tree): array - { - $shortestPath = []; - - foreach ($tree as $pathsForForeignKey) { - foreach ($pathsForForeignKey as $path) { - if (empty($shortestPath) || count($shortestPath) > count($path)) { - $shortestPath = $path; - - foreach ($shortestPath as &$step) { - unset($step['nullable']); - } - } - } - } - - return $shortestPath; + return [ + 'dead_end' => $deadEnd, + 'recursive_relationship' => $recursive, + 'steps' => $steps, + ]; } /** - * Formats the foreign key array retrieved by Postgres to a more readable format. + * Formats the retrieved constraint to a more readable format. * - * Also provides information about whether the foreign key is nullable, - * and the foreign key column comment. These additional details are removed - * from the foreign keys/path steps before returning the final shortest paths. + * Also provides internal metadata about + * - the constraint's nullability (the 'nullable' key), + * - the constraint's comment * - * The 'comment' key gets deleted while generating the full trees (in generateTrees()), - * and the 'nullable' key gets deleted while generating the shortest paths (in findShortestPath()). + * These internal details are then omitted + * from the constraints (or the "path steps") + * before returning the shortest paths in shortestPath(). * * [ - * 'foreignKey' => 'tenant_id', + * 'localColumn' => 'tenant_id', * 'foreignTable' => 'tenants', - * 'foreignId' => 'id', - * 'comment' => 'no-rls', // Foreign key comment – used to explicitly enable/disable RLS - * 'nullable' => false, // Whether the foreign key is nullable + * 'foreignColumn' => 'id', + * 'comment' => 'no-rls', // Used to explicitly enable/disable RLS or to create a comment constraint (internal metadata) + * 'nullable' => false, // Used to determine if the constraint is nullable (internal metadata) * ]. */ - protected function formatForeignKey(array $foreignKey, string $table): array + protected function formatForeignKey(array $constraint, string $table): array { - // $foreignKey is one of the foreign keys retrieved by $this->database->getSchemaBuilder()->getForeignKeys($table) + assert(count($constraint['columns']) === 1); + + $localColumn = $constraint['columns'][0]; + + $comment = collect($this->database->getSchemaBuilder()->getColumns($table)) + ->filter(fn ($column) => $column['name'] === $localColumn) + ->first()['comment'] ?? null; + + $columnIsNullable = $this->database->selectOne( + 'SELECT is_nullable FROM information_schema.columns WHERE table_name = ? AND column_name = ?', + [$table, $localColumn] + )->is_nullable === 'YES'; + + assert(count($constraint['foreign_columns']) === 1); + + return $this->formatConstraint( + localColumn: $localColumn, + foreignTable: $constraint['foreign_table'], + foreignColumn: $constraint['foreign_columns'][0], + comment: $comment, + nullable: $columnIsNullable + ); + } + + /** Single source of truth for our constraint format. */ + protected function formatConstraint( + string $localColumn, + string $foreignTable, + string $foreignColumn, + string|null $comment, + bool $nullable + ): array { return [ - 'foreignKey' => $foreignKeyName = $foreignKey['columns'][0], - 'foreignTable' => $foreignKey['foreign_table'], - 'foreignId' => $foreignKey['foreign_columns'][0], - // Deleted in generateTrees() - 'comment' => $this->getComment($table, $foreignKeyName), - // Deleted in shortestPaths() - 'nullable' => $this->database->selectOne("SELECT is_nullable FROM information_schema.columns WHERE table_name = '{$table}' AND column_name = '{$foreignKeyName}'")->is_nullable === 'YES', + 'localColumn' => $localColumn, + 'foreignTable' => $foreignTable, + 'foreignColumn' => $foreignColumn, + // Internal metadata omitted in shortestPaths() + 'comment' => $comment, + 'nullable' => $nullable, ]; } + /** + * Recursively traverse a table's constraints to find + * the shortest path to the tenants table. + * + * The shortest paths are cached in $cachedPaths to avoid + * generating them for already visited tables repeatedly. + * + * @param string $table The table to find a path from + * @param array &$cachedPaths Reference to array where discovered shortest paths are cached (including dead ends) + * @param array $visitedTables Already visited tables (used for detecting recursive relationships) + * @return array Paths with 'steps' (arrays of formatted constraints), 'dead_end' flag (bool), and 'recursive_relationship' flag (bool). + */ + protected function shortestPathToTenantsTable( + string $table, + array &$cachedPaths = [], + array $visitedTables = [] + ): array { + // Return the shortest path for this table if it was already found and cached + if (isset($cachedPaths[$table])) { + return $cachedPaths[$table]; + } + + // Reached tenants table (last step) + if ($table === tenancy()->model()->getTable()) { + // This pretty much just means we set $cachedPaths['tenants'] to an + // empty path. The significance of an empty path is that this class + // considers it to mean "you are at the tenants table". + $cachedPaths[$table] = $this->buildPath(); + + return $cachedPaths[$table]; + } + + $constraints = $this->getConstraints($table); + + if (empty($constraints)) { + // Dead end + $cachedPaths[$table] = $this->buildPath(deadEnd: true); + + return $cachedPaths[$table]; + } + + /** + * Find the optimal path from a table to the tenants table. + * + * Gather table's constraints (both foreign key constraints and comment constraints) + * and recursively find shortest paths through each constraint (non-nullable paths are preferred for reliability). + * + * Handle recursive relationships by skipping paths that would create loops. + * If there's no valid path in the end, and the table has recursive relationships, + * an appropriate exception is thrown. + * + * At the end, it returns the shortest non-nullable path if available, + * fall back to the overall shortest path. + */ + $visitedTables = [...$visitedTables, $table]; + $shortestPath = []; + $hasRecursiveRelationships = false; + $hasValidPaths = false; + + foreach ($constraints as $constraint) { + $foreignTable = $constraint['foreignTable']; + + // Skip constraints that would create loops + if (in_array($foreignTable, $visitedTables)) { + $hasRecursiveRelationships = true; + continue; + } + + // Recursive call + $pathThroughConstraint = $this->shortestPathToTenantsTable( + $foreignTable, + $cachedPaths, + $visitedTables + ); + + if ($pathThroughConstraint['recursive_relationship']) { + $hasRecursiveRelationships = true; + continue; + } + + // Skip dead ends + if ($pathThroughConstraint['dead_end']) { + continue; + } + + $hasValidPaths = true; + $path = $this->buildPath(steps: array_merge([$constraint], $pathThroughConstraint['steps'])); + + if ($this->isPathPreferable($path, $shortestPath)) { + $shortestPath = $path; + } + } + + // Handle tables with only recursive relationships + if ($hasRecursiveRelationships && ! $hasValidPaths) { + // Don't cache paths that cause recursion - return right away. + // This allows tables with recursive relationships to be processed again. + // Example: + // - posts table has highlighted_comment_id that leads to the comments table + // - comments table has recursive_post_id that leads to the posts table (recursive relationship), + // - comments table also has tenant_id which leads to the tenants table (a valid path). + // If the recursive path got cached first, the path leading directly through tenants would never be found. + return $this->buildPath(recursive: true); + } + + $cachedPaths[$table] = $shortestPath ?: $this->buildPath(deadEnd: true); + + return $cachedPaths[$table]; + } + + /** + * Get all valid relationship constraints for a table. The constraints are also formatted. + * Combines both standard foreign key constraints and comment constraints. + * + * The schema builder retrieves foreign keys in the following format: + * [ + * 'name' => 'posts_tenant_id_foreign', + * 'columns' => ['tenant_id'], + * 'foreign_table' => 'tenants', + * 'foreign_columns' => ['id'], + * ... + * ] + * + * We format that into a more readable format using formatForeignKey(), + * and that method uses formatConstraint(), which serves as a single source of truth + * for our constraint formatting. A formatted constraint looks like this: + * [ + * 'localColumn' => 'tenant_id', + * 'foreignTable' => 'tenants', + * 'foreignColumn' => 'id', + * 'comment' => 'no-rls', + * 'nullable' => false + * ] + * + * The comment constraints are retrieved using getFormattedCommentConstraints(). + * These constraints are formatted in the method itself. + */ + protected function getConstraints(string $table): array + { + $formattedConstraints = array_merge( + array_map( + fn ($schemaStructure) => $this->formatForeignKey($schemaStructure, $table), + $this->database->getSchemaBuilder()->getForeignKeys($table) + ), + $this->getFormattedCommentConstraints($table) + ); + + $validConstraints = []; + + foreach ($formattedConstraints as $constraint) { + if (! $this->shouldSkipPathLeadingThroughConstraint($constraint)) { + $validConstraints[] = $constraint; + } + } + + return $validConstraints; + } + + /** + * Determine if a path leading through the passed constraint + * should be excluded from choosing the shortest path + * based on the constraint's comment. + * + * If $scopeByDefault is true, only skip paths leading through constraints flagged with the 'no-rls' comment. + * If $scopeByDefault is false, skip paths leading through any constraint, unless the key has explicit 'rls' or 'rls table.column' comments. + * + * @param array $constraint Formatted constraint + */ + protected function shouldSkipPathLeadingThroughConstraint(array $constraint): bool + { + $comment = $constraint['comment'] ?? null; + + // Always skip constraints with the 'no-rls' comment + if ($comment === 'no-rls') { + return true; + } + + if (static::$scopeByDefault) { + return false; + } + + // When $scopeByDefault is false, skip every constraint + // with a comment that doesn't start with 'rls'. + if (! is_string($comment)) { + return true; + } + + // Explicit scoping + if ($comment === 'rls') { + return false; + } + + // Comment constraint + if (Str::startsWith($comment, 'rls ')) { + return false; + } + + return true; + } + + /** + * Retrieve a table's comment constraints. + * + * Comment constraints are columns with comments + * structured like "rls .". + * + * Returns an array of formatted comment constraints (check formatConstraint() to see the format). + */ + protected function getFormattedCommentConstraints(string $tableName): array + { + $commentConstraints = array_filter($this->database->getSchemaBuilder()->getColumns($tableName), function ($column) { + return (isset($column['comment']) && is_string($column['comment'])) + && Str::startsWith($column['comment'], 'rls '); + }); + + // Validate and format the comment constraints + $commentConstraints = array_map( + fn ($commentConstraint) => $this->parseCommentConstraint($commentConstraint, $tableName), + $commentConstraints + ); + + return $commentConstraints; + } + + /** + * Parse and validate a comment constraint. + * + * This method validates that the table and column referenced + * in the comment exist, formats and returns the constraint. + * + * @throws RLSCommentConstraintException When comment format is invalid or references don't exist + */ + protected function parseCommentConstraint(array $commentConstraint, string $tableName): array + { + $comment = $commentConstraint['comment']; + $columnName = $commentConstraint['name']; + + $builder = $this->database->getSchemaBuilder(); + $constraint = explode('.', Str::after($comment, 'rls ')); + + // Validate comment constraint format + if (count($constraint) !== 2 || empty($constraint[0]) || empty($constraint[1])) { + throw new RLSCommentConstraintException("Malformed comment constraint on {$tableName}.{$columnName}: '{$comment}'"); + } + + $foreignTable = $constraint[0]; + $foreignColumn = $constraint[1]; + + // Validate table existence + if (! $builder->hasTable($foreignTable)) { + throw new RLSCommentConstraintException("Comment constraint on {$tableName}.{$columnName} references non-existent table '{$foreignTable}'"); + } + + // Validate column existence + if (! $builder->hasColumn($foreignTable, $foreignColumn)) { + throw new RLSCommentConstraintException("Comment constraint on {$tableName}.{$columnName} references non-existent column '{$foreignTable}.{$foreignColumn}'"); + } + + // Return the formatted constraint + return $this->formatConstraint( + localColumn: $commentConstraint['name'], + foreignTable: $foreignTable, + foreignColumn: $foreignColumn, + comment: $commentConstraint['comment'], + nullable: $commentConstraint['nullable'] + ); + } + /** Generates a query that creates a row-level security policy for the passed table. */ protected function generateQuery(string $table, array $path): string { @@ -215,9 +513,9 @@ class TableRLSManager implements RLSPolicyManager $sessionTenantKey = config('tenancy.rls.session_variable_name'); foreach ($path as $index => $relation) { - $column = $relation['foreignKey']; + $column = $relation['localColumn']; $table = $relation['foreignTable']; - $foreignKey = $relation['foreignId']; + $foreignKey = $relation['foreignColumn']; $indentation = str_repeat(' ', ($index + 1) * 4); @@ -250,12 +548,65 @@ class TableRLSManager implements RLSPolicyManager return $query; } - protected function getComment(string $tableName, string $columnName): string|null + /** Returns unprefixed table names. */ + protected function getTableNames(): array { - $column = collect($this->database->getSchemaBuilder()->getColumns($tableName)) - ->filter(fn ($column) => $column['name'] === $columnName) - ->first(); + $builder = $this->database->getSchemaBuilder(); + $tables = []; - return $column['comment'] ?? null; + foreach ($builder->getTableListing(schema: $this->database->getConfig('search_path')) as $table) { + // E.g. "public.table_name" -> "table_name" + $tables[] = str($table)->afterLast('.')->toString(); + } + + return $tables; + } + + /** + * Check if discovered path is valid for RLS policy generation. + * + * A valid path: + * - leads to tenants table (isn't dead end) + * - has at least one step (the tenants table itself will have no steps) + */ + protected function isValidPath(array $path): bool + { + return ! $path['dead_end'] && ! empty($path['steps']); + } + + /** + * Determine if the passed path is preferred to the current shortest path. + * + * Non-nullable paths are preferred to nullable paths. + * From paths of the same nullability, the shorter will be preferred. + */ + protected function isPathPreferable(array $path, array $shortestPath): bool + { + if (! $shortestPath) { + return true; + } + + $pathIsNullable = $this->isPathNullable($path['steps']); + $shortestPathIsNullable = $this->isPathNullable($shortestPath['steps']); + + // Prefer non-nullable + if ($pathIsNullable !== $shortestPathIsNullable) { + return ! $pathIsNullable; + } + + // Prefer shorter + return count($path['steps']) < count($shortestPath['steps']); + } + + /** Determine if any step in the path is nullable. */ + protected function isPathNullable(array $path): bool + { + foreach ($path as $step) { + if ($step['nullable']) { + return true; + } + } + + return false; } } diff --git a/tests/RLS/TableManagerTest.php b/tests/RLS/TableManagerTest.php index c0520a1d..c5c8d254 100644 --- a/tests/RLS/TableManagerTest.php +++ b/tests/RLS/TableManagerTest.php @@ -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 { diff --git a/tests/TestCase.php b/tests/TestCase.php index 6a167c46..47af9e7d 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -36,6 +36,8 @@ abstract class TestCase extends \Orchestra\Testbench\TestCase { parent::setUp(); + ini_set('memory_limit', '1G'); + Redis::connection('default')->flushdb(); Redis::connection('cache')->flushdb(); Artisan::call('cache:clear memcached'); // flush memcached