codeigniter4 / shield

Authentication and Authorization for CodeIgniter 4
https://shield.codeigniter.com
MIT License
351 stars 123 forks source link

Bug: Incorrect route redirection on custom routes. #545

Closed gphg closed 1 year ago

gphg commented 1 year ago

PHP Version

7.4.30

CodeIgniter4 Version

4.2.10

Shield Version

dev-develop

Which operating systems have you tested for this bug?

Windows, Linux

Which server did you use?

cli-server (PHP built-in webserver)

Database

MariaDB 10.4.25

Did you customize Shield?

Yes, I did customize Shield.

Controller classes are: CodeIgniter\Shield\Controllers\LoginController and CodeIgniter\Shield\Controllers\RegisterController. These aren't touched. I simply created file contains extended class of these. There aren't any methods or properties been made yet.

file on app/Controllers/AuthController/Login.php contains:

namespace App\Controllers\AuthController;

use CodeIgniter\Shield\Controllers\LoginController as ShieldLogin;

class Login extends ShieldLogin {}

file on app/Controllers/AuthController/Register.php contains:

namespace App\Controllers\AuthController;

use CodeIgniter\Shield\Controllers\RegisterController as ShieldRegister;

class Register extends ShieldRegister {}

Config classes are: Config\Auth and Config\Routes. file on app/Config/Auth.php has changes:

    public array $redirects = [
        'register' => '/',
        'login'    => '/',
        'logout'   => 'auth/login',
    ];

    public array $views = [
        'login'                       => 'Auth\login',
        'register'                    => 'Auth\register',
        // the rest under this is untouched
    }

file on app/Config/Routes.php has changes:

// Auth System are  grouped under "/auth/"
$routes->group('auth', static function ($routes) {
    $routes->get('logout', 'AuthController\Login::logoutAction');
    $routes->get('login', 'AuthController\Login::loginView');
    $routes->post('login', 'AuthController\Login::loginAction');
    $routes->get('register', 'AuthController\Register::registerView');
    $routes->post('register', 'AuthController\Register::registerAction');
});

service('auth')->routes($routes, ['except' => [
    'logout',
    'login',
    'register',
    'auth/logout',
    'auth/login',
    'auth/register',
    // We don't implement magic-link at the moment
    'login/magic-link',
    'login/verify-magic-link',
]]);

View classes are copies of required views, customized to suit the application routes. These are the origin and the destination:

Changed on these file is focused on url_to function to suit the customized, eg: I changed url_to('login') onto url_to('auth/login') and url_to('register') onto url_to('auth/register').

What happened?

An error occour:

CodeIgniter\HTTP\Exceptions\HTTPException
The route for "login" cannot be found.

Steps to Reproduce

  1. Extends classes as mentioned above.
  2. Register an account by head to /auth/register, input valid for a test.
  3. Assuming logged in, then logout by head to /auth/logout.
  4. Login by head to /auth/login, enter valid credentials.
  5. Assuming correct and no error, then logout again.
  6. Login by head to /auth/login, but this time enter incorrect credentials.

Expected Output

I expected it would redirect to customized login route (auth/login) of config file instead hard coded one (login). This is the cause: https://github.com/codeigniter4/shield/blob/b1bffb4ecbe3d9df2b37c3287e57cca74f94c839/src/Controllers/LoginController.php#L61

Anything else?

I managed to avoid this error by extending the login loginAction() method within all its content, then changed a line, literally. This is the file:

namespace App\Controllers\AuthController;

use CodeIgniter\HTTP\RedirectResponse;
use CodeIgniter\Shield\Authentication\Authenticators\Session;
use CodeIgniter\Shield\Controllers\LoginController as ShieldLogin;

class Login extends ShieldLogin
{
    /**
     * Attempts to log the user in.
     */
    public function loginAction(): RedirectResponse
    {
        // Validate here first, since some things,
        // like the password, can only be validated properly here.
        $rules = $this->getValidationRules();

        if (! $this->validate($rules)) {
            return redirect()->back()->withInput()->with('errors', $this->validator->getErrors());
        }

        $credentials             = $this->request->getPost(setting('Auth.validFields'));
        $credentials             = array_filter($credentials);
        $credentials['password'] = $this->request->getPost('password');
        $remember                = (bool) $this->request->getPost('remember');

        /** @var Session $authenticator */
        $authenticator = auth('session')->getAuthenticator();

        // Attempt to login
        $result = $authenticator->remember($remember)->attempt($credentials);
        if (! $result->isOK()) {
            return redirect()->route('auth/login')->withInput()->with('error', $result->reason());
        }

        // If an action has been defined for login, start it up.
        if ($authenticator->hasAction()) {
            return redirect()->route('auth-action-show')->withCookies();
        }

        return redirect()->to(config('Auth')->loginRedirect())->withCookies();
    }
}

I changed route('login') onto route('auth/login'). I wish it could be customable on config files or setting.

kenjis commented 1 year ago

Try:

    $routes->get('login', 'AuthController\Login::loginView', ['as' => 'login']);
gphg commented 1 year ago

Works fine. I checked php spark routes, and it doesn't seem any changed made on $routes.

https://codeigniter.com/user_guide/incoming/routing.html#redirecting-routes

kenjis commented 1 year ago

vendor/codeigniter4/shield/src/Views/register.php -> app/Views/Auth/register.php Changed on these file is focused on url_to function to suit the customized, eg I changed url_to('login') onto url_to('auth/login') and url_to('register') onto url_to('auth/register').

The argument for url_to() is a named route. So if you define names for the routes, you don't need to change these.

kenjis commented 1 year ago

php spark routes does not show route names now. CodeIgniter v4.3.0 will show route names.

$ php spark routes

CodeIgniter v4.3.0 Command Line Tool - Server Time: 2022-11-26 09:56:50 UTC+00:00

+--------+--------------+------+---------------------------------+----------------+---------------+
| Method | Route        | Name | Handler                         | Before Filters | After Filters |
+--------+--------------+------+---------------------------------+----------------+---------------+
| GET    | /            | »    | \App\Controllers\Home::index    |                | toolbar       |
| GET    | news/create  | »    | \App\Controllers\News::create   |                | toolbar       |
| GET    | news/([^/]+) | »    | \App\Controllers\News::view/$1  |                | toolbar       |
| GET    | news         | »    | \App\Controllers\News::index    |                | toolbar       |
| GET    | pages        | »    | \App\Controllers\Pages::index   |                | toolbar       |
| GET    | (.*)         | »    | \App\Controllers\Pages::view/$1 |                | toolbar       |
| POST   | news/create  | »    | \App\Controllers\News::create   | csrf           | toolbar       |
+--------+--------------+------+---------------------------------+----------------+---------------+
gphg commented 1 year ago

I leave the Config\Auth's $views property as it is, then changed:

    $routes->get('login', 'AuthController\Login::loginView', ['as' => 'login']);
    $routes->get('register', 'AuthController\Register::registerView', ['as' => 'register']);

Edit: It doesn't listed on routes confused me. This is my current routes:

$ php spark routes

CodeIgniter v4.2.10 Command Line Tool - Server Time: 2022-11-26 04:07:13 UTC-06:00

+--------+-------------------------+------------------------------------------------------------------+----------------+---------------+
| Method | Route                   | Handler                                                          | Before Filters | After Filters |
+--------+-------------------------+------------------------------------------------------------------+----------------+---------------+
| GET    | /                       | \App\Controllers\StaticController::index                         |                | toolbar       |
| GET    | auth/logout             | \App\Controllers\AuthController\Login::logoutAction              |                | toolbar       |
| GET    | auth/login              | \App\Controllers\AuthController\Login::loginView                 |                | toolbar       |
| GET    | auth/register           | \App\Controllers\AuthController\Register::registerView           |                | toolbar       |
| GET    | login/magic-link        | \CodeIgniter\Shield\Controllers\MagicLinkController::loginView   |                | toolbar       |
| GET    | login/verify-magic-link | \CodeIgniter\Shield\Controllers\MagicLinkController::verify      |                | toolbar       |
| GET    | auth/a/show             | \CodeIgniter\Shield\Controllers\ActionController::show           |                | toolbar       |
| POST   | auth/login              | \App\Controllers\AuthController\Login::loginAction               |                | toolbar       |
| POST   | auth/register           | \App\Controllers\AuthController\Register::registerAction         |                | toolbar       |
| POST   | login/magic-link        | \CodeIgniter\Shield\Controllers\MagicLinkController::loginAction |                | toolbar       |
| POST   | auth/a/handle           | \CodeIgniter\Shield\Controllers\ActionController::handle         |                | toolbar       |
| POST   | auth/a/verify           | \CodeIgniter\Shield\Controllers\ActionController::verify         |                | toolbar       |
| CLI    | ci(.*)                  | \CodeIgniter\CLI\CommandRunner::index/$1                         |                |               |
+--------+-------------------------+------------------------------------------------------------------+----------------+---------------+
gphg commented 1 year ago

Any plan to make it configurable? Eg, a new property redirect_on_fail or simply redirect to self?

Update: I suggest to replace

            return redirect()->route('login')->withInput()->with('error', $result->reason());

onto

            return redirect()->back()->withInput()->with('error', $result->reason());
kenjis commented 1 year ago

Now login has the name login. See #577