codeigniter4 / shield

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

Bug: ValidationRules strong_password is not working when called using tokens in before filters API routes. #438

Closed arif-rh closed 1 year ago

arif-rh commented 1 year ago

PHP Version

8.1.0

CodeIgniter4 Version

4.2.6

Shield Version

dev-develop

Which operating systems have you tested for this bug?

Windows

Which server did you use?

apache

Database

MySQL 8.0.27

Did you customize Shield?

No

What happened?

I develop API for registration. To secure an API, I implement tokens on before-filters. When calling registration API, I sent my developer tokens in the Authorization header.

But when I post password contains personal information, it can be passed. strong_password valdiation is not working.

Steps to Reproduce

  1. make simple API for registration
  2. implements tokens on before-filter to secure the API routes
  3. Set the token in the Authorization header
  4. post any password contains personal information

Expected Output

Api should return error that "Passwords cannot contain re-hashed personal information."

Anything else?

I am new in this library, so this report could be false, maybe I do wrong setup or implementation of this library. But for now, I am thinking, the cause could be this line:

    if (function_exists('auth') && auth()->user()) {  
            $user = auth()->user();
    }

when I change those line to:

    if (function_exists('auth') && auth('session')->user()) {  
            $user = auth('session')->user();
    }

then strong_password validation is working as expected.

kenjis commented 1 year ago

Thank you for reporting.

But your Steps to Reproduce is too abstract. Can you show us minimum concrete code to reproduce?

I don't understand why you need to change to auth('session'). If we change like that, it seems strong_password cannot be used in other Authenticators.

arif-rh commented 1 year ago

Steps to Reproduce

  1. make simple API for registration
namespace App\Controllers;

use CodeIgniter\RESTful\ResourceController;
use CodeIgniter\Shield\Entities\User;
use CodeIgniter\Shield\Exceptions\ValidationException;
use CodeIgniter\Shield\Models\UserModel;

class Users extends ResourceController
{

    public function signup()
    {
        $users = $this->getUserProvider();

        $rules = $this->getValidationRules();

        if (! $this->validate($rules)) {
            return $this->respond(['errors' => $this->validator->getErrors()]);
        }

    // the rest of code, try to save the registration when validation is OK

note:

getUserProvider and getValidationRules are same function which used on original shield register controller.

  1. implements tokens on before-filter to secure the API routes

Set the API Route in Config/Routes.php

$routes->post('users/signup', 'Users::signup');

Set the Filter in Config/Filters.php

public $filters = [
        'tokens' => [
            'before' => [
                'users/signup', 
            ],
        ],
       // the rest of filters
  1. Set the token in the Authorization header

I am using postman to test this API, add my developer token to the Authorization Header

image

  1. post any password contains personal information

this is the body I post to the API

image

as you can see on the capture, my password arman#1234 contains personal information, both from username and email. But Api does not return the validation error.

When I try to find out, I figure out, that when implementing token in before-filter, the auth()->user() will return user object based on the token passed in the Authorization Header,

this code below

     if (function_exists('auth') && auth()->user()) {
        $user = auth()->user();
    } else {
        $user = empty($data) ? $this->buildUserFromRequest() : $this->buildUserFromData($data);
    }

will return $user as object from my developer account (since I am using developer tokens to connect to the API). Thus, validation does not compare the password from posted data, instead of $user object.

Code above, will only gather $user from post request, using buildUserFromRequest when auth()->user() is false. But since the header has Authorization token, and before filter proceed the task, so auth()->user() will always return an $user object.

As I said previously, my report could be false.

kenjis commented 1 year ago

Thank you for the detail.

In this case, it seems you are creating another user account.

Code above, will only gather $user from post request, using buildUserFromRequest when auth()->user() is false. But since the header has Authorization token, and before filter proceed the task, so auth()->user() will always return an $user object.

In your use case, the logic seems to be wrong. The authenticated user (the return value of auth()->user()) is not the user to be registered. It seems we need to reconsider the logic completely.

arif-rh commented 1 year ago

The authenticated user (the return value of auth()->user()) is not the user to be registered.

Yes, that exactly what I mean.

As I understand, NothingPersonalValidator check the password compare to the user object. So this user should always be the user to be registered, not the authenticated user (using tokens or session).

Do you think, it is better remove completely these lines?

     if (function_exists('auth') && auth()->user()) {
            $user = auth()->user();
    }

and just use

    $user = empty($data) ? $this->buildUserFromRequest() : $this->buildUserFromData($data);

I am new on shield so I have a big worry when removing it, because I could be missundertanding the shield logic here.

kenjis commented 1 year ago

I think this is not a bug. See https://github.com/codeigniter4/shield/pull/439#discussion_r974787536

arif-rh commented 1 year ago

I think this is still a bug. Because strong_password still failed when checking password on registration, if the registration implement tokens in before filter.

See detail on #439 (comment

kenjis commented 1 year ago

If you use tokens in before filter, the user will be authenticated with the token. That is, an authenticated user is trying to set another user's password. It is not a good practice.

arif-rh commented 1 year ago

I will close since this is considered as not an issue.

kenjis commented 1 year ago

I sent a PR to add note for strong_password #449