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..b6e9713e 100644 --- a/tests/TestCase/AuthenticationServiceTest.php +++ b/tests/TestCase/AuthenticationServiceTest.php @@ -1373,4 +1373,62 @@ 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')); } }