From 32197ec0f187ca2412fae2bfe25318df5327da98 Mon Sep 17 00:00:00 2001 From: Hayatunnabi Nabil Date: Mon, 10 Nov 2025 15:35:03 +0600 Subject: [PATCH] fix: Update migrations to allow nullable team_foreign_key in model_has_permissions and model_has_roles tables and relevent tests to proof --- database/migrations/add_teams_fields.php.stub | 12 +- .../create_permission_tables.php.stub | 18 +- tests/TeamHasPermissionsTest.php | 36 ++++ tests/TeamHasRolesTest.php | 181 ++++++++++++++++++ 4 files changed, 237 insertions(+), 10 deletions(-) diff --git a/database/migrations/add_teams_fields.php.stub b/database/migrations/add_teams_fields.php.stub index 01fcca697..1877c7601 100644 --- a/database/migrations/add_teams_fields.php.stub +++ b/database/migrations/add_teams_fields.php.stub @@ -40,7 +40,7 @@ return new class extends Migration if (! Schema::hasColumn($tableNames['model_has_permissions'], $columnNames['team_foreign_key'])) { Schema::table($tableNames['model_has_permissions'], function (Blueprint $table) use ($tableNames, $columnNames, $pivotPermission) { - $table->unsignedBigInteger($columnNames['team_foreign_key'])->default('1'); + $table->unsignedBigInteger($columnNames['team_foreign_key'])->nullable(); $table->index($columnNames['team_foreign_key'], 'model_has_permissions_team_foreign_key_index'); if (DB::getDriverName() !== 'sqlite') { @@ -48,8 +48,10 @@ return new class extends Migration } $table->dropPrimary(); - $table->primary([$columnNames['team_foreign_key'], $pivotPermission, $columnNames['model_morph_key'], 'model_type'], + $table->primary([$pivotPermission, $columnNames['model_morph_key'], 'model_type'], 'model_has_permissions_permission_model_type_primary'); + $table->unique([$columnNames['team_foreign_key'], $pivotPermission, $columnNames['model_morph_key'], 'model_type'], + 'model_has_permissions_team_foreign_key_permission_model_type_unique'); if (DB::getDriverName() !== 'sqlite') { $table->foreign($pivotPermission) ->references('id')->on($tableNames['permissions'])->onDelete('cascade'); @@ -59,7 +61,7 @@ return new class extends Migration if (! Schema::hasColumn($tableNames['model_has_roles'], $columnNames['team_foreign_key'])) { Schema::table($tableNames['model_has_roles'], function (Blueprint $table) use ($tableNames, $columnNames, $pivotRole) { - $table->unsignedBigInteger($columnNames['team_foreign_key'])->default('1'); + $table->unsignedBigInteger($columnNames['team_foreign_key'])->nullable(); $table->index($columnNames['team_foreign_key'], 'model_has_roles_team_foreign_key_index'); if (DB::getDriverName() !== 'sqlite') { @@ -67,8 +69,10 @@ return new class extends Migration } $table->dropPrimary(); - $table->primary([$columnNames['team_foreign_key'], $pivotRole, $columnNames['model_morph_key'], 'model_type'], + $table->primary([$pivotRole, $columnNames['model_morph_key'], 'model_type'], 'model_has_roles_role_model_type_primary'); + $table->unique([$columnNames['team_foreign_key'], $pivotRole, $columnNames['model_morph_key'], 'model_type'], + 'model_has_roles_team_foreign_key_role_model_type_unique'); if (DB::getDriverName() !== 'sqlite') { $table->foreign($pivotRole) ->references('id')->on($tableNames['roles'])->onDelete('cascade'); diff --git a/database/migrations/create_permission_tables.php.stub b/database/migrations/create_permission_tables.php.stub index 66ce1f970..8e43f748e 100644 --- a/database/migrations/create_permission_tables.php.stub +++ b/database/migrations/create_permission_tables.php.stub @@ -48,6 +48,9 @@ return new class extends Migration }); Schema::create($tableNames['model_has_permissions'], static function (Blueprint $table) use ($tableNames, $columnNames, $pivotPermission, $teams) { + if ($teams) { + $table->id(); + } $table->unsignedBigInteger($pivotPermission); $table->string('model_type'); @@ -59,11 +62,11 @@ return new class extends Migration ->on($tableNames['permissions']) ->onDelete('cascade'); if ($teams) { - $table->unsignedBigInteger($columnNames['team_foreign_key']); + $table->unsignedBigInteger($columnNames['team_foreign_key'])->nullable(); $table->index($columnNames['team_foreign_key'], 'model_has_permissions_team_foreign_key_index'); - $table->primary([$columnNames['team_foreign_key'], $pivotPermission, $columnNames['model_morph_key'], 'model_type'], - 'model_has_permissions_permission_model_type_primary'); + $table->unique([$columnNames['team_foreign_key'], $pivotPermission, $columnNames['model_morph_key'], 'model_type'], + 'model_has_permissions_team_foreign_key_permission_model_type_unique'); } else { $table->primary([$pivotPermission, $columnNames['model_morph_key'], 'model_type'], 'model_has_permissions_permission_model_type_primary'); @@ -72,6 +75,9 @@ return new class extends Migration }); Schema::create($tableNames['model_has_roles'], static function (Blueprint $table) use ($tableNames, $columnNames, $pivotRole, $teams) { + if ($teams) { + $table->id(); + } $table->unsignedBigInteger($pivotRole); $table->string('model_type'); @@ -83,11 +89,11 @@ return new class extends Migration ->on($tableNames['roles']) ->onDelete('cascade'); if ($teams) { - $table->unsignedBigInteger($columnNames['team_foreign_key']); + $table->unsignedBigInteger($columnNames['team_foreign_key'])->nullable(); $table->index($columnNames['team_foreign_key'], 'model_has_roles_team_foreign_key_index'); - $table->primary([$columnNames['team_foreign_key'], $pivotRole, $columnNames['model_morph_key'], 'model_type'], - 'model_has_roles_role_model_type_primary'); + $table->unique([$columnNames['team_foreign_key'], $pivotRole, $columnNames['model_morph_key'], 'model_type'], + 'model_has_roles_team_foreign_key_role_model_type_unique'); } else { $table->primary([$pivotRole, $columnNames['model_morph_key'], 'model_type'], 'model_has_roles_role_model_type_primary'); diff --git a/tests/TeamHasPermissionsTest.php b/tests/TeamHasPermissionsTest.php index 1e3ab20af..780d52e70 100644 --- a/tests/TeamHasPermissionsTest.php +++ b/tests/TeamHasPermissionsTest.php @@ -131,4 +131,40 @@ public function it_can_scope_users_on_different_teams() $this->assertEquals(1, $scopedUsers1Team1->count()); $this->assertEquals(0, $scopedUsers2Team1->count()); } + + /** @test */ + #[Test] + public function it_allows_direct_database_insertion_with_null_team_foreign_key_for_permissions() + { + // Test that the database schema actually allows NULL in model_has_permissions pivot table + // This is a direct test of the migration fix for issue #2888 + // This would fail if team_foreign_key was NOT NULL + + $teamKey = config('permission.column_names.team_foreign_key'); + $pivotKey = config('permission.column_names.permission_pivot_key') ?? 'permission_id'; + $modelKey = config('permission.column_names.model_morph_key'); + + $permission = \Spatie\Permission\Models\Permission::create(['name' => 'direct-test-permission']); + + // Directly insert into pivot table with NULL team_foreign_key + // This tests that the column is actually nullable in the database + \DB::table('model_has_permissions')->insert([ + $pivotKey => $permission->id, + $modelKey => $this->testUser->id, + 'model_type' => get_class($this->testUser), + $teamKey => null, // This should not throw an error + ]); + + // Verify the insertion worked - the database schema allows NULL + $this->assertDatabaseHas('model_has_permissions', [ + $pivotKey => $permission->id, + $modelKey => $this->testUser->id, + $teamKey => null, + ]); + + // Note: The query logic checks pivot team_foreign_key against getPermissionsTeamId() + // So NULL in pivot won't match unless team ID is also NULL + // This test verifies the schema allows NULL, which is the core fix for issue #2888 + // The actual assignment logic (givePermissionTo) will set the team ID in the pivot table + } } diff --git a/tests/TeamHasRolesTest.php b/tests/TeamHasRolesTest.php index 113bdc5aa..3a31d7be0 100644 --- a/tests/TeamHasRolesTest.php +++ b/tests/TeamHasRolesTest.php @@ -159,4 +159,185 @@ public function it_can_scope_users_on_different_teams() $this->assertEquals(0, $scopedUsers2Team2->count()); $this->assertEquals(1, $scopedUsers3Team2->count()); } + + /** @test */ + #[Test] + public function it_can_assign_global_role_with_null_team_foreign_key_to_user() + { + // This test verifies the fix for issue #2888 + // Global roles (with team_foreign_key = null) should be assignable to users + // without throwing an error about non-nullable team_foreign_key in pivot tables + + $teamKey = config('permission.column_names.team_foreign_key'); + + // Create a global role with null team_foreign_key + $globalRole = app(Role::class)->create(['name' => 'global-admin', $teamKey => null]); + + $this->assertNull($globalRole->{$teamKey}, 'Global role should have null team_foreign_key'); + + // Assign the global role to a user - this should not throw an error + setPermissionsTeamId(1); + $this->testUser->assignRole($globalRole); + + // Verify the role was assigned + $this->assertTrue($this->testUser->hasRole($globalRole)); + $this->assertTrue($this->testUser->hasRole('global-admin')); + + // Verify the pivot table entry exists and can have null team_foreign_key + $this->assertDatabaseHas('model_has_roles', [ + config('permission.column_names.model_morph_key') => $this->testUser->id, + ]); + + // Verify we can query the role assignment + $this->testUser->load('roles'); + $assignedRoles = $this->testUser->roles; + $this->assertTrue($assignedRoles->contains('id', $globalRole->id)); + } + + /** @test */ + #[Test] + public function it_can_assign_global_role_to_multiple_users_across_different_teams() + { + // Test that unique constraint works correctly with NULL values + // Multiple users should be able to have the same global role + // This test would fail if team_foreign_key was part of primary key (can't have NULL in PK) + + $teamKey = config('permission.column_names.team_foreign_key'); + $globalRole = app(Role::class)->create(['name' => 'global-editor', $teamKey => null]); + + $user1 = User::create(['email' => 'user1-global@test.com']); + $user2 = User::create(['email' => 'user2-global@test.com']); + + // Assign to user1 on team 1 + setPermissionsTeamId(1); + $user1->assignRole($globalRole); + + // Assign to user2 on team 2 + setPermissionsTeamId(2); + $user2->assignRole($globalRole); + + // Both should have the role + setPermissionsTeamId(1); + $user1->load('roles'); + $this->assertTrue($user1->hasRole($globalRole)); + + setPermissionsTeamId(2); + $user2->load('roles'); + $this->assertTrue($user2->hasRole($globalRole)); + + // Verify both entries exist in pivot table (with potentially NULL team_foreign_key) + $this->assertDatabaseHas('model_has_roles', [ + config('permission.column_names.model_morph_key') => $user1->id, + ]); + $this->assertDatabaseHas('model_has_roles', [ + config('permission.column_names.model_morph_key') => $user2->id, + ]); + } + + /** @test */ + #[Test] + public function it_can_query_global_roles_correctly_across_different_teams() + { + // Test that global roles (with NULL team_foreign_key in roles table) work correctly + // The role itself has NULL team_foreign_key, making it a "global" role + // However, when assigned, the pivot table gets the current team ID + // This test verifies the schema allows NULL in roles table and pivot table + + $teamKey = config('permission.column_names.team_foreign_key'); + $globalRole = app(Role::class)->create(['name' => 'global-viewer', $teamKey => null]); + $teamRole = app(Role::class)->create(['name' => 'team-specific', $teamKey => 1]); + + setPermissionsTeamId(1); + $this->testUser->assignRole($globalRole, $teamRole); + + // Should see both roles on team 1 + $this->testUser->load('roles'); + $this->assertTrue($this->testUser->hasRole($globalRole)); + $this->assertTrue($this->testUser->hasRole($teamRole)); + + // Switch to team 2 - assign global role again (it can be assigned on multiple teams) + // The global role (NULL in roles table) can exist on multiple teams via pivot + setPermissionsTeamId(2); + $this->testUser->assignRole($globalRole); + $this->testUser->load('roles'); + $this->assertTrue($this->testUser->hasRole($globalRole), 'Global role should be assignable on multiple teams'); + $this->assertFalse($this->testUser->hasRole($teamRole), 'Team-specific role should not be visible on other teams'); + + // Verify the global role exists in roles table with NULL team_foreign_key + $this->assertNull($globalRole->fresh()->{$teamKey}, 'Global role should have NULL team_foreign_key in roles table'); + } + + /** @test */ + #[Test] + public function it_allows_direct_database_insertion_with_null_team_foreign_key() + { + // Test that the database schema actually allows NULL in the pivot table + // This is a direct test of the migration fix + // This would fail if team_foreign_key was NOT NULL + + $teamKey = config('permission.column_names.team_foreign_key'); + $pivotKey = config('permission.column_names.role_pivot_key') ?? 'role_id'; + $modelKey = config('permission.column_names.model_morph_key'); + + $globalRole = app(Role::class)->create(['name' => 'direct-test-role', $teamKey => null]); + + // Directly insert into pivot table with NULL team_foreign_key + // This tests that the column is actually nullable in the database + \DB::table('model_has_roles')->insert([ + $pivotKey => $globalRole->id, + $modelKey => $this->testUser->id, + 'model_type' => get_class($this->testUser), + $teamKey => null, // This should not throw an error + ]); + + // Verify the insertion worked - the database schema allows NULL + $this->assertDatabaseHas('model_has_roles', [ + $pivotKey => $globalRole->id, + $modelKey => $this->testUser->id, + $teamKey => null, + ]); + + // Note: The query logic checks both pivot team_foreign_key AND role's team_foreign_key + // Since the role has NULL team_foreign_key, it should be accessible from any team + // But the pivot's team_foreign_key needs to match for the query to work + // This test verifies the schema allows NULL, which is the core fix + // The actual assignment logic will set the team ID in the pivot table + } + + /** @test */ + #[Test] + public function it_handles_mixed_team_and_global_roles_correctly() + { + // Test edge case: user has both team-specific and global roles + // Ensures queries handle both NULL and non-NULL values correctly + + $teamKey = config('permission.column_names.team_foreign_key'); + $globalRole = app(Role::class)->create(['name' => 'mixed-global', $teamKey => null]); + $team1Role = app(Role::class)->create(['name' => 'mixed-team1', $teamKey => 1]); + $team2Role = app(Role::class)->create(['name' => 'mixed-team2', $teamKey => 2]); + + // Assign global and team1 role on team 1 + setPermissionsTeamId(1); + $this->testUser->assignRole($globalRole, $team1Role); + + // Assign global and team2 role on team 2 + setPermissionsTeamId(2); + $this->testUser->assignRole($globalRole, $team2Role); + + // On team 1: should see global + team1 roles + setPermissionsTeamId(1); + $this->testUser->load('roles'); + $roleNames = $this->testUser->getRoleNames()->sort()->values(); + $this->assertTrue($roleNames->contains('mixed-global')); + $this->assertTrue($roleNames->contains('mixed-team1')); + $this->assertFalse($roleNames->contains('mixed-team2')); + + // On team 2: should see global + team2 roles + setPermissionsTeamId(2); + $this->testUser->load('roles'); + $roleNames = $this->testUser->getRoleNames()->sort()->values(); + $this->assertTrue($roleNames->contains('mixed-global')); + $this->assertTrue($roleNames->contains('mixed-team2')); + $this->assertFalse($roleNames->contains('mixed-team1')); + } }