D3361.1775311795.diff
No OneTemporary
Actions

Authored By

Unknown

Size

30 KB

Referenced Files

None

Subscribers

None

D3361.1775311795.diff
View Options

	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 <count> 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 @@
	+<?php
	+
	+namespace App;
	+
	+use Illuminate\Database\Eloquent\Model;
	+
	+/**
	+ * A password history entry of a User.
	+ *
	+ * @property int $id
	+ * @property int $user_id
	+ * @property string $password
	+ */
	+class UserPassword extends Model
	+{
	+ /** @var bool Indicates if the model should be timestamped. */
	+ public $timestamps = false;
	+
	+ /** @var array The attributes that should be mutated to dates. */
	+ protected $dates = ['created_at'];
	+
	+ /** @var array The attributes that are mass assignable. */
	+ protected $fillable = ['user_id', 'password'];
	+
	+ /** @var array The attributes that should be hidden for arrays. */
	+ protected $hidden = ['password'];
	+
	+ /**
	+ * The user to which this entry belongs.
	+ *
	+ * @return \Illuminate\Database\Eloquent\Relations\BelongsTo
	+ */
	+ public function user()
	+ {
	+ return $this->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 @@
	+<?php
	+
	+use Illuminate\Support\Facades\Schema;
	+use Illuminate\Database\Schema\Blueprint;
	+use Illuminate\Database\Migrations\Migration;
	+
	+// phpcs:ignore
	+class CreateUserPasswordsTable extends Migration
	+{
	+ /**
	+ * Run the migrations.
	+ *
	+ * @return void
	+ */
	+ public function up()
	+ {
	+ Schema::create(
	+ 'user_passwords',
	+ function (Blueprint $table) {
	+ $table->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 @@
	<ul id="password_policy" class="list-group ms-1 mt-1">
	<li v-for="rule in passwordPolicy" :key="rule.label" class="list-group-item border-0 form-check pt-1 pb-1">
	<input type="checkbox" class="form-check-input" :id="'policy-' + rule.label" :name="rule.label" :checked="rule.enabled">
	- <label :for="'policy-' + rule.label" class="form-check-label pe-2">{{ rule.name.split(':')[0] }}</label>
	+ <span v-if="rule.label == 'last'" v-html="ruleLastHTML(rule)"></span>
	+ <label v-else :for="'policy-' + rule.label" class="form-check-label pe-2">{{ rule.name.split(':')[0] }}</label>
	<input type="text" class="form-control form-control-sm w-auto d-inline" v-if="['min', 'max'].includes(rule.label)" :value="rule.param" size="3">
	</li>
	</ul>
	@@ -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 `<option value="${num}"${selected}>${num}</option>`
	+ })
	+
	+ return `<label for="policy-last" class="form-check-label pe-2">
	+ ${parts[0]} <select class="form-select form-select-sm d-inline w-auto">${options.join('')}</select> ${parts[1]}
	+ </label>`
	+ },
	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()) {

File Metadata

Mime Type: text/plain
Expires: Sat, Apr 4, 2:09 PM (21 h, 40 m ago)
Storage Engine: blob
Storage Format: Raw Data
Storage Handle: 18829861
Default Alt Text: D3361.1775311795.diff (30 KB)

D3361.1775311795.diffNo OneTemporaryActions

D3361.1775311795.diffView Options

File Metadata

Event Timeline

D3361.1775311795.diff
No OneTemporary
Actions

D3361.1775311795.diff
View Options