Merge pull request #6971 from getkirby/enhancement/change-password-co…

…nfirm-current Require current password when user changes own pwd
getkirby · Feb 7, 2025 · 8d471ee · 8d471ee
2 parents 5879249 + 82e48ad
commit 8d471ee
Show file tree

Hide file tree

Showing 6 changed files with 80 additions and 21 deletions.
diff --git a/config/areas/users/dialogs.php b/config/areas/users/dialogs.php
@@ -3,6 +3,7 @@
 use Kirby\Cms\App;
 use Kirby\Cms\Find;
 use Kirby\Cms\UserRules;
+use Kirby\Exception\Exception;
 use Kirby\Exception\InvalidArgumentException;
 use Kirby\Panel\Field;
 use Kirby\Panel\Panel;
@@ -180,12 +181,15 @@
 	'user.changePassword' => [
 		'pattern' => 'users/(:any)/changePassword',
 		'load' => function (string $id) {
-			$user = Find::user($id);
+			Find::user($id);
 
 			return [
 				'component' => 'k-form-dialog',
 				'props' => [
-					'fields' => [
+					'fields'       => [
+						'currentPassword' => Field::password([
+							'label' => I18n::translate('user.changePassword.current'),
+						]),
 						'password' => Field::password([
 							'label' => I18n::translate('user.changePassword.new'),
 						]),
@@ -198,13 +202,26 @@
 			];
 		},
 		'submit' => function (string $id) {
-			$request = App::instance()->request();
+			$kirby   = App::instance();
+			$request = $kirby->request();
 
 			$user                 = Find::user($id);
+			$currentPassword      = $request->get('currentPassword');
 			$password             = $request->get('password');
 			$passwordConfirmation = $request->get('passwordConfirmation');
 
-			// validate the password
+			// validate the current password of the acting user
+			try {
+				$kirby->user()->validatePassword($currentPassword);
+			} catch (Exception) {
+				// catching and re-throwing exception to avoid automatic
+				// sign-out of current user from the Panel
+				throw new InvalidArgumentException([
+					'key' => 'user.password.wrong'
+				]);
+			}
+
+			// validate the new password
 			UserRules::validPassword($user, $password ?? '');
 
 			// compare passwords

diff --git a/i18n/translations/en.json b/i18n/translations/en.json
@@ -715,6 +715,7 @@
 	"user.changeLanguage": "Change language",
 	"user.changeName": "Rename this user",
 	"user.changePassword": "Change password",
+	"user.changePassword.current": "Your current password",
 	"user.changePassword.new": "New password",
 	"user.changePassword.new.confirm": "Confirm the new password…",
 	"user.changeRole": "Change role",

diff --git a/src/Cms/UserActions.php b/src/Cms/UserActions.php
@@ -96,6 +96,9 @@ public function changeName(string $name): static
 
 	/**
 	 * Changes the user password
+	 *
+	 * If this method is used with user input, it is recommended to also
+	 * confirm the current password by the user via `::validatePassword()`
 	 */
 	public function changePassword(
 		#[SensitiveParameter]

diff --git a/tests/Panel/Areas/AccountDialogsTest.php b/tests/Panel/Areas/AccountDialogsTest.php
@@ -97,6 +97,7 @@ public function testChangePassword(): void
 
 		$this->assertFormDialog($dialog);
 
+		$this->assertSame('Your current password', $props['fields']['currentPassword']['label']);
 		$this->assertSame('New password', $props['fields']['password']['label']);
 		$this->assertSame('Confirm the new password…', $props['fields']['passwordConfirmation']['label']);
 		$this->assertSame('Change', $props['submitButton']);
@@ -105,23 +106,39 @@ public function testChangePassword(): void
 	public function testChangePasswordOnSubmit(): void
 	{
 		$this->submit([
-			'password'             => '12345678',
-			'passwordConfirmation' => '12345678'
+			'currentPassword'      => '12345678',
+			'password'             => 'abcdefgh',
+			'passwordConfirmation' => 'abcdefgh'
 		]);
 
 		$dialog = $this->dialog('account/changePassword');
 
 		$this->assertSame('user.changePassword', $dialog['event']);
 		$this->assertSame(200, $dialog['code']);
 
-		$this->assertTrue($this->app->user('test')->validatePassword(12345678));
+		$this->assertTrue($this->app->user('test')->validatePassword('abcdefgh'));
+	}
+
+	public function testChangePasswordOnSubmitWithInvalidCurrentPassword(): void
+	{
+		$this->submit([
+			'currentPassword'      => '123456',
+			'password'             => 'abcdefgh',
+			'passwordConfirmation' => 'abcdefgh'
+		]);
+
+		$dialog = $this->dialog('account/changePassword');
+
+		$this->assertSame(400, $dialog['code']);
+		$this->assertSame('Wrong password', $dialog['error']);
 	}
 
 	public function testChangePasswordOnSubmitWithInvalidPassword(): void
 	{
 		$this->submit([
-			'password'             => '1234567',
-			'passwordConfirmation' => '1234567'
+			'currentPassword'      => '12345678',
+			'password'             => 'abcdef',
+			'passwordConfirmation' => 'abcdef'
 		]);
 
 		$dialog = $this->dialog('account/changePassword');
@@ -133,8 +150,9 @@ public function testChangePasswordOnSubmitWithInvalidPassword(): void
 	public function testChangePasswordOnSubmitWithInvalidConfirmation(): void
 	{
 		$this->submit([
-			'password'             => '12345678',
-			'passwordConfirmation' => '1234567'
+			'currentPassword'      => '12345678',
+			'password'             => 'abcdefgh',
+			'passwordConfirmation' => 'abcdefg'
 		]);
 
 		$dialog = $this->dialog('account/changePassword');

diff --git a/tests/Panel/Areas/AreaTestCase.php b/tests/Panel/Areas/AreaTestCase.php
@@ -4,6 +4,7 @@
 
 use Kirby\Cms\App;
 use Kirby\Cms\Blueprint;
+use Kirby\Cms\User;
 use Kirby\Filesystem\Dir;
 use Kirby\Http\Response;
 use Kirby\Panel\Panel;
@@ -82,9 +83,10 @@ public function install(): void
 		$this->app([
 			'users' => [
 				[
-					'id'    => 'test',
-					'email' => 'test@getkirby.com',
-					'role'  => 'admin',
+					'id'       => 'test',
+					'email'    => 'test@getkirby.com',
+					'role'     => 'admin',
+					'password' => User::hashPassword('12345678')
 				]
 			]
 		]);

diff --git a/tests/Panel/Areas/UsersDialogsTest.php b/tests/Panel/Areas/UsersDialogsTest.php
@@ -160,6 +160,7 @@ public function testChangePassword(): void
 
 		$this->assertFormDialog($dialog);
 
+		$this->assertSame('Your current password', $props['fields']['currentPassword']['label']);
 		$this->assertSame('New password', $props['fields']['password']['label']);
 		$this->assertSame('Confirm the new password…', $props['fields']['passwordConfirmation']['label']);
 		$this->assertSame('Change', $props['submitButton']);
@@ -168,23 +169,39 @@ public function testChangePassword(): void
 	public function testChangePasswordOnSubmit(): void
 	{
 		$this->submit([
-			'password'             => '12345678',
-			'passwordConfirmation' => '12345678'
+			'currentPassword'      => '12345678',
+			'password'             => 'abcdefgh',
+			'passwordConfirmation' => 'abcdefgh'
 		]);
 
 		$dialog = $this->dialog('users/test/changePassword');
 
 		$this->assertSame('user.changePassword', $dialog['event']);
 		$this->assertSame(200, $dialog['code']);
 
-		$this->assertTrue($this->app->user('test')->validatePassword(12345678));
+		$this->assertTrue($this->app->user('test')->validatePassword('abcdefgh'));
+	}
+
+	public function testChangePasswordOnSubmitWithInvalidCurrentPassword(): void
+	{
+		$this->submit([
+			'currentPassword'      => '123456',
+			'password'             => 'abcdefgh',
+			'passwordConfirmation' => 'abcdefgh'
+		]);
+
+		$dialog = $this->dialog('users/test/changePassword');
+
+		$this->assertSame(400, $dialog['code']);
+		$this->assertSame('Wrong password', $dialog['error']);
 	}
 
 	public function testChangePasswordOnSubmitWithInvalidPassword(): void
 	{
 		$this->submit([
-			'password'             => '1234567',
-			'passwordConfirmation' => '1234567'
+			'currentPassword'      => '12345678',
+			'password'             => 'abcdef',
+			'passwordConfirmation' => 'abcdef'
 		]);
 
 		$dialog = $this->dialog('users/test/changePassword');
@@ -196,8 +213,9 @@ public function testChangePasswordOnSubmitWithInvalidPassword(): void
 	public function testChangePasswordOnSubmitWithInvalidConfirmation(): void
 	{
 		$this->submit([
-			'password'             => '12345678',
-			'passwordConfirmation' => '1234567'
+			'currentPassword'      => '12345678',
+			'password'             => 'abcdefgh',
+			'passwordConfirmation' => 'abcdefg'
 		]);
 
 		$dialog = $this->dialog('users/test/changePassword');