Skip to content

Fix: Make team_foreign_key nullable in pivot tables (#2888) #2894

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix: Make team_foreign_key nullable in pivot tables (#2888) #2894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions database/migrations/add_teams_fields.php.stub
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,16 +40,18 @@ 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') {
$table->dropForeign([$pivotPermission]);
}
$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');
Expand All @@ -59,16 +61,18 @@ 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') {
$table->dropForeign([$pivotRole]);
}
$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');
Expand Down
18 changes: 12 additions & 6 deletions database/migrations/create_permission_tables.php.stub
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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');
Expand All @@ -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');
Expand All @@ -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');
Expand Down
36 changes: 36 additions & 0 deletions tests/TeamHasPermissionsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
181 changes: 181 additions & 0 deletions tests/TeamHasRolesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
}
}