From f3ec6602438805f6908a19db46e5c32c3d4ddb07 Mon Sep 17 00:00:00 2001 From: Constantin Graf Date: Wed, 17 Jun 2026 15:51:25 +0200 Subject: [PATCH] Add password check to users.destroy and organizations.destroy --- .../Api/V1/OrganizationController.php | 3 +- .../Controllers/Api/V1/UserController.php | 3 +- .../OrganizationDestroyRequest.php | 48 ++++++++++++++++ .../Requests/V1/User/UserDestroyRequest.php | 48 ++++++++++++++++ docker-compose.yml | 2 +- e2e/organization.spec.ts | 25 ++++++++- e2e/profile.spec.ts | 4 +- .../Pages/Profile/Partials/DeleteUserForm.vue | 35 ++++++------ .../Pages/Teams/Partials/DeleteTeamForm.vue | 56 +++++++++++++++---- resources/js/packages/api/src/index.ts | 2 + .../packages/api/src/openapi.json.client.ts | 11 ++++ resources/js/utils/useOrganization.ts | 35 ++++++++---- resources/js/utils/useUserQuery.ts | 6 +- .../Api/V1/OrganizationEndpointTest.php | 52 ++++++++++++++++- .../Unit/Endpoint/Api/V1/UserEndpointTest.php | 44 ++++++++++++++- .../Resources/OrganizationResourceTest.php | 35 ++++++++++++ 16 files changed, 352 insertions(+), 57 deletions(-) create mode 100644 app/Http/Requests/V1/Organization/OrganizationDestroyRequest.php create mode 100644 app/Http/Requests/V1/User/UserDestroyRequest.php diff --git a/app/Http/Controllers/Api/V1/OrganizationController.php b/app/Http/Controllers/Api/V1/OrganizationController.php index 2d8e327d..7fcddf95 100644 --- a/app/Http/Controllers/Api/V1/OrganizationController.php +++ b/app/Http/Controllers/Api/V1/OrganizationController.php @@ -6,6 +6,7 @@ namespace App\Http\Controllers\Api\V1; use App\Enums\Role; use App\Events\AfterCreateOrganization; +use App\Http\Requests\V1\Organization\OrganizationDestroyRequest; use App\Http\Requests\V1\Organization\OrganizationStoreRequest; use App\Http\Requests\V1\Organization\OrganizationUpdateRequest; use App\Http\Resources\V1\Organization\OrganizationResource; @@ -124,7 +125,7 @@ class OrganizationController extends Controller * * @throws AuthorizationException */ - public function destroy(Organization $organization, DeletionService $deletionService): JsonResponse + public function destroy(Organization $organization, OrganizationDestroyRequest $request, DeletionService $deletionService): JsonResponse { $this->checkPermission($organization, 'organizations:delete'); diff --git a/app/Http/Controllers/Api/V1/UserController.php b/app/Http/Controllers/Api/V1/UserController.php index b8d3fc42..c4e2edca 100644 --- a/app/Http/Controllers/Api/V1/UserController.php +++ b/app/Http/Controllers/Api/V1/UserController.php @@ -6,6 +6,7 @@ namespace App\Http\Controllers\Api\V1; use App\Exceptions\Api\CanNotDeleteUserWhoIsOwnerOfOrganizationWithMultipleMembers; use App\Exceptions\Api\UserResendEmailVerificationNoPendingEmailApiException; +use App\Http\Requests\V1\User\UserDestroyRequest; use App\Http\Requests\V1\User\UserUpdateCurrentOrganizationRequest; use App\Http\Requests\V1\User\UserUpdateRequest; use App\Http\Resources\V1\User\UserResource; @@ -193,7 +194,7 @@ class UserController extends Controller * @throws AuthorizationException Thrown when the authenticated user does not match the user to be deleted. * @throws CanNotDeleteUserWhoIsOwnerOfOrganizationWithMultipleMembers Thrown when the user to be deleted is the owner of an organization with multiple members. */ - public function destroy(User $user, DeletionService $deletionService): JsonResponse + public function destroy(User $user, UserDestroyRequest $request, DeletionService $deletionService): JsonResponse { if ($user->getKey() !== $this->user()->getKey()) { throw new AuthorizationException; diff --git a/app/Http/Requests/V1/Organization/OrganizationDestroyRequest.php b/app/Http/Requests/V1/Organization/OrganizationDestroyRequest.php new file mode 100644 index 00000000..b8f8686c --- /dev/null +++ b/app/Http/Requests/V1/Organization/OrganizationDestroyRequest.php @@ -0,0 +1,48 @@ +> + */ + public function rules(): array + { + return [ + 'password' => [ + 'required', + 'string', + ], + ]; + } + + /** + * @return array + */ + public function after(): array + { + return [ + function (Validator $validator): void { + if ($validator->errors()->has('password')) { + return; + } + + $user = $this->user(); + $password = $this->input('password'); + + if (! is_string($password) || $user === null || ! Hash::check($password, (string) $user->password)) { + $validator->errors()->add('password', __('The password is incorrect.')); + } + }, + ]; + } +} diff --git a/app/Http/Requests/V1/User/UserDestroyRequest.php b/app/Http/Requests/V1/User/UserDestroyRequest.php new file mode 100644 index 00000000..2a86fd18 --- /dev/null +++ b/app/Http/Requests/V1/User/UserDestroyRequest.php @@ -0,0 +1,48 @@ +> + */ + public function rules(): array + { + return [ + 'password' => [ + 'required', + 'string', + ], + ]; + } + + /** + * @return array + */ + public function after(): array + { + return [ + function (Validator $validator): void { + if ($validator->errors()->has('password')) { + return; + } + + $user = $this->user(); + $password = $this->input('password'); + + if (! is_string($password) || $user === null || ! Hash::check($password, (string) $user->password)) { + $validator->errors()->add('password', __('The password is incorrect.')); + } + }, + ]; + } +} diff --git a/docker-compose.yml b/docker-compose.yml index 7cf82ebc..a937253c 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -107,7 +107,7 @@ services: - sail - reverse-proxy playwright: - image: mcr.microsoft.com/playwright:v1.59.1-jammy + image: mcr.microsoft.com/playwright:v1.60.0-jammy command: ['npx', 'playwright', 'test', '--ui-port=8080', '--ui-host=0.0.0.0'] working_dir: /src extra_hosts: diff --git a/e2e/organization.spec.ts b/e2e/organization.spec.ts index d1b6f367..edbf599e 100644 --- a/e2e/organization.spec.ts +++ b/e2e/organization.spec.ts @@ -1,5 +1,5 @@ import { expect, test } from '../playwright/fixtures'; -import { PLAYWRIGHT_BASE_URL } from '../playwright/config'; +import { PLAYWRIGHT_BASE_URL, TEST_USER_PASSWORD } from '../playwright/config'; async function goToOrganizationSettings(page) { await page.goto(PLAYWRIGHT_BASE_URL + '/dashboard'); @@ -471,6 +471,7 @@ test.describe('Organization Create, Delete & Switch', () => { // Open the confirmation modal, then confirm inside the dialog. await page.getByRole('button', { name: 'Delete Organization' }).click(); + await page.getByRole('dialog').getByPlaceholder('Password').fill(TEST_USER_PASSWORD); await Promise.all([ page.waitForResponse( (response) => @@ -488,6 +489,28 @@ test.describe('Organization Create, Delete & Switch', () => { ).not.toContainText(orgName); }); + test('delete organization shows an error when the password is wrong', async ({ page }) => { + const orgName = 'DeleteOrgWrongPassword' + Math.floor(Math.random() * 100000); + await createOrganization(page, orgName); + await goToOrganizationSettings(page); + + await page.getByRole('button', { name: 'Delete Organization' }).click(); + const dialog = page.getByRole('dialog'); + await dialog.getByPlaceholder('Password').fill('not-the-real-password'); + await Promise.all([ + page.waitForResponse( + (response) => + response.url().includes('/api/v1/organizations') && + response.request().method() === 'DELETE' && + response.status() === 422 + ), + dialog.getByRole('button', { name: 'Delete Organization' }).click(), + ]); + + await expect(dialog.getByRole('alert')).toBeVisible(); + await expect(dialog).toBeVisible(); + }); + test('can switch the current organization via the organization switcher', async ({ page }) => { await page.goto(PLAYWRIGHT_BASE_URL + '/dashboard'); const orgSwitcher = page.locator('[data-testid="organization_switcher"]:visible'); diff --git a/e2e/profile.spec.ts b/e2e/profile.spec.ts index bcc115db..8f1b7d2d 100644 --- a/e2e/profile.spec.ts +++ b/e2e/profile.spec.ts @@ -342,8 +342,8 @@ test('delete account shows an error when the password is wrong', async ({ page } await Promise.all([ page.waitForResponse( (response) => - response.url().includes('/user/confirm-password') && - response.request().method() === 'POST' && + response.url().includes('/api/v1/users/') && + response.request().method() === 'DELETE' && response.status() === 422 ), dialog.getByRole('button', { name: 'Delete Account' }).click(), diff --git a/resources/js/Pages/Profile/Partials/DeleteUserForm.vue b/resources/js/Pages/Profile/Partials/DeleteUserForm.vue index ae5cd48b..16014124 100644 --- a/resources/js/Pages/Profile/Partials/DeleteUserForm.vue +++ b/resources/js/Pages/Profile/Partials/DeleteUserForm.vue @@ -1,15 +1,14 @@ diff --git a/resources/js/packages/api/src/index.ts b/resources/js/packages/api/src/index.ts index cd318369..ce846c92 100644 --- a/resources/js/packages/api/src/index.ts +++ b/resources/js/packages/api/src/index.ts @@ -126,6 +126,8 @@ export type UpdateInvoiceBody = ZodiosBodyByAlias export type User = ZodiosResponseByAlias['data']; export type UpdateUserBody = ZodiosBodyByAlias; +export type DeleteUserBody = ZodiosBodyByAlias; +export type DeleteOrganizationBody = ZodiosBodyByAlias; const api = createApiClient('/api', { validate: 'none' }); diff --git a/resources/js/packages/api/src/openapi.json.client.ts b/resources/js/packages/api/src/openapi.json.client.ts index 1313a23f..d44742ad 100644 --- a/resources/js/packages/api/src/openapi.json.client.ts +++ b/resources/js/packages/api/src/openapi.json.client.ts @@ -37,6 +37,7 @@ const ClientStoreRequest = z.object({ name: z.string().min(1).max(255) }).passth const ClientUpdateRequest = z .object({ name: z.string().min(1).max(255), is_archived: z.boolean().optional() }) .passthrough(); +const DestroyWithPasswordRequest = z.object({ password: z.string() }).passthrough(); const ImportRequest = z.object({ type: z.string(), data: z.string() }).passthrough(); const InvitationResource = z .object({ id: z.string(), email: z.string(), role: z.string() }) @@ -917,6 +918,11 @@ const endpoints = makeApi([ alias: 'deleteOrganization', requestFormat: 'json', parameters: [ + { + name: 'body', + type: 'Body', + schema: DestroyWithPasswordRequest, + }, { name: 'organization', type: 'Path', @@ -4642,6 +4648,11 @@ the organization.`, description: `This endpoint is independent of the organization.`, requestFormat: 'json', parameters: [ + { + name: 'body', + type: 'Body', + schema: DestroyWithPasswordRequest, + }, { name: 'user', type: 'Path', diff --git a/resources/js/utils/useOrganization.ts b/resources/js/utils/useOrganization.ts index 3139ba92..e94184da 100644 --- a/resources/js/utils/useOrganization.ts +++ b/resources/js/utils/useOrganization.ts @@ -2,9 +2,11 @@ import { router } from '@inertiajs/vue3'; import { initializeStores } from '@/utils/init'; import { defineStore } from 'pinia'; import { computed, ref } from 'vue'; +import axios from 'axios'; import type { Organization, OrganizationResponse, + DeleteOrganizationBody, UpdateOrganizationBody, } from '@/packages/api/src'; import { useNotificationsStore } from '@/utils/notification'; @@ -38,7 +40,7 @@ export async function switchOrganization(organizationId: string) { export const useOrganizationStore = defineStore('organization', () => { const organizationResponse = ref(null); - const { handleApiRequestNotifications } = useNotificationsStore(); + const { addNotification, handleApiRequestNotifications } = useNotificationsStore(); async function fetchOrganization() { const organization = getCurrentOrganizationId(); @@ -78,17 +80,26 @@ export const useOrganizationStore = defineStore('organization', () => { return response?.data ?? null; } - async function deleteOrganization(organizationId: string) { - await handleApiRequestNotifications( - () => - api.deleteOrganization(undefined, { - params: { - organization: organizationId, - }, - }), - 'Organization deleted successfully', - 'Failed to delete organization' - ); + async function deleteOrganization(organizationId: string, body: DeleteOrganizationBody) { + try { + await api.deleteOrganization(body, { + params: { + organization: organizationId, + }, + }); + addNotification('success', 'Organization deleted successfully'); + } catch (error) { + if (!axios.isAxiosError(error) || error.response?.status !== 422) { + addNotification( + 'error', + 'Failed to delete organization', + axios.isAxiosError(error) + ? (error.response?.data?.message ?? 'Please try again later.') + : 'Please try again later.' + ); + } + throw error; + } } const organization = computed(() => { diff --git a/resources/js/utils/useUserQuery.ts b/resources/js/utils/useUserQuery.ts index 6757fa18..ef59f95d 100644 --- a/resources/js/utils/useUserQuery.ts +++ b/resources/js/utils/useUserQuery.ts @@ -1,7 +1,7 @@ import { useMutation, useQuery, useQueryClient } from '@tanstack/vue-query'; import { computed } from 'vue'; import axios from 'axios'; -import { api, type UpdateUserBody, type User } from '@/packages/api/src'; +import { api, type DeleteUserBody, type UpdateUserBody, type User } from '@/packages/api/src'; import { useNotificationsStore } from '@/utils/notification'; const ME_QUERY_KEY = ['me'] as const; @@ -61,9 +61,9 @@ export function useDeleteUserMutation() { const { addNotification } = useNotificationsStore(); return useMutation({ - mutationFn: async (userId: string) => { + mutationFn: async ({ userId, body }: { userId: string; body: DeleteUserBody }) => { try { - await api.deleteUser(undefined, { params: { user: userId } }); + await api.deleteUser(body, { params: { user: userId } }); } catch (error) { if (!axios.isAxiosError(error) || error.response?.status !== 422) { addNotification( diff --git a/tests/Unit/Endpoint/Api/V1/OrganizationEndpointTest.php b/tests/Unit/Endpoint/Api/V1/OrganizationEndpointTest.php index 76ec58b7..beacf828 100644 --- a/tests/Unit/Endpoint/Api/V1/OrganizationEndpointTest.php +++ b/tests/Unit/Endpoint/Api/V1/OrganizationEndpointTest.php @@ -441,7 +441,9 @@ class OrganizationEndpointTest extends ApiEndpointTestAbstract Passport::actingAs($data->user); // Act - $response = $this->deleteJson(route('api.v1.organizations.destroy', [$data->organization->getKey()])); + $response = $this->deleteJson(route('api.v1.organizations.destroy', [$data->organization->getKey()]), [ + 'password' => 'password', + ]); // Assert $response->assertForbidden(); @@ -456,12 +458,54 @@ class OrganizationEndpointTest extends ApiEndpointTestAbstract Passport::actingAs($data->user); // Act - $response = $this->deleteJson(route('api.v1.organizations.destroy', ['not-uuid'])); + $response = $this->deleteJson(route('api.v1.organizations.destroy', ['not-uuid']), [ + 'password' => 'password', + ]); // Assert $response->assertNotFound(); } + public function test_delete_endpoint_fails_without_password(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'organizations:delete', + ]); + Passport::actingAs($data->user); + + // Act + $response = $this->deleteJson(route('api.v1.organizations.destroy', [$data->organization->getKey()])); + + // Assert + $response->assertUnprocessable(); + $response->assertJsonValidationErrors(['password']); + $this->assertDatabaseHas(Organization::class, [ + 'id' => $data->organization->getKey(), + ]); + } + + public function test_delete_endpoint_fails_with_wrong_password(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'organizations:delete', + ]); + Passport::actingAs($data->user); + + // Act + $response = $this->deleteJson(route('api.v1.organizations.destroy', [$data->organization->getKey()]), [ + 'password' => 'wrong-password', + ]); + + // Assert + $response->assertUnprocessable(); + $response->assertJsonValidationErrors(['password']); + $this->assertDatabaseHas(Organization::class, [ + 'id' => $data->organization->getKey(), + ]); + } + public function test_delete_endpoint_can_delete_organization(): void { // Arrange @@ -472,7 +516,9 @@ class OrganizationEndpointTest extends ApiEndpointTestAbstract Passport::actingAs($data->user); // Act - $response = $this->deleteJson(route('api.v1.organizations.destroy', [$data->organization->getKey()])); + $response = $this->deleteJson(route('api.v1.organizations.destroy', [$data->organization->getKey()]), [ + 'password' => 'password', + ]); // Assert $response->assertNoContent(); diff --git a/tests/Unit/Endpoint/Api/V1/UserEndpointTest.php b/tests/Unit/Endpoint/Api/V1/UserEndpointTest.php index fdb41e4b..e6a7e648 100644 --- a/tests/Unit/Endpoint/Api/V1/UserEndpointTest.php +++ b/tests/Unit/Endpoint/Api/V1/UserEndpointTest.php @@ -649,7 +649,9 @@ class UserEndpointTest extends ApiEndpointTestAbstract Passport::actingAs($otherData->user); // Act - $response = $this->deleteJson(route('api.v1.users.destroy', $data->user->getKey())); + $response = $this->deleteJson(route('api.v1.users.destroy', $data->user->getKey()), [ + 'password' => 'password', + ]); // Assert $response->assertForbidden(); @@ -674,13 +676,15 @@ class UserEndpointTest extends ApiEndpointTestAbstract Passport::actingAs($data->user); // Act - $response = $this->deleteJson(route('api.v1.users.destroy', 'not-valid')); + $response = $this->deleteJson(route('api.v1.users.destroy', 'not-valid'), [ + 'password' => 'password', + ]); // Assert $response->assertNotFound(); } - public function test_delete_removes_user(): void + public function test_delete_fails_without_password(): void { // Arrange $data = $this->createUserWithPermission(); @@ -689,6 +693,40 @@ class UserEndpointTest extends ApiEndpointTestAbstract // Act $response = $this->deleteJson(route('api.v1.users.destroy', $data->user->getKey())); + // Assert + $response->assertUnprocessable(); + $response->assertJsonValidationErrors(['password']); + $this->assertDatabaseHas(User::class, ['id' => $data->user->getKey()]); + } + + public function test_delete_fails_with_wrong_password(): void + { + // Arrange + $data = $this->createUserWithPermission(); + Passport::actingAs($data->user); + + // Act + $response = $this->deleteJson(route('api.v1.users.destroy', $data->user->getKey()), [ + 'password' => 'wrong-password', + ]); + + // Assert + $response->assertUnprocessable(); + $response->assertJsonValidationErrors(['password']); + $this->assertDatabaseHas(User::class, ['id' => $data->user->getKey()]); + } + + public function test_delete_removes_user(): void + { + // Arrange + $data = $this->createUserWithPermission(); + Passport::actingAs($data->user); + + // Act + $response = $this->deleteJson(route('api.v1.users.destroy', $data->user->getKey()), [ + 'password' => 'password', + ]); + // Assert $response->assertNoContent(); $this->assertDatabaseMissing(User::class, ['id' => $data->user->getKey()]); diff --git a/tests/Unit/Filament/Resources/OrganizationResourceTest.php b/tests/Unit/Filament/Resources/OrganizationResourceTest.php index 6762e89b..5ce3c098 100644 --- a/tests/Unit/Filament/Resources/OrganizationResourceTest.php +++ b/tests/Unit/Filament/Resources/OrganizationResourceTest.php @@ -4,12 +4,17 @@ declare(strict_types=1); namespace Tests\Unit\Filament\Resources; +use App\Enums\Role; +use App\Events\OrganizationInvitationAdding; use App\Filament\Resources\OrganizationResource; +use App\Mail\OrganizationInvitationMail; use App\Models\Organization; use App\Models\OrganizationInvitation; use App\Models\User; use App\Service\DeletionService; use Illuminate\Support\Facades\Config; +use Illuminate\Support\Facades\Event; +use Illuminate\Support\Facades\Mail; use Livewire\Livewire; use Mockery\MockInterface; use PHPUnit\Framework\Attributes\UsesClass; @@ -112,4 +117,34 @@ class OrganizationResourceTest extends FilamentTestCase $response->assertSuccessful(); $response->assertCanSeeTableRecords($organizationInvitations); } + + public function test_can_create_related_invitation(): void + { + // Arrange + Event::fake([ + OrganizationInvitationAdding::class, + ]); + Mail::fake(); + $organization = Organization::factory()->create(); + + // Act + $response = Livewire::test(OrganizationResource\RelationManagers\InvitationsRelationManager::class, [ + 'ownerRecord' => $organization, + 'pageClass' => OrganizationResource\Pages\EditOrganization::class, + ])->callTableAction('create', data: [ + 'email' => 'new-user@example.com', + 'role' => Role::Employee->value, + ]); + + // Assert + $response->assertSuccessful(); + $response->assertHasNoTableActionErrors(); + $this->assertDatabaseHas(OrganizationInvitation::class, [ + 'organization_id' => $organization->getKey(), + 'email' => 'new-user@example.com', + 'role' => Role::Employee->value, + ]); + Event::assertDispatched(OrganizationInvitationAdding::class); + Mail::assertQueued(OrganizationInvitationMail::class); + } }