Fix: Unnamed Auth routes returns an error when inside a route group

[sammyskills]

In situations when a developer decides to put the routes in a group like so:

$routes->group('accounts', static function($routes) {
    service('auth')->routes($routes);
});

Calls to route_to('login') and route_to('register') will return an error because there isn't any named route for that.

By explicitly setting named routes for each of the auth routes, the routing works well whether in groups or not.

[lonnieezell]

I think this should be fine. @codeigniter4/core-team can you think of any reason this might be problematic?

[MGatner]

Looks good to me! Those should've been named routes to begin with.

[kenjis]

Looks fine.

[datamweb]

Hi, thanks for PR.
If you use the following code.

$routes->group('accounts', static function($routes) {
    service('auth')->routes($routes);
});

The logout path is not recognized because it is set to login by default.

shield/src/Config/Auth.php

Line 51 in 3fede09

'logout' => 'login',

So I expect you to consider this or at least add some explanation in the documentation.

[sammyskills]

Not sure I understand clearly what you mean.

What you pointed out is the logout redirect, which by default, redirects to the login page. This PR fixes issues that may arise when the auth route is placed inside a group.

If we are also considering the login/register/logout redirects, then I think using route_to() should fix that.

Is this what you mean @datamweb ?

[datamweb]

what you mean

Step1: add

$routes->group('accounts', static function($routes) {
    service('auth')->routes($routes);
});

to file app/Config/Routes.php.

Step2: login with http://localhost:8080/accounts/login.
Step3: is to logout with route http://localhost:8080/accounts/logout.

what will happen? 404 error(404 - File Not Found
Can't find a route for 'get: login'.). Because by default the after logout path is set to http://localhost:8080/login NOT http://localhost:8080/accounts/login.

Now if you change the 'logout' => 'accounts/login'. It will not be a problem.

shield/src/Config/Auth.php

Line 51 in 3fede09

'logout' => 'login',

So my point is that it should be explained that the grouping affects the exit performance of the user should know how to fix this issue or use as.

[kenjis]

As @datamweb says, it seems 404 error will be reported.
The setting is URL (path), not a route name.

shield/src/Config/Auth.php

Lines 42 to 52 in 3fede09

	* Redirect urLs
	* --------------------------------------------------------------------
	* The default URL that a user will be redirected to after
	* various auth actions. If you need more flexibility you can
	* override the `getUrl()` method to apply any logic you may need.
	*/
	public array $redirects = [
	'register' => '/',
	'login' => '/',
	'logout' => 'login',
	];

But the issue is not related to this PR?
If a dev changes the URL of login, it is no wonder s/he needs to change the setting.

[datamweb]

The setting is URL (path), not a route name.

Yes, exactly that.

But the issue is not related to this PR?

Well, if you think this is not related to PR, go ahead. merge.

[sammyskills]

Now I understand your concern @datamweb.

Can we change the $redirects setting to a route name (at least for the logout) instead of just a path? I think this will fix the issue for both the grouped and ungrouped cases.

What do you think @kenjis @MGatner @lonnieezell ?

[kenjis]

Can we change the $redirects setting to a route name (at least for the logout) instead of just a path? I think this will fix the issue for both the grouped and ungrouped cases.

We cannot change it easily. Because it is a URL (path), not a route name.
If we change it, it is a breaking change.

The value will be passed to site_url():

shield/src/Config/Auth.php

Lines 375 to 380 in 3fede09

	protected function getUrl(string $url): string
	{
	return strpos($url, 'http') === 0
	? $url
	: rtrim(site_url($url), '/ ');
	}

[kenjis]

Shield is now in beta releases. So we could introduce breaking changes.

When we are using named routes in Shield,
it is better to use named routes in all places including the $redirects values.

So it seems better to change the $redirects values to:

1. a URL if it begins with http. E.g. http://example.com/logout
2. a named route if there is the named route. E.g. logout
3. a route (URI path). E.g. logout

We can do that if we change getUrl(), and almost all existing system won't break even if it is not updated.

[kenjis]

can you think of any reason this might be problematic?

If a site has more than one login URL (e.g. login and admin/login) and
the admin/login has the route name login, this PR will break the system.
The same goes with logout and register.

But there is probably no such site, so this change will not a problem.

[datamweb]

If a site has more than one login URL (e.g. login and admin/login) and the admin/login has the route name login, this PR will break the system.

That was interesting. I had not thought about this position.
This is an example from Joomla. Admin and user login section.

But there is probably no such site, so this change will not a problem.

However, I agree with this.

[datamweb]

Thank you for being with us.

We all strive to make Shield better.

[datamweb]

LGTM!

[kenjis]

Sorry, I meant please update the explanation. Because it will be not only URL, but also named route.

shield/src/Config/Auth.php

Lines 41 to 46 in 83dd09a

	* --------------------------------------------------------------------
	* Redirect URLs
	* --------------------------------------------------------------------
	* The default URL that a user will be redirected to after
	* various auth actions. If you need more flexibility you can
	* override the `getUrl()` method to apply any logic you may need.

[sammyskills]

Is this really necessary?

I'm asking because no change was done to the $redirects.

[kenjis]

while a logout action will redirect to /login.
https://github.com/codeigniter4/shield/blob/develop/docs/customization.md#custom-redirect-urls

This description also needs to be updated.

[sammyskills]

Oh, I get the point now, thank you.

[kenjis]

Is this really necessary?

In the default state, the route login's path is login . There is no difference between the route name and path.
However, the meaning of the value login is the route name.

So it would be better to have an explanation that if the route name is set, it is the route name.

[kenjis]

LGTM!

[datamweb]

Awaiting approval from lonnieezell.

[kenjis]

@lonnieezell Can you review?
If you don't have time, we will merge this.

[lonnieezell]

Approved