From 7a5e1e15b35ff5c51dd6a934d53a74bb027ecc5f Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Thu, 28 Mar 2024 11:22:46 -0300 Subject: [PATCH 01/17] Failed password attempts model (initial) --- ...328135459_CreateFailedPasswordAttempts.php | 35 ++++++++ src/Model/Entity/FailedPasswordAttempt.php | 33 +++++++ .../Table/FailedPasswordAttemptsTable.php | 85 +++++++++++++++++++ .../Fixture/FailedPasswordAttemptsFixture.php | 23 +++++ .../Table/FailedPasswordAttemptsTableTest.php | 76 +++++++++++++++++ tests/schema.php | 16 ++++ 6 files changed, 268 insertions(+) create mode 100644 config/Migrations/20240328135459_CreateFailedPasswordAttempts.php create mode 100644 src/Model/Entity/FailedPasswordAttempt.php create mode 100644 src/Model/Table/FailedPasswordAttemptsTable.php create mode 100644 tests/Fixture/FailedPasswordAttemptsFixture.php create mode 100644 tests/TestCase/Model/Table/FailedPasswordAttemptsTableTest.php diff --git a/config/Migrations/20240328135459_CreateFailedPasswordAttempts.php b/config/Migrations/20240328135459_CreateFailedPasswordAttempts.php new file mode 100644 index 000000000..d6fe743c0 --- /dev/null +++ b/config/Migrations/20240328135459_CreateFailedPasswordAttempts.php @@ -0,0 +1,35 @@ +table('failed_password_attempts', ['id' => false, 'primary_key' => ['id']]); + $table->addColumn('id', 'uuid', [ + 'null' => false, + ]); + $table->addColumn('user_id', 'uuid', [ + 'default' => null, + 'null' => false, + ]); + $table->addColumn('created', 'datetime', [ + 'default' => null, + 'null' => false, + ]); + $table->addForeignKey('user_id', 'users', 'id', [ + 'delete' => 'CASCADE', + 'update' => 'CASCADE', + ]); + $table->create(); + } +} diff --git a/src/Model/Entity/FailedPasswordAttempt.php b/src/Model/Entity/FailedPasswordAttempt.php new file mode 100644 index 000000000..437f060bc --- /dev/null +++ b/src/Model/Entity/FailedPasswordAttempt.php @@ -0,0 +1,33 @@ + + */ + protected array $_accessible = [ + 'user_id' => true, + 'created' => true, + 'user' => true, + ]; +} diff --git a/src/Model/Table/FailedPasswordAttemptsTable.php b/src/Model/Table/FailedPasswordAttemptsTable.php new file mode 100644 index 000000000..e097ea856 --- /dev/null +++ b/src/Model/Table/FailedPasswordAttemptsTable.php @@ -0,0 +1,85 @@ + newEntities(array $data, array $options = []) + * @method \CakeDC\Users\Model\Entity\FailedPasswordAttempt get(mixed $primaryKey, array|string $finder = 'all', \Psr\SimpleCache\CacheInterface|string|null $cache = null, \Closure|string|null $cacheKey = null, mixed ...$args) + * @method \CakeDC\Users\Model\Entity\FailedPasswordAttempt findOrCreate($search, ?callable $callback = null, array $options = []) + * @method \CakeDC\Users\Model\Entity\FailedPasswordAttempt patchEntity(\Cake\Datasource\EntityInterface $entity, array $data, array $options = []) + * @method array<\CakeDC\Users\Model\Entity\FailedPasswordAttempt> patchEntities(iterable $entities, array $data, array $options = []) + * @method \CakeDC\Users\Model\Entity\FailedPasswordAttempt|false save(\Cake\Datasource\EntityInterface $entity, array $options = []) + * @method \CakeDC\Users\Model\Entity\FailedPasswordAttempt saveOrFail(\Cake\Datasource\EntityInterface $entity, array $options = []) + * @method iterable<\CakeDC\Users\Model\Entity\FailedPasswordAttempt>|\Cake\Datasource\ResultSetInterface<\CakeDC\Users\Model\Entity\FailedPasswordAttempt>|false saveMany(iterable $entities, array $options = []) + * @method iterable<\CakeDC\Users\Model\Entity\FailedPasswordAttempt>|\Cake\Datasource\ResultSetInterface<\CakeDC\Users\Model\Entity\FailedPasswordAttempt> saveManyOrFail(iterable $entities, array $options = []) + * @method iterable<\CakeDC\Users\Model\Entity\FailedPasswordAttempt>|\Cake\Datasource\ResultSetInterface<\CakeDC\Users\Model\Entity\FailedPasswordAttempt>|false deleteMany(iterable $entities, array $options = []) + * @method iterable<\CakeDC\Users\Model\Entity\FailedPasswordAttempt>|\Cake\Datasource\ResultSetInterface<\CakeDC\Users\Model\Entity\FailedPasswordAttempt> deleteManyOrFail(iterable $entities, array $options = []) + * + * @mixin \Cake\ORM\Behavior\TimestampBehavior + */ +class FailedPasswordAttemptsTable extends Table +{ + /** + * Initialize method + * + * @param array $config The configuration for the Table. + * @return void + */ + public function initialize(array $config): void + { + parent::initialize($config); + + $this->setTable('failed_password_attempts'); + $this->setDisplayField('id'); + $this->setPrimaryKey('id'); + + $this->addBehavior('Timestamp'); + + $this->belongsTo('Users', [ + 'foreignKey' => 'user_id', + 'joinType' => 'INNER', + 'className' => 'CakeDC/Users.Users', + ]); + } + + /** + * Default validation rules. + * + * @param \Cake\Validation\Validator $validator Validator instance. + * @return \Cake\Validation\Validator + */ + public function validationDefault(Validator $validator): Validator + { + $validator + ->uuid('user_id') + ->notEmptyString('user_id'); + + return $validator; + } + + /** + * Returns a rules checker object that will be used for validating + * application integrity. + * + * @param \Cake\ORM\RulesChecker $rules The rules object to be modified. + * @return \Cake\ORM\RulesChecker + */ + public function buildRules(RulesChecker $rules): RulesChecker + { + $rules->add($rules->existsIn(['user_id'], 'Users'), ['errorField' => 'user_id']); + + return $rules; + } +} diff --git a/tests/Fixture/FailedPasswordAttemptsFixture.php b/tests/Fixture/FailedPasswordAttemptsFixture.php new file mode 100644 index 000000000..03941cbc5 --- /dev/null +++ b/tests/Fixture/FailedPasswordAttemptsFixture.php @@ -0,0 +1,23 @@ +records = []; + parent::init(); + } +} diff --git a/tests/TestCase/Model/Table/FailedPasswordAttemptsTableTest.php b/tests/TestCase/Model/Table/FailedPasswordAttemptsTableTest.php new file mode 100644 index 000000000..6e985a091 --- /dev/null +++ b/tests/TestCase/Model/Table/FailedPasswordAttemptsTableTest.php @@ -0,0 +1,76 @@ + + */ + protected array $fixtures = [ + 'plugin.CakeDC/Users.FailedPasswordAttempts', + 'plugin.CakeDC/Users.Users', + ]; + + /** + * setUp method + * + * @return void + */ + protected function setUp(): void + { + parent::setUp(); + $config = $this->getTableLocator()->exists('FailedPasswordAttempts') ? [] : ['className' => FailedPasswordAttemptsTable::class]; + $this->FailedPasswordAttempts = $this->getTableLocator()->get('FailedPasswordAttempts', $config); + } + + /** + * tearDown method + * + * @return void + */ + protected function tearDown(): void + { + unset($this->FailedPasswordAttempts); + + parent::tearDown(); + } + + /** + * Test validationDefault method + * + * @return void + * @uses \CakeDC\Users\Model\Table\FailedPasswordAttemptsTable::validationDefault() + */ + public function testValidationDefault(): void + { + $this->markTestIncomplete('Not implemented yet.'); + } + + /** + * Test buildRules method + * + * @return void + * @uses \CakeDC\Users\Model\Table\FailedPasswordAttemptsTable::buildRules() + */ + public function testBuildRules(): void + { + $this->markTestIncomplete('Not implemented yet.'); + } +} diff --git a/tests/schema.php b/tests/schema.php index 18db1b7c5..ac92fb123 100644 --- a/tests/schema.php +++ b/tests/schema.php @@ -92,4 +92,20 @@ 'primary' => ['type' => 'primary', 'columns' => ['id'], 'length' => []], ], ], + [ + 'table' => 'failed_password_attempts', + 'columns' => [ + 'id' => ['type' => 'uuid', 'length' => null, 'null' => false, 'default' => null, 'comment' => '', 'precision' => null], + 'user_id' => ['type' => 'uuid', 'length' => null, 'null' => false, 'default' => null, 'comment' => '', 'precision' => null, 'fixed' => null], + 'created' => ['type' => 'datetime', 'length' => null, 'null' => false, 'default' => null, 'comment' => '', 'precision' => null], + ], + 'constraints' => [ + 'primary' => [ + 'type' => 'primary', + 'columns' => [ + 'id', + ], + ], + ], + ], ]; From 448f25c46435e85b7f6d4a6046cd52ee4a517ba1 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Thu, 28 Mar 2024 11:36:05 -0300 Subject: [PATCH 02/17] cleanup --- .../Table/FailedPasswordAttemptsTableTest.php | 76 ------------------- 1 file changed, 76 deletions(-) delete mode 100644 tests/TestCase/Model/Table/FailedPasswordAttemptsTableTest.php diff --git a/tests/TestCase/Model/Table/FailedPasswordAttemptsTableTest.php b/tests/TestCase/Model/Table/FailedPasswordAttemptsTableTest.php deleted file mode 100644 index 6e985a091..000000000 --- a/tests/TestCase/Model/Table/FailedPasswordAttemptsTableTest.php +++ /dev/null @@ -1,76 +0,0 @@ - - */ - protected array $fixtures = [ - 'plugin.CakeDC/Users.FailedPasswordAttempts', - 'plugin.CakeDC/Users.Users', - ]; - - /** - * setUp method - * - * @return void - */ - protected function setUp(): void - { - parent::setUp(); - $config = $this->getTableLocator()->exists('FailedPasswordAttempts') ? [] : ['className' => FailedPasswordAttemptsTable::class]; - $this->FailedPasswordAttempts = $this->getTableLocator()->get('FailedPasswordAttempts', $config); - } - - /** - * tearDown method - * - * @return void - */ - protected function tearDown(): void - { - unset($this->FailedPasswordAttempts); - - parent::tearDown(); - } - - /** - * Test validationDefault method - * - * @return void - * @uses \CakeDC\Users\Model\Table\FailedPasswordAttemptsTable::validationDefault() - */ - public function testValidationDefault(): void - { - $this->markTestIncomplete('Not implemented yet.'); - } - - /** - * Test buildRules method - * - * @return void - * @uses \CakeDC\Users\Model\Table\FailedPasswordAttemptsTable::buildRules() - */ - public function testBuildRules(): void - { - $this->markTestIncomplete('Not implemented yet.'); - } -} From 1da252bfe229e5781e78cbfba83dc84fa184fe82 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Thu, 28 Mar 2024 11:36:30 -0300 Subject: [PATCH 03/17] Fix deprecated warnings --- tests/TestCase/Model/Table/UsersTableTest.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/TestCase/Model/Table/UsersTableTest.php b/tests/TestCase/Model/Table/UsersTableTest.php index b9995544f..4d731996c 100644 --- a/tests/TestCase/Model/Table/UsersTableTest.php +++ b/tests/TestCase/Model/Table/UsersTableTest.php @@ -20,6 +20,7 @@ use Cake\TestSuite\TestCase; use CakeDC\Users\Exception\AccountNotActiveException; use CakeDC\Users\Model\Table\SocialAccountsTable; +use CakeDC\Users\Model\Table\UsersTable; /** * Users\Model\Table\UsersTable Test Case @@ -36,6 +37,9 @@ class UsersTableTest extends TestCase 'plugin.CakeDC/Users.SocialAccounts', ]; + protected UsersTable $Users; + protected string $fullBaseBackup; + /** * setUp method * From 614c2f71473e53fadfe274b147b948402282f567 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Thu, 28 Mar 2024 12:41:48 -0300 Subject: [PATCH 04/17] [In Progress]Failed Password Attempts - Block --- src/Identifier/PasswordLockoutIdentifier.php | 49 +++++++++++ .../Fixture/FailedPasswordAttemptsFixture.php | 54 +++++++++++- .../PasswordLockoutIdentifierTest.php | 83 +++++++++++++++++++ 3 files changed, 185 insertions(+), 1 deletion(-) create mode 100644 src/Identifier/PasswordLockoutIdentifier.php create mode 100644 tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php diff --git a/src/Identifier/PasswordLockoutIdentifier.php b/src/Identifier/PasswordLockoutIdentifier.php new file mode 100644 index 000000000..9ee414951 --- /dev/null +++ b/src/Identifier/PasswordLockoutIdentifier.php @@ -0,0 +1,49 @@ +get('CakeDC/Users.FailedPasswordAttempts'); + $numberOfAttemptsFail = $this->getConfig('numberOfAttemptsFail', 6); + $timeWindow = $this->getConfig('timeWindowInSeconds', 5 * 60); + $lockTime = $this->getConfig('lockTimeInSeconds', 5 * 60); + $lockTime = (new DateTime())->subSeconds($lockTime); + $query = $Table->find(); + $attempts = $query + ->where([ + 'user_id' => $identity['id'], + $query->newExpr()->gte('created', $lockTime) + ]) + ->orderByDesc('created') + ->all(); + $attemptsCount = $attempts->count(); + $check = parent::_checkPassword($identity, $password); + if ($numberOfAttemptsFail <= $attemptsCount) { + return false; + } + + return $check; + } +} diff --git a/tests/Fixture/FailedPasswordAttemptsFixture.php b/tests/Fixture/FailedPasswordAttemptsFixture.php index 03941cbc5..358000275 100644 --- a/tests/Fixture/FailedPasswordAttemptsFixture.php +++ b/tests/Fixture/FailedPasswordAttemptsFixture.php @@ -17,7 +17,59 @@ class FailedPasswordAttemptsFixture extends TestFixture */ public function init(): void { - $this->records = []; + + $this->records = [ + [ + 'id' => '79cdd7a7-0f34-49dd-a691-21444f94c701', + 'user_id' => '00000000-0000-0000-0000-000000000002', + 'created' => date('Y-m-d H:i:s', strtotime('-20 minutes')), + ], + [ + 'id' => '79cdd7a7-0f34-49dd-a691-21444f94c702', + 'user_id' => '00000000-0000-0000-0000-000000000002', + 'created' => date('Y-m-d H:i:s', strtotime('-4 minutes')), + ], + [ + 'id' => '79cdd7a7-0f34-49dd-a691-21444f94c703', + 'user_id' => '00000000-0000-0000-0000-000000000002', + 'created' => date('Y-m-d H:i:s', strtotime('-4 minutes')), + ], + [ + 'id' => '79cdd7a7-0f34-49dd-a691-21444f94c704', + 'user_id' => '00000000-0000-0000-0000-000000000002', + 'created' => date('Y-m-d H:i:s', strtotime('-3 minutes')), + ], + [ + 'id' => '79cdd7a7-0f34-49dd-a691-21444f94c705', + 'user_id' => '00000000-0000-0000-0000-000000000002', + 'created' => date('Y-m-d H:i:s', strtotime('-2 minutes')), + ], + [ + 'id' => '79cdd7a7-0f34-49dd-a691-21444f94c801', + 'user_id' => '00000000-0000-0000-0000-000000000004', + 'created' => date('Y-m-d H:i:s', strtotime('-4 minutes')), + ], + [ + 'id' => '79cdd7a7-0f34-49dd-a691-21444f94c802', + 'user_id' => '00000000-0000-0000-0000-000000000004', + 'created' => date('Y-m-d H:i:s', strtotime('-4 minutes')), + ], + [ + 'id' => '79cdd7a7-0f34-49dd-a691-21444f94c803', + 'user_id' => '00000000-0000-0000-0000-000000000004', + 'created' => date('Y-m-d H:i:s', strtotime('-3 minutes')), + ], + [ + 'id' => '79cdd7a7-0f34-49dd-a691-21444f94c804', + 'user_id' => '00000000-0000-0000-0000-000000000004', + 'created' => date('Y-m-d H:i:s', strtotime('-3 minutes')), + ], + [ + 'id' => '79cdd7a7-0f34-49dd-a691-21444f94c805', + 'user_id' => '00000000-0000-0000-0000-000000000004', + 'created' => date('Y-m-d H:i:s', strtotime('-2 minutes')), + ], + ]; parent::init(); } } diff --git a/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php b/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php new file mode 100644 index 000000000..9c9891a02 --- /dev/null +++ b/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php @@ -0,0 +1,83 @@ +get('CakeDC/Users.FailedPasswordAttempts'); + $Table = TableRegistry::getTableLocator()->get('CakeDC/Users.Users'); + $user = $Table->get('00000000-0000-0000-0000-000000000002'); + $currentCount = 5; + $this->assertSame($currentCount, $AttemptsTable->find()->where(['user_id' => $user->id])->count()); + $user->password = $password; + $Table->saveOrFail($user); + $identifier = new PasswordLockoutIdentifier(); + $identity = $identifier->identify([ + PasswordIdentifier::CREDENTIAL_USERNAME => $user->username, + PasswordIdentifier::CREDENTIAL_PASSWORD => $password, + ]); + $this->assertInstanceOf(EntityInterface::class, $identity); + $this->assertSame($currentCount, $AttemptsTable->find()->where(['user_id' => $user->id])->count()); + } + + /** + * Test identify method with password and not locked + * + * @return void + */ + public function testIdentifyValidPasswordButLocked() + { + $password = Security::randomString(); + $AttemptsTable = TableRegistry::getTableLocator()->get('CakeDC/Users.FailedPasswordAttempts'); + $Table = TableRegistry::getTableLocator()->get('CakeDC/Users.Users'); + $user = $Table->get('00000000-0000-0000-0000-000000000004'); + $currentCount = 5; + $this->assertSame($currentCount, $AttemptsTable->find()->where(['user_id' => $user->id])->count()); + $user->password = $password; + $Table->saveOrFail($user); + $identifier = new PasswordLockoutIdentifier([ + 'numberOfAttemptsFail' => 5, + ]); + $identity = $identifier->identify([ + PasswordIdentifier::CREDENTIAL_USERNAME => $user->username, + PasswordIdentifier::CREDENTIAL_PASSWORD => $password, + ]); + $this->assertNull($identity); + $this->assertSame($currentCount, $AttemptsTable->find()->where(['user_id' => $user->id])->count()); + } +} From 6d1e5baa66b1503b49e95e42050cfcf66ff6ded6 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Thu, 28 Mar 2024 13:55:25 -0300 Subject: [PATCH 05/17] Account lockout policy --- src/Identifier/PasswordLockoutIdentifier.php | 8 +++- .../PasswordLockoutIdentifierTest.php | 44 +++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/src/Identifier/PasswordLockoutIdentifier.php b/src/Identifier/PasswordLockoutIdentifier.php index 9ee414951..6e2220ff7 100644 --- a/src/Identifier/PasswordLockoutIdentifier.php +++ b/src/Identifier/PasswordLockoutIdentifier.php @@ -30,6 +30,13 @@ protected function _checkPassword(ArrayAccess|array|null $identity, ?string $pas $timeWindow = $this->getConfig('timeWindowInSeconds', 5 * 60); $lockTime = $this->getConfig('lockTimeInSeconds', 5 * 60); $lockTime = (new DateTime())->subSeconds($lockTime); + + $check = parent::_checkPassword($identity, $password); + if (!$check) { + $entity = $Table->newEntity(['user_id' => $identity['id']]); + $Table->saveOrFail($entity); + $Table->deleteAll($Table->query()->newExpr()->lt('created', $lockTime)); + } $query = $Table->find(); $attempts = $query ->where([ @@ -39,7 +46,6 @@ protected function _checkPassword(ArrayAccess|array|null $identity, ?string $pas ->orderByDesc('created') ->all(); $attemptsCount = $attempts->count(); - $check = parent::_checkPassword($identity, $password); if ($numberOfAttemptsFail <= $attemptsCount) { return false; } diff --git a/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php b/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php index 9c9891a02..9241f9f8c 100644 --- a/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php +++ b/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php @@ -80,4 +80,48 @@ public function testIdentifyValidPasswordButLocked() $this->assertNull($identity); $this->assertSame($currentCount, $AttemptsTable->find()->where(['user_id' => $user->id])->count()); } + + /** + * Test identify method with password and not locked + * + * @return void + */ + public function testIdentifyInValidPasswordNotLockedBefore() + { + $password = Security::randomString(); + $AttemptsTable = TableRegistry::getTableLocator()->get('CakeDC/Users.FailedPasswordAttempts'); + $Table = TableRegistry::getTableLocator()->get('CakeDC/Users.Users'); + $user = $Table->get('00000000-0000-0000-0000-000000000002'); + $currentCount = 5; + $attemptsBefore = $AttemptsTable->find() + ->where(['user_id' => $user->id]) + ->orderByAsc('created') + ->all() + ->toArray(); + $this->assertSame($currentCount, count($attemptsBefore)); + $wrongPassword = Security::randomString(); + $this->assertNotEquals($wrongPassword, $password); + $user->password = $password; + $Table->saveOrFail($user); + $identifier = new PasswordLockoutIdentifier(); + $identity = $identifier->identify([ + PasswordIdentifier::CREDENTIAL_USERNAME => $user->username, + PasswordIdentifier::CREDENTIAL_PASSWORD => $wrongPassword,//wrong password + ]); + //First call remove the first failed_password_attempt because is out of the window range and adds a new one + $this->assertNull($identity); + $this->assertFalse($AttemptsTable->exists(['id' => $attemptsBefore[0]->id])); + $attemptsAfter = $AttemptsTable->find()->where(['user_id' => $user->id])->orderByAsc('created')->all()->toArray(); + $this->assertSame($currentCount, count($attemptsAfter)); + + $identity = $identifier->identify([ + PasswordIdentifier::CREDENTIAL_USERNAME => $user->username, + PasswordIdentifier::CREDENTIAL_PASSWORD => $wrongPassword,//wrong password + ]); + //On second call there is no record out of the window range only adds a new one + $this->assertNull($identity); + $this->assertTrue($AttemptsTable->exists(['id' => $attemptsAfter[0]->id])); + $attemptsAfterSecond = $AttemptsTable->find()->where(['user_id' => $user->id])->count(); + $this->assertSame($currentCount + 1, $attemptsAfterSecond); + } } From 35a434b9f33137b5d508711c90197f2b80a9fc23 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Thu, 28 Mar 2024 14:19:43 -0300 Subject: [PATCH 06/17] Account lockout policy --- config/Migrations/schema-dump-default.lock | Bin 8123 -> 0 bytes src/Identifier/PasswordLockoutIdentifier.php | 20 ++++++++--- .../Fixture/FailedPasswordAttemptsFixture.php | 5 +++ .../PasswordLockoutIdentifierTest.php | 34 +++++++++++++++--- 4 files changed, 49 insertions(+), 10 deletions(-) delete mode 100644 config/Migrations/schema-dump-default.lock diff --git a/config/Migrations/schema-dump-default.lock b/config/Migrations/schema-dump-default.lock deleted file mode 100644 index 9c437b3c14deb432447ac8cb12213731dd01e881..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 8123 zcmdT}!E)O;4DGk*xF^e5H(ln|?WKoy5A88C8d{=LPGnI-O4(#G`S(6hl0`xC*oo`d zJ~b{x3Iu_N_aG_e;@OFXh-#;-i*EiCS91~lEAQ3q7g@=rbn5m;`b(|l?N7PPRr7Zw zu4&mnBKq*xhXjx1D!K``aUzZzV`}nxU0`P^<}+G^?R7>!{T6?VsCp{>YW!K(WOCx8 zh;mijRllN}@AwHzQRjJdvlKJ@ycSWWREENvdhvH~Wt5a48x=Imq$2@nIa#qdAU?DW!A&8|$;+qJkbHxmMb`DtUd*EGTt@ zlzIQiqXKeyp}r&CD|4?|al}ADjeW-~HI8kW=q7nRYYej)~ze!&hMsu7EUJLzG32|2JA^hvxDeCOf<_St}c}&jx$vo zCv*=vVv#?uD~}QD;w{tzV{_V;SsjhV5O1>zpfgz?2cQnm1){F>vRwqXp|fj~Z7tf< zI7NsevrH38%bcoLPTe{ZqEe5Zz;7r2{JuA_IodT7;q#m+H+R~Tk3ZPD-+a}$>rP&g zR{17N3h6*zHKowXDcpRob39)6x(3(N>Ii1QqMoNS8|T5yJS3XTh!?hugBd26l9y;9 zGa6DBz5%5grv#%dg5Er_e$@PY^##oiVd=Zj)aG3~;0woy%%9kRBJ(T6n$a8KPha#S z>tyx$+~}%}XE@s;NHtw#>apRk5b|U|yh;D3ra(SPT~waj#w4TRb7}Qj+9yiQE2gYN ztA93_5@5OZGQS-Z;6(u(ZE7af!SS`_BXXKJQM9=r#F z+)n@+|3{LXb_Qkla}BjFjrB*M!9#muXHpFylWA&df8v8iCZ2Hd9sBYQ!NLHcUn3cIa-!q+NR*XTD`i!Y9!lu8cEqDKc%X%Zgp{5j zACNL9peE1=nR2Ps9sn23set+h;txEsXNLpv7t*wW0cY#^5|bPQKEiCne$R|SKV=qDtb=~#o#|xA zrYEpf+tuO-)7Z*=f7`+nGtlZb)dS1sfTOgHw3%pIT7oY86xysJbLj>0&EgDzi>EW-3=(OdawB;cdTm Jzn>U=`3=1y0=@tM diff --git a/src/Identifier/PasswordLockoutIdentifier.php b/src/Identifier/PasswordLockoutIdentifier.php index 6e2220ff7..178243915 100644 --- a/src/Identifier/PasswordLockoutIdentifier.php +++ b/src/Identifier/PasswordLockoutIdentifier.php @@ -29,27 +29,37 @@ protected function _checkPassword(ArrayAccess|array|null $identity, ?string $pas $numberOfAttemptsFail = $this->getConfig('numberOfAttemptsFail', 6); $timeWindow = $this->getConfig('timeWindowInSeconds', 5 * 60); $lockTime = $this->getConfig('lockTimeInSeconds', 5 * 60); - $lockTime = (new DateTime())->subSeconds($lockTime); + $timeWindow = (new DateTime())->subSeconds($timeWindow); $check = parent::_checkPassword($identity, $password); if (!$check) { $entity = $Table->newEntity(['user_id' => $identity['id']]); $Table->saveOrFail($entity); - $Table->deleteAll($Table->query()->newExpr()->lt('created', $lockTime)); + $Table->deleteAll($Table->query()->newExpr()->lt('created', $timeWindow)); } $query = $Table->find(); $attempts = $query ->where([ 'user_id' => $identity['id'], - $query->newExpr()->gte('created', $lockTime) + $query->newExpr()->gte('created', $timeWindow) ]) ->orderByDesc('created') ->all(); $attemptsCount = $attempts->count(); - if ($numberOfAttemptsFail <= $attemptsCount) { + if (!$check) { return false; } - return $check; + if ($numberOfAttemptsFail > $attemptsCount) { + return true; + } + + /** + * @var \CakeDC\Users\Model\Entity\FailedPasswordAttempt $attempt + */ + $attempt = $attempts->first(); + $lockTime = $attempt->created->addSeconds($lockTime); + + return $lockTime->isPast(); } } diff --git a/tests/Fixture/FailedPasswordAttemptsFixture.php b/tests/Fixture/FailedPasswordAttemptsFixture.php index 358000275..e4a4e8d38 100644 --- a/tests/Fixture/FailedPasswordAttemptsFixture.php +++ b/tests/Fixture/FailedPasswordAttemptsFixture.php @@ -44,6 +44,11 @@ public function init(): void 'user_id' => '00000000-0000-0000-0000-000000000002', 'created' => date('Y-m-d H:i:s', strtotime('-2 minutes')), ], + [ + 'id' => '79cdd7a7-0f34-49dd-a691-21444f94c800', + 'user_id' => '00000000-0000-0000-0000-000000000004', + 'created' => date('Y-m-d H:i:s', strtotime('-4 minutes')), + ], [ 'id' => '79cdd7a7-0f34-49dd-a691-21444f94c801', 'user_id' => '00000000-0000-0000-0000-000000000004', diff --git a/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php b/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php index 9241f9f8c..c469dc6ff 100644 --- a/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php +++ b/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php @@ -36,7 +36,7 @@ class PasswordLockoutIdentifierTest extends TestCase * * @return void */ - public function testIdentifyValidPasswordNotLocked() + public function testIdentifyOk() { $password = Security::randomString(); $AttemptsTable = TableRegistry::getTableLocator()->get('CakeDC/Users.FailedPasswordAttempts'); @@ -60,24 +60,48 @@ public function testIdentifyValidPasswordNotLocked() * * @return void */ - public function testIdentifyValidPasswordButLocked() + public function testIdentifyValidPasswordButReachedMaxAttemptsAndLockTimeNotCompleted() { $password = Security::randomString(); $AttemptsTable = TableRegistry::getTableLocator()->get('CakeDC/Users.FailedPasswordAttempts'); $Table = TableRegistry::getTableLocator()->get('CakeDC/Users.Users'); $user = $Table->get('00000000-0000-0000-0000-000000000004'); - $currentCount = 5; + $currentCount = 6; + $this->assertSame($currentCount, $AttemptsTable->find()->where(['user_id' => $user->id])->count()); + $user->password = $password; + $Table->saveOrFail($user); + $identifier = new PasswordLockoutIdentifier(); + $identity = $identifier->identify([ + PasswordIdentifier::CREDENTIAL_USERNAME => $user->username, + PasswordIdentifier::CREDENTIAL_PASSWORD => $password, + ]); + $this->assertNull($identity); + $this->assertSame($currentCount, $AttemptsTable->find()->where(['user_id' => $user->id])->count()); + } + + /** + * Test identify method with password and not locked + * + * @return void + */ + public function testIdentifyValidPasswordButReachedMaxAttemptsAndLockTimeAlreadyCompleted() + { + $password = Security::randomString(); + $AttemptsTable = TableRegistry::getTableLocator()->get('CakeDC/Users.FailedPasswordAttempts'); + $Table = TableRegistry::getTableLocator()->get('CakeDC/Users.Users'); + $user = $Table->get('00000000-0000-0000-0000-000000000004'); + $currentCount = 6; $this->assertSame($currentCount, $AttemptsTable->find()->where(['user_id' => $user->id])->count()); $user->password = $password; $Table->saveOrFail($user); $identifier = new PasswordLockoutIdentifier([ - 'numberOfAttemptsFail' => 5, + 'lockTimeInSeconds' => 60, ]); $identity = $identifier->identify([ PasswordIdentifier::CREDENTIAL_USERNAME => $user->username, PasswordIdentifier::CREDENTIAL_PASSWORD => $password, ]); - $this->assertNull($identity); + $this->assertInstanceOf(EntityInterface::class, $identity); $this->assertSame($currentCount, $AttemptsTable->find()->where(['user_id' => $user->id])->count()); } From 63dc906a60dbba78ad3d75198ab07ca35d73b102 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Thu, 28 Mar 2024 15:10:32 -0300 Subject: [PATCH 07/17] Account lockout policy - refactor --- .../PasswordLockout/LockoutHandler.php | 149 ++++++++++++++++++ .../LockoutHandlerInterface.php | 19 +++ src/Identifier/PasswordLockoutIdentifier.php | 84 ++++++---- .../PasswordLockoutIdentifierTest.php | 6 +- 4 files changed, 226 insertions(+), 32 deletions(-) create mode 100644 src/Identifier/PasswordLockout/LockoutHandler.php create mode 100644 src/Identifier/PasswordLockout/LockoutHandlerInterface.php diff --git a/src/Identifier/PasswordLockout/LockoutHandler.php b/src/Identifier/PasswordLockout/LockoutHandler.php new file mode 100644 index 000000000..a0b556582 --- /dev/null +++ b/src/Identifier/PasswordLockout/LockoutHandler.php @@ -0,0 +1,149 @@ + 5 * 60, + 'lockTimeInSeconds' => 5 * 60, + 'numberOfAttemptsFail' => 6, + 'failedPasswordAttemptsModel' => 'CakeDC/Users.FailedPasswordAttempts' + ]; + + /** + * Constructor + * + * @param array $config Configuration + */ + public function __construct(array $config = []) + { + $this->setConfig($config); + } + + /** + * @param string|int $id User's id + * @return bool + */ + public function isUnlocked(string|int $id): bool + { + $timeWindow = $this->getTimeWindow(); + $attempts = $this->getAttempts($id, $timeWindow); + $attemptsCount = $attempts->count(); + $numberOfAttemptsFail = $this->getNumberOfAttemptsFail(); + + if ($numberOfAttemptsFail > $attemptsCount) { + return true; + } + + $lockTime = $this->getLockTime(); + /** + * @var \CakeDC\Users\Model\Entity\FailedPasswordAttempt $attempt + */ + $attempt = $attempts->first(); + $lockTime = $attempt->created->addSeconds($lockTime); + + return $lockTime->isPast(); + } + + /** + * @param string|int $id User's id + * + * @return void + */ + public function newFail(string|int $id): void + { + $timeWindow = $this->getTimeWindow(); + $Table = $this->getTable(); + $entity = $Table->newEntity(['user_id' => $id]); + $Table->saveOrFail($entity); + $Table->deleteAll($Table->query()->newExpr()->lt('created', $timeWindow)); + } + + /** + * @return \Cake\ORM\Table + */ + protected function getTable(): \Cake\ORM\Table + { + return $this->getTableLocator()->get('CakeDC/Users.FailedPasswordAttempts'); + } + + /** + * @param string|int $id + * @param \Cake\I18n\DateTime $timeWindow + * @return \Cake\Datasource\ResultSetInterface + */ + protected function getAttempts(string|int $id, DateTime $timeWindow): \Cake\Datasource\ResultSetInterface + { + $query = $this->getTable()->find(); + + return $query + ->where([ + 'user_id' => $id, + $query->newExpr()->gte('created', $timeWindow) + ]) + ->orderByDesc('created') + ->all(); + } + + /** + * @return \Cake\I18n\DateTime + */ + protected function getTimeWindow(): DateTime + { + $timeWindow = $this->getConfig('timeWindowInSeconds'); + if (is_int($timeWindow) && $timeWindow >= 60) { + return (new DateTime())->subSeconds($timeWindow); + } + + throw new \UnexpectedValueException(__d('cake_d_c/users', 'Config "timeWindowInSeconds" must be integer greater than 60')); + } + + /** + * @return int + */ + protected function getNumberOfAttemptsFail(): int + { + $number = $this->getConfig('numberOfAttemptsFail'); + if (is_int($number) && $number >= 1) { + return $number; + } + throw new \UnexpectedValueException(__d('cake_d_c/users', 'Config "numberOfAttemptsFail" must be integer greater or equal 0')); + } + + /** + * @return int + */ + protected function getLockTime(): int + { + $lockTime = $this->getConfig('lockTimeInSeconds'); + if (is_int($lockTime) && $lockTime >= 60) { + return $lockTime; + } + + throw new \UnexpectedValueException(__d('cake_d_c/users', 'Config "lockTimeInSeconds" must be integer greater than 60')); + } +} diff --git a/src/Identifier/PasswordLockout/LockoutHandlerInterface.php b/src/Identifier/PasswordLockout/LockoutHandlerInterface.php new file mode 100644 index 000000000..c48884127 --- /dev/null +++ b/src/Identifier/PasswordLockout/LockoutHandlerInterface.php @@ -0,0 +1,19 @@ +_defaultConfig['lockoutHandler'] = [ + 'className' => LockoutHandler::class, + ]; + + parent::__construct($config); + } + + /** * @inheritDoc */ protected function _checkPassword(ArrayAccess|array|null $identity, ?string $password): bool { - $Table = TableRegistry::getTableLocator()->get('CakeDC/Users.FailedPasswordAttempts'); - $numberOfAttemptsFail = $this->getConfig('numberOfAttemptsFail', 6); - $timeWindow = $this->getConfig('timeWindowInSeconds', 5 * 60); - $lockTime = $this->getConfig('lockTimeInSeconds', 5 * 60); - $timeWindow = (new DateTime())->subSeconds($timeWindow); - $check = parent::_checkPassword($identity, $password); + $handler = $this->getLockoutHandler(); if (!$check) { - $entity = $Table->newEntity(['user_id' => $identity['id']]); - $Table->saveOrFail($entity); - $Table->deleteAll($Table->query()->newExpr()->lt('created', $timeWindow)); - } - $query = $Table->find(); - $attempts = $query - ->where([ - 'user_id' => $identity['id'], - $query->newExpr()->gte('created', $timeWindow) - ]) - ->orderByDesc('created') - ->all(); - $attemptsCount = $attempts->count(); - if (!$check) { + $handler->newFail($identity['id']); + return false; } - if ($numberOfAttemptsFail > $attemptsCount) { - return true; + return $handler->isUnlocked($identity['id']); + } + + /** + * @return \CakeDC\Users\Identifier\PasswordLockout\LockoutHandler + */ + protected function getLockoutHandler(): LockoutHandler + { + if ($this->lockoutHandler !== null) { + return $this->lockoutHandler; + } + $config = $this->getConfig('lockoutHandler'); + if ($config !== null) { + $this->lockoutHandler = $this->buildLockoutHandler($config); + + return $this->lockoutHandler; } + throw new \RuntimeException(__d('cake_d_c/users', 'Lockout handler has not been set.')); + } - /** - * @var \CakeDC\Users\Model\Entity\FailedPasswordAttempt $attempt - */ - $attempt = $attempts->first(); - $lockTime = $attempt->created->addSeconds($lockTime); + /** + * @param array|string $config + * @return \CakeDC\Users\Identifier\PasswordLockout\LockoutHandlerInterface + */ + protected function buildLockoutHandler(array|string $config): LockoutHandlerInterface + { + if (is_string($config)) { + $config = [ + 'className' => $config, + ]; + } + if (!isset($config['className'])) { + throw new \InvalidArgumentException(__d('cake_d_c/users', 'Option `className` for lockout handler is not present.')); + } + $className = $config['className']; - return $lockTime->isPast(); + return new $className($config); } } diff --git a/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php b/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php index c469dc6ff..6ce8f6b71 100644 --- a/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php +++ b/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php @@ -95,7 +95,9 @@ public function testIdentifyValidPasswordButReachedMaxAttemptsAndLockTimeAlready $user->password = $password; $Table->saveOrFail($user); $identifier = new PasswordLockoutIdentifier([ - 'lockTimeInSeconds' => 60, + 'lockoutHandler' => [ + 'lockTimeInSeconds' => 60, + ], ]); $identity = $identifier->identify([ PasswordIdentifier::CREDENTIAL_USERNAME => $user->username, @@ -110,7 +112,7 @@ public function testIdentifyValidPasswordButReachedMaxAttemptsAndLockTimeAlready * * @return void */ - public function testIdentifyInValidPasswordNotLockedBefore() + public function testIdentifyInValidPasswordClearOldFailures() { $password = Security::randomString(); $AttemptsTable = TableRegistry::getTableLocator()->get('CakeDC/Users.FailedPasswordAttempts'); From 8e8304112ada0ea6325815e78f2ddebf25311f61 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Thu, 28 Mar 2024 15:16:22 -0300 Subject: [PATCH 08/17] Account lockout policy - refactor --- src/Identifier/PasswordLockout/LockoutHandler.php | 7 +++---- src/Identifier/PasswordLockout/LockoutHandlerInterface.php | 2 +- src/Identifier/PasswordLockoutIdentifier.php | 4 +++- src/Model/Table/FailedPasswordAttemptsTable.php | 3 --- tests/Fixture/FailedPasswordAttemptsFixture.php | 1 - .../TestCase/Identifier/PasswordLockoutIdentifierTest.php | 1 - 6 files changed, 7 insertions(+), 11 deletions(-) diff --git a/src/Identifier/PasswordLockout/LockoutHandler.php b/src/Identifier/PasswordLockout/LockoutHandler.php index a0b556582..f82870e82 100644 --- a/src/Identifier/PasswordLockout/LockoutHandler.php +++ b/src/Identifier/PasswordLockout/LockoutHandler.php @@ -28,10 +28,10 @@ class LockoutHandler implements LockoutHandlerInterface * @var array{timeWindowInSeconds: int, lockTimeInSeconds: int, numberOfAttemptsFail:int} */ protected array $_defaultConfig = [ - 'timeWindowInSeconds' => 5 * 60, + 'timeWindowInSeconds' => 5 * 60, 'lockTimeInSeconds' => 5 * 60, 'numberOfAttemptsFail' => 6, - 'failedPasswordAttemptsModel' => 'CakeDC/Users.FailedPasswordAttempts' + 'failedPasswordAttemptsModel' => 'CakeDC/Users.FailedPasswordAttempts', ]; /** @@ -71,7 +71,6 @@ public function isUnlocked(string|int $id): bool /** * @param string|int $id User's id - * * @return void */ public function newFail(string|int $id): void @@ -103,7 +102,7 @@ protected function getAttempts(string|int $id, DateTime $timeWindow): \Cake\Data return $query ->where([ 'user_id' => $id, - $query->newExpr()->gte('created', $timeWindow) + $query->newExpr()->gte('created', $timeWindow), ]) ->orderByDesc('created') ->all(); diff --git a/src/Identifier/PasswordLockout/LockoutHandlerInterface.php b/src/Identifier/PasswordLockout/LockoutHandlerInterface.php index c48884127..c0262a1be 100644 --- a/src/Identifier/PasswordLockout/LockoutHandlerInterface.php +++ b/src/Identifier/PasswordLockout/LockoutHandlerInterface.php @@ -1,4 +1,5 @@ _defaultConfig['lockoutHandler'] = [ @@ -34,7 +37,6 @@ public function __construct(array $config = []) parent::__construct($config); } - /** * @inheritDoc */ diff --git a/src/Model/Table/FailedPasswordAttemptsTable.php b/src/Model/Table/FailedPasswordAttemptsTable.php index e097ea856..911247d77 100644 --- a/src/Model/Table/FailedPasswordAttemptsTable.php +++ b/src/Model/Table/FailedPasswordAttemptsTable.php @@ -3,7 +3,6 @@ namespace CakeDC\Users\Model\Table; -use Cake\ORM\Query\SelectQuery; use Cake\ORM\RulesChecker; use Cake\ORM\Table; use Cake\Validation\Validator; @@ -12,7 +11,6 @@ * FailedPasswordAttempts Model * * @property \CakeDC\Users\Model\Table\UsersTable&\Cake\ORM\Association\BelongsTo $Users - * * @method \CakeDC\Users\Model\Entity\FailedPasswordAttempt newEmptyEntity() * @method \CakeDC\Users\Model\Entity\FailedPasswordAttempt newEntity(array $data, array $options = []) * @method array<\CakeDC\Users\Model\Entity\FailedPasswordAttempt> newEntities(array $data, array $options = []) @@ -26,7 +24,6 @@ * @method iterable<\CakeDC\Users\Model\Entity\FailedPasswordAttempt>|\Cake\Datasource\ResultSetInterface<\CakeDC\Users\Model\Entity\FailedPasswordAttempt> saveManyOrFail(iterable $entities, array $options = []) * @method iterable<\CakeDC\Users\Model\Entity\FailedPasswordAttempt>|\Cake\Datasource\ResultSetInterface<\CakeDC\Users\Model\Entity\FailedPasswordAttempt>|false deleteMany(iterable $entities, array $options = []) * @method iterable<\CakeDC\Users\Model\Entity\FailedPasswordAttempt>|\Cake\Datasource\ResultSetInterface<\CakeDC\Users\Model\Entity\FailedPasswordAttempt> deleteManyOrFail(iterable $entities, array $options = []) - * * @mixin \Cake\ORM\Behavior\TimestampBehavior */ class FailedPasswordAttemptsTable extends Table diff --git a/tests/Fixture/FailedPasswordAttemptsFixture.php b/tests/Fixture/FailedPasswordAttemptsFixture.php index e4a4e8d38..f609682ec 100644 --- a/tests/Fixture/FailedPasswordAttemptsFixture.php +++ b/tests/Fixture/FailedPasswordAttemptsFixture.php @@ -17,7 +17,6 @@ class FailedPasswordAttemptsFixture extends TestFixture */ public function init(): void { - $this->records = [ [ 'id' => '79cdd7a7-0f34-49dd-a691-21444f94c701', diff --git a/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php b/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php index 6ce8f6b71..c36eb3ceb 100644 --- a/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php +++ b/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php @@ -13,7 +13,6 @@ namespace CakeDC\Users\Test\TestCase\Identifier; - use Authentication\Identifier\PasswordIdentifier; use Cake\Datasource\EntityInterface; use Cake\ORM\TableRegistry; From dcab19ddd9b953580e97318897c0019360ec0140 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Thu, 28 Mar 2024 15:36:22 -0300 Subject: [PATCH 09/17] Account lockout policy - refactor --- .../PasswordLockout/LockoutHandler.php | 12 +-- .../PasswordLockout/LockoutHandlerTest.php | 77 +++++++++++++++++++ .../PasswordLockoutIdentifierTest.php | 2 +- 3 files changed, 84 insertions(+), 7 deletions(-) create mode 100644 tests/TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php diff --git a/src/Identifier/PasswordLockout/LockoutHandler.php b/src/Identifier/PasswordLockout/LockoutHandler.php index f82870e82..da472c930 100644 --- a/src/Identifier/PasswordLockout/LockoutHandler.php +++ b/src/Identifier/PasswordLockout/LockoutHandler.php @@ -25,11 +25,11 @@ class LockoutHandler implements LockoutHandlerInterface /** * Default configuration. * - * @var array{timeWindowInSeconds: int, lockTimeInSeconds: int, numberOfAttemptsFail:int} + * @var array{timeWindowInSeconds: int, lockoutTimeInSeconds: int, numberOfAttemptsFail:int} */ protected array $_defaultConfig = [ 'timeWindowInSeconds' => 5 * 60, - 'lockTimeInSeconds' => 5 * 60, + 'lockoutTimeInSeconds' => 5 * 60, 'numberOfAttemptsFail' => 6, 'failedPasswordAttemptsModel' => 'CakeDC/Users.FailedPasswordAttempts', ]; @@ -59,7 +59,7 @@ public function isUnlocked(string|int $id): bool return true; } - $lockTime = $this->getLockTime(); + $lockTime = $this->getLockoutTime(); /** * @var \CakeDC\Users\Model\Entity\FailedPasswordAttempt $attempt */ @@ -136,13 +136,13 @@ protected function getNumberOfAttemptsFail(): int /** * @return int */ - protected function getLockTime(): int + protected function getLockoutTime(): int { - $lockTime = $this->getConfig('lockTimeInSeconds'); + $lockTime = $this->getConfig('lockoutTimeInSeconds'); if (is_int($lockTime) && $lockTime >= 60) { return $lockTime; } - throw new \UnexpectedValueException(__d('cake_d_c/users', 'Config "lockTimeInSeconds" must be integer greater than 60')); + throw new \UnexpectedValueException(__d('cake_d_c/users', 'Config "lockoutTimeInSeconds" must be integer greater than 60')); } } diff --git a/tests/TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php b/tests/TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php new file mode 100644 index 000000000..25b871efa --- /dev/null +++ b/tests/TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php @@ -0,0 +1,77 @@ +get('CakeDC/Users.FailedPasswordAttempts'); + $id = '00000000-0000-0000-0000-000000000002'; + $handler = new LockoutHandler(); + $currentCount = 5; + $attemptsBefore = $AttemptsTable->find() + ->where(['user_id' => $id]) + ->orderByAsc('created') + ->all() + ->toArray(); + //First time will remove old records and still add a new one + $handler->newFail($id); + $this->assertSame($currentCount, count($attemptsBefore)); + $this->assertFalse($AttemptsTable->exists(['id' => $attemptsBefore[0]->id])); + + //Now only add a new one because there is nothing to remove + $handler = new LockoutHandler(); + $handler->newFail($id); + $attemptsAfterSecond = $AttemptsTable->find()->where(['user_id' => $id])->count(); + $this->assertSame($currentCount + 1, $attemptsAfterSecond); + } + + /** + * @return void + */ + public function testIsUnlockedYes() + { + $handler = new LockoutHandler(); + $actual = $handler->isUnlocked( '00000000-0000-0000-0000-000000000002'); + $this->assertTrue($actual); + } + + /** + * @return void + */ + public function testIsUnlockedNo() + { + $handler = new LockoutHandler(); + $actual = $handler->isUnlocked( '00000000-0000-0000-0000-000000000004'); + $this->assertFalse($actual); + } + + /** + * @return void + */ + public function testIsUnlockedCompletedLockoutTime() + { + $handler = new LockoutHandler([ + 'lockoutTimeInSeconds' => 60, + ]); + $actual = $handler->isUnlocked( '00000000-0000-0000-0000-000000000004'); + $this->assertTrue($actual); + } +} diff --git a/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php b/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php index c36eb3ceb..c2b713112 100644 --- a/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php +++ b/tests/TestCase/Identifier/PasswordLockoutIdentifierTest.php @@ -95,7 +95,7 @@ public function testIdentifyValidPasswordButReachedMaxAttemptsAndLockTimeAlready $Table->saveOrFail($user); $identifier = new PasswordLockoutIdentifier([ 'lockoutHandler' => [ - 'lockTimeInSeconds' => 60, + 'lockoutTimeInSeconds' => 60, ], ]); $identity = $identifier->identify([ From 881306bfd0eef8a3a0616dc1be01c8db4fdc3613 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Thu, 28 Mar 2024 15:54:27 -0300 Subject: [PATCH 10/17] Account lockout policy - refactor --- config/Migrations/schema-dump-default.lock | Bin 0 -> 8123 bytes .../PasswordLockout/LockoutHandler.php | 43 +++++++++++++----- 2 files changed, 32 insertions(+), 11 deletions(-) create mode 100644 config/Migrations/schema-dump-default.lock diff --git a/config/Migrations/schema-dump-default.lock b/config/Migrations/schema-dump-default.lock new file mode 100644 index 0000000000000000000000000000000000000000..9c437b3c14deb432447ac8cb12213731dd01e881 GIT binary patch literal 8123 zcmdT}!E)O;4DGk*xF^e5H(ln|?WKoy5A88C8d{=LPGnI-O4(#G`S(6hl0`xC*oo`d zJ~b{x3Iu_N_aG_e;@OFXh-#;-i*EiCS91~lEAQ3q7g@=rbn5m;`b(|l?N7PPRr7Zw zu4&mnBKq*xhXjx1D!K``aUzZzV`}nxU0`P^<}+G^?R7>!{T6?VsCp{>YW!K(WOCx8 zh;mijRllN}@AwHzQRjJdvlKJ@ycSWWREENvdhvH~Wt5a48x=Imq$2@nIa#qdAU?DW!A&8|$;+qJkbHxmMb`DtUd*EGTt@ zlzIQiqXKeyp}r&CD|4?|al}ADjeW-~HI8kW=q7nRYYej)~ze!&hMsu7EUJLzG32|2JA^hvxDeCOf<_St}c}&jx$vo zCv*=vVv#?uD~}QD;w{tzV{_V;SsjhV5O1>zpfgz?2cQnm1){F>vRwqXp|fj~Z7tf< zI7NsevrH38%bcoLPTe{ZqEe5Zz;7r2{JuA_IodT7;q#m+H+R~Tk3ZPD-+a}$>rP&g zR{17N3h6*zHKowXDcpRob39)6x(3(N>Ii1QqMoNS8|T5yJS3XTh!?hugBd26l9y;9 zGa6DBz5%5grv#%dg5Er_e$@PY^##oiVd=Zj)aG3~;0woy%%9kRBJ(T6n$a8KPha#S z>tyx$+~}%}XE@s;NHtw#>apRk5b|U|yh;D3ra(SPT~waj#w4TRb7}Qj+9yiQE2gYN ztA93_5@5OZGQS-Z;6(u(ZE7af!SS`_BXXKJQM9=r#F z+)n@+|3{LXb_Qkla}BjFjrB*M!9#muXHpFylWA&df8v8iCZ2Hd9sBYQ!NLHcUn3cIa-!q+NR*XTD`i!Y9!lu8cEqDKc%X%Zgp{5j zACNL9peE1=nR2Ps9sn23set+h;txEsXNLpv7t*wW0cY#^5|bPQKEiCne$R|SKV=qDtb=~#o#|xA zrYEpf+tuO-)7Z*=f7`+nGtlZb)dS1sfTOgHw3%pIT7oY86xysJbLj>0&EgDzi>EW-3=(OdawB;cdTm Jzn>U=`3=1y0=@tM literal 0 HcmV?d00001 diff --git a/src/Identifier/PasswordLockout/LockoutHandler.php b/src/Identifier/PasswordLockout/LockoutHandler.php index da472c930..4f99e70aa 100644 --- a/src/Identifier/PasswordLockout/LockoutHandler.php +++ b/src/Identifier/PasswordLockout/LockoutHandler.php @@ -16,6 +16,8 @@ use Cake\Core\InstanceConfigTrait; use Cake\I18n\DateTime; use Cake\ORM\Locator\LocatorAwareTrait; +use Cake\ORM\Query\SelectQuery; +use CakeDC\Users\Model\Entity\FailedPasswordAttempt; class LockoutHandler implements LockoutHandlerInterface { @@ -51,19 +53,14 @@ public function __construct(array $config = []) public function isUnlocked(string|int $id): bool { $timeWindow = $this->getTimeWindow(); - $attempts = $this->getAttempts($id, $timeWindow); - $attemptsCount = $attempts->count(); + $attemptsCount = $this->getAttemptsCount($id, $timeWindow); $numberOfAttemptsFail = $this->getNumberOfAttemptsFail(); - if ($numberOfAttemptsFail > $attemptsCount) { return true; } $lockTime = $this->getLockoutTime(); - /** - * @var \CakeDC\Users\Model\Entity\FailedPasswordAttempt $attempt - */ - $attempt = $attempts->first(); + $attempt = $this->getLastAttempt($id, $timeWindow); $lockTime = $attempt->created->addSeconds($lockTime); return $lockTime->isPast(); @@ -93,9 +90,34 @@ protected function getTable(): \Cake\ORM\Table /** * @param string|int $id * @param \Cake\I18n\DateTime $timeWindow - * @return \Cake\Datasource\ResultSetInterface + * @return int + */ + protected function getAttemptsCount(string|int $id, DateTime $timeWindow): int + { + return $this->getAttemptsQuery($id, $timeWindow)->count(); + } + + + /** + * @param int|string $id + * @param \Cake\I18n\DateTime $timeWindow + * @return \CakeDC\Users\Model\Entity\FailedPasswordAttempt + */ + protected function getLastAttempt(int|string $id, DateTime $timeWindow): FailedPasswordAttempt + { + /** + * @var \CakeDC\Users\Model\Entity\FailedPasswordAttempt $attempt + */ + $attempt = $this->getAttemptsQuery($id, $timeWindow)->first(); + return $attempt; + } + + /** + * @param int|string $id + * @param \Cake\I18n\DateTime $timeWindow + * @return \Cake\ORM\Query\SelectQuery */ - protected function getAttempts(string|int $id, DateTime $timeWindow): \Cake\Datasource\ResultSetInterface + protected function getAttemptsQuery(int|string $id, DateTime $timeWindow): SelectQuery { $query = $this->getTable()->find(); @@ -104,8 +126,7 @@ protected function getAttempts(string|int $id, DateTime $timeWindow): \Cake\Data 'user_id' => $id, $query->newExpr()->gte('created', $timeWindow), ]) - ->orderByDesc('created') - ->all(); + ->orderByDesc('created'); } /** From 8767adaf1179273026dd41ca2ee7e56cfb670900 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Thu, 28 Mar 2024 15:54:49 -0300 Subject: [PATCH 11/17] Account lockout policy - refactor --- config/Migrations/schema-dump-default.lock | Bin 8123 -> 0 bytes 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 config/Migrations/schema-dump-default.lock diff --git a/config/Migrations/schema-dump-default.lock b/config/Migrations/schema-dump-default.lock deleted file mode 100644 index 9c437b3c14deb432447ac8cb12213731dd01e881..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 8123 zcmdT}!E)O;4DGk*xF^e5H(ln|?WKoy5A88C8d{=LPGnI-O4(#G`S(6hl0`xC*oo`d zJ~b{x3Iu_N_aG_e;@OFXh-#;-i*EiCS91~lEAQ3q7g@=rbn5m;`b(|l?N7PPRr7Zw zu4&mnBKq*xhXjx1D!K``aUzZzV`}nxU0`P^<}+G^?R7>!{T6?VsCp{>YW!K(WOCx8 zh;mijRllN}@AwHzQRjJdvlKJ@ycSWWREENvdhvH~Wt5a48x=Imq$2@nIa#qdAU?DW!A&8|$;+qJkbHxmMb`DtUd*EGTt@ zlzIQiqXKeyp}r&CD|4?|al}ADjeW-~HI8kW=q7nRYYej)~ze!&hMsu7EUJLzG32|2JA^hvxDeCOf<_St}c}&jx$vo zCv*=vVv#?uD~}QD;w{tzV{_V;SsjhV5O1>zpfgz?2cQnm1){F>vRwqXp|fj~Z7tf< zI7NsevrH38%bcoLPTe{ZqEe5Zz;7r2{JuA_IodT7;q#m+H+R~Tk3ZPD-+a}$>rP&g zR{17N3h6*zHKowXDcpRob39)6x(3(N>Ii1QqMoNS8|T5yJS3XTh!?hugBd26l9y;9 zGa6DBz5%5grv#%dg5Er_e$@PY^##oiVd=Zj)aG3~;0woy%%9kRBJ(T6n$a8KPha#S z>tyx$+~}%}XE@s;NHtw#>apRk5b|U|yh;D3ra(SPT~waj#w4TRb7}Qj+9yiQE2gYN ztA93_5@5OZGQS-Z;6(u(ZE7af!SS`_BXXKJQM9=r#F z+)n@+|3{LXb_Qkla}BjFjrB*M!9#muXHpFylWA&df8v8iCZ2Hd9sBYQ!NLHcUn3cIa-!q+NR*XTD`i!Y9!lu8cEqDKc%X%Zgp{5j zACNL9peE1=nR2Ps9sn23set+h;txEsXNLpv7t*wW0cY#^5|bPQKEiCne$R|SKV=qDtb=~#o#|xA zrYEpf+tuO-)7Z*=f7`+nGtlZb)dS1sfTOgHw3%pIT7oY86xysJbLj>0&EgDzi>EW-3=(OdawB;cdTm Jzn>U=`3=1y0=@tM From 4edad88e03be0009cede4f37cbc41504919ed5eb Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Thu, 28 Mar 2024 15:55:08 -0300 Subject: [PATCH 12/17] Account lockout policy - refactor --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index f696b2eed..382c08d34 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,4 @@ composer.lock .php_cs* /coverage .phpunit.result.cache +/config/schema-dump-default.lock From 5489d4300055d4036dce5359db4fd7f19e1eab60 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Thu, 28 Mar 2024 15:56:19 -0300 Subject: [PATCH 13/17] Account lockout policy - refactor --- .gitignore | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 382c08d34..b4faf0fdb 100644 --- a/.gitignore +++ b/.gitignore @@ -5,4 +5,4 @@ composer.lock .php_cs* /coverage .phpunit.result.cache -/config/schema-dump-default.lock +/config/Migrations/schema-dump-default.lock From aef8506917f19296f1a0509f97592cddd0248d18 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Thu, 28 Mar 2024 20:17:23 -0300 Subject: [PATCH 14/17] Account lockout policy - refactor --- .../20240328215332_AddLockoutTimeToUsers.php | 24 +++++++++ .../PasswordLockout/LockoutHandler.php | 40 ++++++++++----- .../LockoutHandlerInterface.php | 4 +- src/Identifier/PasswordLockoutIdentifier.php | 2 +- src/Model/Entity/User.php | 1 + .../PasswordLockout/LockoutHandlerTest.php | 49 ++++++++++++++++--- tests/schema.php | 1 + 7 files changed, 100 insertions(+), 21 deletions(-) create mode 100644 config/Migrations/20240328215332_AddLockoutTimeToUsers.php diff --git a/config/Migrations/20240328215332_AddLockoutTimeToUsers.php b/config/Migrations/20240328215332_AddLockoutTimeToUsers.php new file mode 100644 index 000000000..4055ebb5d --- /dev/null +++ b/config/Migrations/20240328215332_AddLockoutTimeToUsers.php @@ -0,0 +1,24 @@ +table('users'); + $table->addColumn('lockout_time', 'datetime', [ + 'default' => null, + 'null' => true, + ]); + $table->update(); + } +} diff --git a/src/Identifier/PasswordLockout/LockoutHandler.php b/src/Identifier/PasswordLockout/LockoutHandler.php index 4f99e70aa..f600f79fd 100644 --- a/src/Identifier/PasswordLockout/LockoutHandler.php +++ b/src/Identifier/PasswordLockout/LockoutHandler.php @@ -34,6 +34,8 @@ class LockoutHandler implements LockoutHandlerInterface 'lockoutTimeInSeconds' => 5 * 60, 'numberOfAttemptsFail' => 6, 'failedPasswordAttemptsModel' => 'CakeDC/Users.FailedPasswordAttempts', + 'userLockoutField' => 'lockout_time', + 'usersModel' => 'Users', ]; /** @@ -47,23 +49,30 @@ public function __construct(array $config = []) } /** - * @param string|int $id User's id + * @param \ArrayAccess|array $identity * @return bool */ - public function isUnlocked(string|int $id): bool + public function isUnlocked(\ArrayAccess|array $identity): bool { + $lockoutField = $this->getConfig('userLockoutField'); + $userLockoutTime = $identity[$lockoutField] ?? null; + if ($userLockoutTime) { + if (!$this->checkLockoutTime($userLockoutTime)) {//Still locked? + return false; + } + } $timeWindow = $this->getTimeWindow(); - $attemptsCount = $this->getAttemptsCount($id, $timeWindow); + $attemptsCount = $this->getAttemptsCount($identity['id'], $timeWindow); $numberOfAttemptsFail = $this->getNumberOfAttemptsFail(); if ($numberOfAttemptsFail > $attemptsCount) { return true; } + $lastAttempt = $this->getLastAttempt($identity['id'], $timeWindow); + $this->getTableLocator() + ->get($this->getConfig('usersModel')) + ->updateAll([$lockoutField => $lastAttempt->created], ['id' => $identity['id']]); - $lockTime = $this->getLockoutTime(); - $attempt = $this->getLastAttempt($id, $timeWindow); - $lockTime = $attempt->created->addSeconds($lockTime); - - return $lockTime->isPast(); + return $this->checkLockoutTime($lastAttempt->created); } /** @@ -97,9 +106,8 @@ protected function getAttemptsCount(string|int $id, DateTime $timeWindow): int return $this->getAttemptsQuery($id, $timeWindow)->count(); } - /** - * @param int|string $id + * @param string|int $id * @param \Cake\I18n\DateTime $timeWindow * @return \CakeDC\Users\Model\Entity\FailedPasswordAttempt */ @@ -109,11 +117,12 @@ protected function getLastAttempt(int|string $id, DateTime $timeWindow): FailedP * @var \CakeDC\Users\Model\Entity\FailedPasswordAttempt $attempt */ $attempt = $this->getAttemptsQuery($id, $timeWindow)->first(); + return $attempt; } /** - * @param int|string $id + * @param string|int $id * @param \Cake\I18n\DateTime $timeWindow * @return \Cake\ORM\Query\SelectQuery */ @@ -166,4 +175,13 @@ protected function getLockoutTime(): int throw new \UnexpectedValueException(__d('cake_d_c/users', 'Config "lockoutTimeInSeconds" must be integer greater than 60')); } + + /** + * @param \Cake\I18n\DateTime $dateTime + * @return bool + */ + protected function checkLockoutTime(DateTime $dateTime): bool + { + return $dateTime->addSeconds($this->getLockoutTime())->isPast(); + } } diff --git a/src/Identifier/PasswordLockout/LockoutHandlerInterface.php b/src/Identifier/PasswordLockout/LockoutHandlerInterface.php index c0262a1be..d524adca1 100644 --- a/src/Identifier/PasswordLockout/LockoutHandlerInterface.php +++ b/src/Identifier/PasswordLockout/LockoutHandlerInterface.php @@ -6,10 +6,10 @@ interface LockoutHandlerInterface { /** - * @param string|int $id User's id + * @param \ArrayAccess|array $identity * @return bool */ - public function isUnlocked(string|int $id): bool; + public function isUnlocked(\ArrayAccess|array $identity): bool; /** * @param string|int $id User's id diff --git a/src/Identifier/PasswordLockoutIdentifier.php b/src/Identifier/PasswordLockoutIdentifier.php index 2a620e910..cd2b099eb 100644 --- a/src/Identifier/PasswordLockoutIdentifier.php +++ b/src/Identifier/PasswordLockoutIdentifier.php @@ -50,7 +50,7 @@ protected function _checkPassword(ArrayAccess|array|null $identity, ?string $pas return false; } - return $handler->isUnlocked($identity['id']); + return $handler->isUnlocked($identity); } /** diff --git a/src/Model/Entity/User.php b/src/Model/Entity/User.php index d286c2632..cd454f375 100644 --- a/src/Model/Entity/User.php +++ b/src/Model/Entity/User.php @@ -32,6 +32,7 @@ * @property array|string $additional_data * @property \CakeDC\Users\Model\Entity\SocialAccount[] $social_accounts * @property string $password + * @property \Cake\I18n\DateTime $lockout_time */ class User extends Entity { diff --git a/tests/TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php b/tests/TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php index 25b871efa..1c1f1329d 100644 --- a/tests/TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php +++ b/tests/TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php @@ -1,10 +1,11 @@ isUnlocked( '00000000-0000-0000-0000-000000000002'); + $UsersTable = TableRegistry::getTableLocator()->get('Users'); + $actual = $handler->isUnlocked($UsersTable->get('00000000-0000-0000-0000-000000000002')); $this->assertTrue($actual); } /** * @return void */ - public function testIsUnlockedNo() + public function testIsUnlockedNotSavedLockoutAndLastFailureMax() { + $userId = '00000000-0000-0000-0000-000000000004'; + $UsersTable = TableRegistry::getTableLocator()->get('Users'); + $userBefore = $UsersTable->get($userId); + $this->assertNull($userBefore->lockout_time); $handler = new LockoutHandler(); - $actual = $handler->isUnlocked( '00000000-0000-0000-0000-000000000004'); + $actual = $handler->isUnlocked($UsersTable->get($userId)); $this->assertFalse($actual); + $userAfter = $UsersTable->get($userId); + $this->assertInstanceOf(DateTime::class, $userAfter->lockout_time); } /** * @return void */ - public function testIsUnlockedCompletedLockoutTime() + public function testIsUnlockedSaveLockoutAndCompleted() { $handler = new LockoutHandler([ - 'lockoutTimeInSeconds' => 60, + 'numberOfAttemptsFail' => 7, ]); - $actual = $handler->isUnlocked( '00000000-0000-0000-0000-000000000004'); + $UsersTable = TableRegistry::getTableLocator()->get('Users'); + $userId = '00000000-0000-0000-0000-000000000004'; + $UsersTable->updateAll(['lockout_time' => new DateTime('-6 minutes')], ['id' => $userId]); + $userBefore = $UsersTable->get($userId); + $this->assertInstanceOf(DateTime::class, $userBefore->lockout_time); + + $actual = $handler->isUnlocked($UsersTable->get($userId)); $this->assertTrue($actual); } + + /** + * @return void + */ + public function testIsUnlockedSaveLockoutAndNotCompleted() + { + $handler = new LockoutHandler([ + 'numberOfAttemptsFail' => 7, + ]); + $userId = '00000000-0000-0000-0000-000000000004'; + $UsersTable = TableRegistry::getTableLocator()->get('Users'); + $UsersTable->updateAll(['lockout_time' => new DateTime('-4 minutes')], ['id' => $userId]); + $userBefore = $UsersTable->get($userId); + $this->assertInstanceOf(DateTime::class, $userBefore->lockout_time); + + $actual = $handler->isUnlocked($UsersTable->get($userId)); + $this->assertFalse($actual); + $userAfter = $UsersTable->get($userId); + $this->assertInstanceOf(DateTime::class, $userAfter->lockout_time); + $this->assertEquals($userBefore->lockout_time, $userAfter->lockout_time); + } } diff --git a/tests/schema.php b/tests/schema.php index ac92fb123..b9c6ac078 100644 --- a/tests/schema.php +++ b/tests/schema.php @@ -71,6 +71,7 @@ 'modified' => ['type' => 'datetime', 'length' => null, 'null' => false, 'default' => null, 'comment' => '', 'precision' => null], 'additional_data' => ['type' => 'text', 'length' => null, 'null' => true, 'default' => null, 'comment' => '', 'precision' => null], 'last_login' => ['type' => 'datetime', 'length' => null, 'null' => true, 'default' => null, 'comment' => '', 'precision' => null], + 'lockout_time' => ['type' => 'datetime', 'length' => null, 'null' => true, 'default' => null, 'comment' => '', 'precision' => null], ], 'constraints' => [ 'primary' => [ From 597f25facf841efd2dea5d2bcca9a170f5e37250 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Thu, 28 Mar 2024 20:20:22 -0300 Subject: [PATCH 15/17] Account lockout policy - refactor --- src/Identifier/PasswordLockoutIdentifier.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Identifier/PasswordLockoutIdentifier.php b/src/Identifier/PasswordLockoutIdentifier.php index cd2b099eb..ccbe6e5f1 100644 --- a/src/Identifier/PasswordLockoutIdentifier.php +++ b/src/Identifier/PasswordLockoutIdentifier.php @@ -54,9 +54,9 @@ protected function _checkPassword(ArrayAccess|array|null $identity, ?string $pas } /** - * @return \CakeDC\Users\Identifier\PasswordLockout\LockoutHandler + * @return \CakeDC\Users\Identifier\PasswordLockout\LockoutHandlerInterface */ - protected function getLockoutHandler(): LockoutHandler + protected function getLockoutHandler(): LockoutHandlerInterface { if ($this->lockoutHandler !== null) { return $this->lockoutHandler; From 6240ba82d04bc7b4047cef030ad49178b476da33 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Fri, 29 Mar 2024 09:59:33 -0300 Subject: [PATCH 16/17] Account lockout policy - fail if no user id --- .../PasswordLockout/LockoutHandler.php | 3 +++ src/Identifier/PasswordLockoutIdentifier.php | 3 +++ .../PasswordLockout/LockoutHandlerTest.php | 24 +++++++++++++++++++ 3 files changed, 30 insertions(+) diff --git a/src/Identifier/PasswordLockout/LockoutHandler.php b/src/Identifier/PasswordLockout/LockoutHandler.php index f600f79fd..4896ef9a7 100644 --- a/src/Identifier/PasswordLockout/LockoutHandler.php +++ b/src/Identifier/PasswordLockout/LockoutHandler.php @@ -54,6 +54,9 @@ public function __construct(array $config = []) */ public function isUnlocked(\ArrayAccess|array $identity): bool { + if (!isset($identity['id'])) { + return false; + } $lockoutField = $this->getConfig('userLockoutField'); $userLockoutTime = $identity[$lockoutField] ?? null; if ($userLockoutTime) { diff --git a/src/Identifier/PasswordLockoutIdentifier.php b/src/Identifier/PasswordLockoutIdentifier.php index ccbe6e5f1..2157dc812 100644 --- a/src/Identifier/PasswordLockoutIdentifier.php +++ b/src/Identifier/PasswordLockoutIdentifier.php @@ -42,6 +42,9 @@ public function __construct(array $config = []) */ protected function _checkPassword(ArrayAccess|array|null $identity, ?string $password): bool { + if (!isset($identity['id'])) { + return false; + } $check = parent::_checkPassword($identity, $password); $handler = $this->getLockoutHandler(); if (!$check) { diff --git a/tests/TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php b/tests/TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php index 1c1f1329d..5845c4845 100644 --- a/tests/TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php +++ b/tests/TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php @@ -109,4 +109,28 @@ public function testIsUnlockedSaveLockoutAndNotCompleted() $this->assertInstanceOf(DateTime::class, $userAfter->lockout_time); $this->assertEquals($userBefore->lockout_time, $userAfter->lockout_time); } + + /** + * @return void + */ + public function testIsUnlockedWithoutIdButNotEmpty() + { + $handler = new LockoutHandler(); + $user = [ + 'username' => 'user-2', + 'email' => 'user-2@test.com' + ]; + $actual = $handler->isUnlocked($user); + $this->assertFalse($actual); + } + + /** + * @return void + */ + public function testIsUnlockedWithoutIdAndEmpty() + { + $handler = new LockoutHandler(); + $actual = $handler->isUnlocked([]); + $this->assertFalse($actual); + } } From 2b2fa3e3370c95f2bcd6dae2b0b4866e66dd7e55 Mon Sep 17 00:00:00 2001 From: Marcelo Rocha Date: Fri, 29 Mar 2024 10:00:06 -0300 Subject: [PATCH 17/17] Account lockout policy - fail if no user id --- .../TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php b/tests/TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php index 5845c4845..45c87ef8b 100644 --- a/tests/TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php +++ b/tests/TestCase/Identifier/PasswordLockout/LockoutHandlerTest.php @@ -118,7 +118,7 @@ public function testIsUnlockedWithoutIdButNotEmpty() $handler = new LockoutHandler(); $user = [ 'username' => 'user-2', - 'email' => 'user-2@test.com' + 'email' => 'user-2@test.com', ]; $actual = $handler->isUnlocked($user); $this->assertFalse($actual);