laravel / nova-issues

556 stars 34 forks source link

Status code 200 on unpermitted DELETE api call #6364

Closed Joorren closed 7 months ago

Joorren commented 7 months ago

Description:

When trying to remove a resource with a user that does not have permission to remove the resource, the API would still return status code 200, even though the resource can't and won't be removed. Because the user isn't permitted to remove the resource, status code 403 would be expected.

I can only trigger this logic from unit testing, as the 'remove resource' button correctly doesn't show in the front-end.

// JobSitePolicy.php
public function delete(User $user, JobSite $jobSite): bool
{
    return $user->isAdmin();
}
// JobSiteTest.php
/**
 * @test
 */
public function job_site_detail_cannot_be_removed_as_coach()
{
    $user = User::factory()->asCoach()->create();
    $jobSite = JobSite::factory()->create();

    $this->actingAs($user)
        ->get(config('nova.path') . '/resources/job-sites/' . $jobSite->id)
        ->assertOk();

    $this
        ->delete('/nova-api/job-sites?resources[]=' . $jobSite->id)
        ->assertForbidden(); // This test fails because it returns status code 200

    $this->get(config('nova.path') . '/resources/job-sites/' . $jobSite->id)
        ->assertOk();
}

Detailed steps to reproduce the issue on a fresh Nova installation:

  1. Login with a user that doesn't have delete permissions according to the policy
  2. Execute the DELETE api call
  3. Receive status code 200 (expected 403)
  4. Find out the resource didn't get removed (correct)
crynobone commented 7 months ago

Hi there,

DELETE API will always return 200 as it would allow batch deletion of resources. However, Laravel\Nova\Http\Requests\DeleteResourceRequest will always check for authorizeToDelete() and prevent the deletion. This are being tested and covered by our integration tests.

crynobone commented 7 months ago

You should be testing if the correct records get deleted by checking model/database records exists instead of status code.