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.

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

SessionAuthenticator 'identify' attribute does not work #683

Open
marcdebus opened this issue Nov 25, 2024 · 6 comments
Open

SessionAuthenticator 'identify' attribute does not work #683

marcdebus opened this issue Nov 25, 2024 · 6 comments

Comments

@marcdebus
Copy link

I would like to check the logged-in user with every request to see if their email address or password has changed. If a user changes their password, every existing session should also be terminated.

This should apparently be possible to solve with identify => true in the SessionAuthenticator. However, if I enable this, the session is always terminated because, in the following lines, the hashed password from the session is hashed again and compared with the hashed password from the database, which results in a FAILURE_CREDENTIALS_INVALID result. Please also see attached file.

SessionAuthenticator.php :

        if ($this->getConfig('identify') === true) {
            $credentials = [];
            foreach ($this->getConfig('fields') as $key => $field) {![Image](https://github.com/user-attachments/assets/154a9b8b-930c-475f-8f66-bf77947e5d22)

                $credentials[$key] = $user[$field];
            }
            $user = $this->_identifier->identify($credentials);

            if (empty($user)) {
                return new Result(null, Result::FAILURE_CREDENTIALS_INVALID);
            }
        }

The login remains unsuccessful if I try to log in without cookies.

Here is my configuration:

            $authenticationService->loadAuthenticator('Authentication.Session', [
                // identify option reads current user information from database and put it into request's identity attribute, not refresh's the session!
                'identify' => true,
                'fields' => [
                    AbstractIdentifier::CREDENTIAL_USERNAME => 'email',
                    AbstractIdentifier::CREDENTIAL_PASSWORD => 'password',
                ],
            ]);


            // Configure form data check to pick email and password
            $authenticationService->loadAuthenticator('Authentication.Form', [
                'fields' => [
                    AbstractIdentifier::CREDENTIAL_USERNAME => 'email',
                    AbstractIdentifier::CREDENTIAL_PASSWORD => 'password',
                ],
                'loginUrl' => '/users/login',
            ]);

            // If the user is on the login page, check for a cookie as well
            $authenticationService->loadAuthenticator('Authentication.Cookie', [
                'fields' => [
                    AbstractIdentifier::CREDENTIAL_USERNAME => 'email',
                    AbstractIdentifier::CREDENTIAL_PASSWORD => 'password',
                ],
                'cookie' => [
                    'name' => 'CookieAuth',
                    'domain' => COOKIE_DOMAIN,
                    'expires' => (new DateTime())->addDays(30),
                ],
                'loginUrl' => '/users/login',
            ]);


            // Load identifiers, ensure we check username and password fields
            $authenticationService->loadIdentifier('Authentication.Password', [
                'resolver' => [
                    'className' => 'Authentication.Orm',
                    'finder' => 'authenticatedUser'
                ],
                'fields' => [
                    AbstractIdentifier::CREDENTIAL_USERNAME => 'email',
                    AbstractIdentifier::CREDENTIAL_PASSWORD => 'password',
                ],
                'passwordHasher' => [
                    'className' => 'Authentication.Fallback',
                    'hashers' => [
                        'Authentication.Default',
                        [
                            'className' => 'Authentication.Legacy',
                            'hashType' => 'md5',
                            'salt' => false // turn off default usage of salt
                        ],
                    ]
                ]
            ]);
@marcdebus
Copy link
Author

Image

@markstory
Copy link
Member

markstory commented Nov 26, 2024

SessionAuthenticator + identify should usually only have id field as the username field. Involving the password in session authentication is redundant and hashing causes problems.

Your session authenticator configuration should look like:

$authenticationService->loadAuthenticator('Authentication.Session', [
    'identify' => true,
    'fields' => [
          AbstractIdentifier::CREDENTIAL_USERNAME => 'id',
    ],
]);

@marcdebus
Copy link
Author

Okay, thank you. I will check that.

@marcdebus
Copy link
Author

marcdebus commented Nov 28, 2024

So I get another exception in these lines:

    public function identify(array $credentials): ArrayAccess|array|null
    {
        if (!isset($credentials[self::CREDENTIAL_USERNAME])) {
            return null;
        }

        $identity = $this->_findIdentity($credentials[self::CREDENTIAL_USERNAME]);
        if (array_key_exists(self::CREDENTIAL_PASSWORD, $credentials)) {
            $password = $credentials[self::CREDENTIAL_PASSWORD];
            if (!$this->_checkPassword($identity, $password)) {
                return null;
            }
        }

        return $identity;
    }

Authentication\Identifier\PasswordIdentifier::_findIdentity(): Argument #1 ($identifier) must be of type string, int given, called in /workspace/vendor/cakephp/authentication/src/Identifier/PasswordIdentifier.php on line 100

Here is the documentation:

identify: Set this key with a value of bool true to enable checking the session credentials against the identifiers. When true, the configured Identifiers are used to identify the user using data stored in the session on each request. Default value is false.

fields: Allows you to map the username field to the unique identifier in your user storage. Defaults to username. This option is used when the identify option is set to true.

So maybe it has to be this configuration:

$authenticationService->loadAuthenticator('Authentication.Session', [
    'identify' => true,
    'fields' => [
          AbstractIdentifier::CREDENTIAL_USERNAME => 'email',
    ],
]);

@marcdebus
Copy link
Author

With that configuration, the login is possible, and it checks the session's email against the user's email from the database.
So, when the user changes their email address, all open sessions are closed.

But what about the password? I need a mechanism to close all open sessions when the password is changed by the user.

Maybe I need to implement that myself?

@markstory
Copy link
Member

So maybe it has to be this configuration:

Yes, you're right. I forgot about the additional type constraints. Using the field you use for unique usernames sounds right.

Maybe I need to implement that myself?

Yes. If you have a solution that could work generally please consider opening a pull request or issue as I think this would be a useful addition to the authentication plugin.

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

No branches or pull requests

2 participants