Skip to content
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

[9.x] Default 404 message on denyAsNotFound #43901

Merged
merged 8 commits into from
Sep 1, 2022
Merged

[9.x] Default 404 message on denyAsNotFound #43901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0775f0d
default 404 message when denyAsNotFound
guilledll Aug 28, 2022
4412c3e
fix test 404
guilledll Aug 28, 2022
df46b55
wording
guilledll Aug 28, 2022
bd3f7ba
message parse on response class
guilledll Aug 29, 2022
c341ad4
partial test fixing
guilledll Aug 29, 2022
b41a851
proposed message precedence change
guilledll Aug 30, 2022
36550e0
formatting
timacdonald Aug 30, 2022
fc9328a
code style
timacdonald Aug 31, 2022
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
4 changes: 3 additions & 1 deletion src/Illuminate/Foundation/Exceptions/Handler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,9 @@ protected function prepareException(Throwable $e)
return match (true) {
$e instanceof BackedEnumCaseNotFoundException => new NotFoundHttpException($e->getMessage(), $e),
$e instanceof ModelNotFoundException => new NotFoundHttpException($e->getMessage(), $e),
$e instanceof AuthorizationException && $e->hasStatus() => new HttpException($e->status(), $e->getMessage(), $e),
$e instanceof AuthorizationException && $e->hasStatus() => new HttpException(
$e->status(), $e->response()?->message() ?: (Response::$statusTexts[$e->status()] ?? 'Whoops, looks like something went wrong.'), $e
),
Comment on lines -381 to +383
Copy link
Member

@timacdonald timacdonald Aug 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that 'Whoops, looks like something went wrong.' is only presented to the user when:

  1. The deny action did not receive a message.
  2. The status code in unknown to Symfony.

Also, this message is really on for JSON responses, as Symfony will show this message no matter what we change this too. For example, doing the following on a fresh application:

// .env:

APP_DEBUG=false


// Route:

Route::get('/', function () {
    throw new HttpException(399, 'foo');
});

Screen Shot 2022-08-31 at 10 12 48 am

So using this messaging actually creates consistency between the HTML response and the JSON response.

$e instanceof AuthorizationException && ! $e->hasStatus() => new AccessDeniedHttpException($e->getMessage(), $e),
$e instanceof TokenMismatchException => new HttpException(419, $e->getMessage(), $e),
$e instanceof SuspiciousOperationException => new NotFoundHttpException('Bad hostname provided.', $e),
Expand Down
7 changes: 7 additions & 0 deletions tests/Auth/AuthAccessResponseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ public function testItSetsEmptyStatusOnExceptionWhenAuthorizing()
} catch (AuthorizationException $e) {
$this->assertNull($e->status());
$this->assertFalse($e->hasStatus());
$this->assertSame('foo', $e->response()->message());
$this->assertSame('foo', $e->getMessage());
$this->assertSame(3, $e->getCode());
}
Expand All @@ -56,6 +57,7 @@ public function testItSetsStatusOnExceptionWhenAuthorizing()
} catch (AuthorizationException $e) {
$this->assertSame(418, $e->status());
$this->assertTrue($e->hasStatus());
$this->assertSame('foo', $e->response()->message());
$this->assertSame('foo', $e->getMessage());
$this->assertSame(3, $e->getCode());
}
Expand All @@ -66,6 +68,7 @@ public function testItSetsStatusOnExceptionWhenAuthorizing()
} catch (AuthorizationException $e) {
$this->assertSame(404, $e->status());
$this->assertTrue($e->hasStatus());
$this->assertSame('foo', $e->response()->message());
$this->assertSame('foo', $e->getMessage());
$this->assertSame(3, $e->getCode());
}
Expand All @@ -76,6 +79,7 @@ public function testItSetsStatusOnExceptionWhenAuthorizing()
} catch (AuthorizationException $e) {
$this->assertSame(444, $e->status());
$this->assertTrue($e->hasStatus());
$this->assertNull($e->response()->message());
$this->assertSame('This action is unauthorized.', $e->getMessage());
$this->assertSame(0, $e->getCode());
}
Expand All @@ -86,6 +90,7 @@ public function testItSetsStatusOnExceptionWhenAuthorizing()
} catch (AuthorizationException $e) {
$this->assertSame(444, $e->status());
$this->assertTrue($e->hasStatus());
$this->assertSame('foo', $e->response()->message());
$this->assertSame('foo', $e->getMessage());
$this->assertSame(3, $e->getCode());
}
Expand All @@ -96,6 +101,7 @@ public function testItSetsStatusOnExceptionWhenAuthorizing()
} catch (AuthorizationException $e) {
$this->assertSame(404, $e->status());
$this->assertTrue($e->hasStatus());
$this->assertNull($e->response()->message());
$this->assertSame('This action is unauthorized.', $e->getMessage());
$this->assertSame(0, $e->getCode());
}
Expand All @@ -106,6 +112,7 @@ public function testItSetsStatusOnExceptionWhenAuthorizing()
} catch (AuthorizationException $e) {
$this->assertSame(404, $e->status());
$this->assertTrue($e->hasStatus());
$this->assertSame('foo', $e->response()->message());
$this->assertSame('foo', $e->getMessage());
$this->assertSame(3, $e->getCode());
}
Expand Down
66 changes: 66 additions & 0 deletions tests/Integration/Foundation/ExceptionHandlerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Illuminate\Tests\Integration\Foundation;

use Illuminate\Auth\Access\AuthorizationException;
use Illuminate\Auth\Access\Response;
use Illuminate\Support\Facades\Route;
use Orchestra\Testbench\TestCase;
Expand Down Expand Up @@ -41,4 +42,69 @@ public function testItRendersAuthorizationExceptionsWithCustomStatusCode()
'message' => 'expected message',
]);
}

public function testItRendersAuthorizationExceptionsWithStatusCodeTextWhenNoMessageIsSet()
{
Route::get('test-route', fn () => Response::denyWithStatus(404)->authorize());

// HTTP request...
$this->get('test-route')
->assertStatus(404)
->assertSeeText('Not Found');

// JSON request...
$this->getJson('test-route')
->assertStatus(404)
->assertExactJson([
'message' => 'Not Found',
]);

Route::get('test-route', fn () => Response::denyWithStatus(418)->authorize());

// HTTP request...
$this->get('test-route')
->assertStatus(418)
->assertSeeText("I'm a teapot", false);

// JSON request...
$this->getJson('test-route')
->assertStatus(418)
->assertExactJson([
'message' => "I'm a teapot",
]);
}

public function testItRendersAuthorizationExceptionsWithStatusButWithoutResponse()
{
Route::get('test-route', fn () => throw (new AuthorizationException())->withStatus(418));

// HTTP request...
$this->get('test-route')
->assertStatus(418)
->assertSeeText("I'm a teapot", false);

// JSON request...
$this->getJson('test-route')
->assertStatus(418)
->assertExactJson([
'message' => "I'm a teapot",
]);
}

public function testItHasFallbackErrorMessageForUnknownStatusCodes()
{
Route::get('test-route', fn () => throw (new AuthorizationException())->withStatus(399));

// HTTP request...
$this->get('test-route')
->assertStatus(399)
->assertSeeText('Whoops, looks like something went wrong.');

// JSON request...
$this->getJson('test-route')
->assertStatus(399)
->assertExactJson([
'message' => 'Whoops, looks like something went wrong.',
]);
}
}