From 3b3f5930801f40b8e09fffe57edf18a9a8133839 Mon Sep 17 00:00:00 2001 From: Constantin Graf Date: Tue, 5 Nov 2024 11:35:55 +0100 Subject: [PATCH] Fix foreign keys and deletion service --- app/Models/User.php | 18 +++ app/Service/DeletionService.php | 6 +- app/Service/MemberService.php | 6 +- ...key_to_organizations_and_members_table.php | 93 ++++++++++++++ ...70614_add_foreign_keys_to_oauth_tables.php | 114 ++++++++++++++++++ database/seeders/DatabaseSeeder.php | 34 +++++- tests/Unit/Model/UserModelTest.php | 65 ++++++++++ tests/Unit/Service/DeletionServiceTest.php | 13 +- 8 files changed, 344 insertions(+), 5 deletions(-) create mode 100644 database/migrations/2024_11_04_164807_add_foreign_key_to_organizations_and_members_table.php create mode 100644 database/migrations/2024_11_04_170614_add_foreign_keys_to_oauth_tables.php diff --git a/app/Models/User.php b/app/Models/User.php index 5b4a1c93..b846acee 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -25,7 +25,9 @@ use Illuminate\Support\Facades\Storage; use Laravel\Fortify\TwoFactorAuthenticatable; use Laravel\Jetstream\HasProfilePhoto; use Laravel\Jetstream\HasTeams; +use Laravel\Passport\AuthCode; use Laravel\Passport\HasApiTokens; +use Laravel\Passport\Token; use OwenIt\Auditing\Contracts\Auditable as AuditableContract; /** @@ -178,6 +180,22 @@ class User extends Authenticatable implements AuditableContract, FilamentUser, M return $this->hasMany(ProjectMember::class, 'user_id'); } + /** + * @return HasMany + */ + public function accessTokens(): HasMany + { + return $this->hasMany(Token::class); + } + + /** + * @return HasMany + */ + public function authCodes(): HasMany + { + return $this->hasMany(AuthCode::class); + } + /** * @param Builder $builder */ diff --git a/app/Service/DeletionService.php b/app/Service/DeletionService.php index 3e066b28..7fc6fd08 100644 --- a/app/Service/DeletionService.php +++ b/app/Service/DeletionService.php @@ -144,6 +144,7 @@ class DeletionService ->get(); foreach ($members as $member) { + /** @var Member $member */ if ($member->role === Role::Owner->value && $member->organization->users()->count() > 1) { throw new CanNotDeleteUserWhoIsOwnerOfOrganizationWithMultipleMembers; } @@ -154,10 +155,13 @@ class DeletionService if ($member->role === Role::Owner->value) { $this->deleteOrganization($member->organization, false, $user); } else { - $this->memberService->makeMemberToPlaceholder($member); + $this->memberService->makeMemberToPlaceholder($member, false); } } + $user->accessTokens()->delete(); + $user->authCodes()->delete(); + // Note: Since the deletion of the profile photo is not reversible via a database rollback this needs to be done last $user->deleteProfilePhoto(); diff --git a/app/Service/MemberService.php b/app/Service/MemberService.php index ab9aa01c..01f5f4ec 100644 --- a/app/Service/MemberService.php +++ b/app/Service/MemberService.php @@ -44,7 +44,7 @@ class MemberService } } - public function makeMemberToPlaceholder(Member $member): void + public function makeMemberToPlaceholder(Member $member, bool $makeSureUserHasAtLeastOneOrganization = true): void { $user = $member->user; $placeholderUser = $user->replicate(); @@ -56,6 +56,8 @@ class MemberService $member->save(); $this->userService->assignOrganizationEntitiesToDifferentMember($member->organization, $user, $placeholderUser, $member); - $this->userService->makeSureUserHasAtLeastOneOrganization($user); + if ($makeSureUserHasAtLeastOneOrganization) { + $this->userService->makeSureUserHasAtLeastOneOrganization($user); + } } } diff --git a/database/migrations/2024_11_04_164807_add_foreign_key_to_organizations_and_members_table.php b/database/migrations/2024_11_04_164807_add_foreign_key_to_organizations_and_members_table.php new file mode 100644 index 00000000..1bdeff06 --- /dev/null +++ b/database/migrations/2024_11_04_164807_add_foreign_key_to_organizations_and_members_table.php @@ -0,0 +1,93 @@ +select(['organizations.id', 'organizations.user_id']) + ->whereNotExists(function (Builder $query): void { + $query->select('id') + ->from('users') + ->whereColumn('organizations.user_id', 'users.id'); + }) + ->get(); + foreach ($foreignKeyProblems as $foreignKeyProblem) { + Log::error('Organization with ID '.$foreignKeyProblem->id.' has non-existing owner with ID '.$foreignKeyProblem->user_id); + } + if ($foreignKeyProblems->count() > 0) { + throw new Exception('There are organizations with non-existing owners, check the logs for more information'); + } + Schema::table('organizations', function (Blueprint $table): void { + $table->foreign('user_id') + ->references('id') + ->on('users') + ->onDelete('restrict') + ->onUpdate('cascade'); + }); + $foreignKeyProblems = DB::table('members') + ->select(['members.id', 'members.organization_id']) + ->whereNotExists(function (Builder $query): void { + $query->select('id') + ->from('organizations') + ->whereColumn('members.organization_id', 'organizations.id'); + }) + ->get(); + foreach ($foreignKeyProblems as $foreignKeyProblem) { + Log::error('Member with ID '.$foreignKeyProblem->id.' has non-existing organization with ID '.$foreignKeyProblem->organization_id); + } + if ($foreignKeyProblems->count() > 0) { + throw new Exception('There are members with non-existing organizations, check the logs for more information'); + } + $foreignKeyProblems = DB::table('members') + ->select(['members.id', 'members.user_id']) + ->whereNotExists(function (Builder $query): void { + $query->select('id') + ->from('users') + ->whereColumn('members.user_id', 'users.id'); + }) + ->get(); + foreach ($foreignKeyProblems as $foreignKeyProblem) { + Log::error('Member with ID '.$foreignKeyProblem->id.' has non-existing user with ID '.$foreignKeyProblem->user_id); + } + if ($foreignKeyProblems->count() > 0) { + throw new Exception('There are members with non-existing users, check the logs for more information'); + } + Schema::table('members', function (Blueprint $table): void { + $table->foreign('organization_id') + ->references('id') + ->on('organizations') + ->onDelete('restrict') + ->onUpdate('cascade'); + $table->foreign('user_id') + ->references('id') + ->on('users') + ->onDelete('restrict') + ->onUpdate('cascade'); + }); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + Schema::table('organizations', function (Blueprint $table): void { + $table->dropForeign(['user_id']); + }); + Schema::table('members', function (Blueprint $table): void { + $table->dropForeign(['organization_id']); + $table->dropForeign(['user_id']); + }); + } +}; diff --git a/database/migrations/2024_11_04_170614_add_foreign_keys_to_oauth_tables.php b/database/migrations/2024_11_04_170614_add_foreign_keys_to_oauth_tables.php new file mode 100644 index 00000000..9ce34ebf --- /dev/null +++ b/database/migrations/2024_11_04_170614_add_foreign_keys_to_oauth_tables.php @@ -0,0 +1,114 @@ +whereNotNull('user_id') + ->whereNotExists(function (Builder $query): void { + $query->select('id') + ->from('users') + ->whereColumn('oauth_access_tokens.user_id', 'users.id'); + }) + ->delete(); + DB::table('oauth_access_tokens') + ->whereNotExists(function (Builder $query): void { + $query->select('id') + ->from('oauth_clients') + ->whereColumn('oauth_access_tokens.client_id', 'oauth_clients.id'); + }) + ->delete(); + Schema::table('oauth_access_tokens', function (Blueprint $table): void { + $table->foreign('user_id') + ->references('id') + ->on('users') + ->onDelete('restrict') + ->onUpdate('cascade'); + $table->foreign('client_id') + ->references('id') + ->on('oauth_clients') + ->onDelete('restrict') + ->onUpdate('cascade'); + }); + DB::table('oauth_auth_codes') + ->whereNotExists(function (Builder $query): void { + $query->select('id') + ->from('users') + ->whereColumn('oauth_auth_codes.user_id', 'users.id'); + }) + ->delete(); + DB::table('oauth_auth_codes') + ->whereNotExists(function (Builder $query): void { + $query->select('id') + ->from('oauth_clients') + ->whereColumn('oauth_auth_codes.client_id', 'oauth_clients.id'); + }) + ->delete(); + Schema::table('oauth_auth_codes', function (Blueprint $table): void { + $table->foreign('user_id') + ->references('id') + ->on('users') + ->onDelete('restrict') + ->onUpdate('cascade'); + $table->foreign('client_id') + ->references('id') + ->on('oauth_clients') + ->onDelete('restrict') + ->onUpdate('cascade'); + }); + DB::table('oauth_clients') + ->whereNotNull('user_id') + ->whereNotExists(function (Builder $query): void { + $query->select('id') + ->from('users') + ->whereColumn('oauth_clients.user_id', 'users.id'); + }) + ->delete(); + Schema::table('oauth_clients', function (Blueprint $table): void { + $table->foreign('user_id') + ->references('id') + ->on('users') + ->onDelete('restrict') + ->onUpdate('cascade'); + }); + Schema::table('oauth_personal_access_clients', function (Blueprint $table): void { + $table->foreign('client_id') + ->references('id') + ->on('oauth_clients') + ->onDelete('restrict') + ->onUpdate('cascade'); + }); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + Schema::table('oauth_access_tokens', function (Blueprint $table): void { + $table->dropForeign(['user_id']); + $table->dropForeign(['client_id']); + }); + Schema::table('oauth_auth_codes', function (Blueprint $table): void { + $table->dropForeign(['user_id']); + $table->dropForeign(['client_id']); + }); + Schema::table('oauth_clients', function (Blueprint $table): void { + $table->dropForeign(['user_id']); + }); + Schema::table('oauth_personal_access_clients', function (Blueprint $table): void { + $table->dropForeign(['client_id']); + }); + } +}; diff --git a/database/seeders/DatabaseSeeder.php b/database/seeders/DatabaseSeeder.php index 832503fc..c286c6fb 100644 --- a/database/seeders/DatabaseSeeder.php +++ b/database/seeders/DatabaseSeeder.php @@ -18,6 +18,12 @@ use App\Models\TimeEntry; use App\Models\User; use Illuminate\Database\Seeder; use Illuminate\Support\Facades\DB; +use Laravel\Passport\AuthCode; +use Laravel\Passport\Client as PassportClient; +use Laravel\Passport\ClientRepository; +use Laravel\Passport\PersonalAccessClient; +use Laravel\Passport\RefreshToken; +use Laravel\Passport\Token; class DatabaseSeeder extends Seeder { @@ -150,10 +156,35 @@ class DatabaseSeeder extends Seeder User::factory()->withPersonalOrganization()->create([ 'email' => 'admin@example.com', ]); + + app(ClientRepository::class)->create( + null, + 'desktop', + 'solidtime://oauth/callback', + null, + false, + false, + false + ); } private function deleteAll(): void { + // Laravel Passport tables + DB::table((new RefreshToken)->getTable())->delete(); + DB::table((new Token)->getTable())->delete(); + DB::table((new AuthCode)->getTable())->delete(); + DB::table((new PersonalAccessClient)->getTable())->delete(); + DB::table((new PassportClient)->getTable())->delete(); + + // Internal tables + DB::table('cache')->delete(); + DB::table('cache_locks')->delete(); + DB::table('jobs')->delete(); + DB::table('failed_jobs')->delete(); + DB::table('sessions')->delete(); + + // Application tables DB::table((new Audit)->getTable())->delete(); DB::table((new TimeEntry)->getTable())->delete(); DB::table((new Task)->getTable())->delete(); @@ -161,8 +192,9 @@ class DatabaseSeeder extends Seeder DB::table((new ProjectMember)->getTable())->delete(); DB::table((new Project)->getTable())->delete(); DB::table((new Client)->getTable())->delete(); - DB::table((new User)->getTable())->delete(); + DB::table((new Member)->getTable())->delete(); DB::table((new OrganizationInvitation)->getTable())->delete(); DB::table((new Organization)->getTable())->delete(); + DB::table((new User)->getTable())->delete(); } } diff --git a/tests/Unit/Model/UserModelTest.php b/tests/Unit/Model/UserModelTest.php index 56f2840b..fa8f88d4 100644 --- a/tests/Unit/Model/UserModelTest.php +++ b/tests/Unit/Model/UserModelTest.php @@ -13,6 +13,9 @@ use App\Models\User; use App\Providers\Filament\AdminPanelProvider; use Filament\Panel; use Illuminate\Support\Facades\Config; +use Laravel\Passport\AuthCode; +use Laravel\Passport\Client; +use Laravel\Passport\Token; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\UsesClass; @@ -138,4 +141,66 @@ class UserModelTest extends ModelTestAbstract $this->assertCount(1, $activeUsers); $this->assertTrue($activeUsers->first()->is($user)); } + + public function test_it_has_many_access_tokens(): void + { + // Arrange + $user = User::factory()->create(); + $client = new Client; + $client->name = 'desktop'; + $client->redirect = 'solidtime://oauth/callback'; + $client->personal_access_client = false; + $client->password_client = false; + $client->revoked = false; + $client->save(); + $token = new Token; + $token->id = 'some-id'; + $token->user_id = $user->getKey(); + $token->client_id = $client->getKey(); + $token->revoked = false; + $token->save(); + + // Act + $user->refresh(); + $tokensRel = $user->accessTokens; + + // Assert + $this->assertNotNull($tokensRel); + $this->assertCount(1, $tokensRel); + $this->assertEqualsCanonicalizing( + [$token->getKey()], + $tokensRel->pluck('id')->toArray() + ); + } + + public function test_it_has_many_auth_codes(): void + { + // Arrange + $user = User::factory()->create(); + $client = new Client; + $client->name = 'desktop'; + $client->redirect = 'solidtime://oauth/callback'; + $client->personal_access_client = false; + $client->password_client = false; + $client->revoked = false; + $client->save(); + $authCode = new AuthCode; + $authCode->id = 'some-id'; + $authCode->user_id = $user->getKey(); + $authCode->client_id = $client->getKey(); + $authCode->revoked = false; + $authCode->save(); + + // Act + $user->refresh(); + $authCodesRel = $user->authCodes; + + // Assert + $this->assertNotNull($authCodesRel); + $this->assertCount(1, $authCodesRel); + $this->assertEqualsCanonicalizing( + [$authCode->getKey()], + $authCodesRel->pluck('id')->toArray() + ); + } } diff --git a/tests/Unit/Service/DeletionServiceTest.php b/tests/Unit/Service/DeletionServiceTest.php index b4b4af55..929636e1 100644 --- a/tests/Unit/Service/DeletionServiceTest.php +++ b/tests/Unit/Service/DeletionServiceTest.php @@ -307,20 +307,31 @@ class DeletionServiceTest extends TestCaseWithDatabase { // Arrange $user = User::factory()->create(); + $otherUser = User::factory()->create(); $organizationOwned = Organization::factory()->withOwner($user)->create(); - $organizationNotOwned = Organization::factory()->create(); + $organizationNotOwned = Organization::factory()->withOwner($otherUser)->create(); $memberOwned = Member::factory()->forUser($user)->forOrganization($organizationOwned)->role(Role::Owner)->create(); $memberNotOwned = Member::factory()->forUser($user)->forOrganization($organizationNotOwned)->role(Role::Employee)->create(); TimeEntry::factory()->forOrganization($organizationOwned)->forMember($memberOwned)->createMany(2); TimeEntry::factory()->forOrganization($organizationNotOwned)->forMember($memberNotOwned)->createMany(2); + $this->assertDatabaseCount(User::class, 2); // Act $this->deletionService->deleteUser($user); // Assert + $this->assertDatabaseCount(Organization::class, 1); + $this->assertDatabaseCount(User::class, 2); $this->assertDatabaseMissing(User::class, [ 'id' => $user->getKey(), ]); + $this->assertDatabaseHas(User::class, [ + 'id' => $otherUser->getKey(), + 'is_placeholder' => false, + ]); + $this->assertDatabaseHas(User::class, [ + 'is_placeholder' => true, + ]); $this->assertDatabaseMissing(Organization::class, [ 'id' => $organizationOwned->getKey(), ]);