From acfdcf80a1eaf08799d810caea9ebc4b40afa225 Mon Sep 17 00:00:00 2001 From: mscherer Date: Mon, 24 Nov 2025 22:56:48 +0100 Subject: [PATCH 1/2] Fix loadIdentifier called after loadAuthenticator losing resolver config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When loadAuthenticator() was called before loadIdentifier(), the authenticator would receive an empty IdentifierCollection and create its own default Password identifier immediately in the constructor. Later calls to loadIdentifier() would add to the service's identifier collection, but the authenticator already had its own separate collection with the default identifier. This fix changes the default identifier loading from eager (in constructor) to lazy (in authenticate()). This ensures that if loadIdentifier() is called after loadAuthenticator(), the identifier will be loaded into the shared collection before the authenticator tries to use it. Fixes #754 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/Authenticator/CookieAuthenticator.php | 28 ++++----- src/Authenticator/FormAuthenticator.php | 37 ++++++------ src/Authenticator/HttpBasicAuthenticator.php | 25 ++++---- src/Authenticator/JwtAuthenticator.php | 29 ++++++--- src/Authenticator/TokenAuthenticator.php | 21 +++---- tests/TestCase/AuthenticationServiceTest.php | 59 +++++++++++++++++++ .../Authenticator/FormAuthenticatorTest.php | 21 +++++-- 7 files changed, 149 insertions(+), 71 deletions(-) diff --git a/src/Authenticator/CookieAuthenticator.php b/src/Authenticator/CookieAuthenticator.php index 8e2836ef..580aec6b 100644 --- a/src/Authenticator/CookieAuthenticator.php +++ b/src/Authenticator/CookieAuthenticator.php @@ -58,26 +58,23 @@ class CookieAuthenticator extends AbstractAuthenticator implements PersistenceIn ]; /** - * Constructor + * Gets the identifier, loading a default Password identifier if none configured. * - * @param \Authentication\Identifier\IdentifierInterface $identifier Identifier or identifiers collection. - * @param array $config Configuration settings. + * This is done lazily to allow loadIdentifier() to be called after loadAuthenticator(). + * + * @return \Authentication\Identifier\IdentifierInterface */ - public function __construct(IdentifierInterface $identifier, array $config = []) + public function getIdentifier(): IdentifierInterface { - // If no identifier is configured, set up a default Password identifier - if ($identifier instanceof IdentifierCollection && $identifier->isEmpty()) { - // Pass the authenticator's fields configuration to the identifier + if ($this->_identifier instanceof IdentifierCollection && $this->_identifier->isEmpty()) { $identifierConfig = []; - if (isset($config['fields'])) { - $identifierConfig['fields'] = $config['fields']; + if ($this->getConfig('fields')) { + $identifierConfig['fields'] = $this->getConfig('fields'); } - $identifier = new IdentifierCollection([ - 'Authentication.Password' => $identifierConfig, - ]); + $this->_identifier->load('Authentication.Password', $identifierConfig); } - parent::__construct($identifier, $config); + return $this->_identifier; } /** @@ -107,10 +104,11 @@ public function authenticate(ServerRequestInterface $request): ResultInterface [$username, $tokenHash] = $token; - $identity = $this->_identifier->identify(compact('username')); + $identifier = $this->getIdentifier(); + $identity = $identifier->identify(compact('username')); if (!$identity) { - return new Result(null, Result::FAILURE_IDENTITY_NOT_FOUND, $this->_identifier->getErrors()); + return new Result(null, Result::FAILURE_IDENTITY_NOT_FOUND, $identifier->getErrors()); } if (!$this->_checkToken($identity, $tokenHash)) { diff --git a/src/Authenticator/FormAuthenticator.php b/src/Authenticator/FormAuthenticator.php index f784c483..781b4e70 100644 --- a/src/Authenticator/FormAuthenticator.php +++ b/src/Authenticator/FormAuthenticator.php @@ -50,26 +50,23 @@ class FormAuthenticator extends AbstractAuthenticator ]; /** - * Constructor + * Gets the identifier, loading a default Password identifier if none configured. * - * @param \Authentication\Identifier\IdentifierInterface $identifier Identifier or identifiers collection. - * @param array $config Configuration settings. + * This is done lazily to allow loadIdentifier() to be called after loadAuthenticator(). + * + * @return \Authentication\Identifier\IdentifierInterface */ - public function __construct(IdentifierInterface $identifier, array $config = []) + public function getIdentifier(): IdentifierInterface { - // If no identifier is configured, set up a default Password identifier - if ($identifier instanceof IdentifierCollection && $identifier->isEmpty()) { - // Pass the authenticator's fields configuration to the identifier + if ($this->_identifier instanceof IdentifierCollection && $this->_identifier->isEmpty()) { $identifierConfig = []; - if (isset($config['fields'])) { - $identifierConfig['fields'] = $config['fields']; + if ($this->getConfig('fields')) { + $identifierConfig['fields'] = $this->getConfig('fields'); } - $identifier = new IdentifierCollection([ - 'Authentication.Password' => $identifierConfig, - ]); + $this->_identifier->load('Authentication.Password', $identifierConfig); } - parent::__construct($identifier, $config); + return $this->_identifier; } /** @@ -141,9 +138,12 @@ protected function _buildLoginUrlErrorResult(ServerRequestInterface $request): R } /** - * Authenticates the identity contained in a request. Will use the `config.userModel`, and `config.fields` - * to find POST data that is used to find a matching record in the `config.userModel`. Will return false if - * there is no post data, either username or password is missing, or if the scope conditions have not been met. + * Authenticates the identity contained in a request. + * + * Will use the `config.userModel`, and `config.fields` to find POST data + * that is used to find a matching record in the `config.userModel`. + * Will return false if there is no post data, either username or password is missing, + * or if the scope conditions have not been met. * * @param \Psr\Http\Message\ServerRequestInterface $request The request that contains login information. * @return \Authentication\Authenticator\ResultInterface @@ -161,10 +161,11 @@ public function authenticate(ServerRequestInterface $request): ResultInterface ]); } - $user = $this->_identifier->identify($data); + $identifier = $this->getIdentifier(); + $user = $identifier->identify($data); if (!$user) { - return new Result(null, Result::FAILURE_IDENTITY_NOT_FOUND, $this->_identifier->getErrors()); + return new Result(null, Result::FAILURE_IDENTITY_NOT_FOUND, $identifier->getErrors()); } return new Result($user, Result::SUCCESS); diff --git a/src/Authenticator/HttpBasicAuthenticator.php b/src/Authenticator/HttpBasicAuthenticator.php index 715e002f..7d5c2b8d 100644 --- a/src/Authenticator/HttpBasicAuthenticator.php +++ b/src/Authenticator/HttpBasicAuthenticator.php @@ -44,26 +44,23 @@ class HttpBasicAuthenticator extends AbstractAuthenticator implements StatelessI ]; /** - * Constructor + * Gets the identifier, loading a default Password identifier if none configured. * - * @param \Authentication\Identifier\IdentifierInterface $identifier Identifier or identifiers collection. - * @param array $config Configuration settings. + * This is done lazily to allow loadIdentifier() to be called after loadAuthenticator(). + * + * @return \Authentication\Identifier\IdentifierInterface */ - public function __construct(IdentifierInterface $identifier, array $config = []) + public function getIdentifier(): IdentifierInterface { - // If no identifier is configured, set up a default Password identifier - if ($identifier instanceof IdentifierCollection && $identifier->isEmpty()) { - // Pass the authenticator's fields configuration to the identifier + if ($this->_identifier instanceof IdentifierCollection && $this->_identifier->isEmpty()) { $identifierConfig = []; - if (isset($config['fields'])) { - $identifierConfig['fields'] = $config['fields']; + if ($this->getConfig('fields')) { + $identifierConfig['fields'] = $this->getConfig('fields'); } - $identifier = new IdentifierCollection([ - 'Authentication.Password' => $identifierConfig, - ]); + $this->_identifier->load('Authentication.Password', $identifierConfig); } - parent::__construct($identifier, $config); + return $this->_identifier; } /** @@ -83,7 +80,7 @@ public function authenticate(ServerRequestInterface $request): ResultInterface return new Result(null, Result::FAILURE_CREDENTIALS_MISSING); } - $user = $this->_identifier->identify([ + $user = $this->getIdentifier()->identify([ AbstractIdentifier::CREDENTIAL_USERNAME => $username, AbstractIdentifier::CREDENTIAL_PASSWORD => $password, ]); diff --git a/src/Authenticator/JwtAuthenticator.php b/src/Authenticator/JwtAuthenticator.php index ff1acd89..3ce212e3 100644 --- a/src/Authenticator/JwtAuthenticator.php +++ b/src/Authenticator/JwtAuthenticator.php @@ -57,13 +57,7 @@ class JwtAuthenticator extends TokenAuthenticator */ public function __construct(IdentifierInterface $identifier, array $config = []) { - // Override parent's default - JWT should use JwtSubject identifier - if ($identifier instanceof IdentifierCollection && $identifier->isEmpty()) { - $identifier = new IdentifierCollection(['Authentication.JwtSubject']); - } - - // Call TokenAuthenticator's constructor but skip its default - AbstractAuthenticator::__construct($identifier, $config); + parent::__construct($identifier, $config); if (empty($this->_config['secretKey'])) { if (!class_exists(Security::class)) { @@ -73,6 +67,22 @@ public function __construct(IdentifierInterface $identifier, array $config = []) } } + /** + * Gets the identifier, loading a default JwtSubject identifier if none configured. + * + * This is done lazily to allow loadIdentifier() to be called after loadAuthenticator(). + * + * @return \Authentication\Identifier\IdentifierInterface + */ + public function getIdentifier(): IdentifierInterface + { + if ($this->_identifier instanceof IdentifierCollection && $this->_identifier->isEmpty()) { + $this->_identifier->load('Authentication.JwtSubject'); + } + + return $this->_identifier; + } + /** * Authenticates the identity based on a JWT token contained in a request. * @@ -113,12 +123,13 @@ public function authenticate(ServerRequestInterface $request): ResultInterface return new Result($user, Result::SUCCESS); } - $user = $this->_identifier->identify([ + $identifier = $this->getIdentifier(); + $user = $identifier->identify([ $subjectKey => $result[$subjectKey], ]); if (!$user) { - return new Result(null, Result::FAILURE_IDENTITY_NOT_FOUND, $this->_identifier->getErrors()); + return new Result(null, Result::FAILURE_IDENTITY_NOT_FOUND, $identifier->getErrors()); } return new Result($user, Result::SUCCESS); diff --git a/src/Authenticator/TokenAuthenticator.php b/src/Authenticator/TokenAuthenticator.php index d02e26b5..3751ac82 100644 --- a/src/Authenticator/TokenAuthenticator.php +++ b/src/Authenticator/TokenAuthenticator.php @@ -38,19 +38,19 @@ class TokenAuthenticator extends AbstractAuthenticator implements StatelessInter ]; /** - * Constructor + * Gets the identifier, loading a default Token identifier if none configured. * - * @param \Authentication\Identifier\IdentifierInterface $identifier Identifier or identifiers collection. - * @param array $config Configuration settings. + * This is done lazily to allow loadIdentifier() to be called after loadAuthenticator(). + * + * @return \Authentication\Identifier\IdentifierInterface */ - public function __construct(IdentifierInterface $identifier, array $config = []) + public function getIdentifier(): IdentifierInterface { - // If no identifier is configured, set up a default Token identifier - if ($identifier instanceof IdentifierCollection && $identifier->isEmpty()) { - $identifier = new IdentifierCollection(['Authentication.Token']); + if ($this->_identifier instanceof IdentifierCollection && $this->_identifier->isEmpty()) { + $this->_identifier->load('Authentication.Token'); } - parent::__construct($identifier, $config); + return $this->_identifier; } /** @@ -142,12 +142,13 @@ public function authenticate(ServerRequestInterface $request): ResultInterface return new Result(null, Result::FAILURE_CREDENTIALS_MISSING); } - $user = $this->_identifier->identify([ + $identifier = $this->getIdentifier(); + $user = $identifier->identify([ TokenIdentifier::CREDENTIAL_TOKEN => $token, ]); if (!$user) { - return new Result(null, Result::FAILURE_IDENTITY_NOT_FOUND, $this->_identifier->getErrors()); + return new Result(null, Result::FAILURE_IDENTITY_NOT_FOUND, $identifier->getErrors()); } return new Result($user, Result::SUCCESS); diff --git a/tests/TestCase/AuthenticationServiceTest.php b/tests/TestCase/AuthenticationServiceTest.php index d2e13d50..5b7de47d 100644 --- a/tests/TestCase/AuthenticationServiceTest.php +++ b/tests/TestCase/AuthenticationServiceTest.php @@ -1373,4 +1373,63 @@ public function testFormAuthenticatorDefaultIdentifier() $authenticator = $service->getAuthenticationProvider(); $this->assertInstanceOf(FormAuthenticator::class, $authenticator); } + + /** + * Test that loadIdentifier called after loadAuthenticator still works. + * + * This is a regression test for https://github.com/cakephp/authentication/issues/754 + * When loadAuthenticator was called before loadIdentifier, the authenticator would + * create its own default identifier collection and ignore the later loadIdentifier call. + * + * @deprecated Note that this test will be removed in 4.x as this is only to keep BC in 3.x. + * + * @return void + */ + public function testLoadIdentifierAfterLoadAuthenticator() + { + $this->skipIf( + version_compare(Version::id(), '11.0', '<'), + 'For some reason PHPUnit doesn\'t pick up the deprecation on v10', + ); + + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_URI' => '/testpath'], + [], + ['username' => 'mariano', 'password' => 'password'], + ); + + $service = new AuthenticationService(); + + // Load authenticator FIRST + $service->loadAuthenticator('Authentication.Form'); + + // Then load identifier with custom resolver config + $this->deprecated(function () use ($service) { + $service->loadIdentifier('Authentication.Password', [ + 'resolver' => [ + 'className' => 'Authentication.Orm', + 'userModel' => 'AuthUsers', + 'finder' => 'auth', + ], + ]); + }); + + // The authenticator should use the identifier we loaded, not its default + $formAuth = $service->authenticators()->get('Form'); + $identifierCollection = $formAuth->getIdentifier(); + $this->assertInstanceOf(IdentifierCollection::class, $identifierCollection); + + $passwordIdentifier = $identifierCollection->get('Password'); + $this->assertInstanceOf(PasswordIdentifier::class, $passwordIdentifier); + + // Verify the resolver config was applied + $resolverConfig = $passwordIdentifier->getConfig('resolver'); + $this->assertIsArray($resolverConfig); + $this->assertSame('AuthUsers', $resolverConfig['userModel']); + $this->assertSame('auth', $resolverConfig['finder']); + + // Verify authentication actually works with the custom config + $result = $service->authenticate($request); + $this->assertTrue($result->isValid()); + } } diff --git a/tests/TestCase/Authenticator/FormAuthenticatorTest.php b/tests/TestCase/Authenticator/FormAuthenticatorTest.php index 13b72414..1c00a414 100644 --- a/tests/TestCase/Authenticator/FormAuthenticatorTest.php +++ b/tests/TestCase/Authenticator/FormAuthenticatorTest.php @@ -641,25 +641,36 @@ public function testDefaultIdentifierInheritsFieldsConfig() $identifiers = new IdentifierCollection(); // Configure authenticator with custom fields mapping + // Also set a loginUrl that won't match, so authenticate() returns early + // without actually trying to identify (which would require database access) $config = [ 'fields' => [ 'username' => 'user_name', 'password' => 'pass_word', ], + 'loginUrl' => '/login', ]; // FormAuthenticator should create default identifier with inherited fields + // The default identifier is loaded lazily when authenticate() is called $form = new FormAuthenticator($identifiers, $config); + // Trigger the lazy loading by calling authenticate on a non-matching URL + $request = ServerRequestFactory::fromGlobals( + ['REQUEST_URI' => '/testpath'], + [], + ['user_name' => 'mariano', 'pass_word' => 'password'], + ); + $form->authenticate($request); + // Verify the identifier was created with the correct configuration $identifier = $form->getIdentifier(); $this->assertInstanceOf(IdentifierCollection::class, $identifier); $this->assertFalse($identifier->isEmpty()); - // Verify the fields are properly configured - // We can't directly access the internal configuration, but we can verify - // the FormAuthenticator has the expected configuration - $this->assertEquals('user_name', $form->getConfig('fields.username')); - $this->assertEquals('pass_word', $form->getConfig('fields.password')); + // Verify the fields are properly configured on the identifier + $passwordIdentifier = $identifier->get('Password'); + $this->assertEquals('user_name', $passwordIdentifier->getConfig('fields.username')); + $this->assertEquals('pass_word', $passwordIdentifier->getConfig('fields.password')); } } From b40d677cf3dab49bf9b1e963af6bb2b8017b4613 Mon Sep 17 00:00:00 2001 From: mscherer Date: Tue, 25 Nov 2025 12:45:09 +0100 Subject: [PATCH 2/2] Fix CS. --- tests/TestCase/AuthenticationServiceTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/TestCase/AuthenticationServiceTest.php b/tests/TestCase/AuthenticationServiceTest.php index 5b7de47d..b6e9713e 100644 --- a/tests/TestCase/AuthenticationServiceTest.php +++ b/tests/TestCase/AuthenticationServiceTest.php @@ -1382,7 +1382,6 @@ public function testFormAuthenticatorDefaultIdentifier() * create its own default identifier collection and ignore the later loadIdentifier call. * * @deprecated Note that this test will be removed in 4.x as this is only to keep BC in 3.x. - * * @return void */ public function testLoadIdentifierAfterLoadAuthenticator()