diff --git a/src/app/Http/Controllers/API/PasswordPolicyController.php b/src/app/Http/Controllers/API/PasswordPolicyController.php --- a/src/app/Http/Controllers/API/PasswordPolicyController.php +++ b/src/app/Http/Controllers/API/PasswordPolicyController.php @@ -45,7 +45,7 @@ $user = !empty($userId) ? \App\User::find($userId) : null; // Get the policy - $policy = new Password($user ? $user->walletOwner() : null); + $policy = new Password($user ? $user->walletOwner() : null, $user); // Check the password $status = $policy->check($request->input('password')); diff --git a/src/app/Observers/UserObserver.php b/src/app/Observers/UserObserver.php --- a/src/app/Observers/UserObserver.php +++ b/src/app/Observers/UserObserver.php @@ -234,6 +234,12 @@ \App\Jobs\User\UpdateJob::dispatch($user_id); }); } + + // Save the old password in the password history + $oldPassword = $user->getOriginal('password'); + if ($oldPassword && $user->password != $oldPassword) { + self::saveOldPassword($user, $oldPassword); + } } /** @@ -280,4 +286,39 @@ ] )->delete(); } + + /** + * Store the old password in user password history. Make sure + * we do not store more passwords than we need in the history. + * + * @param \App\User $user The user + * @param string $password The old password + */ + private static function saveOldPassword(User $user, string $password): void + { + // Note: All this is kinda heavy and complicated because we don't want to store + // more old passwords than we need. However, except the complication/performance, + // there's one issue with it. E.g. the policy changes from 2 to 4, and we already + // removed the old passwords that were excessive before, but not now. + + // Get the account password policy + $policy = new \App\Rules\Password($user->walletOwner()); + $rules = $policy->rules(); + + // Password history disabled? + if (empty($rules['last']) || $rules['last']['param'] < 2) { + return; + } + + // Store the old password + $user->passwords()->create(['password' => $password]); + + // Remove passwords that we don't need anymore + $limit = $rules['last']['param'] - 1; + $ids = $user->passwords()->latest()->limit($limit)->pluck('id')->all(); + + if (count($ids) >= $limit) { + $user->passwords()->where('id', '<', $ids[count($ids) - 1])->delete(); + } + } } diff --git a/src/app/Rules/Password.php b/src/app/Rules/Password.php --- a/src/app/Rules/Password.php +++ b/src/app/Rules/Password.php @@ -3,22 +3,31 @@ namespace App\Rules; use Illuminate\Contracts\Validation\Rule; +use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Validator; use Illuminate\Support\Str; class Password implements Rule { + /** @var ?string The validation error message */ private $message; + + /** @var ?\App\User The account owner which to take the policy from */ private $owner; + /** @var ?\App\User The user to whom the checked password belongs */ + private $user; + /** * Class constructor. * - * @param \App\User $owner The account owner (to take the policy from) + * @param ?\App\User $owner The account owner (to take the policy from) + * @param ?\App\User $user The user the password is for (Null for a new user) */ - public function __construct(?\App\User $owner = null) + public function __construct(?\App\User $owner = null, ?\App\User $user = null) { $this->owner = $owner; + $this->user = $user; } /** @@ -64,41 +73,47 @@ switch ($name) { case 'min': // Check the min length - $pass = strlen($password) >= intval($rule['param']); + $status = strlen($password) >= intval($rule['param']); break; case 'max': // Check the max length $length = strlen($password); - $pass = $length && $length <= intval($rule['param']); + $status = $length && $length <= intval($rule['param']); break; case 'lower': // Check if password contains a lower-case character - $pass = preg_match('/[a-z]/', $password) > 0; + $status = preg_match('/[a-z]/', $password) > 0; break; case 'upper': // Check if password contains a upper-case character - $pass = preg_match('/[A-Z]/', $password) > 0; + $status = preg_match('/[A-Z]/', $password) > 0; break; case 'digit': // Check if password contains a digit - $pass = preg_match('/[0-9]/', $password) > 0; + $status = preg_match('/[0-9]/', $password) > 0; break; case 'special': // Check if password contains a special character - $pass = preg_match('/[-~!@#$%^&*_+=`(){}[]|:;"\'`<>,.?\/\\]/', $password) > 0; + $status = preg_match('/[-~!@#$%^&*_+=`(){}[]|:;"\'`<>,.?\/\\]/', $password) > 0; + break; + + case 'last': + // TODO: For performance reasons we might consider checking the history + // only when the password passed all other checks + $status = $this->checkPasswordHistory($password, (int) $rule['param']); break; default: // Ignore unknown rule name - $pass = true; + $status = true; } - $rules[$name]['status'] = $pass; + $rules[$name]['status'] = $status; } return $rules; @@ -114,7 +129,7 @@ public function rules(bool $all = false): array { // All supported password policy rules (with default params) - $supported = 'min:6,max:255,lower,upper,digit,special'; + $supported = 'min:6,max:255,lower,upper,digit,special,last:3'; // Get the password policy from the $owner settings if ($this->owner) { @@ -168,6 +183,43 @@ } /** + * Check password agains of old passwords in user history + * + * @param string $password The password to check + * @param int $count Number of old passwords to check (including current one) + * + * @return bool True if password is unique, False otherwise + */ + protected function checkPasswordHistory($password, int $count): bool + { + $status = strlen($password) > 0; + + // Check if password is not the same as last X passwords + if ($status && $this->user && $count > 0) { + // Current password + if ($this->user->password) { + $count -= 1; + if (Hash::check($password, $this->user->password)) { + return false; + } + } + + // Passwords from the history + if ($count > 0) { + $this->user->passwords()->latest()->limit($count)->get() + ->each(function ($oldPassword) use (&$status, $password) { + if (Hash::check($password, $oldPassword->password)) { + $status = false; + return false; // stop iteration + } + }); + } + } + + return $status; + } + + /** * Convert an array with password policy rules into one indexed by the rule name * * @param array $rules The rules list diff --git a/src/app/Traits/UserConfigTrait.php b/src/app/Traits/UserConfigTrait.php --- a/src/app/Traits/UserConfigTrait.php +++ b/src/app/Traits/UserConfigTrait.php @@ -67,7 +67,7 @@ protected function validatePasswordPolicyRule(string $rule): ?string { $regexp = [ - 'min:[0-9]+', 'max:[0-9]+', 'upper', 'lower', 'digit', 'special', + 'min:[0-9]+', 'max:[0-9]+', 'upper', 'lower', 'digit', 'special', 'last:[0-9]+' ]; if (empty($rule) || !preg_match('/^(' . implode('|', $regexp) . ')$/', $rule)) { @@ -92,6 +92,13 @@ } } + if (!empty($systemPolicy['last']) && strpos($rule, 'last:') === 0) { + $value = trim(substr($rule, 5)); + if ($value < $systemPolicy['last']) { + return \trans('validation.password-policy-last-error', ['last' => $systemPolicy['last']]); + } + } + return null; } } diff --git a/src/app/User.php b/src/app/User.php --- a/src/app/User.php +++ b/src/app/User.php @@ -451,6 +451,16 @@ } /** + * Old passwords for this user. + * + * @return \Illuminate\Database\Eloquent\Relations\HasMany + */ + public function passwords() + { + return $this->hasMany('App\UserPassword'); + } + + /** * Return resources controlled by the current user. * * @param bool $with_accounts Include resources assigned to wallets diff --git a/src/app/UserPassword.php b/src/app/UserPassword.php new file mode 100644 --- /dev/null +++ b/src/app/UserPassword.php @@ -0,0 +1,37 @@ +belongsTo('\App\User', 'user_id', 'id'); + } +} diff --git a/src/database/migrations/2022_02_04_100000_create_user_passwords_table.php b/src/database/migrations/2022_02_04_100000_create_user_passwords_table.php new file mode 100644 --- /dev/null +++ b/src/database/migrations/2022_02_04_100000_create_user_passwords_table.php @@ -0,0 +1,42 @@ +bigIncrements('id'); + $table->bigInteger('user_id'); + $table->string('password'); + $table->timestamp('created_at')->useCurrent(); + + $table->index(['user_id', 'created_at']); + + $table->foreign('user_id')->references('id')->on('users') + ->onDelete('cascade')->onUpdate('cascade'); + } + ); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + Schema::dropIfExists('user_passwords'); + } +} diff --git a/src/resources/lang/en/app.php b/src/resources/lang/en/app.php --- a/src/resources/lang/en/app.php +++ b/src/resources/lang/en/app.php @@ -116,6 +116,7 @@ 'password-rule-upper' => 'Password contains an upper-case character', 'password-rule-digit' => 'Password contains a digit', 'password-rule-special' => 'Password contains a special character', + 'password-rule-last' => 'Password cannot be the same as the last :param passwords', 'wallet-notice-date' => 'With your current subscriptions your account balance will last until about :date (:days).', 'wallet-notice-nocredit' => 'You are out of credit, top up your balance now.', diff --git a/src/resources/lang/en/validation.php b/src/resources/lang/en/validation.php --- a/src/resources/lang/en/validation.php +++ b/src/resources/lang/en/validation.php @@ -149,6 +149,7 @@ 'invalid-password-policy' => 'Specified password policy is invalid.', 'password-policy-min-len-error' => 'Minimum password length cannot be less than :min.', 'password-policy-max-len-error' => 'Maximum password length cannot be more than :max.', + 'password-policy-last-error' => 'The minimum value for last N passwords is :last.', /* |-------------------------------------------------------------------------- diff --git a/src/resources/vue/Settings.vue b/src/resources/vue/Settings.vue --- a/src/resources/vue/Settings.vue +++ b/src/resources/vue/Settings.vue @@ -13,7 +13,8 @@ @@ -51,6 +52,19 @@ .catch(this.$root.errorHandler) }, methods: { + ruleLastHTML(rule) { + let parts = rule.name.split(/[0-9]+/) + let options = [1, 2, 3, 4, 5, 6] + + options = options.map(num => { + let selected = num == rule.param ? ' selected' : '' + return `` + }) + + return `` + }, submit() { this.$root.clearFormValidation($('#settings form')) @@ -58,10 +72,10 @@ $('#password_policy > li > input:checked').each((i, element) => { let entry = element.name - const input = $(element.parentNode).find('input[type=text]')[0] + let param = $(element.parentNode).find('select,input[type=text]').val() - if (input) { - entry += ':' + input.value + if (param) { + entry += ':' + param } password_policy.push(entry) diff --git a/src/tests/Browser/SettingsTest.php b/src/tests/Browser/SettingsTest.php --- a/src/tests/Browser/SettingsTest.php +++ b/src/tests/Browser/SettingsTest.php @@ -66,7 +66,7 @@ // Password policy ->assertSeeIn('@form .row:nth-child(1) > label', 'Password Policy') ->with('@form #password_policy', function (Browser $browser) { - $browser->assertElementsCount('li', 6) + $browser->assertElementsCount('li', 7) ->assertSeeIn('li:nth-child(1) label', 'Minimum password length') ->assertChecked('li:nth-child(1) input[type=checkbox]') ->assertValue('li:nth-child(1) input[type=text]', '5') @@ -85,6 +85,11 @@ ->assertSeeIn('li:nth-child(6) label', 'Password contains a special character') ->assertNotChecked('li:nth-child(6) input[type=checkbox]') ->assertMissing('li:nth-child(6) input[type=text]') + ->assertSeeIn('li:nth-child(7) label', 'Password cannot be the same as the last') + ->assertNotChecked('li:nth-child(7) input[type=checkbox]') + ->assertMissing('li:nth-child(7) input[type=text]') + ->assertSelected('li:nth-child(7) select', 3) + ->assertSelectHasOptions('li:nth-child(7) select', [1,2,3,4,5,6]) // Change the policy ->type('li:nth-child(1) input[type=text]', '11') ->type('li:nth-child(2) input[type=text]', '120') diff --git a/src/tests/Feature/Controller/PasswordPolicyTest.php b/src/tests/Feature/Controller/PasswordPolicyTest.php --- a/src/tests/Feature/Controller/PasswordPolicyTest.php +++ b/src/tests/Feature/Controller/PasswordPolicyTest.php @@ -85,8 +85,8 @@ $response->assertStatus(200); $this->assertCount(2, $json); - $this->assertSame(6, $json['count']); - $this->assertCount(6, $json['list']); + $this->assertSame(7, $json['count']); + $this->assertCount(7, $json['list']); $this->assertSame('Minimum password length: 8 characters', $json['list'][0]['name']); $this->assertSame('min', $json['list'][0]['label']); $this->assertSame('8', $json['list'][0]['param']); @@ -103,6 +103,8 @@ $this->assertSame(false, $json['list'][4]['enabled']); $this->assertSame('special', $json['list'][5]['label']); $this->assertSame(true, $json['list'][5]['enabled']); + $this->assertSame('last', $json['list'][6]['label']); + $this->assertSame(false, $json['list'][6]['enabled']); // Test acting as Jack $response = $this->actingAs($jack)->get('/api/v4/password-policy'); @@ -111,8 +113,8 @@ $response->assertStatus(200); $this->assertCount(2, $json); - $this->assertSame(6, $json['count']); - $this->assertCount(6, $json['list']); + $this->assertSame(7, $json['count']); + $this->assertCount(7, $json['list']); $this->assertSame('Minimum password length: 8 characters', $json['list'][0]['name']); $this->assertSame('min', $json['list'][0]['label']); $this->assertSame('8', $json['list'][0]['param']); @@ -129,5 +131,7 @@ $this->assertSame(false, $json['list'][4]['enabled']); $this->assertSame('special', $json['list'][5]['label']); $this->assertSame(true, $json['list'][5]['enabled']); + $this->assertSame('last', $json['list'][6]['label']); + $this->assertSame(false, $json['list'][6]['enabled']); } } diff --git a/src/tests/Feature/Controller/UsersTest.php b/src/tests/Feature/Controller/UsersTest.php --- a/src/tests/Feature/Controller/UsersTest.php +++ b/src/tests/Feature/Controller/UsersTest.php @@ -572,7 +572,7 @@ // Test some valid data, acting as another account controller $ned = $this->getTestUser('ned@kolab.org'); - $post = ['greylist_enabled' => 0, 'password_policy' => 'min:10,max:255,upper']; + $post = ['greylist_enabled' => 0, 'password_policy' => 'min:10,max:255,upper,last:1']; $response = $this->actingAs($ned)->post("/api/v4/users/{$john->id}/config", $post); $response->assertStatus(200); @@ -583,7 +583,7 @@ $this->assertSame('User settings updated successfully.', $json['message']); $this->assertSame('false', $john->fresh()->getSetting('greylist_enabled')); - $this->assertSame('min:10,max:255,upper', $john->fresh()->getSetting('password_policy')); + $this->assertSame('min:10,max:255,upper,last:1', $john->fresh()->getSetting('password_policy')); } /** diff --git a/src/tests/Feature/UserTest.php b/src/tests/Feature/UserTest.php --- a/src/tests/Feature/UserTest.php +++ b/src/tests/Feature/UserTest.php @@ -102,9 +102,59 @@ $this->assertTrue($userB->accounts()->get()[0]->id === $userA->wallets()->get()[0]->id); } + /** + * Test User::canDelete() method + */ public function testCanDelete(): void { - $this->markTestIncomplete(); + $john = $this->getTestUser('john@kolab.org'); + $ned = $this->getTestUser('ned@kolab.org'); + $jack = $this->getTestUser('jack@kolab.org'); + $reseller1 = $this->getTestUser('reseller@' . \config('app.domain')); + $admin = $this->getTestUser('jeroen@jeroen.jeroen'); + $domain = $this->getTestDomain('kolab.org'); + + // Admin + $this->assertTrue($admin->canDelete($admin)); + $this->assertFalse($admin->canDelete($john)); + $this->assertFalse($admin->canDelete($jack)); + $this->assertFalse($admin->canDelete($reseller1)); + $this->assertFalse($admin->canDelete($domain)); + $this->assertFalse($admin->canDelete($domain->wallet())); + + // Reseller - kolabnow + $this->assertFalse($reseller1->canDelete($john)); + $this->assertFalse($reseller1->canDelete($jack)); + $this->assertTrue($reseller1->canDelete($reseller1)); + $this->assertFalse($reseller1->canDelete($domain)); + $this->assertFalse($reseller1->canDelete($domain->wallet())); + $this->assertFalse($reseller1->canDelete($admin)); + + // Normal user - account owner + $this->assertTrue($john->canDelete($john)); + $this->assertTrue($john->canDelete($ned)); + $this->assertTrue($john->canDelete($jack)); + $this->assertTrue($john->canDelete($domain)); + $this->assertFalse($john->canDelete($domain->wallet())); + $this->assertFalse($john->canDelete($reseller1)); + $this->assertFalse($john->canDelete($admin)); + + // Normal user - a non-owner and non-controller + $this->assertFalse($jack->canDelete($jack)); + $this->assertFalse($jack->canDelete($john)); + $this->assertFalse($jack->canDelete($domain)); + $this->assertFalse($jack->canDelete($domain->wallet())); + $this->assertFalse($jack->canDelete($reseller1)); + $this->assertFalse($jack->canDelete($admin)); + + // Normal user - John's wallet controller + $this->assertTrue($ned->canDelete($ned)); + $this->assertTrue($ned->canDelete($john)); + $this->assertTrue($ned->canDelete($jack)); + $this->assertTrue($ned->canDelete($domain)); + $this->assertFalse($ned->canDelete($domain->wallet())); + $this->assertFalse($ned->canDelete($reseller1)); + $this->assertFalse($ned->canDelete($admin)); } /** @@ -248,33 +298,25 @@ } /** - * Test user create/creating observer + * Test user created/creating/updated observers */ - public function testCreate(): void + public function testCreateAndUpdate(): void { Queue::fake(); $domain = \config('app.domain'); - $user = User::create(['email' => 'USER-test@' . \strtoupper($domain)]); + $user = User::create([ + 'email' => 'USER-test@' . \strtoupper($domain), + 'password' => 'test', + ]); - $result = User::where('email', 'user-test@' . $domain)->first(); + $result = User::where('email', "user-test@$domain")->first(); - $this->assertSame('user-test@' . $domain, $result->email); + $this->assertSame("user-test@$domain", $result->email); $this->assertSame($user->id, $result->id); $this->assertSame(User::STATUS_NEW | User::STATUS_ACTIVE, $result->status); - } - - /** - * Verify user creation process - */ - public function testCreateJobs(): void - { - Queue::fake(); - - $user = User::create([ - 'email' => 'user-test@' . \config('app.domain') - ]); + $this->assertSame(0, $user->passwords()->count()); Queue::assertPushed(\App\Jobs\User\CreateJob::class, 1); Queue::assertPushed(\App\Jobs\PGP\KeyCreateJob::class, 0); @@ -311,20 +353,13 @@ && $userId === $user->id; }); */ - } - /** - * Verify user creation process invokes the PGP keys creation job (if configured) - */ - public function testCreatePGPJob(): void - { - Queue::fake(); + // Test invoking KeyCreateJob + $this->deleteTestUser("user-test@$domain"); \App\Tenant::find(\config('app.tenant_id'))->setSetting('pgp.enable', 1); - $user = User::create([ - 'email' => 'user-test@' . \config('app.domain') - ]); + $user = User::create(['email' => "user-test@$domain", 'password' => 'test']); Queue::assertPushed(\App\Jobs\PGP\KeyCreateJob::class, 1); @@ -338,6 +373,44 @@ && $userId === $user->id; } ); + + // Update the user, test the password change + $oldPassword = $user->password; + $user->password = 'test123'; + $user->save(); + + $this->assertNotEquals($oldPassword, $user->password); + $this->assertSame(0, $user->passwords()->count()); + + Queue::assertPushed(\App\Jobs\User\UpdateJob::class, 1); + Queue::assertPushed( + \App\Jobs\User\UpdateJob::class, + function ($job) use ($user) { + $userEmail = TestCase::getObjectProperty($job, 'userEmail'); + $userId = TestCase::getObjectProperty($job, 'userId'); + + return $userEmail === $user->email + && $userId === $user->id; + } + ); + + // Update the user, test the password history + $user->setSetting('password_policy', 'last:3'); + $oldPassword = $user->password; + $user->password = 'test1234'; + $user->save(); + + $this->assertSame(1, $user->passwords()->count()); + $this->assertSame($oldPassword, $user->passwords()->first()->password); + + $user->password = 'test12345'; + $user->save(); + $oldPassword = $user->password; + $user->password = 'test123456'; + $user->save(); + + $this->assertSame(2, $user->passwords()->count()); + $this->assertSame($oldPassword, $user->passwords()->latest()->first()->password); } /** diff --git a/src/tests/Unit/Rules/PasswordTest.php b/src/tests/Unit/Rules/PasswordTest.php --- a/src/tests/Unit/Rules/PasswordTest.php +++ b/src/tests/Unit/Rules/PasswordTest.php @@ -3,6 +3,7 @@ namespace Tests\Unit\Rules; use App\Rules\Password; +use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Validator; use Tests\TestCase; @@ -93,6 +94,52 @@ $this->assertSame(null, $result['digit']['param']); $this->assertSame(true, $result['digit']['enabled']); $this->assertSame(false, $result['digit']['status']); + + // Test password history check + $user = $this->getTestUser('john@kolab.org'); + $user->passwords()->delete(); + $user_pass = \App\Utils::generatePassphrase(); // should be the same plain password as John already has + + $pass = new Password(null, $user); + + \config(['app.password_policy' => 'min:5,last:1']); + + $result = $pass->check('abcd'); + + $this->assertCount(2, $result); + $this->assertSame('min', $result['min']['label']); + $this->assertSame('Minimum password length: 5 characters', $result['min']['name']); + $this->assertSame('5', $result['min']['param']); + $this->assertSame(true, $result['min']['enabled']); + $this->assertSame(false, $result['min']['status']); + + $this->assertSame('last', $result['last']['label']); + $this->assertSame('Password cannot be the same as the last 1 passwords', $result['last']['name']); + $this->assertSame('1', $result['last']['param']); + $this->assertSame(true, $result['last']['enabled']); + $this->assertSame(true, $result['last']['status']); + + $result = $pass->check($user_pass); + + $this->assertCount(2, $result); + $this->assertSame('last', $result['last']['label']); + $this->assertSame('Password cannot be the same as the last 1 passwords', $result['last']['name']); + $this->assertSame('1', $result['last']['param']); + $this->assertSame(true, $result['last']['enabled']); + $this->assertSame(false, $result['last']['status']); + + $user->passwords()->create(['password' => Hash::make('1234567891')]); + $user->passwords()->create(['password' => Hash::make('1234567890')]); + + $result = $pass->check('1234567890'); + + $this->assertSame(true, $result['last']['status']); + + \config(['app.password_policy' => 'min:5,last:3']); + + $result = $pass->check('1234567890'); + + $this->assertSame(false, $result['last']['status']); } /** @@ -123,7 +170,7 @@ // Expect to see all supported policy rules $result = $pass->rules(true); - $this->assertCount(6, $result); + $this->assertCount(7, $result); $this->assertSame('min', $result['min']['label']); $this->assertSame('Minimum password length: 10 characters', $result['min']['name']); $this->assertSame('10', $result['min']['param']); @@ -153,24 +200,29 @@ $this->assertSame('Password contains a special character', $result['special']['name']); $this->assertSame(null, $result['digit']['param']); $this->assertSame(false, $result['digit']['enabled']); + + $this->assertSame('last', $result['last']['label']); + $this->assertSame('Password cannot be the same as the last 3 passwords', $result['last']['name']); + $this->assertSame('3', $result['last']['param']); + $this->assertSame(false, $result['last']['enabled']); } /** * Validates the password using Laravel Validator API * * @param string $password The password to validate - * @param ?\App\User $user The account owner + * @param ?\App\User $owner The account owner * * @return ?string Validation error message on error, NULL otherwise */ - private function validate($password, $user = null): ?string + private function validate($password, $owner = null): ?string { // Instead of doing direct tests, we use validator to make sure // it works with the framework api $v = Validator::make( ['password' => $password], - ['password' => new Password($user)] + ['password' => new Password($owner)] ); if ($v->fails()) {