From 6399bacdb6870f2fa00318249b0ea80765771e24 Mon Sep 17 00:00:00 2001 From: "bert.hausmans" Date: Tue, 28 Apr 2026 08:57:06 +0200 Subject: [PATCH] refactor(form-builder): restore type-hinted route model binding for failures controller (WS-6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the manual `$request->route('formSubmissionActionFailure')` workaround with type-hinted parameters. Implicit route model binding now resolves FormSubmissionActionFailure correctly on both the platform admin route (/admin/form-failures/{id}) and the org-scoped route (/organisations/{organisation}/form-failures/{id}). Root cause: On the nested org-scoped route, Laravel's implicit binding triggers its scoped-binding code path: for the second URL segment, it tries to resolve the failure as a relation of the route's parent ({organisation}) by calling `$organisation->formSubmissionActionFailures()`. Organisation has no such relation (failures live under FormSubmission, not Organisation directly), so the lookup silently fell through and the controller received a raw string. PHP then raised a TypeError on the type-hinted parameter. A second issue compounded it: with the controller method declaring `(FormSubmissionActionFailure $formSubmissionActionFailure, ?Organisation $organisation)` the parameter order did NOT match the URL parameter order (/{organisation}/.../{formSubmissionActionFailure}), so Laravel's resolveMethodDependencies — which falls back to positional binding when parameter counts diverge — bound them to the wrong slots. Fix: - Register an explicit `Route::bind('formSubmissionActionFailure', ...)` in AppServiceProvider that loads the model `withoutGlobalScopes()` and throws ModelNotFoundException on miss. This sidesteps the scoped-binding parent-relation lookup entirely. - Add `->withoutScopedBindings()` to all four org-scoped routes (show, retry, resolve, dismiss) as a belt-and-braces guarantee that Laravel never enters the scoped-binding path for these nested routes. - Reorder controller method signatures to put `?Organisation $organisation` FIRST, matching URL parameter order so positional binding lands the ULID strings on the correct method parameters. - Drop the now-unused private `resolveFailure()` helper. - Tenant scoping continues to be enforced by FormSubmissionActionFailurePolicy via the failure.submission.organisation_id FK chain (RFC V3); cross- tenant access still translates denied → 404, never 403. Tests: all 9 controller tests pass (cross-tenant 404 contract verified for view, dismiss, and resolve). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../FormSubmissionActionFailureController.php | 48 +++++------ api/app/Providers/AppServiceProvider.php | 36 +++++++-- api/routes/api.php | 80 ++++++++++--------- ...mSubmissionActionFailureControllerTest.php | 21 +++-- 4 files changed, 105 insertions(+), 80 deletions(-) diff --git a/api/app/Http/Controllers/Api/V1/FormBuilder/FormSubmissionActionFailureController.php b/api/app/Http/Controllers/Api/V1/FormBuilder/FormSubmissionActionFailureController.php index fe7f2228..4b73c2a4 100644 --- a/api/app/Http/Controllers/Api/V1/FormBuilder/FormSubmissionActionFailureController.php +++ b/api/app/Http/Controllers/Api/V1/FormBuilder/FormSubmissionActionFailureController.php @@ -55,17 +55,23 @@ final class FormSubmissionActionFailureController extends Controller return FormSubmissionActionFailureResource::collection($failures); } - public function show(\Illuminate\Http\Request $request): FormSubmissionActionFailureResource + public function show(?Organisation $organisation, FormSubmissionActionFailure $formSubmissionActionFailure): FormSubmissionActionFailureResource { - $failure = $this->resolveFailure((string) $request->route('formSubmissionActionFailure')); - $this->authorizeOrNotFound('view', $failure); + // $organisation is bound only on the org-scoped route; null on the + // platform admin route. We don't use it directly — the policy + // enforces tenant scope via failure.submission.organisation_id (RFC V3). + // Declaration order matches URL parameter order so Laravel's + // resolveMethodDependencies binds positionally as expected. + unset($organisation); + $this->authorizeOrNotFound('view', $formSubmissionActionFailure); - return new FormSubmissionActionFailureResource($failure); + return new FormSubmissionActionFailureResource($formSubmissionActionFailure); } - public function retry(\Illuminate\Http\Request $request, FormBindingApplicator $applicator): FormSubmissionActionFailureResource|JsonResponse + public function retry(?Organisation $organisation, FormSubmissionActionFailure $formSubmissionActionFailure, FormBindingApplicator $applicator): FormSubmissionActionFailureResource|JsonResponse { - $failure = $this->resolveFailure((string) $request->route('formSubmissionActionFailure')); + unset($organisation); + $failure = $formSubmissionActionFailure; $this->authorizeOrNotFound('retry', $failure); if (! $failure->canBeRetried()) { @@ -119,9 +125,12 @@ final class FormSubmissionActionFailureController extends Controller } public function resolve( + ?Organisation $organisation, + FormSubmissionActionFailure $formSubmissionActionFailure, ResolveFailureRequest $request, ): FormSubmissionActionFailureResource|JsonResponse { - $failure = $this->resolveFailure((string) $request->route('formSubmissionActionFailure')); + unset($organisation); + $failure = $formSubmissionActionFailure; $this->authorizeOrNotFound('resolve', $failure); if ($failure->resolved_at !== null) { @@ -145,9 +154,12 @@ final class FormSubmissionActionFailureController extends Controller } public function dismiss( + ?Organisation $organisation, + FormSubmissionActionFailure $formSubmissionActionFailure, DismissFailureRequest $request, ): FormSubmissionActionFailureResource|JsonResponse { - $failure = $this->resolveFailure((string) $request->route('formSubmissionActionFailure')); + unset($organisation); + $failure = $formSubmissionActionFailure; $this->authorizeOrNotFound('dismiss', $failure); if ($failure->resolved_at !== null) { @@ -171,24 +183,6 @@ final class FormSubmissionActionFailureController extends Controller return new FormSubmissionActionFailureResource($failure->refresh()); } - /** - * Manual model resolution. Implicit binding doesn't pick up nested- - * namespace ULID models reliably across nested route groups, so we - * load explicitly without global scopes (cross-tenant access reaches - * the policy, which then translates denied → 404 per RFC V3). - */ - private function resolveFailure(string $id): FormSubmissionActionFailure - { - $failure = FormSubmissionActionFailure::query() - ->withoutGlobalScopes() - ->find($id); - if ($failure === null) { - throw new ModelNotFoundException(); - } - - return $failure; - } - /** * Translate a denied policy to 404 (RFC V3) so cross-tenant access * does NOT confirm resource existence. @@ -196,7 +190,7 @@ final class FormSubmissionActionFailureController extends Controller private function authorizeOrNotFound(string $ability, FormSubmissionActionFailure $failure): void { if (Gate::denies($ability, $failure)) { - throw new ModelNotFoundException(); + throw new ModelNotFoundException; } } } diff --git a/api/app/Providers/AppServiceProvider.php b/api/app/Providers/AppServiceProvider.php index 42cfe9d6..7ccaf536 100644 --- a/api/app/Providers/AppServiceProvider.php +++ b/api/app/Providers/AppServiceProvider.php @@ -4,12 +4,18 @@ declare(strict_types=1); namespace App\Providers; +use App\Events\FormBuilder\FormSubmissionSectionSubmitted; +use App\Events\FormBuilder\FormSubmissionSubmitted; use App\FormBuilder\Bindings\BindingActivityLogger; use App\FormBuilder\Bindings\BindingConflictResolver; use App\FormBuilder\Bindings\BindingTypeRegistry; use App\FormBuilder\Bindings\FormBindingApplicator; use App\FormBuilder\Bindings\PersonProvisioner; use App\FormBuilder\Purposes\PurposeRegistry; +use App\Listeners\FormBuilder\ApplyBindingsOnFormSectionSubmitted; +use App\Listeners\FormBuilder\ApplyBindingsOnFormSubmit; +use App\Listeners\FormBuilder\SyncTagPickerSelectionsOnSubmit; +use App\Listeners\FormBuilder\TriggerPersonIdentityMatchOnFormSubmit; use App\Models\Company; use App\Models\CrowdList; use App\Models\CrowdType; @@ -50,12 +56,6 @@ use App\Models\UserInvitation; use App\Models\UserOrganisationTag; use App\Models\UserProfile; use App\Models\VolunteerAvailability; -use App\Events\FormBuilder\FormSubmissionSectionSubmitted; -use App\Events\FormBuilder\FormSubmissionSubmitted; -use App\Listeners\FormBuilder\ApplyBindingsOnFormSectionSubmitted; -use App\Listeners\FormBuilder\ApplyBindingsOnFormSubmit; -use App\Listeners\FormBuilder\SyncTagPickerSelectionsOnSubmit; -use App\Listeners\FormBuilder\TriggerPersonIdentityMatchOnFormSubmit; use App\Observers\FormBuilder\FormFieldChildTablesCascadeObserver; use App\Observers\FormBuilder\FormSubmissionObserver; use App\Observers\FormBuilder\FormValueObserver; @@ -63,8 +63,8 @@ use App\Observers\PersonObserver; use App\Observers\UserObserver; use Illuminate\Auth\Notifications\ResetPassword; use Illuminate\Database\Eloquent\Relations\Relation; -use Illuminate\Support\ServiceProvider; use Illuminate\Support\Facades\Gate; +use Illuminate\Support\ServiceProvider; use Spatie\Activitylog\Models\Activity; class AppServiceProvider extends ServiceProvider @@ -130,6 +130,26 @@ class AppServiceProvider extends ServiceProvider \App\Policies\FormBuilder\FormSubmissionActionFailurePolicy::class, ); + // RFC-WS-6 v1.1 §3 Q5 — explicit route model binding for the + // form-failures admin endpoints. Implicit binding on nested route + // groups triggers Laravel's scoped-binding logic that tries to + // resolve the failure as a relation of the route's parent + // ({organisation}) — Organisation has no formSubmissionActionFailures() + // relation, so the implicit lookup silently fails and the + // controller receives a string. Route::bind sidesteps the + // scoped-binding path entirely. Bypass global scopes so cross- + // tenant access reaches the policy (which translates denied → 404 + // per RFC V3 in the controller). + \Illuminate\Support\Facades\Route::bind('formSubmissionActionFailure', static function (string $value): \App\Models\FormBuilder\FormSubmissionActionFailure { + $model = \App\Models\FormBuilder\FormSubmissionActionFailure::query() + ->withoutGlobalScopes() + ->find($value); + if ($model === null) { + throw new \Illuminate\Database\Eloquent\ModelNotFoundException; + } + + return $model; + }); Person::observe(PersonObserver::class); User::observe(UserObserver::class); @@ -177,7 +197,7 @@ class AppServiceProvider extends ServiceProvider ); ResetPassword::createUrlUsing(function ($user, string $token) { - return config('crewli.portal_url') . '/wachtwoord-resetten?token=' . $token . '&email=' . urlencode($user->email); + return config('crewli.portal_url').'/wachtwoord-resetten?token='.$token.'&email='.urlencode($user->email); }); // Tag activity log entries with impersonation context diff --git a/api/routes/api.php b/api/routes/api.php index 31f9face..4864a992 100644 --- a/api/routes/api.php +++ b/api/routes/api.php @@ -2,46 +2,24 @@ declare(strict_types=1); -use App\Http\Controllers\Api\V1\CheckEmailController; -use App\Http\Controllers\Api\V1\CompanyController; -use App\Http\Controllers\Api\V1\EmailLogController; -use App\Http\Controllers\Api\V1\CrowdListController; -use App\Http\Controllers\Api\V1\CrowdTypeController; -use App\Http\Controllers\Api\V1\EventController; -use App\Http\Controllers\Api\V1\FestivalSectionController; -use App\Http\Controllers\Api\V1\InvitationController; -use App\Http\Controllers\Api\V1\LocationController; -use App\Http\Controllers\Api\V1\LoginController; -use App\Http\Controllers\Api\V1\LogoutController; -use App\Http\Controllers\Api\V1\MeController; -use App\Http\Controllers\Api\V1\MemberController; -use App\Http\Controllers\Api\V1\OrganisationController; -use App\Http\Controllers\Api\V1\OrganisationEmailSettingsController; -use App\Http\Controllers\Api\V1\OrganisationEmailTemplateController; -use App\Http\Controllers\Api\V1\PersonController; -use App\Http\Controllers\Api\V1\PersonIdentityMatchController; -use App\Http\Controllers\Api\V1\PersonTagController; -use App\Http\Controllers\Api\V1\ShiftAssignmentController; -use App\Http\Controllers\Api\V1\ShiftController; -use App\Http\Controllers\Api\V1\TimeSlotController; -use App\Http\Controllers\Api\V1\VolunteerAvailabilityController; -use App\Http\Controllers\Api\V1\PortalTokenController; use App\Http\Controllers\Api\V1\AccountController; -use App\Http\Controllers\Api\V1\AuthRefreshController; -use App\Http\Controllers\Api\V1\EmailChangeController; -use App\Http\Controllers\Api\V1\PasswordResetController; -use App\Http\Controllers\Api\V1\PortalMeController; -use App\Http\Controllers\Api\V1\Portal\PortalShiftController; -use App\Http\Controllers\Api\V1\UploadController; -use App\Http\Controllers\Api\V1\UserOrganisationTagController; -use App\Http\Controllers\Api\V1\Admin\AdminOrganisationController; -use App\Http\Controllers\Api\V1\Admin\AdminUserController; -use App\Http\Controllers\Api\V1\Admin\AdminStatsController; use App\Http\Controllers\Api\V1\Admin\AdminActivityLogController; use App\Http\Controllers\Api\V1\Admin\AdminImpersonationController; +use App\Http\Controllers\Api\V1\Admin\AdminOrganisationController; +use App\Http\Controllers\Api\V1\Admin\AdminStatsController; +use App\Http\Controllers\Api\V1\Admin\AdminUserController; use App\Http\Controllers\Api\V1\Auth\MfaSetupController; use App\Http\Controllers\Api\V1\Auth\MfaVerifyController; use App\Http\Controllers\Api\V1\Auth\TrustedDeviceController; +use App\Http\Controllers\Api\V1\AuthRefreshController; +use App\Http\Controllers\Api\V1\CheckEmailController; +use App\Http\Controllers\Api\V1\CompanyController; +use App\Http\Controllers\Api\V1\CrowdListController; +use App\Http\Controllers\Api\V1\CrowdTypeController; +use App\Http\Controllers\Api\V1\EmailChangeController; +use App\Http\Controllers\Api\V1\EmailLogController; +use App\Http\Controllers\Api\V1\EventController; +use App\Http\Controllers\Api\V1\FestivalSectionController; use App\Http\Controllers\Api\V1\FormBuilder\FilterRegistryController; use App\Http\Controllers\Api\V1\FormBuilder\FormFieldController; use App\Http\Controllers\Api\V1\FormBuilder\FormFieldLibraryController; @@ -53,6 +31,28 @@ use App\Http\Controllers\Api\V1\FormBuilder\FormSubmissionReviewController; use App\Http\Controllers\Api\V1\FormBuilder\FormTemplateController; use App\Http\Controllers\Api\V1\FormBuilder\FormValueController; use App\Http\Controllers\Api\V1\FormBuilder\PublicFormController; +use App\Http\Controllers\Api\V1\InvitationController; +use App\Http\Controllers\Api\V1\LocationController; +use App\Http\Controllers\Api\V1\LoginController; +use App\Http\Controllers\Api\V1\LogoutController; +use App\Http\Controllers\Api\V1\MeController; +use App\Http\Controllers\Api\V1\MemberController; +use App\Http\Controllers\Api\V1\OrganisationController; +use App\Http\Controllers\Api\V1\OrganisationEmailSettingsController; +use App\Http\Controllers\Api\V1\OrganisationEmailTemplateController; +use App\Http\Controllers\Api\V1\PasswordResetController; +use App\Http\Controllers\Api\V1\PersonController; +use App\Http\Controllers\Api\V1\PersonIdentityMatchController; +use App\Http\Controllers\Api\V1\PersonTagController; +use App\Http\Controllers\Api\V1\Portal\PortalShiftController; +use App\Http\Controllers\Api\V1\PortalMeController; +use App\Http\Controllers\Api\V1\PortalTokenController; +use App\Http\Controllers\Api\V1\ShiftAssignmentController; +use App\Http\Controllers\Api\V1\ShiftController; +use App\Http\Controllers\Api\V1\TimeSlotController; +use App\Http\Controllers\Api\V1\UploadController; +use App\Http\Controllers\Api\V1\UserOrganisationTagController; +use App\Http\Controllers\Api\V1\VolunteerAvailabilityController; use App\Models\FestivalSection; use App\Models\Organisation; use Illuminate\Support\Facades\Gate; @@ -243,11 +243,15 @@ Route::middleware(['auth:sanctum', 'impersonation'])->group(function () { Route::get('email-logs', [EmailLogController::class, 'index']); // RFC-WS-6 §3 (Q5) — org-scoped form-failure admin endpoints. + // withoutScopedBindings(): tenant scope is enforced by the policy + // (RFC V3), not by Laravel's parent-relation scoped binding. + // Organisation has no formSubmissionActionFailures() relation; + // the policy's FK-chain check is the tenant gate. Route::get('form-failures', [\App\Http\Controllers\Api\V1\FormBuilder\FormSubmissionActionFailureController::class, 'orgIndex']); - Route::get('form-failures/{formSubmissionActionFailure}', [\App\Http\Controllers\Api\V1\FormBuilder\FormSubmissionActionFailureController::class, 'show']); - Route::post('form-failures/{formSubmissionActionFailure}/retry', [\App\Http\Controllers\Api\V1\FormBuilder\FormSubmissionActionFailureController::class, 'retry']); - Route::post('form-failures/{formSubmissionActionFailure}/resolve', [\App\Http\Controllers\Api\V1\FormBuilder\FormSubmissionActionFailureController::class, 'resolve']); - Route::post('form-failures/{formSubmissionActionFailure}/dismiss', [\App\Http\Controllers\Api\V1\FormBuilder\FormSubmissionActionFailureController::class, 'dismiss']); + Route::get('form-failures/{formSubmissionActionFailure}', [\App\Http\Controllers\Api\V1\FormBuilder\FormSubmissionActionFailureController::class, 'show'])->withoutScopedBindings(); + Route::post('form-failures/{formSubmissionActionFailure}/retry', [\App\Http\Controllers\Api\V1\FormBuilder\FormSubmissionActionFailureController::class, 'retry'])->withoutScopedBindings(); + Route::post('form-failures/{formSubmissionActionFailure}/resolve', [\App\Http\Controllers\Api\V1\FormBuilder\FormSubmissionActionFailureController::class, 'resolve'])->withoutScopedBindings(); + Route::post('form-failures/{formSubmissionActionFailure}/dismiss', [\App\Http\Controllers\Api\V1\FormBuilder\FormSubmissionActionFailureController::class, 'dismiss'])->withoutScopedBindings(); // Person tags (organisation settings) Route::apiResource('person-tags', PersonTagController::class) diff --git a/api/tests/Feature/FormBuilder/Api/FormSubmissionActionFailureControllerTest.php b/api/tests/Feature/FormBuilder/Api/FormSubmissionActionFailureControllerTest.php index 2bc29f4d..b3be4b11 100644 --- a/api/tests/Feature/FormBuilder/Api/FormSubmissionActionFailureControllerTest.php +++ b/api/tests/Feature/FormBuilder/Api/FormSubmissionActionFailureControllerTest.php @@ -72,7 +72,8 @@ final class FormSubmissionActionFailureControllerTest extends TestCase public function test_super_admin_can_view_failure(): void { $this->withoutExceptionHandling(); - Sanctum::actingAs($this->superAdmin); $this + Sanctum::actingAs($this->superAdmin); + $this ->getJson("/api/v1/admin/form-failures/{$this->failureInOrgA->id}") ->assertOk() ->assertJsonPath('data.id', (string) $this->failureInOrgA->id); @@ -91,14 +92,16 @@ final class FormSubmissionActionFailureControllerTest extends TestCase */ public function test_cross_tenant_access_returns_404_not_403(): void { - Sanctum::actingAs($this->orgAdminB); $this + Sanctum::actingAs($this->orgAdminB); + $this ->getJson("/api/v1/organisations/{$this->orgA->id}/form-failures/{$this->failureInOrgA->id}") ->assertStatus(404); } public function test_resolve_endpoint_with_note(): void { - Sanctum::actingAs($this->superAdmin); $this + Sanctum::actingAs($this->superAdmin); + $this ->postJson("/api/v1/admin/form-failures/{$this->failureInOrgA->id}/resolve", [ 'note' => 'fixed via direct edit', ]) @@ -109,7 +112,8 @@ final class FormSubmissionActionFailureControllerTest extends TestCase public function test_dismiss_endpoint_with_enum_reason(): void { - Sanctum::actingAs($this->superAdmin); $this + Sanctum::actingAs($this->superAdmin); + $this ->postJson("/api/v1/admin/form-failures/{$this->failureInOrgA->id}/dismiss", [ 'reason_type' => DismissalReasonType::SCHEMA_DELETED->value, ]) @@ -120,7 +124,8 @@ final class FormSubmissionActionFailureControllerTest extends TestCase public function test_dismiss_other_without_note_fails_422(): void { - Sanctum::actingAs($this->superAdmin); $this + Sanctum::actingAs($this->superAdmin); + $this ->postJson("/api/v1/admin/form-failures/{$this->failureInOrgA->id}/dismiss", [ 'reason_type' => DismissalReasonType::OTHER->value, ]) @@ -130,7 +135,8 @@ final class FormSubmissionActionFailureControllerTest extends TestCase public function test_cross_tenant_dismiss_returns_404(): void { - Sanctum::actingAs($this->orgAdminB); $this + Sanctum::actingAs($this->orgAdminB); + $this ->postJson("/api/v1/organisations/{$this->orgA->id}/form-failures/{$this->failureInOrgA->id}/dismiss", [ 'reason_type' => DismissalReasonType::OTHER->value, 'note' => 'evil', @@ -140,7 +146,8 @@ final class FormSubmissionActionFailureControllerTest extends TestCase public function test_cross_tenant_resolve_returns_404(): void { - Sanctum::actingAs($this->orgAdminB); $this + Sanctum::actingAs($this->orgAdminB); + $this ->postJson("/api/v1/organisations/{$this->orgA->id}/form-failures/{$this->failureInOrgA->id}/resolve", []) ->assertStatus(404); }