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) {
+ $user->passwords()->create(['password' => $oldPassword]);
+ }
}
/**
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 for 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;
}
/**
@@ -61,44 +70,75 @@
$rules = $this->rules();
foreach ($rules as $name => $rule) {
+ $status = false;
+
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':
+ $status = strlen($password) > 0;
+
+ // Check if password is not the same as last X passwords
+ if ($status && $this->user && $rule['param']) {
+ $count = (int) $rule['param'];
+ // Current password
+ if ($count && $this->user->password) {
+ $count -= 1;
+ if (Hash::check($password, $this->user->password)) {
+ $status = false;
+ break;
+ }
+ }
+
+ // Passwords from the history
+ if ($count) {
+ $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
+ }
+ });
+ }
+ }
+
break;
default:
// Ignore unknown rule name
- $pass = true;
+ $status = true;
}
- $rules[$name]['status'] = $pass;
+ $rules[$name]['status'] = $status;
}
return $rules;
@@ -114,7 +154,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) {
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'])) {
+ $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
@@ -450,6 +450,16 @@
return $name;
}
+ /**
+ * 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.
*
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 input = $(element.parentNode).find('select,input[type=text]')[0]
if (input) {
- entry += ':' + input.value
+ entry += ':' + $(input).val()
}
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/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,24 @@
}
/**
- * 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')
- ]);
Queue::assertPushed(\App\Jobs\User\CreateJob::class, 1);
Queue::assertPushed(\App\Jobs\PGP\KeyCreateJob::class, 0);
@@ -311,20 +352,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 +372,27 @@
&& $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(1, $user->passwords()->count());
+ $this->assertSame($oldPassword, $user->passwords()->first()->password);
+
+ 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;
+ }
+ );
}
/**
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()) {