Skip to content
This repository has been archived by the owner on Jul 16, 2021. It is now read-only.

[5.5] Unauthenticated redirect route is hard to customise #800

Closed
ockle opened this issue Sep 19, 2017 · 20 comments
Closed

[5.5] Unauthenticated redirect route is hard to customise #800

ockle opened this issue Sep 19, 2017 · 20 comments

Comments

@ockle
Copy link

ockle commented Sep 19, 2017

In 5.5, the unauthenticated method of the exception handler has been moved into the Foundation, however, this makes the hard-coded redirect route annoying to set on a per-app basis. A user now has to override the method in the app's exception handler, copying the whole method body just to change one thing. This is obviously not very user-friendly. I would think a separate method on the handler that returns the location to redirect to would be the best solution, as this is easier for a user to include in their app's handler.

@slashequip
Copy link

slashequip commented Sep 19, 2017

I might be mistaken but I presume this is a last ditch attempt to catch and Laravel expects you to have some middleware to prevent this from being thrown in the first place?

Where is this being thrown from for you that wouldn't benefit from middleware?

EDIT: When I say 'Laravel expects' I mean in the sense of being able to override it.

@ockle
Copy link
Author

ockle commented Sep 19, 2017

@scrwdnet Illuminate\Auth\Middleware\Authenticate, which is the default auth middleware since the user land one was removed in 5.3, throws an AuthenticationException. Therefore this code in Foundation is now the standard method used by Laravel to handle an app's authentication redirects.

Seeing as they removed the Auth middleware from the app, I can only assume that they don't expect users to have to override it. And if a user did, they would have to either throw a different exception and then write further code to handle it, or change the logic to not throw an exception, basically reintroducing the 5.2 behaviour.

@slashequip
Copy link

Ok yeah you are right, might feel a little odd coming after updating a project. I'd guess then that the reason this is the case is because this is Laravel's baked in Auth generated by php artisan make:auth. Running this command gives you the full setup including the named routes, which the exceptions redirects you to.

I imagine the expectation of anything much different should be handled by the developer and implemented manually (even if that involved implementing 5.2 behaviours, it's still the developers choice). The logic feels sound, if you are not logged in then go to login page anything other than that is something away from the 'norm' and involves different business logic?

*IMO of course

@ockle
Copy link
Author

ockle commented Sep 19, 2017

The logic feels sound, if you are not logged in then go to login page

Of course, I'm not arguing that :) My point is that what if my login route is not named "login"? What if someone does not even use named routes? Hardcoded route names should not be in the core of the framework. Sure, they can be introduced as part of the scaffolding, as you say, but this one is not. The framework currently forces a route name onto the developer.

To be clear, I don't think there's any malicious intent here - I'm sure it was just something that was fine when it was easily customisable in the app code and was then moved into the core as part of a clean up, and the forcing of route names was overlooked.

@slashequip
Copy link

It only does this by way of the scaffolding though. If you wrote it yourself why would you throw a core AuthenticationException error? You'd have your own exception and you'd have to handle it on it's own anyway?

If you don't use the Laravel auth scaffolding then you'd never know or come across this, I wouldn't have thought?

@ockle
Copy link
Author

ockle commented Sep 19, 2017

I'm not using the auth scaffolding. The middleware is included in a fresh installation. The make:auth command just adds views, routes and controllers that build on the framework's auth functionality.

Anybody not using the scaffolding, building their own auth flow, but using a differently named login route will run into this.

@slashequip
Copy link

It's registered but, as far as I'm aware, you have to make a conscious effort to apply that to your Routes.

They only run into it if they throw one of cores exceptions, don't they? If they were building their own auth flow why wouldn't they create their own exception to catch?

To me it feels like we are discussing some kind of meta in-between auth logic, like using half Laravel half 'my own magic' and then expecting it to work? Still IMO of course...

@ockle
Copy link
Author

ockle commented Sep 19, 2017

It's registered but, as far as I'm aware, you have to make a conscious effort to apply that to your Routes.

Yes. But using the auth middleware is a core, documented feature. It's not like its an optional thing like the scaffolding. Applying the middleware, either to the route or the controller, is how you are supposed to do auth with Laravel.

They only run into it if they throw one of cores exceptions, don't they?

No, they run into it if they follow the documentation.

Route::get('/login', function () {
    return 'Login screen';
})->name('auth.login');

Route::get('/admin', function () {
    return 'You can view this because you are logged in';
})->middleware('auth');

Try that in a fresh 5.5. You get Route [login] not defined.. Change auth.login to login and it works. The framework has forced the developer into a specific route name.

If they were building their own auth flow why wouldn't they create their own exception to catch?

Because they would have to create their own middleware, which is identical to the core one, except make it throw a different exception, then have to catch it somewhere in order to implement identical behaviour to the core, except with a different redirect location.

The core of the framework handles auth, it is catching the exception and making a decision as to where to redirect the visitor. The developer cannot easily change it. This is the issue here.

@slashequip
Copy link

slashequip commented Sep 19, 2017

Yes. But using the auth middleware is a core, documented feature. It's not like its an optional thing like the scaffolding. Applying the middleware, either to the route or the controller, is how you are supposed to do auth with Laravel.

But that page says at the top to run the scaffold? The scaffold is optional because your app might not need auth, in which case you wouldn't be running with the auth middleware anyway. If someone is going through the docs for this then they have run the scaffold. If, like us, you know better then you can run your own middleware with zero hassle. Again goes back to the feels of some meta logic.

Because they would have to create their own middleware, which is identical to the core one, except make it throw a different exception, then have to catch it somewhere in order to implement identical behaviour to the core, except with a different redirect location.

Personally I'd redirect straight from the middleware rather than throw an exception, but personal preference I guess?

For me if you are not running the scaffold then all you need to do is run your own middleware that redirects to your chosen route (named or not). I don't feel it's an issue here but I'll leave it now for someone else to offer an opinion. We don't see eye to eye on this and we will just be going round in circles.

I wish you a good day sir/ma'am.

Edit: added ma'am - it's 2017 folks!

@ockle
Copy link
Author

ockle commented Sep 19, 2017

But that page says at the top to run the scaffold?

No, it asks you if you want to do it. If you answer no, the framework should not then break when you come to create your routes, and follow your own naming convention.

The scaffold is optional because your app might not need auth

Wrong. The scaffolding is optional because you might not want the views/markup it provides. Even then, if you do use the scaffolding, you can then change the route name. The scaffolding is just that - a quick default that you can optionally customise. It's not a concrete "you have to use this if you want auth and you are not allowed to change anything".

Therefore, pretend the scaffolding does not exist (which we must, as it is optional). Pretend you have to implement your own flow. The framework, as of 5.5, forces a naming convention on you.

in which case you wouldn't be running with the auth middleware anyway

Then why is it in the framework without running make:auth?

Personally I'd redirect straight from the middleware rather than throw an exception, but personal preference I guess?

And that's exactly what Laravel 5.0-5.2 did.

We don't see eye to eye on this and we will just be going round in circles.

No, I would agree :) I think we are both seeing Laravel from slightly different angles.

@kailichiang
Copy link

Hi ockle,

Did you find a solution?
There is a way in 5.5 to customise redirect route for unauthenticated by adding a named route called login to your routes. For example,
Route::get('/auth/login', 'LoginController@showLoginForm')->name('login');

This is not described yet in document. And I found lots of people bump into the problem after upgrading. I believe it's better to add a piece of explanation into Authentication section of document.

@kmcluckie
Copy link

This threads a little old, I know, but it's still open, so I'll submit my vote for not hard coding the route.

Even when using the built in auth, I'd like to redirect somewhere else when unauthenticated. I'd specifically like to redirect to route('home'). I can specify where to redirect when registering and when logging in, but specifying where to redirect when being unauthenticated is just hard with it being in the exception handler like this.

@rs-sliske
Copy link

cant you override that method in app/Exceptions/Handler ?

@kmcluckie
Copy link

Yep, you can, thanks. I hadn't noticed that there was an app-level exception handler down the stack trace a little. I've overridden that method and it works well. Thanks again.

@khaledelmahdi
Copy link

Edit the file app/Exceptions/Handler.php add the following:

use Illuminate\Auth\AuthenticationException;

protected function unauthenticated($request, AuthenticationException $exception)
{
    return $request->expectsJson()
        ? response()->json(['message' => $exception->getMessage()], 401)
        : redirect()->guest(route('ROUTENAME'));
}

Change ROUTENAME to your login route name.

@SlyDave
Copy link

SlyDave commented Feb 12, 2018

Shocking that in even in Laravel 5.6, the route for unauthenticated is still hard coded to 'login', rather than being a .env setting or at least class prop.

It renders reusability of the built in auth exceptions all but impossible without having to also override the unauthenticated method.

Even worse when you factor in that 5.0 has it as 'auth/login' and 5.1+ has it as just 'login' - this makes the upgrade path very messy (having to rename your routes and/or change your urls) just to continue working with 'the Laravel way'.

@emsifa
Copy link

emsifa commented Mar 2, 2018

+1 for class property, or just put unauthenticated method in app/Exceptions/Handler.php, so we can easily modify it without copy-paste code from parent class.

@taylorotwell
Copy link
Member

Someone make a PR to improve it.

@ockle
Copy link
Author

ockle commented Mar 13, 2018

PR: laravel/framework#23529

@yannmichaux
Copy link

yannmichaux commented May 23, 2018

Alternatively, you can set your login route as follows:
Route::get('/customLogin', function() { return view('index');})->name('login');
(look for the name)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants