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

Add email link to set password. See issue #1205. #3262

Merged
merged 15 commits into from
Sep 11, 2024

Conversation

kiatng
Copy link
Contributor

@kiatng kiatng commented May 15, 2023

Trying to rebase PR #1283 to the main branch.

Fixed Issues (if relevant)

  1. Fixes Customer cannot see password in email when it is set in backend #1205

Description (*)

When a customer account is created in backed, the password should not be emailed in plaintext. This PR add a new email template, which is modifed from the welcome email, by adding a link to set password.

image

For existing account, the email template used is the same as the forgot-password email.

This PR does not change existing feature on emailing plaintext password from backend, but it'll show a warning:
image

Manual testing scenarios (*)

There are 2 scenarios:

Creating New Customer

  1. In backend, create a new customer with an email address which you own.
  2. When you receive the email, click on the link to set the password.
  3. Login with the new password.

Existing Customer

  1. In backend customer page, go to the customer you created above.
  2. Click on Email Link to Set Password.
  3. When you receive the email, click on the link to reset the password.
  4. Login with the new password.

Questions or comments

The landing page of the link is customer/account/changeforgotten with page title RESET PASSWORD. See screenshot. Does it need to be changed?

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

# Conflicts:
#	app/code/core/Mage/Adminhtml/controllers/CustomerController.php
#	app/code/core/Mage/Customer/Model/Customer.php
@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Customer Relates to Mage_Customer translations Relates to app/locale labels May 15, 2023
kiatng and others added 2 commits May 15, 2023 18:06
…ass.php

Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
@fballiano
Copy link
Contributor

I've tested but I've found a few problems:

  • when I set the password the first time (clicking the link in the email) it didn't work, the process finishes but I can't login with that password
  • since I can't reuse that same link to re-set the password I tried to re-click the "email link to set password" checkbox from the admin and save the customer record, but I didn't get a second email

@kiatng
Copy link
Contributor Author

kiatng commented May 17, 2023

@fballiano I tried to replicate your error:

[backend] Create a customer:

  1. associate the website to admin;
  2. Password Management > click Email Link to Set Password
  3. Save

In the email: click on the set password link, and set new password. I was not able to login.

In backend, customer page:

  1. F12 to bring out the developer panel
  2. Remove disable attribute in the dropdown element Associate to Website
  3. Change it to a website other than Admin.
  4. Save.

Now go to frontend, I was able to login with the password I set.

I also repeat the above but this time I created a customer associated to a website other than Admin. I was able to login with the set password.

@fballiano What steps did you do?

@fballiano
Copy link
Contributor

I've retested it now, so

  • backend, customer mask, open an existing customer
  • click the Email Link to Set Password
  • set the admin password
  • save and continue

the email is not sent, it doesn't enter the } elseif ($sendPassToEmail) { in customercontroller

@fballiano
Copy link
Contributor

I've also retested a new customer creation

  • backend, customers mask, create new customer
  • i leave everything as it is (also the admin website)
  • fill fistname, lastname, check the email link to set password
  • I receive the email with the link
  • click, set the password, save
  • trying to login the password doesn't work :-(

@kiatng
Copy link
Contributor Author

kiatng commented May 18, 2023

@fballiano Very strange, I retested, the email link worked for me, I managed to received the email.
image

When creating a customer in backend with associated website set to admin, it is true that the password set in either admin or email will not work in any frontend store (by definition, admin cannot be a store in frontend; password associated with admin will not work in frontend). But when the associated website of the customer is set to a frontend store, the password will work on that store. This behavior has nothing to do with this PR, it always been like this.

@fballiano
Copy link
Contributor

true, if I select the website then the password I set clicking the like works.

but if I re-enter the newly created customer and re-check the "send password link" then I don't get another email :-\ maybe it's my setup, maybe mailhog is somehow filtering the double email, dunno

@fballiano fballiano requested a review from addison74 June 7, 2023 11:42
@addison74 addison74 self-assigned this Jul 13, 2023
@fballiano
Copy link
Contributor

if somebody can test this we could try to merge it although my tests failed

@kiatng
Copy link
Contributor Author

kiatng commented Jul 22, 2023

@fballiano On the failure to login in frontend, is it because you associated the new customer to the default Admin website? Can you associate the customer to a frontend website? Then you should be able to login on that website with the password you set in backend.
image

@fballiano
Copy link
Contributor

@addison74 would you test this PR by @kiatng cause I have some problems and I'm not sure if it is only because of my environment :-)

@addison74
Copy link
Contributor

addison74 commented Feb 22, 2024

Indeed, there are logical issues related to this feature. Below you may find a simple test I did in the latest OpenMage without applying this PR.

If you create a new customer account in the Backend, the Associate to Website, Group, First Name, Last Name, Email and Password fields are mandatory. The password can be entered or auto-generated.

1 - No matter which password variant is chosen, after saving the customer, he does not receive any email, this is an big issue.

2 - If after saving the customer for the first time, one of the two password variant is used, surprisingly the customer receives an email with the following content

screenshot-1

There is no doubt the email contains wrong information. The customer is not the one who set the password and he cannot change the password in his account, because he doesn't have the password to access it.

My opinion is he must receive the password set up by the administrator even if it is in the plain-text format and for security reasons, the message he receives must contain a warning message and a link to set a new password ASAP.

In conclusion

  • the issue of why no email is sent to the customer when the form is saved for the first time must be investigated.

  • the email message must be changed by replacing the phrase "(the password you set when creating your account)" with the plain-text password. In addition, a security warning and a link to reset the password ASAP is mandatory.

After fixing these two issues, we can evaluate whether this PR is still requested or not.

@fballiano
Copy link
Contributor

I tried to retest but cannot make it work, I'll convert this PR to draft.

@fballiano fballiano marked this pull request as draft February 23, 2024 21:42
# Conflicts:
#	app/code/core/Mage/Customer/Model/Customer.php
@kiatng
Copy link
Contributor Author

kiatng commented Feb 27, 2024

There was a conflict, which is now resolved. Hopefully, this PR can now be tested.

[edit] 2 additional files: LICENSE.txt and LICENSE_AFL.txt were committed. I do not know why but these 2 files are constantly causing problems in all my local repositories.

@kiatng
Copy link
Contributor Author

kiatng commented Feb 27, 2024

@addison74 I think everything you said was said in PR #1205

  • The customer do not see the password in the email when an account is created by admin in backend
  • The email message was wrong

This PR fixes the above issues by taking the suggestions from the comments in PR #1205. Ref the screenshots in PR #1283,

  • To not send plaintext password in email: when an account is created by admin in backend, add the ability to send an email to ask the customer to set new password
  • Add a new email template for the above

This PR is now so old, even I could not remember the details.

@kiatng
Copy link
Contributor Author

kiatng commented Feb 27, 2024

@fballiano I edited the file LICENSE.txt directly in github by copying the entire text from here. Git still thinks the file is not the same as base. I have also tried this many times in my local repo, even though the text is copied from base, so there is no diff, but git thinks otherwise. Git must be right, but I do not know how to make the text the same as base. Any idea?

@addison74
Copy link
Contributor

@addison74 I think everything you said was said in PR #1205

  • The customer do not see the password in the email when an account is created by admin in backend
  • The email message was wrong

This PR fixes the above issues by taking the suggestions from the comments in PR #1205. Ref the screenshots in PR #1283,

  • To not send plaintext password in email: when an account is created by admin in backend, add the ability to send an email to ask the customer to set new password
  • Add a new email template for the above

This PR is now so old, even I could not remember the details.

@kiatng - I carefully read everything that was said about this problem. in addition, I added a feedback testing the OpenMage version without applying this PR. I will check again this PR and let you know my feedback

@addison74
Copy link
Contributor

@kiatng - When you have some time please give me a sign to finish this PR. It increases security level in OM. Just read my comments.

@kiatng
Copy link
Contributor Author

kiatng commented Jul 23, 2024

I will retest again as this has been a while.

@kiatng kiatng marked this pull request as ready for review July 24, 2024 03:53
@kiatng
Copy link
Contributor Author

kiatng commented Jul 24, 2024

During retest, I discovered a bug. I fixed it and retested. It's good for me now. I put it in production as well.

@addison74
Copy link
Contributor

Thank you for your time. in my case, I receive the message, visit the link and change the password, but I cannot log in with the new password. It could be an issue or not.

Do you receive a confirmation email after creating an account in Backend?

@kiatng kiatng marked this pull request as draft July 25, 2024 01:51
@kiatng
Copy link
Contributor Author

kiatng commented Jul 25, 2024

I will retest on the can't-login after password reset when I have the time, hopefully within next couple of weeks.

kiatng and others added 2 commits July 25, 2024 17:59
Co-authored-by: Sven Reichel <github-sr@hotmail.com>
Co-authored-by: Sven Reichel <github-sr@hotmail.com>
@addison74
Copy link
Contributor

@kiatng - I am able to log in, but please check it to confirm too. I did the test sending the link being logged in then logged out. In both cases it worked as expected. Also, I did not find any errors in logs.

We can go further and move it from Draft. I found issues related to this password changing but I will report each as issues.

@kiatng kiatng marked this pull request as ready for review July 28, 2024 02:45
@kiatng
Copy link
Contributor Author

kiatng commented Jul 28, 2024

I retested password reset multiple times, each time I can login with the new password. So, I think it is ready for review.

@kiatng kiatng merged commit 44b7e2d into OpenMage:main Sep 11, 2024
17 checks passed
@kiatng kiatng deleted the email_password_link branch September 11, 2024 06:02
@colinmollenhour
Copy link
Member

Nice work, but should this have gone in next? What is the impact for users that have customized templates stored in the database?

@sreichel
Copy link
Contributor

Mhhh? No template changed. A new one has been added.

I can't see how it could break anything. What do i miss?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Customer Relates to Mage_Customer translations Relates to app/locale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Customer cannot see password in email when it is set in backend
5 participants