Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Password #602

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open

Change Password #602

wants to merge 34 commits into from

Conversation

rossaddison
Copy link
Contributor

Q A
Is bugfix?
New feature? ✔️
Breaks BC?
Fixed issues None

@what-the-diff
Copy link

what-the-diff bot commented Oct 28, 2023

PR Summary

  • Introduction of README File
    A new README.md file has been added to the root directory. This file contains a description of the demo application.

  • Update on Blog README
    The 'blog/README.md' file has been updated to include instructions for installing Bootstrap Node Modules while using WAMP. Furthermore, improvements have been made to the "Support the Project" section.

  • New Route Addition
    A new route has been added to the 'blog/config/common/routes/routes.php' file. This route is for the ResetController and has the URL /reset.

  • Localization Updates
    In the blog/resources/messages/en/app.php file, translations for 'layout.password-verify.new' and 'layout.password.new' have been added. This update also includes the new translation for 'menu.password.change'.

  • Addition of New Menu Item
    A new menu item for changing the password has been put in the blog/resources/views/layout/main.php file. This menu item links to the URL /auth/reset.

  • Password Reset Functionality
    Several new files have been introduced to handle password reset functionality. These include a file to display the password reset form (blog/resources/views/reset/reset.php), a file to control the password reset operation (blog/src/Auth/Controller/ResetController.php), and a file to handle the reset form and its validation (blog/src/Auth/Form/ResetForm.php).

  • Enhanced Login Protocol
    The Identity class has been improved with new methods and logic for managing login keys using cookies.

  • New Identity Repository Functionality
    A new file has been added as blog/src/Auth/IdentityRepository.php which is responsible for handling identity repository operations.

blog/README.md Outdated
@@ -67,6 +67,11 @@ The code is statically analyzed with [Psalm](https://psalm.dev/). To run static
./vendor/bin/psalm
```

### Installing Bootstrap Node Modules with WAMP

1. Make a directory node_modules in the blog directory.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't npm creating node_modules automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing Demo Installation:
Step 1: Downloaded zip and extracted into demo2 folder
Step 2: composer update
Step 3: c:\wamp64\www\demo2\blog>yii serve

image

Step 4: Whilst running wampserver in url typed: 127.0.0.1:8080
image

Step 5: Removed blog from .env file path to remove above error.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

blog/resources/views/layout/main.php Show resolved Hide resolved
blog/resources/views/reset/reset.php Outdated Show resolved Hide resolved
blog/resources/views/reset/reset.php Outdated Show resolved Hide resolved
blog/src/Auth/Form/ResetForm.php Outdated Show resolved Hide resolved
blog/src/Auth/Form/ResetForm.php Outdated Show resolved Hide resolved
blog/src/Auth/Form/ResetForm.php Outdated Show resolved Hide resolved
blog/src/Auth/Form/ResetForm.php Outdated Show resolved Hide resolved
@vjik vjik added the status:under development Someone is working on a pull request. label Nov 10, 2023
rossaddison and others added 4 commits November 15, 2023 14:38
Co-authored-by: Alexander Makarov <sam@rmcreative.ru>
Co-authored-by: Alexander Makarov <sam@rmcreative.ru>
Co-authored-by: Alexander Makarov <sam@rmcreative.ru>
Functionally a change password and not a reset password form that is included in the main layout and not on the login form.

(Change Password)[yiisoft#602]
rossaddison added a commit to rossaddison/invoice that referenced this pull request Nov 18, 2023
Adopt suggestions in (#602)[yiisoft/demo#602]
blog/config/common/routes/routes.php Outdated Show resolved Hide resolved
blog/src/Auth/Controller/ChangeController.php Outdated Show resolved Hide resolved
@rossaddison
Copy link
Contributor Author

Please review these changes

Copy link
Member

@vjik vjik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added several comments, fix several errors and adapt PR to last changes in Yii Form.

blog/src/Auth/Controller/ChangePasswordController.php Outdated Show resolved Hide resolved
{
private string $login = '';
private string $password = '';
private string $passwordVerify = '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private string $passwordVerify = '';
private string $passwordVerify = '';

Not need field for duplicate old password.

@samdark
Copy link
Member

samdark commented Apr 8, 2024

@rossaddison would you please resolve conflicts? Thanks.

@rossaddison rossaddison requested a review from samdark April 9, 2024 23:14
blog/resources/messages/en/app.php Outdated Show resolved Hide resolved
return $this->redirectToMain();
}
// permit an authenticated user, ie. not a guest, only and null!== current user
if (!$authService->isGuest()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (!$authService->isGuest()) { already not need, we have this check above.

In the same way, you can remove "if" nesting in further code.

->id('changeForm')
->open() ?>

<?= Field::text($formModel, 'login')->addInputAttributes(['value'=> $login ?? '', 'readonly'=>'readonly']) ?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want add ability to change password for any user? Not only current?

Copy link
Contributor Author

@rossaddison rossaddison May 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I will need to add a permission to the readonly => readonly e.g.

!$this->currentUser->can('changePasswordForAnyUser',[]) ?  'readonly'=>'readonly' :  '';

This will make sure that only administrators with the canChangePasswordForAnyUser permission can insert a username in the login textbox in place of the {login} in order to change the password.

...And remove the

if ($this->currentUser->can('viewInv',[])) {

since all authenticated users with a lesser permission (of viewInv) should be able to change their password.

changepassword.php

<?= $canChangePasswordForAnyUser ? Field::text($formModel, 'login')->addInputAttributes(['value'=> $login ?? '']) 
                                                     : Field::text($formModel, 'login')->addInputAttributes(['value'=> $login ?? '', 'readonly'=>'readonly']); ?>

Controller.php

public function change(
      AuthService $authService,
      Identity $identity,
      IdentityRepository $identityRepository,
      ServerRequestInterface $request,
      FormHydrator $formHydrator,
      ChangePasswordForm $changePasswordForm
    ): ResponseInterface {
      if ($authService->isGuest()) {
          return $this->redirectToMain();
      }
      // readonly the login detail on the change form
      $identity_id = $this->currentUser->getIdentity()->getId();
      if (null!==$identity_id) {
        $identity = $identityRepository->findIdentity($identity_id);
        if (null!==$identity) {
          // Identity and User are in a HasOne relationship so no null value
          $login = $identity->getUser()?->getLogin();
          if ($request->getMethod() === Method::POST
            && $formHydrator->populate($changePasswordForm, $request->getParsedBody())
            && $changePasswordForm->change() 
          ) {
            // Identity implements CookieLoginIdentityInterface: ensure the regeneration of the cookie auth key by means of $authService->logout();
            // @see vendor\yiisoft\user\src\Login\Cookie\CookieLoginIdentityInterface 
            // Specific note: "Make sure to invalidate earlier issued keys when you implement force user logout,
            // PASSWORD CHANGE and other scenarios, that require forceful access revocation for old sessions.
            // The authService logout function will regenerate the auth key here => overwriting any auth key
            $authService->logout();
            $this->flash_message('success', $this->translator->translate('validator.password.change'));
            return $this->redirectToMain();
          }
          return $this->viewRenderer->render('change', 
                  [
                      'formModel' => $changePasswordForm, 
                      'login' => $login,
                      'canChangePasswordForAnyUser' => $this->currentUser->can('changePasswordForAnyUser')    
                  ]);
        } // identity
      } // identity_id 
    } // r

}
// permit an authenticated user, ie. not a guest, only and null!== current user
if (!$authService->isGuest()) {
if ($this->current_user->can('viewInv',[])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

viewInv isn't a good name for permission. Let's not shorten names.

rossaddison and others added 2 commits April 19, 2024 22:23
Co-authored-by: Sergei Predvoditelev <sergey.predvoditelev@gmail.com>
rossaddison added a commit to rossaddison/invoice that referenced this pull request May 6, 2024
If a user (normally an administrator) has the permission 'changePasswordForAnyUser', the login form 'username' text box will be not readonly so that the administrator can change any username's password in the event of a user's personal request to change their password by typing in their username.
Comment on lines +62 to +63
"rossaddison/mailer": "dev-master",
"rossaddison/mailer-symfony": "*",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rossaddison you can link your packages locally with yiisoft/yii-dev-tool
Take a look at it

Comment on lines +94 to +95
"rossaddison/yii-swagger": "dev-master",
"rossaddison/yii-view": "*"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Comment on lines +14 to +16
* @var array $params['yiisoft/aliases']
* @var array $params['yiisoft/aliases']['aliases']
* @var string $params['yiisoft/aliases']['aliases']['@root']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that work in PhpStorm? Elsewhere?

Comment on lines +29 to 31
'reset' => function (array $defaultArguments = []) {
$defaultArguments = ['_language', 'en'];
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks suspicious. What does it reset?

Comment on lines -35 to -39
->addGroup(
Group::create('/{_language}')->routes(...$config->get('app-routes')),
)
->addGroup(
Group::create()->routes(...$config->get('routes')),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was that created to exclude debug routes from language routing?

Comment on lines +32 to +36
$this->currentUser = $currentUser;
$this->session = $session;
$this->flash = new Flash($session);
$this->translator = $translator;
$this->viewRenderer = $viewRenderer->withControllerName('changepassword');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$this->currentUser = $currentUser;
$this->session = $session;
$this->flash = new Flash($session);
$this->translator = $translator;
$this->viewRenderer = $viewRenderer->withControllerName('changepassword');
$this->flash = new Flash($session);
$this->viewRenderer = $viewRenderer->withControllerName('changepassword');

return $this->redirectToMain();
}

$identity_id = $this->currentUser->getIdentity()->getId();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$identity_id = $this->currentUser->getIdentity()->getId();
$identityId = $this->currentUser->getIdentity()->getId();

// PASSWORD CHANGE and other scenarios, that require forceful access revocation for old sessions.
// The authService logout function will regenerate the auth key here => overwriting any auth key
$authService->logout();
$this->flash_message('success', $this->translator->translate('validator.password.change'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$this->flash_message('success', $this->translator->translate('validator.password.change'));
$this->flashMessage('success', $this->translator->translate('validator.password.change'));

Comment on lines +99 to +106
function (mixed $value): Result {
$result = new Result();
if ($this->userRepository->findByLogin((string)$value) == null) {
$result->addError($this->translator->translate('validator.user.exist.not'));
}
return $result;
},
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, that should be implemented by catching a database exception on inserting a record because SELECT and then INSERT aren't atomic and, thus, can cause an exception anyway if a value is inserted by another user in between.

'password' => $this->PasswordRules(),
'newPassword' => [
new Required(),
new Length(min: 8),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd not limit password length because it weakens security overall.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:under development Someone is working on a pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants